fix(keycloak): avoid undefined values for keycloak group members (#2319)

Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
janus-idp · Oct 9, 2024 · 3447ba8 · 3447ba8
1 parent 46175f2
commit 3447ba8
Show file tree

Hide file tree

Showing 3 changed files with 85 additions and 31 deletions.
diff --git a/plugins/keycloak-backend/src/lib/read.test.ts b/plugins/keycloak-backend/src/lib/read.test.ts
@@ -1,3 +1,5 @@
+import { mockServices } from '@backstage/backend-test-utils';
+
 import KeycloakAdminClient from '@keycloak/keycloak-admin-client';
 
 import {
@@ -28,23 +30,41 @@ const config: KeycloakProviderConfig = {
   baseUrl: 'http://mock-url',
 };
 
+const logger = mockServices.logger.mock();
+
 describe('readKeycloakRealm', () => {
   it('should return the correct number of users and groups (Version 23 or Higher)', async () => {
     const client =
       new KeycloakAdminClientMockServerv24() as unknown as KeycloakAdminClient;
-    const { users, groups } = await readKeycloakRealm(client, config);
+    const { users, groups } = await readKeycloakRealm(client, config, logger);
     expect(users).toHaveLength(3);
     expect(groups).toHaveLength(3);
   });
 
   it('should return the correct number of users and groups (Version Less than 23)', async () => {
     const client =
       new KeycloakAdminClientMockServerv18() as unknown as KeycloakAdminClient;
-    const { users, groups } = await readKeycloakRealm(client, config);
+    const { users, groups } = await readKeycloakRealm(client, config, logger);
     expect(users).toHaveLength(3);
     expect(groups).toHaveLength(3);
   });
 
+  it(`should not contain undefined members when a group member is not found in the fetched user list`, async () => {
+    const client =
+      new KeycloakAdminClientMockServerv24() as unknown as KeycloakAdminClient;
+    client.users.find = jest
+      .fn()
+      .mockResolvedValue([usersFixture[1], usersFixture[2]]);
+    client.users.count = jest.fn().mockResolvedValue(2);
+
+    const { groups } = await readKeycloakRealm(client, config, logger);
+
+    for (const group of groups) {
+      console.log(group.spec.members);
+      expect(group.spec.members).not.toContain(undefined);
+    }
+  });
+
   it('should propagate transformer changes to entities (version 23 or Higher)', async () => {
     const groupTransformer: GroupTransformer = async (entity, _g, _r) => {
       entity.metadata.name = `${entity.metadata.name}_foo`;
@@ -57,7 +77,7 @@ describe('readKeycloakRealm', () => {
 
     const client =
       new KeycloakAdminClientMockServerv24() as unknown as KeycloakAdminClient;
-    const { users, groups } = await readKeycloakRealm(client, config, {
+    const { users, groups } = await readKeycloakRealm(client, config, logger, {
       userTransformer,
       groupTransformer,
     });
@@ -81,7 +101,7 @@ describe('readKeycloakRealm', () => {
 
     const client =
       new KeycloakAdminClientMockServerv18() as unknown as KeycloakAdminClient;
-    const { users, groups } = await readKeycloakRealm(client, config, {
+    const { users, groups } = await readKeycloakRealm(client, config, logger, {
       userTransformer,
       groupTransformer,
     });
@@ -218,11 +238,15 @@ describe('getEntitiesUser', () => {
     const client =
       new KeycloakAdminClientMockServerv24() as unknown as KeycloakAdminClient;
 
-    const users = await getEntities(client.users, {
-      id: '',
-      baseUrl: '',
-      realm: '',
-    });
+    const users = await getEntities(
+      client.users,
+      {
+        id: '',
+        baseUrl: '',
+        realm: '',
+      },
+      logger,
+    );
 
     expect(users).toHaveLength(3);
   });
@@ -231,11 +255,15 @@ describe('getEntitiesUser', () => {
     const client =
       new KeycloakAdminClientMockServerv18() as unknown as KeycloakAdminClient;
 
-    const users = await getEntities(client.users, {
-      id: '',
-      baseUrl: '',
-      realm: '',
-    });
+    const users = await getEntities(
+      client.users,
+      {
+        id: '',
+        baseUrl: '',
+        realm: '',
+      },
+      logger,
+    );
 
     expect(users).toHaveLength(3);
   });
@@ -251,6 +279,7 @@ describe('getEntitiesUser', () => {
         baseUrl: '',
         realm: '',
       },
+      logger,
       1,
     );
 
@@ -268,6 +297,7 @@ describe('getEntitiesUser', () => {
         baseUrl: '',
         realm: '',
       },
+      logger,
       1,
     );
 
@@ -282,6 +312,7 @@ describe('fetch subgroups', () => {
     const groups = await processGroupsRecursively(
       topLevelGroups23orHigher,
       client.groups,
+      config.realm,
     );
 
     expect(groups).toHaveLength(3);

diff --git a/plugins/keycloak-backend/src/lib/read.ts b/plugins/keycloak-backend/src/lib/read.ts
@@ -14,6 +14,7 @@
  * limitations under the License.
  */
 
+import { LoggerService } from '@backstage/backend-plugin-api';
 import { GroupEntity, UserEntity } from '@backstage/catalog-model';
 
 import KeycloakAdminClient from '@keycloak/keycloak-admin-client';
@@ -109,6 +110,7 @@ export const parseUser = async (
 export async function getEntities<T extends Users | Groups>(
   entities: T,
   config: KeycloakProviderConfig,
+  logger: LoggerService,
   entityQuerySize: number = KEYCLOAK_ENTITY_QUERY_SIZE,
 ): Promise<Awaited<ReturnType<T['find']>>> {
   const rawEntityCount = await entities.count({ realm: config.realm });
@@ -121,11 +123,15 @@ export async function getEntities<T extends Users | Groups>(
   const entityPromises = Array.from(
     { length: pageCount },
     (_, i) =>
-      entities.find({
-        realm: config.realm,
-        max: entityQuerySize,
-        first: i * entityQuerySize,
-      }) as ReturnType<T['find']>,
+      entities
+        .find({
+          realm: config.realm,
+          max: entityQuerySize,
+          first: i * entityQuerySize,
+        })
+        .catch(err =>
+          logger.warn('Failed to retieve Keycloak entities.', err),
+        ) as ReturnType<T['find']>,
   );
 
   const entityResults = (await Promise.all(entityPromises)).flat() as Awaited<
@@ -138,6 +144,7 @@ export async function getEntities<T extends Users | Groups>(
 export async function processGroupsRecursively(
   topLevelGroups: GroupRepresentationWithParent[],
   entities: Groups,
+  realm: string,
 ) {
   const allGroups: GroupRepresentationWithParent[] = [];
   for (const group of topLevelGroups) {
@@ -149,10 +156,12 @@ export async function processGroupsRecursively(
         first: 0,
         max: group.subGroupCount,
         briefRepresentation: true,
+        realm,
       });
       const subGroupResults = await processGroupsRecursively(
         subgroups,
         entities,
+        realm,
       );
       allGroups.push(...subGroupResults);
     }
@@ -174,6 +183,7 @@ export function* traverseGroups(
 export const readKeycloakRealm = async (
   client: KeycloakAdminClient,
   config: KeycloakProviderConfig,
+  logger: LoggerService,
   options?: {
     userQuerySize?: number;
     groupQuerySize?: number;
@@ -187,12 +197,14 @@ export const readKeycloakRealm = async (
   const kUsers = await getEntities(
     client.users,
     config,
+    logger,
     options?.userQuerySize,
   );
 
   const topLevelKGroups = (await getEntities(
     client.groups,
     config,
+    logger,
     options?.groupQuerySize,
   )) as GroupRepresentationWithParent[];
 
@@ -216,6 +228,7 @@ export const readKeycloakRealm = async (
     rawKGroups = await processGroupsRecursively(
       topLevelKGroups,
       client.groups as Groups,
+      config.realm,
     );
   } else {
     rawKGroups = topLevelKGroups.reduce(
@@ -240,6 +253,7 @@ export const readKeycloakRealm = async (
             first: 0,
             max: g.subGroupCount,
             briefRepresentation: false,
+            realm: config.realm,
           });
         }
         if (g.parentId) {
@@ -296,13 +310,17 @@ export const readKeycloakRealm = async (
   const groups = parsedGroups.map(g => {
     const entity = g.entity;
     entity.spec.members =
-      g.entity.spec.members?.map(
-        m => parsedUsers.find(p => p.username === m)?.entity.metadata.name!,
-      ) ?? [];
+      g.entity.spec.members?.flatMap(m => {
+        const name = parsedUsers.find(p => p.username === m)?.entity.metadata
+          .name;
+        return name ? [name] : [];
+      }) ?? [];
     entity.spec.children =
-      g.entity.spec.children?.map(
-        c => parsedGroups.find(p => p.name === c)?.entity.metadata.name!,
-      ) ?? [];
+      g.entity.spec.children?.flatMap(c => {
+        const child = parsedGroups.find(p => p.name === c)?.entity.metadata
+          .name;
+        return child ? [child] : [];
+      }) ?? [];
     entity.spec.parent = parsedGroups.find(
       p => p.name === entity.spec.parent,
     )?.entity.metadata.name;

diff --git a/plugins/keycloak-backend/src/providers/KeycloakOrgEntityProvider.ts b/plugins/keycloak-backend/src/providers/KeycloakOrgEntityProvider.ts
@@ -231,12 +231,17 @@ export class KeycloakOrgEntityProvider implements EntityProvider {
 
     await kcAdminClient.auth(credentials);
 
-    const { users, groups } = await readKeycloakRealm(kcAdminClient, provider, {
-      userQuerySize: provider.userQuerySize,
-      groupQuerySize: provider.groupQuerySize,
-      userTransformer: this.options.userTransformer,
-      groupTransformer: this.options.groupTransformer,
-    });
+    const { users, groups } = await readKeycloakRealm(
+      kcAdminClient,
+      provider,
+      logger,
+      {
+        userQuerySize: provider.userQuerySize,
+        groupQuerySize: provider.groupQuerySize,
+        userTransformer: this.options.userTransformer,
+        groupTransformer: this.options.groupTransformer,
+      },
+    );
 
     const { markCommitComplete } = markReadComplete({ users, groups });