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 <git@tilmanvatteroth.de>

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);
+  });
+});

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';
+  }
+}

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<boolean> {
-    const permission = this.reflector.get<RequiredPermission>(
-      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<RequiredPermission>(
+      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<RequiredPermission, RequiredPermission.CREATE>,
+    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;
+    }
+  }
 }

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<NoteEventMap>,
   ) {}
-  /**
-   * 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<RequiredPermission, RequiredPermission.CREATE>,
-    user: User | null,
-    note: Note,
-  ): Promise<boolean> {
-    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<boolean> {
-    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<boolean> {
-    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<boolean> {
-    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<boolean> {
     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<NotePermission>} The determined permission
+   */
+  public async determinePermission(
     user: User | null,
     note: Note,
-    wantEdit: boolean,
-  ): Promise<boolean> {
+  ): Promise<NotePermission> {
     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<NotePermission> {
+    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 {

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,
+    );
+  });
+});

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;
+  }
+}

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<User>({ id: 0 });
+  const user2 = Mock.of<User>({ id: 1 });
+  const user3 = Mock.of<User>({ id: 2 });
+  const group2 = Mock.of<Group>({
+    id: 1,
+    special: false,
+    members: Promise.resolve([user2]),
+  });
+  const group3 = Mock.of<Group>({
+    id: 2,
+    special: false,
+    members: Promise.resolve([user3]),
+  });
+
+  const permissionGroup2Read = Mock.of<NoteGroupPermission>({
+    group: Promise.resolve(group2),
+    canEdit: false,
+  });
+
+  const permissionGroup3Read = Mock.of<NoteGroupPermission>({
+    group: Promise.resolve(group3),
+    canEdit: false,
+  });
+
+  const permissionGroup3Write = Mock.of<NoteGroupPermission>({
+    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<Group>({
+      id: 3,
+      special: true,
+      name: SpecialGroup.EVERYONE,
+    });
+    const groupLoggedIn = Mock.of<Group>({
+      id: 4,
+      special: true,
+      name: SpecialGroup.LOGGED_IN,
+    });
+    const permissionGroupEveryoneRead = Mock.of<NoteGroupPermission>({
+      group: Promise.resolve(groupEveryone),
+      canEdit: false,
+    });
+    const permissionGroupLoggedInRead = Mock.of<NoteGroupPermission>({
+      group: Promise.resolve(groupLoggedIn),
+      canEdit: false,
+    });
+    const permissionGroupEveryoneWrite = Mock.of<NoteGroupPermission>({
+      group: Promise.resolve(groupEveryone),
+      canEdit: true,
+    });
+    const permissionGroupLoggedInWrite = Mock.of<NoteGroupPermission>({
+      group: Promise.resolve(groupLoggedIn),
+      canEdit: true,
+    });
+
+    it('will ignore unknown special groups', async () => {
+      const nonsenseSpecialGroup = Mock.of<Group>({
+        id: 99,
+        special: true,
+        name: 'Unknown Special Group',
+        members: Promise.resolve([]),
+      });
+
+      const permissionUnknownSpecialGroup = Mock.of<NoteGroupPermission>({
+        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);
+    });
+  });
+});

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<NotePermission.DENY | NotePermission.READ | NotePermission.WRITE> {
+  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<NotePermission.DENY | NotePermission.READ | NotePermission.WRITE> {
+  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<boolean> {
+  for (const member of await group.members) {
+    if (member.id === user.id) {
+      return true;
+    }
+  }
+  return false;
+}

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<User>({ id: 0 });
+  const user2 = Mock.of<User>({ id: 1 });
+  const user3 = Mock.of<User>({ id: 2 });
+
+  const permissionUser2Read = Mock.of<NoteUserPermission>({
+    user: Promise.resolve(user2),
+    canEdit: false,
+  });
+
+  const permissionUser3Read = Mock.of<NoteUserPermission>({
+    user: Promise.resolve(user3),
+    canEdit: false,
+  });
+
+  const permissionUser3Write = Mock.of<NoteUserPermission>({
+    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);
+  });
+});

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<NotePermission.DENY | NotePermission.READ | NotePermission.WRITE> {
+  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;
+}

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<AppConfig>({ persistInterval: 0 });
     mockedPermissionService = Mock.of<PermissionsService>({
-      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<SchedulerRegistry>({

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<void> {
     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<boolean> {
-    return await this.permissionService.mayRead(connection.getUser(), note);
-  }
-
   @OnEvent(NoteEvent.DELETION)
   public handleNoteDeleted(noteId: Note['id']): void {
     const realtimeNote = this.realtimeNoteStore.find(noteId);

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<boolean> =>
-          Promise.resolve(
-            (user === mockUser &&
-              note === mockedNote &&
-              userHasReadPermissions) ||
-              (user === null && note === mockedGuestNote),
-          ),
+        async (user: User | null, note: Note): Promise<NotePermission> =>
+          (user === mockUser &&
+            note === mockedNote &&
+            userHasReadPermissions) ||
+          (user === null && note === mockedGuestNote)
+            ? NotePermission.READ
+            : NotePermission.DENY,
       );
 
     const mockedRealtimeNote = Mock.of<RealtimeNote>({

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}'`,