From 6cbacc8cb73afe55f7ecba67d5c5f7474a3a7373 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Mon, 2 Jun 2025 13:34:39 +0200 Subject: [PATCH] [web] fetch project once for joinProject (#25667) * [web] fetch project once for joinProject * [web] await all the nested helpers for getting privilege levels Co-authored-by: Mathias Jakobsen --------- Co-authored-by: Mathias Jakobsen GitOrigin-RevId: f0280c36ef995b417ccdab15014f05954e18c5f0 --- .../Authorization/AuthorizationManager.js | 81 ++++++++-- .../Collaborators/CollaboratorsGetter.js | 19 +++ .../Features/Editor/EditorHttpController.js | 25 +-- .../AuthorizationManagerTests.js | 149 ++++++++++++++---- .../src/Editor/EditorHttpControllerTests.js | 50 ++++-- 5 files changed, 250 insertions(+), 74 deletions(-) diff --git a/services/web/app/src/Features/Authorization/AuthorizationManager.js b/services/web/app/src/Features/Authorization/AuthorizationManager.js index 2f339de83d..22d92ea9d9 100644 --- a/services/web/app/src/Features/Authorization/AuthorizationManager.js +++ b/services/web/app/src/Features/Authorization/AuthorizationManager.js @@ -88,9 +88,54 @@ async function getPrivilegeLevelForProject( opts = {} ) { if (userId) { - return getPrivilegeLevelForProjectWithUser(userId, projectId, opts) + return await getPrivilegeLevelForProjectWithUser( + userId, + projectId, + null, + opts + ) } else { - return getPrivilegeLevelForProjectWithoutUser(projectId, token, opts) + return await getPrivilegeLevelForProjectWithoutUser(projectId, token, opts) + } +} + +/** + * Get the privilege level that the user has for the project. + * + * @param userId - The id of the user that wants to access the project. + * @param projectId - The id of the project to be accessed. + * @param {string} token + * @param {ProjectAccess} projectAccess + * @param {Object} opts + * @param {boolean} opts.ignoreSiteAdmin - Do not consider whether the user is + * a site admin. + * @param {boolean} opts.ignorePublicAccess - Do not consider the project is + * publicly accessible. + * + * @returns {string|boolean} The privilege level. One of "owner", + * "readAndWrite", "readOnly" or false. + */ +async function getPrivilegeLevelForProjectWithProjectAccess( + userId, + projectId, + token, + projectAccess, + opts = {} +) { + if (userId) { + return await getPrivilegeLevelForProjectWithUser( + userId, + projectId, + projectAccess, + opts + ) + } else { + return await _getPrivilegeLevelForProjectWithoutUserWithPublicAccessLevel( + projectId, + token, + projectAccess.publicAccessLevel(), + opts + ) } } @@ -98,6 +143,7 @@ async function getPrivilegeLevelForProject( async function getPrivilegeLevelForProjectWithUser( userId, projectId, + projectAccess, opts = {} ) { if (!opts.ignoreSiteAdmin) { @@ -106,11 +152,11 @@ async function getPrivilegeLevelForProjectWithUser( } } - const privilegeLevel = - await CollaboratorsGetter.promises.getMemberIdPrivilegeLevel( - userId, - projectId - ) + projectAccess = + projectAccess || + (await CollaboratorsGetter.promises.getProjectAccess(projectId)) + + const privilegeLevel = projectAccess.privilegeLevelForUser(userId) if (privilegeLevel && privilegeLevel !== PrivilegeLevels.NONE) { // The user has direct access return privilegeLevel @@ -119,7 +165,7 @@ async function getPrivilegeLevelForProjectWithUser( if (!opts.ignorePublicAccess) { // Legacy public-access system // User is present (not anonymous), but does not have direct access - const publicAccessLevel = await getPublicAccessLevel(projectId) + const publicAccessLevel = projectAccess.publicAccessLevel() if (publicAccessLevel === PublicAccessLevels.READ_ONLY) { return PrivilegeLevels.READ_ONLY } @@ -137,7 +183,21 @@ async function getPrivilegeLevelForProjectWithoutUser( token, opts = {} ) { - const publicAccessLevel = await getPublicAccessLevel(projectId) + return await _getPrivilegeLevelForProjectWithoutUserWithPublicAccessLevel( + projectId, + token, + await getPublicAccessLevel(projectId), + opts + ) +} + +// User is Anonymous, Try Token-based access +async function _getPrivilegeLevelForProjectWithoutUserWithPublicAccessLevel( + projectId, + token, + publicAccessLevel, + opts = {} +) { if (!opts.ignorePublicAccess) { if (publicAccessLevel === PublicAccessLevels.READ_ONLY) { // Legacy public read-only access for anonymous user @@ -149,7 +209,7 @@ async function getPrivilegeLevelForProjectWithoutUser( } } if (publicAccessLevel === PublicAccessLevels.TOKEN_BASED) { - return getPrivilegeLevelForProjectWithToken(projectId, token) + return await getPrivilegeLevelForProjectWithToken(projectId, token) } // Deny anonymous user access @@ -309,6 +369,7 @@ module.exports = { canUserRenameProject, canUserAdminProject, getPrivilegeLevelForProject, + getPrivilegeLevelForProjectWithProjectAccess, isRestrictedUserForProject, isUserSiteAdmin, }, diff --git a/services/web/app/src/Features/Collaborators/CollaboratorsGetter.js b/services/web/app/src/Features/Collaborators/CollaboratorsGetter.js index 2906edad4e..10c8e53757 100644 --- a/services/web/app/src/Features/Collaborators/CollaboratorsGetter.js +++ b/services/web/app/src/Features/Collaborators/CollaboratorsGetter.js @@ -32,6 +32,7 @@ module.exports = { userIsTokenMember: callbackify(userIsTokenMember), getAllInvitedMembers: callbackify(getAllInvitedMembers), promises: { + getProjectAccess, getMemberIdsWithPrivilegeLevels, getMemberIds, getInvitedMemberIds, @@ -134,6 +135,7 @@ class ProjectAccess { * @return {typeof PrivilegeLevels[keyof PrivilegeLevels]} */ privilegeLevelForUser(userId) { + if (!userId) return PrivilegeLevels.NONE for (const member of this.#members) { if (member.id === userId.toString()) { return member.privilegeLevel @@ -142,11 +144,26 @@ class ProjectAccess { return PrivilegeLevels.NONE } + /** + * @param {string | ObjectId} userId + * @return {boolean} + */ + isUserTokenMember(userId) { + if (!userId) return false + for (const member of this.#members) { + if (member.id === userId.toString() && member.source === Sources.TOKEN) { + return true + } + } + return false + } + /** * @param {string | ObjectId} userId * @return {boolean} */ isUserInvitedMember(userId) { + if (!userId) return false for (const member of this.#members) { if (member.id === userId.toString() && member.source !== Sources.TOKEN) { return true @@ -199,6 +216,8 @@ class ProjectAccess { } } +module.exports.ProjectAccess = ProjectAccess + async function getProjectAccess(projectId) { const project = await ProjectGetter.promises.getProject(projectId, { owner_ref: 1, diff --git a/services/web/app/src/Features/Editor/EditorHttpController.js b/services/web/app/src/Features/Editor/EditorHttpController.js index 8128a95b26..def7face04 100644 --- a/services/web/app/src/Features/Editor/EditorHttpController.js +++ b/services/web/app/src/Features/Editor/EditorHttpController.js @@ -4,14 +4,13 @@ const ProjectGetter = require('../Project/ProjectGetter') const AuthorizationManager = require('../Authorization/AuthorizationManager') const ProjectEditorHandler = require('../Project/ProjectEditorHandler') const Metrics = require('@overleaf/metrics') -const CollaboratorsGetter = require('../Collaborators/CollaboratorsGetter') const CollaboratorsInviteGetter = require('../Collaborators/CollaboratorsInviteGetter') -const CollaboratorsHandler = require('../Collaborators/CollaboratorsHandler') const PrivilegeLevels = require('../Authorization/PrivilegeLevels') const SessionManager = require('../Authentication/SessionManager') const Errors = require('../Errors/Errors') const { expressify } = require('@overleaf/promise-utils') const Settings = require('@overleaf/settings') +const { ProjectAccess } = require('../Collaborators/CollaboratorsGetter') module.exports = { joinProject: expressify(joinProject), @@ -75,31 +74,23 @@ async function _buildJoinProjectView(req, projectId, userId) { if (project == null) { throw new Errors.NotFoundError('project not found') } - const members = - await CollaboratorsGetter.promises.getInvitedMembersWithPrivilegeLevels( - projectId - ) + const projectAccess = new ProjectAccess(project) + const members = await projectAccess.loadInvitedMembers() const token = req.body.anonymousAccessToken const privilegeLevel = - await AuthorizationManager.promises.getPrivilegeLevelForProject( + await AuthorizationManager.promises.getPrivilegeLevelForProjectWithProjectAccess( userId, projectId, - token + token, + projectAccess ) if (privilegeLevel == null || privilegeLevel === PrivilegeLevels.NONE) { return { project: null, privilegeLevel: null, isRestrictedUser: false } } const invites = await CollaboratorsInviteGetter.promises.getAllInvites(projectId) - const isTokenMember = await CollaboratorsHandler.promises.userIsTokenMember( - userId, - projectId - ) - const isInvitedMember = - await CollaboratorsGetter.promises.isUserInvitedMemberOfProject( - userId, - projectId - ) + const isTokenMember = projectAccess.isUserTokenMember(userId) + const isInvitedMember = projectAccess.isUserInvitedMember(userId) const isRestrictedUser = AuthorizationManager.isRestrictedUser( userId, privilegeLevel, diff --git a/services/web/test/unit/src/Authorization/AuthorizationManagerTests.js b/services/web/test/unit/src/Authorization/AuthorizationManagerTests.js index 7463bbdeb7..e4c67d2f77 100644 --- a/services/web/test/unit/src/Authorization/AuthorizationManagerTests.js +++ b/services/web/test/unit/src/Authorization/AuthorizationManagerTests.js @@ -27,7 +27,10 @@ describe('AuthorizationManager', function () { this.CollaboratorsGetter = { promises: { - getMemberIdPrivilegeLevel: sinon.stub().resolves(PrivilegeLevels.NONE), + getProjectAccess: sinon.stub().resolves({ + publicAccessLevel: sinon.stub().returns(PublicAccessLevels.PRIVATE), + privilegeLevelForUser: sinon.stub().returns(PrivilegeLevels.NONE), + }), }, } @@ -113,9 +116,17 @@ describe('AuthorizationManager', function () { describe('with a user id with a privilege level', function () { beforeEach(async function () { - this.CollaboratorsGetter.promises.getMemberIdPrivilegeLevel - .withArgs(this.user._id, this.project._id) - .resolves(PrivilegeLevels.READ_ONLY) + this.CollaboratorsGetter.promises.getProjectAccess + .withArgs(this.project._id) + .resolves({ + publicAccessLevel: sinon + .stub() + .returns(PublicAccessLevels.PRIVATE), + privilegeLevelForUser: sinon + .stub() + .withArgs(this.user._id) + .returns(PrivilegeLevels.READ_ONLY), + }) this.result = await this.AuthorizationManager.promises.getPrivilegeLevelForProject( this.user._id, @@ -171,8 +182,8 @@ describe('AuthorizationManager', function () { ) }) - it('should not call CollaboratorsGetter.getMemberIdPrivilegeLevel', function () { - this.CollaboratorsGetter.promises.getMemberIdPrivilegeLevel.called.should.equal( + it('should not call CollaboratorsGetter.getProjectAccess', function () { + this.CollaboratorsGetter.promises.getProjectAccess.called.should.equal( false ) }) @@ -204,8 +215,8 @@ describe('AuthorizationManager', function () { ) }) - it('should not call CollaboratorsGetter.getMemberIdPrivilegeLevel', function () { - this.CollaboratorsGetter.promises.getMemberIdPrivilegeLevel.called.should.equal( + it('should not call CollaboratorsGetter.getProjectAccess', function () { + this.CollaboratorsGetter.promises.getProjectAccess.called.should.equal( false ) }) @@ -237,8 +248,8 @@ describe('AuthorizationManager', function () { ) }) - it('should not call CollaboratorsGetter.getMemberIdPrivilegeLevel', function () { - this.CollaboratorsGetter.promises.getMemberIdPrivilegeLevel.called.should.equal( + it('should not call CollaboratorsGetter.getProjectAccess', function () { + this.CollaboratorsGetter.promises.getProjectAccess.called.should.equal( false ) }) @@ -264,9 +275,17 @@ describe('AuthorizationManager', function () { describe('with a user id with a privilege level', function () { beforeEach(async function () { - this.CollaboratorsGetter.promises.getMemberIdPrivilegeLevel - .withArgs(this.user._id, this.project._id) - .resolves(PrivilegeLevels.READ_ONLY) + this.CollaboratorsGetter.promises.getProjectAccess + .withArgs(this.project._id) + .resolves({ + publicAccessLevel: sinon + .stub() + .returns(PublicAccessLevels.PRIVATE), + privilegeLevelForUser: sinon + .stub() + .withArgs(this.user._id) + .returns(PrivilegeLevels.READ_ONLY), + }) this.result = await this.AuthorizationManager.promises.getPrivilegeLevelForProject( this.user._id, @@ -321,8 +340,8 @@ describe('AuthorizationManager', function () { ) }) - it('should not call CollaboratorsGetter.getMemberIdPrivilegeLevel', function () { - this.CollaboratorsGetter.promises.getMemberIdPrivilegeLevel.called.should.equal( + it('should not call CollaboratorsGetter.getProjectAccess', function () { + this.CollaboratorsGetter.promises.getProjectAccess.called.should.equal( false ) }) @@ -336,13 +355,32 @@ describe('AuthorizationManager', function () { describe('with a public project', function () { beforeEach(function () { this.project.publicAccesLevel = 'readAndWrite' + this.CollaboratorsGetter.promises.getProjectAccess + .withArgs(this.project._id) + .resolves({ + publicAccessLevel: sinon + .stub() + .returns(this.project.publicAccesLevel), + privilegeLevelForUser: sinon + .stub() + .withArgs(this.user._id) + .returns(PrivilegeLevels.NONE), + }) }) describe('with a user id with a privilege level', function () { beforeEach(async function () { - this.CollaboratorsGetter.promises.getMemberIdPrivilegeLevel - .withArgs(this.user._id, this.project._id) - .resolves(PrivilegeLevels.READ_ONLY) + this.CollaboratorsGetter.promises.getProjectAccess + .withArgs(this.project._id) + .resolves({ + publicAccessLevel: sinon + .stub() + .returns(this.project.publicAccesLevel), + privilegeLevelForUser: sinon + .stub() + .withArgs(this.user._id) + .returns(PrivilegeLevels.READ_ONLY), + }) this.result = await this.AuthorizationManager.promises.getPrivilegeLevelForProject( this.user._id, @@ -397,8 +435,8 @@ describe('AuthorizationManager', function () { ) }) - it('should not call CollaboratorsGetter.getMemberIdPrivilegeLevel', function () { - this.CollaboratorsGetter.promises.getMemberIdPrivilegeLevel.called.should.equal( + it('should not call CollaboratorsGetter.getProjectAccess', function () { + this.CollaboratorsGetter.promises.getProjectAccess.called.should.equal( false ) }) @@ -410,6 +448,11 @@ describe('AuthorizationManager', function () { }) describe("when the project doesn't exist", function () { + beforeEach(function () { + this.CollaboratorsGetter.promises.getProjectAccess.rejects( + new Errors.NotFoundError() + ) + }) it('should return a NotFoundError', async function () { const someOtherId = new ObjectId() await expect( @@ -424,9 +467,15 @@ describe('AuthorizationManager', function () { describe('when the project id is not valid', function () { beforeEach(function () { - this.CollaboratorsGetter.promises.getMemberIdPrivilegeLevel - .withArgs(this.user._id, this.project._id) - .resolves(PrivilegeLevels.READ_ONLY) + this.CollaboratorsGetter.promises.getProjectAccess + .withArgs(this.project._id) + .resolves({ + publicAccessLevel: sinon.stub().returns(PublicAccessLevels.PRIVATE), + privilegeLevelForUser: sinon + .stub() + .withArgs(this.user._id) + .returns(PrivilegeLevels.READ_ONLY), + }) }) it('should return a error', async function () { @@ -529,9 +578,15 @@ describe('AuthorizationManager', function () { describe('canUserDeleteOrResolveThread', function () { it('should return true when user has write permissions', async function () { - this.CollaboratorsGetter.promises.getMemberIdPrivilegeLevel - .withArgs(this.user._id, this.project._id) - .resolves(PrivilegeLevels.READ_AND_WRITE) + this.CollaboratorsGetter.promises.getProjectAccess + .withArgs(this.project._id) + .resolves({ + publicAccessLevel: sinon.stub().returns(PublicAccessLevels.PRIVATE), + privilegeLevelForUser: sinon + .stub() + .withArgs(this.user._id) + .returns(PrivilegeLevels.READ_AND_WRITE), + }) const canResolve = await this.AuthorizationManager.promises.canUserDeleteOrResolveThread( @@ -546,9 +601,15 @@ describe('AuthorizationManager', function () { }) it('should return false when user has read permission', async function () { - this.CollaboratorsGetter.promises.getMemberIdPrivilegeLevel - .withArgs(this.user._id, this.project._id) - .resolves(PrivilegeLevels.READ_ONLY) + this.CollaboratorsGetter.promises.getProjectAccess + .withArgs(this.project._id) + .resolves({ + publicAccessLevel: sinon.stub().returns(PublicAccessLevels.PRIVATE), + privilegeLevelForUser: sinon + .stub() + .withArgs(this.user._id) + .returns(PrivilegeLevels.READ_ONLY), + }) const canResolve = await this.AuthorizationManager.promises.canUserDeleteOrResolveThread( @@ -564,9 +625,15 @@ describe('AuthorizationManager', function () { describe('when user has review permission', function () { beforeEach(function () { - this.CollaboratorsGetter.promises.getMemberIdPrivilegeLevel - .withArgs(this.user._id, this.project._id) - .resolves(PrivilegeLevels.REVIEW) + this.CollaboratorsGetter.promises.getProjectAccess + .withArgs(this.project._id) + .resolves({ + publicAccessLevel: sinon.stub().returns(PublicAccessLevels.PRIVATE), + privilegeLevelForUser: sinon + .stub() + .withArgs(this.user._id) + .returns(PrivilegeLevels.REVIEW), + }) }) it('should return false when user is not the comment author', async function () { @@ -691,15 +758,27 @@ function testPermission(permission, privilegeLevels) { function setupUserPrivilegeLevel(privilegeLevel) { beforeEach(`set user privilege level to ${privilegeLevel}`, function () { - this.CollaboratorsGetter.promises.getMemberIdPrivilegeLevel - .withArgs(this.user._id, this.project._id) - .resolves(privilegeLevel) + this.CollaboratorsGetter.promises.getProjectAccess + .withArgs(this.project._id) + .resolves({ + publicAccessLevel: sinon.stub().returns(PublicAccessLevels.PRIVATE), + privilegeLevelForUser: sinon + .stub() + .withArgs(this.user._id) + .returns(privilegeLevel), + }) }) } function setupPublicAccessLevel(level) { beforeEach(`set public access level to ${level}`, function () { this.project.publicAccesLevel = level + this.CollaboratorsGetter.promises.getProjectAccess + .withArgs(this.project._id) + .resolves({ + publicAccessLevel: sinon.stub().returns(this.project.publicAccesLevel), + privilegeLevelForUser: sinon.stub().returns(PrivilegeLevels.NONE), + }) }) } diff --git a/services/web/test/unit/src/Editor/EditorHttpControllerTests.js b/services/web/test/unit/src/Editor/EditorHttpControllerTests.js index dffa2d21ff..f9fcf4362e 100644 --- a/services/web/test/unit/src/Editor/EditorHttpControllerTests.js +++ b/services/web/test/unit/src/Editor/EditorHttpControllerTests.js @@ -51,10 +51,25 @@ describe('EditorHttpController', function () { this.AuthorizationManager = { isRestrictedUser: sinon.stub().returns(false), promises: { - getPrivilegeLevelForProject: sinon.stub().resolves('owner'), + getPrivilegeLevelForProjectWithProjectAccess: sinon + .stub() + .resolves('owner'), }, } this.CollaboratorsGetter = { + ProjectAccess: class { + loadInvitedMembers() { + return [] + } + + isUserTokenMember() { + return false + } + + isUserInvitedMember() { + return false + } + }, promises: { getInvitedMembersWithPrivilegeLevels: sinon .stub() @@ -170,9 +185,12 @@ describe('EditorHttpController', function () { describe('successfully', function () { beforeEach(function (done) { - this.CollaboratorsGetter.promises.isUserInvitedMemberOfProject.resolves( - true - ) + sinon + .stub( + this.CollaboratorsGetter.ProjectAccess.prototype, + 'isUserInvitedMember' + ) + .returns(true) this.res.callback = done this.EditorHttpController.joinProject(this.req, this.res) }) @@ -214,7 +232,7 @@ describe('EditorHttpController', function () { describe('with a restricted user', function () { beforeEach(function (done) { this.AuthorizationManager.isRestrictedUser.returns(true) - this.AuthorizationManager.promises.getPrivilegeLevelForProject.resolves( + this.AuthorizationManager.promises.getPrivilegeLevelForProjectWithProjectAccess.resolves( 'readOnly' ) this.res.callback = done @@ -234,7 +252,7 @@ describe('EditorHttpController', function () { describe('when not authorized', function () { beforeEach(function (done) { - this.AuthorizationManager.promises.getPrivilegeLevelForProject.resolves( + this.AuthorizationManager.promises.getPrivilegeLevelForProjectWithProjectAccess.resolves( null ) this.res.callback = done @@ -258,7 +276,7 @@ describe('EditorHttpController', function () { this.AuthorizationManager.isRestrictedUser .withArgs(null, 'readOnly', false, false) .returns(true) - this.AuthorizationManager.promises.getPrivilegeLevelForProject + this.AuthorizationManager.promises.getPrivilegeLevelForProjectWithProjectAccess .withArgs(null, this.project._id, this.token) .resolves('readOnly') this.EditorHttpController.joinProject(this.req, this.res) @@ -277,11 +295,19 @@ describe('EditorHttpController', function () { describe('with a token access user', function () { beforeEach(function (done) { - this.CollaboratorsGetter.promises.isUserInvitedMemberOfProject.resolves( - false - ) - this.CollaboratorsHandler.promises.userIsTokenMember.resolves(true) - this.AuthorizationManager.promises.getPrivilegeLevelForProject.resolves( + sinon + .stub( + this.CollaboratorsGetter.ProjectAccess.prototype, + 'isUserInvitedMember' + ) + .returns(false) + sinon + .stub( + this.CollaboratorsGetter.ProjectAccess.prototype, + 'isUserTokenMember' + ) + .returns(true) + this.AuthorizationManager.promises.getPrivilegeLevelForProjectWithProjectAccess.resolves( 'readAndWrite' ) this.res.callback = done