Add getThread in Chat service and use it in AuthorizationMiddleware (#28041)

* Add getThread in Chat service and use it in AuthorizationMiddleware

* ensure user_id is a string, not ObjectId

* fix tests

GitOrigin-RevId: 42d63366b9b9350d7cdbcbc3b9f4761d9f55b49a

services/chat/app/js/Features/Messages/MessageHttpController.js

@@ -54,6 +54,10 @@ export async function getThreads(context) {
   return await callMessageHttpController(context, _getAllThreads)
 }
 
+export async function getThread(context) {
+  return await callMessageHttpController(context, _getThread)
+}
+
 export async function resolveThread(context) {
   return await callMessageHttpController(context, _resolveThread)
 }
@@ -144,6 +148,29 @@ const _generateThreadData = async (req, res) => {
   res.json(threadData)
 }
 
+const _getThread = async (req, res) => {
+  const { projectId, threadId } = req.params
+  logger.debug({ projectId, threadId }, 'getting specific thread')
+  try {
+    const room = await ThreadManager.findThread(projectId, threadId)
+    const messages = await MessageManager.findAllMessagesInRooms([room._id])
+    const threads = MessageFormatter.groupMessagesByThreads([room], messages)
+
+    const thread = threads[threadId] || null
+    if (!thread) {
+      res.status(404)
+      return
+    }
+    res.json(thread)
+  } catch (error) {
+    if (error instanceof ThreadManager.MissingThreadError) {
+      res.status(404)
+      return
+    }
+    throw error
+  }
+}
+
 const _resolveThread = async (req, res) => {
   const { projectId, threadId } = req.params
   const { user_id: userId } = req.body

services/chat/app/js/Features/Threads/ThreadManager.js

@@ -147,6 +147,17 @@ export async function duplicateThread(projectId, threadId) {
   return { oldRoom: room, newRoom }
 }
 
+export async function findThread(projectId, threadId) {
+  const room = await db.rooms.findOne({
+    project_id: new ObjectId(projectId),
+    thread_id: new ObjectId(threadId),
+  })
+  if (!room) {
+    throw new MissingThreadError('Thread not found')
+  }
+  return room
+}
+
 export async function findThreadsById(projectId, threadIds) {
   return await db.rooms
     .find({

services/chat/chat.yaml

@@ -270,6 +270,20 @@ paths:
         name: threadId
         in: path
         required: true
+    get:
+      summary: Get Thread
+      tags: []
+      responses:
+        '200':
+          description: OK
+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/Thread'
+        '404':
+          description: Thread not found
+      operationId: getThread
+      description: Get a thread with ThreadID and ProjectID provided
     delete:
       summary: Delete thread
       operationId: deleteThread

services/chat/test/acceptance/js/GettingAThreadTests.js

@@ -0,0 +1,67 @@
+import { ObjectId } from '../../../app/js/mongodb.js'
+import { expect } from 'chai'
+
+import * as ChatClient from './helpers/ChatClient.js'
+import * as ChatApp from './helpers/ChatApp.js'
+
+describe('Getting a thread', async function () {
+  const userId1 = new ObjectId().toString()
+  const userId2 = new ObjectId().toString()
+  const content1 = 'first message'
+  const content2 = 'second message'
+  const projectId = new ObjectId().toString()
+  const threadId = new ObjectId().toString()
+
+  before(async function () {
+    await ChatApp.ensureRunning()
+  })
+
+  describe('when thread exists', async function () {
+    before(async function () {
+      // Send first message to create thread
+      const { response } = await ChatClient.sendMessage(
+        projectId,
+        threadId,
+        userId1,
+        content1
+      )
+      expect(response.statusCode).to.equal(201)
+
+      // Send second message to the same thread
+      const { response: response2 } = await ChatClient.sendMessage(
+        projectId,
+        threadId,
+        userId2,
+        content2
+      )
+      expect(response2.statusCode).to.equal(201)
+    })
+
+    it('should return the thread with all messages', async function () {
+      const { response, body: thread } = await ChatClient.getThread(
+        projectId,
+        threadId
+      )
+      expect(response.statusCode).to.equal(200)
+      expect(thread).to.exist
+      expect(thread.messages).to.exist
+      expect(thread.messages.length).to.equal(2)
+
+      expect(thread.messages[0].content).to.equal(content1)
+      expect(thread.messages[0].user_id).to.equal(userId1)
+      expect(thread.messages[1].content).to.equal(content2)
+      expect(thread.messages[1].user_id).to.equal(userId2)
+    })
+  })
+
+  describe('when thread does not exist', async function () {
+    it('should return 404', async function () {
+      const nonExistentThreadId = new ObjectId().toString()
+      const { response } = await ChatClient.getThread(
+        projectId,
+        nonExistentThreadId
+      )
+      expect(response.statusCode).to.equal(404)
+    })
+  })
+})

services/chat/test/acceptance/js/helpers/ChatClient.js

@@ -46,6 +46,14 @@ export async function sendMessage(projectId, threadId, userId, content) {
   })
 }
 
+export async function getThread(projectId, threadId) {
+  return await asyncRequest({
+    method: 'get',
+    url: `/project/${projectId}/thread/${threadId}`,
+    json: true,
+  })
+}
+
 export async function getThreads(projectId) {
   return await asyncRequest({
     method: 'get',

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

@@ -14,7 +14,7 @@ const {
   getAdminCapabilities,
 } = require('../Helpers/AdminAuthorizationHelper')
 const Settings = require('@overleaf/settings')
-const DocumentUpdaterHandler = require('../DocumentUpdater/DocumentUpdaterHandler')
+const ChatApiHandler = require('../Chat/ChatApiHandler')
 
 function isRestrictedUser(
   userId,
@@ -337,7 +337,6 @@ async function isUserSiteAdmin(userId) {
 async function canUserDeleteOrResolveThread(
   userId,
   projectId,
-  docId,
   threadId,
   token
 ) {
@@ -358,12 +357,14 @@ async function canUserDeleteOrResolveThread(
     return false
   }
 
-  const comment = await DocumentUpdaterHandler.promises.getComment(
-    projectId,
-    docId,
-    threadId
-  )
-  return comment.metadata.user_id === userId
+  try {
+    const thread = await ChatApiHandler.promises.getThread(projectId, threadId)
+    // Check if the user created the thread (first message)
+    return thread.messages.length > 0 && thread.messages[0].user_id === userId
+  } catch (error) {
+    // If thread doesn't exist or other error, deny access
+    return false
+  }
 }
 
 module.exports = {

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

@@ -105,7 +105,6 @@ async function ensureUserCanWriteProjectSettings(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)
@@ -113,7 +112,6 @@ async function ensureUserCanDeleteOrResolveThread(req, res, next) {
     await AuthorizationManager.promises.canUserDeleteOrResolveThread(
       userId,
       projectId,
-      docId,
       threadId,
       token
     )
@@ -221,17 +219,6 @@ 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) {
@@ -246,7 +233,7 @@ function _getThreadId(req) {
 function _getUserId(req) {
   return (
     SessionManager.getLoggedInUserId(req.session) ||
-    (req.oauth_user && req.oauth_user._id) ||
+    (req.oauth_user && req.oauth_user._id?.toString()) ||
     null
   )
 }

services/web/app/src/Features/Chat/ChatApiHandler.js

@@ -4,6 +4,10 @@ const { fetchJson, fetchNothing } = require('@overleaf/fetch-utils')
 const settings = require('@overleaf/settings')
 const { callbackify } = require('util')
 
+async function getThread(projectId, threadId) {
+  return await fetchJson(chatApiUrl(`/project/${projectId}/thread/${threadId}`))
+}
+
 async function getThreads(projectId) {
   return await fetchJson(chatApiUrl(`/project/${projectId}/threads`))
 }
@@ -133,6 +137,7 @@ function chatApiUrl(path) {
 }
 
 module.exports = {
+  getThread: callbackify(getThread),
   getThreads: callbackify(getThreads),
   destroyProject: callbackify(destroyProject),
   sendGlobalMessage: callbackify(sendGlobalMessage),
@@ -148,6 +153,7 @@ module.exports = {
   duplicateCommentThreads: callbackify(duplicateCommentThreads),
   generateThreadData: callbackify(generateThreadData),
   promises: {
+    getThread,
     getThreads,
     destroyProject,
     sendGlobalMessage,

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

@@ -51,11 +51,11 @@ describe('AuthorizationManager', function () {
       },
     }
 
-    this.DocumentUpdaterHandler = {
+    this.ChatApiHandler = {
       promises: {
-        getComment: sinon
+        getThread: sinon
           .stub()
-          .resolves({ metadata: { user_id: new ObjectId() } }),
+          .resolves({ messages: [{ user_id: new ObjectId() }] }),
       },
     }
     this.Modules = { promises: { hooks: { fire: sinon.stub() } } }
@@ -73,8 +73,7 @@ describe('AuthorizationManager', function () {
         '../Project/ProjectGetter': this.ProjectGetter,
         '../../models/User': { User: this.User },
         '../TokenAccess/TokenAccessHandler': this.TokenAccessHandler,
-        '../DocumentUpdater/DocumentUpdaterHandler':
-          this.DocumentUpdaterHandler,
+        '../Chat/ChatApiHandler': this.ChatApiHandler,
         '../../infrastructure/Modules': this.Modules,
         '@overleaf/settings': this.settings,
       },
@@ -618,7 +617,6 @@ describe('AuthorizationManager', function () {
         await this.AuthorizationManager.promises.canUserDeleteOrResolveThread(
           this.user._id,
           this.project._id,
-          this.doc._id,
           this.thread._id,
           this.token
         )
@@ -641,7 +639,6 @@ describe('AuthorizationManager', function () {
         await this.AuthorizationManager.promises.canUserDeleteOrResolveThread(
           this.user._id,
           this.project._id,
-          this.doc._id,
           this.thread._id,
           this.token
         )
@@ -667,7 +664,6 @@ describe('AuthorizationManager', function () {
           await this.AuthorizationManager.promises.canUserDeleteOrResolveThread(
             this.user._id,
             this.project._id,
-            this.doc._id,
             this.thread._id,
             this.token
           )
@@ -675,16 +671,15 @@ describe('AuthorizationManager', function () {
         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 } })
+      it('should return true when user is the thread author', async function () {
+        this.ChatApiHandler.promises.getThread
+          .withArgs(this.project._id, this.thread._id)
+          .resolves({ messages: [{ user_id: this.user._id }] })
 
         const canResolve =
           await this.AuthorizationManager.promises.canUserDeleteOrResolveThread(
             this.user._id,
             this.project._id,
-            this.doc._id,
             this.thread._id,
             this.token
           )

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

@@ -95,19 +95,12 @@ describe('AuthorizationMiddleware', 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.canUserDeleteOrResolveThread
-          .withArgs(
-            this.userId,
-            this.project_id,
-            this.doc_id,
-            this.thread_id,
-            this.token
-          )
+          .withArgs(this.userId, this.project_id, this.thread_id, this.token)
           .resolves(true)
       })
 
@@ -118,13 +111,7 @@ describe('AuthorizationMiddleware', function () {
     describe("when user doesn't have permission", function () {
       beforeEach(function () {
         this.AuthorizationManager.promises.canUserDeleteOrResolveThread
-          .withArgs(
-            this.userId,
-            this.project_id,
-            this.doc_id,
-            this.thread_id,
-            this.token
-          )
+          .withArgs(this.userId, this.project_id, this.thread_id, this.token)
           .resolves(false)
       })