feat: Add guest file uploads and add deletion for note owners

Signed-off-by: Yannick Bungers <git@innay.de>
Signed-off-by: Tilman Vatteroth <git@tilmanvatteroth.de>

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<MediaUploadDto> {
     if (file === undefined) {
       throw new BadRequestException('Request does not contain a file');
     }
-    this.logger.debug(
+    if (user) {
-      `Received filename '${file.originalname}' for note '${note.id}' from user '${user.username}'`,
+      this.logger.debug(
-      'uploadMedia',
+        `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<void> {
-    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);
   }
 }

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<MediaUploadDto> {
-    this.logger.debug(
+    if (file === undefined) {
-      `Recieved filename '${file.originalname}' for note '${note.id}' from user '${user.username}'`,
+      throw new BadRequestException('Request does not contain a file');
-      'uploadMedia',
+    }
-    );
+    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<void> {
-    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);
   }
 }

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
+    // 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
+    // Attention: This gets the note an additional time if used in conjunction with GetNoteInterceptor or NoteHeaderInterceptor
-    const noteIdOrAlias = request.params['noteIdOrAlias'];
+    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:

backend/src/media/media-upload.dto.ts

@@ -42,5 +42,5 @@ export class MediaUploadDto extends BaseDto {
    */
   @IsString()
   @ApiProperty()
-  username: string;
+  username: string | null;
 }

backend/src/media/media-upload.entity.ts

@@ -28,9 +28,9 @@ export class MediaUpload {
   note: Promise<Note | null>;
 
   @ManyToOne((_) => User, (user) => user.mediaUploads, {
-    nullable: false,
+    nullable: true,
   })
-  user: Promise<User>;
+  user: Promise<User | null>;
 
   @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,

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<MediaUpload> {
-    this.logger.debug(
+    if (user) {
-      `Saving file for note '${note.id}' and user '${user.username}'`,
+      this.logger.debug(
-      'saveFile',
+        `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<MediaUploadDto> {
+    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,
     };
   }
 }

backend/test/private-api/media.e2e-spec.ts

@@ -123,32 +123,77 @@ describe('Media', () => {
     });
   });
 
-  it('DELETE /media/{filename}', async () => {
+  describe('DELETE /media/{filename}', () => {
-    // upload a file with the default test user
+    it('deleting user is owner of file', async () => {
-    const testNote = await testSetup.notesService.createNote(
+      // upload a file with the default test user
-      'test content',
+      const testNote = await testSetup.notesService.createNote(
-      null,
+        'test content',
-      'test_delete_media',
+        null,
-    );
+        'test_delete_media_file',
-    const testImage = await fs.readFile('test/private-api/fixtures/test.png');
+      );
-    const upload = await testSetup.mediaService.saveFile(
+      const testImage = await fs.readFile('test/private-api/fixtures/test.png');
-      testImage,
+      const upload = await testSetup.mediaService.saveFile(
-      user,
+        testImage,
-      testNote,
+        user,
-    );
+        testNote,
-    const filename = upload.fileUrl.split('/').pop() || '';
+      );
+      const filename = upload.fileUrl.split('/').pop() || '';
 
-    // login with a different user;
+      // login with a different user;
-    const agent2 = request.agent(testSetup.app.getHttpServer());
+      const agent2 = request.agent(testSetup.app.getHttpServer());
-    await agent2
+      await agent2
-      .post('/api/private/auth/local/login')
+        .post('/api/private/auth/local/login')
-      .send({ username: username2, password: password2 })
+        .send({ username: username2, password: password2 })
-      .expect(201);
+        .expect(201);
 
-    // try to delete upload with second user
+      // try to delete upload with second user
-    await agent2.delete('/api/private/media/' + filename).expect(403);
+      await agent2.delete('/api/private/media/' + filename).expect(403);
 
-    // delete upload for real
+      await agent.get('/uploads/' + filename).expect(200);
-    await agent.delete('/api/private/media/' + filename).expect(204);
+
+      // 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);
+    });
   });
 });

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);
+    });
   });
 });