From 70fa5ec3ec9eb52ffde1b613d67de6ea688692dc Mon Sep 17 00:00:00 2001 From: Phil Kuang Date: Mon, 21 Nov 2022 10:28:44 -0500 Subject: [PATCH] fix(GithubMultiOrgProcessor): correctly apply team memberships Signed-off-by: Phil Kuang --- .changeset/clever-rivers-obey.md | 5 ++ .../src/lib/org.test.ts | 2 +- .../src/lib/org.ts | 18 +++++-- .../GithubMultiOrgReaderProcessor.ts | 47 +++++++++++++++---- .../GithubOrgReaderProcessor.test.ts | 24 ++++++++-- 5 files changed, 77 insertions(+), 19 deletions(-) create mode 100644 .changeset/clever-rivers-obey.md diff --git a/.changeset/clever-rivers-obey.md b/.changeset/clever-rivers-obey.md new file mode 100644 index 0000000000..0fac8711e9 --- /dev/null +++ b/.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. diff --git a/plugins/catalog-backend-module-github/src/lib/org.test.ts b/plugins/catalog-backend-module-github/src/lib/org.test.ts index 712c7c0b88..1cdc55a4da 100644 --- a/plugins/catalog-backend-module-github/src/lib/org.test.ts +++ b/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'], }, }, { diff --git a/plugins/catalog-backend-module-github/src/lib/org.ts b/plugins/catalog-backend-module-github/src/lib/org.ts index 275c5f4aeb..4336f2d49c 100644 --- a/plugins/catalog-backend-module-github/src/lib/org.ts +++ b/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 = []; diff --git a/plugins/catalog-backend-module-github/src/processors/GithubMultiOrgReaderProcessor.ts b/plugins/catalog-backend-module-github/src/processors/GithubMultiOrgReaderProcessor.ts index 745f52d6c5..7777da6b1b 100644 --- a/plugins/catalog-backend-module-github/src/processors/GithubMultiOrgReaderProcessor.ts +++ b/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 => { + 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)); diff --git a/plugins/catalog-backend-module-github/src/processors/GithubOrgReaderProcessor.test.ts b/plugins/catalog-backend-module-github/src/processors/GithubOrgReaderProcessor.test.ts index 2bb0c538eb..ce81e2cf42 100644 --- a/plugins/catalog-backend-module-github/src/processors/GithubOrgReaderProcessor.test.ts +++ b/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' }], + }, + }, ], }, },