Allow reviewers to resolve their own comments (#22582)

* Allow reviewers to resolve their own comments

* check if reviewer is comment author

* add missing translation

* add CommentsController tests

* added DocumentManagerTests

* added HttpControllerTests

* Add AuthorizationManagerTests

* added AuthorizationMiddlewareTests

* added DocumentUpdaterHandler test

* fix test descriptions

* remove returns from CommentsControllerTests

* use ensureUserCanResolveThread in authorizationMiddleware

* move canResolveThread to AuthorizationManager

* commentId as param in NotFoundError

* refactor canUserResolveThread

GitOrigin-RevId: 131c3d1eb9ac916eaaa9221d351a92bc07b80cdc

services/document-updater/app.js

@@ -135,6 +135,10 @@ app.use((req, res, next) => {
 })
 
 app.get('/project/:project_id/doc/:doc_id', HttpController.getDoc)
+app.get(
+  '/project/:project_id/doc/:doc_id/comment/:comment_id',
+  HttpController.getComment
+)
 app.get('/project/:project_id/doc/:doc_id/peek', HttpController.peekDoc)
 // temporarily keep the GET method for backwards compatibility
 app.get('/project/:project_id/doc', HttpController.getProjectDocsAndFlushIfOld)

services/document-updater/app/js/DocumentManager.js

@@ -367,6 +367,21 @@ const DocumentManager = {
     }
   },
 
+  async getComment(projectId, docId, commentId) {
+    const { ranges } = await DocumentManager.getDoc(projectId, docId)
+
+    const comment = ranges?.comments?.find(comment => comment.id === commentId)
+
+    if (!comment) {
+      throw new Errors.NotFoundError({
+        message: 'comment not found',
+        info: { commentId },
+      })
+    }
+
+    return { comment }
+  },
+
   async deleteComment(projectId, docId, commentId, userId) {
     const { lines, version, ranges, pathname, historyRangesSupport } =
       await DocumentManager.getDoc(projectId, docId)
@@ -500,6 +515,16 @@ const DocumentManager = {
     )
   },
 
+  async getCommentWithLock(projectId, docId, commentId) {
+    const UpdateManager = require('./UpdateManager')
+    return await UpdateManager.promises.lockUpdatesAndDo(
+      DocumentManager.getComment,
+      projectId,
+      docId,
+      commentId
+    )
+  },
+
   async getDocAndRecentOpsWithLock(projectId, docId, fromVersion) {
     const UpdateManager = require('./UpdateManager')
     return await UpdateManager.promises.lockUpdatesAndDo(
@@ -676,6 +701,7 @@ module.exports = {
         'pathname',
         'projectHistoryId',
       ],
+      getCommentWithLock: ['comment'],
     },
   }),
   promises: DocumentManager,

services/document-updater/app/js/HttpController.js

@@ -49,6 +49,29 @@ function getDoc(req, res, next) {
   )
 }
 
+function getComment(req, res, next) {
+  const docId = req.params.doc_id
+  const projectId = req.params.project_id
+  const commentId = req.params.comment_id
+
+  logger.debug({ projectId, docId, commentId }, 'getting comment via http')
+
+  DocumentManager.getCommentWithLock(
+    projectId,
+    docId,
+    commentId,
+    (error, comment) => {
+      if (error) {
+        return next(error)
+      }
+      if (comment == null) {
+        return next(new Errors.NotFoundError('comment not found'))
+      }
+      res.json(comment)
+    }
+  )
+}
+
 // return the doc from redis if present, but don't load it from mongo
 function peekDoc(req, res, next) {
   const docId = req.params.doc_id
@@ -506,4 +529,5 @@ module.exports = {
   flushQueuedProjects,
   blockProject,
   unblockProject,
+  getComment,
 }

services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js

@@ -835,6 +835,77 @@ describe('DocumentManager', function () {
     })
   })
 
+  describe('getComment', function () {
+    beforeEach(function () {
+      this.ranges.comments = [
+        {
+          id: 'mock-comment-id-1',
+        },
+        {
+          id: 'mock-comment-id-2',
+        },
+      ]
+      this.DocumentManager.promises.getDoc = sinon.stub().resolves({
+        lines: this.lines,
+        version: this.version,
+        ranges: this.ranges,
+      })
+    })
+
+    describe('when comment exists', function () {
+      beforeEach(async function () {
+        await expect(
+          this.DocumentManager.promises.getComment(
+            this.project_id,
+            this.doc_id,
+            'mock-comment-id-1'
+          )
+        ).to.eventually.deep.equal({
+          comment: { id: 'mock-comment-id-1' },
+        })
+      })
+
+      it("should get the document's current ranges", function () {
+        this.DocumentManager.promises.getDoc
+          .calledWith(this.project_id, this.doc_id)
+          .should.equal(true)
+      })
+    })
+
+    describe('when comment doesnt exists', function () {
+      beforeEach(async function () {
+        await expect(
+          this.DocumentManager.promises.getComment(
+            this.project_id,
+            this.doc_id,
+            'mock-comment-id-x'
+          )
+        ).to.be.rejectedWith(Errors.NotFoundError)
+      })
+
+      it("should get the document's current ranges", function () {
+        this.DocumentManager.promises.getDoc
+          .calledWith(this.project_id, this.doc_id)
+          .should.equal(true)
+      })
+    })
+
+    describe('when the doc is not found', function () {
+      beforeEach(async function () {
+        this.DocumentManager.promises.getDoc = sinon
+          .stub()
+          .resolves({ lines: null, version: null, ranges: null })
+        await expect(
+          this.DocumentManager.promises.acceptChanges(
+            this.project_id,
+            this.doc_id,
+            [this.change_id]
+          )
+        ).to.be.rejectedWith(Errors.NotFoundError)
+      })
+    })
+  })
+
   describe('deleteComment', function () {
     beforeEach(function () {
       this.comment_id = 'mock-comment-id'

services/document-updater/test/unit/js/HttpController/HttpControllerTests.js

@@ -184,6 +184,65 @@ describe('HttpController', function () {
     })
   })
 
+  describe('getComment', function () {
+    beforeEach(function () {
+      this.ranges = {
+        changes: 'mock',
+        comments: [
+          {
+            id: 'comment-id-1',
+          },
+          {
+            id: 'comment-id-2',
+          },
+        ],
+      }
+      this.req = {
+        params: {
+          project_id: this.project_id,
+          doc_id: this.doc_id,
+          comment_id: this.comment_id,
+        },
+        query: {},
+        body: {},
+      }
+    })
+
+    beforeEach(function () {
+      this.DocumentManager.getCommentWithLock = sinon
+        .stub()
+        .callsArgWith(3, null, this.ranges.comments[0])
+      this.HttpController.getComment(this.req, this.res, this.next)
+    })
+
+    it('should get the comment', function () {
+      this.DocumentManager.getCommentWithLock
+        .calledWith(this.project_id, this.doc_id, this.comment_id)
+        .should.equal(true)
+    })
+
+    it('should return the comment as JSON', function () {
+      this.res.json
+        .calledWith({
+          id: 'comment-id-1',
+        })
+        .should.equal(true)
+    })
+
+    it('should log the request', function () {
+      this.logger.debug
+        .calledWith(
+          {
+            projectId: this.project_id,
+            docId: this.doc_id,
+            commentId: this.comment_id,
+          },
+          'getting comment via http'
+        )
+        .should.equal(true)
+    })
+  })
+
   describe('setDoc', function () {
     beforeEach(function () {
       this.lines = ['one', 'two', 'three']

services/web/app/src/Features/Authorization/AuthorizationManager.js

@@ -10,6 +10,7 @@ const PublicAccessLevels = require('./PublicAccessLevels')
 const Errors = require('../Errors/Errors')
 const { hasAdminAccess } = require('../Helpers/AdminAuthorizationHelper')
 const Settings = require('@overleaf/settings')
+const DocumentUpdaterHandler = require('../DocumentUpdater/DocumentUpdaterHandler')
 
 function isRestrictedUser(
   userId,
@@ -190,6 +191,19 @@ async function canUserReadProject(userId, projectId, token) {
   ].includes(privilegeLevel)
 }
 
+async function canUserReviewProjectContent(userId, projectId, token) {
+  const privilegeLevel = await getPrivilegeLevelForProject(
+    userId,
+    projectId,
+    token
+  )
+  return [
+    PrivilegeLevels.OWNER,
+    PrivilegeLevels.READ_AND_WRITE,
+    PrivilegeLevels.REVIEW,
+  ].includes(privilegeLevel)
+}
+
 async function canUserWriteProjectContent(userId, projectId, token) {
   const privilegeLevel = await getPrivilegeLevelForProject(
     userId,
@@ -240,9 +254,37 @@ async function isUserSiteAdmin(userId) {
   return hasAdminAccess(user)
 }
 
+async function canUserResolveThread(userId, projectId, docId, threadId, token) {
+  const privilegeLevel = await getPrivilegeLevelForProject(
+    userId,
+    projectId,
+    token,
+    { ignorePublicAccess: true }
+  )
+  if (
+    privilegeLevel === PrivilegeLevels.OWNER ||
+    privilegeLevel === PrivilegeLevels.READ_AND_WRITE
+  ) {
+    return true
+  }
+
+  if (privilegeLevel !== PrivilegeLevels.REVIEW) {
+    return false
+  }
+
+  const comment = await DocumentUpdaterHandler.promises.getComment(
+    projectId,
+    docId,
+    threadId
+  )
+  return comment.metadata.user_id === userId
+}
+
 module.exports = {
   canUserReadProject: callbackify(canUserReadProject),
   canUserWriteProjectContent: callbackify(canUserWriteProjectContent),
+  canUserReviewProjectContent: callbackify(canUserReviewProjectContent),
+  canUserResolveThread: callbackify(canUserResolveThread),
   canUserWriteProjectSettings: callbackify(canUserWriteProjectSettings),
   canUserRenameProject: callbackify(canUserRenameProject),
   canUserAdminProject: callbackify(canUserAdminProject),
@@ -253,6 +295,8 @@ module.exports = {
   promises: {
     canUserReadProject,
     canUserWriteProjectContent,
+    canUserReviewProjectContent,
+    canUserResolveThread,
     canUserWriteProjectSettings,
     canUserRenameProject,
     canUserAdminProject,

services/web/app/src/Features/Authorization/AuthorizationMiddleware.js

@@ -103,6 +103,32 @@ async function ensureUserCanWriteProjectSettings(req, res, next) {
   next()
 }
 
+async function ensureUserCanResolveThread(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(
+      userId,
+      projectId,
+      docId,
+      threadId,
+      token
+    )
+  if (canResolveThread) {
+    logger.debug({ userId, projectId }, 'allowing user resolve comment thread')
+    return next()
+  }
+
+  logger.debug(
+    { userId, projectId, threadId },
+    'denying user to resolve comment thread'
+  )
+  return HttpErrorHandler.forbidden(req, res)
+}
+
 async function ensureUserCanWriteProjectContent(req, res, next) {
   const projectId = _getProjectId(req)
   const userId = _getUserId(req)
@@ -166,6 +192,28 @@ function _getProjectId(req) {
   return projectId
 }
 
+function _getDocId(req) {
+  const docId = req.params.doc_id
+  if (!docId) {
+    throw new Error('Expected doc_id in request parameters')
+  }
+  if (!ObjectId.isValid(docId)) {
+    throw new Errors.NotFoundError(`invalid docId: ${docId}`)
+  }
+  return docId
+}
+
+function _getThreadId(req) {
+  const threadId = req.params.thread_id
+  if (!threadId) {
+    throw new Error('Expected thread_id in request parameters')
+  }
+  if (!ObjectId.isValid(threadId)) {
+    throw new Errors.NotFoundError(`invalid threadId: ${threadId}`)
+  }
+  return threadId
+}
+
 function _getUserId(req) {
   return (
     SessionManager.getLoggedInUserId(req.session) ||
@@ -200,6 +248,7 @@ module.exports = {
   ensureUserCanWriteProjectSettings: expressify(
     ensureUserCanWriteProjectSettings
   ),
+  ensureUserCanResolveThread: expressify(ensureUserCanResolveThread),
   ensureUserCanWriteProjectContent: expressify(
     ensureUserCanWriteProjectContent
   ),

services/web/app/src/Features/DocumentUpdater/DocumentUpdaterHandler.js

@@ -80,6 +80,23 @@ function deleteDoc(projectId, docId, ignoreFlushErrors, callback) {
   )
 }
 
+function getComment(projectId, docId, commentId, callback) {
+  _makeRequest(
+    {
+      path: `/project/${projectId}/doc/${docId}/comment/${commentId}`,
+      json: true,
+    },
+    projectId,
+    'get-comment',
+    function (error, comment) {
+      if (error) {
+        return callback(error)
+      }
+      callback(null, comment)
+    }
+  )
+}
+
 function getDocument(projectId, docId, fromVersion, callback) {
   _makeRequest(
     {
@@ -548,6 +565,7 @@ module.exports = {
   flushProjectToMongoAndDelete,
   flushDocToMongo,
   deleteDoc,
+  getComment,
   getDocument,
   setDocument,
   appendToDocument,
@@ -567,6 +585,7 @@ module.exports = {
     flushProjectToMongoAndDelete: promisify(flushProjectToMongoAndDelete),
     flushDocToMongo: promisify(flushDocToMongo),
     deleteDoc: promisify(deleteDoc),
+    getComment: promisify(getComment),
     getDocument: promisifyMultiResult(getDocument, [
       'lines',
       'version',

services/web/locales/en.json

@@ -573,7 +573,7 @@
   "easily_manage_your_project_files_everywhere": "Easily manage your project files, everywhere",
   "easy_collaboration_for_students": "Easy collaboration for students. Supports longer or more complex projects.",
   "edit": "Edit",
-  "edit_comment_error_message": "There was an error editing your comment. Please try again in a few moments.",
+  "edit_comment_error_message": "There was an error editing the comment. Please try again in a few moments.",
   "edit_comment_error_title": "Edit Comment Error",
   "edit_dictionary": "Edit Dictionary",
   "edit_dictionary_empty": "Your custom dictionary is empty.",
@@ -1813,6 +1813,8 @@
   "resize": "Resize",
   "resolve": "Resolve",
   "resolve_comment": "Resolve comment",
+  "resolve_comment_error_message": "There was an error resolving the comment. Please try again in a few moments.",
+  "resolve_comment_error_title": "Resolve Comment Error",
   "resolved_comments": "Resolved comments",
   "restore": "Restore",
   "restore_file": "Restore file",

services/web/test/unit/src/Authorization/AuthorizationManagerTests.js

@@ -12,6 +12,8 @@ describe('AuthorizationManager', function () {
   beforeEach(function () {
     this.user = { _id: new ObjectId() }
     this.project = { _id: new ObjectId() }
+    this.doc = { _id: new ObjectId() }
+    this.thread = { _id: new ObjectId() }
     this.token = 'some-token'
 
     this.ProjectGetter = {
@@ -46,6 +48,14 @@ describe('AuthorizationManager', function () {
       },
     }
 
+    this.DocumentUpdaterHandler = {
+      promises: {
+        getComment: sinon
+          .stub()
+          .resolves({ metadata: { user_id: new ObjectId() } }),
+      },
+    }
+
     this.AuthorizationManager = SandboxedModule.require(modulePath, {
       requires: {
         'mongodb-legacy': { ObjectId },
@@ -54,6 +64,8 @@ describe('AuthorizationManager', function () {
         '../Project/ProjectGetter': this.ProjectGetter,
         '../../models/User': { User: this.User },
         '../TokenAccess/TokenAccessHandler': this.TokenAccessHandler,
+        '../DocumentUpdater/DocumentUpdaterHandler':
+          this.DocumentUpdaterHandler,
         '@overleaf/settings': {
           passwordStrengthOptions: {},
           adminPrivilegeAvailable: true,
@@ -70,6 +82,7 @@ describe('AuthorizationManager', function () {
         ['id', 'readAndWrite', true, true],
         ['id', 'readOnly', false, false],
         ['id', 'readOnly', false, true],
+        ['id', 'review', false, true],
       ]
       const restrictedScenarios = [
         [null, 'readOnly', false, false],
@@ -432,6 +445,7 @@ describe('AuthorizationManager', function () {
     siteAdmin: true,
     owner: true,
     readAndWrite: true,
+    review: true,
     readOnly: true,
     publicReadAndWrite: true,
     publicReadOnly: true,
@@ -439,6 +453,15 @@ describe('AuthorizationManager', function () {
     tokenReadOnly: true,
   })
 
+  testPermission('canUserReviewProjectContent', {
+    siteAdmin: true,
+    owner: true,
+    readAndWrite: true,
+    review: true,
+    publicReadAndWrite: true,
+    tokenReadAndWrite: true,
+  })
+
   testPermission('canUserWriteProjectContent', {
     siteAdmin: true,
     owner: true,
@@ -503,6 +526,80 @@ describe('AuthorizationManager', function () {
       })
     })
   })
+
+  describe('canUserReviewThread', 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(
+          this.user._id,
+          this.project._id,
+          this.doc._id,
+          this.thread._id,
+          this.token
+        )
+
+      expect(canResolve).to.equal(true)
+    })
+
+    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)
+
+      const canResolve =
+        await this.AuthorizationManager.promises.canUserResolveThread(
+          this.user._id,
+          this.project._id,
+          this.doc._id,
+          this.thread._id,
+          this.token
+        )
+
+      expect(canResolve).to.equal(false)
+    })
+
+    describe('when user has review permission', function () {
+      beforeEach(function () {
+        this.CollaboratorsGetter.promises.getMemberIdPrivilegeLevel
+          .withArgs(this.user._id, this.project._id)
+          .resolves(PrivilegeLevels.REVIEW)
+      })
+
+      it('should return false when user is not the comment author', async function () {
+        const canResolve =
+          await this.AuthorizationManager.promises.canUserResolveThread(
+            this.user._id,
+            this.project._id,
+            this.doc._id,
+            this.thread._id,
+            this.token
+          )
+
+        expect(canResolve).to.equal(false)
+      })
+
+      it('should return true when user is the comment author', async function () {
+        this.DocumentUpdaterHandler.promises.getComment
+          .withArgs(this.project._id, this.doc._id, this.thread._id)
+          .resolves({ metadata: { user_id: this.user._id } })
+
+        const canResolve =
+          await this.AuthorizationManager.promises.canUserResolveThread(
+            this.user._id,
+            this.project._id,
+            this.doc._id,
+            this.thread._id,
+            this.token
+          )
+
+        expect(canResolve).to.equal(true)
+      })
+    })
+  })
 })
 
 function testPermission(permission, privilegeLevels) {
@@ -525,6 +622,11 @@ function testPermission(permission, privilegeLevels) {
         expectPermission(permission, privilegeLevels.readAndWrite || false)
       })
 
+      describe('when user has review access', function () {
+        setupUserPrivilegeLevel(PrivilegeLevels.REVIEW)
+        expectPermission(permission, privilegeLevels.review || false)
+      })
+
       describe('when user has read-only access', function () {
         setupUserPrivilegeLevel(PrivilegeLevels.READ_ONLY)
         expectPermission(permission, privilegeLevels.readOnly || false)

services/web/test/unit/src/Authorization/AuthorizationMiddlewareTests.js

@@ -11,6 +11,8 @@ describe('AuthorizationMiddleware', function () {
   beforeEach(function () {
     this.userId = new ObjectId().toString()
     this.project_id = new ObjectId().toString()
+    this.doc_id = new ObjectId().toString()
+    this.thread_id = new ObjectId().toString()
     this.token = 'some-token'
     this.AuthenticationController = {}
     this.SessionManager = {
@@ -23,8 +25,10 @@ describe('AuthorizationMiddleware', function () {
         canUserReadProject: sinon.stub(),
         canUserWriteProjectSettings: sinon.stub(),
         canUserWriteProjectContent: sinon.stub(),
+        canUserResolveThread: sinon.stub(),
         canUserAdminProject: sinon.stub(),
         canUserRenameProject: sinon.stub(),
+        canUserReviewProjectContent: sinon.stub(),
         isUserSiteAdmin: sinon.stub(),
         isRestrictedUserForProject: sinon.stub(),
       },
@@ -35,6 +39,11 @@ describe('AuthorizationMiddleware', function () {
     this.TokenAccessHandler = {
       getRequestToken: sinon.stub().returns(this.token),
     }
+    this.DocumentUpdaterHandler = {
+      promises: {
+        getComment: sinon.stub().resolves(),
+      },
+    }
     this.AuthorizationMiddleware = SandboxedModule.require(MODULE_PATH, {
       requires: {
         './AuthorizationManager': this.AuthorizationManager,
@@ -47,6 +56,8 @@ describe('AuthorizationMiddleware', function () {
         '../Helpers/AdminAuthorizationHelper': {
           canRedirectToAdminDomain: sinon.stub().returns(false),
         },
+        '../DocumentUpdater/DocumentUpdaterHandler':
+          this.DocumentUpdaterHandler,
       },
     })
     this.req = {
@@ -75,6 +86,46 @@ describe('AuthorizationMiddleware', function () {
     )
   })
 
+  describe('ensureUserCanResolveThread', 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
+          .withArgs(
+            this.userId,
+            this.project_id,
+            this.doc_id,
+            this.thread_id,
+            this.token
+          )
+          .resolves(true)
+      })
+
+      invokeMiddleware('ensureUserCanResolveThread')
+      expectNext()
+    })
+
+    describe("when user doesn't have permission", function () {
+      beforeEach(function () {
+        this.AuthorizationManager.promises.canUserResolveThread
+          .withArgs(
+            this.userId,
+            this.project_id,
+            this.doc_id,
+            this.thread_id,
+            this.token
+          )
+          .resolves(false)
+      })
+
+      invokeMiddleware('ensureUserCanResolveThread')
+      expectForbidden()
+    })
+  })
+
   describe('ensureUserCanWriteProjectSettings', function () {
     describe('when renaming a project', function () {
       beforeEach(function () {

services/web/test/unit/src/DocumentUpdater/DocumentUpdaterHandlerTests.js

@@ -459,6 +459,40 @@ describe('DocumentUpdaterHandler', function () {
     })
   })
 
+  describe('getComment', function () {
+    describe('successfully', function () {
+      beforeEach(function () {
+        this.comment = {
+          id: 'mock-comment-id-1',
+        }
+        this.body = this.comment
+        this.request.callsArgWith(1, null, { statusCode: 200 }, this.body)
+        this.handler.getComment(
+          this.project_id,
+          this.doc_id,
+          this.comment.id,
+          this.callback
+        )
+      })
+
+      it('should get the comment from the document updater', function () {
+        const url = `${this.settings.apis.documentupdater.url}/project/${this.project_id}/doc/${this.doc_id}/comment/${this.comment.id}`
+        this.request
+          .calledWith({
+            url,
+            method: 'GET',
+            json: true,
+            timeout: 30 * 1000,
+          })
+          .should.equal(true)
+      })
+
+      it('should call the callback with the comment', function () {
+        this.callback.calledWithExactly(null, this.comment).should.equal(true)
+      })
+    })
+  })
+
   describe('getDocument', function () {
     describe('successfully', function () {
       beforeEach(function () {