fix(GithubMultiOrgProcessor): correctly apply team memberships

Signed-off-by: Phil Kuang <pkuang@factset.com>

.changeset/clever-rivers-obey.md

@@ -0,0 +1,5 @@
+---
+'@backstage/plugin-catalog-backend-module-github': patch
+---
+
+Fixes the assignment of group member references in `GithubMultiOrgProcessor` so membership relations are resolved correctly.

plugins/catalog-backend-module-github/src/lib/org.test.ts

@@ -79,7 +79,7 @@ describe('assignGroupsToUsers', () => {
         spec: {
           type: 'team',
           children: [],
-          members: ['u1', 'u2'],
+          members: ['default/u1', 'default/u2'],
         },
       },
       {

plugins/catalog-backend-module-github/src/lib/org.ts

@@ -17,6 +17,8 @@
 import {
   DEFAULT_NAMESPACE,
   GroupEntity,
+  parseEntityRef,
+  stringifyEntityRef,
   UserEntity,
 } from '@backstage/catalog-model';
 
@@ -65,14 +67,20 @@ export function assignGroupsToUsers(
         group.metadata.namespace !== DEFAULT_NAMESPACE
           ? `${group.metadata.namespace}/${group.metadata.name}`
           : group.metadata.name;
-      return [groupKey, group.spec.members || []];
+      // Fully qualify member refs so they can be keyed off of since they may contain namespace prefixes
+      return [
+        groupKey,
+        group.spec.members?.map(m =>
+          stringifyEntityRef(parseEntityRef(m, { defaultKind: 'user' })),
+        ) || [],
+      ];
     }),
   );
 
-  const usersByName = new Map(users.map(u => [u.metadata.name, u]));
-  for (const [groupName, userNames] of groupMemberUsers.entries()) {
-    for (const userName of userNames) {
-      const user = usersByName.get(userName);
+  const usersByRef = new Map(users.map(u => [stringifyEntityRef(u), u]));
+  for (const [groupName, userRefs] of groupMemberUsers.entries()) {
+    for (const ref of userRefs) {
+      const user = usersByRef.get(ref);
       if (user && !user.spec.memberOf?.includes(groupName)) {
         if (!user.spec.memberOf) {
           user.spec.memberOf = [];

plugins/catalog-backend-module-github/src/processors/GithubMultiOrgReaderProcessor.ts

@@ -14,7 +14,12 @@
  * limitations under the License.
  */
 
-import { GroupEntity } from '@backstage/catalog-model';
+import {
+  DEFAULT_NAMESPACE,
+  GroupEntity,
+  stringifyEntityRef,
+  UserEntity,
+} from '@backstage/catalog-model';
 import { Config } from '@backstage/config';
 import {
   DefaultGithubCredentialsProvider,
@@ -36,6 +41,7 @@ import {
   assignGroupsToUsers,
   buildOrgHierarchy,
   defaultOrganizationTeamTransformer,
+  defaultUserTransformer,
   getOrganizationTeams,
   getOrganizationUsers,
   GithubMultiOrgConfig,
@@ -141,8 +147,19 @@ export class GithubMultiOrgReaderProcessor implements CatalogProcessor {
           client,
           orgConfig.name,
           tokenType,
-          this.options.userTransformer,
+          async (githubUser, ctx): Promise<UserEntity | undefined> => {
+            const result = this.options.userTransformer
+              ? await this.options.userTransformer(githubUser, ctx)
+              : await defaultUserTransformer(githubUser, ctx);
+
+            if (result) {
+              result.metadata.namespace = orgConfig.userNamespace;
+            }
+
+            return result;
+          },
         );
+
         const { groups } = await getOrganizationTeams(
           client,
           orgConfig.name,
@@ -153,6 +170,13 @@ export class GithubMultiOrgReaderProcessor implements CatalogProcessor {
 
             if (result) {
               result.metadata.namespace = orgConfig.groupNamespace;
+              // Group `spec.members` inherits the namespace of it's group so need to explicitly specify refs here
+              result.spec.members = team.members.map(
+                user =>
+                  `${orgConfig.userNamespace ?? DEFAULT_NAMESPACE}/${
+                    user.login
+                  }`,
+              );
             }
 
             return result;
@@ -164,15 +188,18 @@ export class GithubMultiOrgReaderProcessor implements CatalogProcessor {
           `Read ${users.length} GitHub users and ${groups.length} GitHub teams from ${orgConfig.name} in ${duration} seconds`,
         );
 
-        let prefix: string = orgConfig.userNamespace ?? '';
-        if (prefix.length > 0) prefix += '/';
-
-        users.forEach(u => {
-          if (!allUsersMap.has(prefix + u.metadata.name)) {
-            allUsersMap.set(prefix + u.metadata.name, u);
+        // Grab current users from `allUsersMap` if they already exist in our
+        // pending users so we can append to their group membership relations
+        const pendingUsers = users.map(u => {
+          const userRef = stringifyEntityRef(u);
+          if (!allUsersMap.has(userRef)) {
+            allUsersMap.set(userRef, u);
           }
+
+          return allUsersMap.get(userRef);
         });
-        assignGroupsToUsers(users, groups);
+
+        assignGroupsToUsers(pendingUsers, groups);
         buildOrgHierarchy(groups);
 
         for (const group of groups) {
@@ -185,6 +212,8 @@ export class GithubMultiOrgReaderProcessor implements CatalogProcessor {
       }
     }
 
+    // Emit all users at the end after all orgs have been processed
+    // so all memberships across org groups are accounted for
     const allUsers = Array.from(allUsersMap.values());
     for (const user of allUsers) {
       emit(processingResult.entity(location, user));

plugins/catalog-backend-module-github/src/processors/GithubOrgReaderProcessor.test.ts

@@ -85,7 +85,10 @@ describe('GithubOrgReaderProcessor', () => {
       mockClient
         .mockResolvedValueOnce({
           organization: {
-            membersWithRole: { pageInfo: { hasNextPage: false }, nodes: [{}] },
+            membersWithRole: {
+              pageInfo: { hasNextPage: false },
+              nodes: [{ login: 'foo' }],
+            },
           },
         })
         .mockResolvedValueOnce({
@@ -93,7 +96,12 @@ describe('GithubOrgReaderProcessor', () => {
             teams: {
               pageInfo: { hasNextPage: false },
               nodes: [
-                { members: { pageInfo: { hasNextPage: false }, nodes: [{}] } },
+                {
+                  members: {
+                    pageInfo: { hasNextPage: false },
+                    nodes: [{ login: 'foo' }],
+                  },
+                },
               ],
             },
           },
@@ -134,7 +142,10 @@ describe('GithubOrgReaderProcessor', () => {
       mockClient
         .mockResolvedValueOnce({
           organization: {
-            membersWithRole: { pageInfo: { hasNextPage: false }, nodes: [{}] },
+            membersWithRole: {
+              pageInfo: { hasNextPage: false },
+              nodes: [{ login: 'foo' }],
+            },
           },
         })
         .mockResolvedValueOnce({
@@ -142,7 +153,12 @@ describe('GithubOrgReaderProcessor', () => {
             teams: {
               pageInfo: { hasNextPage: false },
               nodes: [
-                { members: { pageInfo: { hasNextPage: false }, nodes: [{}] } },
+                {
+                  members: {
+                    pageInfo: { hasNextPage: false },
+                    nodes: [{ login: 'foo' }],
+                  },
+                },
               ],
             },
           },