From 410f3d4c33aa272bc5db2181338ad6ef837d8ccb Mon Sep 17 00:00:00 2001 From: Samuel Bachmann Date: Thu, 27 Nov 2025 19:44:02 +0100 Subject: [PATCH] prevent user self modification --- backend/src/modules/auth/auth.controller.ts | 10 ++++-- .../auth/services/user.service.spec.ts | 19 ++++++++++++ .../src/modules/auth/services/user.service.ts | 31 +++++++++++++++---- .../user-management.component.html | 4 +-- .../user-management.component.ts | 30 ++++++++++++++++-- 5 files changed, 80 insertions(+), 14 deletions(-) diff --git a/backend/src/modules/auth/auth.controller.ts b/backend/src/modules/auth/auth.controller.ts index d5b6d60..8fc1ea6 100644 --- a/backend/src/modules/auth/auth.controller.ts +++ b/backend/src/modules/auth/auth.controller.ts @@ -1214,8 +1214,9 @@ export class AuthController { async updateUserRole( @Param('id') id: string, @Body() dto: UpdateRoleDto, + @Req() request: Request & { user: { id: string } }, ): Promise { - return this.userService.updateRole(id, dto.role); + return this.userService.updateRole(id, dto.role, request.user.id); } /** @@ -1226,8 +1227,11 @@ export class AuthController { @Delete('users/:id') @UseGuards(AdminGuard) @HttpCode(HttpStatus.NO_CONTENT) - async deleteUser(@Param('id') id: string): Promise { - await this.userService.deleteUser(id); + async deleteUser( + @Param('id') id: string, + @Req() request: Request & { user: { id: string } }, + ): Promise { + await this.userService.deleteUser(id, request.user.id); } /** diff --git a/backend/src/modules/auth/services/user.service.spec.ts b/backend/src/modules/auth/services/user.service.spec.ts index 752184e..5ab79a9 100644 --- a/backend/src/modules/auth/services/user.service.spec.ts +++ b/backend/src/modules/auth/services/user.service.spec.ts @@ -142,6 +142,12 @@ describe('UserService', () => { 'Cannot demote the last local administrator', ); }); + + it('should throw BadRequestException when user tries to change their own role', async () => { + await expect(service.updateRole('1', UserRole.USER, '1')).rejects.toThrow( + 'Cannot modify your own role', + ); + }); }); describe('deleteUser', () => { @@ -207,6 +213,19 @@ describe('UserService', () => { // Note: The actual cascade deletion of refresh tokens is handled by the database // via the foreign key constraint with ON DELETE CASCADE }); + + it('should throw BadRequestException when user tries to delete themselves', async () => { + await expect(service.deleteUser('1', '1')).rejects.toThrow( + 'Cannot delete yourself', + ); + }); + + it('should throw BadRequestException when SAML user tries to delete themselves', async () => { + // This confirms the check is provider-agnostic + await expect(service.deleteUser('2', '2')).rejects.toThrow( + 'Cannot delete yourself', + ); + }); }); describe('countAdministrators', () => { diff --git a/backend/src/modules/auth/services/user.service.ts b/backend/src/modules/auth/services/user.service.ts index 5da2093..2de23ed 100644 --- a/backend/src/modules/auth/services/user.service.ts +++ b/backend/src/modules/auth/services/user.service.ts @@ -42,8 +42,12 @@ export class UserService { /** * Update a user's role */ - async updateRole(id: string, role: UserRole): Promise { - await this.validateRoleChange(id, role); + async updateRole( + id: string, + role: UserRole, + requestingUserId?: string, + ): Promise { + await this.validateRoleChange(id, role, requestingUserId); const user = await this.findById(id); user.role = role; @@ -61,8 +65,8 @@ export class UserService { /** * Delete a user */ - async deleteUser(id: string): Promise { - await this.validateUserDeletion(id); + async deleteUser(id: string, requestingUserId?: string): Promise { + await this.validateUserDeletion(id, requestingUserId); const user = await this.findById(id); await this.userRepository.remove(user); @@ -117,7 +121,15 @@ export class UserService { * Validate that a role change is allowed * Throws an error if the change would violate last admin protection */ - async validateRoleChange(userId: string, newRole: UserRole): Promise { + async validateRoleChange( + userId: string, + newRole: UserRole, + requestingUserId?: string, + ): Promise { + if (requestingUserId && userId === requestingUserId) { + throw new BadRequestException('Cannot modify your own role'); + } + if (newRole === UserRole.USER) { const isLastLocal = await this.isLastLocalAdministrator(userId); if (isLastLocal) { @@ -132,7 +144,14 @@ export class UserService { * Validate that a user deletion is allowed * Throws an error if the deletion would violate last admin protection */ - async validateUserDeletion(userId: string): Promise { + async validateUserDeletion( + userId: string, + requestingUserId?: string, + ): Promise { + if (requestingUserId && userId === requestingUserId) { + throw new BadRequestException('Cannot delete yourself'); + } + const isLastLocal = await this.isLastLocalAdministrator(userId); if (isLastLocal) { throw new BadRequestException( diff --git a/frontend/src/app/pages/user-management/user-management.component.html b/frontend/src/app/pages/user-management/user-management.component.html index af5ed57..186686d 100644 --- a/frontend/src/app/pages/user-management/user-management.component.html +++ b/frontend/src/app/pages/user-management/user-management.component.html @@ -63,7 +63,7 @@ [value]="user.role" (selectionChange)="changeUserRole(user, $event.value)" [disabled]="!canModifyUser(user)" - [matTooltip]="isLastAdmin(user) ? 'Cannot modify last administrator' : 'Change role'" + [matTooltip]="getModifyTooltip(user)" class="role-select" > User @@ -76,7 +76,7 @@ color="warn" (click)="deleteUser(user)" [disabled]="!canModifyUser(user)" - [matTooltip]="isLastAdmin(user) ? 'Cannot delete last administrator' : 'Delete user'" + [matTooltip]="getDeleteTooltip(user)" > delete diff --git a/frontend/src/app/pages/user-management/user-management.component.ts b/frontend/src/app/pages/user-management/user-management.component.ts index 38eef31..513deb2 100644 --- a/frontend/src/app/pages/user-management/user-management.component.ts +++ b/frontend/src/app/pages/user-management/user-management.component.ts @@ -44,6 +44,7 @@ export class UserManagementComponent implements OnInit { users = signal([]); invites = signal([]); currentUser = signal(null); + loggedInUserId = signal(null); loading = signal(false); invitesLoading = signal(false); passwordLoginAllowed = signal(true); @@ -107,7 +108,8 @@ export class UserManagementComponent implements OnInit { this.authService.getCurrentUser().subscribe({ next: (user) => { if (user) { - // Find the full user details from the users list + this.loggedInUserId.set(user.id); + // Find the full user details from the users list if available const fullUser = this.users().find((u) => u.id === user.id); if (fullUser) { this.currentUser.set(fullUser); @@ -279,8 +281,8 @@ export class UserManagementComponent implements OnInit { canModifyUser(user: UserResponse): boolean { // Can't modify yourself - const current = this.currentUser(); - if (current && current.id === user.id) { + const currentId = this.loggedInUserId(); + if (currentId && currentId === user.id) { return false; } @@ -327,4 +329,26 @@ export class UserManagementComponent implements OnInit { const hoursUntilExpiry = (expires.getTime() - now.getTime()) / (1000 * 60 * 60); return hoursUntilExpiry < 24 && hoursUntilExpiry > 0; } + + getModifyTooltip(user: UserResponse): string { + const currentId = this.loggedInUserId(); + if (currentId && currentId === user.id) { + return 'Cannot modify your own role'; + } + if (this.isLastAdmin(user)) { + return 'Cannot modify last administrator'; + } + return 'Change role'; + } + + getDeleteTooltip(user: UserResponse): string { + const currentId = this.loggedInUserId(); + if (currentId && currentId === user.id) { + return 'Cannot delete yourself'; + } + if (this.isLastAdmin(user)) { + return 'Cannot delete last administrator'; + } + return 'Delete user'; + } }