Merge pull request #17023 from overleaf/ii-token-access-null-values

[web] Fix projects token access values

GitOrigin-RevId: f0c6a4993e42320c06753cb65198138afe55b71a

services/web/app/src/Features/Collaborators/CollaboratorsGetter.js

@@ -24,6 +24,9 @@ module.exports = {
   getMemberIdPrivilegeLevel: callbackify(getMemberIdPrivilegeLevel),
   getInvitedCollaboratorCount: callbackify(getInvitedCollaboratorCount),
   getProjectsUserIsMemberOf: callbackify(getProjectsUserIsMemberOf),
+  dangerouslyGetAllProjectsUserIsMemberOf: callbackify(
+    dangerouslyGetAllProjectsUserIsMemberOf
+  ),
   isUserInvitedMemberOfProject: callbackify(isUserInvitedMemberOfProject),
   getPublicShareTokens: callbackify(getPublicShareTokens),
   userIsTokenMember: callbackify(userIsTokenMember),
@@ -37,6 +40,7 @@ module.exports = {
     getMemberIdPrivilegeLevel,
     getInvitedCollaboratorCount,
     getProjectsUserIsMemberOf,
+    dangerouslyGetAllProjectsUserIsMemberOf,
     isUserInvitedMemberOfProject,
     getPublicShareTokens,
     userIsTokenMember,
@@ -169,6 +173,9 @@ async function getPublicShareTokens(userId, projectId) {
   }
 }
 
+// This function returns all the projects that a user currently has access to,
+// excluding projects where the user is listed in the token access fields when
+// token access has been disabled.
 async function getProjectsUserIsMemberOf(userId, fields) {
   const limit = pLimit(2)
   const [readAndWrite, readOnly, tokenReadAndWrite, tokenReadOnly] =
@@ -197,6 +204,26 @@ async function getProjectsUserIsMemberOf(userId, fields) {
   return { readAndWrite, readOnly, tokenReadAndWrite, tokenReadOnly }
 }
 
+// This function returns all the projects that a user is a member of, regardless of
+// the current state of the project, so it includes those projects where token access
+// has been disabled.
+async function dangerouslyGetAllProjectsUserIsMemberOf(userId, fields) {
+  const readAndWrite = await Project.find(
+    { collaberator_refs: userId },
+    fields
+  ).exec()
+  const readOnly = await Project.find({ readOnly_refs: userId }, fields).exec()
+  const tokenReadAndWrite = await Project.find(
+    { tokenAccessReadAndWrite_refs: userId },
+    fields
+  ).exec()
+  const tokenReadOnly = await Project.find(
+    { tokenAccessReadOnly_refs: userId },
+    fields
+  ).exec()
+  return { readAndWrite, readOnly, tokenReadAndWrite, tokenReadOnly }
+}
+
 async function getAllInvitedMembers(projectId) {
   try {
     const rawMembers = await getInvitedMembersWithPrivilegeLevels(projectId)

services/web/app/src/Features/Collaborators/CollaboratorsHandler.js

@@ -80,9 +80,12 @@ async function removeUserFromProject(projectId, userId) {
 
 async function removeUserFromAllProjects(userId) {
   const { readAndWrite, readOnly, tokenReadAndWrite, tokenReadOnly } =
-    await CollaboratorsGetter.promises.getProjectsUserIsMemberOf(userId, {
-      _id: 1,
-    })
+    await CollaboratorsGetter.promises.dangerouslyGetAllProjectsUserIsMemberOf(
+      userId,
+      {
+        _id: 1,
+      }
+    )
   const allProjects = readAndWrite
     .concat(readOnly)
     .concat(tokenReadAndWrite)

services/web/migrations/20240220130452_remove_deleted_users_from_token_access_refs.js

@@ -0,0 +1,9 @@
+const runScript = require('../scripts/remove_deleted_users_from_token_access_refs')
+
+exports.tags = ['server-ce', 'server-pro', 'saas']
+
+exports.migrate = async () => {
+  await runScript(false)
+}
+
+exports.rollback = async () => {}

services/web/scripts/remove_deleted_users_from_token_access_refs.js

@@ -0,0 +1,174 @@
+const { db, waitForDb } = require('../app/src/infrastructure/mongodb')
+const { batchedUpdate } = require('./helpers/batchedUpdate')
+const { ObjectId } = require('mongodb')
+const minimist = require('minimist')
+const CollaboratorsHandler = require('../app/src/Features/Collaborators/CollaboratorsHandler')
+
+const argv = minimist(process.argv.slice(2), {
+  string: ['projects'],
+  boolean: ['dry-run', 'help'],
+  alias: {
+    projects: 'p',
+  },
+  default: {
+    'dry-run': true,
+  },
+})
+
+if (argv.help || argv._.length > 1) {
+  console.error(`Usage: node scripts/remove_deleted_users_from_token_access_refs.js [OPTS]
+    Finds or removes deleted user ids from token access fields
+    "tokenAccessReadOnly_refs" and "tokenAccessReadAndWrite_refs" in the "projects" collection.
+
+    If no projects are specified, all projects will be processed.
+
+    Options:
+
+        --dry-run         finds projects and deleted users but does not do any updates
+        --projects        list of projects ids to be fixed (comma separated)
+    `)
+  process.exit(1)
+}
+
+const DRY_RUN = argv['dry-run']
+const PROJECTS_LIST = argv.projects
+
+async function findUserIds() {
+  const userIds = new Set()
+  const cursor = db.users.find({}, { _id: 1 })
+  for await (const user of cursor) {
+    userIds.add(user._id.toString())
+  }
+  console.log(`=> User ids count: ${userIds.size}`)
+  return userIds
+}
+
+async function fixProjectsWithInvalidTokenAccessRefsIds(
+  DRY_RUN,
+  PROJECTS_LIST
+) {
+  if (DRY_RUN) {
+    console.log('=> Doing dry run')
+  }
+
+  const DELETED_USER_COLLABORATOR_IDS = new Set()
+  const PROJECTS_WITH_DELETED_USER = new Set()
+
+  // get a set of all users ids as an in-memory cache
+  const userIds = await findUserIds()
+
+  // default query for finding all projects with non-empty token access fields
+  let query = {
+    $or: [
+      { 'tokenAccessReadOnly_refs.0': { $exists: true } },
+      { 'tokenAccessReadAndWrite_refs.0': { $exists: true } },
+    ],
+  }
+
+  const projectIds = PROJECTS_LIST?.split(',').map(
+    projectId => new ObjectId(projectId)
+  )
+
+  // query for finding projects passed in as args
+  if (projectIds) {
+    query = { $and: [{ _id: { $in: projectIds } }] }
+  }
+
+  await batchedUpdate(
+    'projects',
+    query,
+    async projects => {
+      for (const project of projects) {
+        // find the set of user ids that are in the token access fields
+        // i.e. the set of collaborators
+        const collaboratorIds = new Set()
+        for (const roUserId of project.tokenAccessReadOnly_refs) {
+          collaboratorIds.add(roUserId.toString())
+        }
+        for (const rwUserId of project.tokenAccessReadAndWrite_refs) {
+          collaboratorIds.add(rwUserId.toString())
+        }
+        // determine which collaborator ids are not in the `users` collection
+        // i.e. the user has been deleted
+        const deletedUserIds = new Set()
+        for (const collaboratorId of collaboratorIds) {
+          if (!userIds.has(collaboratorId)) {
+            deletedUserIds.add(collaboratorId)
+          }
+        }
+
+        // double-check that users doesn't exist in the users collection
+        // we don't want to remove users that were added after the initial query
+        const existingUsersCursor = db.users.find(
+          { _id: { $in: [...deletedUserIds].map(id => new ObjectId(id)) } },
+          { _id: 1 }
+        )
+        for await (const user of existingUsersCursor) {
+          const id = user._id.toString()
+          deletedUserIds.delete(id)
+          // add the user id to the cache
+          userIds.add(id)
+        }
+
+        // remove the actual deleted users
+        for (const deletedUserId of deletedUserIds) {
+          DELETED_USER_COLLABORATOR_IDS.add(deletedUserId)
+          PROJECTS_WITH_DELETED_USER.add(project._id.toString())
+          console.log(
+            '=> Found deleted user id:',
+            deletedUserId,
+            'in project:',
+            project._id.toString()
+          )
+          if (DRY_RUN) {
+            console.log(
+              `=> DRY RUN - would remove deleted ${deletedUserId} from all projects (found in project ${project._id.toString()})`
+            )
+            continue
+          }
+          console.log(
+            `=> Removing deleted ${deletedUserId} from all projects (found in project ${project._id.toString()})`
+          )
+          await CollaboratorsHandler.promises.removeUserFromAllProjects(
+            new ObjectId(deletedUserId)
+          )
+        }
+      }
+    },
+    { tokenAccessReadOnly_refs: 1, tokenAccessReadAndWrite_refs: 1 }
+  )
+
+  console.log(
+    `=> ${DRY_RUN ? 'DRY RUN - would delete' : 'Deleted'} user ids (${
+      DELETED_USER_COLLABORATOR_IDS.size
+    })`
+  )
+  if (DELETED_USER_COLLABORATOR_IDS.size) {
+    console.log(Array.from(DELETED_USER_COLLABORATOR_IDS).join('\n'))
+  }
+  console.log(
+    `=> Projects with deleted user ids (${PROJECTS_WITH_DELETED_USER.size})`
+  )
+  if (PROJECTS_WITH_DELETED_USER.size) {
+    console.log(Array.from(PROJECTS_WITH_DELETED_USER).join('\n'))
+  }
+}
+
+async function main(DRY_RUN, PROJECTS_LIST) {
+  await waitForDb()
+  await fixProjectsWithInvalidTokenAccessRefsIds(DRY_RUN, PROJECTS_LIST)
+}
+
+module.exports = main
+
+if (require.main === module) {
+  main(DRY_RUN, PROJECTS_LIST)
+    .then(() => {
+      console.error('Done')
+      process.exit(0)
+    })
+    .catch(err => {
+      console.error(err)
+      process.exit(1)
+    })
+}

services/web/test/acceptance/src/RemoveDeletedUsersFromTokenAccessRefsTests.js

@@ -0,0 +1,207 @@
+const { db, ObjectId } = require('../../../app/src/infrastructure/mongodb')
+const { promisify } = require('util')
+const { exec } = require('child_process')
+const logger = require('@overleaf/logger/logging-manager')
+const { expect } = require('chai')
+
+describe('RemoveDeletedUsersFromTokenAccessRefsTests', function () {
+  const userId1 = new ObjectId()
+  const userId2 = new ObjectId()
+  const userId3 = new ObjectId()
+
+  let insertedUsersCount
+  beforeEach('insert users', async function () {
+    const users = await db.users.insertMany([
+      { _id: userId1, email: 'user1@example.com' },
+    ])
+    insertedUsersCount = users.insertedCount
+  })
+
+  const projectId1 = new ObjectId('65d726e807c024c8db43be22')
+  const projectId2 = new ObjectId('65d726e807c024c8db43be23')
+  const projectId3 = new ObjectId('65d726e807c024c8db43be24')
+
+  let insertedProjects
+  beforeEach('insert projects', async function () {
+    insertedProjects = await db.projects.insertMany([
+      {
+        _id: projectId1,
+        tokenAccessReadAndWrite_refs: [userId1],
+        tokenAccessReadOnly_refs: [],
+      },
+      {
+        _id: projectId2,
+        tokenAccessReadAndWrite_refs: [userId2],
+        tokenAccessReadOnly_refs: [],
+      },
+      {
+        _id: projectId3,
+        tokenAccessReadAndWrite_refs: [userId3],
+        tokenAccessReadOnly_refs: [],
+      },
+    ])
+  })
+
+  let stdOut
+
+  const runScript = async (dryRun, projectsList) => {
+    let result
+    try {
+      result = await promisify(exec)(
+        [
+          'VERBOSE_LOGGING=true',
+          'node',
+          'scripts/remove_deleted_users_from_token_access_refs',
+          dryRun,
+          projectsList,
+        ].join(' ')
+      )
+    } catch (error) {
+      // dump details like exit code, stdErr and stdOut
+      logger.error({ error }, 'script failed')
+      throw error
+    }
+    const { stdout } = result
+    stdOut = stdout
+
+    expect(stdOut).to.match(new RegExp(`User ids count: ${insertedUsersCount}`))
+  }
+
+  describe('dry-run=true', function () {
+    beforeEach('run script', async function () {
+      await runScript('--dry-run=true')
+      expect(stdOut).to.match(/doing dry run/i)
+    })
+
+    it('should show current user id to be removed', function () {
+      expect(stdOut).to.match(
+        new RegExp(
+          `Found deleted user id: ${userId2.toString()} in project: ${projectId2.toString()}`
+        )
+      )
+      expect(stdOut).to.match(
+        new RegExp(
+          `DRY RUN - would remove deleted ${userId2.toString()} from all projects \\(found in project ${projectId2.toString()}\\)`
+        )
+      )
+      expect(stdOut).to.match(
+        new RegExp(
+          `Found deleted user id: ${userId3.toString()} in project: ${projectId3.toString()}`
+        )
+      )
+      expect(stdOut).to.match(
+        new RegExp(
+          `DRY RUN - would remove deleted ${userId3.toString()} from all projects \\(found in project ${projectId3.toString()}\\)`
+        )
+      )
+    })
+
+    it('should show the user ids (and their count) to be deleted', function () {
+      expect(stdOut).to.match(
+        new RegExp(
+          `DRY RUN - would delete user ids \\(2\\)\\n${userId2.toString()}\\n${userId3.toString()}`
+        )
+      )
+    })
+
+    it('should show the project ids (and their count) that needs fixing', function () {
+      expect(stdOut).to.match(
+        new RegExp(
+          `Projects with deleted user ids \\(2\\)\\n${projectId2.toString()}\\n${projectId3.toString()}`
+        )
+      )
+    })
+
+    it('should not fix the token access fields of projects', async function () {
+      const projects = await db.projects
+        .find({}, { $sort: { _id: 1 } })
+        .toArray()
+      const users = [userId1, userId2, userId3]
+      projects.forEach((project, i) => {
+        expect(project.tokenAccessReadAndWrite_refs[0].toString()).to.eq(
+          users[i].toString()
+        )
+      })
+    })
+  })
+
+  describe('dry-run=false', function () {
+    beforeEach('run script', async function () {
+      await runScript('--dry-run=false')
+      expect(stdOut).to.not.match(/dry run/i)
+    })
+
+    it('should show current user id to be removed', function () {
+      expect(stdOut).to.match(
+        new RegExp(
+          `Found deleted user id: ${userId2.toString()} in project: ${projectId2.toString()}`
+        )
+      )
+      expect(stdOut).to.match(
+        new RegExp(
+          `Removing deleted ${userId2.toString()} from all projects \\(found in project ${projectId2.toString()}\\)`
+        )
+      )
+      expect(stdOut).to.match(
+        new RegExp(
+          `Found deleted user id: ${userId3.toString()} in project: ${projectId3.toString()}`
+        )
+      )
+      expect(stdOut).to.match(
+        new RegExp(
+          `Removing deleted ${userId3.toString()} from all projects \\(found in project ${projectId3.toString()}\\)`
+        )
+      )
+    })
+
+    it('should show the deleted user ids (and their count) that were removed', function () {
+      expect(stdOut).to.match(
+        new RegExp(
+          `Deleted user ids \\(2\\)\\n${userId2.toString()}\\n${userId3.toString()}`
+        )
+      )
+    })
+
+    it('should show the project ids (and their count) that were fixed', function () {
+      expect(stdOut).to.match(
+        new RegExp(
+          `Projects with deleted user ids \\(2\\)\\n${projectId2.toString()}\\n${projectId3.toString()}`
+        )
+      )
+    })
+
+    it('should fix the token access fields of projects', async function () {
+      const [, ...fixedProjects] = await db.projects
+        .find({}, { $sort: { _id: 1 } })
+        .toArray()
+      expect(fixedProjects).to.deep.equal([
+        {
+          _id: projectId2,
+          tokenAccessReadAndWrite_refs: [],
+          tokenAccessReadOnly_refs: [],
+        },
+        {
+          _id: projectId3,
+          tokenAccessReadAndWrite_refs: [],
+          tokenAccessReadOnly_refs: [],
+        },
+      ])
+    })
+  })
+
+  describe('projects=projectId2', function () {
+    beforeEach('run script', async function () {
+      const projectId2 = insertedProjects.insertedIds[1]
+      await runScript('--dry-run=false', `--projects=${projectId2.toString()}`)
+    })
+
+    it('should fix only the projects provided', async function () {
+      const [project1, project2, project3] = await db.projects
+        .find({}, { $sort: { _id: 1 } })
+        .toArray()
+      expect(project1.tokenAccessReadAndWrite_refs.length).to.be.gt(0)
+      expect(project2.tokenAccessReadAndWrite_refs.length).to.eq(0) // deleted user removed
+      expect(project3.tokenAccessReadAndWrite_refs.length).to.be.gt(0)
+    })
+  })
+})

services/web/test/unit/src/Collaborators/CollaboratorsHandlerTests.js

@@ -64,7 +64,7 @@ describe('CollaboratorsHandler', function () {
     }
     this.CollaboratorsGetter = {
       promises: {
-        getProjectsUserIsMemberOf: sinon.stub(),
+        dangerouslyGetAllProjectsUserIsMemberOf: sinon.stub(),
       },
     }
     this.CollaboratorsHandler = SandboxedModule.require(MODULE_PATH, {
@@ -333,7 +333,7 @@ describe('CollaboratorsHandler', function () {
 
   describe('removeUserFromAllProjects', function () {
     it('should remove the user from each project', async function () {
-      this.CollaboratorsGetter.promises.getProjectsUserIsMemberOf
+      this.CollaboratorsGetter.promises.dangerouslyGetAllProjectsUserIsMemberOf
         .withArgs(this.userId, { _id: 1 })
         .resolves({
           readAndWrite: [