From 8d0e78be7f95d8f48786a228927b241b64ccd00e Mon Sep 17 00:00:00 2001 From: Vadim Date: Fri, 20 Mar 2020 14:05:12 +0200 Subject: [PATCH] fix(Google Login): Prevents uninvited users gaining access (LearningLocker/learninglocker#1517 - LLC-10) https://github.com/LearningLocker/learninglocker/pull/1517 https://learningpool.atlassian.net/browse/LLC-10 --- api/src/auth/jwt.js | 8 ++-- api/src/auth/passport.js | 26 +++++++--- api/src/controllers/AuthController.js | 15 ++++-- api/src/routes/HttpRoutes.js | 27 ++++++++++- lib/constants/auth.js | 4 +- lib/constants/routes.js | 1 + lib/models/user.js | 69 +++++++++++++++++++++++++++ package.json | 1 + ui/src/containers/App/index.js | 2 + ui/src/pages/LoginPage/index.js | 15 +++++- ui/src/utils/createAsyncDuck.js | 2 +- ui/src/utils/oauth.js | 63 ++++++++++++++++++++---- yarn.lock | 5 ++ 13 files changed, 208 insertions(+), 30 deletions(-) diff --git a/api/src/auth/jwt.js b/api/src/auth/jwt.js index 16ec2885c0..326883fb32 100644 --- a/api/src/auth/jwt.js +++ b/api/src/auth/jwt.js @@ -53,7 +53,7 @@ const payloadDefaults = ({ /** * @param {*} - payload * @param {*} opts - (optional) - * @returns {Promise} + * @returns {Promise} */ const createJWT = ({ userId, @@ -84,7 +84,7 @@ const createUserTokenPayload = (user, provider) => payloadDefaults({ /** * @param {*} user * @param {*} provider - * @returns {Promise} + * @returns {Promise} */ const createUserJWT = (user, provider) => createJWT( @@ -103,7 +103,7 @@ const createUserRefreshTokenPayload = (user, provider) => payloadDefaults({ /** * @param {*} user * @param {*} provider - * @returns {Promise} + * @returns {Promise} */ const createUserRefreshJWT = (user, provider) => createJWT( @@ -150,7 +150,7 @@ const createOrgRefreshTokenPayload = (user, organisationId, provider) => payload * @param {*} user * @param {*} organisationId * @param {*} provider - * @returns {Promise} + * @returns {Promise} */ const createOrgRefreshJWT = (user, organisationId, provider) => createJWT( diff --git a/api/src/auth/passport.js b/api/src/auth/passport.js index 91c74fc163..a28be49903 100644 --- a/api/src/auth/passport.js +++ b/api/src/auth/passport.js @@ -212,13 +212,25 @@ if ( profile.emails, email => email.verified === true ); - User.findOrCreate({ email: userEmail.value }, (err, user) => { - assert.ifError(err); - user.googleId = profile.id; - user.imageUrl = get(profile, 'photos.0.value'); - user.name = profile.displayName; - user.save((err, savedUser) => done(err, savedUser)); - }); + + User.findOne( + { + email: userEmail.value + }, + (err, user) => { + assert.ifError(err); + + if (!user) { + return done(null, false, { message: 'User does not exist' }); + } + + user.googleId = profile.id; + user.imageUrl = get(profile, 'photos.0.value'); + user.name = profile.displayName; + + user.save((err, savedUser) => done(err, savedUser)); + }, + ); } ) ); diff --git a/api/src/controllers/AuthController.js b/api/src/controllers/AuthController.js index 6a643e44d9..ccd27b7f41 100644 --- a/api/src/controllers/AuthController.js +++ b/api/src/controllers/AuthController.js @@ -2,6 +2,7 @@ import passport from 'passport'; import jsonwebtoken from 'jsonwebtoken'; import { v4 as uuid } from 'uuid'; import ms from 'ms'; +import status from 'http-status'; import logger from 'lib/logger'; import User from 'lib/models/user'; import OAuthToken from 'lib/models/oAuthToken'; @@ -261,12 +262,18 @@ const jwtOrganisation = (req, res) => { } }; -const googleSuccess = (req, res) => { +const googleSuccess = (user, response) => { // we have successfully signed into google // create a JWT and set it in the query params (only way to return it with a redirect) - createUserJWT(req.user, 'google') - .then(token => res.redirect(`/api${AUTH_JWT_SUCCESS}?access_token=${token}`)) - .catch(err => res.status(500).send(err)); + createUserJWT(user, 'google') + .then(token => response.redirect(`/api${AUTH_JWT_SUCCESS}?access_token=${token}`)) + .catch( + (err) => { + response + .status(status.INTERNAL_SERVER_ERROR) + .send(err); + } + ); }; const clientInfo = (req, res) => { diff --git a/api/src/routes/HttpRoutes.js b/api/src/routes/HttpRoutes.js index 5d0bafed9c..1a9dc37f84 100644 --- a/api/src/routes/HttpRoutes.js +++ b/api/src/routes/HttpRoutes.js @@ -117,6 +117,13 @@ router.get( AuthController.clientInfo ); +router.get( + routes.OAUTH2_FAILED, + (request, response) => { + response.send('Authorization failed'); + }, +); + router.post( routes.OAUTH2_TOKEN, AuthController.issueOAuth2AccessToken @@ -136,10 +143,26 @@ if (process.env.GOOGLE_ENABLED) { routes.AUTH_JWT_GOOGLE, passport.authenticate('google', GOOGLE_AUTH_OPTIONS) ); + router.get( routes.AUTH_JWT_GOOGLE_CALLBACK, - passport.authenticate('google', DEFAULT_PASSPORT_OPTIONS), - AuthController.googleSuccess + (request, response, next) => { + passport.authenticate( + 'google', + DEFAULT_PASSPORT_OPTIONS, + (error, user, info) => { + const defaultErrorMessage = 'Something bad happened'; + + if (!user) { + response.redirect(`/api${routes.OAUTH2_FAILED}?error=${get(info, 'message', defaultErrorMessage)}`); + + return; + } + + AuthController.googleSuccess(user, response); + }, + )(request, response, next); + }, ); } diff --git a/lib/constants/auth.js b/lib/constants/auth.js index 072cef0c18..a53d2e6308 100644 --- a/lib/constants/auth.js +++ b/lib/constants/auth.js @@ -19,8 +19,8 @@ import getAuthFromRequest from 'lib/helpers/getAuthFromRequest'; export const GOOGLE_AUTH_OPTIONS = { scope: ['https://www.googleapis.com/auth/plus.login', 'email'], - approvalPrompt: 'auto', - session: false + session: false, + prompt: 'select_account' }; export const DEFAULT_PASSPORT_OPTIONS = { diff --git a/lib/constants/routes.js b/lib/constants/routes.js index 17e2fb46cf..c242472471 100644 --- a/lib/constants/routes.js +++ b/lib/constants/routes.js @@ -15,6 +15,7 @@ export const AUTH_JWT_SUCCESS = '/auth/jwt/success'; export const AUTH_CLIENT_INFO = '/auth/client/info'; export const OAUTH2_TOKEN = '/oauth2/token'; +export const OAUTH2_FAILED = '/oauth2/failed'; export const SENDSMS = '/sendsms'; diff --git a/lib/models/user.js b/lib/models/user.js index 5a8f4c0c67..75e828ae89 100644 --- a/lib/models/user.js +++ b/lib/models/user.js @@ -17,6 +17,73 @@ import getOrgFromAuthInfo from 'lib/services/auth/authInfoSelectors/getOrgFromAu import getUserIdFromAuthInfo from 'lib/services/auth/authInfoSelectors/getUserIdFromAuthInfo'; import { validatePasswordUtil } from 'lib/utils/validators/User'; +/** + * @typedef {object} PasswordHistoryItem + * @property {string} hash + * @property {Date} date + */ + +/** + * @typedef {object} ResetTokensItem + * @property {string} token + * @property {Date} expires + */ + +/** + * @typedef {object} UserSettings + * @property {boolean} CONFIRM_BEFORE_DELETE + */ + +/** + * @typedef {object} OrganisationSettings + * @property {object} organisation - TODO: define type + * @property {string[]} scopes + * @property {object[]} roles - TODO: define type + * @property {string} filter + * @property {string} oldFilter + * @property {string} timezone + */ + +/** + * @typedef {object} OwnerOrganisationSettings + * @property {boolean} LOCKOUT_ENABLED + * @property {number} LOCKOUT_ATTEMPS + * @property {number} LOCKOUT_SECONDS + * @property {boolean} PASSWORD_HISTORY_CHECK + * @property {number} PASSWORD_HISTORY_TOTAL + * @property {number} PASSWORD_MIN_LENGTH + * @property {boolean} PASSWORD_REQUIRE_ALPHA + * @property {boolean} PASSWORD_REQUIRE_NUMBER + * @property {boolean} PASSWORD_USE_CUSTOM_REGEX + * @property {string} PASSWORD_CUSTOM_REGEX + * @property {string} PASSWORD_CUSTOM_MESSAGE + */ + +/** + * Plain object structure without mongoose model methods + * @typedef {object} User + * @property {string} name + * @property {string} email + * @property {object[]} organisations - TODO: define type + * @property {OrganisationSettings[]} organisationSettings + * @property {string} imageUrl + * @property {string} googleId + * @property {string} password + * @property {object} ownerOrganisation - TODO: define type + * @property {OwnerOrganisationSettings} ownerOrganisationSettings + * @property {UserSettings} settings + * @property {string[]} scopes + * @property {boolean} verified + * @property {ResetTokensItem[]} resetTokens + * @property {PasswordHistoryItem[]} passwordHistory + * @property {Date} authLastAttempt + * @property {number} authFailedAttempts + * @property {Date} authLockoutExpiry + * @property {boolean} hasBeenMigrated + */ + +/** @typedef {module:mongoose.Model} UserModel */ + /** * @param {string} value * @returns {true} if validation is success @@ -63,6 +130,7 @@ async function validatePassword(value) { } // TODO: Remove `organisationSettings.oldFilter` and `hasBeenMigrated` after we confirm success of $in migration +/** @class UserSchema */ const schema = new mongoose.Schema({ name: { type: String }, email: { @@ -110,6 +178,7 @@ const schema = new mongoose.Schema({ // "owned" users when the organisation's settings are updated ownerOrganisationSettings: { LOCKOUT_ENABLED: { type: Boolean, default: true }, + // TODO: fix typo 🤨 LOCKOUT_ATTEMPS: { type: Number, default: 5 }, // number of attempts before lock out LOCKOUT_SECONDS: { type: Number, default: 1800 }, // 30 minute lock out period diff --git a/package.json b/package.json index 1b9cc49112..c2d37d42e1 100644 --- a/package.json +++ b/package.json @@ -87,6 +87,7 @@ "highland": "^2.8.1", "html-to-text": "^2.1.0", "http-proxy": "^1.12.0", + "http-status": "^1.4.2", "immutable": "^3.8.1", "ioredis": "^3.2.2", "js-cookie": "^2.1.3", diff --git a/ui/src/containers/App/index.js b/ui/src/containers/App/index.js index fe769b3f4f..3841d816d3 100644 --- a/ui/src/containers/App/index.js +++ b/ui/src/containers/App/index.js @@ -10,6 +10,7 @@ import { routeNodeSelector } from 'redux-router5'; import { startsWithSegment } from 'router5.helpers'; import get from 'lodash/get'; import createAsyncComponent from 'ui/utils/createAsyncComponent'; +import SaveBarErrors from 'ui/containers/SaveBarErrors'; const renderPage = (routeName) => { const testRoute = startsWithSegment(routeName); @@ -62,6 +63,7 @@ const component = ({ route }) => { {renderPage(name)} + ); }; diff --git a/ui/src/pages/LoginPage/index.js b/ui/src/pages/LoginPage/index.js index a43fe27a1b..98cbe6ddaf 100644 --- a/ui/src/pages/LoginPage/index.js +++ b/ui/src/pages/LoginPage/index.js @@ -35,6 +35,14 @@ const enhance = compose( }) ); +/** + * @param oAuthLoginStart - {@see reduxOAuthLoginStart} + * @param loginStart - {@see reduxLoginStart} + * @param loginRequestState - {@see loginRequestStateSelector} + * @param loginError - {@see loginErrorSelector} + * @param googleAuth - {@see getAppDataSelector} + * @returns {*} + */ const render = ({ oAuthLoginStart, loginStart, @@ -52,8 +60,11 @@ const render = ({ if (email && password) loginStart({ username: email, password }).catch(() => { }); }; - const onClickOAuthLogin = (e) => { - if (e) e.preventDefault(); + const onClickOAuthLogin = (event) => { + if (event) { + event.preventDefault(); + } + oAuthLoginStart('google').catch(() => { }); }; diff --git a/ui/src/utils/createAsyncDuck.js b/ui/src/utils/createAsyncDuck.js index f9784951ba..160f897270 100644 --- a/ui/src/utils/createAsyncDuck.js +++ b/ui/src/utils/createAsyncDuck.js @@ -82,7 +82,7 @@ export default function createAsyncDuck({ yield put(actions.complete(args)); } } catch (err) { - yield put(actions.failure({ ...args, message: err.message })); + yield put(actions.failure({ ...args, message: err.message || '' })); yield put(alert({ ...args, message: err.message, diff --git a/ui/src/utils/oauth.js b/ui/src/utils/oauth.js index ea6bbc1605..d56b483157 100644 --- a/ui/src/utils/oauth.js +++ b/ui/src/utils/oauth.js @@ -81,10 +81,32 @@ export function openPopup(provider) { return window.open(getEndpoint(provider), provider, `${settings},${getPopupDimensions(provider)}`); } +/** + * @typedef {object} OauthError + * @property {string} message + */ + +/** + * @callback checkForToken~tokenResolve + * @param {string} token + */ + +/** + * @callback checkForToken~tokenReject + * @param {OauthError} error + */ + +/** + * @param {Window} popup + * @param {checkForToken~tokenResolve} resolve + * @param {checkForToken~tokenReject} reject + */ function checkForToken(popup, resolve, reject) { - if (popup.closed) reject({ errors: 'Authentication was cancelled.' }); - else { + if (popup.closed) { + reject({ message: 'Authentication was cancelled' }); + } else { let parsed; + try { parsed = url.parse(popup.location.href, true); } catch (e) { @@ -92,15 +114,40 @@ function checkForToken(popup, resolve, reject) { // access the popup when it is on the third party site } - if (_.has(parsed, 'query.access_token')) { + const accessToken = _.get(parsed, 'query.access_token'); + const error = _.get(parsed, 'query.error'); + + if (accessToken || error) { popup.close(); - resolve(_.get(parsed, 'query.access_token')); - } else setTimeout(checkForToken.bind(null, popup, resolve, reject), 0); + + if (error) { + reject({ message: error }); + + return; + } + + resolve(accessToken); + } else { + setTimeout( + checkForToken.bind(null, popup, resolve, reject), + 0, + ); + } } } +/** + * @param {Window} popup + * @returns {Promise} + */ export function listenForToken(popup) { - return new Promise((resolve, reject) => { - checkForToken(popup, resolve, reject); - }); + return new Promise( + /** + * @param {checkForToken~tokenResolve} resolve + * @param {checkForToken~tokenReject} reject + */ + (resolve, reject) => { + checkForToken(popup, resolve, reject); + } + ); } diff --git a/yarn.lock b/yarn.lock index 1eda922b50..d1055c512d 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6761,6 +6761,11 @@ http-status-codes@^1.3.0: version "1.4.0" resolved "https://registry.yarnpkg.com/http-status-codes/-/http-status-codes-1.4.0.tgz#6e4c15d16ff3a9e2df03b89f3a55e1aae05fb477" +http-status@^1.4.2: + version "1.4.2" + resolved "https://registry.yarnpkg.com/http-status/-/http-status-1.4.2.tgz#75406e829dca9bfdf92972c579b47cd6a58ab6c8" + integrity sha512-mBnIohUwRw9NyXMEMMv8/GANnzEYUj0Y8d3uL01zDWFkxUjYyZ6rgCaAI2zZ1Wb34Oqtbx/nFZolPRDc8Xlm5A== + httpntlm@1.6.1: version "1.6.1" resolved "https://registry.yarnpkg.com/httpntlm/-/httpntlm-1.6.1.tgz#ad01527143a2e8773cfae6a96f58656bb52a34b2"