Merge pull request #2016 from overleaf/sk-fix-session-data

Fix mismatched user session data

GitOrigin-RevId: d464d05431ac86e279109aa3f7bc26dcf76662f4

services/web/app/src/Features/Authentication/AuthenticationController.js

@@ -1,12 +1,3 @@
-/*
- * decaffeinate suggestions:
- * DS101: Remove unnecessary use of Array.from
- * DS102: Remove unnecessary code created because of implicit returns
- * DS103: Rewrite code to no longer use __guard__
- * DS205: Consider reworking code to avoid use of IIFEs
- * DS207: Consider shorter variations of null checks
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
 const AuthenticationManager = require('./AuthenticationManager')
 const LoginRateLimiter = require('../Security/LoginRateLimiter')
 const UserUpdater = require('../User/UserUpdater')
@@ -22,9 +13,15 @@ const passport = require('passport')
 const NotificationsBuilder = require('../Notifications/NotificationsBuilder')
 const SudoModeHandler = require('../SudoMode/SudoModeHandler')
 const { URL } = require('url')
+const _ = require('lodash')
 
 const AuthenticationController = (module.exports = {
   serializeUser(user, callback) {
+    if (!user._id || !user.email) {
+      const err = new Error('serializeUser called with non-user object')
+      logger.warn({ user }, err.message)
+      return callback(err)
+    }
     const lightUser = {
       _id: user._id,
       first_name: user.first_name,
@@ -38,27 +35,27 @@ const AuthenticationController = (module.exports = {
       must_reconfirm: user.must_reconfirm,
       v1_id: user.overleaf != null ? user.overleaf.id : undefined
     }
-    return callback(null, lightUser)
+    callback(null, lightUser)
   },
 
   deserializeUser(user, cb) {
-    return cb(null, user)
+    cb(null, user)
   },
 
   afterLoginSessionSetup(req, user, callback) {
     if (callback == null) {
       callback = function() {}
     }
-    return req.login(user, function(err) {
+    req.login(user, function(err) {
-      if (err != null) {
+      if (err) {
         logger.warn({ user_id: user._id, err }, 'error from req.login')
         return callback(err)
       }
       // Regenerate the session to get a new sessionID (cookie value) to
       // protect against session fixation attacks
       const oldSession = req.session
-      return req.session.destroy(function(err) {
+      req.session.destroy(function(err) {
-        if (err != null) {
+        if (err) {
           logger.warn(
             { user_id: user._id, err },
             'error when trying to destroy old session'
@@ -72,10 +69,8 @@ const AuthenticationController = (module.exports = {
             req.session[key] = value
           }
         }
-        // copy to the old `session.user` location, for backward-comptability
+        req.session.save(function(err) {
-        req.session.user = req.session.passport.user
+          if (err) {
-        return req.session.save(function(err) {
-          if (err != null) {
             logger.warn(
               { user_id: user._id },
               'error saving regenerated session after login'
@@ -83,7 +78,7 @@ const AuthenticationController = (module.exports = {
             return callback(err)
           }
           UserSessionsManager.trackSession(user, req.sessionID, function() {})
-          return callback(null)
+          callback(null)
         })
       })
     })
@@ -93,8 +88,8 @@ const AuthenticationController = (module.exports = {
     // This function is middleware which wraps the passport.authenticate middleware,
     // so we can send back our custom `{message: {text: "", type: ""}}` responses on failure,
     // and send a `{redir: ""}` response on success
-    return passport.authenticate('local', function(err, user, info) {
+    passport.authenticate('local', function(err, user, info) {
-      if (err != null) {
+      if (err) {
         return next(err)
       }
       if (user) {
@@ -116,19 +111,16 @@ const AuthenticationController = (module.exports = {
     } // OAuth2 'state' mismatch
     if (user.must_reconfirm) {
       return AuthenticationController._redirectToReconfirmPage(req, res, user)
-    } else {
+    }
     const redir =
       AuthenticationController._getRedirectFromSession(req) || '/project'
     AuthenticationController._loginAsyncHandlers(req, user)
-      return AuthenticationController.afterLoginSessionSetup(
+    AuthenticationController.afterLoginSessionSetup(req, user, function(err) {
-        req,
+      if (err) {
-        user,
-        function(err) {
-          if (err != null) {
         return next(err)
       }
-          return SudoModeHandler.activateSudoMode(user._id, function(err) {
+      SudoModeHandler.activateSudoMode(user._id, function(err) {
-            if (err != null) {
+        if (err) {
           logger.err(
             { err, user_id: user._id },
             'Error activating Sudo Mode on login, continuing'
@@ -136,40 +128,32 @@ const AuthenticationController = (module.exports = {
         }
         AuthenticationController._clearRedirectFromSession(req)
         if (
-              __guard__(
+          _.get(req, ['headers', 'accept'], '').match(/^application\/json.*$/)
-                req.headers != null ? req.headers['accept'] : undefined,
-                x => x.match(/^application\/json.*$/)
-              )
         ) {
-              return res.json({ redir })
+          res.json({ redir })
         } else {
-              return res.redirect(redir)
+          res.redirect(redir)
         }
       })
-        }
+    })
-      )
-    }
   },
 
   doPassportLogin(req, username, password, done) {
     const email = username.toLowerCase()
     const Modules = require('../../infrastructure/Modules')
-    return Modules.hooks.fire('preDoPassportLogin', req, email, function(
+    Modules.hooks.fire('preDoPassportLogin', req, email, function(
       err,
       infoList
     ) {
-      if (err != null) {
+      if (err) {
         return done(err)
       }
       const info = infoList.find(i => i != null)
       if (info != null) {
         return done(null, false, info)
       }
-      return LoginRateLimiter.processLoginRequest(email, function(
+      LoginRateLimiter.processLoginRequest(email, function(err, isAllowed) {
-        err,
+        if (err) {
-        isAllowed
-      ) {
-        if (err != null) {
           return done(err)
         }
         if (!isAllowed) {
@@ -179,7 +163,7 @@ const AuthenticationController = (module.exports = {
             type: 'error'
           })
         }
-        return AuthenticationManager.authenticate({ email }, password, function(
+        AuthenticationManager.authenticate({ email }, password, function(
           error,
           user
         ) {
@@ -188,11 +172,11 @@ const AuthenticationController = (module.exports = {
           }
           if (user != null) {
             // async actions
-            return done(null, user)
+            done(null, user)
           } else {
             AuthenticationController._recordFailedLogin()
             logger.log({ email }, 'failed log in')
-            return done(null, false, {
+            done(null, false, {
               text: req.i18n.translate('email_or_password_wrong_try_again'),
               type: 'error'
             })
@@ -228,29 +212,15 @@ const AuthenticationController = (module.exports = {
   },
 
   setInSessionUser(req, props) {
-    return (() => {
+    const sessionUser = AuthenticationController.getSessionUser(req)
-      const result = []
+    if (!sessionUser) {
+      return
+    }
     for (let key in props) {
       const value = props[key]
-        if (
+      sessionUser[key] = value
-          __guard__(
-            __guard__(req != null ? req.session : undefined, x1 => x1.passport),
-            x => x.user
-          ) != null
-        ) {
-          req.session.passport.user[key] = value
     }
-        if (
+    return null
-          __guard__(req != null ? req.session : undefined, x2 => x2.user) !=
-          null
-        ) {
-          result.push((req.session.user[key] = value))
-        } else {
-          result.push(undefined)
-        }
-      }
-      return result
-    })()
   },
 
   isUserLoggedIn(req) {
@@ -278,18 +248,9 @@ const AuthenticationController = (module.exports = {
   },
 
   getSessionUser(req) {
-    if (__guard__(req != null ? req.session : undefined, x => x.user) != null) {
+    const sessionUser = _.get(req, ['session', 'user'])
-      return req.session.user
+    const sessionPassportUser = _.get(req, ['session', 'passport', 'user'])
-    } else if (
+    return sessionUser || sessionPassportUser || null
-      __guard__(
-        __guard__(req != null ? req.session : undefined, x2 => x2.passport),
-        x1 => x1.user
-      )
-    ) {
-      return req.session.passport.user
-    } else {
-      return null
-    }
   },
 
   requireLogin() {
@@ -321,7 +282,7 @@ const AuthenticationController = (module.exports = {
         err,
         token
       ) {
-        if (err != null) {
+        if (err) {
           // use a 401 status code for malformed header for git-bridge
           if (
             err.code === 400 &&
@@ -349,7 +310,7 @@ const AuthenticationController = (module.exports = {
 
   requireGlobalLogin(req, res, next) {
     if (
-      Array.from(AuthenticationController._globalLoginWhitelist).includes(
+      AuthenticationController._globalLoginWhitelist.includes(
         req._parsedUrl.pathname
       )
     ) {
@@ -357,16 +318,16 @@ const AuthenticationController = (module.exports = {
     }
 
     if (req.headers['authorization'] != null) {
-      return AuthenticationController.httpAuth(req, res, next)
+      AuthenticationController.httpAuth(req, res, next)
     } else if (AuthenticationController.isUserLoggedIn(req)) {
-      return next()
+      next()
     } else {
       logger.log(
         { url: req.url },
         'user trying to access endpoint not in global whitelist'
       )
       AuthenticationController.setRedirectInSession(req)
-      return res.redirect('/login')
+      res.redirect('/login')
     }
   },
 
@@ -401,9 +362,9 @@ const AuthenticationController = (module.exports = {
       req.query.project_name != null ||
       req.path === '/user/subscription/new'
     ) {
-      return AuthenticationController._redirectToRegisterPage(req, res)
+      AuthenticationController._redirectToRegisterPage(req, res)
     } else {
-      return AuthenticationController._redirectToLoginPage(req, res)
+      AuthenticationController._redirectToLoginPage(req, res)
     }
   },
 
@@ -415,7 +376,7 @@ const AuthenticationController = (module.exports = {
     AuthenticationController.setRedirectInSession(req)
     const url = `/login?${querystring.stringify(req.query)}`
     res.redirect(url)
-    return Metrics.inc('security.login-redirect')
+    Metrics.inc('security.login-redirect')
   },
 
   _redirectToReconfirmPage(req, res, user) {
@@ -425,14 +386,10 @@ const AuthenticationController = (module.exports = {
     )
     req.session.reconfirm_email = user != null ? user.email : undefined
     const redir = '/user/reconfirm'
-    if (
+    if (_.get(req, ['headers', 'accept'], '').match(/^application\/json.*$/)) {
-      __guard__(req.headers != null ? req.headers['accept'] : undefined, x =>
+      res.json({ redir })
-        x.match(/^application\/json.*$/)
-      )
-    ) {
-      return res.json({ redir })
     } else {
-      return res.redirect(redir)
+      res.redirect(redir)
     }
   },
 
@@ -444,14 +401,14 @@ const AuthenticationController = (module.exports = {
     AuthenticationController.setRedirectInSession(req)
     const url = `/register?${querystring.stringify(req.query)}`
     res.redirect(url)
-    return Metrics.inc('security.login-redirect')
+    Metrics.inc('security.login-redirect')
   },
 
   _recordSuccessfulLogin(userId, callback) {
     if (callback == null) {
       callback = function() {}
     }
-    return UserUpdater.updateUser(
+    UserUpdater.updateUser(
       userId.toString(),
       {
         $set: { lastLoggedIn: new Date() },
@@ -462,7 +419,7 @@ const AuthenticationController = (module.exports = {
           callback(error)
         }
         Metrics.inc('user.login.success')
-        return callback()
+        callback()
       }
     )
   },
@@ -474,10 +431,7 @@ const AuthenticationController = (module.exports = {
 
   _getRedirectFromSession(req) {
     let safePath
-    const value = __guard__(
+    const value = _.get(req, ['session', 'postLoginRedirect'])
-      req != null ? req.session : undefined,
-      x => x.postLoginRedirect
-    )
     if (value) {
       safePath = AuthenticationController._getSafeRedirectPath(value)
     }
@@ -486,7 +440,7 @@ const AuthenticationController = (module.exports = {
 
   _clearRedirectFromSession(req) {
     if (req.session != null) {
-      return delete req.session.postLoginRedirect
+      delete req.session.postLoginRedirect
     }
   },
 
@@ -500,9 +454,3 @@ const AuthenticationController = (module.exports = {
     return safePath
   }
 })
-
-function __guard__(value, transform) {
-  return typeof value !== 'undefined' && value !== null
-    ? transform(value)
-    : undefined
-}

services/web/app/src/Features/Collaborators/CollaboratorsController.js

@@ -1,12 +1,6 @@
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS207: Consider shorter variations of null checks
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
 const async = require('async')
 const CollaboratorsHandler = require('./CollaboratorsHandler')
+const AuthenticationController = require('../Authentication/AuthenticationController')
 const EditorRealTimeController = require('../Editor/EditorRealTimeController')
 const TagsHandler = require('../Tags/TagsHandler')
 const logger = require('logger-sharelatex')
@@ -19,7 +13,7 @@ const CollaboratorsController = {
       projectId,
       userId,
       function(error) {
-        if (error != null) {
+        if (error) {
           return next(error)
         }
         EditorRealTimeController.emitToRoom(
@@ -34,12 +28,12 @@ const CollaboratorsController = {
 
   removeSelfFromProject(req, res, next) {
     const projectId = req.params.Project_id
-    const userId = req.session.user ? req.session.user._id : undefined
+    const userId = AuthenticationController.getLoggedInUserId(req)
     CollaboratorsController._removeUserIdFromProject(
       projectId,
       userId,
       function(error) {
-        if (error != null) {
+        if (error) {
           return next(error)
         }
         res.sendStatus(204)
@@ -66,7 +60,7 @@ const CollaboratorsController = {
         }
       ],
       function(error) {
-        if (error != null) {
+        if (error) {
           return callback(error)
         }
         callback()
@@ -81,7 +75,7 @@ const CollaboratorsController = {
       err,
       members
     ) {
-      if (err != null) {
+      if (err) {
         logger.warn({ projectId }, 'error getting members for project')
         return next(err)
       }

services/web/app/src/Features/User/UserEmailsController.js

@@ -92,6 +92,7 @@ module.exports = UserEmailsController = {
       if (err) {
         return UserEmailsController._handleEmailError(err, req, res, next)
       }
+      AuthenticationController.setInSessionUser(req, { email: email })
       res.sendStatus(200)
     })
   },

services/web/test/unit/src/Authentication/AuthenticationControllerTests.js

@@ -142,8 +142,9 @@ describe('AuthenticationController', function() {
         last_name: 'b',
         email: 'c'
       }
-      this.req.session.passport = { user: this.user }
+      this.AuthenticationController.getSessionUser = sinon
-      return (this.req.session.user = this.user)
+        .stub()
+        .returns(this.user)
     })
 
     it('should update the right properties', function() {
@@ -157,8 +158,8 @@ describe('AuthenticationController', function() {
         last_name: 'b',
         email: 'new_email'
       }
-      expect(this.req.session.passport.user).to.deep.equal(expectedUser)
+      expect(this.user).to.deep.equal(expectedUser)
-      return expect(this.req.session.user).to.deep.equal(expectedUser)
+      return expect(this.user).to.deep.equal(expectedUser)
     })
   })
 

services/web/test/unit/src/Collaborators/CollaboratorsControllerTests.js

@@ -32,6 +32,7 @@ describe('CollaboratorsController', function() {
         './CollaboratorsHandler': (this.CollaboratorsHandler = {}),
         '../Editor/EditorRealTimeController': (this.EditorRealTimeController = {}),
         '../Tags/TagsHandler': (this.TagsHandler = {}),
+        '../Authentication/AuthenticationController': (this.AuthenticationController = {}),
         'logger-sharelatex': (this.logger = {
           err: sinon.stub(),
           warn: sinon.stub(),
@@ -87,7 +88,10 @@ describe('CollaboratorsController', function() {
 
   describe('removeSelfFromProject', function() {
     beforeEach(function() {
-      this.req.session = { user: { _id: (this.user_id = 'user-id-123') } }
+      this.user_id = 'user-id-123'
+      this.AuthenticationController.getLoggedInUserId = sinon
+        .stub()
+        .returns(this.user_id)
       this.req.params = { Project_id: this.project_id }
       this.res.sendStatus = sinon.stub()
       this.CollaboratorsHandler.removeUserFromProject = sinon.stub().callsArg(2)
@@ -126,7 +130,9 @@ describe('CollaboratorsController', function() {
 
   describe('getAllMembers', function() {
     beforeEach(function() {
-      this.req.session = { user: { _id: (this.user_id = 'user-id-123') } }
+      this.AuthenticationController.getLoggedInUserId = sinon
+        .stub()
+        .returns((this.user_id = 'user-id-123'))
       this.req.params = { Project_id: this.project_id }
       this.res.json = sinon.stub()
       this.next = sinon.stub()

services/web/test/unit/src/User/UserEmailsControllerTests.js

@@ -29,7 +29,8 @@ describe('UserEmailsController', function() {
 
     this.UserGetter = { getUserFullEmails: sinon.stub() }
     this.AuthenticationController = {
-      getLoggedInUserId: sinon.stub().returns(this.user._id)
+      getLoggedInUserId: sinon.stub().returns(this.user._id),
+      setInSessionUser: sinon.stub()
     }
     this.UserUpdater = {
       addEmailAddress: sinon.stub(),
@@ -188,6 +189,7 @@ describe('UserEmailsController', function() {
       this.email = 'email_to_set_default@bar.com'
       this.req.body.email = this.email
       this.EmailHelper.parseEmail.returns(this.email)
+      this.AuthenticationController.setInSessionUser.returns(null)
     })
 
     it('sets default email', function(done) {
@@ -197,6 +199,11 @@ describe('UserEmailsController', function() {
         sendStatus: code => {
           code.should.equal(200)
           assertCalledWith(this.EmailHelper.parseEmail, this.email)
+          assertCalledWith(
+            this.AuthenticationController.setInSessionUser,
+            this.req,
+            { email: this.email }
+          )
           assertCalledWith(
             this.UserUpdater.setDefaultEmailAddress,
             this.user._id,