refactor: extract permission checking from controllers and guard

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

backend/src/api/private/media/media.controller.ts

@@ -24,6 +24,7 @@ import { MediaService } from '../../../media/media.service';
 import { MulterFile } from '../../../media/multer-file.interface';
 import { Note } from '../../../notes/note.entity';
 import { Permission } from '../../../permissions/permissions.enum';
+import { PermissionsService } from '../../../permissions/permissions.service';
 import { User } from '../../../users/user.entity';
 import { NoteHeaderInterceptor } from '../../utils/note-header.interceptor';
 import { OpenApi } from '../../utils/openapi.decorator';
@@ -40,6 +41,7 @@ export class MediaController {
   constructor(
     private readonly logger: ConsoleLoggerService,
     private mediaService: MediaService,
+    private permissionsService: PermissionsService,
   ) {
     this.logger.setContext(MediaController.name);
   }
@@ -106,12 +108,11 @@ export class MediaController {
     @Param('filename') filename: string,
   ): Promise<void> {
     const mediaUpload = await this.mediaService.findUploadByFilename(filename);
-    const mediaUploadOwner = await mediaUpload.user;
-    const mediaUploadNote = await mediaUpload.note;
-    const mediaUploadNoteOwner = await mediaUploadNote?.owner;
     if (
-      mediaUploadOwner?.id === user.id ||
+      await this.permissionsService.checkMediaDeletePermission(
-      user.id === mediaUploadNoteOwner?.id
+        user,
+        mediaUpload,
+      )
     ) {
       this.logger.debug(
         `Deleting '${filename}' for user '${user.username}'`,
@@ -123,10 +124,11 @@ export class MediaController {
         `${user.username} tried to delete '${filename}', but is not the owner of upload or connected note`,
         'deleteMedia',
       );
+      const mediaUploadNote = await mediaUpload.note;
       throw new PermissionError(
         `Neither file '${filename}' nor note '${
           mediaUploadNote?.id ?? 'unknown'
-        }'is not owned by '${user.username}'`,
+        }'is owned by '${user.username}'`,
       );
     }
   }

backend/src/api/public/media/media.controller.ts

@@ -30,6 +30,7 @@ import { MediaService } from '../../../media/media.service';
 import { MulterFile } from '../../../media/multer-file.interface';
 import { Note } from '../../../notes/note.entity';
 import { Permission } from '../../../permissions/permissions.enum';
+import { PermissionsService } from '../../../permissions/permissions.service';
 import { User } from '../../../users/user.entity';
 import { NoteHeaderInterceptor } from '../../utils/note-header.interceptor';
 import { OpenApi } from '../../utils/openapi.decorator';
@@ -47,6 +48,7 @@ export class MediaController {
   constructor(
     private readonly logger: ConsoleLoggerService,
     private mediaService: MediaService,
+    private permissionsService: PermissionsService,
   ) {
     this.logger.setContext(MediaController.name);
   }
@@ -113,12 +115,11 @@ export class MediaController {
     @Param('filename') filename: string,
   ): Promise<void> {
     const mediaUpload = await this.mediaService.findUploadByFilename(filename);
-    const mediaUploadOwner = await mediaUpload.user;
-    const mediaUploadNote = await mediaUpload.note;
-    const mediaUploadNoteOwner = await mediaUploadNote?.owner;
     if (
-      mediaUploadOwner?.id === user.id ||
+      await this.permissionsService.checkMediaDeletePermission(
-      user.id === mediaUploadNoteOwner?.id
+        user,
+        mediaUpload,
+      )
     ) {
       this.logger.debug(
         `Deleting '${filename}' for user '${user.username}'`,
@@ -130,10 +131,11 @@ export class MediaController {
         `${user.username} tried to delete '${filename}', but is not the owner of upload or connected note`,
         'deleteMedia',
       );
+      const mediaUploadNote = await mediaUpload.note;
       throw new PermissionError(
         `Neither file '${filename}' nor note '${
           mediaUploadNote?.id ?? 'unknown'
-        }'is not owned by '${user.username}'`,
+        }'is owned by '${user.username}'`,
       );
     }
   }

backend/src/api/utils/permissions.guard.ts

@@ -54,14 +54,10 @@ export class PermissionsGuard implements CanActivate {
     if (noteIdOrAlias === undefined)
       noteIdOrAlias = request.headers['hedgedoc-note'] as string;
     const note = await getNote(this.noteService, noteIdOrAlias);
-    switch (permissions[0]) {
+    return await this.permissionsService.checkPermissionOnNote(
-      case Permission.READ:
+      permissions[0],
-        return await this.permissionsService.mayRead(user, note);
+      user,
-      case Permission.WRITE:
+      note,
-        return await this.permissionsService.mayWrite(user, note);
+    );
-      case Permission.OWNER:
-        return await this.permissionsService.isOwner(user, note);
-    }
-    return false;
   }
 }

backend/src/permissions/permissions.service.ts

@@ -19,6 +19,7 @@ import { Group } from '../groups/group.entity';
 import { GroupsService } from '../groups/groups.service';
 import { SpecialGroup } from '../groups/groups.special';
 import { ConsoleLoggerService } from '../logger/console-logger.service';
+import { MediaUpload } from '../media/media-upload.entity';
 import { NotePermissionsUpdateDto } from '../notes/note-permissions.dto';
 import { Note } from '../notes/note.entity';
 import { User } from '../users/user.entity';
@@ -26,6 +27,7 @@ import { UsersService } from '../users/users.service';
 import { checkArrayForDuplicates } from '../utils/arrayDuplicatCheck';
 import { NoteGroupPermission } from './note-group-permission.entity';
 import { NoteUserPermission } from './note-user-permission.entity';
+import { Permission } from './permissions.enum';
 
 @Injectable()
 export class PermissionsService {
@@ -38,6 +40,43 @@ export class PermissionsService {
     private noteConfig: NoteConfig,
     private eventEmitter: EventEmitter2<NoteEventMap>,
   ) {}
+  /**
+   * Checks if the given {@link User} is has the in {@link desiredPermission} specified permission on {@link Note}.
+   *
+   * @async
+   * @param {Permission} desiredPermission - permission level to check for
+   * @param {User} user - The user whose permission should be checked. Value is null if guest access should be checked
+   * @param {Note} note - The note for which the permission should be checked
+   * @return if the user has the specified permission on the note
+   */
+  public async checkPermissionOnNote(
+    desiredPermission: Permission,
+    user: User | null,
+    note: Note,
+  ): Promise<boolean> {
+    switch (desiredPermission) {
+      case Permission.READ:
+        return await this.mayRead(user, note);
+      case Permission.WRITE:
+        return await this.mayWrite(user, note);
+      case Permission.OWNER:
+        return await this.isOwner(user, note);
+    }
+    return false;
+  }
+
+  public async checkMediaDeletePermission(
+    user: User,
+    mediaUpload: MediaUpload,
+  ): Promise<boolean> {
+    const mediaUploadNote = await mediaUpload.note;
+    const mediaUploadOwner = await mediaUpload.user;
+
+    const owner =
+      !!mediaUploadNote && (await this.isOwner(user, mediaUploadNote));
+
+    return mediaUploadOwner?.id === user.id || owner;
+  }
 
   /**
    * Checks if the given {@link User} is allowed to read the given {@link Note}.