From eccd40dbcae7e6b196bd04edce78d0ed6b2c04b0 Mon Sep 17 00:00:00 2001 From: gabriellsh <40830821+gabriellsh@users.noreply.github.com> Date: Tue, 19 Nov 2024 18:28:19 -0300 Subject: [PATCH] feat: Fetch avatars using user id (#33214) Co-authored-by: Marcos Spessatto Defendi --- .changeset/plenty-snakes-dream.md | 5 + apps/meteor/server/models/raw/Avatars.ts | 8 +- .../server/models/raw/BaseUploadModel.ts | 1 + apps/meteor/server/routes/avatar/index.ts | 3 +- .../routes/avatar/middlewares/auth.spec.ts | 55 +++++-- .../server/routes/avatar/middlewares/auth.ts | 3 + .../server/routes/avatar/middlewares/index.ts | 3 +- apps/meteor/server/routes/avatar/user.spec.ts | 138 +++++++++++++++++- apps/meteor/server/routes/avatar/user.ts | 60 ++++++++ .../model-typings/src/models/IAvatarsModel.ts | 7 +- 10 files changed, 267 insertions(+), 16 deletions(-) create mode 100644 .changeset/plenty-snakes-dream.md diff --git a/.changeset/plenty-snakes-dream.md b/.changeset/plenty-snakes-dream.md new file mode 100644 index 000000000000..eecbf0cbb466 --- /dev/null +++ b/.changeset/plenty-snakes-dream.md @@ -0,0 +1,5 @@ +--- +"@rocket.chat/meteor": minor +--- + +Adds a new route to allow fetching avatars by the user's id `/avatar/uid/` diff --git a/apps/meteor/server/models/raw/Avatars.ts b/apps/meteor/server/models/raw/Avatars.ts index 1e28c878848a..c4935c18d6be 100644 --- a/apps/meteor/server/models/raw/Avatars.ts +++ b/apps/meteor/server/models/raw/Avatars.ts @@ -1,6 +1,6 @@ -import type { IAvatar, RocketChatRecordDeleted } from '@rocket.chat/core-typings'; +import type { IAvatar, RocketChatRecordDeleted, IUser } from '@rocket.chat/core-typings'; import type { IAvatarsModel } from '@rocket.chat/model-typings'; -import type { Collection, Db } from 'mongodb'; +import type { Collection, Db, FindOptions } from 'mongodb'; import { BaseUploadModelRaw } from './BaseUploadModel'; @@ -8,4 +8,8 @@ export class AvatarsRaw extends BaseUploadModelRaw implements IAvatarsModel { constructor(db: Db, trash?: Collection>) { super(db, 'avatars', trash); } + + findOneByUserId(userId: IUser['_id'], options?: FindOptions) { + return this.findOne({ userId }, options); + } } diff --git a/apps/meteor/server/models/raw/BaseUploadModel.ts b/apps/meteor/server/models/raw/BaseUploadModel.ts index 4037566272b9..6d2760a19f78 100644 --- a/apps/meteor/server/models/raw/BaseUploadModel.ts +++ b/apps/meteor/server/models/raw/BaseUploadModel.ts @@ -19,6 +19,7 @@ type T = IUpload; export abstract class BaseUploadModelRaw extends BaseRaw implements IBaseUploadsModel { protected modelIndexes(): IndexDescription[] { return [ + { key: { userId: 1 }, sparse: true }, { key: { name: 1 }, sparse: true }, { key: { rid: 1 }, sparse: true }, { key: { expiresAt: 1 }, sparse: true }, diff --git a/apps/meteor/server/routes/avatar/index.ts b/apps/meteor/server/routes/avatar/index.ts index 3cffaa336241..5a6adfffdd2a 100644 --- a/apps/meteor/server/routes/avatar/index.ts +++ b/apps/meteor/server/routes/avatar/index.ts @@ -1,9 +1,10 @@ import { WebApp } from 'meteor/webapp'; import { roomAvatar } from './room'; -import { userAvatarByUsername } from './user'; +import { userAvatarByUsername, userAvatarById } from './user'; import './middlewares'; WebApp.connectHandlers.use('/avatar/room/', roomAvatar); +WebApp.connectHandlers.use('/avatar/uid/', userAvatarById); WebApp.connectHandlers.use('/avatar/', userAvatarByUsername); diff --git a/apps/meteor/server/routes/avatar/middlewares/auth.spec.ts b/apps/meteor/server/routes/avatar/middlewares/auth.spec.ts index cffa82eabd0b..05292fb16c48 100644 --- a/apps/meteor/server/routes/avatar/middlewares/auth.spec.ts +++ b/apps/meteor/server/routes/avatar/middlewares/auth.spec.ts @@ -10,7 +10,7 @@ const mocks = { }, }; -const { protectAvatarsWithFallback } = proxyquire.noCallThru().load('./auth.ts', { +const { protectAvatarsWithFallback, protectAvatars } = proxyquire.noCallThru().load('./auth.ts', { '../utils': mocks.utils, }); @@ -54,10 +54,10 @@ describe('#protectAvatarsWithFallback()', () => { expect(response.end.calledOnce).to.be.true; }); - it(`should write 200 to head and write fallback to body (user avatar)`, async () => { + it(`should write 200 to head and write fallback to body (room avatar)`, async () => { mocks.utils.renderSVGLetters.returns('fallback'); - await protectAvatarsWithFallback({ url: '/jon' }, response, next); + await protectAvatarsWithFallback({ url: '/room/jon' }, response, next); expect(next.called).to.be.false; expect(response.setHeader.called).to.be.false; expect(response.writeHead.calledWith(200, { 'Content-Type': 'image/svg+xml' })).to.be.true; @@ -65,14 +65,51 @@ describe('#protectAvatarsWithFallback()', () => { expect(response.end.calledOnce).to.be.true; }); - it(`should write 200 to head and write fallback to body (room avatar)`, async () => { - mocks.utils.renderSVGLetters.returns('fallback'); + it(`should call next if user can access avatar`, async () => { + mocks.utils.userCanAccessAvatar.returns(true); + const request = { url: '/jon' }; + + await protectAvatarsWithFallback(request, response, next); + expect(mocks.utils.userCanAccessAvatar.calledWith(request)).to.be.true; + expect(next.called).to.be.true; + }); +}); + +describe('#protectAvatars()', () => { + const response = { + setHeader: sinon.spy(), + writeHead: sinon.spy(), + write: sinon.spy(), + end: sinon.spy(), + }; + const next = sinon.spy(); + + afterEach(() => { + response.setHeader.resetHistory(); + response.writeHead.resetHistory(); + response.end.resetHistory(); + next.resetHistory(); + + Object.values(mocks.utils).forEach((mock) => mock.reset()); + }); + + it(`should write 404 to head if no url provided`, async () => { + await protectAvatars({}, response, next); - await protectAvatarsWithFallback({ url: '/room/jon' }, response, next); expect(next.called).to.be.false; expect(response.setHeader.called).to.be.false; - expect(response.writeHead.calledWith(200, { 'Content-Type': 'image/svg+xml' })).to.be.true; - expect(response.write.calledWith('fallback')).to.be.true; + expect(response.writeHead.calledWith(404)).to.be.true; + expect(response.end.calledOnce).to.be.true; + }); + + it(`should write 404 to head if access is denied`, async () => { + mocks.utils.userCanAccessAvatar.returns(false); + + await protectAvatars({ url: '/room/jon' }, response, next); + + expect(next.called).to.be.false; + expect(response.setHeader.called).to.be.false; + expect(response.writeHead.calledWith(404)).to.be.true; expect(response.end.calledOnce).to.be.true; }); @@ -80,7 +117,7 @@ describe('#protectAvatarsWithFallback()', () => { mocks.utils.userCanAccessAvatar.returns(true); const request = { url: '/jon' }; - await protectAvatarsWithFallback(request, response, next); + await protectAvatars(request, response, next); expect(mocks.utils.userCanAccessAvatar.calledWith(request)).to.be.true; expect(next.called).to.be.true; }); diff --git a/apps/meteor/server/routes/avatar/middlewares/auth.ts b/apps/meteor/server/routes/avatar/middlewares/auth.ts index f7bf2df271ac..958e05379c88 100644 --- a/apps/meteor/server/routes/avatar/middlewares/auth.ts +++ b/apps/meteor/server/routes/avatar/middlewares/auth.ts @@ -42,3 +42,6 @@ const getProtectAvatars = (callback?: typeof renderFallback) => async (req: Inco // If unauthorized returns the SVG fallback (letter avatar) export const protectAvatarsWithFallback = getProtectAvatars(renderFallback); + +// Just returns 404 +export const protectAvatars = getProtectAvatars(); diff --git a/apps/meteor/server/routes/avatar/middlewares/index.ts b/apps/meteor/server/routes/avatar/middlewares/index.ts index 0cec2b07cdbd..659c6e926624 100644 --- a/apps/meteor/server/routes/avatar/middlewares/index.ts +++ b/apps/meteor/server/routes/avatar/middlewares/index.ts @@ -1,7 +1,8 @@ import { WebApp } from 'meteor/webapp'; -import { protectAvatarsWithFallback } from './auth'; +import { protectAvatarsWithFallback, protectAvatars } from './auth'; import { handleBrowserVersionCheck } from './browserVersion'; WebApp.connectHandlers.use(handleBrowserVersionCheck); +WebApp.connectHandlers.use('/avatar/uid/', protectAvatars); WebApp.connectHandlers.use('/avatar/', protectAvatarsWithFallback); diff --git a/apps/meteor/server/routes/avatar/user.spec.ts b/apps/meteor/server/routes/avatar/user.spec.ts index d34948cd3964..f5e6d87dbb63 100644 --- a/apps/meteor/server/routes/avatar/user.spec.ts +++ b/apps/meteor/server/routes/avatar/user.spec.ts @@ -7,6 +7,7 @@ import sinon from 'sinon'; const mocks = { settingsGet: sinon.stub(), findOneByUsernameIgnoringCase: sinon.stub(), + findOneById: sinon.stub(), utils: { serveSvgAvatarInRequestedFormat: sinon.spy(), wasFallbackModified: sinon.stub(), @@ -15,15 +16,18 @@ const mocks = { }, serverFetch: sinon.stub(), avatarFindOneByName: sinon.stub(), + avatarFindOneByUserId: sinon.stub(), }; -const { userAvatarByUsername } = proxyquire.noCallThru().load('./user', { +const { userAvatarById, userAvatarByUsername } = proxyquire.noCallThru().load('./user', { '@rocket.chat/models': { Users: { findOneByUsernameIgnoringCase: mocks.findOneByUsernameIgnoringCase, + findOneById: mocks.findOneById, }, Avatars: { findOneByName: mocks.avatarFindOneByName, + findOneByUserId: mocks.avatarFindOneByUserId, }, }, '../../../app/settings/server': { @@ -37,6 +41,138 @@ const { userAvatarByUsername } = proxyquire.noCallThru().load('./user', { }, }); +describe('#userAvatarById()', () => { + const response = { + setHeader: sinon.spy(), + writeHead: sinon.spy(), + end: sinon.spy(), + }; + const next = sinon.spy(); + + afterEach(() => { + mocks.settingsGet.reset(); + mocks.avatarFindOneByUserId.reset(); + + response.setHeader.resetHistory(); + response.writeHead.resetHistory(); + response.end.resetHistory(); + next.resetHistory(); + + Object.values(mocks.utils).forEach((mock) => ('reset' in mock ? mock.reset() : mock.resetHistory())); + }); + + it(`should do nothing if url is not in request object`, async () => { + await userAvatarById({}, response, next); + expect(next.called).to.be.false; + expect(response.setHeader.called).to.be.false; + expect(response.writeHead.called).to.be.false; + expect(response.end.called).to.be.false; + }); + + it(`should write 404 if Id is not provided`, async () => { + await userAvatarById({ url: '/' }, response, next); + expect(next.called).to.be.false; + expect(response.setHeader.called).to.be.false; + expect(response.writeHead.calledWith(404)).to.be.true; + expect(response.end.calledOnce).to.be.true; + }); + + it(`should call external provider`, async () => { + const userId = 'xvf5Tr34'; + const request = { url: `/${userId}` }; + + const pipe = sinon.spy(); + const mockResponseHeaders = new Headers(); + mockResponseHeaders.set('header1', 'true'); + mockResponseHeaders.set('header2', 'false'); + + mocks.serverFetch.returns({ + headers: mockResponseHeaders, + body: { pipe }, + }); + + mocks.settingsGet.returns('test123/{username}'); + + mocks.findOneById.returns({ username: 'jon' }); + + await userAvatarById(request, response, next); + + expect(mocks.utils.setCacheAndDispositionHeaders.calledWith(request, response)).to.be.true; + expect(mocks.findOneById.calledWith(userId)).to.be.true; + expect(mocks.serverFetch.calledWith('test123/jon')).to.be.true; + expect(response.setHeader.calledTwice).to.be.true; + expect(response.setHeader.getCall(0).calledWith('header1', 'true')).to.be.true; + expect(response.setHeader.getCall(1).calledWith('header2', 'false')).to.be.true; + expect(pipe.calledWith(response)).to.be.true; + }); + + it(`should serve avatar file if found`, async () => { + const request = { url: '/jon' }; + + const file = { uploadedAt: new Date(0), type: 'image/png', size: 100 }; + mocks.avatarFindOneByUserId.returns(file); + + await userAvatarById(request, response, next); + + expect(mocks.utils.setCacheAndDispositionHeaders.calledWith(request, response)).to.be.true; + expect(mocks.utils.serveAvatarFile.calledWith(file, request, response, next)).to.be.true; + }); + + it(`should write 304 to head if content is not modified`, async () => { + const request = { url: '/xyzabc', headers: {} }; + + mocks.utils.wasFallbackModified.returns(false); + + await userAvatarById(request, response, next); + + expect(mocks.utils.setCacheAndDispositionHeaders.calledWith(request, response)).to.be.true; + expect(response.writeHead.calledWith(304)).to.be.true; + expect(response.end.calledOnce).to.be.true; + }); + + it(`should write 404 if userId is not found`, async () => { + mocks.utils.wasFallbackModified.returns(true); + mocks.findOneById.returns(null); + + const userId = 'awdasdaw'; + const request = { url: `/${userId}`, headers: {} }; + + await userAvatarById(request, response, next); + expect(mocks.utils.setCacheAndDispositionHeaders.calledWith(request, response)).to.be.true; + + expect(response.writeHead.calledWith(404)).to.be.true; + expect(response.end.calledOnce).to.be.true; + }); + + it(`should fallback to SVG if no avatar found`, async () => { + const userId = '2apso9283'; + const request = { url: `/${userId}`, headers: {} }; + + mocks.findOneById.returns({ username: 'jon' }); + mocks.utils.wasFallbackModified.returns(true); + + await userAvatarById(request, response, next); + + expect(mocks.findOneById.calledWith(userId)).to.be.true; + expect(mocks.utils.setCacheAndDispositionHeaders.calledWith(request, response)).to.be.true; + expect(mocks.utils.serveSvgAvatarInRequestedFormat.calledWith({ nameOrUsername: 'jon', req: request, res: response })).to.be.true; + }); + + it(`should fallback to SVG with user name if UI_Use_Name_Avatar is true`, async () => { + const userId = '2apso9283'; + const request = { url: `/${userId}`, headers: {} }; + + mocks.findOneById.returns({ username: 'jon', name: 'Doe' }); + mocks.utils.wasFallbackModified.returns(true); + mocks.settingsGet.withArgs('UI_Use_Name_Avatar').returns(true); + + await userAvatarById(request, response, next); + + expect(mocks.utils.setCacheAndDispositionHeaders.calledWith(request, response)).to.be.true; + expect(mocks.utils.serveSvgAvatarInRequestedFormat.calledWith({ nameOrUsername: 'Doe', req: request, res: response })).to.be.true; + }); +}); + describe('#userAvatarByUsername()', () => { const response = { setHeader: sinon.spy(), diff --git a/apps/meteor/server/routes/avatar/user.ts b/apps/meteor/server/routes/avatar/user.ts index 4930ad044244..7b20287ddac3 100644 --- a/apps/meteor/server/routes/avatar/user.ts +++ b/apps/meteor/server/routes/avatar/user.ts @@ -1,5 +1,6 @@ import type { IncomingMessage, ServerResponse } from 'http'; +import type { IUser } from '@rocket.chat/apps-engine/definition/users'; import type { IIncomingMessage } from '@rocket.chat/core-typings'; import { Avatars, Users } from '@rocket.chat/models'; import { serverFetch as fetch } from '@rocket.chat/server-fetch'; @@ -72,3 +73,62 @@ export const userAvatarByUsername = async function (request: IncomingMessage, re serveSvgAvatarInRequestedFormat({ nameOrUsername: requestUsername, req, res }); }; + +export const userAvatarById = async function (request: IncomingMessage, res: ServerResponse, next: NextFunction) { + const req = request as IIncomingMessage; + + if (!req.url) { + return; + } + + // replace removes the query string + const requestUserId = decodeURIComponent(req.url.slice(1).replace(/\?.*$/, '')); + if (!requestUserId) { + res.writeHead(404); + res.end(); + return; + } + + setCacheAndDispositionHeaders(req, res); + + const externalProviderUrl = settings.get('Accounts_AvatarExternalProviderUrl'); + if (externalProviderUrl) { + const user = await Users.findOneById>(requestUserId, { projection: { username: 1 } }); + + if (!user?.username) { + res.writeHead(404); + res.end(); + return; + } + + void handleExternalProvider(externalProviderUrl, user.username, res); + return; + } + + const file = await Avatars.findOneByUserId(requestUserId); + if (file) { + void serveAvatarFile(file, req, res, next); + return; + } + + if (!wasFallbackModified(req.headers['if-modified-since'])) { + res.writeHead(304); + res.end(); + return; + } + + const user = await Users.findOneById>(requestUserId, { projection: { username: 1, name: 1 } }); + if (!user?.username) { + res.writeHead(404); + res.end(); + return; + } + + // Use real name for SVG letters + if (settings.get('UI_Use_Name_Avatar') && user?.name) { + serveSvgAvatarInRequestedFormat({ nameOrUsername: user.name, req, res }); + return; + } + + serveSvgAvatarInRequestedFormat({ nameOrUsername: user.username, req, res }); +}; diff --git a/packages/model-typings/src/models/IAvatarsModel.ts b/packages/model-typings/src/models/IAvatarsModel.ts index 5c8e6c7f0f06..4f9ea5bb3706 100644 --- a/packages/model-typings/src/models/IAvatarsModel.ts +++ b/packages/model-typings/src/models/IAvatarsModel.ts @@ -1,5 +1,8 @@ -import type { IAvatar } from '@rocket.chat/core-typings'; +import type { IAvatar, IUser } from '@rocket.chat/core-typings'; +import type { FindOptions } from 'mongodb'; import type { IBaseUploadsModel } from './IBaseUploadsModel'; -export type IAvatarsModel = IBaseUploadsModel; +export interface IAvatarsModel extends IBaseUploadsModel { + findOneByUserId(userId: IUser['_id'], options?: FindOptions): Promise; +}