From a047388b08dd85744bb961f76a6096ae5de26e83 Mon Sep 17 00:00:00 2001 From: Thomas Date: Fri, 28 Jun 2024 14:05:38 +0200 Subject: [PATCH] Add serverside checks for changing the user access level after link sharing changes (#19168) * Add getEditInviteCount to count only edit collaborators * Add getInvitedEditCollaboratorCount to count joined editors * Add canAddXEditCollaborators to determine if owner can add more editors * Update setCollaboratorInfo to check if editor slots are available GitOrigin-RevId: a88707f102dfbde39322f5a7bbc79d47b6e810d5 --- .../Collaborators/CollaboratorsController.js | 26 +++ .../Collaborators/CollaboratorsGetter.js | 11 ++ .../CollaboratorsInviteHandler.js | 10 + .../Subscription/LimitationsManager.js | 21 ++- .../CollaboratorsControllerTests.js | 108 +++++++++++ .../Collaborators/CollaboratorsGetterTests.js | 10 + .../CollaboratorsInviteHandlerTests.js | 34 ++++ .../Subscription/LimitationsManagerTests.js | 172 +++++++++++++++++- 8 files changed, 389 insertions(+), 3 deletions(-) diff --git a/services/web/app/src/Features/Collaborators/CollaboratorsController.js b/services/web/app/src/Features/Collaborators/CollaboratorsController.js index d21da72751..9b1a017b1e 100644 --- a/services/web/app/src/Features/Collaborators/CollaboratorsController.js +++ b/services/web/app/src/Features/Collaborators/CollaboratorsController.js @@ -13,6 +13,10 @@ const { expressify } = require('@overleaf/promise-utils') const { hasAdminAccess } = require('../Helpers/AdminAuthorizationHelper') const TokenAccessHandler = require('../TokenAccess/TokenAccessHandler') const ProjectAuditLogHandler = require('../Project/ProjectAuditLogHandler') +const ProjectGetter = require('../Project/ProjectGetter') +const SplitTestHandler = require('../SplitTests/SplitTestHandler') +const LimitationsManager = require('../Subscription/LimitationsManager') +const PrivilegeLevels = require('../Authorization/PrivilegeLevels') module.exports = { removeUserFromProject: expressify(removeUserFromProject), @@ -75,6 +79,28 @@ async function setCollaboratorInfo(req, res, next) { const projectId = req.params.Project_id const userId = req.params.user_id const { privilegeLevel } = req.body + + if (privilegeLevel !== PrivilegeLevels.READ_ONLY) { + const project = await ProjectGetter.promises.getProject(projectId, { + owner_ref: 1, + }) + const linkSharingChanges = + await SplitTestHandler.promises.getAssignmentForUser( + project.owner_ref, + 'link-sharing-warning' + ) + const allowed = + await LimitationsManager.promises.canAddXEditCollaborators(projectId, 1) + if (linkSharingChanges?.variant === 'active') { + if (!allowed) { + return HttpErrorHandler.forbidden( + req, + res, + 'edit collaborator limit reached' + ) + } + } + } await CollaboratorsHandler.promises.setCollaboratorPrivilegeLevel( projectId, userId, diff --git a/services/web/app/src/Features/Collaborators/CollaboratorsGetter.js b/services/web/app/src/Features/Collaborators/CollaboratorsGetter.js index 080dcce37f..8d053b028d 100644 --- a/services/web/app/src/Features/Collaborators/CollaboratorsGetter.js +++ b/services/web/app/src/Features/Collaborators/CollaboratorsGetter.js @@ -39,6 +39,7 @@ module.exports = { getInvitedMembersWithPrivilegeLevelsFromFields, getMemberIdPrivilegeLevel, getInvitedCollaboratorCount, + getInvitedEditCollaboratorCount, getProjectsUserIsMemberOf, dangerouslyGetAllProjectsUserIsMemberOf, isUserInvitedMemberOfProject, @@ -125,6 +126,16 @@ async function getInvitedCollaboratorCount(projectId) { return count - 1 // Don't count project owner } +async function getInvitedEditCollaboratorCount(projectId) { + // Only counts invited members with readAndWrite privilege + const members = await getMemberIdsWithPrivilegeLevels(projectId) + return members.filter( + m => + m.source === Sources.INVITE && + m.privilegeLevel === PrivilegeLevels.READ_AND_WRITE + ).length +} + async function isUserInvitedMemberOfProject(userId, projectId) { if (!userId) { return false diff --git a/services/web/app/src/Features/Collaborators/CollaboratorsInviteHandler.js b/services/web/app/src/Features/Collaborators/CollaboratorsInviteHandler.js index 8f92742cc0..05540b0389 100644 --- a/services/web/app/src/Features/Collaborators/CollaboratorsInviteHandler.js +++ b/services/web/app/src/Features/Collaborators/CollaboratorsInviteHandler.js @@ -8,6 +8,7 @@ const UserGetter = require('../User/UserGetter') const ProjectGetter = require('../Project/ProjectGetter') const Crypto = require('crypto') const NotificationsBuilder = require('../Notifications/NotificationsBuilder') +const PrivilegeLevels = require('../Authorization/PrivilegeLevels') const randomBytes = promisify(Crypto.randomBytes) @@ -28,6 +29,15 @@ const CollaboratorsInviteHandler = { return count }, + async getEditInviteCount(projectId) { + logger.debug({ projectId }, 'counting edit invites for project') + const count = await ProjectInvite.countDocuments({ + projectId, + privileges: { $ne: PrivilegeLevels.READ_ONLY }, + }).exec() + return count + }, + async _trySendInviteNotification(projectId, sendingUser, invite) { const { email } = invite const existingUser = await UserGetter.promises.getUserByAnyEmail(email, { diff --git a/services/web/app/src/Features/Subscription/LimitationsManager.js b/services/web/app/src/Features/Subscription/LimitationsManager.js index c42289f7fd..bd50f0f002 100644 --- a/services/web/app/src/Features/Subscription/LimitationsManager.js +++ b/services/web/app/src/Features/Subscription/LimitationsManager.js @@ -36,7 +36,24 @@ async function canAddXCollaborators(projectId, numberOfNewCollaborators) { await CollaboratorsInvitesHandler.promises.getInviteCount(projectId) return ( currentNumber + inviteCount + numberOfNewCollaborators <= allowedNumber || - allowedNumber < 0 + allowedNumber < 0 // -1 means unlimited + ) +} + +async function canAddXEditCollaborators( + projectId, + numberOfNewEditCollaborators +) { + const allowedNumber = await allowedNumberOfCollaboratorsInProject(projectId) + const currentEditors = + await CollaboratorsGetter.promises.getInvitedEditCollaboratorCount( + projectId + ) + const editInviteCount = + await CollaboratorsInvitesHandler.promises.getEditInviteCount(projectId) + return ( + currentEditors + editInviteCount + numberOfNewEditCollaborators <= + allowedNumber || allowedNumber < 0 // -1 means unlimited ) } @@ -130,6 +147,7 @@ const LimitationsManager = { allowedNumberOfCollaboratorsForUser ), canAddXCollaborators: callbackify(canAddXCollaborators), + canAddXEditCollaborators: callbackify(canAddXEditCollaborators), hasPaidSubscription: callbackifyMultiResult(hasPaidSubscription, [ 'hasPaidSubscription', 'subscription', @@ -159,6 +177,7 @@ const LimitationsManager = { allowedNumberOfCollaboratorsInProject, allowedNumberOfCollaboratorsForUser, canAddXCollaborators, + canAddXEditCollaborators, hasPaidSubscription, userHasSubscriptionOrIsGroupMember, userHasV2Subscription, diff --git a/services/web/test/unit/src/Collaborators/CollaboratorsControllerTests.js b/services/web/test/unit/src/Collaborators/CollaboratorsControllerTests.js index 25d8594255..25c9d38400 100644 --- a/services/web/test/unit/src/Collaborators/CollaboratorsControllerTests.js +++ b/services/web/test/unit/src/Collaborators/CollaboratorsControllerTests.js @@ -59,6 +59,24 @@ describe('CollaboratorsController', function () { addEntryInBackground: sinon.stub(), } + this.ProjectGetter = { + promises: { + getProject: sinon.stub().resolves({ owner_ref: this.user._id }), + }, + } + + this.SplitTestHandler = { + promises: { + getAssignmentForUser: sinon.stub().resolves({ variant: 'default' }), + }, + } + + this.LimitationsManager = { + promises: { + canAddXEditCollaborators: sinon.stub().resolves(), + }, + } + this.CollaboratorsController = SandboxedModule.require(MODULE_PATH, { requires: { mongodb: { ObjectId }, @@ -71,6 +89,9 @@ describe('CollaboratorsController', function () { '../Authentication/SessionManager': this.SessionManager, '../TokenAccess/TokenAccessHandler': this.TokenAccessHandler, '../Project/ProjectAuditLogHandler': this.ProjectAuditLogHandler, + '../Project/ProjectGetter': this.ProjectGetter, + '../SplitTests/SplitTestHandler': this.SplitTestHandler, + '../Subscription/LimitationsManager': this.LimitationsManager, }, }) }) @@ -271,6 +292,93 @@ describe('CollaboratorsController', function () { this.next ) }) + + describe('when link-sharing-warning test active', function () { + beforeEach(function () { + this.SplitTestHandler.promises.getAssignmentForUser.resolves({ + variant: 'active', + }) + }) + + describe('when setting privilege level to readAndWrite', function () { + beforeEach(function () { + this.req.body = { privilegeLevel: 'readAndWrite' } + }) + + describe('when owner can add new edit collaborators', function () { + beforeEach(function () { + this.LimitationsManager.promises.canAddXEditCollaborators.resolves( + true + ) + }) + + it('should set privilege level after checking collaborators can be added', function (done) { + this.res.sendStatus = status => { + expect(status).to.equal(204) + expect( + this.LimitationsManager.promises.canAddXEditCollaborators + ).to.have.been.calledWith(this.projectId, 1) + done() + } + this.CollaboratorsController.setCollaboratorInfo(this.req, this.res) + }) + }) + + describe('when owner cannot add edit collaborators', function () { + beforeEach(function () { + this.LimitationsManager.promises.canAddXEditCollaborators.resolves( + false + ) + }) + + it('should return a 403 if trying to set a new edit collaborator', function (done) { + this.HttpErrorHandler.forbidden = sinon.spy((req, res) => { + expect(req).to.equal(this.req) + expect(res).to.equal(this.res) + expect( + this.LimitationsManager.promises.canAddXEditCollaborators + ).to.have.been.calledWith(this.projectId, 1) + expect( + this.CollaboratorsHandler.promises.setCollaboratorPrivilegeLevel + ).to.not.have.been.called + done() + }) + this.CollaboratorsController.setCollaboratorInfo(this.req, this.res) + }) + }) + }) + + describe('when setting privilege level to readOnly', function () { + beforeEach(function () { + this.req.body = { privilegeLevel: 'readOnly' } + }) + + describe('when owner cannot add edit collaborators', function () { + beforeEach(function () { + this.LimitationsManager.promises.canAddXEditCollaborators.resolves( + false + ) + }) + + it('should always allow setting a collaborator to viewer even if user cant add edit collaborators', function (done) { + this.res.sendStatus = status => { + expect(status).to.equal(204) + expect(this.LimitationsManager.promises.canAddXEditCollaborators) + .to.not.have.been.called + expect( + this.CollaboratorsHandler.promises.setCollaboratorPrivilegeLevel + ).to.have.been.calledWith( + this.projectId, + this.user._id, + 'readOnly' + ) + done() + } + this.CollaboratorsController.setCollaboratorInfo(this.req, this.res) + }) + }) + }) + }) }) describe('transferOwnership', function () { diff --git a/services/web/test/unit/src/Collaborators/CollaboratorsGetterTests.js b/services/web/test/unit/src/Collaborators/CollaboratorsGetterTests.js index 96cb770b19..ab00c03ce9 100644 --- a/services/web/test/unit/src/Collaborators/CollaboratorsGetterTests.js +++ b/services/web/test/unit/src/Collaborators/CollaboratorsGetterTests.js @@ -474,4 +474,14 @@ describe('CollaboratorsGetter', function () { }) }) }) + + describe('getInvitedEditCollaboratorCount', function () { + it('should return the count of invited edit collaborators (token, readAndWrite)', async function () { + const count = + await this.CollaboratorsGetter.promises.getInvitedEditCollaboratorCount( + this.project._id + ) + expect(count).to.equal(2) + }) + }) }) diff --git a/services/web/test/unit/src/Collaborators/CollaboratorsInviteHandlerTests.js b/services/web/test/unit/src/Collaborators/CollaboratorsInviteHandlerTests.js index 9823808c26..c575b83e10 100644 --- a/services/web/test/unit/src/Collaborators/CollaboratorsInviteHandlerTests.js +++ b/services/web/test/unit/src/Collaborators/CollaboratorsInviteHandlerTests.js @@ -117,6 +117,40 @@ describe('CollaboratorsInviteHandler', function () { }) }) + describe('getEditInviteCount', function () { + beforeEach(function () { + this.ProjectInvite.countDocuments.returns({ + exec: sinon.stub().resolves(2), + }) + this.call = async () => { + return await this.CollaboratorsInviteHandler.promises.getEditInviteCount( + this.projectId + ) + } + }) + + it('should produce the count of documents', async function () { + const count = await this.call() + expect(this.ProjectInvite.countDocuments).to.be.calledWith({ + projectId: this.projectId, + privileges: { $ne: 'readOnly' }, + }) + expect(count).to.equal(2) + }) + + describe('when model.countDocuments produces an error', function () { + beforeEach(function () { + this.ProjectInvite.countDocuments.returns({ + exec: sinon.stub().rejects(new Error('woops')), + }) + }) + + it('should produce an error', async function () { + await expect(this.call()).to.be.rejectedWith(Error) + }) + }) + }) + describe('getAllInvites', function () { beforeEach(function () { this.fakeInvites = [ diff --git a/services/web/test/unit/src/Subscription/LimitationsManagerTests.js b/services/web/test/unit/src/Subscription/LimitationsManagerTests.js index 6a78249229..e438e37a8a 100644 --- a/services/web/test/unit/src/Subscription/LimitationsManagerTests.js +++ b/services/web/test/unit/src/Subscription/LimitationsManagerTests.js @@ -54,11 +54,17 @@ describe('LimitationsManager', function () { } this.CollaboratorsGetter = { - promises: { getInvitedCollaboratorCount: sinon.stub().resolves() }, + promises: { + getInvitedCollaboratorCount: sinon.stub().resolves(), + getInvitedEditCollaboratorCount: sinon.stub().resolves(), + }, } this.CollaboratorsInviteHandler = { - promises: { getInviteCount: sinon.stub().resolves() }, + promises: { + getInviteCount: sinon.stub().resolves(), + getEditInviteCount: sinon.stub().resolves(), + }, } this.LimitationsManager = SandboxedModule.require(modulePath, { @@ -320,6 +326,168 @@ describe('LimitationsManager', function () { }) }) }) + describe('canAddXEditCollaborators', function () { + describe('when the project has fewer collaborators than allowed', function () { + beforeEach(function (done) { + this.current_number = 1 + this.user.features.collaborators = 2 + this.invite_count = 0 + this.CollaboratorsGetter.promises.getInvitedEditCollaboratorCount = + sinon.stub().resolves(this.current_number) + this.CollaboratorsInviteHandler.promises.getEditInviteCount = sinon + .stub() + .resolves(this.invite_count) + this.callback = sinon.stub().callsFake(() => done()) + this.LimitationsManager.canAddXEditCollaborators( + this.projectId, + 1, + this.callback + ) + }) + + it('should return true', function () { + this.callback.calledWith(null, true).should.equal(true) + }) + }) + + describe('when the project has fewer collaborators and invites than allowed', function () { + beforeEach(function (done) { + this.current_number = 1 + this.user.features.collaborators = 4 + this.invite_count = 1 + this.CollaboratorsGetter.promises.getInvitedEditCollaboratorCount = + sinon.stub().resolves(this.current_number) + this.CollaboratorsInviteHandler.promises.getEditInviteCount = sinon + .stub() + .resolves(this.invite_count) + this.callback = sinon.stub().callsFake(() => done()) + this.LimitationsManager.canAddXEditCollaborators( + this.projectId, + 1, + this.callback + ) + }) + + it('should return true', function () { + this.callback.calledWith(null, true).should.equal(true) + }) + }) + + describe('when the project has fewer collaborators than allowed but I want to add more than allowed', function () { + beforeEach(function (done) { + this.current_number = 1 + this.user.features.collaborators = 2 + this.invite_count = 0 + this.CollaboratorsGetter.promises.getInvitedEditCollaboratorCount = + sinon.stub().resolves(this.current_number) + this.CollaboratorsInviteHandler.promises.getEditInviteCount = sinon + .stub() + .resolves(this.invite_count) + this.callback = sinon.stub().callsFake(() => done()) + this.LimitationsManager.canAddXEditCollaborators( + this.projectId, + 2, + this.callback + ) + }) + + it('should return false', function () { + this.callback.calledWith(null, false).should.equal(true) + }) + }) + + describe('when the project has more collaborators than allowed', function () { + beforeEach(function (done) { + this.current_number = 3 + this.user.features.collaborators = 2 + this.invite_count = 0 + this.CollaboratorsGetter.promises.getInvitedEditCollaboratorCount = + sinon.stub().resolves(this.current_number) + this.CollaboratorsInviteHandler.promises.getEditInviteCount = sinon + .stub() + .resolves(this.invite_count) + this.callback = sinon.stub().callsFake(() => done()) + this.LimitationsManager.canAddXEditCollaborators( + this.projectId, + 1, + this.callback + ) + }) + + it('should return false', function () { + this.callback.calledWith(null, false).should.equal(true) + }) + }) + + describe('when the project has infinite collaborators', function () { + beforeEach(function (done) { + this.current_number = 100 + this.user.features.collaborators = -1 + this.invite_count = 0 + this.CollaboratorsGetter.promises.getInvitedEditCollaboratorCount = + sinon.stub().resolves(this.current_number) + this.CollaboratorsInviteHandler.promises.getEditInviteCount = sinon + .stub() + .resolves(this.invite_count) + this.callback = sinon.stub().callsFake(() => done()) + this.LimitationsManager.canAddXEditCollaborators( + this.projectId, + 1, + this.callback + ) + }) + + it('should return true', function () { + this.callback.calledWith(null, true).should.equal(true) + }) + }) + + describe('when the project has more invites than allowed', function () { + beforeEach(function (done) { + this.current_number = 0 + this.user.features.collaborators = 2 + this.invite_count = 2 + this.CollaboratorsGetter.promises.getInvitedEditCollaboratorCount = + sinon.stub().resolves(this.current_number) + this.CollaboratorsInviteHandler.promises.getEditInviteCount = sinon + .stub() + .resolves(this.invite_count) + this.callback = sinon.stub().callsFake(() => done()) + this.LimitationsManager.canAddXEditCollaborators( + this.projectId, + 1, + this.callback + ) + }) + + it('should return false', function () { + this.callback.calledWith(null, false).should.equal(true) + }) + }) + + describe('when the project has more invites and collaborators than allowed', function () { + beforeEach(function (done) { + this.current_number = 1 + this.user.features.collaborators = 2 + this.invite_count = 1 + this.CollaboratorsGetter.promises.getInvitedEditCollaboratorCount = + sinon.stub().resolves(this.current_number) + this.CollaboratorsInviteHandler.promises.getEditInviteCount = sinon + .stub() + .resolves(this.invite_count) + this.callback = sinon.stub().callsFake(() => done()) + this.LimitationsManager.canAddXEditCollaborators( + this.projectId, + 1, + this.callback + ) + }) + + it('should return false', function () { + this.callback.calledWith(null, false).should.equal(true) + }) + }) + }) describe('userHasV2Subscription', function () { beforeEach(function () {