From c316e52e65ae75d3de9dfdcd211d133df98b8dfc Mon Sep 17 00:00:00 2001 From: neon <18559658+neonsolstice@users.noreply.github.com> Date: Mon, 14 Sep 2026 04:58:40 +0000 Subject: [PATCH] fix(auth): honor delegated admin permissions Allow manage_users administrators to manage user content filters and allow manage_email administrators to share providers they own. Preserve provider ownership and keep system mail designation restricted to superusers. Fixes #1401 --- .../email/email-provider.service.test.ts | 44 +++++++++++++-- .../modules/email/email-provider.service.ts | 5 +- server/src/modules/user/user.service.test.ts | 55 ++++++++++++++++--- server/src/modules/user/user.service.ts | 16 ++++-- 4 files changed, 102 insertions(+), 18 deletions(-) diff --git a/server/src/modules/email/email-provider.service.test.ts b/server/src/modules/email/email-provider.service.test.ts index 1bdfd5ba2..17b2696ac 100644 --- a/server/src/modules/email/email-provider.service.test.ts +++ b/server/src/modules/email/email-provider.service.test.ts @@ -4,7 +4,7 @@ import { EmailProviderRepository } from './email-provider.repository'; import { EmailEncryptionService } from './email-encryption.service'; import { EmailTransportService } from './email-transport.service'; import type { RequestUser } from '../../common/types/request-user'; -import { EMPTY_CONTENT_FILTER_RULES } from '@bookorbit/types'; +import { EMPTY_CONTENT_FILTER_RULES, Permission } from '@bookorbit/types'; describe('EmailProviderService', () => { let service: EmailProviderService; @@ -36,6 +36,11 @@ describe('EmailProviderService', () => { contentFilters: EMPTY_CONTENT_FILTER_RULES, }; + const mockEmailManager: RequestUser = { + ...mockUser, + permissions: [Permission.ManageEmail], + }; + const mockProvider = { id: 10, userId: 1, @@ -62,6 +67,8 @@ describe('EmailProviderService', () => { clearDefault: vi.fn(), setDefault: vi.fn().mockResolvedValue([mockProvider]), setSharedByOwner: vi.fn().mockResolvedValue([{ ...mockProvider, isShared: true }]), + setSystemProvider: vi.fn().mockResolvedValue({ ...mockProvider, isSystemProvider: true }), + clearSystemProvider: vi.fn(), } as unknown as EmailProviderRepository; encryption = { @@ -186,14 +193,43 @@ describe('EmailProviderService', () => { }); describe('toggleShared', () => { - it('should allow superuser to toggle sharing', async () => { + it('allows a superuser to toggle sharing', async () => { const result = await service.toggleShared(10, mockAdmin); expect(result.isShared).toBe(true); - expect(repo.setSharedByOwner).toHaveBeenCalled(); + expect(repo.setSharedByOwner).toHaveBeenCalledWith(10, mockAdmin.id, true); }); - it('should throw ForbiddenException for non-superuser', async () => { + it('allows a manage_email user to toggle sharing on an owned provider', async () => { + const result = await service.toggleShared(10, mockEmailManager); + + expect(result.isShared).toBe(true); + expect(repo.setSharedByOwner).toHaveBeenCalledWith(10, mockEmailManager.id, true); + }); + + it('rejects a caller without manage_email before looking up the provider', async () => { await expect(service.toggleShared(10, mockUser)).rejects.toThrow(ForbiddenException); + expect(repo.findById).not.toHaveBeenCalled(); + expect(repo.setSharedByOwner).not.toHaveBeenCalled(); + }); + + it('does not let manage_email bypass provider ownership', async () => { + (repo.findById as vi.Mock).mockResolvedValue([{ ...mockProvider, userId: 99 }]); + + await expect(service.toggleShared(10, mockEmailManager)).rejects.toThrow(ForbiddenException); + expect(repo.setSharedByOwner).not.toHaveBeenCalled(); + }); + }); + + describe('system provider authorization', () => { + it('keeps setting the system provider restricted to superusers', async () => { + await expect(service.setSystemProvider(10, mockEmailManager)).rejects.toThrow(ForbiddenException); + expect(repo.findById).not.toHaveBeenCalled(); + expect(repo.setSystemProvider).not.toHaveBeenCalled(); + }); + + it('keeps clearing the system provider restricted to superusers', async () => { + await expect(service.clearSystemProvider(mockEmailManager)).rejects.toThrow(ForbiddenException); + expect(repo.clearSystemProvider).not.toHaveBeenCalled(); }); }); diff --git a/server/src/modules/email/email-provider.service.ts b/server/src/modules/email/email-provider.service.ts index de48b33f9..dc04594ff 100644 --- a/server/src/modules/email/email-provider.service.ts +++ b/server/src/modules/email/email-provider.service.ts @@ -1,4 +1,5 @@ import { BadRequestException, ConflictException, ForbiddenException, Injectable, NotFoundException } from '@nestjs/common'; +import { Permission } from '@bookorbit/types'; import type { RequestUser } from '../../common/types/request-user'; import { emailProviders } from '../../db/schema'; @@ -100,7 +101,9 @@ export class EmailProviderService { } async toggleShared(id: number, user: RequestUser) { - if (!user.isSuperuser) throw new ForbiddenException('Only superusers can share providers'); + if (!user.isSuperuser && !user.permissions.includes(Permission.ManageEmail)) { + throw new ForbiddenException(`Missing permission: ${Permission.ManageEmail}`); + } const provider = await this.getOwned(id, user); const [updated] = await this.repo.setSharedByOwner(id, user.id, !provider.isShared); if (!updated) throw new NotFoundException('Provider not found'); diff --git a/server/src/modules/user/user.service.test.ts b/server/src/modules/user/user.service.test.ts index 79af10088..7d4774ab3 100644 --- a/server/src/modules/user/user.service.test.ts +++ b/server/src/modules/user/user.service.test.ts @@ -657,10 +657,27 @@ describe('UserService', () => { expect(contentFilterRepo.findByUserIdWithNames).toHaveBeenCalledWith(5); }); - it('getContentFilters blocks non-superusers from reading another user filters', async () => { + it('getContentFilters allows manage_users to read another user filters', async () => { + const filters = { + includeTags: [{ id: 1, name: 'Sci-Fi' }], + excludeTags: [], + includeGenres: [], + excludeGenres: [], + }; userRepo.findByIdWithPermissions.mockResolvedValue({ id: 5, isSuperuser: false }); + contentFilterRepo.findByUserIdWithNames.mockResolvedValue(filters); - await expect(service.getContentFilters(5, reqUser({ id: 1, isSuperuser: false }))).rejects.toBeInstanceOf(ForbiddenException); + await expect(service.getContentFilters(5, reqUser({ id: 1, isSuperuser: false, permissions: [Permission.ManageUsers] }))).resolves.toEqual( + filters, + ); + expect(contentFilterRepo.findByUserIdWithNames).toHaveBeenCalledWith(5); + }); + + it('getContentFilters blocks callers without manage_users from reading another user filters', async () => { + await expect( + service.getContentFilters(5, reqUser({ id: 1, isSuperuser: false, permissions: [Permission.LibraryDownload] })), + ).rejects.toBeInstanceOf(ForbiddenException); + expect(userRepo.findByIdWithPermissions).not.toHaveBeenCalled(); expect(contentFilterRepo.findByUserIdWithNames).not.toHaveBeenCalled(); }); @@ -687,17 +704,41 @@ describe('UserService', () => { await expect(service.setContentFilters(5, {} as any, reqUser({ isSuperuser: true }))).rejects.toBeInstanceOf(NotFoundException); }); - it('setContentFilters blocks non-superusers from updating filters', async () => { + it('setContentFilters allows manage_users to update filters', async () => { userRepo.findByIdWithPermissions.mockResolvedValue({ id: 5, isSuperuser: false }); - await expect(service.setContentFilters(5, {} as any, reqUser({ isSuperuser: false }))).rejects.toBeInstanceOf(ForbiddenException); - expect(contentFilterRepo.replaceFilters).not.toHaveBeenCalled(); + await expect( + service.setContentFilters( + 5, + { includeTagIds: [1], excludeGenreIds: [3], seeOwnRequestedBooks: true } as any, + reqUser({ isSuperuser: false, permissions: [Permission.ManageUsers] }), + ), + ).resolves.toBeUndefined(); + + expect(contentFilterRepo.replaceFilters).toHaveBeenCalledWith(5, { + includeTagIds: [1], + excludeTagIds: [], + includeGenreIds: [], + excludeGenreIds: [3], + }); + expect(userRepo.update).toHaveBeenCalledWith(5, { seeOwnRequestedBooks: true }); }); - it('setContentFilters rejects administrator targets', async () => { + it('setContentFilters blocks callers without manage_users from updating filters', async () => { + await expect( + service.setContentFilters(5, {} as any, reqUser({ isSuperuser: false, permissions: [Permission.LibraryDownload] })), + ).rejects.toBeInstanceOf(ForbiddenException); + expect(userRepo.findByIdWithPermissions).not.toHaveBeenCalled(); + expect(contentFilterRepo.replaceFilters).not.toHaveBeenCalled(); + expect(userRepo.update).not.toHaveBeenCalled(); + }); + + it('setContentFilters rejects superuser targets even for manage_users callers', async () => { userRepo.findByIdWithPermissions.mockResolvedValue({ id: 5, isSuperuser: true }); - await expect(service.setContentFilters(5, {} as any, reqUser({ isSuperuser: true }))).rejects.toBeInstanceOf(BadRequestException); + await expect( + service.setContentFilters(5, {} as any, reqUser({ isSuperuser: false, permissions: [Permission.ManageUsers] })), + ).rejects.toBeInstanceOf(BadRequestException); expect(contentFilterRepo.replaceFilters).not.toHaveBeenCalled(); }); diff --git a/server/src/modules/user/user.service.ts b/server/src/modules/user/user.service.ts index 2af29fe0c..4b84b1d7f 100644 --- a/server/src/modules/user/user.service.ts +++ b/server/src/modules/user/user.service.ts @@ -522,20 +522,20 @@ export class UserService { } async getContentFilters(targetUserId: number, requestingUser: RequestUser) { - const target = await this.userRepo.findByIdWithPermissions(targetUserId); - if (!target) throw new NotFoundException('User not found'); - if (targetUserId !== requestingUser.id && !requestingUser.isSuperuser) { + if (targetUserId !== requestingUser.id && !this.canManageUsers(requestingUser)) { throw new ForbiddenException('Cannot view another user content filters'); } + const target = await this.userRepo.findByIdWithPermissions(targetUserId); + if (!target) throw new NotFoundException('User not found'); return this.contentFilterRepo.findByUserIdWithNames(targetUserId); } async setContentFilters(targetUserId: number, dto: SetContentFiltersDto, requestingUser: RequestUser) { + if (!this.canManageUsers(requestingUser)) { + throw new ForbiddenException(`Missing permission: ${Permission.ManageUsers}`); + } const target = await this.userRepo.findByIdWithPermissions(targetUserId); if (!target) throw new NotFoundException('User not found'); - if (!requestingUser.isSuperuser) { - throw new ForbiddenException('Only administrators can set content filters'); - } if (target.isSuperuser) { throw new BadRequestException('Content filters cannot be applied to administrators'); } @@ -551,4 +551,8 @@ export class UserService { await this.userRepo.update(targetUserId, { seeOwnRequestedBooks: dto.seeOwnRequestedBooks }); } } + + private canManageUsers(user: RequestUser): boolean { + return user.isSuperuser || user.permissions.includes(Permission.ManageUsers); + } }