From 8693edbf6af3c20fd02506f3368c01ee641511b5 Mon Sep 17 00:00:00 2001 From: Erik Michelson Date: Sun, 24 Mar 2024 01:02:26 +0100 Subject: [PATCH] refactor(media): add media redirection endpoint Previous versions of HedgeDoc suffered from the problem that changing the media backend required manipulation of the media links in all created notes. We discussed in #3704 that it's favourable to have an endpoint that redirects to the image's original URL. When changing the media backend, the link stays the same but just the redirect changes. Signed-off-by: Erik Michelson --- .../src/api/private/media/media.controller.ts | 14 +++++++++ .../src/api/public/media/media.controller.ts | 14 +++++++++ backend/src/media/media-upload-url.dto.ts | 15 --------- backend/src/media/media-upload.dto.ts | 6 ++-- backend/src/media/media.service.ts | 2 +- backend/test/private-api/me.e2e-spec.ts | 28 ++++++++--------- backend/test/private-api/media.e2e-spec.ts | 20 ++++++------ backend/test/private-api/notes.e2e-spec.ts | 7 ++--- backend/test/public-api/me.e2e-spec.ts | 31 +++++++++---------- backend/test/public-api/media.e2e-spec.ts | 16 +++++----- backend/test/public-api/notes.e2e-spec.ts | 7 ++--- frontend/cypress/e2e/fileUpload.spec.ts | 12 +++---- frontend/src/api/media/types.ts | 2 +- .../editor-pane/hooks/use-handle-upload.tsx | 9 ++++-- frontend/src/pages/api/private/me/media.ts | 6 ++-- frontend/src/pages/api/private/media.ts | 2 +- 16 files changed, 104 insertions(+), 87 deletions(-) delete mode 100644 backend/src/media/media-upload-url.dto.ts diff --git a/backend/src/api/private/media/media.controller.ts b/backend/src/api/private/media/media.controller.ts index 08d74a0b9..3a90dbef6 100644 --- a/backend/src/api/private/media/media.controller.ts +++ b/backend/src/api/private/media/media.controller.ts @@ -7,14 +7,17 @@ import { BadRequestException, Controller, Delete, + Get, Param, Post, + Res, UploadedFile, UseGuards, UseInterceptors, } from '@nestjs/common'; import { FileInterceptor } from '@nestjs/platform-express'; import { ApiBody, ApiConsumes, ApiHeader, ApiTags } from '@nestjs/swagger'; +import { Response } from 'express'; import { PermissionError } from '../../../errors/errors'; import { SessionGuard } from '../../../identity/session.guard'; @@ -101,6 +104,17 @@ export class MediaController { return await this.mediaService.toMediaUploadDto(upload); } + @Get(':filename') + @OpenApi(404, 500) + async getMedia( + @Param('filename') filename: string, + @Res() response: Response, + ): Promise { + const mediaUpload = await this.mediaService.findUploadByFilename(filename); + const targetUrl = mediaUpload.fileUrl; + response.redirect(targetUrl); + } + @Delete(':filename') @OpenApi(204, 403, 404, 500) async deleteMedia( diff --git a/backend/src/api/public/media/media.controller.ts b/backend/src/api/public/media/media.controller.ts index f11356c13..71d169047 100644 --- a/backend/src/api/public/media/media.controller.ts +++ b/backend/src/api/public/media/media.controller.ts @@ -7,8 +7,10 @@ import { BadRequestException, Controller, Delete, + Get, Param, Post, + Res, UploadedFile, UseGuards, UseInterceptors, @@ -21,6 +23,7 @@ import { ApiSecurity, ApiTags, } from '@nestjs/swagger'; +import { Response } from 'express'; import { TokenAuthGuard } from '../../../auth/token.strategy'; import { PermissionError } from '../../../errors/errors'; @@ -101,6 +104,17 @@ export class MediaController { return await this.mediaService.toMediaUploadDto(upload); } + @Get(':filename') + @OpenApi(404, 500) + async getMedia( + @Param('filename') filename: string, + @Res() response: Response, + ): Promise { + const mediaUpload = await this.mediaService.findUploadByFilename(filename); + const targetUrl = mediaUpload.fileUrl; + response.redirect(targetUrl); + } + @Delete(':filename') @OpenApi(204, 403, 404, 500) async deleteMedia( diff --git a/backend/src/media/media-upload-url.dto.ts b/backend/src/media/media-upload-url.dto.ts deleted file mode 100644 index 47a1e2a78..000000000 --- a/backend/src/media/media-upload-url.dto.ts +++ /dev/null @@ -1,15 +0,0 @@ -/* - * SPDX-FileCopyrightText: 2022 The HedgeDoc developers (see AUTHORS file) - * - * SPDX-License-Identifier: AGPL-3.0-only - */ -import { ApiProperty } from '@nestjs/swagger'; -import { IsString } from 'class-validator'; - -import { BaseDto } from '../utils/base.dto.'; - -export class MediaUploadUrlDto extends BaseDto { - @IsString() - @ApiProperty() - link: string; -} diff --git a/backend/src/media/media-upload.dto.ts b/backend/src/media/media-upload.dto.ts index 069e40100..ac7512dd8 100644 --- a/backend/src/media/media-upload.dto.ts +++ b/backend/src/media/media-upload.dto.ts @@ -12,12 +12,12 @@ import { Username } from '../utils/username'; export class MediaUploadDto extends BaseDto { /** - * The link to the media file. - * @example "https://example.com/uploads/testfile123.jpg" + * The id of the media file. + * @example "testfile123.jpg" */ @IsString() @ApiProperty() - url: string; + id: string; /** * The publicId of the note to which the uploaded file is linked to. diff --git a/backend/src/media/media.service.ts b/backend/src/media/media.service.ts index 6d17b12a7..0bac3fa67 100644 --- a/backend/src/media/media.service.ts +++ b/backend/src/media/media.service.ts @@ -232,7 +232,7 @@ export class MediaService { async toMediaUploadDto(mediaUpload: MediaUpload): Promise { const user = await mediaUpload.user; return { - url: mediaUpload.fileUrl, + id: mediaUpload.id, notePublicId: (await mediaUpload.note)?.publicId ?? null, createdAt: mediaUpload.createdAt, username: user?.username ?? null, diff --git a/backend/test/private-api/me.e2e-spec.ts b/backend/test/private-api/me.e2e-spec.ts index ee28b0fc7..9c6890551 100644 --- a/backend/test/private-api/me.e2e-spec.ts +++ b/backend/test/private-api/me.e2e-spec.ts @@ -68,18 +68,18 @@ describe('Me', () => { expect(responseBefore.body).toHaveLength(0); const testImage = await fs.readFile('test/public-api/fixtures/test.png'); - const imageUrls = []; - imageUrls.push( - (await testSetup.mediaService.saveFile(testImage, user, note1)).fileUrl, + const imageIds = []; + imageIds.push( + (await testSetup.mediaService.saveFile(testImage, user, note1)).id, ); - imageUrls.push( - (await testSetup.mediaService.saveFile(testImage, user, note1)).fileUrl, + imageIds.push( + (await testSetup.mediaService.saveFile(testImage, user, note1)).id, ); - imageUrls.push( - (await testSetup.mediaService.saveFile(testImage, user, note2)).fileUrl, + imageIds.push( + (await testSetup.mediaService.saveFile(testImage, user, note2)).id, ); - imageUrls.push( - (await testSetup.mediaService.saveFile(testImage, user, note2)).fileUrl, + imageIds.push( + (await testSetup.mediaService.saveFile(testImage, user, note2)).id, ); const response = await agent @@ -87,10 +87,10 @@ describe('Me', () => { .expect('Content-Type', /json/) .expect(200); expect(response.body).toHaveLength(4); - expect(imageUrls).toContain(response.body[0].url); - expect(imageUrls).toContain(response.body[1].url); - expect(imageUrls).toContain(response.body[2].url); - expect(imageUrls).toContain(response.body[3].url); + expect(imageIds).toContain(response.body[0].id); + expect(imageIds).toContain(response.body[1].id); + expect(imageIds).toContain(response.body[2].id); + expect(imageIds).toContain(response.body[3].id); const mediaUploads = await testSetup.mediaService.listUploadsByUser(user); for (const upload of mediaUploads) { await testSetup.mediaService.deleteFile(upload); @@ -122,7 +122,7 @@ describe('Me', () => { expect(dbUser).toBeInstanceOf(User); const mediaUploads = await testSetup.mediaService.listUploadsByUser(dbUser); expect(mediaUploads).toHaveLength(1); - expect(mediaUploads[0].fileUrl).toEqual(upload.fileUrl); + expect(mediaUploads[0].id).toEqual(upload.id); await agent.delete('/api/private/me').expect(204); await expect( testSetup.userService.getUserByUsername('hardcoded'), diff --git a/backend/test/private-api/media.e2e-spec.ts b/backend/test/private-api/media.e2e-spec.ts index 16bf34b27..03fb5242e 100644 --- a/backend/test/private-api/media.e2e-spec.ts +++ b/backend/test/private-api/media.e2e-spec.ts @@ -71,14 +71,15 @@ describe('Media', () => { .set('HedgeDoc-Note', 'test_upload_media') .expect('Content-Type', /json/) .expect(201); - const path: string = uploadResponse.body.url; + const fileName: string = uploadResponse.body.id; const testImage = await fs.readFile( 'test/private-api/fixtures/test.png', ); - const downloadResponse = await agent.get(path); + const path = '/api/private/media/' + fileName; + const apiResponse = await agent.get(path); + expect(apiResponse.statusCode).toEqual(302); + const downloadResponse = await agent.get(apiResponse.header.location); expect(downloadResponse.body).toEqual(testImage); - // Remove /uploads/ from path as we just need the filename. - const fileName = path.replace('/uploads/', ''); // delete the file afterwards await fs.unlink(join(uploadPath, fileName)); }); @@ -90,14 +91,15 @@ describe('Media', () => { .set('HedgeDoc-Note', 'test_upload_media') .expect('Content-Type', /json/) .expect(201); - const path: string = uploadResponse.body.url; + const fileName: string = uploadResponse.body.id; const testImage = await fs.readFile( 'test/private-api/fixtures/test.png', ); - const downloadResponse = await agent.get(path); + const path = '/api/private/media/' + fileName; + const apiResponse = await agent.get(path); + expect(apiResponse.statusCode).toEqual(302); + const downloadResponse = await agent.get(apiResponse.header.location); expect(downloadResponse.body).toEqual(testImage); - // Remove /uploads/ from path as we just need the filename. - const fileName = path.replace('/uploads/', ''); // delete the file afterwards await fs.unlink(join(uploadPath, fileName)); }); @@ -160,7 +162,7 @@ describe('Media', () => { user, testNote, ); - const filename = upload.fileUrl.split('/').pop() || ''; + const filename = upload.id; // login with a different user; const agent2 = request.agent(testSetup.app.getHttpServer()); diff --git a/backend/test/private-api/notes.e2e-spec.ts b/backend/test/private-api/notes.e2e-spec.ts index 12ea4cecf..8dc321076 100644 --- a/backend/test/private-api/notes.e2e-spec.ts +++ b/backend/test/private-api/notes.e2e-spec.ts @@ -412,12 +412,11 @@ describe('Notes', () => { .expect('Content-Type', /json/) .expect(200); expect(responseAfter.body).toHaveLength(1); - expect(responseAfter.body[0].url).toEqual(upload0.fileUrl); - expect(responseAfter.body[0].url).not.toEqual(upload1.fileUrl); + expect(responseAfter.body[0].id).toEqual(upload0.id); + expect(responseAfter.body[0].id).not.toEqual(upload1.id); for (const upload of [upload0, upload1]) { - const fileName = upload.fileUrl.replace('/uploads/', ''); // delete the file afterwards - await fs.unlink(join(uploadPath, fileName)); + await fs.unlink(join(uploadPath, upload.id)); } await fs.rm(uploadPath, { recursive: true }); }); diff --git a/backend/test/public-api/me.e2e-spec.ts b/backend/test/public-api/me.e2e-spec.ts index 1fb34ebd8..565f8e0ac 100644 --- a/backend/test/public-api/me.e2e-spec.ts +++ b/backend/test/public-api/me.e2e-spec.ts @@ -199,18 +199,18 @@ describe('Me', () => { expect(response1.body).toHaveLength(0); const testImage = await fs.readFile('test/public-api/fixtures/test.png'); - const imageUrls = []; - imageUrls.push( - (await testSetup.mediaService.saveFile(testImage, user, note1)).fileUrl, + const imageIds = []; + imageIds.push( + (await testSetup.mediaService.saveFile(testImage, user, note1)).id, ); - imageUrls.push( - (await testSetup.mediaService.saveFile(testImage, user, note1)).fileUrl, + imageIds.push( + (await testSetup.mediaService.saveFile(testImage, user, note1)).id, ); - imageUrls.push( - (await testSetup.mediaService.saveFile(testImage, user, note2)).fileUrl, + imageIds.push( + (await testSetup.mediaService.saveFile(testImage, user, note2)).id, ); - imageUrls.push( - (await testSetup.mediaService.saveFile(testImage, user, note2)).fileUrl, + imageIds.push( + (await testSetup.mediaService.saveFile(testImage, user, note2)).id, ); const response = await request(httpServer) @@ -218,14 +218,13 @@ describe('Me', () => { .expect('Content-Type', /json/) .expect(200); expect(response.body).toHaveLength(4); - expect(imageUrls).toContain(response.body[0].url); - expect(imageUrls).toContain(response.body[1].url); - expect(imageUrls).toContain(response.body[2].url); - expect(imageUrls).toContain(response.body[3].url); - for (const fileUrl of imageUrls) { - const fileName = fileUrl.replace('/uploads/', ''); + expect(imageIds).toContain(response.body[0].id); + expect(imageIds).toContain(response.body[1].id); + expect(imageIds).toContain(response.body[2].id); + expect(imageIds).toContain(response.body[3].id); + for (const imageId of imageIds) { // delete the file afterwards - await fs.unlink(join(uploadPath, fileName)); + await fs.unlink(join(uploadPath, imageId)); } await fs.rm(uploadPath, { recursive: true }); }); diff --git a/backend/test/public-api/media.e2e-spec.ts b/backend/test/public-api/media.e2e-spec.ts index a4780ae6b..0634ed9ba 100644 --- a/backend/test/public-api/media.e2e-spec.ts +++ b/backend/test/public-api/media.e2e-spec.ts @@ -41,21 +41,23 @@ describe('Media', () => { describe('POST /media', () => { it('works', async () => { - const uploadResponse = await request(testSetup.app.getHttpServer()) + const agent = request.agent(testSetup.app.getHttpServer()); + const uploadResponse = await agent .post('/api/v2/media') .set('Authorization', `Bearer ${testSetup.authTokens[0].secret}`) .attach('file', 'test/public-api/fixtures/test.png') .set('HedgeDoc-Note', 'testAlias1') .expect('Content-Type', /json/) .expect(201); - const path: string = uploadResponse.body.url; + const fileName = uploadResponse.body.id; + const path: string = '/api/v2/media/' + fileName; const testImage = await fs.readFile('test/public-api/fixtures/test.png'); - const downloadResponse = await request(testSetup.app.getHttpServer()).get( - path, - ); + const apiResponse = await agent + .get(path) + .set('Authorization', `Bearer ${testSetup.authTokens[0].secret}`); + expect(apiResponse.statusCode).toEqual(302); + const downloadResponse = await agent.get(apiResponse.header.location); expect(downloadResponse.body).toEqual(testImage); - // Remove /uploads/ from path as we just need the filename. - const fileName = path.replace('/uploads/', ''); // delete the file afterwards await fs.unlink(join(uploadPath, fileName)); }); diff --git a/backend/test/public-api/notes.e2e-spec.ts b/backend/test/public-api/notes.e2e-spec.ts index bd7a9ef9a..d7aafb489 100644 --- a/backend/test/public-api/notes.e2e-spec.ts +++ b/backend/test/public-api/notes.e2e-spec.ts @@ -495,12 +495,11 @@ describe('Notes', () => { .expect('Content-Type', /json/) .expect(200); expect(responseAfter.body).toHaveLength(1); - expect(responseAfter.body[0].url).toEqual(upload0.fileUrl); - expect(responseAfter.body[0].url).not.toEqual(upload1.fileUrl); + expect(responseAfter.body[0].id).toEqual(upload0.id); + expect(responseAfter.body[0].id).not.toEqual(upload1.id); for (const upload of [upload0, upload1]) { - const fileName = upload.fileUrl.replace('/uploads/', ''); // delete the file afterwards - await fs.unlink(join(uploadPath, fileName)); + await fs.unlink(join(uploadPath, upload.id)); } await fs.rm(uploadPath, { recursive: true }); }); diff --git a/frontend/cypress/e2e/fileUpload.spec.ts b/frontend/cypress/e2e/fileUpload.spec.ts index 978d6f31e..abe92a952 100644 --- a/frontend/cypress/e2e/fileUpload.spec.ts +++ b/frontend/cypress/e2e/fileUpload.spec.ts @@ -1,10 +1,10 @@ /* - * SPDX-FileCopyrightText: 2022 The HedgeDoc developers (see AUTHORS file) + * SPDX-FileCopyrightText: 2024 The HedgeDoc developers (see AUTHORS file) * * SPDX-License-Identifier: AGPL-3.0-only */ -const imageUrl = 'http://example.com/non-existing.png' +const imageId = 'non-existing.png' describe('File upload', () => { beforeEach(() => { @@ -22,7 +22,7 @@ describe('File upload', () => { { statusCode: 201, body: { - url: imageUrl + id: imageId } } ) @@ -38,7 +38,7 @@ describe('File upload', () => { }, { force: true } ) - cy.get('.cm-line').contains(`![demo.png](${imageUrl})`) + cy.get('.cm-line').contains(`![demo.png](http://127.0.0.1:3001/api/private/media/${imageId})`) }) it('via paste', () => { @@ -51,7 +51,7 @@ describe('File upload', () => { } } cy.get('.cm-content').trigger('paste', pasteEvent) - cy.get('.cm-line').contains(`![](${imageUrl})`) + cy.get('.cm-line').contains(`![](http://127.0.0.1:3001/api/private/media/${imageId})`) }) }) @@ -65,7 +65,7 @@ describe('File upload', () => { }, { action: 'drag-drop', force: true } ) - cy.get('.cm-line').contains(`![demo.png](${imageUrl})`) + cy.get('.cm-line').contains(`![demo.png](http://127.0.0.1:3001/api/private/media/${imageId})`) }) }) diff --git a/frontend/src/api/media/types.ts b/frontend/src/api/media/types.ts index ff59fd275..6b888d761 100644 --- a/frontend/src/api/media/types.ts +++ b/frontend/src/api/media/types.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: AGPL-3.0-only */ export interface MediaUpload { - url: string + id: string noteId: string | null createdAt: string username: string diff --git a/frontend/src/components/editor-page/editor-pane/hooks/use-handle-upload.tsx b/frontend/src/components/editor-page/editor-pane/hooks/use-handle-upload.tsx index 21f7f2959..f6917822a 100644 --- a/frontend/src/components/editor-page/editor-pane/hooks/use-handle-upload.tsx +++ b/frontend/src/components/editor-page/editor-pane/hooks/use-handle-upload.tsx @@ -15,6 +15,7 @@ import type { CursorSelection } from '../tool-bar/formatters/types/cursor-select import type { EditorView } from '@codemirror/view' import { useCallback } from 'react' import { useTranslation } from 'react-i18next' +import { useBaseUrl } from '../../../../hooks/common/use-base-url' /** * @param view the codemirror instance that is used to insert the Markdown code @@ -37,6 +38,7 @@ type handleUploadSignature = ( export const useHandleUpload = (): handleUploadSignature => { const { t } = useTranslation() const { showErrorNotification } = useUiNotifications() + const baseUrl = useBaseUrl() return useCallback( (view, file, cursorSelection, description, additionalUrlText) => { @@ -58,8 +60,9 @@ export const useHandleUpload = (): handleUploadSignature => { return replaceSelection(cursorSelection ?? currentSelection, uploadPlaceholder, false) }) uploadFile(noteId, file) - .then(({ url }) => { - const replacement = `![${description ?? file.name ?? ''}](${url}${additionalUrlText ?? ''})` + .then(({ id }) => { + const fullUrl = `${baseUrl}api/private/media/${id}` + const replacement = `![${description ?? file.name ?? ''}](${fullUrl}${additionalUrlText ?? ''})` changeContent(({ markdownContent }) => [ replaceInContent(markdownContent, uploadPlaceholder, replacement), undefined @@ -74,6 +77,6 @@ export const useHandleUpload = (): handleUploadSignature => { ]) }) }, - [showErrorNotification, t] + [showErrorNotification, t, baseUrl] ) } diff --git a/frontend/src/pages/api/private/me/media.ts b/frontend/src/pages/api/private/me/media.ts index 7f2909f13..8e28689ee 100644 --- a/frontend/src/pages/api/private/me/media.ts +++ b/frontend/src/pages/api/private/me/media.ts @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2022 The HedgeDoc developers (see AUTHORS file) + * SPDX-FileCopyrightText: 2024 The HedgeDoc developers (see AUTHORS file) * * SPDX-License-Identifier: AGPL-3.0-only */ @@ -12,13 +12,13 @@ const handler = (req: NextApiRequest, res: NextApiResponse) => { { username: 'tilman', createdAt: '2022-03-20T20:36:32Z', - url: 'https://dummyimage.com/256/f00', + id: 'dummy.png', noteId: 'features' }, { username: 'tilman', createdAt: '2022-03-20T20:36:57+0000', - url: 'https://dummyimage.com/256/00f', + id: 'dummy.png', noteId: null } ]) diff --git a/frontend/src/pages/api/private/media.ts b/frontend/src/pages/api/private/media.ts index 0b1b6f543..e0aaf2956 100644 --- a/frontend/src/pages/api/private/media.ts +++ b/frontend/src/pages/api/private/media.ts @@ -20,7 +20,7 @@ const handler = async (req: NextApiRequest, res: NextApiResponse): Promise req, res, { - url: '/public/img/avatar.png', + id: '/public/img/avatar.png', noteId: null, username: 'test', createdAt: '2022-02-27T21:54:23.856Z'