diff --git a/services/web/app/src/Features/Subscription/GroupSSOHandler.js b/services/web/app/src/Features/Subscription/GroupSSOHandler.js index b8d4a2b7c5..45fe595882 100644 --- a/services/web/app/src/Features/Subscription/GroupSSOHandler.js +++ b/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, diff --git a/services/web/app/src/Features/Subscription/GroupUtils.js b/services/web/app/src/Features/Subscription/GroupUtils.js new file mode 100644 index 0000000000..55aea1a180 --- /dev/null +++ b/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, +} diff --git a/services/web/app/src/Features/Subscription/SubscriptionGroupController.js b/services/web/app/src/Features/Subscription/SubscriptionGroupController.js index 238fcd9d80..75fe8872da 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionGroupController.js +++ b/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} + */ +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} + */ +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} + * @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), } diff --git a/services/web/app/src/Features/Subscription/SubscriptionGroupHandler.js b/services/web/app/src/Features/Subscription/SubscriptionGroupHandler.js index a246bd6a83..fbcff33c8d 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionGroupHandler.js +++ b/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 diff --git a/services/web/test/unit/src/Subscription/SubscriptionGroupControllerTests.js b/services/web/test/unit/src/Subscription/SubscriptionGroupControllerTests.js index ff855e7534..ec60851096 100644 --- a/services/web/test/unit/src/Subscription/SubscriptionGroupControllerTests.js +++ b/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) }) }) })