From a852c799472df0ec237626c02c73f55c6dbd8be4 Mon Sep 17 00:00:00 2001 From: Tilman Vatteroth Date: Wed, 10 May 2023 22:50:03 +0200 Subject: [PATCH] refactor: replace permission check methods with ordered permission enum This commit replaces the "mayWrite", "mayRead" and "checkPermissionOnNote" functions with one that returns a sortable permission value. This is done because many places in the code need to do actions based on the fact if the user has no, read or write access. If done with the may-functions then the permission data need to be looked through multiple times. Also, the whole check code is split into more functions that are tested separately and make it easier to understand the process. Signed-off-by: Tilman Vatteroth --- .../permissions/note-permission.enum.spec.ts | 32 + .../src/permissions/note-permission.enum.ts | 34 + backend/src/permissions/permissions.guard.ts | 58 +- .../permissions/permissions.service.spec.ts | 931 ++++++------------ .../src/permissions/permissions.service.ts | 181 ++-- ...rt-guest-access-to-note-permission.spec.ts | 34 + ...convert-guest-access-to-note-permission.ts | 28 + ...d-highest-note-permission-by-group.spec.ts | 194 ++++ .../find-highest-note-permission-by-group.ts | 62 ++ ...nd-highest-note-permission-by-user.spec.ts | 65 ++ .../find-highest-note-permission-by-user.ts | 35 + .../realtime-note.service.spec.ts | 13 +- .../realtime-note/realtime-note.service.ts | 21 +- .../websocket/websocket.gateway.spec.ts | 17 +- .../realtime/websocket/websocket.gateway.ts | 7 +- 15 files changed, 925 insertions(+), 787 deletions(-) create mode 100644 backend/src/permissions/note-permission.enum.spec.ts create mode 100644 backend/src/permissions/note-permission.enum.ts create mode 100644 backend/src/permissions/utils/convert-guest-access-to-note-permission.spec.ts create mode 100644 backend/src/permissions/utils/convert-guest-access-to-note-permission.ts create mode 100644 backend/src/permissions/utils/find-highest-note-permission-by-group.spec.ts create mode 100644 backend/src/permissions/utils/find-highest-note-permission-by-group.ts create mode 100644 backend/src/permissions/utils/find-highest-note-permission-by-user.spec.ts create mode 100644 backend/src/permissions/utils/find-highest-note-permission-by-user.ts diff --git a/backend/src/permissions/note-permission.enum.spec.ts b/backend/src/permissions/note-permission.enum.spec.ts new file mode 100644 index 000000000..303b14832 --- /dev/null +++ b/backend/src/permissions/note-permission.enum.spec.ts @@ -0,0 +1,32 @@ +/* + * SPDX-FileCopyrightText: 2023 The HedgeDoc developers (see AUTHORS file) + * + * SPDX-License-Identifier: AGPL-3.0-only + */ +import { + getNotePermissionDisplayName, + NotePermission, +} from './note-permission.enum'; + +describe('note permission order', () => { + it('DENY is less than READ', () => { + expect(NotePermission.DENY < NotePermission.READ).toBeTruthy(); + }); + it('READ is less than WRITE', () => { + expect(NotePermission.READ < NotePermission.WRITE).toBeTruthy(); + }); + it('WRITE is less than OWNER', () => { + expect(NotePermission.WRITE < NotePermission.OWNER).toBeTruthy(); + }); +}); + +describe('getNotePermissionDisplayName', () => { + it.each([ + ['deny', NotePermission.DENY], + ['read', NotePermission.READ], + ['write', NotePermission.WRITE], + ['owner', NotePermission.OWNER], + ])('displays %s correctly', (displayName, permission) => { + expect(getNotePermissionDisplayName(permission)).toBe(displayName); + }); +}); diff --git a/backend/src/permissions/note-permission.enum.ts b/backend/src/permissions/note-permission.enum.ts new file mode 100644 index 000000000..76ae51307 --- /dev/null +++ b/backend/src/permissions/note-permission.enum.ts @@ -0,0 +1,34 @@ +/* + * SPDX-FileCopyrightText: 2023 The HedgeDoc developers (see AUTHORS file) + * + * SPDX-License-Identifier: AGPL-3.0-only + */ + +/** + * Defines if a user can access a note and if yes how much power they have. + */ +export enum NotePermission { + DENY = 0, + READ = 1, + WRITE = 2, + OWNER = 3, +} + +/** + * Returns the display name for the given {@link NotePermission}. + * + * @param {NotePermission} value the note permission to display + * @return {string} The display name + */ +export function getNotePermissionDisplayName(value: NotePermission): string { + switch (value) { + case NotePermission.DENY: + return 'deny'; + case NotePermission.READ: + return 'read'; + case NotePermission.WRITE: + return 'write'; + case NotePermission.OWNER: + return 'owner'; + } +} diff --git a/backend/src/permissions/permissions.guard.ts b/backend/src/permissions/permissions.guard.ts index a76c41c24..5952ab826 100644 --- a/backend/src/permissions/permissions.guard.ts +++ b/backend/src/permissions/permissions.guard.ts @@ -10,6 +10,7 @@ import { extractNoteFromRequest } from '../api/utils/extract-note-from-request'; import { CompleteRequest } from '../api/utils/request.type'; import { ConsoleLoggerService } from '../logger/console-logger.service'; import { NotesService } from '../notes/notes.service'; +import { NotePermission } from './note-permission.enum'; import { PermissionsService } from './permissions.service'; import { PERMISSION_METADATA_KEY } from './require-permission.decorator'; import { RequiredPermission } from './required-permission.enum'; @@ -32,24 +33,18 @@ export class PermissionsGuard implements CanActivate { } async canActivate(context: ExecutionContext): Promise { - const permission = this.reflector.get( - PERMISSION_METADATA_KEY, - context.getHandler(), - ); - // If no permissions are set this is probably an error and this guard should not let the request pass - if (!permission) { - 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', - ); + const requiredAccessLevel = this.extractRequiredPermission(context); + if (requiredAccessLevel === undefined) { return false; } const request: CompleteRequest = context.switchToHttp().getRequest(); const user = request.user ?? null; - // handle CREATE permissions, as this does not need any note - if (permission === RequiredPermission.CREATE) { + + // handle CREATE requiredAccessLevel, as this does not need any note + if (requiredAccessLevel === RequiredPermission.CREATE) { return this.permissionsService.mayCreate(user); } - // Attention: This gets the note an additional time if used in conjunction with GetNoteInterceptor or NoteHeaderInterceptor + const note = await extractNoteFromRequest(request, this.noteService); if (note === undefined) { this.logger.error( @@ -57,10 +52,41 @@ export class PermissionsGuard implements CanActivate { ); return false; } - return await this.permissionsService.checkPermissionOnNote( - permission, - user, - note, + + return this.isNotePermissionFulfillingRequiredAccessLevel( + requiredAccessLevel, + await this.permissionsService.determinePermission(user, note), ); } + + private extractRequiredPermission( + context: ExecutionContext, + ): RequiredPermission | undefined { + const requiredPermission = this.reflector.get( + PERMISSION_METADATA_KEY, + context.getHandler(), + ); + // If no requiredPermission are set this is probably an error and this guard should not let the request pass + if (!requiredPermission) { + this.logger.error( + 'Could not find requiredPermission metadata. This should never happen. If you see this, please open an issue at https://github.com/hedgedoc/hedgedoc/issues', + ); + return undefined; + } + return requiredPermission; + } + + private isNotePermissionFulfillingRequiredAccessLevel( + requiredAccessLevel: Exclude, + actualNotePermission: NotePermission, + ): boolean { + switch (requiredAccessLevel) { + case RequiredPermission.READ: + return actualNotePermission >= NotePermission.READ; + case RequiredPermission.WRITE: + return actualNotePermission >= NotePermission.WRITE; + case RequiredPermission.OWNER: + return actualNotePermission >= NotePermission.OWNER; + } + } } diff --git a/backend/src/permissions/permissions.service.spec.ts b/backend/src/permissions/permissions.service.spec.ts index ba2ae2e0a..83af24699 100644 --- a/backend/src/permissions/permissions.service.spec.ts +++ b/backend/src/permissions/permissions.service.spec.ts @@ -7,6 +7,7 @@ import { ConfigModule } from '@nestjs/config'; import { EventEmitter2, EventEmitterModule } from '@nestjs/event-emitter'; import { Test, TestingModule } from '@nestjs/testing'; import { getRepositoryToken } from '@nestjs/typeorm'; +import { Mock } from 'ts-mockery'; import { DataSource, EntityManager, Repository } from 'typeorm'; import { AuthToken } from '../auth/auth-token.entity'; @@ -25,7 +26,7 @@ import { PermissionsUpdateInconsistentError } from '../errors/errors'; import { eventModuleConfig, NoteEvent } from '../events'; import { Group } from '../groups/group.entity'; import { GroupsModule } from '../groups/groups.module'; -import { SpecialGroup } from '../groups/groups.special'; +import { GroupsService } from '../groups/groups.service'; import { Identity } from '../identity/identity.entity'; import { LoggerModule } from '../logger/logger.module'; import { MediaUpload } from '../media/media-upload.entity'; @@ -43,10 +44,32 @@ import { Session } from '../users/session.entity'; import { User } from '../users/user.entity'; import { UsersModule } from '../users/users.module'; import { NoteGroupPermission } from './note-group-permission.entity'; +import { + getNotePermissionDisplayName, + NotePermission, +} from './note-permission.enum'; import { NoteUserPermission } from './note-user-permission.entity'; import { PermissionsModule } from './permissions.module'; import { PermissionsService } from './permissions.service'; -import { RequiredPermission } from './required-permission.enum'; +import { convertGuestAccessToNotePermission } from './utils/convert-guest-access-to-note-permission'; +import * as FindHighestNotePermissionByGroupModule from './utils/find-highest-note-permission-by-group'; +import * as FindHighestNotePermissionByUserModule from './utils/find-highest-note-permission-by-user'; + +jest.mock( + './utils/find-highest-note-permission-by-user', + () => + jest.requireActual( + './utils/find-highest-note-permission-by-user', + ) as unknown, +); + +jest.mock( + './utils/find-highest-note-permission-by-group', + () => + jest.requireActual( + './utils/find-highest-note-permission-by-group', + ) as unknown, +); function mockedEventEmitter(eventEmitter: EventEmitter2) { return jest.spyOn(eventEmitter, 'emit').mockImplementationOnce((event) => { @@ -67,7 +90,7 @@ function mockNoteRepo(noteRepo: Repository) { describe('PermissionsService', () => { let service: PermissionsService; - let notes: Note[]; + let groupService: GroupsService; let noteRepo: Repository; let userRepo: Repository; let groupRepo: Repository; @@ -165,570 +188,42 @@ describe('PermissionsService', () => { .useValue({}) .compile(); service = module.get(PermissionsService); - notes = await createNoteUserPermissionNotes(); + groupService = module.get(GroupsService); groupRepo = module.get>(getRepositoryToken(Group)); noteRepo = module.get>(getRepositoryToken(Note)); eventEmitter = module.get(EventEmitter2); }); - function mockMayReadTrue(): void { - jest.spyOn(service, 'mayRead').mockImplementationOnce(async () => { - return true; - }); - } - - function mockMayWriteTrue(): void { - jest.spyOn(service, 'mayWrite').mockImplementationOnce(async () => { - return true; - }); - } - - function mockIsOwner(isOwner: boolean): void { - jest.spyOn(service, 'isOwner').mockImplementationOnce(async () => { - return isOwner; - }); - } - beforeEach(() => { mockNoteRepo(noteRepo); eventEmitterEmitSpy = mockedEventEmitter(eventEmitter); }); + afterEach(() => { - jest.clearAllMocks(); + jest.resetModules(); + jest.restoreAllMocks(); }); // The two users we test with: - const user2 = {} as User; - user2.id = 2; - const user1 = {} as User; - user1.id = 1; + const user1 = Mock.of({ id: 1 }); + const user2 = Mock.of({ id: 2 }); + + function mockNote( + owner: User, + userPermissions: NoteUserPermission[] = [], + groupPermissions: NoteGroupPermission[] = [], + ): Note { + return Mock.of({ + owner: Promise.resolve(owner), + userPermissions: Promise.resolve(userPermissions), + groupPermissions: Promise.resolve(groupPermissions), + }); + } it('should be defined', () => { expect(service).toBeDefined(); }); - function createNote(owner: User): Note { - const note = {} as Note; - note.userPermissions = Promise.resolve([]); - note.groupPermissions = Promise.resolve([]); - note.owner = Promise.resolve(owner); - return note; - } - - /* - * Creates the permission objects for UserPermission for two users with write and with out write permission - */ - async function createNoteUserPermissionNotes(): Promise { - 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 = Promise.resolve(user1); - const noteUserPermission2 = {} as NoteUserPermission; - noteUserPermission2.user = Promise.resolve(user2); - const noteUserPermission3 = {} as NoteUserPermission; - noteUserPermission3.user = Promise.resolve(user1); - noteUserPermission3.canEdit = true; - const noteUserPermission4 = {} as NoteUserPermission; - noteUserPermission4.user = Promise.resolve(user2); - noteUserPermission4.canEdit = true; - - (await note1.userPermissions).push(noteUserPermission1); - - (await note2.userPermissions).push(noteUserPermission1); - (await note2.userPermissions).push(noteUserPermission2); - - (await note3.userPermissions).push(noteUserPermission2); - (await note3.userPermissions).push(noteUserPermission1); - - (await note4.userPermissions).push(noteUserPermission3); - - (await note5.userPermissions).push(noteUserPermission3); - (await note5.userPermissions).push(noteUserPermission4); - - (await note6.userPermissions).push(noteUserPermission4); - (await note6.userPermissions).push(noteUserPermission3); - - (await note7.userPermissions).push(noteUserPermission2); - - const everybody = {} as Group; - everybody.name = SpecialGroup.EVERYONE; - everybody.special = true; - - const noteEverybodyNone = createNote(user1); - noteEverybodyNone.groupPermissions = Promise.resolve([]); - - const noteEverybodyRead = createNote(user1); - const noteGroupPermissionRead = {} as NoteGroupPermission; - noteGroupPermissionRead.group = Promise.resolve(everybody); - noteGroupPermissionRead.canEdit = false; - noteGroupPermissionRead.note = Promise.resolve(noteEverybodyRead); - noteEverybodyRead.groupPermissions = Promise.resolve([ - noteGroupPermissionRead, - ]); - - const noteEverybodyWrite = createNote(user1); - const noteGroupPermissionWrite = {} as NoteGroupPermission; - noteGroupPermissionWrite.group = Promise.resolve(everybody); - noteGroupPermissionWrite.canEdit = true; - noteGroupPermissionWrite.note = Promise.resolve(noteEverybodyWrite); - noteEverybodyWrite.groupPermissions = Promise.resolve([ - noteGroupPermissionWrite, - ]); - - return [ - note0, - note1, - note2, - note3, - note4, - note5, - note6, - note7, - noteEverybodyRead, - noteEverybodyWrite, - noteEverybodyNone, - ]; - } - - describe('mayRead works with', () => { - it('Owner', async () => { - expect(await service.mayRead(user1, notes[0])).toBeTruthy(); - expect(await service.mayRead(user1, notes[7])).toBeFalsy(); - }); - it('userPermission read', async () => { - expect(await service.mayRead(user1, notes[1])).toBeTruthy(); - expect(await service.mayRead(user1, notes[2])).toBeTruthy(); - expect(await service.mayRead(user1, notes[3])).toBeTruthy(); - }); - it('userPermission write', async () => { - expect(await service.mayRead(user1, notes[4])).toBeTruthy(); - expect(await service.mayRead(user1, notes[5])).toBeTruthy(); - expect(await service.mayRead(user1, notes[6])).toBeTruthy(); - expect(await service.mayRead(user1, notes[7])).toBeFalsy(); - }); - - describe('guest permission', () => { - beforeEach(() => { - noteMockConfig.permissions.default.loggedIn = DefaultAccessLevel.WRITE; - noteMockConfig.permissions.default.everyone = DefaultAccessLevel.WRITE; - }); - describe('with guest access deny', () => { - beforeEach(() => { - noteMockConfig.guestAccess = GuestAccess.DENY; - }); - it('guest permission none', async () => { - expect(await service.mayRead(null, notes[10])).toBeFalsy(); - }); - it('guest permission read', async () => { - expect(await service.mayRead(null, notes[8])).toBeFalsy(); - }); - it('guest permission write', async () => { - expect(await service.mayRead(null, notes[9])).toBeFalsy(); - }); - }); - describe('with guest access read', () => { - beforeEach(() => { - noteMockConfig.guestAccess = GuestAccess.READ; - }); - it('guest permission none', async () => { - expect(await service.mayRead(null, notes[10])).toBeFalsy(); - }); - it('guest permission read', async () => { - expect(await service.mayRead(null, notes[8])).toBeTruthy(); - }); - it('guest permission write', async () => { - expect(await service.mayRead(null, notes[9])).toBeTruthy(); - }); - }); - describe('with guest access write', () => { - beforeEach(() => { - noteMockConfig.guestAccess = GuestAccess.WRITE; - }); - it('guest permission none', async () => { - expect(await service.mayRead(null, notes[10])).toBeFalsy(); - }); - it('guest permission read', async () => { - expect(await service.mayRead(null, notes[8])).toBeTruthy(); - }); - it('guest permission write', async () => { - expect(await service.mayRead(null, notes[9])).toBeTruthy(); - }); - }); - describe('with guest access create', () => { - beforeEach(() => { - noteMockConfig.guestAccess = GuestAccess.CREATE; - }); - it('guest permission none', async () => { - expect(await service.mayRead(null, notes[10])).toBeFalsy(); - }); - it('guest permission read', async () => { - expect(await service.mayRead(null, notes[8])).toBeTruthy(); - }); - it('guest permission write', async () => { - expect(await service.mayRead(null, notes[9])).toBeTruthy(); - }); - }); - }); - }); - - describe('mayWrite works with', () => { - it('Owner', async () => { - expect(await service.mayWrite(user1, notes[0])).toBeTruthy(); - expect(await service.mayWrite(user1, notes[7])).toBeFalsy(); - }); - it('userPermission read', async () => { - expect(await service.mayWrite(user1, notes[1])).toBeFalsy(); - expect(await service.mayWrite(user1, notes[2])).toBeFalsy(); - expect(await service.mayWrite(user1, notes[3])).toBeFalsy(); - }); - it('userPermission write', async () => { - expect(await service.mayWrite(user1, notes[4])).toBeTruthy(); - expect(await service.mayWrite(user1, notes[5])).toBeTruthy(); - expect(await service.mayWrite(user1, notes[6])).toBeTruthy(); - expect(await service.mayWrite(user1, notes[7])).toBeFalsy(); - }); - describe('guest permission', () => { - beforeEach(() => { - noteMockConfig.permissions.default.loggedIn = DefaultAccessLevel.WRITE; - noteMockConfig.permissions.default.everyone = DefaultAccessLevel.WRITE; - }); - - describe('with guest access deny', () => { - beforeEach(() => { - noteMockConfig.guestAccess = GuestAccess.DENY; - }); - it('guest permission none', async () => { - expect(await service.mayWrite(null, notes[10])).toBeFalsy(); - }); - it('guest permission read', async () => { - expect(await service.mayWrite(null, notes[8])).toBeFalsy(); - }); - it('guest permission write', async () => { - expect(await service.mayWrite(null, notes[9])).toBeFalsy(); - }); - }); - - describe('with guest access read', () => { - beforeEach(() => { - noteMockConfig.guestAccess = GuestAccess.READ; - }); - it('guest permission none', async () => { - expect(await service.mayWrite(null, notes[10])).toBeFalsy(); - }); - it('guest permission read', async () => { - expect(await service.mayWrite(null, notes[8])).toBeFalsy(); - }); - it('guest permission write', async () => { - expect(await service.mayWrite(null, notes[9])).toBeFalsy(); - }); - }); - - describe('with guest access write', () => { - beforeEach(() => { - noteMockConfig.guestAccess = GuestAccess.WRITE; - }); - it('guest permission none', async () => { - expect(await service.mayWrite(null, notes[10])).toBeFalsy(); - }); - it('guest permission read', async () => { - expect(await service.mayWrite(null, notes[8])).toBeFalsy(); - }); - it('guest permission write', async () => { - expect(await service.mayWrite(null, notes[9])).toBeTruthy(); - }); - }); - - describe('with guest access create', () => { - beforeEach(() => { - noteMockConfig.guestAccess = GuestAccess.CREATE; - }); - it('guest permission none', async () => { - expect(await service.mayWrite(null, notes[10])).toBeFalsy(); - }); - it('guest permission read', async () => { - expect(await service.mayWrite(null, notes[8])).toBeFalsy(); - }); - it('guest permission write', async () => { - expect(await service.mayWrite(null, notes[9])).toBeTruthy(); - }); - }); - }); - }); - - /* - * 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 } = {}; - - result[SpecialGroup.EVERYONE] = Group.create( - SpecialGroup.EVERYONE, - SpecialGroup.EVERYONE, - true, - ) as Group; - - result[SpecialGroup.LOGGED_IN] = Group.create( - SpecialGroup.LOGGED_IN, - SpecialGroup.LOGGED_IN, - true, - ) as Group; - - const user1group = Group.create('user1group', 'user1group', false) as Group; - user1group.members = Promise.resolve([user1]); - result['user1group'] = user1group; - - const user2group = Group.create('user2group', 'user2group', false) as Group; - user2group.members = Promise.resolve([user2]); - result['user2group'] = user2group; - - const user1and2group = Group.create( - 'user1and2group', - 'user1and2group', - false, - ) as Group; - user1and2group.members = Promise.resolve([user1, user2]); - result['user1and2group'] = user1and2group; - - const user2and1group = Group.create( - 'user2and1group', - 'user2and1group', - false, - ) as Group; - user2and1group.members = Promise.resolve([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 | null)[][] { - const groups = createGroups(); - - /* - * Helper function for creating GroupPermissions - */ - function createNoteGroupPermission( - group: Group, - write: boolean, - ): NoteGroupPermission { - return NoteGroupPermission.create(group, {} as Note, write); - } - - const everybodyRead = createNoteGroupPermission( - groups[SpecialGroup.EVERYONE], - false, - ); - const everybodyWrite = createNoteGroupPermission( - groups[SpecialGroup.EVERYONE], - true, - ); - - const loggedInRead = createNoteGroupPermission( - groups[SpecialGroup.LOGGED_IN], - false, - ); - const loggedInWrite = createNoteGroupPermission( - groups[SpecialGroup.LOGGED_IN], - 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( - everyoneDefaultPermission: DefaultAccessLevel, - ): 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: NoteGroupPermission[] = []; - 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) { - if (everyoneDefaultPermission === DefaultAccessLevel.WRITE) { - writePermission = writePermission || group2.canEdit; - readPermission = true; - } else if ( - everyoneDefaultPermission === DefaultAccessLevel.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: NoteGroupPermission[][] = []; - - function permute( - arr: NoteGroupPermission[], - memo: NoteGroupPermission[], - ): NoteGroupPermission[][] { - let cur: NoteGroupPermission[]; - - 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 rawPermissions = createNoteGroupPermissionsCombinations( - DefaultAccessLevel.WRITE, - ); - const permissions = permuteNoteGroupPermissions(rawPermissions); - let i = 0; - for (const permission of permissions) { - const note = createNote(user2); - note.groupPermissions = Promise.resolve(permission.permissions); - let permissionString = ''; - for (const perm of permission.permissions) { - permissionString += ` ${perm.id}:${String(perm.canEdit)}`; - } - it(`mayWrite - test #${i}:${permissionString}`, async () => { - expect(await service.mayWrite(user1, note)).toEqual( - permission.allowsWrite, - ); - }); - it(`mayRead - test #${i}:${permissionString}`, async () => { - expect(await service.mayRead(user1, note)).toEqual( - permission.allowsRead, - ); - }); - i++; - } - }); - describe('mayCreate', () => { it('allows creation for logged in', () => { expect(service.mayCreate(user1)).toBeTruthy(); @@ -747,20 +242,33 @@ describe('PermissionsService', () => { }); }); - describe('isOwner works', () => { - it('for positive case', async () => { - expect(await service.isOwner(user1, notes[0])).toBeTruthy(); + describe('isOwner', () => { + it('works correctly if user is owner', async () => { + const note = mockNote(user1); + expect(await service.isOwner(user1, note)).toBeTruthy(); }); - it('for negative case', async () => { - expect(await service.isOwner(user1, notes[1])).toBeFalsy(); + it("works correctly if user isn't the owner", async () => { + const note = mockNote(user2); + expect(await service.isOwner(user1, note)).toBeFalsy(); + }); + it('works correctly if no user is provided', async () => { + const note = mockNote(user2); + expect(await service.isOwner(null, note)).toBeFalsy(); }); }); describe('checkMediaDeletePermission', () => { + const noteUserPermission1 = Mock.of({ + user: Promise.resolve(user1), + canEdit: false, + }); + + const noteOfUser2 = mockNote(user2, [noteUserPermission1]); + describe('accepts', () => { it('for media owner', async () => { const mediaUpload = {} as MediaUpload; - mediaUpload.note = Promise.resolve(notes[1]); + mediaUpload.note = Promise.resolve(noteOfUser2); mediaUpload.user = Promise.resolve(user1); expect( service.checkMediaDeletePermission(user1, mediaUpload), @@ -768,7 +276,7 @@ describe('PermissionsService', () => { }); it('for note owner', async () => { const mediaUpload = {} as MediaUpload; - mediaUpload.note = Promise.resolve(notes[1]); + mediaUpload.note = Promise.resolve(noteOfUser2); mediaUpload.user = Promise.resolve(user1); expect( service.checkMediaDeletePermission(user2, mediaUpload), @@ -777,10 +285,9 @@ describe('PermissionsService', () => { }); describe('denies', () => { it('for not owner', async () => { - const user3 = {} as User; - user3.id = 3; - const mediaUpload = {} as MediaUpload; - mediaUpload.note = Promise.resolve(notes[1]); + const user3 = Mock.of({ id: 3 }); + const mediaUpload = Mock.of(); + mediaUpload.note = Promise.resolve(noteOfUser2); mediaUpload.user = Promise.resolve(user1); expect( await service.checkMediaDeletePermission(user3, mediaUpload), @@ -789,51 +296,166 @@ describe('PermissionsService', () => { }); }); - describe('checkPermissionOnNote', () => { - describe('accepts', () => { - it('with mayRead', async () => { - mockMayReadTrue(); - expect( - await service.checkPermissionOnNote( - RequiredPermission.READ, - user1, - notes[0], - ), - ).toBeTruthy(); + describe('determinePermission', () => { + const everyoneGroup = Mock.of({ id: 99 }); + const loggedInGroup = Mock.of({ id: 98 }); + + beforeEach(() => { + jest + .spyOn(groupService, 'getEveryoneGroup') + .mockImplementation(() => Promise.resolve(everyoneGroup)); + jest + .spyOn(groupService, 'getLoggedInGroup') + .mockImplementation(() => Promise.resolve(loggedInGroup)); + }); + + describe('with guest user', () => { + const loggedInReadPermission = Mock.of({ + canEdit: false, + group: Promise.resolve(loggedInGroup), }); - it('with mayWrite', async () => { - mockMayWriteTrue(); - expect( - await service.checkPermissionOnNote( - RequiredPermission.WRITE, - user1, - notes[0], - ), - ).toBeTruthy(); + + it(`with no everyone permission will deny`, async () => { + const note = mockNote(user1, [], [loggedInReadPermission]); + const foundPermission = await service.determinePermission(null, note); + expect(foundPermission).toBe(NotePermission.DENY); }); - it('with isOwner', async () => { - mockIsOwner(true); - expect( - await service.checkPermissionOnNote( - RequiredPermission.OWNER, - user1, - notes[0], - ), - ).toBeTruthy(); + + describe.each([ + GuestAccess.DENY, + GuestAccess.READ, + GuestAccess.WRITE, + GuestAccess.CREATE, + ])('with configured guest access %s', (guestAccess) => { + beforeEach(() => { + noteMockConfig.guestAccess = guestAccess; + }); + + const guestAccessNotePermission = + convertGuestAccessToNotePermission(guestAccess); + + describe.each([false, true])( + 'with everybody group permission with edit set to %s', + (canEdit) => { + const editPermission = canEdit + ? NotePermission.WRITE + : NotePermission.READ; + const expectedLimitedPermission = + guestAccessNotePermission >= editPermission + ? editPermission + : guestAccessNotePermission; + + const permissionDisplayName = getNotePermissionDisplayName( + expectedLimitedPermission, + ); + it(`will ${permissionDisplayName}`, async () => { + const everybodyPermission = Mock.of({ + group: Promise.resolve(everyoneGroup), + canEdit: canEdit, + }); + + const note = mockNote( + user1, + [], + [everybodyPermission, loggedInReadPermission], + ); + + const foundPermission = await service.determinePermission( + null, + note, + ); + expect(foundPermission).toBe(expectedLimitedPermission); + }); + }, + ); }); }); - describe('denies', () => { - it('with no permission', async () => { - mockMayReadTrue(); - mockMayWriteTrue(); - mockIsOwner(false); - expect( - await service.checkPermissionOnNote( - RequiredPermission.OWNER, + + describe('with logged in user', () => { + describe('as owner will be OWNER permission', () => { + it('without other permissions', async () => { + const note = mockNote(user1); + + const foundPermission = await service.determinePermission( user1, - notes[0], - ), - ).toBeFalsy(); + note, + ); + + expect(foundPermission).toBe(NotePermission.OWNER); + }); + it('with other lower permissions', async () => { + const userPermission = Mock.of({ + user: Promise.resolve(user1), + canEdit: true, + }); + + const group1 = Mock.of({ + name: 'mockGroup', + id: 99, + members: Promise.resolve([user1]), + }); + + const groupPermission = Mock.of({ + group: Promise.resolve(group1), + canEdit: true, + }); + + const note = mockNote(user1, [userPermission], [groupPermission]); + + const foundPermission = await service.determinePermission( + user1, + note, + ); + + expect(foundPermission).toBe(NotePermission.OWNER); + }); + }); + describe('as non owner', () => { + it('with user permission higher than group permission', async () => { + jest + .spyOn( + FindHighestNotePermissionByUserModule, + 'findHighestNotePermissionByUser', + ) + .mockReturnValue(Promise.resolve(NotePermission.DENY)); + jest + .spyOn( + FindHighestNotePermissionByGroupModule, + 'findHighestNotePermissionByGroup', + ) + .mockReturnValue(Promise.resolve(NotePermission.WRITE)); + + const note = mockNote(user2); + + const foundPermission = await service.determinePermission( + user1, + note, + ); + expect(foundPermission).toBe(NotePermission.WRITE); + }); + + it('with group permission higher than user permission', async () => { + jest + .spyOn( + FindHighestNotePermissionByUserModule, + 'findHighestNotePermissionByUser', + ) + .mockReturnValue(Promise.resolve(NotePermission.WRITE)); + jest + .spyOn( + FindHighestNotePermissionByGroupModule, + 'findHighestNotePermissionByGroup', + ) + .mockReturnValue(Promise.resolve(NotePermission.DENY)); + + const note = mockNote(user2); + + const foundPermission = await service.determinePermission( + user1, + note, + ); + expect(foundPermission).toBe(NotePermission.WRITE); + }); }); }); }); @@ -1183,24 +805,49 @@ describe('PermissionsService', () => { expect(eventEmitterEmitSpy).toHaveBeenCalled(); }); describe('works', () => { - it('with user added before and editable', async () => { - const note = Note.create(null) as Note; - const user = User.create('test', 'Testy') as User; - note.userPermissions = Promise.resolve([ - NoteUserPermission.create(user, note, true), - ]); + const note = Note.create(null) as Note; + const user1 = Mock.of({ id: 1 }); + const user2 = Mock.of({ id: 2 }); - const resultNote = await service.removeUserPermission(note, user); - expect((await resultNote.userPermissions).length).toStrictEqual(0); - }); - it('with user not added before and not editable', async () => { - const note = Note.create(null) as Note; - const user = User.create('test', 'Testy') as User; + it('with user added before and editable', async () => { + const noteUserPermission1 = NoteUserPermission.create( + user1, + note, + true, + ); + const noteUserPermission2 = NoteUserPermission.create( + user2, + note, + true, + ); note.userPermissions = Promise.resolve([ - NoteUserPermission.create(user, note, false), + noteUserPermission1, + noteUserPermission2, + ]); + const resultNote = await service.removeUserPermission(note, user1); + expect(await resultNote.userPermissions).toStrictEqual([ + noteUserPermission2, + ]); + }); + it('with user added before and not editable', async () => { + const noteUserPermission1 = NoteUserPermission.create( + user1, + note, + false, + ); + const noteUserPermission2 = NoteUserPermission.create( + user2, + note, + false, + ); + note.userPermissions = Promise.resolve([ + noteUserPermission1, + noteUserPermission2, + ]); + const resultNote = await service.removeUserPermission(note, user1); + expect(await resultNote.userPermissions).toStrictEqual([ + noteUserPermission2, ]); - const resultNote = await service.removeUserPermission(note, user); - expect((await resultNote.userPermissions).length).toStrictEqual(0); }); }); }); @@ -1290,24 +937,50 @@ describe('PermissionsService', () => { expect(eventEmitterEmitSpy).toHaveBeenCalled(); }); describe('works', () => { - it('with user added before and editable', async () => { - const note = Note.create(null) as Note; - const group = Group.create('test', 'Testy', false) as Group; + const note = Note.create(null) as Note; + const group1 = Mock.of({ id: 1 }); + const group2 = Mock.of({ id: 2 }); + + it('with editable group', async () => { + const noteGroupPermission1 = NoteGroupPermission.create( + group1, + note, + true, + ); + const noteGroupPermission2 = NoteGroupPermission.create( + group2, + note, + true, + ); note.groupPermissions = Promise.resolve([ - NoteGroupPermission.create(group, note, true), + noteGroupPermission1, + noteGroupPermission2, ]); - const resultNote = await service.removeGroupPermission(note, group); - expect((await resultNote.groupPermissions).length).toStrictEqual(0); - }); - it('with user not added before and not editable', async () => { - const note = Note.create(null) as Note; - const group = Group.create('test', 'Testy', false) as Group; - note.groupPermissions = Promise.resolve([ - NoteGroupPermission.create(group, note, false), + const resultNote = await service.removeGroupPermission(note, group1); + expect(await resultNote.groupPermissions).toStrictEqual([ + noteGroupPermission2, + ]); + }); + it('with not editable group', async () => { + const noteGroupPermission1 = NoteGroupPermission.create( + group1, + note, + false, + ); + const noteGroupPermission2 = NoteGroupPermission.create( + group2, + note, + false, + ); + note.groupPermissions = Promise.resolve([ + noteGroupPermission1, + noteGroupPermission2, + ]); + const resultNote = await service.removeGroupPermission(note, group1); + expect(await resultNote.groupPermissions).toStrictEqual([ + noteGroupPermission2, ]); - const resultNote = await service.removeGroupPermission(note, group); - expect((await resultNote.groupPermissions).length).toStrictEqual(0); }); }); }); diff --git a/backend/src/permissions/permissions.service.ts b/backend/src/permissions/permissions.service.ts index 84706500b..f2f7a79a5 100644 --- a/backend/src/permissions/permissions.service.ts +++ b/backend/src/permissions/permissions.service.ts @@ -8,16 +8,12 @@ import { EventEmitter2 } from '@nestjs/event-emitter'; import { InjectRepository } from '@nestjs/typeorm'; import { Repository } from 'typeorm'; -import { - getGuestAccessOrdinal, - GuestAccess, -} from '../config/guest_access.enum'; +import { GuestAccess } from '../config/guest_access.enum'; import noteConfiguration, { NoteConfig } from '../config/note.config'; import { PermissionsUpdateInconsistentError } from '../errors/errors'; import { NoteEvent, NoteEventMap } from '../events'; import { Group } from '../groups/group.entity'; import { GroupsService } from '../groups/groups.service'; -import { SpecialGroup } from '../groups/groups.special'; import { ConsoleLoggerService } from '../logger/console-logger.service'; import { MediaUpload } from '../media/media-upload.entity'; import { NotePermissionsUpdateDto } from '../notes/note-permissions.dto'; @@ -26,8 +22,11 @@ import { User } from '../users/user.entity'; import { UsersService } from '../users/users.service'; import { checkArrayForDuplicates } from '../utils/arrayDuplicatCheck'; import { NoteGroupPermission } from './note-group-permission.entity'; +import { NotePermission } from './note-permission.enum'; import { NoteUserPermission } from './note-user-permission.entity'; -import { RequiredPermission } from './required-permission.enum'; +import { convertGuestAccessToNotePermission } from './utils/convert-guest-access-to-note-permission'; +import { findHighestNotePermissionByGroup } from './utils/find-highest-note-permission-by-group'; +import { findHighestNotePermissionByUser } from './utils/find-highest-note-permission-by-user'; @Injectable() export class PermissionsService { @@ -40,29 +39,6 @@ export class PermissionsService { private noteConfig: NoteConfig, private eventEmitter: EventEmitter2, ) {} - /** - * Checks if the given {@link User} is has the in {@link desiredPermission} specified permission on {@link Note}. - * - * @async - * @param {RequiredPermission} desiredPermission - permission level to check for - * @param {User} user - The user whose permission should be checked. Value is null if guest access should be checked - * @param {Note} note - The note for which the permission should be checked - * @return if the user has the specified permission on the note - */ - public async checkPermissionOnNote( - desiredPermission: Exclude, - user: User | null, - note: Note, - ): Promise { - switch (desiredPermission) { - case RequiredPermission.READ: - return await this.mayRead(user, note); - case RequiredPermission.WRITE: - return await this.mayWrite(user, note); - case RequiredPermission.OWNER: - return await this.isOwner(user, note); - } - } public async checkMediaDeletePermission( user: User, @@ -77,38 +53,6 @@ export class PermissionsService { return mediaUploadOwner?.id === user.id || owner; } - /** - * Checks if the given {@link User} is allowed to read the given {@link Note}. - * - * @async - * @param {User} user - The user whose permission should be checked. Value is null if guest access should be checked - * @param {Note} note - The note for which the permission should be checked - * @return if the user is allowed to read the note - */ - public async mayRead(user: User | null, note: Note): Promise { - return ( - (await this.isOwner(user, note)) || - (await this.hasPermissionUser(user, note, false)) || - (await this.hasPermissionGroup(user, note, false)) - ); - } - - /** - * Checks if the given {@link User} is allowed to edit the given {@link Note}. - * - * @async - * @param {User} user - The user whose permission should be checked - * @param {Note} note - The note for which the permission should be checked. Value is null if guest access should be checked - * @return if the user is allowed to edit the note - */ - public async mayWrite(user: User | null, note: Note): Promise { - return ( - (await this.isOwner(user, note)) || - (await this.hasPermissionUser(user, note, true)) || - (await this.hasPermissionGroup(user, note, true)) - ); - } - /** * Checks if the given {@link User} is allowed to create notes. * @@ -121,74 +65,77 @@ export class PermissionsService { } async isOwner(user: User | null, note: Note): Promise { - if (!user) return false; - const owner = await note.owner; - if (!owner) return false; - return owner.id === user.id; - } - - // noinspection JSMethodCanBeStatic - private async hasPermissionUser( - user: User | null, - note: Note, - wantEdit: boolean, - ): Promise { if (!user) { return false; } - for (const userPermission of await note.userPermissions) { - if ( - (await userPermission.user).id === user.id && - (userPermission.canEdit || !wantEdit) - ) { - return true; - } + const owner = await note.owner; + if (!owner) { + return false; } - return false; + return owner.id === user.id; } - private async hasPermissionGroup( + /** + * Determines the {@link NotePermission permission} of the user on the given {@link Note}. + * + * @param {User | null} user The user whose permission should be checked + * @param {Note} note The note that is accessed by the given user + * @return {Promise} The determined permission + */ + public async determinePermission( user: User | null, note: Note, - wantEdit: boolean, - ): Promise { + ): Promise { if (user === null) { - if ( - (!wantEdit && - getGuestAccessOrdinal(this.noteConfig.guestAccess) < - getGuestAccessOrdinal(GuestAccess.READ)) || - (wantEdit && - getGuestAccessOrdinal(this.noteConfig.guestAccess) < - getGuestAccessOrdinal(GuestAccess.WRITE)) - ) { - return false; - } + return await this.findGuestNotePermission(await note.groupPermissions); } - for (const groupPermission of await note.groupPermissions) { - if (groupPermission.canEdit || !wantEdit) { - // Handle special groups - if ((await groupPermission.group).special) { - if ( - (user && - (await groupPermission.group).name == SpecialGroup.LOGGED_IN) || - (await groupPermission.group).name == SpecialGroup.EVERYONE - ) { - return true; - } - } else { - // Handle normal groups - if (user) { - for (const member of await ( - await groupPermission.group - ).members) { - if (member.id === user.id) return true; - } - } - } - } + if (await this.isOwner(user, note)) { + return NotePermission.OWNER; } - return false; + const userPermission = await findHighestNotePermissionByUser( + user, + await note.userPermissions, + ); + if (userPermission === NotePermission.WRITE) { + return userPermission; + } + const groupPermission = await findHighestNotePermissionByGroup( + user, + await note.groupPermissions, + ); + return groupPermission > userPermission ? groupPermission : userPermission; + } + + private async findGuestNotePermission( + groupPermissions: NoteGroupPermission[], + ): Promise { + if (this.noteConfig.guestAccess === GuestAccess.DENY) { + return NotePermission.DENY; + } + + const everyonePermission = await this.findPermissionForGroup( + groupPermissions, + await this.groupsService.getEveryoneGroup(), + ); + if (everyonePermission === undefined) { + return NotePermission.DENY; + } + const notePermission = everyonePermission.canEdit + ? NotePermission.WRITE + : NotePermission.READ; + return this.limitNotePermissionToGuestAccessLevel(notePermission); + } + + private limitNotePermissionToGuestAccessLevel( + notePermission: NotePermission, + ): NotePermission { + const configuredGuestNotePermission = convertGuestAccessToNotePermission( + this.noteConfig.guestAccess, + ); + return configuredGuestNotePermission < notePermission + ? configuredGuestNotePermission + : notePermission; } private notifyOthers(note: Note): void { diff --git a/backend/src/permissions/utils/convert-guest-access-to-note-permission.spec.ts b/backend/src/permissions/utils/convert-guest-access-to-note-permission.spec.ts new file mode 100644 index 000000000..04bef2ef7 --- /dev/null +++ b/backend/src/permissions/utils/convert-guest-access-to-note-permission.spec.ts @@ -0,0 +1,34 @@ +/* + * SPDX-FileCopyrightText: 2023 The HedgeDoc developers (see AUTHORS file) + * + * SPDX-License-Identifier: AGPL-3.0-only + */ +import { GuestAccess } from '../../config/guest_access.enum'; +import { NotePermission } from '../note-permission.enum'; +import { convertGuestAccessToNotePermission } from './convert-guest-access-to-note-permission'; + +describe('convert guest access to note permission', () => { + it('no guest access means no note access', () => { + expect(convertGuestAccessToNotePermission(GuestAccess.DENY)).toBe( + NotePermission.DENY, + ); + }); + + it('translates read access to read permission', () => { + expect(convertGuestAccessToNotePermission(GuestAccess.READ)).toBe( + NotePermission.READ, + ); + }); + + it('translates write access to write permission', () => { + expect(convertGuestAccessToNotePermission(GuestAccess.WRITE)).toBe( + NotePermission.WRITE, + ); + }); + + it('translates create access to write permission', () => { + expect(convertGuestAccessToNotePermission(GuestAccess.CREATE)).toBe( + NotePermission.WRITE, + ); + }); +}); diff --git a/backend/src/permissions/utils/convert-guest-access-to-note-permission.ts b/backend/src/permissions/utils/convert-guest-access-to-note-permission.ts new file mode 100644 index 000000000..39dc7dd35 --- /dev/null +++ b/backend/src/permissions/utils/convert-guest-access-to-note-permission.ts @@ -0,0 +1,28 @@ +/* + * SPDX-FileCopyrightText: 2023 The HedgeDoc developers (see AUTHORS file) + * + * SPDX-License-Identifier: AGPL-3.0-only + */ +import { GuestAccess } from '../../config/guest_access.enum'; +import { NotePermission } from '../note-permission.enum'; + +/** + * Converts the given guest access level to the highest possible {@link NotePermission}. + * + * @param guestAccess the guest access level to should be converted + * @return the {@link NotePermission} representation + */ +export function convertGuestAccessToNotePermission( + guestAccess: GuestAccess, +): NotePermission.READ | NotePermission.WRITE | NotePermission.DENY { + switch (guestAccess) { + case GuestAccess.DENY: + return NotePermission.DENY; + case GuestAccess.READ: + return NotePermission.READ; + case GuestAccess.WRITE: + return NotePermission.WRITE; + case GuestAccess.CREATE: + return NotePermission.WRITE; + } +} diff --git a/backend/src/permissions/utils/find-highest-note-permission-by-group.spec.ts b/backend/src/permissions/utils/find-highest-note-permission-by-group.spec.ts new file mode 100644 index 000000000..6d5533d62 --- /dev/null +++ b/backend/src/permissions/utils/find-highest-note-permission-by-group.spec.ts @@ -0,0 +1,194 @@ +/* + * SPDX-FileCopyrightText: 2023 The HedgeDoc developers (see AUTHORS file) + * + * SPDX-License-Identifier: AGPL-3.0-only + */ +import { Mock } from 'ts-mockery'; + +import { Group } from '../../groups/group.entity'; +import { SpecialGroup } from '../../groups/groups.special'; +import { User } from '../../users/user.entity'; +import { NoteGroupPermission } from '../note-group-permission.entity'; +import { NotePermission } from '../note-permission.enum'; +import { findHighestNotePermissionByGroup } from './find-highest-note-permission-by-group'; + +describe('find highest note permission by group', () => { + const user1 = Mock.of({ id: 0 }); + const user2 = Mock.of({ id: 1 }); + const user3 = Mock.of({ id: 2 }); + const group2 = Mock.of({ + id: 1, + special: false, + members: Promise.resolve([user2]), + }); + const group3 = Mock.of({ + id: 2, + special: false, + members: Promise.resolve([user3]), + }); + + const permissionGroup2Read = Mock.of({ + group: Promise.resolve(group2), + canEdit: false, + }); + + const permissionGroup3Read = Mock.of({ + group: Promise.resolve(group3), + canEdit: false, + }); + + const permissionGroup3Write = Mock.of({ + group: Promise.resolve(group3), + canEdit: true, + }); + + describe('normal groups', () => { + it('will fallback to NONE if no permission for the user could be found', async () => { + const result = await findHighestNotePermissionByGroup(user1, [ + permissionGroup2Read, + permissionGroup3Write, + ]); + expect(result).toBe(NotePermission.DENY); + }); + + it('can extract a READ permission for the correct user', async () => { + const result = await findHighestNotePermissionByGroup(user2, [ + permissionGroup2Read, + permissionGroup3Write, + ]); + expect(result).toBe(NotePermission.READ); + }); + + it('can extract a WRITE permission for the correct user', async () => { + const result = await findHighestNotePermissionByGroup(user3, [ + permissionGroup2Read, + permissionGroup3Write, + ]); + expect(result).toBe(NotePermission.WRITE); + }); + + it('can extract a WRITE permission for the correct user if read and write are defined', async () => { + const result = await findHighestNotePermissionByGroup(user3, [ + permissionGroup2Read, + permissionGroup3Read, + permissionGroup3Write, + ]); + expect(result).toBe(NotePermission.WRITE); + }); + }); + + describe('special group', () => { + const groupEveryone = Mock.of({ + id: 3, + special: true, + name: SpecialGroup.EVERYONE, + }); + const groupLoggedIn = Mock.of({ + id: 4, + special: true, + name: SpecialGroup.LOGGED_IN, + }); + const permissionGroupEveryoneRead = Mock.of({ + group: Promise.resolve(groupEveryone), + canEdit: false, + }); + const permissionGroupLoggedInRead = Mock.of({ + group: Promise.resolve(groupLoggedIn), + canEdit: false, + }); + const permissionGroupEveryoneWrite = Mock.of({ + group: Promise.resolve(groupEveryone), + canEdit: true, + }); + const permissionGroupLoggedInWrite = Mock.of({ + group: Promise.resolve(groupLoggedIn), + canEdit: true, + }); + + it('will ignore unknown special groups', async () => { + const nonsenseSpecialGroup = Mock.of({ + id: 99, + special: true, + name: 'Unknown Special Group', + members: Promise.resolve([]), + }); + + const permissionUnknownSpecialGroup = Mock.of({ + group: Promise.resolve(nonsenseSpecialGroup), + canEdit: false, + }); + + const result = await findHighestNotePermissionByGroup(user1, [ + permissionUnknownSpecialGroup, + ]); + expect(result).toBe(NotePermission.DENY); + }); + + it('can extract the READ permission for logged in users', async () => { + const result = await findHighestNotePermissionByGroup(user1, [ + permissionGroupLoggedInRead, + ]); + expect(result).toBe(NotePermission.READ); + }); + + it('can extract the READ permission for everyone', async () => { + const result = await findHighestNotePermissionByGroup(user1, [ + permissionGroupEveryoneRead, + ]); + expect(result).toBe(NotePermission.READ); + }); + it('can extract the WRITE permission for logged in users', async () => { + const result = await findHighestNotePermissionByGroup(user1, [ + permissionGroupLoggedInWrite, + ]); + expect(result).toBe(NotePermission.WRITE); + }); + + it('can extract the WRITE permission for everyone', async () => { + const result = await findHighestNotePermissionByGroup(user1, [ + permissionGroupEveryoneWrite, + ]); + expect(result).toBe(NotePermission.WRITE); + }); + + it('can prefer everyone over logged in if necessary', async () => { + const result = await findHighestNotePermissionByGroup(user1, [ + permissionGroupEveryoneWrite, + permissionGroupLoggedInRead, + ]); + expect(result).toBe(NotePermission.WRITE); + }); + + it('can prefer normal groups over logged in if necessary', async () => { + const result = await findHighestNotePermissionByGroup(user3, [ + permissionGroup3Write, + permissionGroupLoggedInRead, + ]); + expect(result).toBe(NotePermission.WRITE); + }); + + it('can prefer normal groups over everyone if necessary', async () => { + const result = await findHighestNotePermissionByGroup(user3, [ + permissionGroup3Write, + permissionGroupEveryoneRead, + ]); + expect(result).toBe(NotePermission.WRITE); + }); + + it('can prefer logged in over normal groups if necessary', async () => { + const result = await findHighestNotePermissionByGroup(user3, [ + permissionGroup3Read, + permissionGroupLoggedInWrite, + ]); + expect(result).toBe(NotePermission.WRITE); + }); + + it('can prefer everyone over normal groups if necessary', async () => { + const result = await findHighestNotePermissionByGroup(user3, [ + permissionGroup3Read, + permissionGroupEveryoneWrite, + ]); + expect(result).toBe(NotePermission.WRITE); + }); + }); +}); diff --git a/backend/src/permissions/utils/find-highest-note-permission-by-group.ts b/backend/src/permissions/utils/find-highest-note-permission-by-group.ts new file mode 100644 index 000000000..310f14868 --- /dev/null +++ b/backend/src/permissions/utils/find-highest-note-permission-by-group.ts @@ -0,0 +1,62 @@ +/* + * SPDX-FileCopyrightText: 2023 The HedgeDoc developers (see AUTHORS file) + * + * SPDX-License-Identifier: AGPL-3.0-only + */ +import { Group } from '../../groups/group.entity'; +import { SpecialGroup } from '../../groups/groups.special'; +import { User } from '../../users/user.entity'; +import { NoteGroupPermission } from '../note-group-permission.entity'; +import { NotePermission } from '../note-permission.enum'; + +/** + * Inspects the given note permissions and finds the highest {@link NoteGroupPermission} for the given {@link Group}. + * + * @param user The group whose permissions should be determined + * @param groupPermissions The search basis + * @return The found permission or {@link NotePermission.DENY} if no permission could be found. + * @async + */ +export async function findHighestNotePermissionByGroup( + user: User, + groupPermissions: NoteGroupPermission[], +): Promise { + let highestGroupPermission = NotePermission.DENY; + for (const groupPermission of groupPermissions) { + const permission = await findNotePermissionByGroup(user, groupPermission); + if (permission === NotePermission.WRITE) { + return NotePermission.WRITE; + } + highestGroupPermission = + highestGroupPermission > permission ? highestGroupPermission : permission; + } + return highestGroupPermission; +} + +async function findNotePermissionByGroup( + user: User, + groupPermission: NoteGroupPermission, +): Promise { + const group = await groupPermission.group; + if (!isSpecialGroup(group) && !(await isUserInGroup(user, group))) { + return NotePermission.DENY; + } + return groupPermission.canEdit ? NotePermission.WRITE : NotePermission.READ; +} + +function isSpecialGroup(group: Group): boolean { + return ( + group.special && + (group.name === SpecialGroup.LOGGED_IN || + group.name === SpecialGroup.EVERYONE) + ); +} + +async function isUserInGroup(user: User, group: Group): Promise { + for (const member of await group.members) { + if (member.id === user.id) { + return true; + } + } + return false; +} diff --git a/backend/src/permissions/utils/find-highest-note-permission-by-user.spec.ts b/backend/src/permissions/utils/find-highest-note-permission-by-user.spec.ts new file mode 100644 index 000000000..c59f5b7c7 --- /dev/null +++ b/backend/src/permissions/utils/find-highest-note-permission-by-user.spec.ts @@ -0,0 +1,65 @@ +/* + * SPDX-FileCopyrightText: 2023 The HedgeDoc developers (see AUTHORS file) + * + * SPDX-License-Identifier: AGPL-3.0-only + */ +import { Mock } from 'ts-mockery'; + +import { User } from '../../users/user.entity'; +import { NotePermission } from '../note-permission.enum'; +import { NoteUserPermission } from '../note-user-permission.entity'; +import { findHighestNotePermissionByUser } from './find-highest-note-permission-by-user'; + +describe('find highest note permission by user', () => { + const user1 = Mock.of({ id: 0 }); + const user2 = Mock.of({ id: 1 }); + const user3 = Mock.of({ id: 2 }); + + const permissionUser2Read = Mock.of({ + user: Promise.resolve(user2), + canEdit: false, + }); + + const permissionUser3Read = Mock.of({ + user: Promise.resolve(user3), + canEdit: false, + }); + + const permissionUser3Write = Mock.of({ + user: Promise.resolve(user3), + canEdit: true, + }); + + it('will fallback to NONE if no permission for the user could be found', async () => { + const result = await findHighestNotePermissionByUser(user1, [ + permissionUser2Read, + permissionUser3Write, + ]); + expect(result).toBe(NotePermission.DENY); + }); + + it('can extract a READ permission for the correct user', async () => { + const result = await findHighestNotePermissionByUser(user2, [ + permissionUser2Read, + permissionUser3Write, + ]); + expect(result).toBe(NotePermission.READ); + }); + + it('can extract a WRITE permission for the correct user', async () => { + const result = await findHighestNotePermissionByUser(user3, [ + permissionUser2Read, + permissionUser3Write, + ]); + expect(result).toBe(NotePermission.WRITE); + }); + + it('can extract a WRITE permission for the correct user if read and write are defined', async () => { + const result = await findHighestNotePermissionByUser(user3, [ + permissionUser2Read, + permissionUser3Read, + permissionUser3Write, + ]); + expect(result).toBe(NotePermission.WRITE); + }); +}); diff --git a/backend/src/permissions/utils/find-highest-note-permission-by-user.ts b/backend/src/permissions/utils/find-highest-note-permission-by-user.ts new file mode 100644 index 000000000..67bb9a799 --- /dev/null +++ b/backend/src/permissions/utils/find-highest-note-permission-by-user.ts @@ -0,0 +1,35 @@ +/* + * SPDX-FileCopyrightText: 2023 The HedgeDoc developers (see AUTHORS file) + * + * SPDX-License-Identifier: AGPL-3.0-only + */ +import { User } from '../../users/user.entity'; +import { NotePermission } from '../note-permission.enum'; +import { NoteUserPermission } from '../note-user-permission.entity'; + +/** + * Inspects the given note permissions and finds the highest {@link NoteUserPermission} for the given {@link User}. + * + * @param user The user whose permissions should be determined + * @param userPermissions The search basis + * @return The found permission or {@link NotePermission.DENY} if no permission could be found. + * @async + */ +export async function findHighestNotePermissionByUser( + user: User, + userPermissions: NoteUserPermission[], +): Promise { + let hasReadPermission = false; + for (const userPermission of userPermissions) { + if ((await userPermission.user).id !== user.id) { + continue; + } + + if (userPermission.canEdit) { + return NotePermission.WRITE; + } + + hasReadPermission = true; + } + return hasReadPermission ? NotePermission.READ : NotePermission.DENY; +} diff --git a/backend/src/realtime/realtime-note/realtime-note.service.spec.ts b/backend/src/realtime/realtime-note/realtime-note.service.spec.ts index 723362269..2595bb55d 100644 --- a/backend/src/realtime/realtime-note/realtime-note.service.spec.ts +++ b/backend/src/realtime/realtime-note/realtime-note.service.spec.ts @@ -9,6 +9,7 @@ import { Mock } from 'ts-mockery'; import { AppConfig } from '../../config/app.config'; import { ConsoleLoggerService } from '../../logger/console-logger.service'; import { Note } from '../../notes/note.entity'; +import { NotePermission } from '../../permissions/note-permission.enum'; import { PermissionsService } from '../../permissions/permissions.service'; import { Revision } from '../../revisions/revision.entity'; import { RevisionsService } from '../../revisions/revisions.service'; @@ -91,9 +92,15 @@ describe('RealtimeNoteService', () => { mockedAppConfig = Mock.of({ persistInterval: 0 }); mockedPermissionService = Mock.of({ - mayRead: async (user: User) => - [readWriteUsername, onlyReadUsername].includes(user?.username), - mayWrite: async (user: User) => user?.username === readWriteUsername, + determinePermission: async (user: User | null) => { + if (user?.username === readWriteUsername) { + return NotePermission.WRITE; + } else if (user?.username === onlyReadUsername) { + return NotePermission.READ; + } else { + return NotePermission.DENY; + } + }, }); const schedulerRegistry = Mock.of({ diff --git a/backend/src/realtime/realtime-note/realtime-note.service.ts b/backend/src/realtime/realtime-note/realtime-note.service.ts index ad13139fe..d7900dd3b 100644 --- a/backend/src/realtime/realtime-note/realtime-note.service.ts +++ b/backend/src/realtime/realtime-note/realtime-note.service.ts @@ -12,6 +12,7 @@ import appConfiguration, { AppConfig } from '../../config/app.config'; import { NoteEvent } from '../../events'; import { ConsoleLoggerService } from '../../logger/console-logger.service'; import { Note } from '../../notes/note.entity'; +import { NotePermission } from '../../permissions/note-permission.enum'; import { PermissionsService } from '../../permissions/permissions.service'; import { RevisionsService } from '../../revisions/revisions.service'; import { RealtimeConnection } from './realtime-connection'; @@ -126,24 +127,18 @@ export class RealtimeNoteService implements BeforeApplicationShutdown { note: Note, ): Promise { for (const connection of connections) { - if (await this.connectionCanRead(connection, note)) { - connection.acceptEdits = await this.permissionService.mayWrite( - connection.getUser(), - note, - ); - } else { + const permission = await this.permissionService.determinePermission( + connection.getUser(), + note, + ); + if (permission === NotePermission.DENY) { connection.getTransporter().disconnect(); + } else { + connection.acceptEdits = permission > NotePermission.READ; } } } - private async connectionCanRead( - connection: RealtimeConnection, - note: Note, - ): Promise { - return await this.permissionService.mayRead(connection.getUser(), note); - } - @OnEvent(NoteEvent.DELETION) public handleNoteDeleted(noteId: Note['id']): void { const realtimeNote = this.realtimeNoteStore.find(noteId); diff --git a/backend/src/realtime/websocket/websocket.gateway.spec.ts b/backend/src/realtime/websocket/websocket.gateway.spec.ts index dafaafa48..9545e22c3 100644 --- a/backend/src/realtime/websocket/websocket.gateway.spec.ts +++ b/backend/src/realtime/websocket/websocket.gateway.spec.ts @@ -29,6 +29,7 @@ import { NotesModule } from '../../notes/notes.module'; import { NotesService } from '../../notes/notes.service'; import { Tag } from '../../notes/tag.entity'; import { NoteGroupPermission } from '../../permissions/note-group-permission.entity'; +import { NotePermission } from '../../permissions/note-permission.enum'; import { NoteUserPermission } from '../../permissions/note-user-permission.entity'; import { PermissionsModule } from '../../permissions/permissions.module'; import { PermissionsService } from '../../permissions/permissions.service'; @@ -221,15 +222,15 @@ describe('Websocket gateway', () => { }); jest - .spyOn(permissionsService, 'mayRead') + .spyOn(permissionsService, 'determinePermission') .mockImplementation( - (user: User | null, note: Note): Promise => - Promise.resolve( - (user === mockUser && - note === mockedNote && - userHasReadPermissions) || - (user === null && note === mockedGuestNote), - ), + async (user: User | null, note: Note): Promise => + (user === mockUser && + note === mockedNote && + userHasReadPermissions) || + (user === null && note === mockedGuestNote) + ? NotePermission.READ + : NotePermission.DENY, ); const mockedRealtimeNote = Mock.of({ diff --git a/backend/src/realtime/websocket/websocket.gateway.ts b/backend/src/realtime/websocket/websocket.gateway.ts index 5226f5130..7a55e5279 100644 --- a/backend/src/realtime/websocket/websocket.gateway.ts +++ b/backend/src/realtime/websocket/websocket.gateway.ts @@ -14,6 +14,7 @@ import WebSocket from 'ws'; import { ConsoleLoggerService } from '../../logger/console-logger.service'; import { NotesService } from '../../notes/notes.service'; +import { NotePermission } from '../../permissions/note-permission.enum'; import { PermissionsService } from '../../permissions/permissions.service'; import { SessionService } from '../../session/session.service'; import { User } from '../../users/user.entity'; @@ -59,7 +60,11 @@ export class WebsocketGateway implements OnGatewayConnection { const username = user?.username ?? 'guest'; - if (!(await this.permissionsService.mayRead(user, note))) { + const notePermission = await this.permissionsService.determinePermission( + user, + note, + ); + if (notePermission < NotePermission.READ) { //TODO: [mrdrogdrog] inform client about reason of disconnect. this.logger.log( `Access denied to note '${note.id}' for user '${username}'`,