From 485f7cd338f571e51746a7ee8785ac95bfd5398b Mon Sep 17 00:00:00 2001 From: Yannick Bungers Date: Sat, 25 Mar 2023 17:47:46 +0100 Subject: [PATCH] feat: Add guest file uploads and add deletion for note owners Signed-off-by: Yannick Bungers Signed-off-by: Tilman Vatteroth --- .../src/api/private/media/media.controller.ts | 50 ++++++---- .../src/api/public/media/media.controller.ts | 52 ++++++++--- backend/src/api/utils/permissions.guard.ts | 10 +- backend/src/media/media-upload.dto.ts | 2 +- backend/src/media/media-upload.entity.ts | 6 +- backend/src/media/media.service.ts | 20 ++-- backend/test/private-api/media.e2e-spec.ts | 93 ++++++++++++++----- backend/test/public-api/media.e2e-spec.ts | 79 ++++++++++++++++ 8 files changed, 244 insertions(+), 68 deletions(-) diff --git a/backend/src/api/private/media/media.controller.ts b/backend/src/api/private/media/media.controller.ts index 313525187..584946a9a 100644 --- a/backend/src/api/private/media/media.controller.ts +++ b/backend/src/api/private/media/media.controller.ts @@ -23,10 +23,12 @@ import { MediaUploadDto } from '../../../media/media-upload.dto'; import { MediaService } from '../../../media/media.service'; import { MulterFile } from '../../../media/multer-file.interface'; import { Note } from '../../../notes/note.entity'; -import { NotesService } from '../../../notes/notes.service'; +import { Permission } from '../../../permissions/permissions.enum'; import { User } from '../../../users/user.entity'; import { NoteHeaderInterceptor } from '../../utils/note-header.interceptor'; import { OpenApi } from '../../utils/openapi.decorator'; +import { Permissions } from '../../utils/permissions.decorator'; +import { PermissionsGuard } from '../../utils/permissions.guard'; import { RequestNote } from '../../utils/request-note.decorator'; import { RequestUser } from '../../utils/request-user.decorator'; @@ -38,7 +40,6 @@ export class MediaController { constructor( private readonly logger: ConsoleLoggerService, private mediaService: MediaService, - private noteService: NotesService, ) { this.logger.setContext(MediaController.name); } @@ -60,8 +61,10 @@ export class MediaController { name: 'HedgeDoc-Note', description: 'ID or alias of the parent note', }) + @UseGuards(PermissionsGuard) @UseInterceptors(FileInterceptor('file')) @UseInterceptors(NoteHeaderInterceptor) + @Permissions(Permission.WRITE) @OpenApi( { code: 201, @@ -76,15 +79,22 @@ export class MediaController { async uploadMedia( @UploadedFile() file: MulterFile | undefined, @RequestNote() note: Note, - @RequestUser() user: User, + @RequestUser({ guestsAllowed: true }) user: User | null, ): Promise { if (file === undefined) { throw new BadRequestException('Request does not contain a file'); } - this.logger.debug( - `Received filename '${file.originalname}' for note '${note.id}' from user '${user.username}'`, - 'uploadMedia', - ); + if (user) { + this.logger.debug( + `Received filename '${file.originalname}' for note '${note.id}' from user '${user.username}'`, + 'uploadMedia', + ); + } else { + this.logger.debug( + `Received filename '${file.originalname}' for note '${note.id}' from not logged in user`, + 'uploadMedia', + ); + } const upload = await this.mediaService.saveFile(file.buffer, user, note); return await this.mediaService.toMediaUploadDto(upload); } @@ -95,21 +105,29 @@ export class MediaController { @RequestUser() user: User, @Param('filename') filename: string, ): Promise { - const username = user.username; - this.logger.debug( - `Deleting '${filename}' for user '${username}'`, - 'deleteMedia', - ); const mediaUpload = await this.mediaService.findUploadByFilename(filename); - if ((await mediaUpload.user).username !== username) { + const mediaUploadOwner = await mediaUpload.user; + const mediaUploadNote = await mediaUpload.note; + const mediaUploadNoteOwner = await mediaUploadNote?.owner; + if ( + mediaUploadOwner?.id === user.id || + user.id === mediaUploadNoteOwner?.id + ) { + this.logger.debug( + `Deleting '${filename}' for user '${user.username}'`, + 'deleteMedia', + ); + await this.mediaService.deleteFile(mediaUpload); + } else { this.logger.warn( - `${username} tried to delete '${filename}', but is not the owner`, + `${user.username} tried to delete '${filename}', but is not the owner of upload or connected note`, 'deleteMedia', ); throw new PermissionError( - `File '${filename}' is not owned by '${username}'`, + `Neither file '${filename}' nor note '${ + mediaUploadNote?.id ?? 'unknown' + }'is not owned by '${user.username}'`, ); } - await this.mediaService.deleteFile(mediaUpload); } } diff --git a/backend/src/api/public/media/media.controller.ts b/backend/src/api/public/media/media.controller.ts index 0fbd1a151..9329f6aa6 100644 --- a/backend/src/api/public/media/media.controller.ts +++ b/backend/src/api/public/media/media.controller.ts @@ -4,6 +4,7 @@ * SPDX-License-Identifier: AGPL-3.0-only */ import { + BadRequestException, Controller, Delete, Param, @@ -28,10 +29,12 @@ import { MediaUploadDto } from '../../../media/media-upload.dto'; import { MediaService } from '../../../media/media.service'; import { MulterFile } from '../../../media/multer-file.interface'; import { Note } from '../../../notes/note.entity'; -import { NotesService } from '../../../notes/notes.service'; +import { Permission } from '../../../permissions/permissions.enum'; import { User } from '../../../users/user.entity'; import { NoteHeaderInterceptor } from '../../utils/note-header.interceptor'; import { OpenApi } from '../../utils/openapi.decorator'; +import { Permissions } from '../../utils/permissions.decorator'; +import { PermissionsGuard } from '../../utils/permissions.guard'; import { RequestNote } from '../../utils/request-note.decorator'; import { RequestUser } from '../../utils/request-user.decorator'; @@ -44,7 +47,6 @@ export class MediaController { constructor( private readonly logger: ConsoleLoggerService, private mediaService: MediaService, - private noteService: NotesService, ) { this.logger.setContext(MediaController.name); } @@ -77,17 +79,29 @@ export class MediaController { 404, 500, ) + @UseGuards(PermissionsGuard) @UseInterceptors(FileInterceptor('file')) @UseInterceptors(NoteHeaderInterceptor) + @Permissions(Permission.WRITE) async uploadMedia( @RequestUser() user: User, @UploadedFile() file: MulterFile, @RequestNote() note: Note, ): Promise { - this.logger.debug( - `Recieved filename '${file.originalname}' for note '${note.id}' from user '${user.username}'`, - 'uploadMedia', - ); + if (file === undefined) { + throw new BadRequestException('Request does not contain a file'); + } + if (user) { + this.logger.debug( + `Received filename '${file.originalname}' for note '${note.publicId}' from user '${user.username}'`, + 'uploadMedia', + ); + } else { + this.logger.debug( + `Received filename '${file.originalname}' for note '${note.publicId}' from not logged in user`, + 'uploadMedia', + ); + } const upload = await this.mediaService.saveFile(file.buffer, user, note); return await this.mediaService.toMediaUploadDto(upload); } @@ -98,21 +112,29 @@ export class MediaController { @RequestUser() user: User, @Param('filename') filename: string, ): Promise { - const username = user.username; - this.logger.debug( - `Deleting '${filename}' for user '${username}'`, - 'deleteMedia', - ); const mediaUpload = await this.mediaService.findUploadByFilename(filename); - if ((await mediaUpload.user).username !== username) { + const mediaUploadOwner = await mediaUpload.user; + const mediaUploadNote = await mediaUpload.note; + const mediaUploadNoteOwner = await mediaUploadNote?.owner; + if ( + mediaUploadOwner?.id === user.id || + user.id === mediaUploadNoteOwner?.id + ) { + this.logger.debug( + `Deleting '${filename}' for user '${user.username}'`, + 'deleteMedia', + ); + await this.mediaService.deleteFile(mediaUpload); + } else { this.logger.warn( - `${username} tried to delete '${filename}', but is not the owner`, + `${user.username} tried to delete '${filename}', but is not the owner of upload or connected note`, 'deleteMedia', ); throw new PermissionError( - `File '${filename}' is not owned by '${username}'`, + `Neither file '${filename}' nor note '${ + mediaUploadNote?.id ?? 'unknown' + }'is not owned by '${user.username}'`, ); } - await this.mediaService.deleteFile(mediaUpload); } } diff --git a/backend/src/api/utils/permissions.guard.ts b/backend/src/api/utils/permissions.guard.ts index 95125e317..620d671fa 100644 --- a/backend/src/api/utils/permissions.guard.ts +++ b/backend/src/api/utils/permissions.guard.ts @@ -16,6 +16,8 @@ import { CompleteRequest } from './request.type'; /** * This guards controller methods from access, if the user has not the appropriate permissions. * The permissions are set via the {@link Permissions} decorator in addition to this guard. + * If the check permission is not CREATE the method needs to extract the noteIdOrAlias from + * request.params['noteIdOrAlias'] or request.headers['hedgedoc-note'] to check if the user has the permission. */ @Injectable() export class PermissionsGuard implements CanActivate { @@ -46,9 +48,11 @@ export class PermissionsGuard implements CanActivate { if (permissions[0] === Permission.CREATE) { return this.permissionsService.mayCreate(user); } - // Get the note from the parameter noteIdOrAlias - // Attention: This gets the note an additional time if used in conjunction with GetNoteInterceptor - const noteIdOrAlias = request.params['noteIdOrAlias']; + // Get the note from the parameter noteIdOrAlias or the http header hedgedoc-note + // Attention: This gets the note an additional time if used in conjunction with GetNoteInterceptor or NoteHeaderInterceptor + let noteIdOrAlias = request.params['noteIdOrAlias']; + if (noteIdOrAlias === undefined) + noteIdOrAlias = request.headers['hedgedoc-note'] as string; const note = await getNote(this.noteService, noteIdOrAlias); switch (permissions[0]) { case Permission.READ: diff --git a/backend/src/media/media-upload.dto.ts b/backend/src/media/media-upload.dto.ts index 690617009..b3d3fe2c0 100644 --- a/backend/src/media/media-upload.dto.ts +++ b/backend/src/media/media-upload.dto.ts @@ -42,5 +42,5 @@ export class MediaUploadDto extends BaseDto { */ @IsString() @ApiProperty() - username: string; + username: string | null; } diff --git a/backend/src/media/media-upload.entity.ts b/backend/src/media/media-upload.entity.ts index 2e2051ca6..eec5d85b4 100644 --- a/backend/src/media/media-upload.entity.ts +++ b/backend/src/media/media-upload.entity.ts @@ -28,9 +28,9 @@ export class MediaUpload { note: Promise; @ManyToOne((_) => User, (user) => user.mediaUploads, { - nullable: false, + nullable: true, }) - user: Promise; + user: Promise; @Column({ nullable: false, @@ -65,7 +65,7 @@ export class MediaUpload { public static create( id: string, note: Note, - user: User, + user: User | null, extension: string, backendType: BackendType, fileUrl: string, diff --git a/backend/src/media/media.service.ts b/backend/src/media/media.service.ts index bffa9642d..fa805d590 100644 --- a/backend/src/media/media.service.ts +++ b/backend/src/media/media.service.ts @@ -78,13 +78,20 @@ export class MediaService { */ async saveFile( fileBuffer: Buffer, - user: User, + user: User | null, note: Note, ): Promise { - this.logger.debug( - `Saving file for note '${note.id}' and user '${user.username}'`, - 'saveFile', - ); + if (user) { + this.logger.debug( + `Saving file for note '${note.id}' and user '${user.username}'`, + 'saveFile', + ); + } else { + this.logger.debug( + `Saving file for note '${note.id}' and not logged in user`, + 'saveFile', + ); + } const fileTypeResult = await FileType.fromBuffer(fileBuffer); if (!fileTypeResult) { throw new ClientError('Could not detect file type.'); @@ -223,11 +230,12 @@ export class MediaService { } async toMediaUploadDto(mediaUpload: MediaUpload): Promise { + const user = await mediaUpload.user; return { url: mediaUpload.fileUrl, notePublicId: (await mediaUpload.note)?.publicId ?? null, createdAt: mediaUpload.createdAt, - username: (await mediaUpload.user).username, + username: user?.username ?? null, }; } } diff --git a/backend/test/private-api/media.e2e-spec.ts b/backend/test/private-api/media.e2e-spec.ts index e7ef800a6..183cef9e1 100644 --- a/backend/test/private-api/media.e2e-spec.ts +++ b/backend/test/private-api/media.e2e-spec.ts @@ -123,32 +123,77 @@ describe('Media', () => { }); }); - it('DELETE /media/{filename}', async () => { - // upload a file with the default test user - const testNote = await testSetup.notesService.createNote( - 'test content', - null, - 'test_delete_media', - ); - const testImage = await fs.readFile('test/private-api/fixtures/test.png'); - const upload = await testSetup.mediaService.saveFile( - testImage, - user, - testNote, - ); - const filename = upload.fileUrl.split('/').pop() || ''; + describe('DELETE /media/{filename}', () => { + it('deleting user is owner of file', async () => { + // upload a file with the default test user + const testNote = await testSetup.notesService.createNote( + 'test content', + null, + 'test_delete_media_file', + ); + const testImage = await fs.readFile('test/private-api/fixtures/test.png'); + const upload = await testSetup.mediaService.saveFile( + testImage, + user, + testNote, + ); + const filename = upload.fileUrl.split('/').pop() || ''; - // login with a different user; - const agent2 = request.agent(testSetup.app.getHttpServer()); - await agent2 - .post('/api/private/auth/local/login') - .send({ username: username2, password: password2 }) - .expect(201); + // login with a different user; + const agent2 = request.agent(testSetup.app.getHttpServer()); + await agent2 + .post('/api/private/auth/local/login') + .send({ username: username2, password: password2 }) + .expect(201); - // try to delete upload with second user - await agent2.delete('/api/private/media/' + filename).expect(403); + // try to delete upload with second user + await agent2.delete('/api/private/media/' + filename).expect(403); - // delete upload for real - await agent.delete('/api/private/media/' + filename).expect(204); + await agent.get('/uploads/' + filename).expect(200); + + // delete upload for real + await agent.delete('/api/private/media/' + filename).expect(204); + + // Test if file is really deleted + await agent.get('/uploads/' + filename).expect(404); + }); + it('deleting user is owner of note', async () => { + // upload a file with the default test user + const testNote = await testSetup.notesService.createNote( + 'test content', + await testSetup.userService.getUserByUsername(username2), + 'test_delete_media_note', + ); + const testImage = await fs.readFile('test/private-api/fixtures/test.png'); + const upload = await testSetup.mediaService.saveFile( + testImage, + null, + testNote, + ); + const filename = upload.fileUrl.split('/').pop() || ''; + + // login with a different user; + const agent2 = request.agent(testSetup.app.getHttpServer()); + await agent2 + .post('/api/private/auth/local/login') + .send({ username: username2, password: password2 }) + .expect(201); + + const agentGuest = request.agent(testSetup.app.getHttpServer()); + + // try to delete upload with second user + await agent.delete('/api/private/media/' + filename).expect(403); + + await agent.get('/uploads/' + filename).expect(200); + + await agentGuest.delete('/api/private/media/' + filename).expect(401); + + await agent.get('/uploads/' + filename).expect(200); + // delete upload for real + await agent2.delete('/api/private/media/' + filename).expect(204); + + // Test if file is really deleted + await agent.get('/uploads/' + filename).expect(404); + }); }); }); diff --git a/backend/test/public-api/media.e2e-spec.ts b/backend/test/public-api/media.e2e-spec.ts index 5cf8f8886..514c3d85c 100644 --- a/backend/test/public-api/media.e2e-spec.ts +++ b/backend/test/public-api/media.e2e-spec.ts @@ -126,5 +126,84 @@ describe('Media', () => { .set('Authorization', `Bearer ${testSetup.authTokens[1].secret}`) .expect(403); }); + it('deleting user is owner of file', async () => { + // upload a file with the default test user + const testNote = await testSetup.notesService.createNote( + 'test content', + null, + 'test_delete_media_file', + ); + const testImage = await fs.readFile('test/public-api/fixtures/test.png'); + const upload = await testSetup.mediaService.saveFile( + testImage, + testSetup.users[0], + testNote, + ); + const filename = upload.fileUrl.split('/').pop() || ''; + + const agent2 = request.agent(testSetup.app.getHttpServer()); + + // try to delete upload with second user + await agent2 + .delete('/api/v2/media/' + filename) + .set('Authorization', `Bearer ${testSetup.authTokens[1].secret}`) + .expect(403); + + await agent2 + .get('/uploads/' + filename) + .set('Authorization', `Bearer ${testSetup.authTokens[1].secret}`) + .expect(200); + + // delete upload for real + await agent2 + .delete('/api/v2/media/' + filename) + .set('Authorization', `Bearer ${testSetup.authTokens[0].secret}`) + .expect(204); + + // Test if file is really deleted + await agent2 + .get('/uploads/' + filename) + .set('Authorization', `Bearer ${testSetup.authTokens[1].secret}`) + .expect(404); + }); + it('deleting user is owner of note', async () => { + // upload a file with the default test user + const testNote = await testSetup.notesService.createNote( + 'test content', + testSetup.users[2], + 'test_delete_media_note', + ); + const testImage = await fs.readFile('test/public-api/fixtures/test.png'); + const upload = await testSetup.mediaService.saveFile( + testImage, + testSetup.users[0], + testNote, + ); + const filename = upload.fileUrl.split('/').pop() || ''; + + const agent2 = request.agent(testSetup.app.getHttpServer()); + // try to delete upload with second user + await agent2 + .delete('/api/v2/media/' + filename) + .set('Authorization', `Bearer ${testSetup.authTokens[1].secret}`) + .expect(403); + + await agent2 + .get('/uploads/' + filename) + .set('Authorization', `Bearer ${testSetup.authTokens[1].secret}`) + .expect(200); + + // delete upload for real + await agent2 + .delete('/api/v2/media/' + filename) + .set('Authorization', `Bearer ${testSetup.authTokens[2].secret}`) + .expect(204); + + // Test if file is really deleted + await agent2 + .get('/uploads/' + filename) + .set('Authorization', `Bearer ${testSetup.authTokens[1].secret}`) + .expect(404); + }); }); });