From 4b3c726101fe35ecb16231516970cce2c31baeb1 Mon Sep 17 00:00:00 2001 From: Philip Molares Date: Sun, 21 Nov 2021 17:05:27 +0100 Subject: [PATCH 1/8] chore: move get-note-pipe to api utils Signed-off-by: Philip Molares --- src/api/private/me/history/history.controller.ts | 2 +- src/api/private/notes/notes.controller.ts | 2 +- src/api/public/me/me.controller.ts | 2 +- src/api/public/notes/notes.controller.ts | 2 +- src/{notes => api/utils}/get-note.pipe.ts | 8 ++++---- 5 files changed, 8 insertions(+), 8 deletions(-) rename src/{notes => api/utils}/get-note.pipe.ts (78%) diff --git a/src/api/private/me/history/history.controller.ts b/src/api/private/me/history/history.controller.ts index 2bdedab36..3cb5f32a8 100644 --- a/src/api/private/me/history/history.controller.ts +++ b/src/api/private/me/history/history.controller.ts @@ -24,9 +24,9 @@ import { HistoryEntryDto } from '../../../../history/history-entry.dto'; import { HistoryService } from '../../../../history/history.service'; import { SessionGuard } from '../../../../identity/session.guard'; import { ConsoleLoggerService } from '../../../../logger/console-logger.service'; -import { GetNotePipe } from '../../../../notes/get-note.pipe'; import { Note } from '../../../../notes/note.entity'; import { User } from '../../../../users/user.entity'; +import { GetNotePipe } from '../../../utils/get-note.pipe'; import { RequestUser } from '../../../utils/request-user.decorator'; @UseGuards(SessionGuard) diff --git a/src/api/private/notes/notes.controller.ts b/src/api/private/notes/notes.controller.ts index 0b829ebb6..c7007c878 100644 --- a/src/api/private/notes/notes.controller.ts +++ b/src/api/private/notes/notes.controller.ts @@ -27,7 +27,6 @@ import { SessionGuard } from '../../../identity/session.guard'; import { ConsoleLoggerService } from '../../../logger/console-logger.service'; import { MediaUploadDto } from '../../../media/media-upload.dto'; import { MediaService } from '../../../media/media.service'; -import { GetNotePipe } from '../../../notes/get-note.pipe'; import { NoteDto } from '../../../notes/note.dto'; import { Note } from '../../../notes/note.entity'; import { NoteMediaDeletionDto } from '../../../notes/note.media-deletion.dto'; @@ -38,6 +37,7 @@ import { RevisionDto } from '../../../revisions/revision.dto'; import { RevisionsService } from '../../../revisions/revisions.service'; import { User } from '../../../users/user.entity'; import { UsersService } from '../../../users/users.service'; +import { GetNotePipe } from '../../utils/get-note.pipe'; import { MarkdownBody } from '../../utils/markdownbody-decorator'; import { RequestUser } from '../../utils/request-user.decorator'; diff --git a/src/api/public/me/me.controller.ts b/src/api/public/me/me.controller.ts index ae05cbaac..a64e412dd 100644 --- a/src/api/public/me/me.controller.ts +++ b/src/api/public/me/me.controller.ts @@ -31,7 +31,6 @@ import { HistoryService } from '../../../history/history.service'; import { ConsoleLoggerService } from '../../../logger/console-logger.service'; import { MediaUploadDto } from '../../../media/media-upload.dto'; import { MediaService } from '../../../media/media.service'; -import { GetNotePipe } from '../../../notes/get-note.pipe'; import { NoteMetadataDto } from '../../../notes/note-metadata.dto'; import { Note } from '../../../notes/note.entity'; import { NotesService } from '../../../notes/notes.service'; @@ -43,6 +42,7 @@ import { successfullyDeletedDescription, unauthorizedDescription, } from '../../utils/descriptions'; +import { GetNotePipe } from '../../utils/get-note.pipe'; import { RequestUser } from '../../utils/request-user.decorator'; @ApiTags('me') diff --git a/src/api/public/notes/notes.controller.ts b/src/api/public/notes/notes.controller.ts index 31172cc78..49583e92c 100644 --- a/src/api/public/notes/notes.controller.ts +++ b/src/api/public/notes/notes.controller.ts @@ -39,7 +39,6 @@ import { HistoryService } from '../../../history/history.service'; import { ConsoleLoggerService } from '../../../logger/console-logger.service'; import { MediaUploadDto } from '../../../media/media-upload.dto'; import { MediaService } from '../../../media/media.service'; -import { GetNotePipe } from '../../../notes/get-note.pipe'; import { NoteMetadataDto } from '../../../notes/note-metadata.dto'; import { NotePermissionsDto, @@ -60,6 +59,7 @@ import { unauthorizedDescription, } from '../../utils/descriptions'; import { FullApi } from '../../utils/fullapi-decorator'; +import { GetNotePipe } from '../../utils/get-note.pipe'; import { MarkdownBody } from '../../utils/markdownbody-decorator'; import { RequestUser } from '../../utils/request-user.decorator'; diff --git a/src/notes/get-note.pipe.ts b/src/api/utils/get-note.pipe.ts similarity index 78% rename from src/notes/get-note.pipe.ts rename to src/api/utils/get-note.pipe.ts index c8cdd4cd5..f20c770ca 100644 --- a/src/notes/get-note.pipe.ts +++ b/src/api/utils/get-note.pipe.ts @@ -11,10 +11,10 @@ import { PipeTransform, } from '@nestjs/common'; -import { ForbiddenIdError, NotInDBError } from '../errors/errors'; -import { ConsoleLoggerService } from '../logger/console-logger.service'; -import { Note } from './note.entity'; -import { NotesService } from './notes.service'; +import { ForbiddenIdError, NotInDBError } from '../../errors/errors'; +import { ConsoleLoggerService } from '../../logger/console-logger.service'; +import { Note } from '../../notes/note.entity'; +import { NotesService } from '../../notes/notes.service'; @Injectable() export class GetNotePipe implements PipeTransform> { From dbf467fea56ecb62a6c8d49e7b7bf8ad7ce7541a Mon Sep 17 00:00:00 2001 From: Philip Molares Date: Sun, 21 Nov 2021 17:12:35 +0100 Subject: [PATCH 2/8] chore: extract getNote code from GetNotePipe.transform This was done so the same code could be used in the PermissionsGuard Signed-off-by: Philip Molares --- src/api/utils/get-note.pipe.ts | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/src/api/utils/get-note.pipe.ts b/src/api/utils/get-note.pipe.ts index f20c770ca..8790e326a 100644 --- a/src/api/utils/get-note.pipe.ts +++ b/src/api/utils/get-note.pipe.ts @@ -26,18 +26,25 @@ export class GetNotePipe implements PipeTransform> { } async transform(noteIdOrAlias: string, _: ArgumentMetadata): Promise { - let note: Note; - try { - note = await this.noteService.getNoteByIdOrAlias(noteIdOrAlias); - } catch (e) { - if (e instanceof NotInDBError) { - throw new NotFoundException(e.message); - } - if (e instanceof ForbiddenIdError) { - throw new BadRequestException(e.message); - } - throw e; - } - return note; + return await getNote(this.noteService, noteIdOrAlias); } } + +export async function getNote( + noteService: NotesService, + noteIdOrAlias: string, +): Promise { + let note: Note; + try { + note = await noteService.getNoteByIdOrAlias(noteIdOrAlias); + } catch (e) { + if (e instanceof NotInDBError) { + throw new NotFoundException(e.message); + } + if (e instanceof ForbiddenIdError) { + throw new BadRequestException(e.message); + } + throw e; + } + return note; +} From c30a06d90b936eb4c49973de669dec458d444985 Mon Sep 17 00:00:00 2001 From: Philip Molares Date: Sun, 21 Nov 2021 17:16:12 +0100 Subject: [PATCH 3/8] feat: create permission enum This enum makes it possible which permissions a user needs to hold to access a specific resource Signed-off-by: Philip Molares --- src/permissions/permissions.enum.ts | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 src/permissions/permissions.enum.ts diff --git a/src/permissions/permissions.enum.ts b/src/permissions/permissions.enum.ts new file mode 100644 index 000000000..5d90367cc --- /dev/null +++ b/src/permissions/permissions.enum.ts @@ -0,0 +1,21 @@ +/* + * SPDX-FileCopyrightText: 2021 The HedgeDoc developers (see AUTHORS file) + * + * SPDX-License-Identifier: AGPL-3.0-only + */ + +/* + * SPDX-FileCopyrightText: 2021 The HedgeDoc developers (see AUTHORS file) + * + * SPDX-License-Identifier: AGPL-3.0-only + */ + +/** + * Represents the Permissions a user may hold in a request + */ +export enum Permission { + READ = 'read', + WRITE = 'write', + CREATE = 'create', + OWNER = 'owner', +} From 6f7cfced3917b283367b7b5abf62109595a5d7ab Mon Sep 17 00:00:00 2001 From: Philip Molares Date: Sun, 21 Nov 2021 17:17:33 +0100 Subject: [PATCH 4/8] feat: create permission decorator This gathers the permission a user needs to hold to access a resource for the PermissionsGuard. See https://docs.nestjs.com/guards#setting-roles-per-handler Signed-off-by: Philip Molares --- src/permissions/permissions.decorator.ts | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 src/permissions/permissions.decorator.ts diff --git a/src/permissions/permissions.decorator.ts b/src/permissions/permissions.decorator.ts new file mode 100644 index 000000000..87e7d18de --- /dev/null +++ b/src/permissions/permissions.decorator.ts @@ -0,0 +1,17 @@ +/* + * SPDX-FileCopyrightText: 2021 The HedgeDoc developers (see AUTHORS file) + * + * SPDX-License-Identifier: AGPL-3.0-only + */ +import { CustomDecorator, SetMetadata } from '@nestjs/common'; + +import { Permission } from './permissions.enum'; + +/** + * This decorator gathers the {@link Permission Permission} a user must hold for the {@link PermissionsGuard} + * @param permissions - an array of permissions. In practice this should always contain exactly one {@link Permission} + * @constructor + */ +// eslint-disable-next-line func-style,@typescript-eslint/naming-convention +export const Permissions = (...permissions: Permission[]): CustomDecorator => + SetMetadata('permissions', permissions); From f6ae0d30a1c9e4eb7fc8da4e09d5c6313508ac83 Mon Sep 17 00:00:00 2001 From: Philip Molares Date: Sun, 21 Nov 2021 17:18:23 +0100 Subject: [PATCH 5/8] feat: create permissions guard This guard protects resources and let's users only access them if they hold the correct permission Signed-off-by: Philip Molares --- src/api/utils/permissions.guard.ts | 66 ++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 src/api/utils/permissions.guard.ts diff --git a/src/api/utils/permissions.guard.ts b/src/api/utils/permissions.guard.ts new file mode 100644 index 000000000..5206f1c48 --- /dev/null +++ b/src/api/utils/permissions.guard.ts @@ -0,0 +1,66 @@ +/* + * SPDX-FileCopyrightText: 2021 The HedgeDoc developers (see AUTHORS file) + * + * SPDX-License-Identifier: AGPL-3.0-only + */ +import { CanActivate, ExecutionContext, Injectable } from '@nestjs/common'; +import { Reflector } from '@nestjs/core'; +import { Request } from 'express'; + +import { ConsoleLoggerService } from '../../logger/console-logger.service'; +import { NotesService } from '../../notes/notes.service'; +import { Permission } from '../../permissions/permissions.enum'; +import { PermissionsService } from '../../permissions/permissions.service'; +import { User } from '../../users/user.entity'; +import { getNote } from './get-note.pipe'; + +/** + * 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. + */ +@Injectable() +export class PermissionsGuard implements CanActivate { + constructor( + private readonly logger: ConsoleLoggerService, + private reflector: Reflector, + private permissionsService: PermissionsService, + private noteService: NotesService, + ) { + this.logger.setContext(PermissionsGuard.name); + } + + async canActivate(context: ExecutionContext): Promise { + const permissions = this.reflector.get( + 'permissions', + context.getHandler(), + ); + // If no permissions are set this is probably an error and this guard should not let the request pass + if (!permissions) { + this.logger.error( + 'Could not find permission metadata. This should never happen. If you see this, please open an issue at https://github.com/hedgedoc/hedgedoc/issues', + ); + return false; + } + const request: Request & { user: User } = context + .switchToHttp() + .getRequest(); + const user = request.user; + // handle CREATE permissions, as this does not need any note + 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 GetNotePipe + const noteIdOrAlias = request.params['noteIdOrAlias']; + const note = await getNote(this.noteService, noteIdOrAlias); + switch (permissions[0]) { + case Permission.READ: + return this.permissionsService.mayRead(user, note); + case Permission.WRITE: + return this.permissionsService.mayWrite(user, note); + case Permission.OWNER: + return this.permissionsService.isOwner(user, note); + } + return false; + } +} From d27c531d9ae20e0317a60623bd0ccd9a0e56d915 Mon Sep 17 00:00:00 2001 From: Philip Molares Date: Sun, 21 Nov 2021 17:19:57 +0100 Subject: [PATCH 6/8] refactor: move permissions service calls into permissions guard This commit removes all previous calls to the permissions service at the beginning of the controller methods to the permissions guard. This should make the code a bit cleaner and remove boilerplate code. Signed-off-by: Philip Molares --- src/api/private/notes/notes.controller.ts | 47 ++++++--------- src/api/public/notes/notes.controller.ts | 73 ++++++++--------------- 2 files changed, 44 insertions(+), 76 deletions(-) diff --git a/src/api/private/notes/notes.controller.ts b/src/api/private/notes/notes.controller.ts index c7007c878..4d38ff561 100644 --- a/src/api/private/notes/notes.controller.ts +++ b/src/api/private/notes/notes.controller.ts @@ -13,7 +13,6 @@ import { NotFoundException, Param, Post, - UnauthorizedException, UseGuards, } from '@nestjs/common'; @@ -31,7 +30,8 @@ import { NoteDto } from '../../../notes/note.dto'; import { Note } from '../../../notes/note.entity'; import { NoteMediaDeletionDto } from '../../../notes/note.media-deletion.dto'; import { NotesService } from '../../../notes/notes.service'; -import { PermissionsService } from '../../../permissions/permissions.service'; +import { Permissions } from '../../../permissions/permissions.decorator'; +import { Permission } from '../../../permissions/permissions.enum'; import { RevisionMetadataDto } from '../../../revisions/revision-metadata.dto'; import { RevisionDto } from '../../../revisions/revision.dto'; import { RevisionsService } from '../../../revisions/revisions.service'; @@ -39,6 +39,7 @@ import { User } from '../../../users/user.entity'; import { UsersService } from '../../../users/users.service'; import { GetNotePipe } from '../../utils/get-note.pipe'; import { MarkdownBody } from '../../utils/markdownbody-decorator'; +import { PermissionsGuard } from '../../utils/permissions.guard'; import { RequestUser } from '../../utils/request-user.decorator'; @UseGuards(SessionGuard) @@ -47,7 +48,6 @@ export class NotesController { constructor( private readonly logger: ConsoleLoggerService, private noteService: NotesService, - private permissionsService: PermissionsService, private historyService: HistoryService, private userService: UsersService, private mediaService: MediaService, @@ -57,38 +57,34 @@ export class NotesController { } @Get(':noteIdOrAlias') + @Permissions(Permission.READ) + @UseGuards(PermissionsGuard) async getNote( @RequestUser() user: User, @Param('noteIdOrAlias', GetNotePipe) note: Note, ): Promise { - if (!this.permissionsService.mayRead(user, note)) { - throw new UnauthorizedException('Reading note denied!'); - } await this.historyService.updateHistoryEntryTimestamp(note, user); return await this.noteService.toNoteDto(note); } @Get(':noteIdOrAlias/media') + @Permissions(Permission.READ) + @UseGuards(PermissionsGuard) async getNotesMedia( @Param('noteIdOrAlias', GetNotePipe) note: Note, - @RequestUser() user: User, ): Promise { - if (!this.permissionsService.mayRead(user, note)) { - throw new UnauthorizedException('Reading note denied!'); - } const media = await this.mediaService.listUploadsByNote(note); return media.map((media) => this.mediaService.toMediaUploadDto(media)); } @Post() @HttpCode(201) + @Permissions(Permission.CREATE) + @UseGuards(PermissionsGuard) async createNote( @RequestUser() user: User, @MarkdownBody() text: string, ): Promise { - if (!this.permissionsService.mayCreate(user)) { - throw new UnauthorizedException('Creating note denied!'); - } this.logger.debug('Got raw markdown:\n' + text); return await this.noteService.toNoteDto( await this.noteService.createNote(text, user), @@ -97,14 +93,13 @@ export class NotesController { @Post(':noteAlias') @HttpCode(201) + @Permissions(Permission.CREATE) + @UseGuards(PermissionsGuard) async createNamedNote( @RequestUser() user: User, @Param('noteAlias') noteAlias: string, @MarkdownBody() text: string, ): Promise { - if (!this.permissionsService.mayCreate(user)) { - throw new UnauthorizedException('Creating note denied!'); - } this.logger.debug('Got raw markdown:\n' + text, 'createNamedNote'); try { return await this.noteService.toNoteDto( @@ -123,14 +118,13 @@ export class NotesController { @Delete(':noteIdOrAlias') @HttpCode(204) + @Permissions(Permission.OWNER) + @UseGuards(PermissionsGuard) async deleteNote( @RequestUser() user: User, @Param('noteIdOrAlias', GetNotePipe) note: Note, @Body() noteMediaDeletionDto: NoteMediaDeletionDto, ): Promise { - if (!this.permissionsService.isOwner(user, note)) { - throw new UnauthorizedException('Deleting note denied!'); - } const mediaUploads = await this.mediaService.listUploadsByNote(note); for (const mediaUpload of mediaUploads) { if (!noteMediaDeletionDto.keepMedia) { @@ -146,13 +140,12 @@ export class NotesController { } @Get(':noteIdOrAlias/revisions') + @Permissions(Permission.READ) + @UseGuards(PermissionsGuard) async getNoteRevisions( @RequestUser() user: User, @Param('noteIdOrAlias', GetNotePipe) note: Note, ): Promise { - if (!this.permissionsService.mayRead(user, note)) { - throw new UnauthorizedException('Reading note denied!'); - } const revisions = await this.revisionsService.getAllRevisions(note); return await Promise.all( revisions.map((revision) => @@ -163,13 +156,12 @@ export class NotesController { @Delete(':noteIdOrAlias/revisions') @HttpCode(204) + @Permissions(Permission.READ) + @UseGuards(PermissionsGuard) async purgeNoteRevisions( @RequestUser() user: User, @Param('noteIdOrAlias', GetNotePipe) note: Note, ): Promise { - if (!this.permissionsService.mayRead(user, note)) { - throw new UnauthorizedException('Reading note denied!'); - } this.logger.debug( 'Purging history of note: ' + note.id, 'purgeNoteRevisions', @@ -183,15 +175,14 @@ export class NotesController { } @Get(':noteIdOrAlias/revisions/:revisionId') + @Permissions(Permission.READ) + @UseGuards(PermissionsGuard) async getNoteRevision( @RequestUser() user: User, @Param('noteIdOrAlias', GetNotePipe) note: Note, @Param('revisionId') revisionId: number, ): Promise { try { - if (!this.permissionsService.mayRead(user, note)) { - throw new UnauthorizedException('Reading note denied!'); - } return this.revisionsService.toRevisionDto( await this.revisionsService.getRevision(note, revisionId), ); diff --git a/src/api/public/notes/notes.controller.ts b/src/api/public/notes/notes.controller.ts index 49583e92c..fc4ccdf63 100644 --- a/src/api/public/notes/notes.controller.ts +++ b/src/api/public/notes/notes.controller.ts @@ -15,7 +15,6 @@ import { Param, Post, Put, - UnauthorizedException, UseGuards, } from '@nestjs/common'; import { @@ -48,7 +47,8 @@ import { NoteDto } from '../../../notes/note.dto'; import { Note } from '../../../notes/note.entity'; import { NoteMediaDeletionDto } from '../../../notes/note.media-deletion.dto'; import { NotesService } from '../../../notes/notes.service'; -import { PermissionsService } from '../../../permissions/permissions.service'; +import { Permissions } from '../../../permissions/permissions.decorator'; +import { Permission } from '../../../permissions/permissions.enum'; import { RevisionMetadataDto } from '../../../revisions/revision-metadata.dto'; import { RevisionDto } from '../../../revisions/revision.dto'; import { RevisionsService } from '../../../revisions/revisions.service'; @@ -61,6 +61,7 @@ import { import { FullApi } from '../../utils/fullapi-decorator'; import { GetNotePipe } from '../../utils/get-note.pipe'; import { MarkdownBody } from '../../utils/markdownbody-decorator'; +import { PermissionsGuard } from '../../utils/permissions.guard'; import { RequestUser } from '../../utils/request-user.decorator'; @ApiTags('notes') @@ -71,14 +72,14 @@ export class NotesController { private readonly logger: ConsoleLoggerService, private noteService: NotesService, private revisionsService: RevisionsService, - private permissionsService: PermissionsService, private historyService: HistoryService, private mediaService: MediaService, ) { this.logger.setContext(NotesController.name); } - @UseGuards(TokenAuthGuard) + @Permissions(Permission.CREATE) + @UseGuards(TokenAuthGuard, PermissionsGuard) @Post() @HttpCode(201) @ApiUnauthorizedResponse({ description: unauthorizedDescription }) @@ -87,17 +88,14 @@ export class NotesController { @RequestUser() user: User, @MarkdownBody() text: string, ): Promise { - // ToDo: provide user for createNoteDto - if (!this.permissionsService.mayCreate(user)) { - throw new UnauthorizedException('Creating note denied!'); - } this.logger.debug('Got raw markdown:\n' + text); return await this.noteService.toNoteDto( await this.noteService.createNote(text, user), ); } - @UseGuards(TokenAuthGuard) + @Permissions(Permission.READ) + @UseGuards(TokenAuthGuard, PermissionsGuard) @Get(':noteIdOrAlias') @ApiOkResponse({ description: 'Get information about the newly created note', @@ -108,14 +106,12 @@ export class NotesController { @RequestUser() user: User, @Param('noteIdOrAlias', GetNotePipe) note: Note, ): Promise { - if (!this.permissionsService.mayRead(user, note)) { - throw new UnauthorizedException('Reading note denied!'); - } await this.historyService.updateHistoryEntryTimestamp(note, user); return await this.noteService.toNoteDto(note); } - @UseGuards(TokenAuthGuard) + @Permissions(Permission.CREATE) + @UseGuards(TokenAuthGuard, PermissionsGuard) @Post(':noteAlias') @HttpCode(201) @ApiCreatedResponse({ @@ -129,9 +125,6 @@ export class NotesController { @Param('noteAlias') noteAlias: string, @MarkdownBody() text: string, ): Promise { - if (!this.permissionsService.mayCreate(user)) { - throw new UnauthorizedException('Creating note denied!'); - } this.logger.debug('Got raw markdown:\n' + text, 'createNamedNote'); try { return await this.noteService.toNoteDto( @@ -148,7 +141,8 @@ export class NotesController { } } - @UseGuards(TokenAuthGuard) + @Permissions(Permission.OWNER) + @UseGuards(TokenAuthGuard, PermissionsGuard) @Delete(':noteIdOrAlias') @HttpCode(204) @ApiNoContentResponse({ description: successfullyDeletedDescription }) @@ -158,9 +152,6 @@ export class NotesController { @Param('noteIdOrAlias', GetNotePipe) note: Note, @Body() noteMediaDeletionDto: NoteMediaDeletionDto, ): Promise { - if (!this.permissionsService.isOwner(user, note)) { - throw new UnauthorizedException('Deleting note denied!'); - } const mediaUploads = await this.mediaService.listUploadsByNote(note); for (const mediaUpload of mediaUploads) { if (!noteMediaDeletionDto.keepMedia) { @@ -175,7 +166,8 @@ export class NotesController { return; } - @UseGuards(TokenAuthGuard) + @Permissions(Permission.WRITE) + @UseGuards(TokenAuthGuard, PermissionsGuard) @Put(':noteIdOrAlias') @ApiOkResponse({ description: 'The new, changed note', @@ -187,16 +179,14 @@ export class NotesController { @Param('noteIdOrAlias', GetNotePipe) note: Note, @MarkdownBody() text: string, ): Promise { - if (!this.permissionsService.mayWrite(user, note)) { - throw new UnauthorizedException('Updating note denied!'); - } this.logger.debug('Got raw markdown:\n' + text, 'updateNote'); return await this.noteService.toNoteDto( await this.noteService.updateNote(note, text), ); } - @UseGuards(TokenAuthGuard) + @Permissions(Permission.READ) + @UseGuards(TokenAuthGuard, PermissionsGuard) @Get(':noteIdOrAlias/content') @ApiProduces('text/markdown') @ApiOkResponse({ @@ -208,13 +198,11 @@ export class NotesController { @RequestUser() user: User, @Param('noteIdOrAlias', GetNotePipe) note: Note, ): Promise { - if (!this.permissionsService.mayRead(user, note)) { - throw new UnauthorizedException('Reading note denied!'); - } return await this.noteService.getNoteContent(note); } - @UseGuards(TokenAuthGuard) + @Permissions(Permission.READ) + @UseGuards(TokenAuthGuard, PermissionsGuard) @Get(':noteIdOrAlias/metadata') @ApiOkResponse({ description: 'The metadata of the note', @@ -225,13 +213,11 @@ export class NotesController { @RequestUser() user: User, @Param('noteIdOrAlias', GetNotePipe) note: Note, ): Promise { - if (!this.permissionsService.mayRead(user, note)) { - throw new UnauthorizedException('Reading note denied!'); - } return await this.noteService.toNoteMetadataDto(note); } - @UseGuards(TokenAuthGuard) + @Permissions(Permission.OWNER) + @UseGuards(TokenAuthGuard, PermissionsGuard) @Put(':noteIdOrAlias/metadata/permissions') @ApiOkResponse({ description: 'The updated permissions of the note', @@ -243,15 +229,13 @@ export class NotesController { @Param('noteIdOrAlias', GetNotePipe) note: Note, @Body() updateDto: NotePermissionsUpdateDto, ): Promise { - if (!this.permissionsService.isOwner(user, note)) { - throw new UnauthorizedException('Updating note denied!'); - } return this.noteService.toNotePermissionsDto( await this.noteService.updateNotePermissions(note, updateDto), ); } - @UseGuards(TokenAuthGuard) + @Permissions(Permission.READ) + @UseGuards(TokenAuthGuard, PermissionsGuard) @Get(':noteIdOrAlias/revisions') @ApiOkResponse({ description: 'Revisions of the note', @@ -263,9 +247,6 @@ export class NotesController { @RequestUser() user: User, @Param('noteIdOrAlias', GetNotePipe) note: Note, ): Promise { - if (!this.permissionsService.mayRead(user, note)) { - throw new UnauthorizedException('Reading note denied!'); - } const revisions = await this.revisionsService.getAllRevisions(note); return await Promise.all( revisions.map((revision) => @@ -274,7 +255,8 @@ export class NotesController { ); } - @UseGuards(TokenAuthGuard) + @Permissions(Permission.READ) + @UseGuards(TokenAuthGuard, PermissionsGuard) @Get(':noteIdOrAlias/revisions/:revisionId') @ApiOkResponse({ description: 'Revision of the note for the given id or alias', @@ -286,9 +268,6 @@ export class NotesController { @Param('noteIdOrAlias', GetNotePipe) note: Note, @Param('revisionId') revisionId: number, ): Promise { - if (!this.permissionsService.mayRead(user, note)) { - throw new UnauthorizedException('Reading note denied!'); - } try { return this.revisionsService.toRevisionDto( await this.revisionsService.getRevision(note, revisionId), @@ -301,7 +280,8 @@ export class NotesController { } } - @UseGuards(TokenAuthGuard) + @Permissions(Permission.READ) + @UseGuards(TokenAuthGuard, PermissionsGuard) @Get(':noteIdOrAlias/media') @ApiOkResponse({ description: 'All media uploads of the note', @@ -313,9 +293,6 @@ export class NotesController { @RequestUser() user: User, @Param('noteIdOrAlias', GetNotePipe) note: Note, ): Promise { - if (!this.permissionsService.mayRead(user, note)) { - throw new UnauthorizedException('Reading note denied!'); - } const media = await this.mediaService.listUploadsByNote(note); return media.map((media) => this.mediaService.toMediaUploadDto(media)); } From 40e8acb6bbd3ba85d7a9247129d0ff2f75d3eeb5 Mon Sep 17 00:00:00 2001 From: Philip Molares Date: Sun, 21 Nov 2021 18:03:29 +0100 Subject: [PATCH 7/8] test: fix note e2e test 'fails, when user can't read note' Because the rejection now happens automatically in the permissions guard it now returns a 403 instead of 401 Signed-off-by: Philip Molares --- test/private-api/notes.e2e-spec.ts | 2 +- test/public-api/notes.e2e-spec.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/private-api/notes.e2e-spec.ts b/test/private-api/notes.e2e-spec.ts index 5e800e89d..efa5f5eb3 100644 --- a/test/private-api/notes.e2e-spec.ts +++ b/test/private-api/notes.e2e-spec.ts @@ -349,7 +349,7 @@ describe('Notes', () => { await agent .get(`/api/private/notes/${alias}/media/`) .expect('Content-Type', /json/) - .expect(401); + .expect(403); }); }); diff --git a/test/public-api/notes.e2e-spec.ts b/test/public-api/notes.e2e-spec.ts index 971af9913..3c608e302 100644 --- a/test/public-api/notes.e2e-spec.ts +++ b/test/public-api/notes.e2e-spec.ts @@ -464,7 +464,7 @@ describe('Notes', () => { await request(testSetup.app.getHttpServer()) .get(`/api/v2/notes/${alias}/media/`) .expect('Content-Type', /json/) - .expect(401); + .expect(403); }); }); From 16cd42f1970fb1e4b3d421b7f5eb64e1b84f048c Mon Sep 17 00:00:00 2001 From: Philip Molares Date: Sun, 21 Nov 2021 18:04:47 +0100 Subject: [PATCH 8/8] test: fix note e2e test 'fails with non-existing alias' Because the rejection now happens automatically in the permissions guard it does not get to the controller method and does not report the Content-Type to text/markdown Signed-off-by: Philip Molares --- test/public-api/notes.e2e-spec.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/test/public-api/notes.e2e-spec.ts b/test/public-api/notes.e2e-spec.ts index 3c608e302..bc8093e6d 100644 --- a/test/public-api/notes.e2e-spec.ts +++ b/test/public-api/notes.e2e-spec.ts @@ -396,7 +396,6 @@ describe('Notes', () => { // check if a missing note correctly returns 404 await request(testSetup.app.getHttpServer()) .get('/api/v2/notes/i_dont_exist/content') - .expect('Content-Type', /text\/markdown/) .expect(404); }); });