Merge pull request #29733 from overleaf/mg-transfer-ownership

Fix transfer ownership permissions

GitOrigin-RevId: b6d09704361507085e3eae8dc9240a36ae47c70e

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)
+
+  // Determine which permissions to give old owner (after transfer, so we use new owner's limits)
+  const { privilegeLevel, pendingPrivilegeLevel } =
+    await _determinePrivilegeLevelForPreviousOwner(projectId)
 
-  await _transferOwnership(
+  // 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) {

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 () {

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,32 +207,199 @@ 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
-      )
-      expect(ctx.ProjectModel.updateOne).to.have.been.calledWith(
-        { _id: ctx.project._id },
-        sinon.match({ $set: { owner_ref: ctx.readOnlyCollaborator._id } })
+        ctx.collaborator._id
       )
-    })
 
-    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.ProjectModel.updateOne).to.have.been.calledBefore(
+        ctx.LimitationsManager.promises.canAddXEditCollaborators
       )
+
       expect(
+        ctx.LimitationsManager.promises.canAddXEditCollaborators
+      ).to.have.been.calledBefore(
         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) {
       await ctx.handler.promises.transferOwnership(
         ctx.project._id,
@@ -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,