From 48d08f5b286292e8e51bce52c4be852caa8d9485 Mon Sep 17 00:00:00 2001 From: Domagoj Kriskovic Date: Thu, 23 Jan 2025 12:41:51 +0100 Subject: [PATCH] Allow reviewers to delete their own comment threads (#23044) GitOrigin-RevId: 2165e0f549c9df923fb1c124a7622a49d579c2e3 --- .../Authorization/AuthorizationManager.js | 12 +++++++++--- .../Authorization/AuthorizationMiddleware.js | 19 ++++++++++++------- .../AuthorizationManagerTests.js | 10 +++++----- .../AuthorizationMiddlewareTests.js | 12 ++++++------ 4 files changed, 32 insertions(+), 21 deletions(-) diff --git a/services/web/app/src/Features/Authorization/AuthorizationManager.js b/services/web/app/src/Features/Authorization/AuthorizationManager.js index a457199844..04291cfac1 100644 --- a/services/web/app/src/Features/Authorization/AuthorizationManager.js +++ b/services/web/app/src/Features/Authorization/AuthorizationManager.js @@ -254,7 +254,13 @@ async function isUserSiteAdmin(userId) { return hasAdminAccess(user) } -async function canUserResolveThread(userId, projectId, docId, threadId, token) { +async function canUserDeleteOrResolveThread( + userId, + projectId, + docId, + threadId, + token +) { const privilegeLevel = await getPrivilegeLevelForProject( userId, projectId, @@ -297,7 +303,7 @@ module.exports = { canUserReadProject: callbackify(canUserReadProject), canUserWriteProjectContent: callbackify(canUserWriteProjectContent), canUserReviewProjectContent: callbackify(canUserReviewProjectContent), - canUserResolveThread: callbackify(canUserResolveThread), + canUserDeleteOrResolveThread: callbackify(canUserDeleteOrResolveThread), canUserSendComment: callbackify(canUserSendComment), canUserWriteProjectSettings: callbackify(canUserWriteProjectSettings), canUserRenameProject: callbackify(canUserRenameProject), @@ -310,7 +316,7 @@ module.exports = { canUserReadProject, canUserWriteProjectContent, canUserReviewProjectContent, - canUserResolveThread, + canUserDeleteOrResolveThread, canUserSendComment, canUserWriteProjectSettings, canUserRenameProject, diff --git a/services/web/app/src/Features/Authorization/AuthorizationMiddleware.js b/services/web/app/src/Features/Authorization/AuthorizationMiddleware.js index 7d0d174381..851254cf21 100644 --- a/services/web/app/src/Features/Authorization/AuthorizationMiddleware.js +++ b/services/web/app/src/Features/Authorization/AuthorizationMiddleware.js @@ -103,28 +103,31 @@ async function ensureUserCanWriteProjectSettings(req, res, next) { next() } -async function ensureUserCanResolveThread(req, res, next) { +async function ensureUserCanDeleteOrResolveThread(req, res, next) { const projectId = _getProjectId(req) const docId = _getDocId(req) const threadId = _getThreadId(req) const userId = _getUserId(req) const token = TokenAccessHandler.getRequestToken(req, projectId) - const canResolveThread = - await AuthorizationManager.promises.canUserResolveThread( + const canDeleteThread = + await AuthorizationManager.promises.canUserDeleteOrResolveThread( userId, projectId, docId, threadId, token ) - if (canResolveThread) { - logger.debug({ userId, projectId }, 'allowing user resolve comment thread') + if (canDeleteThread) { + logger.debug( + { userId, projectId }, + 'allowing user to delete or resolve a comment thread' + ) return next() } logger.debug( { userId, projectId, threadId }, - 'denying user to resolve comment thread' + 'denying user to delete or resolve a comment thread' ) return HttpErrorHandler.forbidden(req, res) } @@ -267,7 +270,9 @@ module.exports = { ensureUserCanWriteProjectSettings: expressify( ensureUserCanWriteProjectSettings ), - ensureUserCanResolveThread: expressify(ensureUserCanResolveThread), + ensureUserCanDeleteOrResolveThread: expressify( + ensureUserCanDeleteOrResolveThread + ), ensureUserCanSendComment: expressify(ensureUserCanSendComment), ensureUserCanWriteProjectContent: expressify( ensureUserCanWriteProjectContent diff --git a/services/web/test/unit/src/Authorization/AuthorizationManagerTests.js b/services/web/test/unit/src/Authorization/AuthorizationManagerTests.js index 01b36aee47..02ae7c1912 100644 --- a/services/web/test/unit/src/Authorization/AuthorizationManagerTests.js +++ b/services/web/test/unit/src/Authorization/AuthorizationManagerTests.js @@ -536,14 +536,14 @@ describe('AuthorizationManager', function () { }) }) - describe('canUserReviewThread', 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) const canResolve = - await this.AuthorizationManager.promises.canUserResolveThread( + await this.AuthorizationManager.promises.canUserDeleteOrResolveThread( this.user._id, this.project._id, this.doc._id, @@ -560,7 +560,7 @@ describe('AuthorizationManager', function () { .resolves(PrivilegeLevels.READ_ONLY) const canResolve = - await this.AuthorizationManager.promises.canUserResolveThread( + await this.AuthorizationManager.promises.canUserDeleteOrResolveThread( this.user._id, this.project._id, this.doc._id, @@ -580,7 +580,7 @@ describe('AuthorizationManager', function () { it('should return false when user is not the comment author', async function () { const canResolve = - await this.AuthorizationManager.promises.canUserResolveThread( + await this.AuthorizationManager.promises.canUserDeleteOrResolveThread( this.user._id, this.project._id, this.doc._id, @@ -597,7 +597,7 @@ describe('AuthorizationManager', function () { .resolves({ metadata: { user_id: this.user._id } }) const canResolve = - await this.AuthorizationManager.promises.canUserResolveThread( + await this.AuthorizationManager.promises.canUserDeleteOrResolveThread( this.user._id, this.project._id, this.doc._id, diff --git a/services/web/test/unit/src/Authorization/AuthorizationMiddlewareTests.js b/services/web/test/unit/src/Authorization/AuthorizationMiddlewareTests.js index 415576ddd1..bc3535c3c9 100644 --- a/services/web/test/unit/src/Authorization/AuthorizationMiddlewareTests.js +++ b/services/web/test/unit/src/Authorization/AuthorizationMiddlewareTests.js @@ -25,7 +25,7 @@ describe('AuthorizationMiddleware', function () { canUserReadProject: sinon.stub(), canUserWriteProjectSettings: sinon.stub(), canUserWriteProjectContent: sinon.stub(), - canUserResolveThread: sinon.stub(), + canUserDeleteOrResolveThread: sinon.stub(), canUserSendComment: sinon.stub(), canUserAdminProject: sinon.stub(), canUserRenameProject: sinon.stub(), @@ -91,14 +91,14 @@ describe('AuthorizationMiddleware', function () { testMiddleware('ensureUserCanSendComment', 'canUserSendComment') }) - describe('ensureUserCanResolveThread', function () { + describe('ensureUserCanDeleteOrResolveThread', function () { beforeEach(function () { this.req.params.doc_id = this.doc_id this.req.params.thread_id = this.thread_id }) describe('when user has permission', function () { beforeEach(function () { - this.AuthorizationManager.promises.canUserResolveThread + this.AuthorizationManager.promises.canUserDeleteOrResolveThread .withArgs( this.userId, this.project_id, @@ -109,13 +109,13 @@ describe('AuthorizationMiddleware', function () { .resolves(true) }) - invokeMiddleware('ensureUserCanResolveThread') + invokeMiddleware('ensureUserCanDeleteOrResolveThread') expectNext() }) describe("when user doesn't have permission", function () { beforeEach(function () { - this.AuthorizationManager.promises.canUserResolveThread + this.AuthorizationManager.promises.canUserDeleteOrResolveThread .withArgs( this.userId, this.project_id, @@ -126,7 +126,7 @@ describe('AuthorizationMiddleware', function () { .resolves(false) }) - invokeMiddleware('ensureUserCanResolveThread') + invokeMiddleware('ensureUserCanDeleteOrResolveThread') expectForbidden() }) })