diff --git a/services/chat/app/js/Features/Messages/MessageHttpController.js b/services/chat/app/js/Features/Messages/MessageHttpController.js index 45208e2c03..fc587e4549 100644 --- a/services/chat/app/js/Features/Messages/MessageHttpController.js +++ b/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 diff --git a/services/chat/app/js/Features/Threads/ThreadManager.js b/services/chat/app/js/Features/Threads/ThreadManager.js index 5697b39393..9cab1e2f72 100644 --- a/services/chat/app/js/Features/Threads/ThreadManager.js +++ b/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({ diff --git a/services/chat/chat.yaml b/services/chat/chat.yaml index 35ed3d378d..ce07d84a04 100644 --- a/services/chat/chat.yaml +++ b/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 diff --git a/services/chat/test/acceptance/js/GettingAThreadTests.js b/services/chat/test/acceptance/js/GettingAThreadTests.js new file mode 100644 index 0000000000..3d9be34b25 --- /dev/null +++ b/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) + }) + }) +}) diff --git a/services/chat/test/acceptance/js/helpers/ChatClient.js b/services/chat/test/acceptance/js/helpers/ChatClient.js index 6a8196dd02..98314bb22a 100644 --- a/services/chat/test/acceptance/js/helpers/ChatClient.js +++ b/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', diff --git a/services/web/app/src/Features/Authorization/AuthorizationManager.js b/services/web/app/src/Features/Authorization/AuthorizationManager.js index 07a556dc09..b111ef0139 100644 --- a/services/web/app/src/Features/Authorization/AuthorizationManager.js +++ b/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 = { diff --git a/services/web/app/src/Features/Authorization/AuthorizationMiddleware.js b/services/web/app/src/Features/Authorization/AuthorizationMiddleware.js index 02043442c8..142264c493 100644 --- a/services/web/app/src/Features/Authorization/AuthorizationMiddleware.js +++ b/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 ) } diff --git a/services/web/app/src/Features/Chat/ChatApiHandler.js b/services/web/app/src/Features/Chat/ChatApiHandler.js index 2929891c8c..7d03e7eb08 100644 --- a/services/web/app/src/Features/Chat/ChatApiHandler.js +++ b/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, diff --git a/services/web/test/unit/src/Authorization/AuthorizationManagerTests.js b/services/web/test/unit/src/Authorization/AuthorizationManagerTests.js index cd99af3e29..20ef3bcb0e 100644 --- a/services/web/test/unit/src/Authorization/AuthorizationManagerTests.js +++ b/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 ) diff --git a/services/web/test/unit/src/Authorization/AuthorizationMiddlewareTests.js b/services/web/test/unit/src/Authorization/AuthorizationMiddlewareTests.js index 16514fbe63..c0ab761f27 100644 --- a/services/web/test/unit/src/Authorization/AuthorizationMiddlewareTests.js +++ b/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) })