From f4ef06f69bd3cb462e958fe4f74490a30510906d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nuno=20G=C3=B3is?= Date: Fri, 12 Apr 2024 08:23:35 +0100 Subject: [PATCH] chore: SCIM guard for users (#6836) https://linear.app/unleash/issue/2-2093/api-should-not-allow-manual-management-of-scim-managed-users-in Introduces a SCIM guard for SCIM users. SCIM users should be managed exclusively by the SCIM client, not Unleash. We decided to be restrictive for now, completely covering all of the write methods, but may fine-tune some of this at a later stage. Will eventually be followed up by a UI-centric PR. --- .../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 | 41 ++++++- src/lib/types/user.ts | 6 + .../e2e/api/admin/user-admin.scim.e2e.test.ts | 116 ++++++++++++++++++ 7 files changed, 179 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 5253426212..c0ebb44fa5 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 9c49931354..ec71c06ded 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 73a4e1e2ef..f8bf03eaf5 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 ecb092d4df..a3ee176195 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 7aaf145b3d..68406fa150 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,34 @@ export default class UserAdminController extends Controller { projectRoles, }); } + + async throwIfScimUser({ + id, + scimId, + }: Pick): Promise { + if (!this.isEnterprise) return; + if (!this.flagResolver.isEnabled('scimApi')) return; + + const isScimUser = await this.isScimUser({ id, scimId }); + if (!isScimUser) return; + + const { enabled } = await this.settingService.getWithDefault('scim', { + enabled: false, + }); + if (!enabled) return; + + throw new ForbiddenError( + 'This user is managed by your SCIM provider and cannot be changed manually', + ); + } + + async isScimUser({ + id, + scimId, + }: Pick): Promise { + return ( + Boolean(scimId) || + Boolean((await this.userService.getUser(id)).scimId) + ); + } } diff --git a/src/lib/types/user.ts b/src/lib/types/user.ts index e8ac49d2a2..dd5dd15ee2 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 0000000000..3461ab1f66 --- /dev/null +++ b/src/test/e2e/api/admin/user-admin.scim.e2e.test.ts @@ -0,0 +1,116 @@ +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', +}; + +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; + 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(scimGuardErrorMessage); +}); + +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(scimGuardErrorMessage); +}); + +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(scimGuardErrorMessage); +}); + +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(scimGuardErrorMessage); +});