From 001a49329cf00a3e1bce38a540b277b0cb677b03 Mon Sep 17 00:00:00 2001
From: Yannick Bungers <git@innay.de>
Date: Sun, 26 Mar 2023 15:51:08 +0200
Subject: [PATCH] refactor: extract permission checking from controllers and
guard
Signed-off-by: Yannick Bungers <git@innay.de>
Signed-off-by: Tilman Vatteroth <git@tilmanvatteroth.de>
---
.../src/api/private/media/media.controller.ts | 14 ++++---
.../src/api/public/media/media.controller.ts | 14 ++++---
backend/src/api/utils/permissions.guard.ts | 14 +++----
.../src/permissions/permissions.service.ts | 39 +++++++++++++++++++
4 files changed, 60 insertions(+), 21 deletions(-)
diff --git a/backend/src/api/private/media/media.controller.ts b/backend/src/api/private/media/media.controller.ts
index 584946a9a..8eb1b04ac 100644
--- a/backend/src/api/private/media/media.controller.ts
+++ b/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 ||
- user.id === mediaUploadNoteOwner?.id
+ await this.permissionsService.checkMediaDeletePermission(
+ 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}'`,
);
}
}
diff --git a/backend/src/api/public/media/media.controller.ts b/backend/src/api/public/media/media.controller.ts
index 9329f6aa6..91cca2862 100644
--- a/backend/src/api/public/media/media.controller.ts
+++ b/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 ||
- user.id === mediaUploadNoteOwner?.id
+ await this.permissionsService.checkMediaDeletePermission(
+ 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}'`,
);
}
}
diff --git a/backend/src/api/utils/permissions.guard.ts b/backend/src/api/utils/permissions.guard.ts
index 620d671fa..5fd913d1f 100644
--- a/backend/src/api/utils/permissions.guard.ts
+++ b/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]) {
- case Permission.READ:
- return await this.permissionsService.mayRead(user, note);
- case Permission.WRITE:
- return await this.permissionsService.mayWrite(user, note);
- case Permission.OWNER:
- return await this.permissionsService.isOwner(user, note);
- }
- return false;
+ return await this.permissionsService.checkPermissionOnNote(
+ permissions[0],
+ user,
+ note,
+ );
}
}
diff --git a/backend/src/permissions/permissions.service.ts b/backend/src/permissions/permissions.service.ts
index 6c4b3e34d..b5fd5d9ed 100644
--- a/backend/src/permissions/permissions.service.ts
+++ b/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}.