diff --git a/services/web/app/src/Features/Collaborators/OwnershipTransferHandler.mjs b/services/web/app/src/Features/Collaborators/OwnershipTransferHandler.mjs index a0b9bfa054..8df7f78c6e 100644 --- a/services/web/app/src/Features/Collaborators/OwnershipTransferHandler.mjs +++ b/services/web/app/src/Features/Collaborators/OwnershipTransferHandler.mjs @@ -12,6 +12,7 @@ import AnalyticsManager from '../Analytics/AnalyticsManager.mjs' import OError from '@overleaf/o-error' import TagsHandler from '../Tags/TagsHandler.mjs' import { promiseMapWithLimit } from '@overleaf/promise-utils' +import LimitationsManager from '../Subscription/LimitationsManager.mjs' export default { promises: { @@ -124,16 +125,20 @@ async function transferOwnership(projectId, newOwnerId, options = {}) { { previousOwnerId, newOwnerId } ) - // Determine which permissions to give old owner based on - // new owner's existing permissions - const newPermissions = - _getUserPermissions(newOwner, project) || PrivilegeLevels.READ_ONLY + // Transfer ownership + await _transferOwnership(projectId, previousOwnerId, newOwnerId) - await _transferOwnership( + // Determine which permissions to give old owner (after transfer, so we use new owner's limits) + const { privilegeLevel, pendingPrivilegeLevel } = + await _determinePrivilegeLevelForPreviousOwner(projectId) + + // Add the previous owner back to the project with determined permissions + await CollaboratorsHandler.promises.addUserIdToProject( projectId, - previousOwnerId, newOwnerId, - newPermissions + previousOwnerId, + privilegeLevel, + pendingPrivilegeLevel ) // Flush project to TPDS @@ -189,26 +194,34 @@ function _userIsCollaborator(user, project) { return Boolean(_getUserPermissions(user, project)) } -async function _transferOwnership( - projectId, - previousOwnerId, - newOwnerId, - newPermissions -) { +async function _determinePrivilegeLevelForPreviousOwner(projectId) { + // Try to give READ_AND_WRITE if space available based on new owner's limits + const canAddEditor = + await LimitationsManager.promises.canAddXEditCollaborators(projectId, 1) + + if (canAddEditor) { + return { privilegeLevel: PrivilegeLevels.READ_AND_WRITE } + } + + // Collaborator limit is reached for editor and reviewer so fall back to read-only + // Add pending editor status so they are automatically upgraded when possible + return { + privilegeLevel: PrivilegeLevels.READ_ONLY, + pendingPrivilegeLevel: { pendingEditor: true }, + } +} + +async function _transferOwnership(projectId, previousOwnerId, newOwnerId) { + // Remove new owner from collaborators list await CollaboratorsHandler.promises.removeUserFromProject( projectId, newOwnerId ) + // Update project ownership await Project.updateOne( { _id: projectId }, { $set: { owner_ref: newOwnerId } } ).exec() - await CollaboratorsHandler.promises.addUserIdToProject( - projectId, - newOwnerId, - previousOwnerId, - newPermissions - ) } async function _sendEmails(project, previousOwner, newOwner) { diff --git a/services/web/test/acceptance/src/ProjectOwnershipTransferTests.mjs b/services/web/test/acceptance/src/ProjectOwnershipTransferTests.mjs index 9d01efd7bc..7272f0926e 100644 --- a/services/web/test/acceptance/src/ProjectOwnershipTransferTests.mjs +++ b/services/web/test/acceptance/src/ProjectOwnershipTransferTests.mjs @@ -1,5 +1,6 @@ import { expect } from 'chai' import UserHelper from './helpers/User.mjs' +import Features from '../../../app/src/infrastructure/Features.mjs' const User = UserHelper.promises @@ -53,11 +54,38 @@ describe('Project ownership transfer', function () { }) it('adds the previous owner as a read/write collaborator', async function () { + // Skip this test in SaaS environments as limited collaborators are enforced + if (Features.hasFeature('saas')) { + this.skip() + } + const project = await this.collaboratorSession.getProject(this.projectId) expect(project.collaberator_refs.map(x => x.toString())).to.have.members([ this.owner._id.toString(), this.invitedAdmin._id.toString(), ]) + expect(project.owner_ref.toString()).to.equal( + this.collaborator._id.toString() + ) + expect(project.readOnly_refs.map(x => x.toString())).to.be.empty + }) + + it('adds the previous owner as a read only', async function () { + // Skip this test in non-SaaS environments as unlimited collaborators are allowed + if (!Features.hasFeature('saas')) { + this.skip() + } + + const project = await this.collaboratorSession.getProject(this.projectId) + expect(project.collaberator_refs.map(x => x.toString())).to.have.members([ + this.invitedAdmin._id.toString(), + ]) + expect(project.owner_ref.toString()).to.equal( + this.collaborator._id.toString() + ) + expect(project.readOnly_refs.map(x => x.toString())).to.have.members([ + this.owner._id.toString(), + ]) }) it('lets the new owner open the project', async function () { diff --git a/services/web/test/unit/src/Collaborators/OwnershipTransferHandler.test.mjs b/services/web/test/unit/src/Collaborators/OwnershipTransferHandler.test.mjs index 4cf14afcd1..2ec6831ee3 100644 --- a/services/web/test/unit/src/Collaborators/OwnershipTransferHandler.test.mjs +++ b/services/web/test/unit/src/Collaborators/OwnershipTransferHandler.test.mjs @@ -80,6 +80,11 @@ describe('OwnershipTransferHandler', function () { addProjectsToTag: sinon.stub().resolves(), }, } + ctx.LimitationsManager = { + promises: { + canAddXEditCollaborators: sinon.stub().resolves(true), + }, + } vi.mock('../../../../app/src/Features/Errors/Errors.js', () => vi.importActual('../../../../app/src/Features/Errors/Errors.js') @@ -136,6 +141,13 @@ describe('OwnershipTransferHandler', function () { }) ) + vi.doMock( + '../../../../app/src/Features/Subscription/LimitationsManager.mjs', + () => ({ + default: ctx.LimitationsManager, + }) + ) + ctx.handler = (await import(MODULE_PATH)).default }) @@ -195,30 +207,197 @@ describe('OwnershipTransferHandler', function () { ) }) - it('should transfer ownership of the project to a read-only collaborator', async function (ctx) { + it('should check collaborator limits after ownership transfer', async function (ctx) { await ctx.handler.promises.transferOwnership( ctx.project._id, - ctx.readOnlyCollaborator._id + ctx.collaborator._id ) - expect(ctx.ProjectModel.updateOne).to.have.been.calledWith( - { _id: ctx.project._id }, - sinon.match({ $set: { owner_ref: ctx.readOnlyCollaborator._id } }) + + expect(ctx.ProjectModel.updateOne).to.have.been.calledBefore( + ctx.LimitationsManager.promises.canAddXEditCollaborators + ) + + expect( + ctx.LimitationsManager.promises.canAddXEditCollaborators + ).to.have.been.calledBefore( + ctx.CollaboratorsHandler.promises.addUserIdToProject ) }) - it('gives old owner read-only permissions if new owner was previously a viewer', async function (ctx) { - await ctx.handler.promises.transferOwnership( - ctx.project._id, - ctx.readOnlyCollaborator._id - ) - expect( - ctx.CollaboratorsHandler.promises.addUserIdToProject - ).to.have.been.calledWith( - ctx.project._id, - ctx.readOnlyCollaborator._id, - ctx.user._id, - PrivilegeLevels.READ_ONLY - ) + describe('when there are edit collaborator slots available', function () { + beforeEach(function (ctx) { + ctx.LimitationsManager.promises.canAddXEditCollaborators.resolves(true) + }) + + it('should give old owner read/write permissions when transferring to a read-only collaborator', async function (ctx) { + await ctx.handler.promises.transferOwnership( + ctx.project._id, + ctx.readOnlyCollaborator._id + ) + expect( + ctx.CollaboratorsHandler.promises.addUserIdToProject + ).to.have.been.calledWith( + ctx.project._id, + ctx.readOnlyCollaborator._id, + ctx.user._id, + PrivilegeLevels.READ_AND_WRITE, + undefined + ) + expect( + ctx.LimitationsManager.promises.canAddXEditCollaborators + ).to.have.been.calledWith(ctx.project._id, 1) + }) + + it('should give old owner read/write permissions when transferring to an editor', async function (ctx) { + await ctx.handler.promises.transferOwnership( + ctx.project._id, + ctx.collaborator._id + ) + expect( + ctx.CollaboratorsHandler.promises.addUserIdToProject + ).to.have.been.calledWith( + ctx.project._id, + ctx.collaborator._id, + ctx.user._id, + PrivilegeLevels.READ_AND_WRITE, + undefined + ) + expect( + ctx.LimitationsManager.promises.canAddXEditCollaborators + ).to.have.been.calledWith(ctx.project._id, 1) + }) + + it('should give old owner read/write permissions when transferring to a reviewer', async function (ctx) { + await ctx.handler.promises.transferOwnership( + ctx.project._id, + ctx.reviewer._id + ) + expect( + ctx.CollaboratorsHandler.promises.addUserIdToProject + ).to.have.been.calledWith( + ctx.project._id, + ctx.reviewer._id, + ctx.user._id, + PrivilegeLevels.READ_AND_WRITE, + undefined + ) + expect( + ctx.LimitationsManager.promises.canAddXEditCollaborators + ).to.have.been.calledWith(ctx.project._id, 1) + }) + + it('should give old owner read/write permissions when transferring to a non-collaborator', async function (ctx) { + ctx.project.collaberator_refs = [] + ctx.project.readOnly_refs = [] + ctx.project.reviewer_refs = [] + const newOwner = { + _id: new ObjectId(), + email: 'admin@example.com', + } + ctx.UserGetter.promises.getUser + .withArgs(newOwner._id) + .resolves(newOwner) + + await ctx.handler.promises.transferOwnership( + ctx.project._id, + newOwner._id, + { allowTransferToNonCollaborators: true } + ) + expect( + ctx.CollaboratorsHandler.promises.addUserIdToProject + ).to.have.been.calledWith( + ctx.project._id, + newOwner._id, + ctx.user._id, + PrivilegeLevels.READ_AND_WRITE, + undefined + ) + expect( + ctx.LimitationsManager.promises.canAddXEditCollaborators + ).to.have.been.calledWith(ctx.project._id, 1) + }) + }) + + describe('when there are no edit collaborator slots available', function () { + beforeEach(function (ctx) { + ctx.LimitationsManager.promises.canAddXEditCollaborators.resolves(false) + }) + + it('should give old owner read-only with pending editor flag when transferring to an editor', async function (ctx) { + await ctx.handler.promises.transferOwnership( + ctx.project._id, + ctx.collaborator._id + ) + expect( + ctx.CollaboratorsHandler.promises.addUserIdToProject + ).to.have.been.calledWith( + ctx.project._id, + ctx.collaborator._id, + ctx.user._id, + PrivilegeLevels.READ_ONLY, + { pendingEditor: true } + ) + }) + + it('should give old owner read-only with pending editor flag when transferring to a reviewer', async function (ctx) { + await ctx.handler.promises.transferOwnership( + ctx.project._id, + ctx.reviewer._id + ) + expect( + ctx.CollaboratorsHandler.promises.addUserIdToProject + ).to.have.been.calledWith( + ctx.project._id, + ctx.reviewer._id, + ctx.user._id, + PrivilegeLevels.READ_ONLY, + { pendingEditor: true } + ) + }) + + it('should give old owner read-only with pending editor flag when transferring to a read-only collaborator', async function (ctx) { + await ctx.handler.promises.transferOwnership( + ctx.project._id, + ctx.readOnlyCollaborator._id + ) + expect( + ctx.CollaboratorsHandler.promises.addUserIdToProject + ).to.have.been.calledWith( + ctx.project._id, + ctx.readOnlyCollaborator._id, + ctx.user._id, + PrivilegeLevels.READ_ONLY, + { pendingEditor: true } + ) + }) + + it('should give old owner read-only with pending editor flag when transferring to a non-collaborator', async function (ctx) { + ctx.project.collaberator_refs = [] + ctx.project.readOnly_refs = [] + ctx.project.reviewer_refs = [] + const newOwner = { + _id: new ObjectId(), + email: 'newowner@example.com', + } + ctx.UserGetter.promises.getUser + .withArgs(newOwner._id) + .resolves(newOwner) + + await ctx.handler.promises.transferOwnership( + ctx.project._id, + newOwner._id, + { allowTransferToNonCollaborators: true } + ) + expect( + ctx.CollaboratorsHandler.promises.addUserIdToProject + ).to.have.been.calledWith( + ctx.project._id, + newOwner._id, + ctx.user._id, + PrivilegeLevels.READ_ONLY, + { pendingEditor: true } + ) + }) }) it('should do nothing if transferring back to the owner', async function (ctx) { @@ -239,21 +418,6 @@ describe('OwnershipTransferHandler', function () { ).to.have.been.calledWith(ctx.project._id, ctx.collaborator._id) }) - it('should add the former project owner as a read/write collaborator', async function (ctx) { - await ctx.handler.promises.transferOwnership( - ctx.project._id, - ctx.collaborator._id - ) - expect( - ctx.CollaboratorsHandler.promises.addUserIdToProject - ).to.have.been.calledWith( - ctx.project._id, - ctx.collaborator._id, - ctx.user._id, - PrivilegeLevels.READ_AND_WRITE - ) - }) - it('should transfer ownership of the project to a reviewer', async function (ctx) { await ctx.handler.promises.transferOwnership( ctx.project._id, @@ -265,21 +429,6 @@ describe('OwnershipTransferHandler', function () { ) }) - it('gives old owner reviewer permissions if new owner was previously a reviewer', async function (ctx) { - await ctx.handler.promises.transferOwnership( - ctx.project._id, - ctx.reviewer._id - ) - expect( - ctx.CollaboratorsHandler.promises.addUserIdToProject - ).to.have.been.calledWith( - ctx.project._id, - ctx.reviewer._id, - ctx.user._id, - PrivilegeLevels.REVIEW - ) - }) - it('should flush the project to tpds', async function (ctx) { await ctx.handler.promises.transferOwnership( ctx.project._id,