From 71a0b48a68cd9707cd657286458c1396f925207d Mon Sep 17 00:00:00 2001 From: Domagoj Kriskovic Date: Wed, 11 Dec 2024 10:51:39 +0100 Subject: [PATCH] Set track changes state permissions for reviewer role (#22345) (#22436) * Support for adding reviewer role * added collaboratorsGetter tests * emit toggle-track-changes when reviewer is added * Add reviewer in change privilege level handler GitOrigin-RevId: 88ec39f2b760b5d1ca6dc3a363df31c087268972 --- .../Collaborators/CollaboratorsHandler.js | 25 +++++- .../Collaborators/CollaboratorsRouter.mjs | 6 +- .../Features/Helpers/AuthorizationHelper.js | 32 ++++++++ .../restricted-link-sharing/edit-member.tsx | 1 + services/web/locales/en.json | 1 + .../web/test/acceptance/src/SharingTests.mjs | 50 ++++++++++++ .../web/test/acceptance/src/helpers/User.js | 4 + .../CollaboratorsHandlerTests.js | 35 +++++++++ .../HelperFiles/AuthorizationHelperTests.js | 76 +++++++++++++++++++ 9 files changed, 226 insertions(+), 4 deletions(-) diff --git a/services/web/app/src/Features/Collaborators/CollaboratorsHandler.js b/services/web/app/src/Features/Collaborators/CollaboratorsHandler.js index 2a696beaff..7247c9170d 100644 --- a/services/web/app/src/Features/Collaborators/CollaboratorsHandler.js +++ b/services/web/app/src/Features/Collaborators/CollaboratorsHandler.js @@ -284,20 +284,39 @@ async function setCollaboratorPrivilegeLevel( // collaborator const query = { _id: projectId, - $or: [{ collaberator_refs: userId }, { readOnly_refs: userId }], + $or: [ + { collaberator_refs: userId }, + { readOnly_refs: userId }, + { reviewer_refs: userId }, + ], } let update switch (privilegeLevel) { case PrivilegeLevels.READ_AND_WRITE: { update = { - $pull: { readOnly_refs: userId, pendingEditor_refs: userId }, + $pull: { + readOnly_refs: userId, + pendingEditor_refs: userId, + reviewer_refs: userId, + }, $addToSet: { collaberator_refs: userId }, } break } + case PrivilegeLevels.REVIEW: { + update = { + $pull: { + readOnly_refs: userId, + pendingEditor_refs: userId, + collaberator_refs: userId, + }, + $addToSet: { reviewer_refs: userId }, + } + break + } case PrivilegeLevels.READ_ONLY: { update = { - $pull: { collaberator_refs: userId }, + $pull: { collaberator_refs: userId, reviewer_refs: userId }, $addToSet: { readOnly_refs: userId }, } if (pendingEditor) { diff --git a/services/web/app/src/Features/Collaborators/CollaboratorsRouter.mjs b/services/web/app/src/Features/Collaborators/CollaboratorsRouter.mjs index 4ff5097124..a057e1d8fe 100644 --- a/services/web/app/src/Features/Collaborators/CollaboratorsRouter.mjs +++ b/services/web/app/src/Features/Collaborators/CollaboratorsRouter.mjs @@ -50,7 +50,11 @@ export default { }), body: Joi.object({ privilegeLevel: Joi.string() - .valid(PrivilegeLevels.READ_ONLY, PrivilegeLevels.READ_AND_WRITE) + .valid( + PrivilegeLevels.READ_ONLY, + PrivilegeLevels.READ_AND_WRITE, + PrivilegeLevels.REVIEW + ) .required(), }), }), diff --git a/services/web/app/src/Features/Helpers/AuthorizationHelper.js b/services/web/app/src/Features/Helpers/AuthorizationHelper.js index 8369f2d321..d8e6bd932d 100644 --- a/services/web/app/src/Features/Helpers/AuthorizationHelper.js +++ b/services/web/app/src/Features/Helpers/AuthorizationHelper.js @@ -1,7 +1,14 @@ const { UserSchema } = require('../../models/User') +const SplitTestHandler = require('../SplitTests/SplitTestHandler') +const ProjectGetter = require('../Project/ProjectGetter') +const { callbackify } = require('@overleaf/promise-utils') module.exports = { hasAnyStaffAccess, + isReviewerRoleEnabled: callbackify(isReviewerRoleEnabled), + promises: { + isReviewerRoleEnabled, + }, } function hasAnyStaffAccess(user) { @@ -14,3 +21,28 @@ function hasAnyStaffAccess(user) { } return false } + +async function isReviewerRoleEnabled(userId, projectId) { + const project = await ProjectGetter.promises.getProject(projectId, { + reviewer_refs: 1, + owner_ref: 1, + }) + + // if there are reviewers, it means the role is enabled + if (Object.keys(project.reviewer_refs || {}).length > 0) { + return true + } + + // if there are no reviewers, check split test from project owner + if (project.owner_ref === userId) { + const reviewerRoleAssigment = + await SplitTestHandler.promises.getAssignmentForUser( + userId, + 'reviewer-role' + ) + + return reviewerRoleAssigment.variant === 'enabled' + } + + return false +} diff --git a/services/web/frontend/js/features/share-project-modal/components/restricted-link-sharing/edit-member.tsx b/services/web/frontend/js/features/share-project-modal/components/restricted-link-sharing/edit-member.tsx index 1189176b42..635073b840 100644 --- a/services/web/frontend/js/features/share-project-modal/components/restricted-link-sharing/edit-member.tsx +++ b/services/web/frontend/js/features/share-project-modal/components/restricted-link-sharing/edit-member.tsx @@ -92,6 +92,7 @@ export default function EditMember({ ) } else if ( newPrivileges === 'readAndWrite' || + newPrivileges === 'review' || newPrivileges === 'readOnly' ) { monitorRequest(() => diff --git a/services/web/locales/en.json b/services/web/locales/en.json index 41d8cf144f..d6259fbd96 100644 --- a/services/web/locales/en.json +++ b/services/web/locales/en.json @@ -1824,6 +1824,7 @@ "revert_pending_plan_change": "Revert scheduled plan change", "review": "Review", "review_your_peers_work": "Review your peers’ work", + "reviewer": "Reviewer", "revoke": "Revoke", "revoke_invite": "Revoke Invite", "right": "Right", diff --git a/services/web/test/acceptance/src/SharingTests.mjs b/services/web/test/acceptance/src/SharingTests.mjs index aa537d76b6..f644c39180 100644 --- a/services/web/test/acceptance/src/SharingTests.mjs +++ b/services/web/test/acceptance/src/SharingTests.mjs @@ -8,12 +8,15 @@ describe('Sharing', function () { this.ownerSession = new User() this.collaboratorSession = new User() this.strangerSession = new User() + this.reviewerSession = new User() await this.ownerSession.login() await this.collaboratorSession.login() await this.strangerSession.login() + await this.reviewerSession.login() this.owner = await this.ownerSession.get() this.collaborator = await this.collaboratorSession.get() this.stranger = await this.strangerSession.get() + this.reviewer = await this.reviewerSession.get() this.projectId = await this.ownerSession.createProject('Test project') }) @@ -35,6 +38,19 @@ describe('Sharing', function () { const project = await this.ownerSession.getProject(this.projectId) expect(project.collaberator_refs).to.deep.equal([this.collaborator._id]) expect(project.readOnly_refs).to.deep.equal([]) + expect(project.reviewer_refs).to.deep.equal([]) + }) + + it('sets the privilege level to review', async function () { + await this.ownerSession.setCollaboratorInfo( + this.projectId, + this.collaborator._id, + { privilegeLevel: 'review' } + ) + const project = await this.ownerSession.getProject(this.projectId) + expect(project.reviewer_refs).to.deep.equal([this.collaborator._id]) + expect(project.collaberator_refs).to.deep.equal([]) + expect(project.readOnly_refs).to.deep.equal([]) }) it('treats setting the privilege to read-only as a noop', async function () { @@ -45,6 +61,7 @@ describe('Sharing', function () { ) const project = await this.ownerSession.getProject(this.projectId) expect(project.collaberator_refs).to.deep.equal([]) + expect(project.reviewer_refs).to.deep.equal([]) expect(project.readOnly_refs).to.deep.equal([this.collaborator._id]) }) @@ -96,7 +113,40 @@ describe('Sharing', function () { ) const project = await this.ownerSession.getProject(this.projectId) expect(project.collaberator_refs).to.deep.equal([]) + expect(project.reviewer_refs).to.deep.equal([]) expect(project.readOnly_refs).to.deep.equal([this.collaborator._id]) }) }) + + describe('with reviewer collaborator', function () { + beforeEach(async function () { + await this.ownerSession.addUserToProject( + this.projectId, + this.reviewer, + 'review' + ) + }) + + it('prevents non-owners to set the privilege level', async function () { + await expect( + this.collaboratorSession.setCollaboratorInfo( + this.projectId, + this.reviewer._id, + { privilegeLevel: 'review' } + ) + ).to.be.rejectedWith(/failed: status=403 /) + }) + + it('sets the privilege level to read-only', async function () { + await this.ownerSession.setCollaboratorInfo( + this.projectId, + this.reviewer._id, + { privilegeLevel: 'readOnly' } + ) + const project = await this.ownerSession.getProject(this.projectId) + expect(project.collaberator_refs).to.deep.equal([]) + expect(project.reviewer_refs).to.deep.equal([]) + expect(project.readOnly_refs).to.deep.equal([this.reviewer._id]) + }) + }) }) diff --git a/services/web/test/acceptance/src/helpers/User.js b/services/web/test/acceptance/src/helpers/User.js index fa4ab677ee..75a8d287ac 100644 --- a/services/web/test/acceptance/src/helpers/User.js +++ b/services/web/test/acceptance/src/helpers/User.js @@ -968,6 +968,10 @@ class User { updateOp = { $addToSet: { readOnly_refs: user._id, pendingEditor_refs: user._id }, } + } else if (privileges === 'review') { + updateOp = { + $addToSet: { reviewer_refs: user._id }, + } } db.projects.updateOne({ _id: new ObjectId(projectId) }, updateOp, callback) } diff --git a/services/web/test/unit/src/Collaborators/CollaboratorsHandlerTests.js b/services/web/test/unit/src/Collaborators/CollaboratorsHandlerTests.js index aec7ab0e98..8e14590b0f 100644 --- a/services/web/test/unit/src/Collaborators/CollaboratorsHandlerTests.js +++ b/services/web/test/unit/src/Collaborators/CollaboratorsHandlerTests.js @@ -590,12 +590,14 @@ describe('CollaboratorsHandler', function () { $or: [ { collaberator_refs: this.userId }, { readOnly_refs: this.userId }, + { reviewer_refs: this.userId }, ], }, { $pull: { collaberator_refs: this.userId, pendingEditor_refs: this.userId, + reviewer_refs: this.userId, }, $addToSet: { readOnly_refs: this.userId }, } @@ -617,12 +619,14 @@ describe('CollaboratorsHandler', function () { $or: [ { collaberator_refs: this.userId }, { readOnly_refs: this.userId }, + { reviewer_refs: this.userId }, ], }, { $addToSet: { collaberator_refs: this.userId }, $pull: { readOnly_refs: this.userId, + reviewer_refs: this.userId, pendingEditor_refs: this.userId, }, } @@ -636,6 +640,35 @@ describe('CollaboratorsHandler', function () { ) }) + it('sets a collaborator to reviewer', async function () { + this.ProjectMock.expects('updateOne') + .withArgs( + { + _id: this.projectId, + $or: [ + { collaberator_refs: this.userId }, + { readOnly_refs: this.userId }, + { reviewer_refs: this.userId }, + ], + }, + { + $addToSet: { reviewer_refs: this.userId }, + $pull: { + readOnly_refs: this.userId, + collaberator_refs: this.userId, + pendingEditor_refs: this.userId, + }, + } + ) + .chain('exec') + .resolves({ matchedCount: 1 }) + await this.CollaboratorsHandler.promises.setCollaboratorPrivilegeLevel( + this.projectId, + this.userId, + 'review' + ) + }) + it('sets a collaborator to read-only as a pendingEditor', async function () { this.ProjectMock.expects('updateOne') .withArgs( @@ -644,6 +677,7 @@ describe('CollaboratorsHandler', function () { $or: [ { collaberator_refs: this.userId }, { readOnly_refs: this.userId }, + { reviewer_refs: this.userId }, ], }, { @@ -653,6 +687,7 @@ describe('CollaboratorsHandler', function () { }, $pull: { collaberator_refs: this.userId, + reviewer_refs: this.userId, }, } ) diff --git a/services/web/test/unit/src/HelperFiles/AuthorizationHelperTests.js b/services/web/test/unit/src/HelperFiles/AuthorizationHelperTests.js index 7a887e2beb..4aa21ef7a9 100644 --- a/services/web/test/unit/src/HelperFiles/AuthorizationHelperTests.js +++ b/services/web/test/unit/src/HelperFiles/AuthorizationHelperTests.js @@ -25,6 +25,10 @@ describe('AuthorizationHelper', function () { }, }, }, + '../Project/ProjectGetter': (this.ProjectGetter = { promises: {} }), + '../SplitTests/SplitTestHandler': (this.SplitTestHandler = { + promises: {}, + }), }, }) }) @@ -59,4 +63,76 @@ describe('AuthorizationHelper', function () { expect(this.AuthorizationHelper.hasAnyStaffAccess(user)).to.be.false }) }) + + describe('isReviewerRoleEnabled', function () { + it('with no reviewers and no split test', async function () { + this.ProjectGetter.promises.getProject = sinon.stub().resolves({ + reviewer_refs: {}, + owner_ref: 'ownerId', + }) + this.SplitTestHandler.promises.getAssignmentForUser = sinon + .stub() + .resolves({ + variant: 'disabled', + }) + expect( + await this.AuthorizationHelper.promises.isReviewerRoleEnabled( + 'userId', + 'projectId' + ) + ).to.be.false + }) + + it('with no reviewers and enabled split test', async function () { + this.ProjectGetter.promises.getProject = sinon.stub().resolves({ + reviewer_refs: {}, + owner_ref: 'userId', + }) + this.SplitTestHandler.promises.getAssignmentForUser = sinon + .stub() + .resolves({ + variant: 'enabled', + }) + expect( + await this.AuthorizationHelper.promises.isReviewerRoleEnabled( + 'userId', + 'projectId' + ) + ).to.be.true + }) + + it('with reviewers and disabled split test', async function () { + this.ProjectGetter.promises.getProject = sinon.stub().resolves({ + reviewer_refs: [{ $oid: 'userId' }], + }) + this.SplitTestHandler.promises.getAssignmentForUser = sinon + .stub() + .resolves({ + variant: 'default', + }) + expect( + await this.AuthorizationHelper.promises.isReviewerRoleEnabled( + 'userId', + 'projectId' + ) + ).to.be.true + }) + + it('with reviewers and enabled split test', async function () { + this.ProjectGetter.promises.getProject = sinon.stub().resolves({ + reviewer_refs: [{ $oid: 'userId' }], + }) + this.SplitTestHandler.promises.getAssignmentForUser = sinon + .stub() + .resolves({ + variant: 'enabled', + }) + expect( + await this.AuthorizationHelper.promises.isReviewerRoleEnabled( + 'userId', + 'projectId' + ) + ).to.be.true + }) + }) })