fix: SSO auto create with SCIM tend to override each other (#10675)

## About the changes Having SCIM enabled with SAML and auto-create can generate issues with each protocol stepping into the other protocol's toes. This PR adds protection to avoid updating SCIM-managed users with SAML data (cause SCIM will override this data later). It also adds a new method in the store to check if we have cases where deleted_at is set but the email is not cleared, and there's no delete event in the audit log (we've found one case, and we believe it might be related to interoperability issues between SAML and SCIM)
2025-09-24 17:51:14 +02:00 · 2025-09-22 17:55:22 +02:00 · 2025-09-22 17:55:22 +02:00 · a628755506
commit a628755506
parent 8d03ce340d
3 changed files with 186 additions and 120 deletions
--- a/src/lib/features/users/user-store.ts
+++ b/src/lib/features/users/user-store.ts
@ -344,7 +344,7 @@ export class UserStore implements IUserStore {
    }

    async getFirstUserDate(): Promise<Date | null> {
-        const firstInstanceUser = await this.db('users')
+        const firstInstanceUser = await this.db(TABLE)
            .select('created_at')
            .where('is_system', '=', false)
            .orderBy('created_at', 'asc')
@ -352,4 +352,13 @@ export class UserStore implements IUserStore {

        return firstInstanceUser ? firstInstanceUser.created_at : null;
    }
+
+    // this is temporary to find out how many cases we have
+    async findDeletedUsersWithEmail(): Promise<User[]> {
+        return this.db(TABLE)
+            .select('*')
+            .whereNotNull('deleted_at')
+            .andWhereRaw('length(email) > 0')
+            .then((rows) => rows.map(rowToUser));
+    }
 }
--- a/src/lib/services/user-service.ts
+++ b/src/lib/services/user-service.ts
@ -506,9 +506,30 @@ export class UserService {

        try {
            user = await this.store.getByQuery({ email });
-            // Update user if autCreate is enabled.
-            if (name && user.name !== name) {
-                user = await this.store.update(user.id, { name, email });
+            // Update user if not managed by scim
+            if (name && user.name !== name && !user.scimId) {
+                const currentRole = await this.accessService.getRootRoleForUser(
+                    user.id,
+                );
+                const updatedUser = await this.store.update(user.id, {
+                    name,
+                    email,
+                });
+
+                await this.eventService.storeEvent(
+                    new UserUpdatedEvent({
+                        auditUser: SYSTEM_USER_AUDIT,
+                        preUser: {
+                            ...user,
+                            rootRole: currentRole.id,
+                        },
+                        postUser: {
+                            ...updatedUser,
+                            rootRole: currentRole.id,
+                        },
+                    }),
+                );
+                user = { ...user, ...updatedUser };
            }
        } catch (e) {
            // User does not exists. Create if 'autoCreate' is enabled
--- a/src/test/e2e/services/user-service.e2e.test.ts
+++ b/src/test/e2e/services/user-service.e2e.test.ts
@ -435,141 +435,177 @@ test('updating a user without an email should not strip the email', async () =>
    expect(updatedUser.email).toBe(email);
 });

-test('should login and create user via SSO', async () => {
-    const recordedEvents: Array<{ loginOrder: number }> = [];
-    eventBus.on(USER_LOGIN, (data) => {
-        recordedEvents.push(data);
-    });
-    const email = 'some@test.com';
-    const user = await userService.loginUserSSO({
-        email,
-        rootRole: RoleName.VIEWER,
-        name: 'some',
-        autoCreate: true,
-    });
-
-    const userWithRole = await userService.getUser(user.id);
-    expect(user.email).toBe(email);
-    expect(user.name).toBe('some');
-    expect(userWithRole.name).toBe('some');
-    expect(userWithRole.rootRole).toBe(viewerRole.id);
-    expect(recordedEvents).toEqual([{ loginOrder: 0 }]);
-});
-
-test('should throw if rootRole is wrong via SSO', async () => {
-    expect.assertions(1);
-
-    await expect(
-        userService.loginUserSSO({
-            email: 'some@test.com',
-            rootRole: RoleName.MEMBER,
+describe('loginUserSSO', () => {
+    test('should login and create user via SSO', async () => {
+        const recordedEvents: Array<{ loginOrder: number }> = [];
+        eventBus.on(USER_LOGIN, (data) => {
+            recordedEvents.push(data);
+        });
+        const email = 'some@test.com';
+        const user = await userService.loginUserSSO({
+            email,
+            rootRole: RoleName.VIEWER,
            name: 'some',
            autoCreate: true,
-        }),
-    ).rejects.errorWithMessage(
-        new BadDataError('Could not find rootRole=Member'),
-    );
-});
+        });

-test('should update user name when signing in via SSO', async () => {
-    const email = 'some@test.com';
-    const originalUser = await userService.createUser(
-        {
-            email,
-            rootRole: RoleName.VIEWER,
-            name: 'some',
-        },
-        TEST_AUDIT_USER,
-    );
-
-    await userService.loginUserSSO({
-        email,
-        rootRole: RoleName.ADMIN,
-        name: 'New name!',
-        autoCreate: true,
+        const userWithRole = await userService.getUser(user.id);
+        expect(user.email).toBe(email);
+        expect(user.name).toBe('some');
+        expect(userWithRole.name).toBe('some');
+        expect(userWithRole.rootRole).toBe(viewerRole.id);
+        expect(recordedEvents).toEqual([{ loginOrder: 0 }]);
    });

-    const actualUser = await userService.getUser(originalUser.id);
+    test('should throw if rootRole is wrong via SSO', async () => {
+        expect.assertions(1);

-    expect(actualUser.email).toBe(email);
-    expect(actualUser.name).toBe('New name!');
-    expect(actualUser.rootRole).toBe(viewerRole.id);
-});
-
-test('should update name if it is different via SSO', async () => {
-    const email = 'some@test.com';
-    const originalUser = await userService.createUser(
-        {
-            email,
-            rootRole: RoleName.VIEWER,
-            name: 'some',
-        },
-        TEST_AUDIT_USER,
-    );
-
-    await userService.loginUserSSO({
-        email,
-        rootRole: RoleName.ADMIN,
-        name: 'New name!',
-        autoCreate: false,
+        await expect(
+            userService.loginUserSSO({
+                email: 'some@test.com',
+                rootRole: RoleName.MEMBER,
+                name: 'some',
+                autoCreate: true,
+            }),
+        ).rejects.errorWithMessage(
+            new BadDataError('Could not find rootRole=Member'),
+        );
    });

-    const actualUser = await userService.getUser(originalUser.id);
+    test('should update user name when signing in via SSO', async () => {
+        const email = 'some@test.com';
+        const originalUser = await userService.createUser(
+            {
+                email,
+                rootRole: RoleName.VIEWER,
+                name: 'some',
+            },
+            TEST_AUDIT_USER,
+        );

-    expect(actualUser.email).toBe(email);
-    expect(actualUser.name).toBe('New name!');
-    expect(actualUser.rootRole).toBe(viewerRole.id);
-});
+        await userService.loginUserSSO({
+            email,
+            rootRole: RoleName.ADMIN,
+            name: 'New name!',
+            autoCreate: true,
+        });

-test('should throw if autoCreate is false via SSO', async () => {
-    expect.assertions(1);
+        const actualUser = await userService.getUser(originalUser.id);

-    await expect(
-        userService.loginUserSSO({
-            email: 'some@test.com',
-            rootRole: RoleName.MEMBER,
-            name: 'some',
+        expect(actualUser.email).toBe(email);
+        expect(actualUser.name).toBe('New name!');
+        expect(actualUser.rootRole).toBe(viewerRole.id);
+        const { events } = await eventService.getEvents();
+        const updateEvent = events.find(
+            (e) => e.data.id === originalUser.id && e.data.name === 'New name!',
+        );
+        expect(updateEvent).toBeDefined();
+    });
+
+    test('should update name if it is different via SSO', async () => {
+        const email = 'some@test.com';
+        const originalUser = await userService.createUser(
+            {
+                email,
+                rootRole: RoleName.VIEWER,
+                name: 'some',
+            },
+            TEST_AUDIT_USER,
+        );
+
+        await userService.loginUserSSO({
+            email,
+            rootRole: RoleName.ADMIN,
+            name: 'New name!',
            autoCreate: false,
-        }),
-    ).rejects.errorWithMessage(new NotFoundError('No user found'));
-});
+        });

-test('should support a root role id when logging in and creating user via SSO', async () => {
-    const name = 'root-role-id';
-    const email = `${name}@test.com`;
-    const user = await userService.loginUserSSO({
-        email,
-        rootRole: viewerRole.id,
-        name,
-        autoCreate: true,
+        const actualUser = await userService.getUser(originalUser.id);
+
+        expect(actualUser.email).toBe(email);
+        expect(actualUser.name).toBe('New name!');
+        expect(actualUser.rootRole).toBe(viewerRole.id);
    });

-    const userWithRole = await userService.getUser(user.id);
-    expect(user.email).toBe(email);
-    expect(user.name).toBe(name);
-    expect(userWithRole.name).toBe(name);
-    expect(userWithRole.rootRole).toBe(viewerRole.id);
-});
+    test('should throw if autoCreate is false via SSO', async () => {
+        expect.assertions(1);

-test('should support a custom root role id when logging in and creating user via SSO', async () => {
-    const name = 'custom-root-role-id';
-    const email = `${name}@test.com`;
-    const user = await userService.loginUserSSO({
-        email,
-        rootRole: customRole.id,
-        name,
-        autoCreate: true,
+        await expect(
+            userService.loginUserSSO({
+                email: 'some@test.com',
+                rootRole: RoleName.MEMBER,
+                name: 'some',
+                autoCreate: false,
+            }),
+        ).rejects.errorWithMessage(new NotFoundError('No user found'));
    });

-    const userWithRole = await userService.getUser(user.id);
-    expect(user.email).toBe(email);
-    expect(user.name).toBe(name);
-    expect(userWithRole.name).toBe(name);
-    expect(userWithRole.rootRole).toBe(customRole.id);
+    test('should support a root role id when logging in and creating user via SSO', async () => {
+        const name = 'root-role-id';
+        const email = `${name}@test.com`;
+        const user = await userService.loginUserSSO({
+            email,
+            rootRole: viewerRole.id,
+            name,
+            autoCreate: true,
+        });

-    const permissions = await accessService.getPermissionsForUser(user);
-    expect(permissions).toHaveLength(1);
-    expect(permissions[0].permission).toBe(CREATE_ADDON);
+        const userWithRole = await userService.getUser(user.id);
+        expect(user.email).toBe(email);
+        expect(user.name).toBe(name);
+        expect(userWithRole.name).toBe(name);
+        expect(userWithRole.rootRole).toBe(viewerRole.id);
+    });
+
+    test('should support a custom root role id when logging in and creating user via SSO', async () => {
+        const name = 'custom-root-role-id';
+        const email = `${name}@test.com`;
+        const user = await userService.loginUserSSO({
+            email,
+            rootRole: customRole.id,
+            name,
+            autoCreate: true,
+        });
+
+        const userWithRole = await userService.getUser(user.id);
+        expect(user.email).toBe(email);
+        expect(user.name).toBe(name);
+        expect(userWithRole.name).toBe(name);
+        expect(userWithRole.rootRole).toBe(customRole.id);
+
+        const permissions = await accessService.getPermissionsForUser(user);
+        expect(permissions).toHaveLength(1);
+        expect(permissions[0].permission).toBe(CREATE_ADDON);
+    });
+
+    test(`should not update the username if managed by SCIM`, async () => {
+        const email = 'test-1@getunleash.io';
+        const originalName = 'Original Name';
+        const name = 'Updated Name';
+        const createdUser = await userStore.insert({
+            name: originalName,
+            username: 'random-1234',
+            email,
+        });
+        await db
+            .rawDatabase('users')
+            .update({
+                scim_id: '123',
+            })
+            .where({ id: createdUser.id });
+
+        const user = await userService.loginUserSSO({
+            email,
+            autoCreate: true,
+            name,
+        });
+
+        expect(user.name).toBe(originalName);
+
+        // Fetch the user directly from the store to verify
+        const storedUser = await userStore.get(user.id);
+        expect(storedUser!.name).toBe(originalName);
+    });
 });

 describe('Should not be able to use any of previous 5 passwords', () => {