From abf04c5d6c94bf29c595a8d435161ac9150d865c Mon Sep 17 00:00:00 2001 From: Thomas Date: Thu, 22 Aug 2024 12:49:35 +0200 Subject: [PATCH] Merge pull request #20036 from overleaf/tm-collab-limit-link-sharing Enforce collaborator limit for link sharing GitOrigin-RevId: b724dca0c616ef15e5bd6d07e9d898d34dd46acd --- .../Collaborators/CollaboratorsHandler.js | 10 +- .../TokenAccess/TokenAccessController.js | 53 +++- .../CollaboratorsHandlerTests.js | 30 ++ .../TokenAccess/TokenAccessControllerTests.js | 262 +++++++++++++++++- 4 files changed, 343 insertions(+), 12 deletions(-) diff --git a/services/web/app/src/Features/Collaborators/CollaboratorsHandler.js b/services/web/app/src/Features/Collaborators/CollaboratorsHandler.js index e4f795ac6a..10dd76dd7a 100644 --- a/services/web/app/src/Features/Collaborators/CollaboratorsHandler.js +++ b/services/web/app/src/Features/Collaborators/CollaboratorsHandler.js @@ -230,7 +230,8 @@ async function transferProjects(fromUserId, toUserId) { async function setCollaboratorPrivilegeLevel( projectId, userId, - privilegeLevel + privilegeLevel, + { pendingEditor } = {} ) { // Make sure we're only updating the project if the user is already a // collaborator @@ -249,9 +250,14 @@ async function setCollaboratorPrivilegeLevel( } case PrivilegeLevels.READ_ONLY: { update = { - $pull: { collaberator_refs: userId, pendingEditor_refs: userId }, + $pull: { collaberator_refs: userId }, $addToSet: { readOnly_refs: userId }, } + if (pendingEditor) { + update.$addToSet.pendingEditor_refs = userId + } else { + update.$pull.pendingEditor_refs = userId + } break } default: { diff --git a/services/web/app/src/Features/TokenAccess/TokenAccessController.js b/services/web/app/src/Features/TokenAccess/TokenAccessController.js index 26fed096ed..1cc5437d31 100644 --- a/services/web/app/src/Features/TokenAccess/TokenAccessController.js +++ b/services/web/app/src/Features/TokenAccess/TokenAccessController.js @@ -22,6 +22,7 @@ const { const { getSafeAdminDomainRedirect } = require('../Helpers/UrlHelper') const UserGetter = require('../User/UserGetter') const Settings = require('@overleaf/settings') +const LimitationsManager = require('../Subscription/LimitationsManager') const orderedPrivilegeLevels = [ PrivilegeLevels.NONE, @@ -333,27 +334,43 @@ async function grantTokenAccessReadAndWrite(req, res, next) { }) } + const linkSharingEnforcement = + await SplitTestHandler.promises.getAssignmentForUser( + project.owner_ref, + 'link-sharing-enforcement' + ) + const pendingEditor = + linkSharingEnforcement?.variant === 'active' && + !(await LimitationsManager.promises.canAcceptEditCollaboratorInvite( + project._id + )) await ProjectAuditLogHandler.promises.addEntry( project._id, 'accept-via-link-sharing', userId, req.ip, - { privileges: 'readAndWrite' } + { + privileges: pendingEditor ? 'readOnly' : 'readAndWrite', + ...(pendingEditor && { pendingEditor: true }), + } ) AnalyticsManager.recordEventForUserInBackground( userId, 'project-joined', { - mode: 'read-write', + mode: pendingEditor ? 'read-only' : 'read-write', projectId: project._id.toString(), + ...(pendingEditor && { pendingEditor: true }), } ) - // Currently does not enforce the collaborator limit (warning phase) await CollaboratorsHandler.promises.addUserIdToProject( project._id, undefined, userId, - PrivilegeLevels.READ_AND_WRITE + pendingEditor + ? PrivilegeLevels.READ_ONLY + : PrivilegeLevels.READ_AND_WRITE, + { pendingEditor } ) // Does not remove any pending invite or the invite notification // Should be a noop if the user is already a member, @@ -536,20 +553,36 @@ async function sharingUpdatesConsent(req, res, next) { async function moveReadWriteToCollaborators(req, res, next) { const { Project_id: projectId } = req.params const userId = SessionManager.getLoggedInUserId(req.session) + const project = await ProjectGetter.promises.getProject(projectId, { + owner_ref: 1, + }) const isInvitedMember = await CollaboratorsGetter.promises.isUserInvitedMemberOfProject( userId, projectId ) + const linkSharingEnforcement = + await SplitTestHandler.promises.getAssignmentForUser( + project.owner_ref, + 'link-sharing-enforcement' + ) + const pendingEditor = + linkSharingEnforcement?.variant === 'active' && + !(await LimitationsManager.promises.canAcceptEditCollaboratorInvite( + project._id + )) await ProjectAuditLogHandler.promises.addEntry( projectId, 'accept-via-link-sharing', userId, req.ip, { - privileges: 'readAndWrite', + privileges: pendingEditor + ? PrivilegeLevels.READ_ONLY + : PrivilegeLevels.READ_AND_WRITE, tokenMember: true, invitedMember: isInvitedMember, + ...(pendingEditor && { pendingEditor: true }), } ) if (isInvitedMember) { @@ -561,7 +594,10 @@ async function moveReadWriteToCollaborators(req, res, next) { await CollaboratorsHandler.promises.setCollaboratorPrivilegeLevel( projectId, userId, - PrivilegeLevels.READ_AND_WRITE + pendingEditor + ? PrivilegeLevels.READ_ONLY + : PrivilegeLevels.READ_AND_WRITE, + { pendingEditor } ) } else { // Normal case, not invited, joining via link sharing @@ -573,7 +609,10 @@ async function moveReadWriteToCollaborators(req, res, next) { projectId, undefined, userId, - PrivilegeLevels.READ_AND_WRITE + pendingEditor + ? PrivilegeLevels.READ_ONLY + : PrivilegeLevels.READ_AND_WRITE, + { pendingEditor } ) } EditorRealTimeController.emitToRoom(projectId, 'project:membership:changed', { diff --git a/services/web/test/unit/src/Collaborators/CollaboratorsHandlerTests.js b/services/web/test/unit/src/Collaborators/CollaboratorsHandlerTests.js index a333a75a46..a36b438578 100644 --- a/services/web/test/unit/src/Collaborators/CollaboratorsHandlerTests.js +++ b/services/web/test/unit/src/Collaborators/CollaboratorsHandlerTests.js @@ -592,6 +592,36 @@ describe('CollaboratorsHandler', function () { ) }) + it('sets a collaborator to read-only as a pendingEditor', async function () { + this.ProjectMock.expects('updateOne') + .withArgs( + { + _id: this.projectId, + $or: [ + { collaberator_refs: this.userId }, + { readOnly_refs: this.userId }, + ], + }, + { + $addToSet: { + readOnly_refs: this.userId, + pendingEditor_refs: this.userId, + }, + $pull: { + collaberator_refs: this.userId, + }, + } + ) + .chain('exec') + .resolves({ matchedCount: 1 }) + await this.CollaboratorsHandler.promises.setCollaboratorPrivilegeLevel( + this.projectId, + this.userId, + 'readOnly', + { pendingEditor: true } + ) + }) + it('throws a NotFoundError if the project or collaborator does not exist', async function () { this.ProjectMock.expects('updateOne') .chain('exec') diff --git a/services/web/test/unit/src/TokenAccess/TokenAccessControllerTests.js b/services/web/test/unit/src/TokenAccess/TokenAccessControllerTests.js index 9749cd14a1..e5b0db2f39 100644 --- a/services/web/test/unit/src/TokenAccess/TokenAccessControllerTests.js +++ b/services/web/test/unit/src/TokenAccess/TokenAccessControllerTests.js @@ -123,6 +123,12 @@ describe('TokenAccessController', function () { }, } + this.LimitationsManager = { + promises: { + canAcceptEditCollaboratorInvite: sinon.stub().resolves(), + }, + } + this.TokenAccessController = SandboxedModule.require(MODULE_PATH, { requires: { '@overleaf/settings': this.Settings, @@ -145,6 +151,7 @@ describe('TokenAccessController', function () { }), '../Analytics/AnalyticsManager': this.AnalyticsManager, '../User/UserGetter': this.UserGetter, + '../Subscription/LimitationsManager': this.LimitationsManager, }, }) }) @@ -195,9 +202,13 @@ describe('TokenAccessController', function () { describe('when project owner in link-sharing-warning split test', function () { beforeEach(function () { - this.SplitTestHandler.promises.getAssignmentForUser.resolves({ - variant: 'active', - }) + this.SplitTestHandler.promises.getAssignmentForUser.callsFake( + async (userId, test) => { + if (test === 'link-sharing-warning') { + return { variant: 'active' } + } + } + ) }) it('tells the ui to show the link-sharing-warning variant', async function () { @@ -284,6 +295,168 @@ describe('TokenAccessController', function () { ) }) }) + + describe('when the project owner is in the link-sharing-enforcement split test', function () { + beforeEach(function () { + this.SplitTestHandler.promises.getAssignmentForUser.callsFake( + async (userId, test) => { + if (test === 'link-sharing-warning') { + return { variant: 'active' } + } else if (test === 'link-sharing-enforcement') { + return { variant: 'active' } + } + } + ) + }) + + describe('normal case (edit slot available)', function () { + beforeEach(function (done) { + this.LimitationsManager.promises.canAcceptEditCollaboratorInvite.resolves( + true + ) + this.req.params = { token: this.token } + this.req.body = { + confirmedByUser: true, + tokenHashPrefix: '#prefix', + } + this.res.callback = done + this.TokenAccessController.grantTokenAccessReadAndWrite( + this.req, + this.res, + done + ) + }) + + it('adds the user as a read and write invited member', function () { + expect( + this.CollaboratorsHandler.promises.addUserIdToProject + ).to.have.been.calledWith( + this.project._id, + undefined, + this.user._id, + PrivilegeLevels.READ_AND_WRITE + ) + }) + + it('writes a project audit log', function () { + expect( + this.ProjectAuditLogHandler.promises.addEntry + ).to.have.been.calledWith( + this.project._id, + 'accept-via-link-sharing', + this.user._id, + this.req.ip, + { privileges: 'readAndWrite' } + ) + }) + + it('records a project-joined event for the user', function () { + expect( + this.AnalyticsManager.recordEventForUserInBackground + ).to.have.been.calledWith(this.user._id, 'project-joined', { + mode: 'read-write', + projectId: this.project._id.toString(), + }) + }) + + it('emits a project membership changed event', function () { + expect( + this.EditorRealTimeController.emitToRoom + ).to.have.been.calledWith( + this.project._id, + 'project:membership:changed', + { members: true } + ) + }) + + it('checks token hash', function () { + expect( + this.TokenAccessHandler.checkTokenHashPrefix + ).to.have.been.calledWith( + this.token, + '#prefix', + 'readAndWrite', + this.user._id, + { projectId: this.project._id, action: 'continue' } + ) + }) + }) + + describe('when there are no edit collaborator slots available', function () { + beforeEach(function (done) { + this.LimitationsManager.promises.canAcceptEditCollaboratorInvite.resolves( + false + ) + this.req.params = { token: this.token } + this.req.body = { + confirmedByUser: true, + tokenHashPrefix: '#prefix', + } + this.res.callback = done + this.TokenAccessController.grantTokenAccessReadAndWrite( + this.req, + this.res, + done + ) + }) + + it('adds the user as a read only invited member instead (pendingEditor)', function () { + expect( + this.CollaboratorsHandler.promises.addUserIdToProject + ).to.have.been.calledWith( + this.project._id, + undefined, + this.user._id, + PrivilegeLevels.READ_ONLY, + { pendingEditor: true } + ) + }) + + it('writes a project audit log', function () { + expect( + this.ProjectAuditLogHandler.promises.addEntry + ).to.have.been.calledWith( + this.project._id, + 'accept-via-link-sharing', + this.user._id, + this.req.ip, + { privileges: 'readOnly', pendingEditor: true } + ) + }) + + it('records a project-joined event for the user', function () { + expect( + this.AnalyticsManager.recordEventForUserInBackground + ).to.have.been.calledWith(this.user._id, 'project-joined', { + mode: 'read-only', + projectId: this.project._id.toString(), + pendingEditor: true, + }) + }) + + it('emits a project membership changed event', function () { + expect( + this.EditorRealTimeController.emitToRoom + ).to.have.been.calledWith( + this.project._id, + 'project:membership:changed', + { members: true } + ) + }) + + it('checks token hash', function () { + expect( + this.TokenAccessHandler.checkTokenHashPrefix + ).to.have.been.calledWith( + this.token, + '#prefix', + 'readAndWrite', + this.user._id, + { projectId: this.project._id, action: 'continue' } + ) + }) + }) + }) }) describe('when the access was already granted', function () { @@ -994,6 +1167,89 @@ describe('TokenAccessController', function () { expect(this.res.sendStatus).to.have.been.calledWith(204) }) }) + + describe('when link-sharing-enforcement test is active', function () { + beforeEach(function () { + this.SplitTestHandler.promises.getAssignmentForUser.resolves({ + variant: 'active', + }) + }) + + describe('when there are collaborator slots available', function () { + beforeEach(function () { + this.LimitationsManager.promises.canAcceptEditCollaboratorInvite.resolves( + true + ) + }) + + describe('previously joined token access user moving to named collaborator', function () { + beforeEach(function (done) { + this.CollaboratorsGetter.promises.isUserInvitedMemberOfProject.resolves( + false + ) + this.res.callback = done + this.TokenAccessController.moveReadWriteToCollaborators( + this.req, + this.res, + done + ) + }) + + it('sets the privilege level to read and write for the invited viewer', function () { + expect( + this.TokenAccessHandler.promises.removeReadAndWriteUserFromProject + ).to.have.been.calledWith(this.user._id, this.project._id) + expect( + this.CollaboratorsHandler.promises.addUserIdToProject + ).to.have.been.calledWith( + this.project._id, + undefined, + this.user._id, + PrivilegeLevels.READ_AND_WRITE + ) + expect(this.res.sendStatus).to.have.been.calledWith(204) + }) + }) + }) + + describe('when there are no edit collaborator slots available', function () { + beforeEach(function () { + this.LimitationsManager.promises.canAcceptEditCollaboratorInvite.resolves( + false + ) + }) + + describe('previously joined token access user moving to named collaborator', function () { + beforeEach(function (done) { + this.CollaboratorsGetter.promises.isUserInvitedMemberOfProject.resolves( + false + ) + this.res.callback = done + this.TokenAccessController.moveReadWriteToCollaborators( + this.req, + this.res, + done + ) + }) + + it('sets the privilege level to read only for the invited viewer (pendingEditor)', function () { + expect( + this.TokenAccessHandler.promises.removeReadAndWriteUserFromProject + ).to.have.been.calledWith(this.user._id, this.project._id) + expect( + this.CollaboratorsHandler.promises.addUserIdToProject + ).to.have.been.calledWith( + this.project._id, + undefined, + this.user._id, + PrivilegeLevels.READ_ONLY, + { pendingEditor: true } + ) + expect(this.res.sendStatus).to.have.been.calledWith(204) + }) + }) + }) + }) }) describe('moveReadWriteToReadOnly', function () {