From b04247dd5ace6469e1020b84f96ca58cb98a85b1 Mon Sep 17 00:00:00 2001 From: ilkin-overleaf <100852799+ilkin-overleaf@users.noreply.github.com> Date: Fri, 23 Feb 2024 16:46:04 +0200 Subject: [PATCH] Merge pull request #17023 from overleaf/ii-token-access-null-values [web] Fix projects token access values GitOrigin-RevId: f0c6a4993e42320c06753cb65198138afe55b71a --- .../Collaborators/CollaboratorsGetter.js | 27 +++ .../Collaborators/CollaboratorsHandler.js | 9 +- ...ve_deleted_users_from_token_access_refs.js | 9 + ...ve_deleted_users_from_token_access_refs.js | 174 +++++++++++++++ ...oveDeletedUsersFromTokenAccessRefsTests.js | 207 ++++++++++++++++++ .../CollaboratorsHandlerTests.js | 4 +- 6 files changed, 425 insertions(+), 5 deletions(-) create mode 100644 services/web/migrations/20240220130452_remove_deleted_users_from_token_access_refs.js create mode 100644 services/web/scripts/remove_deleted_users_from_token_access_refs.js create mode 100644 services/web/test/acceptance/src/RemoveDeletedUsersFromTokenAccessRefsTests.js diff --git a/services/web/app/src/Features/Collaborators/CollaboratorsGetter.js b/services/web/app/src/Features/Collaborators/CollaboratorsGetter.js index 3dfcadd225..feab60559b 100644 --- a/services/web/app/src/Features/Collaborators/CollaboratorsGetter.js +++ b/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) diff --git a/services/web/app/src/Features/Collaborators/CollaboratorsHandler.js b/services/web/app/src/Features/Collaborators/CollaboratorsHandler.js index 886de8b9c1..87e3755126 100644 --- a/services/web/app/src/Features/Collaborators/CollaboratorsHandler.js +++ b/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) diff --git a/services/web/migrations/20240220130452_remove_deleted_users_from_token_access_refs.js b/services/web/migrations/20240220130452_remove_deleted_users_from_token_access_refs.js new file mode 100644 index 0000000000..646d1f7385 --- /dev/null +++ b/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 () => {} diff --git a/services/web/scripts/remove_deleted_users_from_token_access_refs.js b/services/web/scripts/remove_deleted_users_from_token_access_refs.js new file mode 100644 index 0000000000..4d23f1abf5 --- /dev/null +++ b/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) + }) +} diff --git a/services/web/test/acceptance/src/RemoveDeletedUsersFromTokenAccessRefsTests.js b/services/web/test/acceptance/src/RemoveDeletedUsersFromTokenAccessRefsTests.js new file mode 100644 index 0000000000..ef8aa5d7e9 --- /dev/null +++ b/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) + }) + }) +}) diff --git a/services/web/test/unit/src/Collaborators/CollaboratorsHandlerTests.js b/services/web/test/unit/src/Collaborators/CollaboratorsHandlerTests.js index 3812915a90..0992a3ef3e 100644 --- a/services/web/test/unit/src/Collaborators/CollaboratorsHandlerTests.js +++ b/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: [