diff --git a/e2e/src/api/specs/user-admin.e2e-spec.ts b/e2e/src/api/specs/user-admin.e2e-spec.ts index 1fbee84c3f..b0696dcada 100644 --- a/e2e/src/api/specs/user-admin.e2e-spec.ts +++ b/e2e/src/api/specs/user-admin.e2e-spec.ts @@ -118,7 +118,7 @@ describe('/admin/users', () => { }); } - it('should ignore `isAdmin`', async () => { + it('should accept `isAdmin`', async () => { const { status, body } = await request(app) .post(`/admin/users`) .send({ @@ -130,7 +130,7 @@ describe('/admin/users', () => { .set('Authorization', `Bearer ${admin.accessToken}`); expect(body).toMatchObject({ email: 'user5@immich.cloud', - isAdmin: false, + isAdmin: true, shouldChangePassword: true, }); expect(status).toBe(201); @@ -163,14 +163,15 @@ describe('/admin/users', () => { }); } - it('should not allow a non-admin to become an admin', async () => { + it('should allow a non-admin to become an admin', async () => { + const user = await utils.userSetup(admin.accessToken, createUserDto.create('admin2')); const { status, body } = await request(app) - .put(`/admin/users/${nonAdmin.userId}`) + .put(`/admin/users/${user.userId}`) .send({ isAdmin: true }) .set('Authorization', `Bearer ${admin.accessToken}`); expect(status).toBe(200); - expect(body).toMatchObject({ isAdmin: false }); + expect(body).toMatchObject({ isAdmin: true }); }); it('ignores updates to profileImagePath', async () => { diff --git a/e2e/src/immich-admin/specs/immich-admin.e2e-spec.ts b/e2e/src/immich-admin/specs/immich-admin.e2e-spec.ts index cf0558883a..24699cda30 100644 --- a/e2e/src/immich-admin/specs/immich-admin.e2e-spec.ts +++ b/e2e/src/immich-admin/specs/immich-admin.e2e-spec.ts @@ -7,6 +7,44 @@ describe(`immich-admin`, () => { await utils.adminSetup(); }); + describe('revoke-admin', () => { + it('should revoke admin privileges from a user', async () => { + const { child, promise } = immichAdmin(['revoke-admin']); + + let data = ''; + child.stdout.on('data', (chunk) => { + data += chunk; + if (data.includes('Please enter the user email:')) { + child.stdin.end('admin@immich.cloud\n'); + } + }); + + const { stdout, exitCode } = await promise; + expect(exitCode).toBe(0); + + expect(stdout).toContain('Admin access has been revoked from'); + }); + }); + + describe('grant-admin', () => { + it('should grant admin privileges to a user', async () => { + const { child, promise } = immichAdmin(['grant-admin']); + + let data = ''; + child.stdout.on('data', (chunk) => { + data += chunk; + if (data.includes('Please enter the user email:')) { + child.stdin.end('admin@immich.cloud\n'); + } + }); + + const { stdout, exitCode } = await promise; + expect(exitCode).toBe(0); + + expect(stdout).toContain('Admin access has been granted to'); + }); + }); + describe('list-users', () => { it('should list the admin user', async () => { const { stdout, exitCode } = await immichAdmin(['list-users']).promise; diff --git a/e2e/src/web/specs/user-admin.e2e-spec.ts b/e2e/src/web/specs/user-admin.e2e-spec.ts new file mode 100644 index 0000000000..3d64e47aef --- /dev/null +++ b/e2e/src/web/specs/user-admin.e2e-spec.ts @@ -0,0 +1,89 @@ +import { getUserAdmin } from '@immich/sdk'; +import { expect, test } from '@playwright/test'; +import { asBearerAuth, utils } from 'src/utils'; + +test.describe('User Administration', () => { + test.beforeAll(() => { + utils.initSdk(); + }); + + test.beforeEach(async () => { + await utils.resetDatabase(); + }); + + test('validate admin/users link', async ({ context, page }) => { + const admin = await utils.adminSetup(); + await utils.setAuthCookies(context, admin.accessToken); + + // Navigate to user management page and verify title and header + await page.goto(`/admin/users`); + await expect(page).toHaveTitle(/User Management/); + await expect(page.getByText('User Management')).toBeVisible(); + }); + + test('create user', async ({ context, page }) => { + const admin = await utils.adminSetup(); + await utils.setAuthCookies(context, admin.accessToken); + + // Create a new user + await page.goto('/admin/users'); + await page.getByRole('button', { name: 'Create user' }).click(); + await page.getByLabel('Email').fill('user@immich.cloud'); + await page.getByLabel('Password', { exact: true }).fill('password'); + await page.getByLabel('Confirm Password').fill('password'); + await page.getByLabel('Name').fill('Immich User'); + await page.getByRole('button', { name: 'Create', exact: true }).click(); + + // Verify the user exists in the user list + await page.getByRole('row', { name: 'user@immich.cloud' }); + }); + + test('promote to admin', async ({ context, page }) => { + const admin = await utils.adminSetup(); + await utils.setAuthCookies(context, admin.accessToken); + + const user = await utils.userSetup(admin.accessToken, { + name: 'Admin 2', + email: 'admin2@immich.cloud', + password: 'password', + }); + + expect(user.isAdmin).toBe(false); + + await page.goto(`/admin/users/${user.userId}`); + + await page.getByRole('button', { name: 'Edit user' }).click(); + await expect(page.getByLabel('Admin User')).not.toBeChecked(); + await page.getByText('Admin User').click(); + await expect(page.getByLabel('Admin User')).toBeChecked(); + await page.getByRole('button', { name: 'Confirm' }).click(); + + const updated = await getUserAdmin({ id: user.userId }, { headers: asBearerAuth(admin.accessToken) }); + expect(updated.isAdmin).toBe(true); + }); + + test('revoke admin access', async ({ context, page }) => { + const admin = await utils.adminSetup(); + await utils.setAuthCookies(context, admin.accessToken); + + const user = await utils.userSetup(admin.accessToken, { + name: 'Admin 2', + email: 'admin2@immich.cloud', + password: 'password', + isAdmin: true, + }); + + expect(user.isAdmin).toBe(true); + + await page.goto(`/admin/users/${user.userId}`); + + await page.getByRole('button', { name: 'Edit user' }).click(); + await expect(page.getByLabel('Admin User')).toBeChecked(); + await page.getByText('Admin User').click(); + await expect(page.getByLabel('Admin User')).not.toBeChecked(); + await page.getByRole('button', { name: 'Confirm' }).click(); + + const updated = await getUserAdmin({ id: user.userId }, { headers: asBearerAuth(admin.accessToken) }); + expect(updated.isAdmin).toBe(false); + }); +}); diff --git a/i18n/en.json b/i18n/en.json index 86de3808ba..2735393d33 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -34,6 +34,7 @@ "added_to_favorites_count": "Added {count, number} to favorites", "admin": { "add_exclusion_pattern_description": "Add exclusion patterns. Globbing using *, **, and ? is supported. To ignore all files in any directory named \"Raw\", use \"**/Raw/**\". To ignore all files ending in \".tif\", use \"**/*.tif\". To ignore an absolute path, use \"/path/to/ignore/**\".", + "admin_user": "Admin User", "asset_offline_description": "This external library asset is no longer found on disk and has been moved to trash. If the file was moved within the library, check your timeline for the new corresponding asset. To restore this asset, please ensure that the file path below can be accessed by Immich and scan the library.", "authentication_settings": "Authentication Settings", "authentication_settings_description": "Manage password, OAuth, and other authentication settings", diff --git a/mobile/openapi/lib/model/user_admin_create_dto.dart b/mobile/openapi/lib/model/user_admin_create_dto.dart index 1477c82ca1..8c8b70fbce 100644 --- a/mobile/openapi/lib/model/user_admin_create_dto.dart +++ b/mobile/openapi/lib/model/user_admin_create_dto.dart @@ -15,6 +15,7 @@ class UserAdminCreateDto { UserAdminCreateDto({ this.avatarColor, required this.email, + this.isAdmin, required this.name, this.notify, required this.password, @@ -27,6 +28,14 @@ class UserAdminCreateDto { String email; + /// + /// Please note: This property should have been non-nullable! Since the specification file + /// does not include a default value (using the "default:" property), however, the generated + /// source code must fall back to having a nullable type. + /// Consider adding a "default:" property in the specification file to hide this note. + /// + bool? isAdmin; + String name; /// @@ -56,6 +65,7 @@ class UserAdminCreateDto { bool operator ==(Object other) => identical(this, other) || other is UserAdminCreateDto && other.avatarColor == avatarColor && other.email == email && + other.isAdmin == isAdmin && other.name == name && other.notify == notify && other.password == password && @@ -68,6 +78,7 @@ class UserAdminCreateDto { // ignore: unnecessary_parenthesis (avatarColor == null ? 0 : avatarColor!.hashCode) + (email.hashCode) + + (isAdmin == null ? 0 : isAdmin!.hashCode) + (name.hashCode) + (notify == null ? 0 : notify!.hashCode) + (password.hashCode) + @@ -76,7 +87,7 @@ class UserAdminCreateDto { (storageLabel == null ? 0 : storageLabel!.hashCode); @override - String toString() => 'UserAdminCreateDto[avatarColor=$avatarColor, email=$email, name=$name, notify=$notify, password=$password, quotaSizeInBytes=$quotaSizeInBytes, shouldChangePassword=$shouldChangePassword, storageLabel=$storageLabel]'; + String toString() => 'UserAdminCreateDto[avatarColor=$avatarColor, email=$email, isAdmin=$isAdmin, name=$name, notify=$notify, password=$password, quotaSizeInBytes=$quotaSizeInBytes, shouldChangePassword=$shouldChangePassword, storageLabel=$storageLabel]'; Map toJson() { final json = {}; @@ -86,6 +97,11 @@ class UserAdminCreateDto { // json[r'avatarColor'] = null; } json[r'email'] = this.email; + if (this.isAdmin != null) { + json[r'isAdmin'] = this.isAdmin; + } else { + // json[r'isAdmin'] = null; + } json[r'name'] = this.name; if (this.notify != null) { json[r'notify'] = this.notify; @@ -122,6 +138,7 @@ class UserAdminCreateDto { return UserAdminCreateDto( avatarColor: UserAvatarColor.fromJson(json[r'avatarColor']), email: mapValueOfType(json, r'email')!, + isAdmin: mapValueOfType(json, r'isAdmin'), name: mapValueOfType(json, r'name')!, notify: mapValueOfType(json, r'notify'), password: mapValueOfType(json, r'password')!, diff --git a/mobile/openapi/lib/model/user_admin_update_dto.dart b/mobile/openapi/lib/model/user_admin_update_dto.dart index ee5c006840..9605552d20 100644 --- a/mobile/openapi/lib/model/user_admin_update_dto.dart +++ b/mobile/openapi/lib/model/user_admin_update_dto.dart @@ -15,6 +15,7 @@ class UserAdminUpdateDto { UserAdminUpdateDto({ this.avatarColor, this.email, + this.isAdmin, this.name, this.password, this.pinCode, @@ -33,6 +34,14 @@ class UserAdminUpdateDto { /// String? email; + /// + /// Please note: This property should have been non-nullable! Since the specification file + /// does not include a default value (using the "default:" property), however, the generated + /// source code must fall back to having a nullable type. + /// Consider adding a "default:" property in the specification file to hide this note. + /// + bool? isAdmin; + /// /// Please note: This property should have been non-nullable! Since the specification file /// does not include a default value (using the "default:" property), however, the generated @@ -68,6 +77,7 @@ class UserAdminUpdateDto { bool operator ==(Object other) => identical(this, other) || other is UserAdminUpdateDto && other.avatarColor == avatarColor && other.email == email && + other.isAdmin == isAdmin && other.name == name && other.password == password && other.pinCode == pinCode && @@ -80,6 +90,7 @@ class UserAdminUpdateDto { // ignore: unnecessary_parenthesis (avatarColor == null ? 0 : avatarColor!.hashCode) + (email == null ? 0 : email!.hashCode) + + (isAdmin == null ? 0 : isAdmin!.hashCode) + (name == null ? 0 : name!.hashCode) + (password == null ? 0 : password!.hashCode) + (pinCode == null ? 0 : pinCode!.hashCode) + @@ -88,7 +99,7 @@ class UserAdminUpdateDto { (storageLabel == null ? 0 : storageLabel!.hashCode); @override - String toString() => 'UserAdminUpdateDto[avatarColor=$avatarColor, email=$email, name=$name, password=$password, pinCode=$pinCode, quotaSizeInBytes=$quotaSizeInBytes, shouldChangePassword=$shouldChangePassword, storageLabel=$storageLabel]'; + String toString() => 'UserAdminUpdateDto[avatarColor=$avatarColor, email=$email, isAdmin=$isAdmin, name=$name, password=$password, pinCode=$pinCode, quotaSizeInBytes=$quotaSizeInBytes, shouldChangePassword=$shouldChangePassword, storageLabel=$storageLabel]'; Map toJson() { final json = {}; @@ -102,6 +113,11 @@ class UserAdminUpdateDto { } else { // json[r'email'] = null; } + if (this.isAdmin != null) { + json[r'isAdmin'] = this.isAdmin; + } else { + // json[r'isAdmin'] = null; + } if (this.name != null) { json[r'name'] = this.name; } else { @@ -146,6 +162,7 @@ class UserAdminUpdateDto { return UserAdminUpdateDto( avatarColor: UserAvatarColor.fromJson(json[r'avatarColor']), email: mapValueOfType(json, r'email'), + isAdmin: mapValueOfType(json, r'isAdmin'), name: mapValueOfType(json, r'name'), password: mapValueOfType(json, r'password'), pinCode: mapValueOfType(json, r'pinCode'), diff --git a/open-api/immich-openapi-specs.json b/open-api/immich-openapi-specs.json index 4f0fa44fa4..8318756295 100644 --- a/open-api/immich-openapi-specs.json +++ b/open-api/immich-openapi-specs.json @@ -15131,6 +15131,9 @@ "format": "email", "type": "string" }, + "isAdmin": { + "type": "boolean" + }, "name": { "type": "string" }, @@ -15281,6 +15284,9 @@ "format": "email", "type": "string" }, + "isAdmin": { + "type": "boolean" + }, "name": { "type": "string" }, diff --git a/open-api/typescript-sdk/src/fetch-client.ts b/open-api/typescript-sdk/src/fetch-client.ts index 417055e484..8dced80394 100644 --- a/open-api/typescript-sdk/src/fetch-client.ts +++ b/open-api/typescript-sdk/src/fetch-client.ts @@ -109,6 +109,7 @@ export type UserAdminResponseDto = { export type UserAdminCreateDto = { avatarColor?: (UserAvatarColor) | null; email: string; + isAdmin?: boolean; name: string; notify?: boolean; password: string; @@ -122,6 +123,7 @@ export type UserAdminDeleteDto = { export type UserAdminUpdateDto = { avatarColor?: (UserAvatarColor) | null; email?: string; + isAdmin?: boolean; name?: string; password?: string; pinCode?: string | null; diff --git a/server/src/commands/grant-admin.ts b/server/src/commands/grant-admin.ts new file mode 100644 index 0000000000..41fd4f2340 --- /dev/null +++ b/server/src/commands/grant-admin.ts @@ -0,0 +1,67 @@ +import { Command, CommandRunner, InquirerService, Question, QuestionSet } from 'nest-commander'; +import { CliService } from 'src/services/cli.service'; + +const prompt = (inquirer: InquirerService) => { + return function ask(): Promise { + return inquirer.ask<{ email: string }>('prompt-email', {}).then(({ email }: { email: string }) => email); + }; +}; + +@Command({ + name: 'grant-admin', + description: 'Grant admin privileges to a user (by email)', +}) +export class GrantAdminCommand extends CommandRunner { + constructor( + private service: CliService, + private inquirer: InquirerService, + ) { + super(); + } + + async run(): Promise { + try { + const email = await prompt(this.inquirer)(); + await this.service.grantAdminAccess(email); + console.debug('Admin access has been granted to', email); + } catch (error) { + console.error(error); + console.error('Unable to grant admin access to user'); + } + } +} + +@Command({ + name: 'revoke-admin', + description: 'Revoke admin privileges from a user (by email)', +}) +export class RevokeAdminCommand extends CommandRunner { + constructor( + private service: CliService, + private inquirer: InquirerService, + ) { + super(); + } + + async run(): Promise { + try { + const email = await prompt(this.inquirer)(); + await this.service.revokeAdminAccess(email); + console.debug('Admin access has been revoked from', email); + } catch (error) { + console.error(error); + console.error('Unable to revoke admin access from user'); + } + } +} + +@QuestionSet({ name: 'prompt-email' }) +export class PromptEmailQuestion { + @Question({ + message: 'Please enter the user email: ', + name: 'email', + }) + parseEmail(value: string) { + return value; + } +} diff --git a/server/src/commands/index.ts b/server/src/commands/index.ts index 59846628bf..ce085f6e34 100644 --- a/server/src/commands/index.ts +++ b/server/src/commands/index.ts @@ -1,3 +1,4 @@ +import { GrantAdminCommand, PromptEmailQuestion, RevokeAdminCommand } from 'src/commands/grant-admin'; import { ListUsersCommand } from 'src/commands/list-users.command'; import { DisableOAuthLogin, EnableOAuthLogin } from 'src/commands/oauth-login'; import { DisablePasswordLoginCommand, EnablePasswordLoginCommand } from 'src/commands/password-login'; @@ -7,10 +8,13 @@ import { VersionCommand } from 'src/commands/version.command'; export const commands = [ ResetAdminPasswordCommand, PromptPasswordQuestions, + PromptEmailQuestion, EnablePasswordLoginCommand, DisablePasswordLoginCommand, EnableOAuthLogin, DisableOAuthLogin, ListUsersCommand, VersionCommand, + GrantAdminCommand, + RevokeAdminCommand, ]; diff --git a/server/src/dtos/user.dto.ts b/server/src/dtos/user.dto.ts index 9d43e53f89..ed08f7534d 100644 --- a/server/src/dtos/user.dto.ts +++ b/server/src/dtos/user.dto.ts @@ -106,6 +106,10 @@ export class UserAdminCreateDto { @Optional() @IsBoolean() notify?: boolean; + + @Optional() + @IsBoolean() + isAdmin?: boolean; } export class UserAdminUpdateDto { @@ -145,6 +149,10 @@ export class UserAdminUpdateDto { @Min(0) @ApiProperty({ type: 'integer', format: 'int64' }) quotaSizeInBytes?: number | null; + + @Optional() + @IsBoolean() + isAdmin?: boolean; } export class UserAdminDeleteDto { diff --git a/server/src/services/cli.service.ts b/server/src/services/cli.service.ts index f6173c69f7..021a5240f6 100644 --- a/server/src/services/cli.service.ts +++ b/server/src/services/cli.service.ts @@ -37,6 +37,24 @@ export class CliService extends BaseService { await this.updateConfig(config); } + async grantAdminAccess(email: string): Promise { + const user = await this.userRepository.getByEmail(email); + if (!user) { + throw new Error('User does not exist'); + } + + await this.userRepository.update(user.id, { isAdmin: true }); + } + + async revokeAdminAccess(email: string): Promise { + const user = await this.userRepository.getByEmail(email); + if (!user) { + throw new Error('User does not exist'); + } + + await this.userRepository.update(user.id, { isAdmin: false }); + } + async disableOAuthLogin(): Promise { const config = await this.getConfig({ withCache: false }); config.oauth.enabled = false; diff --git a/server/src/services/user-admin.service.spec.ts b/server/src/services/user-admin.service.spec.ts index 3e613bc485..85cbb8238a 100644 --- a/server/src/services/user-admin.service.spec.ts +++ b/server/src/services/user-admin.service.spec.ts @@ -4,6 +4,7 @@ import { JobName, UserStatus } from 'src/enum'; import { UserAdminService } from 'src/services/user-admin.service'; import { authStub } from 'test/fixtures/auth.stub'; import { userStub } from 'test/fixtures/user.stub'; +import { factory } from 'test/small.factory'; import { newTestService, ServiceMocks } from 'test/utils'; import { describe } from 'vitest'; @@ -116,7 +117,7 @@ describe(UserAdminService.name, () => { it('should throw error if user could not be found', async () => { mocks.user.get.mockResolvedValue(void 0); - await expect(sut.delete(authStub.admin, userStub.admin.id, {})).rejects.toThrowError(BadRequestException); + await expect(sut.delete(authStub.admin, 'not-found', {})).rejects.toThrowError(BadRequestException); expect(mocks.user.delete).not.toHaveBeenCalled(); }); @@ -124,8 +125,11 @@ describe(UserAdminService.name, () => { await expect(sut.delete(authStub.admin, userStub.admin.id, {})).rejects.toBeInstanceOf(ForbiddenException); }); - it('should require the auth user be an admin', async () => { - await expect(sut.delete(authStub.user1, authStub.admin.user.id, {})).rejects.toBeInstanceOf(ForbiddenException); + it('should not allow deleting own account', async () => { + const user = factory.userAdmin({ isAdmin: false }); + const auth = factory.auth({ user }); + mocks.user.get.mockResolvedValue(user); + await expect(sut.delete(auth, user.id, {})).rejects.toBeInstanceOf(ForbiddenException); expect(mocks.user.delete).not.toHaveBeenCalled(); }); diff --git a/server/src/services/user-admin.service.ts b/server/src/services/user-admin.service.ts index dcd415174d..332496a95e 100644 --- a/server/src/services/user-admin.service.ts +++ b/server/src/services/user-admin.service.ts @@ -52,6 +52,10 @@ export class UserAdminService extends BaseService { async update(auth: AuthDto, id: string, dto: UserAdminUpdateDto): Promise { const user = await this.findOrFail(id, {}); + if (dto.isAdmin !== undefined && dto.isAdmin !== auth.user.isAdmin && auth.user.id === id) { + throw new BadRequestException('Admin status can only be changed by another admin'); + } + if (dto.quotaSizeInBytes && user.quotaSizeInBytes !== dto.quotaSizeInBytes) { await this.userRepository.syncUsage(id); } @@ -89,9 +93,9 @@ export class UserAdminService extends BaseService { async delete(auth: AuthDto, id: string, dto: UserAdminDeleteDto): Promise { const { force } = dto; - const { isAdmin } = await this.findOrFail(id, {}); - if (isAdmin) { - throw new ForbiddenException('Cannot delete admin user'); + await this.findOrFail(id, {}); + if (auth.user.id === id) { + throw new ForbiddenException('Cannot delete your own account'); } await this.albumRepository.softDeleteAll(id); diff --git a/web/src/lib/modals/UserEditModal.svelte b/web/src/lib/modals/UserEditModal.svelte index 0bb018721b..aa0d6c3e87 100644 --- a/web/src/lib/modals/UserEditModal.svelte +++ b/web/src/lib/modals/UserEditModal.svelte @@ -1,10 +1,11 @@