[web] Promisify LdapController (#18500)

* Promisify LdapController

* Update tests LdapControllerTests.js

* Promisify `AuthenticationController.finishLogin`

* Simplify null checks in LdapController

* Fix: don't use spread operator in module.exports

* Make `AuthenticationController.promises.finishLogin` a promise that resolves

* Fixup: `finishLogin` does not call `next` then the promise finishes, it calls it only on errors

* Use `Modules.promises.hooks.fire`

* Revert `processPassportLogin` callback style

* Update error handling: Use `OError.tag` instead of `logger.err`

* Fix unit tests: Rely on callbacks rather than promises

* Fix: Actually call `passport.authenticate` (!!)

* Update test: fixup `passport.authenticate` mocks

This would have caught the bugs that the previous commit is solving

* Remove `.then(() => next())` in `processPassportLogin`

Co-authored-by: Eric Mc Sween <eric.mcsween@overleaf.com>

---------

Co-authored-by: Eric Mc Sween <eric.mcsween@overleaf.com>
GitOrigin-RevId: a7eab5f5289956aeb8f2418408958daef3511ab7

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

@@ -28,7 +28,7 @@ const {
 } = require('./AuthenticationErrors')
 const { hasAdminAccess } = require('../Helpers/AdminAuthorizationHelper')
 const Modules = require('../../infrastructure/Modules')
-const { expressify } = require('@overleaf/promise-utils')
+const { expressify, promisify } = require('@overleaf/promise-utils')
 
 function send401WithChallenge(res) {
   res.setHeader('WWW-Authenticate', 'OverleafLogin')
@@ -63,6 +63,7 @@ function userHasStaffAccess(user) {
   return user.staffAccess && Object.values(user.staffAccess).includes(true)
 }
 
+// TODO: Finish making these methods async
 const AuthenticationController = {
   serializeUser(user, callback) {
     if (!user._id || !user.email) {
@@ -134,7 +135,7 @@ const AuthenticationController = {
     )(req, res, next)
   },
 
-  finishLogin(user, req, res, next) {
+  async _finishLoginAsync(user, req, res) {
     if (user === false) {
       return AsyncFormHelper.redirect(req, res, '/login')
     } // OAuth2 'state' mismatch
@@ -154,54 +155,46 @@ const AuthenticationController = {
     const anonymousAnalyticsId = req.session.analyticsId
     const isNewUser = req.session.justRegistered || false
 
-    Modules.hooks.fire(
+    const results = await Modules.promises.hooks.fire(
       'preFinishLogin',
       req,
       res,
-      user,
-      function (error, results) {
-        if (error) {
-          return next(error)
-        }
-        if (results.some(result => result && result.doNotFinish)) {
-          return
-        }
+      user
+    )
 
-        if (user.must_reconfirm) {
-          return AuthenticationController._redirectToReconfirmPage(
-            req,
-            res,
-            user
-          )
-        }
+    if (results.some(result => result && result.doNotFinish)) {
+      return
+    }
 
-        const redir =
-          AuthenticationController.getRedirectFromSession(req) || '/project'
-
-        _loginAsyncHandlers(req, user, anonymousAnalyticsId, isNewUser)
-        const userId = user._id
-        UserAuditLogHandler.addEntry(
-          userId,
-          'login',
-          userId,
-          req.ip,
-          auditInfo,
-          err => {
-            if (err) {
-              return next(err)
-            }
-            _afterLoginSessionSetup(req, user, function (err) {
-              if (err) {
-                return next(err)
-              }
-              AuthenticationController._clearRedirectFromSession(req)
-              AnalyticsRegistrationSourceHelper.clearSource(req.session)
-              AnalyticsRegistrationSourceHelper.clearInbound(req.session)
-              AsyncFormHelper.redirect(req, res, redir)
-            })
-          }
-        )
-      }
+    if (user.must_reconfirm) {
+      return AuthenticationController._redirectToReconfirmPage(req, res, user)
+    }
+
+    const redir =
+      AuthenticationController.getRedirectFromSession(req) || '/project'
+
+    _loginAsyncHandlers(req, user, anonymousAnalyticsId, isNewUser)
+    const userId = user._id
+
+    await UserAuditLogHandler.promises.addEntry(
+      userId,
+      'login',
+      userId,
+      req.ip,
+      auditInfo
+    )
+
+    await _afterLoginSessionSetupAsync(req, user)
+
+    AuthenticationController._clearRedirectFromSession(req)
+    AnalyticsRegistrationSourceHelper.clearSource(req.session)
+    AnalyticsRegistrationSourceHelper.clearInbound(req.session)
+    AsyncFormHelper.redirect(req, res, redir)
+  },
+
+  finishLogin(user, req, res, next) {
+    AuthenticationController._finishLoginAsync(user, req, res).catch(err =>
+      next(err)
     )
   },
 
@@ -637,6 +630,8 @@ function _afterLoginSessionSetup(req, user, callback) {
   })
 }
 
+const _afterLoginSessionSetupAsync = promisify(_afterLoginSessionSetup)
+
 function _loginAsyncHandlers(req, user, anonymousAnalyticsId, isNewUser) {
   UserHandler.setupLoginData(user, err => {
     if (err != null) {
@@ -663,4 +658,8 @@ function _loginAsyncHandlers(req, user, anonymousAnalyticsId, isNewUser) {
   return (user._login_req_ip = req.ip)
 }
 
+AuthenticationController.promises = {
+  finishLogin: AuthenticationController._finishLoginAsync,
+}
+
 module.exports = AuthenticationController

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

@@ -68,6 +68,9 @@ describe('AuthenticationController', function () {
         './AuthenticationErrors': AuthenticationErrors,
         '../User/UserAuditLogHandler': (this.UserAuditLogHandler = {
           addEntry: sinon.stub().yields(null),
+          promises: {
+            addEntry: sinon.stub().resolves(),
+          },
         }),
         '../Helpers/AsyncFormHelper': (this.AsyncFormHelper = {
           redirect: sinon.stub(),
@@ -107,6 +110,7 @@ describe('AuthenticationController', function () {
         }),
         '../../infrastructure/Modules': (this.Modules = {
           hooks: { fire: sinon.stub().yields(null, []) },
+          promises: { hooks: { fire: sinon.stub().resolves([]) } },
         }),
         '../Notifications/NotificationsBuilder': (this.NotificationsBuilder = {
           ipMatcherAffiliation: sinon.stub().returns({ create: sinon.stub() }),
@@ -350,7 +354,6 @@ describe('AuthenticationController', function () {
     beforeEach(function () {
       this.AuthenticationController._recordFailedLogin = sinon.stub()
       this.AuthenticationController._recordSuccessfulLogin = sinon.stub()
-      this.Modules.hooks.fire = sinon.stub().yields(null, [])
       this.req.body = {
         email: this.email,
         password: this.password,
@@ -1125,8 +1128,8 @@ describe('AuthenticationController', function () {
       this.res.redirect = sinon.stub()
     })
 
-    it('should extract the redirect from the session', function () {
-      this.AuthenticationController.finishLogin(
+    it('should extract the redirect from the session', async function () {
+      await this.AuthenticationController.promises.finishLogin(
         this.user,
         this.req,
         this.res,
@@ -1142,8 +1145,8 @@ describe('AuthenticationController', function () {
       ).to.equal(true)
     })
 
-    it('should clear redirect from session', function () {
-      this.AuthenticationController.finishLogin(
+    it('should clear redirect from session', async function () {
+      await this.AuthenticationController.promises.finishLogin(
         this.user,
         this.req,
         this.res,
@@ -1159,8 +1162,8 @@ describe('AuthenticationController', function () {
       ).to.equal(true)
     })
 
-    it('should issue a json response with a redirect', function () {
-      this.AuthenticationController.finishLogin(
+    it('should issue a json response with a redirect', async function () {
+      await this.AuthenticationController.promises.finishLogin(
         this.user,
         this.req,
         this.res,
@@ -1182,8 +1185,8 @@ describe('AuthenticationController', function () {
         this.res.redirect = sinon.stub()
       })
 
-      it('should issue a plain redirect', function () {
-        this.AuthenticationController.finishLogin(
+      it('should issue a plain redirect', async function () {
+        await this.AuthenticationController.promises.finishLogin(
           this.user,
           this.req,
           this.res,
@@ -1204,8 +1207,8 @@ describe('AuthenticationController', function () {
         this.req.session = {}
         this.user.must_reconfirm = true
       })
-      it('should redirect to reconfirm page', function () {
-        this.AuthenticationController.finishLogin(
+      it('should redirect to reconfirm page', async function () {
+        await this.AuthenticationController.promises.finishLogin(
           this.user,
           this.req,
           this.res,
@@ -1224,8 +1227,8 @@ describe('AuthenticationController', function () {
         this.req.session = {}
         this.user.suspended = true
       })
-      it('should not log in and instead redirect to suspended account page', function () {
-        this.AuthenticationController.finishLogin(
+      it('should not log in and instead redirect to suspended account page', async function () {
+        await this.AuthenticationController.promises.finishLogin(
           this.user,
           this.req,
           this.res,
@@ -1242,16 +1245,15 @@ describe('AuthenticationController', function () {
     })
 
     describe('preFinishLogin hook', function () {
-      it('call hook and proceed', function () {
-        this.Modules.hooks.fire = sinon.stub().yields(null, [])
-        this.AuthenticationController.finishLogin(
+      it('call hook and proceed', async function () {
+        await this.AuthenticationController.promises.finishLogin(
           this.user,
           this.req,
           this.res,
           this.next
         )
         sinon.assert.calledWith(
-          this.Modules.hooks.fire,
+          this.Modules.promises.hooks.fire,
           'preFinishLogin',
           this.req,
           this.res,
@@ -1260,11 +1262,11 @@ describe('AuthenticationController', function () {
         expect(this.AsyncFormHelper.redirect.called).to.equal(true)
       })
 
-      it('stop if hook has redirected', function (done) {
-        this.Modules.hooks.fire = sinon
+      it('stop if hook has redirected', async function () {
+        this.Modules.promises.hooks.fire = sinon
           .stub()
-          .yields(null, [{ doNotFinish: true }])
-        this.AuthenticationController.finishLogin(
+          .resolves([{ doNotFinish: true }])
+        await this.AuthenticationController.promises.finishLogin(
           this.user,
           this.req,
           this.res,
@@ -1272,33 +1274,31 @@ describe('AuthenticationController', function () {
         )
         expect(this.next.callCount).to.equal(0)
         expect(this.res.json.callCount).to.equal(0)
-        done()
       })
 
       it('call next with hook errors', function (done) {
-        this.Modules.hooks.fire = sinon.stub().yields(new Error())
-        this.AuthenticationController.finishLogin(
-          this.user,
-          this.req,
-          this.res,
-          error => {
-            expect(error).to.exist
+        this.Modules.promises.hooks.fire = sinon.stub().yields(new Error())
+        this.AuthenticationController.promises
+          .finishLogin(this.user, this.req, this.res)
+          .catch(err => {
+            expect(err).to.exist
             expect(this.res.json.callCount).to.equal(0)
             done()
-          }
-        )
+          })
       })
     })
 
     describe('UserAuditLog', function () {
-      it('should add an audit log entry', function () {
-        this.AuthenticationController.finishLogin(
+      it('should add an audit log entry', async function () {
+        await this.AuthenticationController.promises.finishLogin(
           this.user,
           this.req,
           this.res,
           this.next
         )
-        expect(this.UserAuditLogHandler.addEntry).to.have.been.calledWith(
+        expect(
+          this.UserAuditLogHandler.promises.addEntry
+        ).to.have.been.calledWith(
           this.user._id,
           'login',
           this.user._id,
@@ -1306,42 +1306,47 @@ describe('AuthenticationController', function () {
         )
       })
 
-      it('should add an audit log entry before logging the user in', function () {
-        this.AuthenticationController.finishLogin(
+      it('should add an audit log entry before logging the user in', async function () {
+        await this.AuthenticationController.promises.finishLogin(
           this.user,
           this.req,
           this.res,
           this.next
         )
-        expect(this.UserAuditLogHandler.addEntry).to.have.been.calledBefore(
-          this.req.login
-        )
+        expect(
+          this.UserAuditLogHandler.promises.addEntry
+        ).to.have.been.calledBefore(this.req.login)
       })
 
-      it('should not log the user in without an audit log entry', function () {
+      it('should not log the user in without an audit log entry', function (done) {
         const theError = new Error()
-        this.UserAuditLogHandler.addEntry.yields(theError)
+        this.UserAuditLogHandler.promises.addEntry.rejects(theError)
+        this.next.callsFake(err => {
+          expect(err).to.equal(theError)
+          expect(this.next).to.have.been.calledWith(theError)
+          expect(this.req.login).to.not.have.been.called
+          done()
+        })
         this.AuthenticationController.finishLogin(
           this.user,
           this.req,
           this.res,
           this.next
         )
-        expect(this.next).to.have.been.calledWith(theError)
-        expect(this.req.login).to.not.have.been.called
       })
 
-      it('should pass along auditInfo when present', function () {
+      it('should pass along auditInfo when present', async function () {
         this.AuthenticationController.setAuditInfo(this.req, {
           method: 'Login',
         })
-        this.AuthenticationController.finishLogin(
+        await this.AuthenticationController.promises.finishLogin(
           this.user,
           this.req,
-          this.res,
-          this.next
+          this.res
         )
-        expect(this.UserAuditLogHandler.addEntry).to.have.been.calledWith(
+        expect(
+          this.UserAuditLogHandler.promises.addEntry
+        ).to.have.been.calledWith(
           this.user._id,
           'login',
           this.user._id,
@@ -1354,8 +1359,8 @@ describe('AuthenticationController', function () {
     describe('_afterLoginSessionSetup', function () {
       beforeEach(function () {})
 
-      it('should call req.login', function () {
-        this.AuthenticationController.finishLogin(
+      it('should call req.login', async function () {
+        await this.AuthenticationController.promises.finishLogin(
           this.user,
           this.req,
           this.res,
@@ -1364,8 +1369,8 @@ describe('AuthenticationController', function () {
         this.req.login.callCount.should.equal(1)
       })
 
-      it('should erase the CSRF secret', function () {
-        this.AuthenticationController.finishLogin(
+      it('should erase the CSRF secret', async function () {
+        await this.AuthenticationController.promises.finishLogin(
           this.user,
           this.req,
           this.res,
@@ -1374,8 +1379,8 @@ describe('AuthenticationController', function () {
         expect(this.req.session.csrfSecret).to.not.exist
       })
 
-      it('should call req.session.save', function () {
-        this.AuthenticationController.finishLogin(
+      it('should call req.session.save', async function () {
+        await this.AuthenticationController.promises.finishLogin(
           this.user,
           this.req,
           this.res,
@@ -1384,8 +1389,8 @@ describe('AuthenticationController', function () {
         this.req.session.save.callCount.should.equal(1)
       })
 
-      it('should call UserSessionsManager.trackSession', function () {
-        this.AuthenticationController.finishLogin(
+      it('should call UserSessionsManager.trackSession', async function () {
+        await this.AuthenticationController.promises.finishLogin(
           this.user,
           this.req,
           this.res,
@@ -1400,36 +1405,30 @@ describe('AuthenticationController', function () {
         })
 
         it('should produce an error', function (done) {
-          this.AuthenticationController.finishLogin(
-            this.user,
-            this.req,
-            this.res,
-            err => {
+          this.AuthenticationController.promises
+            .finishLogin(this.user, this.req, this.res)
+            .catch(err => {
               expect(err).to.not.be.oneOf([null, undefined])
               expect(err).to.be.instanceof(Error)
               done()
-            }
-          )
+            })
         })
 
         it('should not call UserSessionsManager.trackSession', function (done) {
-          this.AuthenticationController.finishLogin(
-            this.user,
-            this.req,
-            this.res,
-            err => {
+          this.AuthenticationController.promises
+            .finishLogin(this.user, this.req, this.res)
+            .catch(err => {
               expect(err).to.exist
               this.UserSessionsManager.trackSession.callCount.should.equal(0)
               done()
-            }
-          )
+            })
         })
       })
     })
 
     describe('_loginAsyncHandlers', function () {
-      beforeEach(function () {
-        this.AuthenticationController.finishLogin(
+      beforeEach(async function () {
+        await this.AuthenticationController.promises.finishLogin(
           this.user,
           this.req,
           this.res,