From 173133c58211638f70f4afdaea1974d5e0650cd8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nuno=20G=C3=B3is?= Date: Thu, 11 Apr 2024 14:06:28 +0100 Subject: [PATCH 1/3] chore: SCIM guard for users --- .../ChangePassword/ChangePassword.tsx | 7 + .../useAdminUsersApi/useAdminUsersApi.ts | 11 +- src/lib/db/user-store.ts | 2 + src/lib/openapi/spec/user-schema.ts | 7 + src/lib/routes/admin-api/user-admin.ts | 35 ++++- src/lib/types/user.ts | 6 + .../e2e/api/admin/user-admin.scim.e2e.test.ts | 121 ++++++++++++++++++ 7 files changed, 178 insertions(+), 11 deletions(-) create mode 100644 src/test/e2e/api/admin/user-admin.scim.e2e.test.ts diff --git a/frontend/src/component/admin/users/UsersList/ChangePassword/ChangePassword.tsx b/frontend/src/component/admin/users/UsersList/ChangePassword/ChangePassword.tsx index 5253426212db..c0ebb44fa530 100644 --- a/frontend/src/component/admin/users/UsersList/ChangePassword/ChangePassword.tsx +++ b/frontend/src/component/admin/users/UsersList/ChangePassword/ChangePassword.tsx @@ -13,6 +13,7 @@ import PasswordMatcher from 'component/user/common/ResetPasswordForm/PasswordMat import type { IUser } from 'interfaces/user'; import useAdminUsersApi from 'hooks/api/actions/useAdminUsersApi/useAdminUsersApi'; import { UserAvatar } from 'component/common/UserAvatar/UserAvatar'; +import useToast from 'hooks/useToast'; const StyledUserAvatar = styled(UserAvatar)(({ theme }) => ({ width: theme.spacing(5), @@ -36,6 +37,7 @@ const ChangePassword = ({ const [validPassword, setValidPassword] = useState(false); const { classes: themeStyles } = useThemeStyles(); const { changePassword } = useAdminUsersApi(); + const { setToastData } = useToast(); const updateField: React.ChangeEventHandler = (event) => { setError(undefined); @@ -58,6 +60,11 @@ const ChangePassword = ({ await changePassword(user.id, data.password); setData({}); closeDialog(); + setToastData({ + title: 'Password changed successfully', + text: 'The user can now sign in using the new password.', + type: 'success', + }); } catch (error: unknown) { console.warn(error); setError(PASSWORD_FORMAT_MESSAGE); diff --git a/frontend/src/hooks/api/actions/useAdminUsersApi/useAdminUsersApi.ts b/frontend/src/hooks/api/actions/useAdminUsersApi/useAdminUsersApi.ts index 9c4993135413..ec71c06ded2d 100644 --- a/frontend/src/hooks/api/actions/useAdminUsersApi/useAdminUsersApi.ts +++ b/frontend/src/hooks/api/actions/useAdminUsersApi/useAdminUsersApi.ts @@ -1,10 +1,4 @@ import useAPI from '../useApi/useApi'; -import { - handleBadRequest, - handleForbidden, - handleNotFound, - handleUnauthorized, -} from './errorHandlers'; interface IUserPayload { name: string; @@ -16,10 +10,7 @@ export const REMOVE_USER_ERROR = 'removeUser'; const useAdminUsersApi = () => { const { loading, makeRequest, createRequest, errors } = useAPI({ - handleBadRequest, - handleNotFound, - handleUnauthorized, - handleForbidden, + propagateErrors: true, }); const addUser = async (user: IUserPayload) => { diff --git a/src/lib/db/user-store.ts b/src/lib/db/user-store.ts index 73a4e1e2efdf..f8bf03eaf539 100644 --- a/src/lib/db/user-store.ts +++ b/src/lib/db/user-store.ts @@ -22,6 +22,7 @@ const USER_COLUMNS_PUBLIC = [ 'image_url', 'seen_at', 'is_service', + 'scim_id', ]; const USER_COLUMNS = [...USER_COLUMNS_PUBLIC, 'login_attempts', 'created_at']; @@ -56,6 +57,7 @@ const rowToUser = (row) => { seenAt: row.seen_at, createdAt: row.created_at, isService: row.is_service, + scimId: row.scim_id, }); }; diff --git a/src/lib/openapi/spec/user-schema.ts b/src/lib/openapi/spec/user-schema.ts index ecb092d4df66..a3ee1761951b 100644 --- a/src/lib/openapi/spec/user-schema.ts +++ b/src/lib/openapi/spec/user-schema.ts @@ -92,6 +92,13 @@ export const userSchema = { type: 'string', }, }, + scimId: { + description: + 'The SCIM ID of the user, only present if managed by SCIM', + type: 'string', + nullable: true, + example: '01HTMEXAMPLESCIMID7SWWGHN6', + }, }, components: {}, } as const; diff --git a/src/lib/routes/admin-api/user-admin.ts b/src/lib/routes/admin-api/user-admin.ts index 7aaf145b3df7..10a834f0829f 100644 --- a/src/lib/routes/admin-api/user-admin.ts +++ b/src/lib/routes/admin-api/user-admin.ts @@ -51,7 +51,7 @@ import { type AdminCountSchema, adminCountSchema, } from '../../openapi/spec/admin-count-schema'; -import { BadDataError } from '../../error'; +import { BadDataError, ForbiddenError } from '../../error'; import { createUserResponseSchema, type CreateUserResponseSchema, @@ -81,6 +81,8 @@ export default class UserAdminController extends Controller { readonly unleashUrl: string; + readonly isEnterprise: boolean; + constructor( config: IUnleashConfig, { @@ -116,6 +118,7 @@ export default class UserAdminController extends Controller { this.logger = config.getLogger('routes/user-controller.ts'); this.unleashUrl = config.server.unleashUrl; this.flagResolver = config.flagResolver; + this.isEnterprise = config.isEnterprise; this.route({ method: 'post', @@ -402,6 +405,8 @@ export default class UserAdminController extends Controller { ): Promise { const { user } = req; const receiver = req.body.id; + const receiverUser = await this.userService.getByEmail(receiver); + await this.throwIfScimUser(receiverUser); const resetPasswordUrl = await this.userService.createResetPasswordEmail(receiver, user); @@ -605,6 +610,7 @@ export default class UserAdminController extends Controller { if (!Number.isInteger(Number(id))) { throw new BadDataError('User id should be an integer'); } + await this.throwIfScimUser({ id: Number(id) }); const normalizedRootRole = Number.isInteger(Number(rootRole)) ? Number(rootRole) : (rootRole as RoleName); @@ -636,6 +642,7 @@ export default class UserAdminController extends Controller { if (!Number.isInteger(Number(id))) { throw new BadDataError('User id should be an integer'); } + await this.throwIfScimUser({ id: Number(id) }); await this.userService.deleteUser(+id, user); res.status(200).send(); @@ -658,6 +665,8 @@ export default class UserAdminController extends Controller { const { id } = req.params; const { password } = req.body; + await this.throwIfScimUser({ id: Number(id) }); + await this.userService.changePassword(+id, password); res.status(200).send(); } @@ -716,4 +725,28 @@ export default class UserAdminController extends Controller { projectRoles, }); } + + async throwIfScimUser({ + id, + scimId, + }: Pick): Promise { + if (this.isEnterprise && this.flagResolver.isEnabled('scimApi')) { + const { enabled } = await this.settingService.getWithDefault( + 'scim', + { enabled: false }, + ); + + if (enabled) { + const isScim = + Boolean(scimId) || + Boolean((await this.userService.getUser(id)).scimId); + + if (isScim) { + throw new ForbiddenError( + 'Cannot perform this operation on SCIM users', + ); + } + } + } + } } diff --git a/src/lib/types/user.ts b/src/lib/types/user.ts index e8ac49d2a2e9..dd5dd15ee2b6 100644 --- a/src/lib/types/user.ts +++ b/src/lib/types/user.ts @@ -14,6 +14,7 @@ export interface UserData { loginAttempts?: number; createdAt?: Date; isService?: boolean; + scimId?: string; } export interface IUser { @@ -29,6 +30,7 @@ export interface IUser { isAPI: boolean; imageUrl?: string; accountType?: AccountType; + scimId?: string; } export interface IProjectUser extends IUser { @@ -58,6 +60,8 @@ export default class User implements IUser { accountType?: AccountType = 'User'; + scimId?: string; + constructor({ id, name, @@ -68,6 +72,7 @@ export default class User implements IUser { loginAttempts, createdAt, isService, + scimId, }: UserData) { if (!id) { throw new ValidationError('Id is required', [], undefined); @@ -85,6 +90,7 @@ export default class User implements IUser { this.loginAttempts = loginAttempts; this.createdAt = createdAt; this.accountType = isService ? 'Service Account' : 'User'; + this.scimId = scimId; } generateImageUrl(): string { diff --git a/src/test/e2e/api/admin/user-admin.scim.e2e.test.ts b/src/test/e2e/api/admin/user-admin.scim.e2e.test.ts new file mode 100644 index 000000000000..cda1c5e31256 --- /dev/null +++ b/src/test/e2e/api/admin/user-admin.scim.e2e.test.ts @@ -0,0 +1,121 @@ +import { + type IUnleashTest, + setupAppWithCustomConfig, +} from '../../helpers/test-helper'; +import dbInit, { type ITestDb } from '../../helpers/database-init'; +import getLogger from '../../../fixtures/no-logger'; +import type { IUnleashStores } from '../../../../lib/types'; + +let stores: IUnleashStores; +let db: ITestDb; +let app: IUnleashTest; + +let scimUserId: number; +let regularUserId: number; + +const scimUser = { + email: 'scim-user@test.com', + name: 'SCIM User', + scim_id: 'some-random-scim-id', +}; + +const regularUser = { + email: 'regular-user@test.com', + name: 'Regular User', +}; + +beforeAll(async () => { + db = await dbInit('user_admin_scim', getLogger); + stores = db.stores; + app = await setupAppWithCustomConfig(stores, { + enterpriseVersion: 'enterprise', + experimental: { + flags: { + strictSchemaValidation: true, + scimApi: true, + }, + }, + }); + + await stores.settingStore.insert('scim', { + enabled: true, + }); + + scimUserId = ( + await db.rawDatabase('users').insert(scimUser).returning('id') + )[0].id; + + regularUserId = ( + await db.rawDatabase('users').insert(regularUser).returning('id') + )[0].id; +}); + +afterAll(async () => { + await app.destroy(); + await db.destroy(); +}); + +test('fetching a SCIM user should include scimId', async () => { + const { body } = await app.request + .get(`/api/admin/user-admin/${scimUserId}`) + .expect(200); + + expect(body.email).toBe(scimUser.email); + expect(body.scimId).toBe('some-random-scim-id'); +}); + +test('fetching a regular user should not include scimId', async () => { + const { body } = await app.request + .get(`/api/admin/user-admin/${regularUserId}`) + .expect(200); + + expect(body.email).toBe(regularUser.email); + expect(body.scimId).toBeFalsy(); +}); + +test('should prevent editing a SCIM user', async () => { + const { body } = await app.request + .put(`/api/admin/user-admin/${scimUserId}`) + .send({ + name: 'New name', + }) + .expect(403); + + expect(body.details[0].message).toBe( + 'Cannot perform this operation on SCIM users', + ); +}); + +test('should prevent deleting a SCIM user', async () => { + const { body } = await app.request + .delete(`/api/admin/user-admin/${scimUserId}`) + .expect(403); + + expect(body.details[0].message).toBe( + 'Cannot perform this operation on SCIM users', + ); +}); + +test('should prevent changing password for a SCIM user', async () => { + const { body } = await app.request + .post(`/api/admin/user-admin/${scimUserId}/change-password`) + .send({ + password: 'new-password', + }) + .expect(403); + + expect(body.details[0].message).toBe( + 'Cannot perform this operation on SCIM users', + ); +}); + +test('should prevent resetting password for a SCIM user', async () => { + const { body } = await app.request + .post(`/api/admin/user-admin/reset-password`) + .send({ id: scimUser.email }) + .expect(403); + + expect(body.details[0].message).toBe( + 'Cannot perform this operation on SCIM users', + ); +}); From 9b88eb26e08bfae867c52ac4b0b8a50360e84cb5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nuno=20G=C3=B3is?= Date: Thu, 11 Apr 2024 14:53:33 +0100 Subject: [PATCH 2/3] refactor: throwIfScimUser --- src/lib/routes/admin-api/user-admin.ts | 36 ++++++++++++++------------ 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/src/lib/routes/admin-api/user-admin.ts b/src/lib/routes/admin-api/user-admin.ts index 10a834f0829f..909e87f8fbc6 100644 --- a/src/lib/routes/admin-api/user-admin.ts +++ b/src/lib/routes/admin-api/user-admin.ts @@ -730,23 +730,27 @@ export default class UserAdminController extends Controller { id, scimId, }: Pick): Promise { - if (this.isEnterprise && this.flagResolver.isEnabled('scimApi')) { - const { enabled } = await this.settingService.getWithDefault( - 'scim', - { enabled: false }, - ); + if (!this.isEnterprise) return; + if (!this.flagResolver.isEnabled('scimApi')) return; - if (enabled) { - const isScim = - Boolean(scimId) || - Boolean((await this.userService.getUser(id)).scimId); + const isScimUser = await this.isScimUser({ id, scimId }); + if (!isScimUser) return; - if (isScim) { - throw new ForbiddenError( - 'Cannot perform this operation on SCIM users', - ); - } - } - } + const { enabled } = await this.settingService.getWithDefault('scim', { + enabled: false, + }); + if (!enabled) return; + + throw new ForbiddenError('Cannot perform this operation on SCIM users'); + } + + async isScimUser({ + id, + scimId, + }: Pick): Promise { + return ( + Boolean(scimId) || + Boolean((await this.userService.getUser(id)).scimId) + ); } } From 1940d2a3bb3e16b10a9f940612c5857299d605a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nuno=20G=C3=B3is?= Date: Thu, 11 Apr 2024 14:58:14 +0100 Subject: [PATCH 3/3] chore: better message in guard --- src/lib/routes/admin-api/user-admin.ts | 4 +++- .../e2e/api/admin/user-admin.scim.e2e.test.ts | 19 +++++++------------ 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/src/lib/routes/admin-api/user-admin.ts b/src/lib/routes/admin-api/user-admin.ts index 909e87f8fbc6..68406fa1501d 100644 --- a/src/lib/routes/admin-api/user-admin.ts +++ b/src/lib/routes/admin-api/user-admin.ts @@ -741,7 +741,9 @@ export default class UserAdminController extends Controller { }); if (!enabled) return; - throw new ForbiddenError('Cannot perform this operation on SCIM users'); + throw new ForbiddenError( + 'This user is managed by your SCIM provider and cannot be changed manually', + ); } async isScimUser({ diff --git a/src/test/e2e/api/admin/user-admin.scim.e2e.test.ts b/src/test/e2e/api/admin/user-admin.scim.e2e.test.ts index cda1c5e31256..3461ab1f6610 100644 --- a/src/test/e2e/api/admin/user-admin.scim.e2e.test.ts +++ b/src/test/e2e/api/admin/user-admin.scim.e2e.test.ts @@ -24,6 +24,9 @@ const regularUser = { name: 'Regular User', }; +const scimGuardErrorMessage = + 'This user is managed by your SCIM provider and cannot be changed manually'; + beforeAll(async () => { db = await dbInit('user_admin_scim', getLogger); stores = db.stores; @@ -81,9 +84,7 @@ test('should prevent editing a SCIM user', async () => { }) .expect(403); - expect(body.details[0].message).toBe( - 'Cannot perform this operation on SCIM users', - ); + expect(body.details[0].message).toBe(scimGuardErrorMessage); }); test('should prevent deleting a SCIM user', async () => { @@ -91,9 +92,7 @@ test('should prevent deleting a SCIM user', async () => { .delete(`/api/admin/user-admin/${scimUserId}`) .expect(403); - expect(body.details[0].message).toBe( - 'Cannot perform this operation on SCIM users', - ); + expect(body.details[0].message).toBe(scimGuardErrorMessage); }); test('should prevent changing password for a SCIM user', async () => { @@ -104,9 +103,7 @@ test('should prevent changing password for a SCIM user', async () => { }) .expect(403); - expect(body.details[0].message).toBe( - 'Cannot perform this operation on SCIM users', - ); + expect(body.details[0].message).toBe(scimGuardErrorMessage); }); test('should prevent resetting password for a SCIM user', async () => { @@ -115,7 +112,5 @@ test('should prevent resetting password for a SCIM user', async () => { .send({ id: scimUser.email }) .expect(403); - expect(body.details[0].message).toBe( - 'Cannot perform this operation on SCIM users', - ); + expect(body.details[0].message).toBe(scimGuardErrorMessage); });