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.

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<HTMLInputElement> = (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);

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) => {

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,
     });
 };
 

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;

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<void> {
         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<IUser, 'id' | 'scimId'>): Promise<void> {
+        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<IUser, 'id' | 'scimId'>): Promise<boolean> {
+        return (
+            Boolean(scimId) ||
+            Boolean((await this.userService.getUser(id)).scimId)
+        );
+    }
 }

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 {

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: '[email protected]',
+    name: 'SCIM User',
+    scim_id: 'some-random-scim-id',
+};
+
+const regularUser = {
+    email: '[email protected]',
+    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);
+});