From f8e07f69406430d9dfa23e19f32ab87a88f2c981 Mon Sep 17 00:00:00 2001 From: Yannick Bungers Date: Wed, 27 Jan 2021 22:58:55 +0100 Subject: [PATCH 1/7] Add relation between User and Group This represents the users which are members of this group Signed-off-by: Yannick Bungers --- docs/content/dev/db-schema.plantuml | 17 +++++++++----- src/api/public/me/me.controller.spec.ts | 6 +++++ src/api/public/media/media.controller.spec.ts | 6 +++++ src/api/public/notes/notes.controller.spec.ts | 15 ++++++++++++- src/groups/group.entity.ts | 16 +++++++++++++- src/history/history.service.spec.ts | 22 ++++++++++++------- src/media/media.service.spec.ts | 6 +++++ src/notes/notes.module.ts | 10 ++++++++- src/notes/notes.service.spec.ts | 6 +++++ src/notes/notes.service.ts | 1 + src/revisions/revisions.service.spec.ts | 6 +++++ src/users/user.entity.ts | 8 +++++++ 12 files changed, 103 insertions(+), 16 deletions(-) diff --git a/docs/content/dev/db-schema.plantuml b/docs/content/dev/db-schema.plantuml index b64bce37f..707ec9c89 100644 --- a/docs/content/dev/db-schema.plantuml +++ b/docs/content/dev/db-schema.plantuml @@ -119,6 +119,11 @@ entity "note_group_permission" { *canEdit : boolean } +entity "group_members_user" { + *group : number <> + *member : uuid <> +} + entity "tag" { *id: number <> *name: text @@ -144,16 +149,16 @@ entity "history_entry" { user "1" -- "0..*" note: owner user "1" -u- "1..*" identity -user "1" - "1..*" auth_token: authTokens -user "1" -l- "1..*" session -user "1" - "0..*" media_upload +user "1" -l- "1..*" auth_token: authTokens +user "1" -r- "1..*" session +user "1" -- "0..*" media_upload user "1" - "0..*" history_entry user "0..*" -- "0..*" note -user "1" - "0..*" authorship +user "1" -- "0..*" authorship (user, note) . author_colors -revision "0..*" - "0..*" authorship +revision "0..*" -- "0..*" authorship (revision, authorship) .. revision_authorship media_upload "0..*" -- "1" note @@ -161,9 +166,11 @@ note "1" - "1..*" revision note "1" - "0..*" history_entry note "0..*" -l- "0..*" tag note "0..*" -- "0..*" group +user "1..*" -- "0..*" group user "0..*" -- "0..*" note (user, note) . note_user_permission (note, group) . note_group_permission +(user, group) . group_members_user @enduml diff --git a/src/api/public/me/me.controller.spec.ts b/src/api/public/me/me.controller.spec.ts index a71d73dcf..43110f49c 100644 --- a/src/api/public/me/me.controller.spec.ts +++ b/src/api/public/me/me.controller.spec.ts @@ -20,6 +20,8 @@ import { User } from '../../../users/user.entity'; import { UsersModule } from '../../../users/users.module'; import { MeController } from './me.controller'; import { HistoryEntry } from '../../../history/history-entry.entity'; +import { NoteGroupPermission } from '../../../permissions/note-group-permission.entity'; +import { NoteUserPermission } from '../../../permissions/note-user-permission.entity'; describe('Me Controller', () => { let controller: MeController; @@ -47,6 +49,10 @@ describe('Me Controller', () => { .useValue({}) .overrideProvider(getRepositoryToken(HistoryEntry)) .useValue({}) + .overrideProvider(getRepositoryToken(NoteGroupPermission)) + .useValue({}) + .overrideProvider(getRepositoryToken(NoteUserPermission)) + .useValue({}) .compile(); controller = module.get(MeController); diff --git a/src/api/public/media/media.controller.spec.ts b/src/api/public/media/media.controller.spec.ts index eb699f1c2..7da3ca771 100644 --- a/src/api/public/media/media.controller.spec.ts +++ b/src/api/public/media/media.controller.spec.ts @@ -22,6 +22,8 @@ import { AuthToken } from '../../../auth/auth-token.entity'; import { Identity } from '../../../users/identity.entity'; import { User } from '../../../users/user.entity'; import { MediaController } from './media.controller'; +import { NoteGroupPermission } from '../../../permissions/note-group-permission.entity'; +import { NoteUserPermission } from '../../../permissions/note-user-permission.entity'; describe('Media Controller', () => { let controller: MediaController; @@ -57,6 +59,10 @@ describe('Media Controller', () => { .useValue({}) .overrideProvider(getRepositoryToken(Tag)) .useValue({}) + .overrideProvider(getRepositoryToken(NoteGroupPermission)) + .useValue({}) + .overrideProvider(getRepositoryToken(NoteUserPermission)) + .useValue({}) .compile(); controller = module.get(MediaController); diff --git a/src/api/public/notes/notes.controller.spec.ts b/src/api/public/notes/notes.controller.spec.ts index 6df9c73cb..18f28a708 100644 --- a/src/api/public/notes/notes.controller.spec.ts +++ b/src/api/public/notes/notes.controller.spec.ts @@ -19,8 +19,11 @@ import { Identity } from '../../../users/identity.entity'; import { User } from '../../../users/user.entity'; import { UsersModule } from '../../../users/users.module'; import { NotesController } from './notes.controller'; +import { PermissionsModule } from '../../../permissions/permissions.module'; import { HistoryModule } from '../../../history/history.module'; import { HistoryEntry } from '../../../history/history-entry.entity'; +import { NoteGroupPermission } from '../../../permissions/note-group-permission.entity'; +import { NoteUserPermission } from '../../../permissions/note-user-permission.entity'; describe('Notes Controller', () => { let controller: NotesController; @@ -39,7 +42,13 @@ describe('Notes Controller', () => { useValue: {}, }, ], - imports: [RevisionsModule, UsersModule, LoggerModule, HistoryModule], + imports: [ + RevisionsModule, + UsersModule, + LoggerModule, + PermissionsModule, + HistoryModule, + ], }) .overrideProvider(getRepositoryToken(Note)) .useValue({}) @@ -61,6 +70,10 @@ describe('Notes Controller', () => { .useValue({}) .overrideProvider(getRepositoryToken(HistoryEntry)) .useValue({}) + .overrideProvider(getRepositoryToken(NoteGroupPermission)) + .useValue({}) + .overrideProvider(getRepositoryToken(NoteUserPermission)) + .useValue({}) .compile(); controller = module.get(NotesController); diff --git a/src/groups/group.entity.ts b/src/groups/group.entity.ts index 427709718..0574e243e 100644 --- a/src/groups/group.entity.ts +++ b/src/groups/group.entity.ts @@ -4,7 +4,14 @@ * SPDX-License-Identifier: AGPL-3.0-only */ -import { Column, Entity, PrimaryGeneratedColumn } from 'typeorm'; +import { + Column, + Entity, + JoinTable, + ManyToMany, + PrimaryGeneratedColumn, +} from 'typeorm'; +import { User } from '../users/user.entity'; @Entity() export class Group { @@ -26,4 +33,11 @@ export class Group { */ @Column() special: boolean; + + @ManyToMany((_) => User, (user) => user.groups, { + eager: true, + cascade: true, + }) + @JoinTable() + members: User[]; } diff --git a/src/history/history.service.spec.ts b/src/history/history.service.spec.ts index ccf0ba04e..caa177c36 100644 --- a/src/history/history.service.spec.ts +++ b/src/history/history.service.spec.ts @@ -21,6 +21,8 @@ import { AuthToken } from '../auth/auth-token.entity'; import { Revision } from '../revisions/revision.entity'; import { Repository } from 'typeorm'; import { NotInDBError } from '../errors/errors'; +import { NoteGroupPermission } from '../permissions/note-group-permission.entity'; +import { NoteUserPermission } from '../permissions/note-user-permission.entity'; describe('HistoryService', () => { let service: HistoryService; @@ -54,6 +56,10 @@ describe('HistoryService', () => { .useClass(Repository) .overrideProvider(getRepositoryToken(Tag)) .useValue({}) + .overrideProvider(getRepositoryToken(NoteGroupPermission)) + .useValue({}) + .overrideProvider(getRepositoryToken(NoteUserPermission)) + .useValue({}) .compile(); service = module.get(HistoryService); @@ -99,7 +105,7 @@ describe('HistoryService', () => { describe('createOrUpdateHistoryEntry', () => { describe('works', () => { it('without an preexisting entry', async () => { - const user = new User(); + const user = {} as User; const alias = 'alias'; jest.spyOn(historyRepo, 'findOne').mockResolvedValueOnce(undefined); jest @@ -118,7 +124,7 @@ describe('HistoryService', () => { }); it('with an preexisting entry', async () => { - const user = new User(); + const user = {} as User; const alias = 'alias'; const historyEntry = HistoryEntry.create( user, @@ -148,7 +154,7 @@ describe('HistoryService', () => { describe('updateHistoryEntry', () => { describe('works', () => { it('with an entry', async () => { - const user = new User(); + const user = {} as User; const alias = 'alias'; const note = Note.create(user, alias); const historyEntry = HistoryEntry.create(user, note); @@ -173,7 +179,7 @@ describe('HistoryService', () => { }); it('without an entry', async () => { - const user = new User(); + const user = {} as User; const alias = 'alias'; const note = Note.create(user, alias); jest.spyOn(historyRepo, 'findOne').mockResolvedValueOnce(undefined); @@ -192,7 +198,7 @@ describe('HistoryService', () => { describe('deleteHistoryEntry', () => { describe('works', () => { it('with an entry', async () => { - const user = new User(); + const user = {} as User; const alias = 'alias'; const note = Note.create(user, alias); const historyEntry = HistoryEntry.create(user, note); @@ -208,7 +214,7 @@ describe('HistoryService', () => { }); it('without an entry', async () => { - const user = new User(); + const user = {} as User; const alias = 'alias'; const note = Note.create(user, alias); jest.spyOn(historyRepo, 'findOne').mockResolvedValueOnce(undefined); @@ -225,7 +231,7 @@ describe('HistoryService', () => { describe('toHistoryEntryDto', () => { describe('works', () => { it('with aliased note', async () => { - const user = new User(); + const user = {} as User; const alias = 'alias'; const title = 'title'; const tags = ['tag1', 'tag2']; @@ -247,7 +253,7 @@ describe('HistoryService', () => { }); it('with regular note', async () => { - const user = new User(); + const user = {} as User; const title = 'title'; const id = 'id'; const tags = ['tag1', 'tag2']; diff --git a/src/media/media.service.spec.ts b/src/media/media.service.spec.ts index 82821b857..9c2c0535e 100644 --- a/src/media/media.service.spec.ts +++ b/src/media/media.service.spec.ts @@ -25,6 +25,8 @@ import { MediaService } from './media.service'; import { Repository } from 'typeorm'; import { promises as fs } from 'fs'; import { ClientError, NotInDBError, PermissionError } from '../errors/errors'; +import { NoteGroupPermission } from '../permissions/note-group-permission.entity'; +import { NoteUserPermission } from '../permissions/note-user-permission.entity'; describe('MediaService', () => { let service: MediaService; @@ -70,6 +72,10 @@ describe('MediaService', () => { .useClass(Repository) .overrideProvider(getRepositoryToken(Tag)) .useValue({}) + .overrideProvider(getRepositoryToken(NoteGroupPermission)) + .useValue({}) + .overrideProvider(getRepositoryToken(NoteUserPermission)) + .useValue({}) .overrideProvider(getRepositoryToken(MediaUpload)) .useClass(Repository) .compile(); diff --git a/src/notes/notes.module.ts b/src/notes/notes.module.ts index fc8bd92ac..cba204cf6 100644 --- a/src/notes/notes.module.ts +++ b/src/notes/notes.module.ts @@ -13,10 +13,18 @@ import { AuthorColor } from './author-color.entity'; import { Note } from './note.entity'; import { NotesService } from './notes.service'; import { Tag } from './tag.entity'; +import { NoteGroupPermission } from '../permissions/note-group-permission.entity'; +import { NoteUserPermission } from '../permissions/note-user-permission.entity'; @Module({ imports: [ - TypeOrmModule.forFeature([Note, AuthorColor, Tag]), + TypeOrmModule.forFeature([ + Note, + AuthorColor, + Tag, + NoteGroupPermission, + NoteUserPermission, + ]), forwardRef(() => RevisionsModule), UsersModule, LoggerModule, diff --git a/src/notes/notes.service.spec.ts b/src/notes/notes.service.spec.ts index 2c623796a..d08cc8f03 100644 --- a/src/notes/notes.service.spec.ts +++ b/src/notes/notes.service.spec.ts @@ -18,6 +18,8 @@ import { AuthorColor } from './author-color.entity'; import { Note } from './note.entity'; import { NotesService } from './notes.service'; import { Tag } from './tag.entity'; +import { NoteGroupPermission } from '../permissions/note-group-permission.entity'; +import { NoteUserPermission } from '../permissions/note-user-permission.entity'; describe('NotesService', () => { let service: NotesService; @@ -53,6 +55,10 @@ describe('NotesService', () => { .useValue({}) .overrideProvider(getRepositoryToken(Tag)) .useValue({}) + .overrideProvider(getRepositoryToken(NoteGroupPermission)) + .useValue({}) + .overrideProvider(getRepositoryToken(NoteUserPermission)) + .useValue({}) .compile(); service = module.get(NotesService); }); diff --git a/src/notes/notes.service.ts b/src/notes/notes.service.ts index af14b8174..107403e82 100644 --- a/src/notes/notes.service.ts +++ b/src/notes/notes.service.ts @@ -159,6 +159,7 @@ export class NotesService { historyEntries: [], updatedAt: new Date(), userName: 'Testy', + groups: [], }, description: 'Very descriptive text.', userPermissions: [], diff --git a/src/revisions/revisions.service.spec.ts b/src/revisions/revisions.service.spec.ts index 5c1d9d437..43799ae0d 100644 --- a/src/revisions/revisions.service.spec.ts +++ b/src/revisions/revisions.service.spec.ts @@ -17,6 +17,8 @@ import { Authorship } from './authorship.entity'; import { Revision } from './revision.entity'; import { RevisionsService } from './revisions.service'; import { Tag } from '../notes/tag.entity'; +import { NoteGroupPermission } from '../permissions/note-group-permission.entity'; +import { NoteUserPermission } from '../permissions/note-user-permission.entity'; describe('RevisionsService', () => { let service: RevisionsService; @@ -48,6 +50,10 @@ describe('RevisionsService', () => { .useValue({}) .overrideProvider(getRepositoryToken(Tag)) .useValue({}) + .overrideProvider(getRepositoryToken(NoteGroupPermission)) + .useValue({}) + .overrideProvider(getRepositoryToken(NoteUserPermission)) + .useValue({}) .compile(); service = module.get(RevisionsService); diff --git a/src/users/user.entity.ts b/src/users/user.entity.ts index ba8016495..ca753c7bd 100644 --- a/src/users/user.entity.ts +++ b/src/users/user.entity.ts @@ -7,6 +7,7 @@ import { CreateDateColumn, Entity, + ManyToMany, PrimaryGeneratedColumn, UpdateDateColumn, } from 'typeorm'; @@ -14,6 +15,7 @@ import { Column, OneToMany } from 'typeorm'; import { Note } from '../notes/note.entity'; import { AuthToken } from '../auth/auth-token.entity'; import { Identity } from './identity.entity'; +import { Group } from '../groups/group.entity'; import { HistoryEntry } from '../history/history-entry.entity'; @Entity() @@ -52,9 +54,15 @@ export class User { @OneToMany((_) => Identity, (identity) => identity.user) identities: Identity[]; + @ManyToMany((_) => Group, (group) => group.members) + groups: Group[]; + @OneToMany((_) => HistoryEntry, (historyEntry) => historyEntry.user) historyEntries: HistoryEntry[]; + // eslint-disable-next-line @typescript-eslint/no-empty-function + private constructor() {} + public static create( userName: string, displayName: string, From f40ed5db2a79c5f52819c60f58a886c7ced143c3 Mon Sep 17 00:00:00 2001 From: Yannick Bungers Date: Tue, 16 Feb 2021 09:32:58 +0100 Subject: [PATCH 2/7] Add permissions Service Checks if the given user has sufficient rights on the given resource. Signed-off-by: Yannick Bungers --- src/permissions/permissions.module.ts | 7 +- src/permissions/permissions.service.ts | 100 +++++++++++++++++++++++++ 2 files changed, 103 insertions(+), 4 deletions(-) create mode 100644 src/permissions/permissions.service.ts diff --git a/src/permissions/permissions.module.ts b/src/permissions/permissions.module.ts index 9d0d334e2..16e4e0db8 100644 --- a/src/permissions/permissions.module.ts +++ b/src/permissions/permissions.module.ts @@ -9,11 +9,10 @@ import { TypeOrmModule } from '@nestjs/typeorm'; import { LoggerModule } from '../logger/logger.module'; import { NoteGroupPermission } from './note-group-permission.entity'; import { NoteUserPermission } from './note-user-permission.entity'; +import { PermissionsService } from './permissions.service'; @Module({ - imports: [ - TypeOrmModule.forFeature([NoteUserPermission, NoteGroupPermission]), - LoggerModule, - ], + exports: [PermissionsService], + providers: [PermissionsService], }) export class PermissionsModule {} diff --git a/src/permissions/permissions.service.ts b/src/permissions/permissions.service.ts new file mode 100644 index 000000000..8c5edf585 --- /dev/null +++ b/src/permissions/permissions.service.ts @@ -0,0 +1,100 @@ +/* + * SPDX-FileCopyrightText: 2021 The HedgeDoc developers (see AUTHORS file) + * + * SPDX-License-Identifier: AGPL-3.0-only + */ + +import { Injectable } from '@nestjs/common'; +import { User } from '../users/user.entity'; +import { Note } from '../notes/note.entity'; +import { ConsoleLoggerService } from '../logger/console-logger.service'; + +@Injectable() +export class PermissionsService { + constructor(private readonly logger: ConsoleLoggerService) {} + mayRead(user: User, note: Note): boolean { + if (this.isOwner(user, note)) return true; + + if (this.hasPermissionUser(user, note, false)) return true; + + if (this.hasPermissionGroup(user, note, false)) return true; + + return false; + } + + mayWrite(user: User, note: Note): boolean { + if (this.isOwner(user, note)) return true; + + if (this.hasPermissionUser(user, note, true)) return true; + + if (this.hasPermissionGroup(user, note, true)) return true; + + return false; + } + mayCreate(user: User): boolean { + if (user) { + // TODO: (config.guestPermission == "create") + return true; + } + return false; + } + isOwner(user: User, note: Note): boolean { + if (!user) return false; + return note.owner.id === user.id; + } + + private hasPermissionUser( + user: User, + note: Note, + wantEdit: boolean, + ): boolean { + if (!user) { + return false; + } + for (const userPermission of note.userPermissions) { + if ( + userPermission.user.id === user.id && + (userPermission.canEdit || !wantEdit) + ) { + return true; + } + } + return false; + } + + private hasPermissionGroup( + user: User, + note: Note, + wantEdit: boolean, + ): boolean { + // TODO: Get real config value + const guestsAllowed = false; // (config.guestPermission == "write" || config.guestPermission == "read" && !wantEdit) + for (const groupPermission of note.groupPermissions) { + if (groupPermission.canEdit || !wantEdit) { + // Handle special groups + if (groupPermission.group.special) { + if (groupPermission.group.name == 'loggedIn') { + // TODO: Name of group for logged in users + return true; + } + if ( + groupPermission.group.name == 'everybody' && + (groupPermission.canEdit || !wantEdit) && + guestsAllowed + ) { + // TODO: Name of group in which everybody even guests can edit + return true; + } + } else { + // Handle normal groups + if (user) { + for (const member of groupPermission.group.members) { + if (member.id === user.id) return true; + } + } + } + } + } + return false; + } +} From a694d71fffffb54d4c6c0329b9ecdf4a1ca8fc29 Mon Sep 17 00:00:00 2001 From: Yannick Bungers Date: Tue, 16 Feb 2021 09:33:42 +0100 Subject: [PATCH 3/7] Add permission checks for notes routes Signed-off-by: Yannick Bungers --- src/api/public/notes/notes.controller.ts | 64 ++++++++++++++++++------ src/api/public/public-api.module.ts | 2 + 2 files changed, 52 insertions(+), 14 deletions(-) diff --git a/src/api/public/notes/notes.controller.ts b/src/api/public/notes/notes.controller.ts index d5df2c585..68a98c8dd 100644 --- a/src/api/public/notes/notes.controller.ts +++ b/src/api/public/notes/notes.controller.ts @@ -15,6 +15,7 @@ import { Post, Put, Request, + UnauthorizedException, UseGuards, } from '@nestjs/common'; import { NotInDBError } from '../../../errors/errors'; @@ -33,6 +34,8 @@ import { NoteDto } from '../../../notes/note.dto'; import { NoteMetadataDto } from '../../../notes/note-metadata.dto'; import { RevisionMetadataDto } from '../../../revisions/revision-metadata.dto'; import { RevisionDto } from '../../../revisions/revision.dto'; +import { PermissionsService } from '../../../permissions/permissions.service'; +import { Note } from '../../../notes/note.entity'; @ApiTags('notes') @ApiSecurity('token') @@ -42,6 +45,7 @@ export class NotesController { private readonly logger: ConsoleLoggerService, private noteService: NotesService, private revisionsService: RevisionsService, + private permissionsService: PermissionsService, private historyService: HistoryService, ) { this.logger.setContext(NotesController.name); @@ -54,7 +58,10 @@ export class NotesController { @MarkdownBody() text: string, ): Promise { // ToDo: provide user for createNoteDto - this.logger.debug('Got raw markdown:\n' + text, 'createNote'); + if (!this.permissionsService.mayCreate(req.user)) { + throw new UnauthorizedException('Creating note denied!'); + } + this.logger.debug('Got raw markdown:\n' + text); return this.noteService.toNoteDto( await this.noteService.createNote(text, undefined, req.user), ); @@ -62,18 +69,24 @@ export class NotesController { @UseGuards(TokenAuthGuard) @Get(':noteIdOrAlias') - async getNote(@Request() req, @Param('noteIdOrAlias') noteIdOrAlias: string) { - // ToDo: check if user is allowed to view this note + async getNote( + @Request() req, + @Param('noteIdOrAlias') noteIdOrAlias: string, + ): Promise { + let note: Note; try { - const note = await this.noteService.getNoteByIdOrAlias(noteIdOrAlias); - await this.historyService.createOrUpdateHistoryEntry(note, req.user); - return this.noteService.toNoteDto(note); + note = await this.noteService.getNoteByIdOrAlias(noteIdOrAlias); } catch (e) { if (e instanceof NotInDBError) { throw new NotFoundException(e.message); } throw e; } + if (!this.permissionsService.mayRead(req.user, note)) { + throw new UnauthorizedException('Reading note denied!'); + } + await this.historyService.createOrUpdateHistoryEntry(note, req.user); + return this.noteService.toNoteDto(note); } @UseGuards(TokenAuthGuard) @@ -83,7 +96,9 @@ export class NotesController { @Param('noteAlias') noteAlias: string, @MarkdownBody() text: string, ): Promise { - // ToDo: check if user is allowed to view this note + if (!this.permissionsService.mayCreate(req.user)) { + throw new UnauthorizedException('Creating note denied!'); + } this.logger.debug('Got raw markdown:\n' + text, 'createNamedNote'); return this.noteService.toNoteDto( await this.noteService.createNote(text, noteAlias, req.user), @@ -96,7 +111,10 @@ export class NotesController { @Request() req, @Param('noteIdOrAlias') noteIdOrAlias: string, ): Promise { - // ToDo: check if user is allowed to delete this note + const note = await this.noteService.getNoteByIdOrAlias(noteIdOrAlias); + if (!this.permissionsService.isOwner(req.user, note)) { + throw new UnauthorizedException('Deleting note denied!'); + } this.logger.debug('Deleting note: ' + noteIdOrAlias, 'deleteNote'); try { await this.noteService.deleteNoteByIdOrAlias(noteIdOrAlias); @@ -117,7 +135,10 @@ export class NotesController { @Param('noteIdOrAlias') noteIdOrAlias: string, @MarkdownBody() text: string, ): Promise { - // ToDo: check if user is allowed to change this note + const note = await this.noteService.getNoteByIdOrAlias(noteIdOrAlias); + if (!this.permissionsService.mayWrite(req.user, note)) { + throw new UnauthorizedException('Updating note denied!'); + } this.logger.debug('Got raw markdown:\n' + text, 'updateNote'); try { return this.noteService.toNoteDto( @@ -138,7 +159,10 @@ export class NotesController { @Request() req, @Param('noteIdOrAlias') noteIdOrAlias: string, ): Promise { - // ToDo: check if user is allowed to view this notes content + const note = await this.noteService.getNoteByIdOrAlias(noteIdOrAlias); + if (!this.permissionsService.mayRead(req.user, note)) { + throw new UnauthorizedException('Reading note denied!'); + } try { return await this.noteService.getNoteContent(noteIdOrAlias); } catch (e) { @@ -155,7 +179,10 @@ export class NotesController { @Request() req, @Param('noteIdOrAlias') noteIdOrAlias: string, ): Promise { - // ToDo: check if user is allowed to view this notes metadata + const note = await this.noteService.getNoteByIdOrAlias(noteIdOrAlias); + if (!this.permissionsService.mayRead(req.user, note)) { + throw new UnauthorizedException('Reading note denied!'); + } try { return this.noteService.toNoteMetadataDto( await this.noteService.getNoteByIdOrAlias(noteIdOrAlias), @@ -175,7 +202,10 @@ export class NotesController { @Param('noteIdOrAlias') noteIdOrAlias: string, @Body() updateDto: NotePermissionsUpdateDto, ): Promise { - // ToDo: check if user is allowed to view this notes permissions + const note = await this.noteService.getNoteByIdOrAlias(noteIdOrAlias); + if (!this.permissionsService.isOwner(req.user, note)) { + throw new UnauthorizedException('Updating note denied!'); + } try { return this.noteService.toNotePermissionsDto( await this.noteService.updateNotePermissions(noteIdOrAlias, updateDto), @@ -194,7 +224,10 @@ export class NotesController { @Request() req, @Param('noteIdOrAlias') noteIdOrAlias: string, ): Promise { - // ToDo: check if user is allowed to view this notes revisions + const note = await this.noteService.getNoteByIdOrAlias(noteIdOrAlias); + if (!this.permissionsService.mayRead(req.user, note)) { + throw new UnauthorizedException('Reading note denied!'); + } try { const revisions = await this.revisionsService.getAllRevisions( noteIdOrAlias, @@ -219,7 +252,10 @@ export class NotesController { @Param('noteIdOrAlias') noteIdOrAlias: string, @Param('revisionId') revisionId: number, ): Promise { - // ToDo: check if user is allowed to view this notes revision + const note = await this.noteService.getNoteByIdOrAlias(noteIdOrAlias); + if (!this.permissionsService.mayRead(req.user, note)) { + throw new UnauthorizedException('Reading note denied!'); + } try { return this.revisionsService.toRevisionDto( await this.revisionsService.getRevision(noteIdOrAlias, revisionId), diff --git a/src/api/public/public-api.module.ts b/src/api/public/public-api.module.ts index 75e1620a3..c8e9284d3 100644 --- a/src/api/public/public-api.module.ts +++ b/src/api/public/public-api.module.ts @@ -16,6 +16,7 @@ import { MeController } from './me/me.controller'; import { NotesController } from './notes/notes.controller'; import { MediaController } from './media/media.controller'; import { MonitoringController } from './monitoring/monitoring.controller'; +import { PermissionsModule } from '../../permissions/permissions.module'; @Module({ imports: [ @@ -26,6 +27,7 @@ import { MonitoringController } from './monitoring/monitoring.controller'; MonitoringModule, LoggerModule, MediaModule, + PermissionsModule, ], controllers: [ MeController, From 0ea7991e3619e2cf27b9fdf81fe7a22e63c31a9d Mon Sep 17 00:00:00 2001 From: Yannick Bungers Date: Sat, 13 Feb 2021 14:00:29 +0100 Subject: [PATCH 4/7] Add guest permission mock and checking mocked by attribute of permission service Signed-off-by: Yannick Bungers --- src/permissions/permissions.service.ts | 37 +++++++++++++++++++++++--- 1 file changed, 33 insertions(+), 4 deletions(-) diff --git a/src/permissions/permissions.service.ts b/src/permissions/permissions.service.ts index 8c5edf585..af3e20f08 100644 --- a/src/permissions/permissions.service.ts +++ b/src/permissions/permissions.service.ts @@ -7,16 +7,25 @@ import { Injectable } from '@nestjs/common'; import { User } from '../users/user.entity'; import { Note } from '../notes/note.entity'; -import { ConsoleLoggerService } from '../logger/console-logger.service'; + +// TODO move to config or remove +export enum GuestPermission { + DENY = 'deny', + READ = 'read', + WRITE = 'write', + CREATE = 'create', + CREATE_ALIAS = 'createAlias', +} @Injectable() export class PermissionsService { - constructor(private readonly logger: ConsoleLoggerService) {} + public guestPermission: GuestPermission; // TODO change to configOption mayRead(user: User, note: Note): boolean { if (this.isOwner(user, note)) return true; if (this.hasPermissionUser(user, note, false)) return true; + // noinspection RedundantIfStatementJS if (this.hasPermissionGroup(user, note, false)) return true; return false; @@ -27,19 +36,30 @@ export class PermissionsService { if (this.hasPermissionUser(user, note, true)) return true; + // noinspection RedundantIfStatementJS if (this.hasPermissionGroup(user, note, true)) return true; return false; } + mayCreate(user: User): boolean { if (user) { - // TODO: (config.guestPermission == "create") return true; + } else { + if ( + this.guestPermission == GuestPermission.CREATE || + this.guestPermission == GuestPermission.CREATE_ALIAS + ) { + // TODO change to guestPermission to config option + return true; + } } return false; } + isOwner(user: User, note: Note): boolean { if (!user) return false; + if (!note.owner) return false; return note.owner.id === user.id; } @@ -68,7 +88,16 @@ export class PermissionsService { wantEdit: boolean, ): boolean { // TODO: Get real config value - const guestsAllowed = false; // (config.guestPermission == "write" || config.guestPermission == "read" && !wantEdit) + let guestsAllowed = false; + switch (this.guestPermission) { + case GuestPermission.CREATE_ALIAS: + case GuestPermission.CREATE: + case GuestPermission.WRITE: + guestsAllowed = true; + break; + case GuestPermission.READ: + guestsAllowed = !wantEdit; + } for (const groupPermission of note.groupPermissions) { if (groupPermission.canEdit || !wantEdit) { // Handle special groups From 0fc9c11a41c9950a46a77578bc00a3460e66d5dd Mon Sep 17 00:00:00 2001 From: Yannick Bungers Date: Sat, 13 Feb 2021 14:04:16 +0100 Subject: [PATCH 5/7] Add test for permission service Many tests are generated and not static like in other files. Signed-off-by: Yannick Bungers --- src/permissions/permissions.service.spec.ts | 526 ++++++++++++++++++++ 1 file changed, 526 insertions(+) create mode 100644 src/permissions/permissions.service.spec.ts diff --git a/src/permissions/permissions.service.spec.ts b/src/permissions/permissions.service.spec.ts new file mode 100644 index 000000000..b5b4eb56e --- /dev/null +++ b/src/permissions/permissions.service.spec.ts @@ -0,0 +1,526 @@ +/* + * SPDX-FileCopyrightText: 2021 The HedgeDoc developers (see AUTHORS file) + * + * SPDX-License-Identifier: AGPL-3.0-only + */ + +import { Test, TestingModule } from '@nestjs/testing'; +import { LoggerModule } from '../logger/logger.module'; +import { GuestPermission, PermissionsService } from './permissions.service'; +import { User } from '../users/user.entity'; +import { Note } from '../notes/note.entity'; +import { UsersModule } from '../users/users.module'; +import { NotesModule } from '../notes/notes.module'; +import { PermissionsModule } from './permissions.module'; +import { getRepositoryToken } from '@nestjs/typeorm'; +import { NoteGroupPermission } from './note-group-permission.entity'; +import { NoteUserPermission } from './note-user-permission.entity'; +import { Identity } from '../users/identity.entity'; +import { AuthToken } from '../auth/auth-token.entity'; +import { Authorship } from '../revisions/authorship.entity'; +import { AuthorColor } from '../notes/author-color.entity'; +import { Revision } from '../revisions/revision.entity'; +import { Tag } from '../notes/tag.entity'; +import { Group } from '../groups/group.entity'; + +jest.mock('../permissions/note-group-permission.entity.ts'); +jest.mock('../groups/group.entity.ts'); +jest.mock('../notes/note.entity.ts'); +jest.mock('../users/user.entity.ts'); + +describe('PermissionsService', () => { + let permissionsService: PermissionsService; + + beforeAll(async () => { + const module: TestingModule = await Test.createTestingModule({ + providers: [PermissionsService], + imports: [PermissionsModule, UsersModule, LoggerModule, NotesModule], + }) + .overrideProvider(getRepositoryToken(User)) + .useValue({}) + .overrideProvider(getRepositoryToken(AuthToken)) + .useValue({}) + .overrideProvider(getRepositoryToken(Identity)) + .useValue({}) + .overrideProvider(getRepositoryToken(Authorship)) + .useValue({}) + .overrideProvider(getRepositoryToken(AuthorColor)) + .useValue({}) + .overrideProvider(getRepositoryToken(Revision)) + .useValue({}) + .overrideProvider(getRepositoryToken(Note)) + .useValue({}) + .overrideProvider(getRepositoryToken(Tag)) + .useValue({}) + .overrideProvider(getRepositoryToken(NoteGroupPermission)) + .useValue({}) + .overrideProvider(getRepositoryToken(NoteUserPermission)) + .useValue({}) + .compile(); + permissionsService = module.get(PermissionsService); + }); + + // The two users we test with: + const user2 = {} as User; + user2.id = '2'; + const user1 = {} as User; + user1.id = '1'; + + it('should be defined', () => { + expect(permissionsService).toBeDefined(); + }); + + function createNote(owner: User): Note { + const note = {} as Note; + note.userPermissions = []; + note.groupPermissions = []; + note.owner = owner; + return note; + } + + /* + * Creates the permission objects for UserPermission for two users with write and with out write permission + */ + function createNoteUserPermissionNotes(): Note[] { + const note0 = createNote(user1); + const note1 = createNote(user2); + const note2 = createNote(user2); + const note3 = createNote(user2); + const note4 = createNote(user2); + const note5 = createNote(user2); + const note6 = createNote(user2); + const note7 = createNote(user2); + const noteUserPermission1 = {} as NoteUserPermission; + noteUserPermission1.user = user1; + const noteUserPermission2 = {} as NoteUserPermission; + noteUserPermission2.user = user2; + const noteUserPermission3 = {} as NoteUserPermission; + noteUserPermission3.user = user1; + noteUserPermission3.canEdit = true; + const noteUserPermission4 = {} as NoteUserPermission; + noteUserPermission4.user = user2; + noteUserPermission4.canEdit = true; + + note1.userPermissions.push(noteUserPermission1); + + note2.userPermissions.push(noteUserPermission1); + note2.userPermissions.push(noteUserPermission2); + + note3.userPermissions.push(noteUserPermission2); + note3.userPermissions.push(noteUserPermission1); + + note4.userPermissions.push(noteUserPermission3); + + note5.userPermissions.push(noteUserPermission3); + note5.userPermissions.push(noteUserPermission4); + + note6.userPermissions.push(noteUserPermission4); + note6.userPermissions.push(noteUserPermission3); + + note7.userPermissions.push(noteUserPermission2); + + const everybody = {} as Group; + everybody.name = 'everybody'; + everybody.special = true; + const noteEverybodyRead = createNote(user1); + + const noteGroupPermissionRead = {} as NoteGroupPermission; + noteGroupPermissionRead.group = everybody; + noteGroupPermissionRead.canEdit = false; + noteGroupPermissionRead.note = noteEverybodyRead; + noteEverybodyRead.groupPermissions = [noteGroupPermissionRead]; + + const noteEverybodyWrite = createNote(user1); + + const noteGroupPermissionWrite = {} as NoteGroupPermission; + noteGroupPermissionWrite.group = everybody; + noteGroupPermissionWrite.canEdit = true; + noteGroupPermissionWrite.note = noteEverybodyWrite; + noteEverybodyWrite.groupPermissions = [noteGroupPermissionWrite]; + + return [ + note0, + note1, + note2, + note3, + note4, + note5, + note6, + note7, + noteEverybodyRead, + noteEverybodyWrite, + ]; + } + const notes = createNoteUserPermissionNotes(); + + describe('mayRead works with', () => { + it('Owner', () => { + permissionsService.guestPermission = GuestPermission.DENY; + expect(permissionsService.mayRead(user1, notes[0])).toBeTruthy(); + expect(permissionsService.mayRead(user1, notes[7])).toBeFalsy(); + }); + it('userPermission read', () => { + permissionsService.guestPermission = GuestPermission.DENY; + expect(permissionsService.mayRead(user1, notes[1])).toBeTruthy(); + expect(permissionsService.mayRead(user1, notes[2])).toBeTruthy(); + expect(permissionsService.mayRead(user1, notes[3])).toBeTruthy(); + }); + it('userPermission write', () => { + permissionsService.guestPermission = GuestPermission.DENY; + expect(permissionsService.mayRead(user1, notes[4])).toBeTruthy(); + expect(permissionsService.mayRead(user1, notes[5])).toBeTruthy(); + expect(permissionsService.mayRead(user1, notes[6])).toBeTruthy(); + expect(permissionsService.mayRead(user1, notes[7])).toBeFalsy(); + }); + + describe('guest permission', () => { + it('CREATE_ALIAS', () => { + permissionsService.guestPermission = GuestPermission.CREATE_ALIAS; + expect(permissionsService.mayRead(null, notes[8])).toBeTruthy(); + }); + it('CREATE', () => { + permissionsService.guestPermission = GuestPermission.CREATE; + expect(permissionsService.mayRead(null, notes[8])).toBeTruthy(); + }); + it('WRITE', () => { + permissionsService.guestPermission = GuestPermission.WRITE; + expect(permissionsService.mayRead(null, notes[8])).toBeTruthy(); + }); + it('READ', () => { + permissionsService.guestPermission = GuestPermission.READ; + expect(permissionsService.mayRead(null, notes[8])).toBeTruthy(); + }); + }); + }); + describe('mayWrite works with', () => { + it('Owner', () => { + permissionsService.guestPermission = GuestPermission.DENY; + expect(permissionsService.mayWrite(user1, notes[0])).toBeTruthy(); + expect(permissionsService.mayWrite(user1, notes[7])).toBeFalsy(); + }); + it('userPermission read', () => { + permissionsService.guestPermission = GuestPermission.DENY; + expect(permissionsService.mayWrite(user1, notes[1])).toBeFalsy(); + expect(permissionsService.mayWrite(user1, notes[2])).toBeFalsy(); + expect(permissionsService.mayWrite(user1, notes[3])).toBeFalsy(); + }); + it('userPermission write', () => { + permissionsService.guestPermission = GuestPermission.DENY; + expect(permissionsService.mayWrite(user1, notes[4])).toBeTruthy(); + expect(permissionsService.mayWrite(user1, notes[5])).toBeTruthy(); + expect(permissionsService.mayWrite(user1, notes[6])).toBeTruthy(); + expect(permissionsService.mayWrite(user1, notes[7])).toBeFalsy(); + }); + describe('guest permission', () => { + it('CREATE_ALIAS', () => { + permissionsService.guestPermission = GuestPermission.CREATE_ALIAS; + expect(permissionsService.mayWrite(null, notes[9])).toBeTruthy(); + }); + it('CREATE', () => { + permissionsService.guestPermission = GuestPermission.CREATE; + expect(permissionsService.mayWrite(null, notes[9])).toBeTruthy(); + }); + it('WRITE', () => { + permissionsService.guestPermission = GuestPermission.WRITE; + expect(permissionsService.mayWrite(null, notes[9])).toBeTruthy(); + }); + it('READ', () => { + permissionsService.guestPermission = GuestPermission.READ; + expect(permissionsService.mayWrite(null, notes[9])).toBeFalsy(); + }); + }); + }); + + /* + * Helper Object that arranges a list of GroupPermissions and if they allow a user to read or write a particular note. + */ + class NoteGroupPermissionWithResultForUser { + permissions: NoteGroupPermission[]; + allowsRead: boolean; + allowsWrite: boolean; + } + + /* + * Setup function to create all the groups we use in the tests. + */ + function createGroups(): { [id: string]: Group } { + const result: { [id: string]: Group } = {}; + + const everybody: Group = new Group(); + everybody.special = true; + everybody.name = 'everybody'; + result['everybody'] = everybody; + + const loggedIn = new Group(); + loggedIn.special = true; + loggedIn.name = 'loggedIn'; + result['loggedIn'] = loggedIn; + + const user1group = new Group(); + user1group.name = 'user1group'; + user1group.members = [user1]; + result['user1group'] = user1group; + + const user2group = new Group(); + user2group.name = 'user2group'; + user2group.members = [user2]; + result['user2group'] = user2group; + + const user1and2group = new Group(); + user1and2group.name = 'user1and2group'; + user1and2group.members = [user1, user2]; + result['user1and2group'] = user1and2group; + + const user2and1group = new Group(); + user2and1group.name = 'user2and1group'; + user2and1group.members = [user2, user1]; + result['user2and1group'] = user2and1group; + + return result; + } + + /* + * Create all GroupPermissions: For each group two GroupPermissions are created one with read permission and one with write permission. + */ + function createAllNoteGroupPermissions(): NoteGroupPermission[][] { + const groups = createGroups(); + + /* + * Helper function for creating GroupPermissions + */ + function createNoteGroupPermission( + group: Group, + write: boolean, + ): NoteGroupPermission { + const noteGroupPermission = new NoteGroupPermission(); + noteGroupPermission.canEdit = write; + noteGroupPermission.group = group; + return noteGroupPermission; + } + + const everybodyRead = createNoteGroupPermission(groups['everybody'], false); + const everybodyWrite = createNoteGroupPermission(groups['everybody'], true); + + const loggedInRead = createNoteGroupPermission(groups['loggedIn'], false); + const loggedInWrite = createNoteGroupPermission(groups['loggedIn'], true); + + const user1groupRead = createNoteGroupPermission( + groups['user1group'], + false, + ); + const user1groupWrite = createNoteGroupPermission( + groups['user1group'], + true, + ); + + const user2groupRead = createNoteGroupPermission( + groups['user2group'], + false, + ); + const user2groupWrite = createNoteGroupPermission( + groups['user2group'], + true, + ); + + const user1and2groupRead = createNoteGroupPermission( + groups['user1and2group'], + false, + ); + const user1and2groupWrite = createNoteGroupPermission( + groups['user1and2group'], + true, + ); + + const user2and1groupRead = createNoteGroupPermission( + groups['user2and1group'], + false, + ); + const user2and1groupWrite = createNoteGroupPermission( + groups['user2and1group'], + true, + ); + + return [ + [user1groupRead, user1and2groupRead, user2and1groupRead, null], // group0: allow user1 to read via group + [user2and1groupWrite, user1and2groupWrite, user1groupWrite, null], // group1: allow user1 to write via group + [everybodyRead, everybodyWrite, null], // group2: permissions of the special group everybody + [loggedInRead, loggedInWrite, null], // group3: permissions of the special group loggedIn + [user2groupWrite, user2groupRead, null], // group4: don't allow user1 to read or write via group + ]; + } + /* + * creates the matrix multiplication of group0 to group4 of createAllNoteGroupPermissions + */ + function createNoteGroupPermissionsCombinations( + guestPermission: GuestPermission, + ): NoteGroupPermissionWithResultForUser[] { + // for logged in users + const noteGroupPermissions = createAllNoteGroupPermissions(); + const result: NoteGroupPermissionWithResultForUser[] = []; + for (const group0 of noteGroupPermissions[0]) { + for (const group1 of noteGroupPermissions[1]) { + for (const group2 of noteGroupPermissions[2]) { + for (const group3 of noteGroupPermissions[3]) { + for (const group4 of noteGroupPermissions[4]) { + const insert = []; + let readPermission = false; + let writePermission = false; + if (group0 !== null) { + // user1 in ReadGroups + readPermission = true; + insert.push(group0); + } + if (group1 !== null) { + // user1 in WriteGroups + readPermission = true; + writePermission = true; + insert.push(group1); + } + + if (group2 !== null) { + // everybody group TODO config options + switch (guestPermission) { + case GuestPermission.CREATE_ALIAS: + case GuestPermission.CREATE: + case GuestPermission.WRITE: + writePermission = writePermission || group2.canEdit; + readPermission = true; + break; + case GuestPermission.READ: + readPermission = true; + } + insert.push(group2); + } + if (group3 !== null) { + // loggedIn users + readPermission = true; + writePermission = writePermission || group3.canEdit; + insert.push(group3); + } + if (group4 !== null) { + // user not in group + insert.push(group4); + } + result.push({ + permissions: insert, + allowsRead: readPermission, + allowsWrite: writePermission, + }); + } + } + } + } + } + return result; + } + + // inspired by https://stackoverflow.com/questions/9960908/permutations-in-javascript + function permutator( + inputArr: NoteGroupPermission[], + ): NoteGroupPermission[][] { + const results = []; + + function permute(arr, memo) { + let cur; + + for (let i = 0; i < arr.length; i++) { + cur = arr.splice(i, 1); + if (arr.length === 0) { + results.push(memo.concat(cur)); + } + permute(arr.slice(), memo.concat(cur)); + arr.splice(i, 0, cur[0]); + } + + return results; + } + + return permute(inputArr, []); + } + + // takes each set of permissions from createNoteGroupPermissionsCombinations, permute them and add them to the list + function permuteNoteGroupPermissions( + noteGroupPermissions: NoteGroupPermissionWithResultForUser[], + ): NoteGroupPermissionWithResultForUser[] { + const result: NoteGroupPermissionWithResultForUser[] = []; + for (const permission of noteGroupPermissions) { + const permutations = permutator(permission.permissions); + for (const permutation of permutations) { + result.push({ + permissions: permutation, + allowsRead: permission.allowsRead, + allowsWrite: permission.allowsWrite, + }); + } + } + return result; + } + + describe('check if groups work with', () => { + const guestPermission = GuestPermission.WRITE; + const rawPermissions = createNoteGroupPermissionsCombinations( + guestPermission, + ); + const permissions = permuteNoteGroupPermissions(rawPermissions); + let i = 0; + for (const permission of permissions) { + const note = createNote(user2); + note.groupPermissions = permission.permissions; + let permissionString = ''; + for (const perm of permission.permissions) { + permissionString += ' ' + perm.group.name + ':' + perm.canEdit; + } + it('mayWrite - test #' + i + ':' + permissionString, () => { + permissionsService.guestPermission = guestPermission; + expect(permissionsService.mayWrite(user1, note)).toEqual( + permission.allowsWrite, + ); + }); + it('mayRead - test #' + i + ':' + permissionString, () => { + permissionsService.guestPermission = guestPermission; + expect(permissionsService.mayRead(user1, note)).toEqual( + permission.allowsRead, + ); + }); + i++; + } + }); + + describe('mayCreate works for', () => { + it('logged in', () => { + permissionsService.guestPermission = GuestPermission.DENY; + expect(permissionsService.mayCreate(user1)).toBeTruthy(); + }); + it('guest denied', () => { + permissionsService.guestPermission = GuestPermission.DENY; + expect(permissionsService.mayCreate(null)).toBeFalsy(); + }); + it('guest read', () => { + permissionsService.guestPermission = GuestPermission.READ; + expect(permissionsService.mayCreate(null)).toBeFalsy(); + }); + it('guest write', () => { + permissionsService.guestPermission = GuestPermission.WRITE; + expect(permissionsService.mayCreate(null)).toBeFalsy(); + }); + it('guest create', () => { + permissionsService.guestPermission = GuestPermission.CREATE; + expect(permissionsService.mayCreate(null)).toBeTruthy(); + }); + it('guest create alias', () => { + permissionsService.guestPermission = GuestPermission.CREATE_ALIAS; + expect(permissionsService.mayCreate(null)).toBeTruthy(); + }); + }); + + describe('isOwner works', () => { + it('for positive case', () => { + permissionsService.guestPermission = GuestPermission.DENY; + expect(permissionsService.isOwner(user1, notes[0])).toBeTruthy(); + }); + it('for negative case', () => { + permissionsService.guestPermission = GuestPermission.DENY; + expect(permissionsService.isOwner(user1, notes[1])).toBeFalsy(); + }); + }); +}); From 1aa821460f34861753665c1395e581cbe6de66e1 Mon Sep 17 00:00:00 2001 From: Philip Molares Date: Wed, 17 Feb 2021 13:15:26 +0100 Subject: [PATCH 6/7] NotesController: Catch NotInDBErrors from permission checks The permission check also tries to get the note and a non existing note needs to be handled there too. Signed-off-by: Philip Molares --- src/api/public/notes/notes.controller.ts | 64 ++++++++++++------------ 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/src/api/public/notes/notes.controller.ts b/src/api/public/notes/notes.controller.ts index 68a98c8dd..4bca6da81 100644 --- a/src/api/public/notes/notes.controller.ts +++ b/src/api/public/notes/notes.controller.ts @@ -111,21 +111,21 @@ export class NotesController { @Request() req, @Param('noteIdOrAlias') noteIdOrAlias: string, ): Promise { - const note = await this.noteService.getNoteByIdOrAlias(noteIdOrAlias); - if (!this.permissionsService.isOwner(req.user, note)) { - throw new UnauthorizedException('Deleting note denied!'); - } - this.logger.debug('Deleting note: ' + noteIdOrAlias, 'deleteNote'); try { + const note = await this.noteService.getNoteByIdOrAlias(noteIdOrAlias); + if (!this.permissionsService.isOwner(req.user, note)) { + throw new UnauthorizedException('Deleting note denied!'); + } + this.logger.debug('Deleting note: ' + noteIdOrAlias, 'deleteNote'); await this.noteService.deleteNoteByIdOrAlias(noteIdOrAlias); + this.logger.debug('Successfully deleted ' + noteIdOrAlias, 'deleteNote'); + return; } catch (e) { if (e instanceof NotInDBError) { throw new NotFoundException(e.message); } throw e; } - this.logger.debug('Successfully deleted ' + noteIdOrAlias, 'deleteNote'); - return; } @UseGuards(TokenAuthGuard) @@ -135,12 +135,12 @@ export class NotesController { @Param('noteIdOrAlias') noteIdOrAlias: string, @MarkdownBody() text: string, ): Promise { - const note = await this.noteService.getNoteByIdOrAlias(noteIdOrAlias); - if (!this.permissionsService.mayWrite(req.user, note)) { - throw new UnauthorizedException('Updating note denied!'); - } - this.logger.debug('Got raw markdown:\n' + text, 'updateNote'); try { + const note = await this.noteService.getNoteByIdOrAlias(noteIdOrAlias); + if (!this.permissionsService.mayWrite(req.user, note)) { + throw new UnauthorizedException('Updating note denied!'); + } + this.logger.debug('Got raw markdown:\n' + text, 'updateNote'); return this.noteService.toNoteDto( await this.noteService.updateNoteByIdOrAlias(noteIdOrAlias, text), ); @@ -159,11 +159,11 @@ export class NotesController { @Request() req, @Param('noteIdOrAlias') noteIdOrAlias: string, ): Promise { - const note = await this.noteService.getNoteByIdOrAlias(noteIdOrAlias); - if (!this.permissionsService.mayRead(req.user, note)) { - throw new UnauthorizedException('Reading note denied!'); - } try { + const note = await this.noteService.getNoteByIdOrAlias(noteIdOrAlias); + if (!this.permissionsService.mayRead(req.user, note)) { + throw new UnauthorizedException('Reading note denied!'); + } return await this.noteService.getNoteContent(noteIdOrAlias); } catch (e) { if (e instanceof NotInDBError) { @@ -179,11 +179,11 @@ export class NotesController { @Request() req, @Param('noteIdOrAlias') noteIdOrAlias: string, ): Promise { - const note = await this.noteService.getNoteByIdOrAlias(noteIdOrAlias); - if (!this.permissionsService.mayRead(req.user, note)) { - throw new UnauthorizedException('Reading note denied!'); - } try { + const note = await this.noteService.getNoteByIdOrAlias(noteIdOrAlias); + if (!this.permissionsService.mayRead(req.user, note)) { + throw new UnauthorizedException('Reading note denied!'); + } return this.noteService.toNoteMetadataDto( await this.noteService.getNoteByIdOrAlias(noteIdOrAlias), ); @@ -202,11 +202,11 @@ export class NotesController { @Param('noteIdOrAlias') noteIdOrAlias: string, @Body() updateDto: NotePermissionsUpdateDto, ): Promise { - const note = await this.noteService.getNoteByIdOrAlias(noteIdOrAlias); - if (!this.permissionsService.isOwner(req.user, note)) { - throw new UnauthorizedException('Updating note denied!'); - } try { + const note = await this.noteService.getNoteByIdOrAlias(noteIdOrAlias); + if (!this.permissionsService.isOwner(req.user, note)) { + throw new UnauthorizedException('Updating note denied!'); + } return this.noteService.toNotePermissionsDto( await this.noteService.updateNotePermissions(noteIdOrAlias, updateDto), ); @@ -224,11 +224,11 @@ export class NotesController { @Request() req, @Param('noteIdOrAlias') noteIdOrAlias: string, ): Promise { - const note = await this.noteService.getNoteByIdOrAlias(noteIdOrAlias); - if (!this.permissionsService.mayRead(req.user, note)) { - throw new UnauthorizedException('Reading note denied!'); - } try { + const note = await this.noteService.getNoteByIdOrAlias(noteIdOrAlias); + if (!this.permissionsService.mayRead(req.user, note)) { + throw new UnauthorizedException('Reading note denied!'); + } const revisions = await this.revisionsService.getAllRevisions( noteIdOrAlias, ); @@ -252,11 +252,11 @@ export class NotesController { @Param('noteIdOrAlias') noteIdOrAlias: string, @Param('revisionId') revisionId: number, ): Promise { - const note = await this.noteService.getNoteByIdOrAlias(noteIdOrAlias); - if (!this.permissionsService.mayRead(req.user, note)) { - throw new UnauthorizedException('Reading note denied!'); - } try { + const note = await this.noteService.getNoteByIdOrAlias(noteIdOrAlias); + if (!this.permissionsService.mayRead(req.user, note)) { + throw new UnauthorizedException('Reading note denied!'); + } return this.revisionsService.toRevisionDto( await this.revisionsService.getRevision(noteIdOrAlias, revisionId), ); From 8e9717c92ace839be25a7de4da24ea7166ad78c3 Mon Sep 17 00:00:00 2001 From: Philip Molares Date: Wed, 17 Feb 2021 13:20:54 +0100 Subject: [PATCH 7/7] NotesE2E: Fix e2e test split success and fail cases in separate tests for better readability add the correct user to all notes created by service (instead of api) to make the permissions checks viable. extracted test content of notes to a global variable. Signed-off-by: Philip Molares --- test/public-api/notes.e2e-spec.ts | 245 +++++++++++++++++------------- 1 file changed, 136 insertions(+), 109 deletions(-) diff --git a/test/public-api/notes.e2e-spec.ts b/test/public-api/notes.e2e-spec.ts index 5059f3e93..8c2b71c71 100644 --- a/test/public-api/notes.e2e-spec.ts +++ b/test/public-api/notes.e2e-spec.ts @@ -20,11 +20,15 @@ import { PermissionsModule } from '../../src/permissions/permissions.module'; import { AuthModule } from '../../src/auth/auth.module'; import { TokenAuthGuard } from '../../src/auth/token-auth.guard'; import { MockAuthGuard } from '../../src/auth/mock-auth.guard'; +import { UsersService } from '../../src/users/users.service'; +import { User } from '../../src/users/user.entity'; import { UsersModule } from '../../src/users/users.module'; describe('Notes', () => { let app: INestApplication; let notesService: NotesService; + let user: User; + let content: string; beforeAll(async () => { const moduleRef = await Test.createTestingModule({ @@ -56,14 +60,16 @@ describe('Notes', () => { app = moduleRef.createNestApplication(); await app.init(); notesService = moduleRef.get(NotesService); + const userService = moduleRef.get(UsersService); + user = await userService.createUser('hardcoded', 'Testy'); + content = 'This is a test note.'; }); - it(`POST /notes`, async () => { - const newNote = 'This is a test note.'; + it('POST /notes', async () => { const response = await request(app.getHttpServer()) .post('/notes') .set('Content-Type', 'text/markdown') - .send(newNote) + .send(content) .expect('Content-Type', /json/) .expect(201); expect(response.body.metadata?.id).toBeDefined(); @@ -71,88 +77,98 @@ describe('Notes', () => { await notesService.getCurrentContent( await notesService.getNoteByIdOrAlias(response.body.metadata.id), ), - ).toEqual(newNote); + ).toEqual(content); }); - it(`GET /notes/{note}`, async () => { - // check if we can succefully get a note that exists - await notesService.createNote('This is a test note.', 'test1'); - const response = await request(app.getHttpServer()) - .get('/notes/test1') - .expect('Content-Type', /json/) - .expect(200); - expect(response.body.content).toEqual('This is a test note.'); - - // check if a missing note correctly returns 404 - await request(app.getHttpServer()) - .get('/notes/i_dont_exist') - .expect('Content-Type', /json/) - .expect(404); + describe('GET /notes/{note}', () => { + it('works with an existing note', async () => { + // check if we can succefully get a note that exists + await notesService.createNote(content, 'test1', user); + const response = await request(app.getHttpServer()) + .get('/notes/test1') + .expect('Content-Type', /json/) + .expect(200); + expect(response.body.content).toEqual(content); + }); + it('fails with an non-existing note', async () => { + // check if a missing note correctly returns 404 + await request(app.getHttpServer()) + .get('/notes/i_dont_exist') + .expect('Content-Type', /json/) + .expect(404); + }); }); - it(`POST /notes/{note}`, async () => { - const newNote = 'This is a test note.'; - const response = await request(app.getHttpServer()) - .post('/notes/test2') - .set('Content-Type', 'text/markdown') - .send(newNote) - .expect('Content-Type', /json/) - .expect(201); - expect(response.body.metadata?.id).toBeDefined(); - return expect( - await notesService.getCurrentContent( - await notesService.getNoteByIdOrAlias(response.body.metadata?.id), - ), - ).toEqual(newNote); + describe('POST /notes/{note}', () => { + it('works with a non-existing alias', async () => { + const response = await request(app.getHttpServer()) + .post('/notes/test2') + .set('Content-Type', 'text/markdown') + .send(content) + .expect('Content-Type', /json/) + .expect(201); + expect(response.body.metadata?.id).toBeDefined(); + return expect( + await notesService.getCurrentContent( + await notesService.getNoteByIdOrAlias(response.body.metadata?.id), + ), + ).toEqual(content); + }); }); - it(`DELETE /notes/{note}`, async () => { - await notesService.createNote('This is a test note.', 'test3'); - await request(app.getHttpServer()).delete('/notes/test3').expect(200); - await expect(notesService.getNoteByIdOrAlias('test3')).rejects.toEqual( - new NotInDBError("Note with id/alias 'test3' not found."), - ); - // check if a missing note correctly returns 404 - await request(app.getHttpServer()) - .delete('/notes/i_dont_exist') - .expect(404); + describe('DELETE /notes/{note}', () => { + it('works with an existing alias', async () => { + await notesService.createNote(content, 'test3', user); + await request(app.getHttpServer()).delete('/notes/test3').expect(200); + await expect(notesService.getNoteByIdOrAlias('test3')).rejects.toEqual( + new NotInDBError("Note with id/alias 'test3' not found."), + ); + }); + it('fails with a non-existing alias', async () => { + await request(app.getHttpServer()) + .delete('/notes/i_dont_exist') + .expect(404); + }); }); - it(`PUT /notes/{note}`, async () => { - await notesService.createNote('This is a test note.', 'test4'); - const response = await request(app.getHttpServer()) - .put('/notes/test4') - .set('Content-Type', 'text/markdown') - .send('New note text') - .expect(200); - await expect( - await notesService.getCurrentContent( - await notesService.getNoteByIdOrAlias('test4'), - ), - ).toEqual('New note text'); - expect(response.body.content).toEqual('New note text'); - - // check if a missing note correctly returns 404 - await request(app.getHttpServer()) - .put('/notes/i_dont_exist') - .set('Content-Type', 'text/markdown') - .expect('Content-Type', /json/) - .expect(404); + describe('PUT /notes/{note}', () => { + const changedContent = 'New note text'; + it('works with existing alias', async () => { + await notesService.createNote(content, 'test4', user); + const response = await request(app.getHttpServer()) + .put('/notes/test4') + .set('Content-Type', 'text/markdown') + .send(changedContent) + .expect(200); + await expect( + await notesService.getCurrentContent( + await notesService.getNoteByIdOrAlias('test4'), + ), + ).toEqual(changedContent); + expect(response.body.content).toEqual(changedContent); + }); + it('fails with a non-existing alias', async () => { + await request(app.getHttpServer()) + .put('/notes/i_dont_exist') + .set('Content-Type', 'text/markdown') + .expect('Content-Type', /json/) + .expect(404); + }); }); describe('GET /notes/{note}/metadata', () => { - it(`returns complete metadata object`, async () => { - await notesService.createNote('This is a test note.', 'test6'); + it('returns complete metadata object', async () => { + await notesService.createNote(content, 'test5', user); const metadata = await request(app.getHttpServer()) - .get('/notes/test6/metadata') + .get('/notes/test5/metadata') .expect(200); expect(typeof metadata.body.id).toEqual('string'); - expect(metadata.body.alias).toEqual('test6'); + expect(metadata.body.alias).toEqual('test5'); expect(metadata.body.title).toBeNull(); expect(metadata.body.description).toBeNull(); expect(typeof metadata.body.createTime).toEqual('string'); expect(metadata.body.editedBy).toEqual([]); - expect(metadata.body.permissions.owner).toBeNull(); + expect(metadata.body.permissions.owner.userName).toEqual('hardcoded'); expect(metadata.body.permissions.sharedToUsers).toEqual([]); expect(metadata.body.permissions.sharedToUsers).toEqual([]); expect(metadata.body.tags).toEqual([]); @@ -163,7 +179,9 @@ describe('Notes', () => { expect(typeof metadata.body.updateUser.photo).toEqual('string'); expect(typeof metadata.body.viewCount).toEqual('number'); expect(metadata.body.editedBy).toEqual([]); + }); + it('fails with non-existing alias', async () => { // check if a missing note correctly returns 404 await request(app.getHttpServer()) .get('/notes/i_dont_exist/metadata') @@ -173,66 +191,75 @@ describe('Notes', () => { it('has the correct update/create dates', async () => { // create a note - const note = await notesService.createNote( - 'This is a test note.', - 'test6a', - ); + const note = await notesService.createNote(content, 'test5a', user); // save the creation time const createDate = (await note.revisions)[0].createdAt; // wait one second await new Promise((r) => setTimeout(r, 1000)); // update the note - await notesService.updateNoteByIdOrAlias('test6a', 'More test content'); + await notesService.updateNoteByIdOrAlias('test5a', 'More test content'); const metadata = await request(app.getHttpServer()) - .get('/notes/test6a/metadata') + .get('/notes/test5a/metadata') .expect(200); expect(metadata.body.createTime).toEqual(createDate.toISOString()); expect(metadata.body.updateTime).not.toEqual(createDate.toISOString()); }); }); - it(`GET /notes/{note}/revisions`, async () => { - await notesService.createNote('This is a test note.', 'test7'); - const response = await request(app.getHttpServer()) - .get('/notes/test7/revisions') - .expect('Content-Type', /json/) - .expect(200); - expect(response.body).toHaveLength(1); + describe('GET /notes/{note}/revisions', () => { + it('works with existing alias', async () => { + await notesService.createNote(content, 'test6', user); + const response = await request(app.getHttpServer()) + .get('/notes/test6/revisions') + .expect('Content-Type', /json/) + .expect(200); + expect(response.body).toHaveLength(1); + }); - // check if a missing note correctly returns 404 - await request(app.getHttpServer()) - .get('/notes/i_dont_exist/revisions') - .expect('Content-Type', /json/) - .expect(404); + it('fails with non-existing alias', async () => { + // check if a missing note correctly returns 404 + await request(app.getHttpServer()) + .get('/notes/i_dont_exist/revisions') + .expect('Content-Type', /json/) + .expect(404); + }); }); - it(`GET /notes/{note}/revisions/{revision-id}`, async () => { - const note = await notesService.createNote('This is a test note.', 'test8'); - const revision = await notesService.getLatestRevision(note); - const response = await request(app.getHttpServer()) - .get('/notes/test8/revisions/' + revision.id) - .expect('Content-Type', /json/) - .expect(200); - expect(response.body.content).toEqual('This is a test note.'); - - // check if a missing note correctly returns 404 - await request(app.getHttpServer()) - .get('/notes/i_dont_exist/revisions/1') - .expect('Content-Type', /json/) - .expect(404); + describe('GET /notes/{note}/revisions/{revision-id}', () => { + it('works with an existing alias', async () => { + const note = await notesService.createNote(content, 'test7', user); + const revision = await notesService.getLatestRevision(note); + const response = await request(app.getHttpServer()) + .get('/notes/test7/revisions/' + revision.id) + .expect('Content-Type', /json/) + .expect(200); + expect(response.body.content).toEqual(content); + }); + it('fails with non-existing alias', async () => { + // check if a missing note correctly returns 404 + await request(app.getHttpServer()) + .get('/notes/i_dont_exist/revisions/1') + .expect('Content-Type', /json/) + .expect(404); + }); }); - it(`GET /notes/{note}/content`, async () => { - await notesService.createNote('This is a test note.', 'test9'); - const response = await request(app.getHttpServer()) - .get('/notes/test9/content') - .expect(200); - expect(response.text).toEqual('This is a test note.'); + describe('GET /notes/{note}/content', () => { + it('works with an existing alias', async () => { + await notesService.createNote(content, 'test8', user); + const response = await request(app.getHttpServer()) + .get('/notes/test8/content') + .expect(200); + expect(response.text).toEqual(content); + }); - // check if a missing note correctly returns 404 - await request(app.getHttpServer()) - .get('/notes/i_dont_exist/content') - .expect(404); + it('fails with non-existing alias', async () => { + // check if a missing note correctly returns 404 + await request(app.getHttpServer()) + .get('/notes/i_dont_exist/content') + .expect('Content-Type', /text\/markdown/) + .expect(404); + }); }); afterAll(async () => {