Merge pull request #16550 from overleaf/ab-unlink-sso-user-leaves-group

[web] Unlink group SSO when user leaves/is removed from group

GitOrigin-RevId: a4515f8b6f0f0012fc4d9f47b11e6f711743c2ec

services/web/app/src/Features/Subscription/GroupSSOHandler.js

@@ -4,6 +4,7 @@ const UserUpdater = require('../User/UserUpdater')
 const SAMLIdentityManager = require('../User/SAMLIdentityManager')
 const { User } = require('../../models/User')
 const Errors = require('../Errors/Errors')
+const GroupUtils = require('./GroupUtils')
 
 async function checkUserCanEnrollInSubscription(userId, subscription) {
   const ssoConfig = await SSOConfig.findById(subscription?.ssoConfig).exec()
@@ -37,7 +38,7 @@ async function enrollInSubscription(
 ) {
   await checkUserCanEnrollInSubscription(userId, subscription)
 
-  const providerId = `ol-group-subscription-id:${subscription._id.toString()}`
+  const providerId = GroupUtils.getProviderId(subscription._id)
 
   const userBySamlIdentifier = await SAMLIdentityManager.getUser(
     providerId,

services/web/app/src/Features/Subscription/GroupUtils.js

@@ -0,0 +1,14 @@
+// ts-check
+/**
+ * Builds a group subscription's `providerId` to be used to identify SAML identifiers
+ * belonging to this group.
+ * @param {string | import('mongodb').ObjectId} subscriptionId
+ * @returns {string}
+ */
+function getProviderId(subscriptionId) {
+  return `ol-group-subscription-id:${subscriptionId.toString()}`
+}
+
+module.exports = {
+  getProviderId,
+}

services/web/app/src/Features/Subscription/SubscriptionGroupController.js

@@ -1,95 +1,116 @@
+// ts-check
 const SubscriptionGroupHandler = require('./SubscriptionGroupHandler')
 const OError = require('@overleaf/o-error')
 const logger = require('@overleaf/logger')
 const SubscriptionLocator = require('./SubscriptionLocator')
 const SessionManager = require('../Authentication/SessionManager')
 const UserAuditLogHandler = require('../User/UserAuditLogHandler')
+const { expressify } = require('@overleaf/promise-utils')
+const Modules = require('../../infrastructure/Modules')
 
-function removeUserFromGroup(req, res, next) {
+/**
+ * @typedef {import("../../../../types/subscription/dashboard/subscription").Subscription} Subscription
+ */
+
+/**
+ * @param {import("express").Request} req
+ * @param {import("express").Response} res
+ * @returns {Promise<void>}
+ */
+async function removeUserFromGroup(req, res) {
   const subscription = req.entity
   const userToRemoveId = req.params.user_id
   const loggedInUserId = SessionManager.getLoggedInUserId(req.session)
+  const subscriptionId = subscription._id
   logger.debug(
-    { subscriptionId: subscription._id, userToRemoveId },
+    { subscriptionId, userToRemoveId },
     'removing user from group subscription'
   )
-  UserAuditLogHandler.addEntry(
+
+  await _removeUserFromGroup(req, res, {
     userToRemoveId,
-    'remove-from-group-subscription',
     loggedInUserId,
-    req.ip,
-    { subscriptionId: subscription._id },
-    function (auditLogError) {
-      if (auditLogError) {
-        OError.tag(auditLogError, 'error adding audit log entry', {
-          userToRemoveId,
-          subscriptionId: subscription._id,
-        })
-        return next(auditLogError)
-      }
-      SubscriptionGroupHandler.removeUserFromGroup(
-        subscription._id,
-        userToRemoveId,
-        function (error) {
-          if (error) {
-            OError.tag(error, 'error removing user from group', {
-              subscriptionId: subscription._id,
-              userToRemove_id: userToRemoveId,
-            })
-            return next(error)
-          }
-          res.sendStatus(200)
-        }
-      )
-    }
-  )
+    subscription,
+  })
 }
 
-function removeSelfFromGroup(req, res, next) {
-  const subscriptionId = req.query.subscriptionId
+/**
+ * @param {import("express").Request} req
+ * @param {import("express").Response} res
+ * @returns {Promise<void>}
+ */
+async function removeSelfFromGroup(req, res) {
   const userToRemoveId = SessionManager.getLoggedInUserId(req.session)
-  SubscriptionLocator.getSubscription(
-    subscriptionId,
-    function (error, subscription) {
-      if (error) {
-        return next(error)
-      }
-
-      UserAuditLogHandler.addEntry(
-        userToRemoveId,
-        'remove-from-group-subscription',
-        userToRemoveId,
-        req.ip,
-        { subscriptionId: subscription._id },
-        function (auditLogError) {
-          if (auditLogError) {
-            OError.tag(auditLogError, 'error adding audit log entry', {
-              userToRemoveId,
-              subscriptionId,
-            })
-            return next(auditLogError)
-          }
-          SubscriptionGroupHandler.removeUserFromGroup(
-            subscription._id,
-            userToRemoveId,
-            function (error) {
-              if (error) {
-                logger.err(
-                  { err: error, userToRemoveId, subscriptionId },
-                  'error removing self from group'
-                )
-                return res.sendStatus(500)
-              }
-              res.sendStatus(200)
-            }
-          )
-        }
-      )
-    }
+  const subscription = await SubscriptionLocator.promises.getSubscription(
+    req.query.subscriptionId
   )
+
+  await _removeUserFromGroup(req, res, {
+    userToRemoveId,
+    loggedInUserId: userToRemoveId,
+    subscription,
+  })
+}
+
+/**
+ * @param {import("express").Request} req
+ * @param {import("express").Response} res
+ * @param {string} userToRemoveId
+ * @param {string} loggedInUserId
+ * @param {Subscription} subscription
+ * @returns {Promise<void>}
+ * @private
+ */
+async function _removeUserFromGroup(
+  req,
+  res,
+  { userToRemoveId, loggedInUserId, subscription }
+) {
+  const subscriptionId = subscription._id
+
+  const groupSSOActive = (
+    await Modules.promises.hooks.fire('hasGroupSSOEnabled', subscription)
+  )?.[0]
+  if (groupSSOActive) {
+    await Modules.promises.hooks.fire(
+      'unlinkUserFromGroupSSO',
+      userToRemoveId,
+      subscriptionId
+    )
+  }
+
+  try {
+    await UserAuditLogHandler.promises.addEntry(
+      userToRemoveId,
+      'remove-from-group-subscription',
+      loggedInUserId,
+      req.ip,
+      { subscriptionId }
+    )
+  } catch (auditLogError) {
+    throw OError.tag(auditLogError, 'error adding audit log entry', {
+      userToRemoveId,
+      subscriptionId,
+    })
+  }
+
+  try {
+    await SubscriptionGroupHandler.promises.removeUserFromGroup(
+      subscriptionId,
+      userToRemoveId
+    )
+  } catch (error) {
+    logger.err(
+      { err: error, userToRemoveId, subscriptionId },
+      'error removing self from group'
+    )
+    return res.sendStatus(500)
+  }
+
+  res.sendStatus(200)
 }
 
 module.exports = {
-  removeUserFromGroup,
-  removeSelfFromGroup,
+  removeUserFromGroup: expressify(removeUserFromGroup),
+  removeSelfFromGroup: expressify(removeSelfFromGroup),
 }

services/web/app/src/Features/Subscription/SubscriptionGroupHandler.js

@@ -3,65 +3,58 @@ const SubscriptionUpdater = require('./SubscriptionUpdater')
 const SubscriptionLocator = require('./SubscriptionLocator')
 const { Subscription } = require('../../models/Subscription')
 
-const SubscriptionGroupHandler = {
-  removeUserFromGroup(subscriptionId, userIdToRemove, callback) {
-    SubscriptionUpdater.removeUserFromGroup(
-      subscriptionId,
-      userIdToRemove,
-      callback
-    )
-  },
-
-  replaceUserReferencesInGroups(oldId, newId, callback) {
-    Subscription.updateOne(
-      { admin_id: oldId },
-      { admin_id: newId },
-      function (error) {
-        if (error) {
-          return callback(error)
-        }
-
-        replaceInArray(
-          Subscription,
-          'manager_ids',
-          oldId,
-          newId,
-          function (error) {
-            if (error) {
-              return callback(error)
-            }
-
-            replaceInArray(Subscription, 'member_ids', oldId, newId, callback)
-          }
-        )
-      }
-    )
-  },
-
-  isUserPartOfGroup(userId, subscriptionId, callback) {
-    SubscriptionLocator.getSubscriptionByMemberIdAndId(
-      userId,
-      subscriptionId,
-      function (err, subscription) {
-        let partOfGroup
-        if (subscription) {
-          partOfGroup = true
-        } else {
-          partOfGroup = false
-        }
-        callback(err, partOfGroup)
-      }
-    )
-  },
-
-  getTotalConfirmedUsersInGroup(subscriptionId, callback) {
-    SubscriptionLocator.getSubscription(subscriptionId, (err, subscription) =>
-      callback(err, subscription?.member_ids?.length)
-    )
-  },
+function removeUserFromGroup(subscriptionId, userIdToRemove, callback) {
+  SubscriptionUpdater.removeUserFromGroup(
+    subscriptionId,
+    userIdToRemove,
+    callback
+  )
 }
 
-function replaceInArray(model, property, oldValue, newValue, callback) {
+function replaceUserReferencesInGroups(oldId, newId, callback) {
+  Subscription.updateOne(
+    { admin_id: oldId },
+    { admin_id: newId },
+    function (error) {
+      if (error) {
+        return callback(error)
+      }
+
+      _replaceInArray(
+        Subscription,
+        'manager_ids',
+        oldId,
+        newId,
+        function (error) {
+          if (error) {
+            return callback(error)
+          }
+
+          _replaceInArray(Subscription, 'member_ids', oldId, newId, callback)
+        }
+      )
+    }
+  )
+}
+
+function isUserPartOfGroup(userId, subscriptionId, callback) {
+  SubscriptionLocator.getSubscriptionByMemberIdAndId(
+    userId,
+    subscriptionId,
+    function (err, subscription) {
+      const partOfGroup = !!subscription
+      callback(err, partOfGroup)
+    }
+  )
+}
+
+function getTotalConfirmedUsersInGroup(subscriptionId, callback) {
+  SubscriptionLocator.getSubscription(subscriptionId, (err, subscription) =>
+    callback(err, subscription?.member_ids?.length)
+  )
+}
+
+function _replaceInArray(model, property, oldValue, newValue, callback) {
   // Mongo won't let us pull and addToSet in the same query, so do it in
   // two. Note we need to add first, since the query is based on the old user.
   const query = {}
@@ -81,11 +74,15 @@ function replaceInArray(model, property, oldValue, newValue, callback) {
   })
 }
 
-SubscriptionGroupHandler.promises = {
-  getTotalConfirmedUsersInGroup: promisify(
-    SubscriptionGroupHandler.getTotalConfirmedUsersInGroup
-  ),
-  isUserPartOfGroup: promisify(SubscriptionGroupHandler.isUserPartOfGroup),
+module.exports = {
+  removeUserFromGroup,
+  replaceUserReferencesInGroups,
+  getTotalConfirmedUsersInGroup,
+  isUserPartOfGroup,
+  promises: {
+    removeUserFromGroup: promisify(removeUserFromGroup),
+    replaceUserReferencesInGroups: promisify(replaceUserReferencesInGroups),
+    getTotalConfirmedUsersInGroup: promisify(getTotalConfirmedUsersInGroup),
+    isUserPartOfGroup: promisify(isUserPartOfGroup),
+  },
 }
-
-module.exports = SubscriptionGroupHandler

services/web/test/unit/src/Subscription/SubscriptionGroupControllerTests.js

@@ -26,10 +26,16 @@ describe('SubscriptionGroupController', function () {
       _id: this.subscriptionId,
     }
 
-    this.GroupHandler = { removeUserFromGroup: sinon.stub().callsArgWith(2) }
+    this.SubscriptionGroupHandler = {
+      promises: {
+        removeUserFromGroup: sinon.stub().resolves(),
+      },
+    }
 
     this.SubscriptionLocator = {
-      getSubscription: sinon.stub().callsArgWith(1, null, this.subscription),
+      promises: {
+        getSubscription: sinon.stub().resolves(this.subscription),
+      },
     }
 
     this.SessionManager = {
@@ -42,15 +48,26 @@ describe('SubscriptionGroupController', function () {
     }
 
     this.UserAuditLogHandler = {
-      addEntry: sinon.stub().callsArgWith(5),
+      promises: {
+        addEntry: sinon.stub().resolves(),
+      },
+    }
+
+    this.Modules = {
+      promises: {
+        hooks: {
+          fire: sinon.stub().resolves(),
+        },
+      },
     }
 
     this.Controller = SandboxedModule.require(modulePath, {
       requires: {
-        './SubscriptionGroupHandler': this.GroupHandler,
+        './SubscriptionGroupHandler': this.SubscriptionGroupHandler,
         './SubscriptionLocator': this.SubscriptionLocator,
         '../Authentication/SessionManager': this.SessionManager,
         '../User/UserAuditLogHandler': this.UserAuditLogHandler,
+        '../../infrastructure/Modules': this.Modules,
       },
     })
   })
@@ -63,13 +80,13 @@ describe('SubscriptionGroupController', function () {
 
       const res = {
         sendStatus: () => {
-          this.GroupHandler.removeUserFromGroup
+          this.SubscriptionGroupHandler.promises.removeUserFromGroup
             .calledWith(this.subscriptionId, userIdToRemove)
             .should.equal(true)
           done()
         },
       }
-      this.Controller.removeUserFromGroup(this.req, res)
+      this.Controller.removeUserFromGroup(this.req, res, done)
     })
 
     it('should log that the user has been removed', function (done) {
@@ -80,7 +97,7 @@ describe('SubscriptionGroupController', function () {
       const res = {
         sendStatus: () => {
           sinon.assert.calledWith(
-            this.UserAuditLogHandler.addEntry,
+            this.UserAuditLogHandler.promises.addEntry,
             userIdToRemove,
             'remove-from-group-subscription',
             this.adminUserId,
@@ -90,7 +107,54 @@ describe('SubscriptionGroupController', function () {
           done()
         },
       }
-      this.Controller.removeUserFromGroup(this.req, res)
+      this.Controller.removeUserFromGroup(this.req, res, done)
+    })
+
+    it('should call the group SSO hooks with group SSO enabled', function (done) {
+      const userIdToRemove = '31231'
+      this.req.params = { user_id: userIdToRemove }
+      this.req.entity = this.subscription
+      this.Modules.promises.hooks.fire
+        .withArgs('hasGroupSSOEnabled', this.subscription)
+        .resolves([true])
+
+      const res = {
+        sendStatus: () => {
+          this.Modules.promises.hooks.fire
+            .calledWith('hasGroupSSOEnabled', this.subscription)
+            .should.equal(true)
+          this.Modules.promises.hooks.fire
+            .calledWith(
+              'unlinkUserFromGroupSSO',
+              userIdToRemove,
+              this.subscriptionId
+            )
+            .should.equal(true)
+          sinon.assert.calledTwice(this.Modules.promises.hooks.fire)
+          done()
+        },
+      }
+      this.Controller.removeUserFromGroup(this.req, res, done)
+    })
+
+    it('should call the group SSO hooks with group SSO disabled', function (done) {
+      const userIdToRemove = '31231'
+      this.req.params = { user_id: userIdToRemove }
+      this.req.entity = this.subscription
+      this.Modules.promises.hooks.fire
+        .withArgs('hasGroupSSOEnabled', this.subscription)
+        .resolves([false])
+
+      const res = {
+        sendStatus: () => {
+          this.Modules.promises.hooks.fire
+            .calledWith('hasGroupSSOEnabled', this.subscription)
+            .should.equal(true)
+          sinon.assert.calledOnce(this.Modules.promises.hooks.fire)
+          done()
+        },
+      }
+      this.Controller.removeUserFromGroup(this.req, res, done)
     })
   })
 
@@ -103,29 +167,29 @@ describe('SubscriptionGroupController', function () {
       const res = {
         sendStatus: () => {
           sinon.assert.calledWith(
-            this.SubscriptionLocator.getSubscription,
+            this.SubscriptionLocator.promises.getSubscription,
             this.subscriptionId
           )
           sinon.assert.calledWith(
-            this.GroupHandler.removeUserFromGroup,
+            this.SubscriptionGroupHandler.promises.removeUserFromGroup,
             this.subscriptionId,
             memberUserIdToremove
           )
           done()
         },
       }
-      this.Controller.removeSelfFromGroup(this.req, res)
+      this.Controller.removeSelfFromGroup(this.req, res, done)
     })
 
     it('should log that the user has left the subscription', function (done) {
       this.req.query = { subscriptionId: this.subscriptionId }
-      const memberUserIdToremove = 123456789
+      const memberUserIdToremove = '123456789'
       this.req.session.user._id = memberUserIdToremove
 
       const res = {
         sendStatus: () => {
           sinon.assert.calledWith(
-            this.UserAuditLogHandler.addEntry,
+            this.UserAuditLogHandler.promises.addEntry,
             memberUserIdToremove,
             'remove-from-group-subscription',
             memberUserIdToremove,
@@ -135,7 +199,56 @@ describe('SubscriptionGroupController', function () {
           done()
         },
       }
-      this.Controller.removeSelfFromGroup(this.req, res)
+      this.Controller.removeSelfFromGroup(this.req, res, done)
+    })
+
+    it('should call the group SSO hooks with group SSO enabled', function (done) {
+      this.req.query = { subscriptionId: this.subscriptionId }
+      const memberUserIdToremove = '123456789'
+      this.req.session.user._id = memberUserIdToremove
+
+      this.Modules.promises.hooks.fire
+        .withArgs('hasGroupSSOEnabled', this.subscription)
+        .resolves([true])
+
+      const res = {
+        sendStatus: () => {
+          this.Modules.promises.hooks.fire
+            .calledWith('hasGroupSSOEnabled', this.subscription)
+            .should.equal(true)
+          this.Modules.promises.hooks.fire
+            .calledWith(
+              'unlinkUserFromGroupSSO',
+              memberUserIdToremove,
+              this.subscriptionId
+            )
+            .should.equal(true)
+          sinon.assert.calledTwice(this.Modules.promises.hooks.fire)
+          done()
+        },
+      }
+      this.Controller.removeSelfFromGroup(this.req, res, done)
+    })
+
+    it('should call the group SSO hooks with group SSO disabled', function (done) {
+      const userIdToRemove = '31231'
+      this.req.session.user._id = userIdToRemove
+      this.req.params = { user_id: userIdToRemove }
+      this.req.entity = this.subscription
+      this.Modules.promises.hooks.fire
+        .withArgs('hasGroupSSOEnabled', this.subscription)
+        .resolves([false])
+
+      const res = {
+        sendStatus: () => {
+          this.Modules.promises.hooks.fire
+            .calledWith('hasGroupSSOEnabled', this.subscription)
+            .should.equal(true)
+          sinon.assert.calledOnce(this.Modules.promises.hooks.fire)
+          done()
+        },
+      }
+      this.Controller.removeSelfFromGroup(this.req, res, done)
     })
   })
 })