fix(Google Login): Prevents uninvited users gaining access (#1517 - LLC-10)

crazy-grizzly · web-flow · commit 8d0e78be7f95 · 2020-03-20T12:05:12.000Z
#1517 https://learningpool.atlassian.net/browse/LLC-10
diff --git 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<any>}
+ * @returns {Promise<string>}
  */
 const createJWT = ({
   userId,
@@ -84,7 +84,7 @@ const createUserTokenPayload = (user, provider) => payloadDefaults({
 /**
  * @param {*} user
  * @param {*} provider
- * @returns {Promise<any>}
+ * @returns {Promise<string>}
  */
 const createUserJWT = (user, provider) =>
   createJWT(
@@ -103,7 +103,7 @@ const createUserRefreshTokenPayload = (user, provider) => payloadDefaults({
 /**
  * @param {*} user
  * @param {*} provider
- * @returns {Promise<any>}
+ * @returns {Promise<string>}
  */
 const createUserRefreshJWT = (user, provider) =>
   createJWT(
@@ -150,7 +150,7 @@ const createOrgRefreshTokenPayload = (user, organisationId, provider) => payload
  * @param {*} user
  * @param {*} organisationId
  * @param {*} provider
- * @returns {Promise<any>}
+ * @returns {Promise<string>}
  */
 const createOrgRefreshJWT = (user, organisationId, provider) =>
   createJWT(
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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<User>} 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
@@ -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
@@ -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 }) => {
       <Helmet {...config.app.head} />
       {renderPage(name)}
       <Toasts />
+      <SaveBarErrors />
     </div>
   );
 };
diff --git 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
@@ -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
@@ -81,26 +81,73 @@ 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) {
       // cross origin errors will be thrown trying to
       // 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<string>}
+ */
 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
