diff --git a/.changeset/experimental-rest-suspended-check.md b/.changeset/experimental-rest-suspended-check.md new file mode 100644 index 0000000000..1bc7696c14 --- /dev/null +++ b/.changeset/experimental-rest-suspended-check.md @@ -0,0 +1,6 @@ +--- +'@backstage/plugin-catalog-backend-module-github': patch +'@backstage/plugin-catalog-backend-module-github-org': patch +--- + +Added experimental support for checking suspended users via the GitHub REST API instead of the GraphQL `suspendedAt` field. Enable by setting both `excludeSuspendedUsers: true` and `experimental_checkForSuspendedUsersWithRest: true` in the provider config. When enabled, responses are cached using conditional HTTP requests to minimize REST API rate limit usage. diff --git a/plugins/catalog-backend-module-github-org/src/module.ts b/plugins/catalog-backend-module-github-org/src/module.ts index 08a557e3b1..831cd6184a 100644 --- a/plugins/catalog-backend-module-github-org/src/module.ts +++ b/plugins/catalog-backend-module-github-org/src/module.ts @@ -94,13 +94,14 @@ export const catalogModuleGithubOrgEntityProvider = createBackendModule({ env.registerInit({ deps: { catalog: catalogProcessingExtensionPoint, + cache: coreServices.cache, config: coreServices.rootConfig, events: eventsServiceRef, logger: coreServices.logger, scheduler: coreServices.scheduler, }, - async init({ catalog, config, events, logger, scheduler }) { + async init({ catalog, cache, config, events, logger, scheduler }) { const definitions = readDefinitionsFromConfig(config); for (const definition of definitions) { @@ -127,6 +128,9 @@ export const catalogModuleGithubOrgEntityProvider = createBackendModule({ definitions.length === 1 && definition.orgs?.length === 1, pageSizes: definition.pageSizes, excludeSuspendedUsers: definition.excludeSuspendedUsers, + cache, + experimental_checkForSuspendedUsersWithRest: + definition.experimental_checkForSuspendedUsersWithRest, }), ); } @@ -146,6 +150,7 @@ function readDefinitionsFromConfig(rootConfig: Config): Array<{ organizationMembers?: number; }; excludeSuspendedUsers?: boolean; + experimental_checkForSuspendedUsersWithRest?: boolean; useVerifiedEmails?: boolean; }> { const baseKey = 'catalog.providers.githubOrg'; @@ -176,6 +181,9 @@ function readDefinitionsFromConfig(rootConfig: Config): Array<{ : undefined, excludeSuspendedUsers: c.getOptionalBoolean('excludeSuspendedUsers') ?? false, + experimental_checkForSuspendedUsersWithRest: + c.getOptionalBoolean('experimental_checkForSuspendedUsersWithRest') ?? + false, useVerifiedEmails: c.getOptionalBoolean('defaultUserTransformer.useVerifiedEmails') ?? false, })); diff --git a/plugins/catalog-backend-module-github/config.d.ts b/plugins/catalog-backend-module-github/config.d.ts index 6a9d5cf808..78bb14d2be 100644 --- a/plugins/catalog-backend-module-github/config.d.ts +++ b/plugins/catalog-backend-module-github/config.d.ts @@ -270,6 +270,14 @@ export interface Config { */ excludeSuspendedUsers?: boolean; + /** + * (Optional) When set to true alongside `excludeSuspendedUsers`, use the GitHub REST API + * to check for suspended users instead of the GraphQL `suspendedAt` field. + * REST responses are cached using conditional HTTP requests to minimize rate limit usage. + * Default: `false`. + */ + experimental_checkForSuspendedUsersWithRest?: boolean; + /** * (Optional) Configuration for the default user transformer. * These options only apply when using the built-in transformer; @@ -345,6 +353,14 @@ export interface Config { */ excludeSuspendedUsers?: boolean; + /** + * (Optional) When set to true alongside `excludeSuspendedUsers`, use the GitHub REST API + * to check for suspended users instead of the GraphQL `suspendedAt` field. + * REST responses are cached using conditional HTTP requests to minimize rate limit usage. + * Default: `false`. + */ + experimental_checkForSuspendedUsersWithRest?: boolean; + /** * (Optional) Configuration for the default user transformer. * These options only apply when using the built-in transformer; diff --git a/plugins/catalog-backend-module-github/report.api.md b/plugins/catalog-backend-module-github/report.api.md index a2642bb1f4..3307b3a39e 100644 --- a/plugins/catalog-backend-module-github/report.api.md +++ b/plugins/catalog-backend-module-github/report.api.md @@ -6,6 +6,7 @@ import { AnalyzeOptions } from '@backstage/plugin-catalog-node'; import { AuthService } from '@backstage/backend-plugin-api'; import { BackendFeature } from '@backstage/backend-plugin-api'; +import { CacheService } from '@backstage/backend-plugin-api'; import { CatalogProcessor } from '@backstage/plugin-catalog-node'; import { CatalogProcessorEmit } from '@backstage/plugin-catalog-node'; import { CatalogService } from '@backstage/plugin-catalog-node'; @@ -158,6 +159,8 @@ export class GithubMultiOrgEntityProvider implements EntityProvider { alwaysUseDefaultNamespace?: boolean; pageSizes?: Partial; excludeSuspendedUsers?: boolean; + cache?: CacheService; + experimental_checkForSuspendedUsersWithRest?: boolean; }); connect(connection: EntityProviderConnection): Promise; // (undocumented) @@ -172,8 +175,10 @@ export class GithubMultiOrgEntityProvider implements EntityProvider { // @public export interface GithubMultiOrgEntityProviderOptions { alwaysUseDefaultNamespace?: boolean; + cache?: CacheService; events?: EventsService; excludeSuspendedUsers?: boolean; + experimental_checkForSuspendedUsersWithRest?: boolean; githubCredentialsProvider?: GithubCredentialsProvider; githubUrl: string; id: string; @@ -237,6 +242,8 @@ export class GithubOrgEntityProvider implements EntityProvider { teamTransformer?: TeamTransformer; pageSizes?: Partial; excludeSuspendedUsers?: boolean; + cache?: CacheService; + experimental_checkForSuspendedUsersWithRest?: boolean; }); connect(connection: EntityProviderConnection): Promise; // (undocumented) @@ -253,8 +260,10 @@ export type GitHubOrgEntityProviderOptions = GithubOrgEntityProviderOptions; // @public export interface GithubOrgEntityProviderOptions { + cache?: CacheService; events?: EventsService; excludeSuspendedUsers?: boolean; + experimental_checkForSuspendedUsersWithRest?: boolean; githubCredentialsProvider?: GithubCredentialsProvider; id: string; logger: LoggerService; diff --git a/plugins/catalog-backend-module-github/src/lib/github.test.ts b/plugins/catalog-backend-module-github/src/lib/github.test.ts index 2b9c35be85..d6d2aff7bb 100644 --- a/plugins/catalog-backend-module-github/src/lib/github.test.ts +++ b/plugins/catalog-backend-module-github/src/lib/github.test.ts @@ -20,7 +20,8 @@ import { } from '@backstage/backend-test-utils'; import { GroupEntity, UserEntity } from '@backstage/catalog-model'; import { graphql as graphqlOctokit } from '@octokit/graphql'; -import { graphql as graphqlMsw, HttpResponse } from 'msw'; +import { CacheService } from '@backstage/backend-plugin-api'; +import { graphql as graphqlMsw, http, HttpResponse } from 'msw'; import { setupServer } from 'msw/node'; import { TeamTransformer, UserTransformer } from './defaultTransformers'; import { @@ -37,18 +38,18 @@ import { createRemoveEntitiesOperation, createReplaceEntitiesOperation, createGraphqlClient, + createRestClient, getOrganizationTeamsForUser, + isSuspended, + isGitHubEnterprise, } from './github'; import { Octokit } from '@octokit/core'; import { throttling } from '@octokit/plugin-throttling'; import { retry } from '@octokit/plugin-retry'; -jest.mock('@octokit/core', () => ({ - ...jest.requireActual('@octokit/core'), - Octokit: { - plugin: jest.fn().mockReturnValue({ defaults: jest.fn() }), - }, -})); +// Note: We do NOT mock @octokit/core globally because createRestClient +// needs a real Octokit.plugin. The createGraphqlClient tests use a local +// jest.spyOn instead. describe('github', () => { const server = setupServer(); @@ -216,6 +217,200 @@ describe('github', () => { getOrganizationUsers(graphql, 'a', 'token', undefined, undefined, true), ).resolves.toEqual(output); }); + + it('reads members excluding suspended users via REST when restClient provided', async () => { + const input: QueryResponse = { + organization: { + membersWithRole: { + pageInfo: { hasNextPage: false }, + nodes: [ + { + login: 'suspended-user', + name: 'b', + bio: 'c', + email: 'd', + avatarUrl: 'e', + }, + { + login: 'active-user', + name: 'b', + bio: 'c', + email: 'd', + avatarUrl: 'e', + }, + ], + }, + }, + }; + + const output = { + users: [ + expect.objectContaining({ + metadata: expect.objectContaining({ + name: 'active-user', + description: 'c', + }), + spec: { + profile: { displayName: 'b', email: 'd', picture: 'e' }, + memberOf: [], + }, + }), + ], + }; + + server.use( + graphqlMsw.query('users', () => HttpResponse.json({ data: input })), + ); + + const mockRestClient = { + request: jest.fn().mockImplementation((route: string, params: any) => { + if (route === 'GET /versions') { + return Promise.resolve({ + headers: { 'x-github-enterprise-version': '3.12.0' }, + }); + } + if (route === 'GET /users/{username}') { + if (params.username === 'suspended-user') { + return { data: { suspended_at: '2025-01-01T00:00:00Z' } }; + } + return { data: { suspended_at: null } }; + } + return { data: { role: 'member', state: 'active' } }; + }), + } as any; + + await expect( + getOrganizationUsers( + graphql, + 'a', + 'token', + undefined, + undefined, + true, + mockRestClient, + ), + ).resolves.toEqual(output); + }); + + it('reads members excluding org-membership-suspended users via REST', async () => { + const input: QueryResponse = { + organization: { + membersWithRole: { + pageInfo: { hasNextPage: false }, + nodes: [ + { + login: 'org-suspended-user', + name: 'b', + bio: 'c', + email: 'd', + avatarUrl: 'e', + }, + { + login: 'active-user', + name: 'b', + bio: 'c', + email: 'd', + avatarUrl: 'e', + }, + ], + }, + }, + }; + + server.use( + graphqlMsw.query('users', () => HttpResponse.json({ data: input })), + ); + + const mockRestClient = { + request: jest.fn().mockImplementation((route: string, params: any) => { + if (route === 'GET /versions') { + return Promise.resolve({ + headers: { 'x-github-enterprise-version': '3.12.0' }, + }); + } + if (route === 'GET /users/{username}') { + return { data: { suspended_at: null } }; + } + if (route === 'GET /orgs/{org}/memberships/{username}') { + if (params.username === 'org-suspended-user') { + return { data: { role: 'suspended', state: 'active' } }; + } + return { data: { role: 'member', state: 'active' } }; + } + return { data: {} }; + }), + } as any; + + const result = await getOrganizationUsers( + graphql, + 'a', + 'token', + undefined, + undefined, + true, + mockRestClient, + ); + + expect(result.users).toHaveLength(1); + expect(result.users[0].metadata.name).toBe('active-user'); + }); + + it('skips REST suspended user check on non-enterprise GitHub', async () => { + const input: QueryResponse = { + organization: { + membersWithRole: { + pageInfo: { hasNextPage: false }, + nodes: [ + { + login: 'suspended-user', + name: 'b', + bio: 'c', + email: 'd', + avatarUrl: 'e', + }, + { + login: 'active-user', + name: 'b', + bio: 'c', + email: 'd', + avatarUrl: 'e', + }, + ], + }, + }, + }; + + server.use( + graphqlMsw.query('users', () => HttpResponse.json({ data: input })), + ); + + const nonEnterpriseRestClient = { + request: jest.fn().mockImplementation((route: string) => { + if (route === 'GET /versions') { + return Promise.resolve({ headers: {} }); + } + throw new Error('isSuspended should not be called'); + }), + } as any; + + const result = await getOrganizationUsers( + graphql, + 'a', + 'token', + undefined, + undefined, + true, + nonEnterpriseRestClient, + ); + + // Both users should be returned because the REST check is skipped + // on non-enterprise (no suspendedAt field in query either since restClient is provided) + expect(result.users).toHaveLength(2); + expect(nonEnterpriseRestClient.request).toHaveBeenCalledTimes(1); + expect(nonEnterpriseRestClient.request).toHaveBeenCalledWith( + 'GET /versions', + ); + }); }); describe('getOrganizationUsers using custom UserTransformer', () => { @@ -999,23 +1194,40 @@ describe('github', () => { defaults: graphqlDefaults, }, })); - (Octokit.plugin as jest.Mock).mockReturnValue(mockedOctokit); + + let pluginSpy: jest.SpyInstance; + + beforeEach(() => { + pluginSpy = jest + .spyOn(Octokit, 'plugin') + .mockReturnValue(mockedOctokit as any); + }); + + afterEach(() => { + pluginSpy.mockRestore(); + }); const rateLimitOptions = { method: 'POST', url: '/graphql', }; - const client = createGraphqlClient({ - headers, - baseUrl, - logger, - }); + it('should return a graphql client with throttling and retry', async () => { + const client = createGraphqlClient({ + headers, + baseUrl, + logger, + }); expect(client).toBeDefined(); expect(Octokit.plugin).toHaveBeenCalledWith(throttling, retry); }); it('should return a graphql client with the correct options', async () => { + createGraphqlClient({ + headers, + baseUrl, + logger, + }); expect(graphqlDefaults).toHaveBeenCalledWith({ baseUrl, headers, @@ -1028,6 +1240,8 @@ describe('github', () => { { retryCount: 1, expectedResult: true }, { retryCount: 2, expectedResult: false }, ])('should return %s', async ({ retryCount, expectedResult }) => { + createGraphqlClient({ headers, baseUrl, logger }); + const throttleOptions = mockedOctokit.mock.calls[0][0].throttle; const result = throttleOptions.onRateLimit( @@ -1047,6 +1261,8 @@ describe('github', () => { { retryCount: 1, expectedResult: true }, { retryCount: 2, expectedResult: false }, ])('should return %s', async ({ retryCount, expectedResult }) => { + createGraphqlClient({ headers, baseUrl, logger }); + const throttleOptions = mockedOctokit.mock.calls[0][0].throttle; const result = throttleOptions.onSecondaryRateLimit( @@ -1061,6 +1277,407 @@ describe('github', () => { }); }); + describe('createRestClient', () => { + const baseUrl = 'https://api.github.com'; + const orgUrl = 'https://github.com/my-org'; + + const mockCredentialsProvider = { + getCredentials: jest.fn().mockResolvedValue({ + type: 'token' as const, + headers: { authorization: 'token test-token' }, + }), + }; + + describe('conditional request caching', () => { + function createMockCache(): CacheService & { + store: Map; + } { + const store = new Map(); + const cache: CacheService & { store: Map } = { + store, + async get(key: string) { + return store.get(key) as any; + }, + async set(key: string, value: unknown) { + store.set(key, value); + }, + async delete(key: string) { + store.delete(key); + }, + withOptions() { + return cache; + }, + }; + return cache; + } + + it('caches responses using last-modified header', async () => { + let requestCount = 0; + server.use( + http.get(`${baseUrl}/users/testuser`, () => { + requestCount++; + return HttpResponse.json( + { login: 'testuser', suspended_at: null }, + { headers: { 'Last-Modified': 'Thu, 01 Jan 2025 00:00:00 GMT' } }, + ); + }), + ); + + const cache = createMockCache(); + const octokit = createRestClient({ + baseUrl, + orgUrl, + credentialsProvider: mockCredentialsProvider, + logger: mockServices.logger.mock(), + cache, + }); + + await octokit.request('GET /users/{username}', { + username: 'testuser', + }); + + expect(requestCount).toBe(1); + const cached = cache.store.get( + `catalog-backend-module-github:GET:${baseUrl}/users/testuser`, + ) as any; + expect(cached.lastModified).toBe('Thu, 01 Jan 2025 00:00:00 GMT'); + expect(cached.data).toEqual({ login: 'testuser', suspended_at: null }); + }); + + it('sends if-modified-since on subsequent requests', async () => { + let receivedHeaders: Record = {}; + server.use( + http.get(`${baseUrl}/users/testuser`, ({ request }) => { + receivedHeaders = Object.fromEntries(request.headers.entries()); + return HttpResponse.json( + { login: 'testuser', suspended_at: null }, + { headers: { 'Last-Modified': 'Thu, 01 Jan 2025 00:00:00 GMT' } }, + ); + }), + ); + + const cache = createMockCache(); + cache.store.set( + `catalog-backend-module-github:GET:${baseUrl}/users/testuser`, + { + lastModified: 'Wed, 01 Jan 2025 00:00:00 GMT', + data: { login: 'testuser', suspended_at: null }, + }, + ); + + const octokit = createRestClient({ + baseUrl, + orgUrl, + credentialsProvider: mockCredentialsProvider, + logger: mockServices.logger.mock(), + cache, + }); + + await octokit.request('GET /users/{username}', { + username: 'testuser', + }); + + expect(receivedHeaders['if-modified-since']).toBe( + 'Wed, 01 Jan 2025 00:00:00 GMT', + ); + }); + + it('sends if-none-match when only etag is cached', async () => { + let receivedHeaders: Record = {}; + server.use( + http.get(`${baseUrl}/users/testuser`, ({ request }) => { + receivedHeaders = Object.fromEntries(request.headers.entries()); + return HttpResponse.json( + { login: 'testuser', suspended_at: null }, + { headers: { ETag: '"new-etag"' } }, + ); + }), + ); + + const cache = createMockCache(); + cache.store.set( + `catalog-backend-module-github:GET:${baseUrl}/users/testuser`, + { + etag: '"old-etag"', + data: { login: 'testuser', suspended_at: null }, + }, + ); + + const octokit = createRestClient({ + baseUrl, + orgUrl, + credentialsProvider: mockCredentialsProvider, + logger: mockServices.logger.mock(), + cache, + }); + + await octokit.request('GET /users/{username}', { + username: 'testuser', + }); + + expect(receivedHeaders['if-none-match']).toBe('"old-etag"'); + expect(receivedHeaders['if-modified-since']).toBeUndefined(); + }); + + it('returns cached data and headers on 304 response', async () => { + const cachedData = { login: 'testuser', suspended_at: null }; + const cachedHeaders = { + 'x-github-enterprise-version': '3.12.0', + 'last-modified': 'Thu, 01 Jan 2025 00:00:00 GMT', + }; + + server.use( + http.get(`${baseUrl}/users/testuser`, () => { + return new HttpResponse(null, { status: 304 }); + }), + ); + + const cache = createMockCache(); + cache.store.set( + `catalog-backend-module-github:GET:${baseUrl}/users/testuser`, + { + lastModified: 'Thu, 01 Jan 2025 00:00:00 GMT', + headers: cachedHeaders, + data: cachedData, + }, + ); + + const octokit = createRestClient({ + baseUrl, + orgUrl, + credentialsProvider: mockCredentialsProvider, + logger: mockServices.logger.mock(), + cache, + }); + + const response = await octokit.request('GET /users/{username}', { + username: 'testuser', + }); + + expect(response.data).toEqual(cachedData); + expect(response.headers['x-github-enterprise-version']).toBe('3.12.0'); + }); + + it('propagates non-304 errors', async () => { + server.use( + http.get(`${baseUrl}/users/testuser`, () => { + return new HttpResponse(JSON.stringify({ message: 'Not Found' }), { + status: 404, + headers: { 'Content-Type': 'application/json' }, + }); + }), + ); + + const cache = createMockCache(); + const octokit = createRestClient({ + baseUrl, + orgUrl, + credentialsProvider: mockCredentialsProvider, + logger: mockServices.logger.mock(), + cache, + }); + + await expect( + octokit.request('GET /users/{username}', { username: 'testuser' }), + ).rejects.toThrow(); + }); + + it('prefers last-modified over etag for conditional headers', async () => { + let receivedHeaders: Record = {}; + server.use( + http.get(`${baseUrl}/users/testuser`, ({ request }) => { + receivedHeaders = Object.fromEntries(request.headers.entries()); + return HttpResponse.json({ login: 'testuser' }); + }), + ); + + const cache = createMockCache(); + cache.store.set( + `catalog-backend-module-github:GET:${baseUrl}/users/testuser`, + { + lastModified: 'Thu, 01 Jan 2025 00:00:00 GMT', + etag: '"some-etag"', + data: { login: 'testuser' }, + }, + ); + + const octokit = createRestClient({ + baseUrl, + orgUrl, + credentialsProvider: mockCredentialsProvider, + logger: mockServices.logger.mock(), + cache, + }); + + await octokit.request('GET /users/{username}', { + username: 'testuser', + }); + + expect(receivedHeaders['if-modified-since']).toBe( + 'Thu, 01 Jan 2025 00:00:00 GMT', + ); + expect(receivedHeaders['if-none-match']).toBeUndefined(); + }); + + it('uses distinct cache keys per user', async () => { + server.use( + http.get(`${baseUrl}/users/:username`, ({ params }) => { + return HttpResponse.json( + { login: params.username, suspended_at: null }, + { headers: { 'Last-Modified': 'Thu, 01 Jan 2025 00:00:00 GMT' } }, + ); + }), + ); + + const cache = createMockCache(); + const octokit = createRestClient({ + baseUrl, + orgUrl, + credentialsProvider: mockCredentialsProvider, + logger: mockServices.logger.mock(), + cache, + }); + + await octokit.request('GET /users/{username}', { + username: 'user-a', + }); + await octokit.request('GET /users/{username}', { + username: 'user-b', + }); + + expect( + cache.store.has( + `catalog-backend-module-github:GET:${baseUrl}/users/user-a`, + ), + ).toBe(true); + expect( + cache.store.has( + `catalog-backend-module-github:GET:${baseUrl}/users/user-b`, + ), + ).toBe(true); + }); + + it('works without a cache', async () => { + server.use( + http.get(`${baseUrl}/users/testuser`, () => { + return HttpResponse.json({ login: 'testuser' }); + }), + ); + + const octokit = createRestClient({ + baseUrl, + orgUrl, + credentialsProvider: mockCredentialsProvider, + logger: mockServices.logger.mock(), + }); + + const response = await octokit.request('GET /users/{username}', { + username: 'testuser', + }); + + expect(response.data).toEqual({ login: 'testuser' }); + }); + }); + }); + + describe('isSuspended', () => { + it('returns true when the user account is suspended', async () => { + const octokit = { + request: jest.fn().mockImplementation((route: string) => { + if (route === 'GET /users/{username}') { + return { data: { suspended_at: '2025-01-01T00:00:00Z' } }; + } + return { data: { role: 'member', state: 'active' } }; + }), + } as any; + + await expect( + isSuspended('suspended-user', octokit, { org: 'my-org' }), + ).resolves.toBe(true); + }); + + it('returns false for an active user', async () => { + const octokit = { + request: jest.fn().mockImplementation((route: string) => { + if (route === 'GET /users/{username}') { + return { data: { suspended_at: null } }; + } + return { data: { role: 'member', state: 'active' } }; + }), + } as any; + + await expect( + isSuspended('active-user', octokit, { org: 'my-org' }), + ).resolves.toBe(false); + }); + + it('returns true when org membership is suspended', async () => { + const octokit = { + request: jest.fn().mockImplementation((route: string) => { + if (route === 'GET /users/{username}') { + return { data: { suspended_at: null } }; + } + return { data: { role: 'suspended', state: 'active' } }; + }), + } as any; + + await expect( + isSuspended('org-suspended', octokit, { org: 'my-org' }), + ).resolves.toBe(true); + }); + + it('checks both user suspension and org membership', async () => { + const octokit = { + request: jest.fn().mockImplementation((route: string) => { + if (route === 'GET /users/{username}') { + return { data: { suspended_at: null } }; + } + return { data: { role: 'member', state: 'active' } }; + }), + } as any; + + await isSuspended('some-user', octokit, { org: 'my-org' }); + + expect(octokit.request).toHaveBeenCalledTimes(2); + expect(octokit.request).toHaveBeenCalledWith('GET /users/{username}', { + username: 'some-user', + }); + expect(octokit.request).toHaveBeenCalledWith( + 'GET /orgs/{org}/memberships/{username}', + { org: 'my-org', username: 'some-user' }, + ); + }); + }); + + describe('isGitHubEnterprise', () => { + it('returns true when x-github-enterprise-version header is present', async () => { + const octokit = { + request: jest.fn().mockResolvedValue({ + headers: { 'x-github-enterprise-version': '3.12.0' }, + }), + } as any; + + await expect(isGitHubEnterprise(octokit)).resolves.toBe(true); + expect(octokit.request).toHaveBeenCalledWith('GET /versions'); + }); + + it('returns false when x-github-enterprise-version header is absent', async () => { + const octokit = { + request: jest.fn().mockResolvedValue({ headers: {} }), + } as any; + + await expect(isGitHubEnterprise(octokit)).resolves.toBe(false); + }); + + it('returns false when the request throws', async () => { + const octokit = { + request: jest.fn().mockRejectedValue(new Error('Not Found')), + } as any; + + await expect(isGitHubEnterprise(octokit)).resolves.toBe(false); + }); + }); + describe('Page sizes configuration', () => { const org = 'my-org'; diff --git a/plugins/catalog-backend-module-github/src/lib/github.ts b/plugins/catalog-backend-module-github/src/lib/github.ts index fc72004d9c..be8407c5c2 100644 --- a/plugins/catalog-backend-module-github/src/lib/github.ts +++ b/plugins/catalog-backend-module-github/src/lib/github.ts @@ -15,8 +15,13 @@ */ import { Entity } from '@backstage/catalog-model'; -import { GithubCredentialType } from '@backstage/integration'; +import { + GithubCredentials, + GithubCredentialsProvider, + GithubCredentialType, +} from '@backstage/integration'; import { graphql } from '@octokit/graphql'; +import { OctokitResponse, RequestParameters } from '@octokit/types'; import { defaultOrganizationTeamTransformer, defaultUserTransformer, @@ -28,9 +33,10 @@ import { withLocations } from './withLocations'; import { DeferredEntity } from '@backstage/plugin-catalog-node'; import { Octokit } from '@octokit/core'; -import { LoggerService } from '@backstage/backend-plugin-api'; -import { throttling } from '@octokit/plugin-throttling'; +import { CacheService, LoggerService } from '@backstage/backend-plugin-api'; +import { throttling, ThrottlingOptions } from '@octokit/plugin-throttling'; import { retry } from '@octokit/plugin-retry'; +import { JsonValue } from '@backstage/types'; /** * Configuration for GitHub GraphQL API page sizes. @@ -185,6 +191,7 @@ export type Connection = { * @param userTransformer - Optional transformer for user entities * @param pageSizes - Optional page sizes configuration * @param excludeSuspendedUsers - Optional flag to exclude suspended users (only for GitHub Enterprise instances) + * @param restClient - Optional Octokit REST client; when provided alongside excludeSuspendedUsers, the REST API is used instead of the GraphQL suspendedAt field */ export async function getOrganizationUsers( client: typeof graphql, @@ -193,8 +200,11 @@ export async function getOrganizationUsers( userTransformer: UserTransformer = defaultUserTransformer, pageSizes: GithubPageSizes = DEFAULT_PAGE_SIZES, excludeSuspendedUsers: boolean = false, + restClient?: Octokit, ): Promise<{ users: Entity[] }> { - const suspendedAtField = excludeSuspendedUsers ? 'suspendedAt,' : ''; + const useRestForSuspension = excludeSuspendedUsers && !!restClient; + const suspendedAtField = + excludeSuspendedUsers && !useRestForSuspension ? 'suspendedAt,' : ''; const query = ` query users($org: String!, $email: Boolean!, $cursor: String, $organizationMembersPageSize: Int!) { organization(login: $org) { @@ -214,6 +224,18 @@ export async function getOrganizationUsers( } }`; + let restFilter: + | ((user: GithubUser) => Promise | boolean) + | undefined; + + if (useRestForSuspension) { + const isEnterprise = await isGitHubEnterprise(restClient); + if (isEnterprise) { + restFilter = async (user: GithubUser) => + !(await isSuspended(user.login, restClient, { org })); + } + } + // There is no user -> teams edge, so we leave the memberships empty for // now and let the team iteration handle it instead @@ -228,7 +250,9 @@ export async function getOrganizationUsers( email: tokenType === 'token', organizationMembersPageSize: pageSizes.organizationMembers, }, - filter: u => (excludeSuspendedUsers ? !u.suspendedAt : true), + filter: useRestForSuspension + ? restFilter + : u => (excludeSuspendedUsers ? !u.suspendedAt : true), }); return { users }; @@ -793,7 +817,7 @@ export async function queryWithPaging< ctx: TransformerContext, ) => Promise; variables: Variables; - filter?: (item: GraphqlType) => boolean; + filter?: (item: GraphqlType) => Promise | boolean; }): Promise { const { client, query, org, connection, transformer, variables, filter } = params; @@ -813,7 +837,7 @@ export async function queryWithPaging< } for (const node of conn.nodes) { - if (filter && !filter(node)) { + if (filter && !(await filter(node))) { continue; } const transformedNode = await transformer(node, { @@ -821,7 +845,6 @@ export async function queryWithPaging< query, org, }); - if (transformedNode) { result.push(transformedNode); } @@ -928,3 +951,200 @@ export const createGraphqlClient = (args: { return client; }; + +function octokitThrottlingOptions(logger: LoggerService): ThrottlingOptions { + return { + onRateLimit: (retryAfter, rateLimitData, _, retryCount) => { + logger.warn( + `Request quota exhausted for request ${rateLimitData?.method} ${rateLimitData?.url}`, + ); + + if (retryCount < 2) { + logger.warn( + `Retrying after ${retryAfter} seconds for the ${retryCount} time due to Rate Limit!`, + ); + return true; + } + + return false; + }, + onSecondaryRateLimit: (retryAfter, rateLimitData, _, retryCount) => { + logger.warn( + `Secondary Rate Limit Exhausted for request ${rateLimitData?.method} ${rateLimitData?.url}`, + ); + + if (retryCount < 2) { + logger.warn( + `Retrying after ${retryAfter} seconds for the ${retryCount} time due to Secondary Rate Limit!`, + ); + return true; + } + + return false; + }, + }; +} + +/** + * Creates an Octokit REST client with throttling, retry, and optional + * conditional request caching. + * + * @public + */ +export function createRestClient(options: { + baseUrl: string | undefined; + orgUrl: string; + credentialsProvider: GithubCredentialsProvider; + logger: LoggerService; + cache?: CacheService; +}): Octokit & { auth: () => Promise } { + const getCredentials = () => + options.credentialsProvider.getCredentials({ + url: options.orgUrl, + }); + + const authStrategy = () => { + const auth = () => getCredentials(); + auth.hook = async ( + request: ( + requestOptions: RequestParameters, + ) => Promise>, + hookOptions: RequestParameters, + ) => { + const { headers } = await getCredentials(); + return request({ + ...hookOptions, + headers: { ...hookOptions.headers, ...headers }, + }); + }; + return auth; + }; + + const ThrottledOctokit = Octokit.plugin(throttling, retry); + + const octokit = new ThrottledOctokit({ + baseUrl: options.baseUrl, + authStrategy, + throttle: octokitThrottlingOptions(options.logger), + }); + + if (options.cache) { + installConditionalRequestCache(octokit, options.cache); + } + + return octokit as Octokit & { auth: () => Promise }; +} + +type CachedGitHubResponse = { + lastModified?: string; + etag?: string; + headers: JsonValue; + data: JsonValue; +}; + +function installConditionalRequestCache( + octokit: Octokit, + cache: CacheService, +): void { + octokit.hook.wrap('request', async (request, wrappedOptions) => { + const resolvedUrl = (wrappedOptions.url || '').replace( + /\{([^}]+)\}/g, + (_, key) => encodeURIComponent((wrappedOptions as any)[key]), + ); + const cacheKey = `catalog-backend-module-github:${wrappedOptions.method}:${wrappedOptions.baseUrl}${resolvedUrl}`; + const cached = await cache + .get(cacheKey) + .catch(() => undefined); + + if (cached?.lastModified) { + wrappedOptions.headers['if-modified-since'] = cached.lastModified; + } else if (cached?.etag) { + wrappedOptions.headers['if-none-match'] = cached.etag; + } + + try { + const response = await request(wrappedOptions); + + const lastModified = response.headers['last-modified']; + const etag = response.headers.etag; + + if (lastModified || etag) { + cache + .set( + cacheKey, + { + lastModified, + etag, + headers: JSON.parse(JSON.stringify(response.headers)), + data: JSON.parse(JSON.stringify(response.data)), + }, + // The TTL can be long here, since we only use the cache for + // conditional GitHub requests - it's never returned unless we get a + // 304 back from GitHub indicating that it's still correct. + { ttl: { years: 1 } }, + ) + .catch(() => {}); + } + return response; + } catch (error: any) { + if (error?.status === 304 && cached) { + return { + ...error.response, + headers: cached.headers, + data: cached.data, + }; + } + throw error; + } + }); +} + +/** + * Checks whether a user is suspended via the REST API. + */ +export async function isSuspended( + username: string, + octokit: Octokit, + options: { org: string }, +): Promise { + const [userResponse, membershipResponse] = await Promise.all([ + octokit.request('GET /users/{username}', { username }), + octokit.request('GET /orgs/{org}/memberships/{username}', { + org: options.org, + username, + }), + ]); + + // Octokit types are based on the public GitHub API, and since public GitHub + // doesn't include the ability to suspend users, there's no "suspended_at" + // field on the type, nor a "suspended" role on org memberships. However these + // fields are present for GitHub Enterprise, so we augment the types to + // include them. + const userSuspendedAt = ( + userResponse.data as typeof userResponse.data & { suspended_at?: string } + ).suspended_at; + const membershipRole = ( + membershipResponse.data as + | typeof membershipResponse.data + | { + role?: 'suspended'; + } + ).role; + + const userSuspended = !!userSuspendedAt; + const orgMembershipSuspended = membershipRole === 'suspended'; + + return userSuspended || orgMembershipSuspended; +} + +/** + * Checks whether the GitHub instance is a GitHub Enterprise server. + */ +export async function isGitHubEnterprise(octokit: Octokit): Promise { + try { + const response = await octokit.request('GET /versions'); + return !!response.headers['x-github-enterprise-version']; + } catch { + return false; + } +} diff --git a/plugins/catalog-backend-module-github/src/lib/index.ts b/plugins/catalog-backend-module-github/src/lib/index.ts index 8806604888..14bcd3e962 100644 --- a/plugins/catalog-backend-module-github/src/lib/index.ts +++ b/plugins/catalog-backend-module-github/src/lib/index.ts @@ -20,6 +20,7 @@ export { getOrganizationRepositories, getOrganizationTeams, getOrganizationUsers, + createRestClient, type GithubUser, type GithubTeam, type GithubPageSizes, diff --git a/plugins/catalog-backend-module-github/src/providers/GithubMultiOrgEntityProvider.test.ts b/plugins/catalog-backend-module-github/src/providers/GithubMultiOrgEntityProvider.test.ts index cf4527193c..a48071e177 100644 --- a/plugins/catalog-backend-module-github/src/providers/GithubMultiOrgEntityProvider.test.ts +++ b/plugins/catalog-backend-module-github/src/providers/GithubMultiOrgEntityProvider.test.ts @@ -29,8 +29,19 @@ import { } from './GithubMultiOrgEntityProvider'; import { LoggerService } from '@backstage/backend-plugin-api'; import { mockServices } from '@backstage/backend-test-utils'; +import { + createRestClient, + isGitHubEnterprise, + isSuspended, +} from '../lib/github'; jest.mock('@octokit/graphql'); +jest.mock('../lib/github', () => ({ + ...jest.requireActual('../lib/github'), + createRestClient: jest.fn(), + isGitHubEnterprise: jest.fn(), + isSuspended: jest.fn(), +})); const getAllInstallationsMock = jest.fn(); jest.mock('@backstage/integration', () => ({ @@ -2471,5 +2482,146 @@ describe('GithubMultiOrgEntityProvider', () => { }); }); }); + + describe('suspended user handling', () => { + let suspendedEvents: EventsService; + let suspendedConnection: EntityProviderConnection; + + beforeEach(async () => { + const logger = mockServices.logger.mock(); + suspendedEvents = DefaultEventsService.create({ logger }); + + suspendedConnection = { + applyMutation: jest.fn(), + refresh: jest.fn(), + }; + + const config = new ConfigReader({ + integrations: { + github: [{ host: 'github.com' }], + }, + }); + + const mockGetCredentials = jest.fn().mockReturnValue({ + headers: { token: 'blah' }, + type: 'app', + }); + + (createRestClient as jest.Mock).mockReturnValue({}); + (isGitHubEnterprise as jest.Mock).mockResolvedValue(true); + (isSuspended as jest.Mock).mockResolvedValue(true); + + const entityProvider = GithubMultiOrgEntityProvider.fromConfig(config, { + events: suspendedEvents, + id: 'my-id', + githubCredentialsProvider: { getCredentials: mockGetCredentials }, + githubUrl: 'https://github.com', + logger, + orgs: ['orgA', 'orgB'], + excludeSuspendedUsers: true, + experimental_checkForSuspendedUsersWithRest: true, + cache: mockServices.cache.mock(), + }); + + await entityProvider.connect(suspendedConnection); + }); + + it('should skip adding a suspended user on member_added event', async () => { + await suspendedEvents.publish({ + topic: 'github.organization', + eventPayload: { + action: 'member_added', + organization: { login: 'orgA' }, + membership: { + user: { + name: 'a', + node_id: 'f', + avatar_url: 'https://example.com/avatar', + email: 'a@test.com', + login: 'a', + }, + }, + }, + }); + + expect(suspendedConnection.applyMutation).not.toHaveBeenCalled(); + expect(isSuspended).toHaveBeenCalledWith('a', expect.anything(), { + org: 'orgA', + }); + }); + + it('should exclude suspended user on membership event', async () => { + const mockClient = jest.fn(); + + mockClient.mockResolvedValueOnce({ + organization: { + team: { + slug: 'team', + combinedSlug: 'orgA/team', + name: 'Team', + description: 'The team', + avatarUrl: 'http://example.com/team.jpeg', + parentTeam: null, + members: { + pageInfo: { hasNextPage: false }, + nodes: [{ login: 'a' }], + }, + }, + }, + }); + + (graphql.defaults as jest.Mock).mockReturnValue(mockClient); + + await suspendedEvents.publish({ + topic: 'github.membership', + eventPayload: { + action: 'added', + team: { + name: 'Team', + slug: 'team', + description: 'The team', + html_url: 'https://github.com/orgs/orgA/teams/team', + parent: null, + }, + member: { + login: 'a', + avatar_url: 'https://example.com/avatar', + email: 'a@test.com', + name: 'a', + node_id: 'f', + }, + organization: { login: 'orgA' }, + }, + }); + + await new Promise(process.nextTick); + + expect(suspendedConnection.applyMutation).toHaveBeenCalledTimes(1); + expect(suspendedConnection.applyMutation).toHaveBeenCalledWith({ + type: 'delta', + added: [ + { + locationKey: 'github-multi-org-provider:my-id', + entity: expect.objectContaining({ + kind: 'Group', + metadata: expect.objectContaining({ name: 'team' }), + }), + }, + ], + removed: [ + { + locationKey: 'github-multi-org-provider:my-id', + entity: expect.objectContaining({ + kind: 'User', + metadata: expect.objectContaining({ name: 'a' }), + }), + }, + ], + }); + expect(isSuspended).toHaveBeenCalledWith('a', expect.anything(), { + org: 'orgA', + }); + }); + }); }); }); diff --git a/plugins/catalog-backend-module-github/src/providers/GithubMultiOrgEntityProvider.ts b/plugins/catalog-backend-module-github/src/providers/GithubMultiOrgEntityProvider.ts index 634250d90e..21308a6452 100644 --- a/plugins/catalog-backend-module-github/src/providers/GithubMultiOrgEntityProvider.ts +++ b/plugins/catalog-backend-module-github/src/providers/GithubMultiOrgEntityProvider.ts @@ -38,6 +38,7 @@ import { EntityProviderConnection, } from '@backstage/plugin-catalog-node'; import { EventParams, EventsService } from '@backstage/plugin-events-node'; +import { Octokit } from '@octokit/core'; import { graphql } from '@octokit/graphql'; import { InstallationCreatedEvent, @@ -51,7 +52,7 @@ import { TeamEditedEvent, TeamEvent, } from '@octokit/webhooks-types'; -import { merge } from 'lodash'; +import { memoize, merge } from 'lodash'; import { randomUUID } from 'node:crypto'; import { @@ -74,14 +75,18 @@ import { ANNOTATION_GITHUB_USER_LOGIN, } from '../lib/annotation'; import { + createRestClient, getOrganizationsFromUser, getOrganizationTeam, getOrganizationTeamsForUser, getOrganizationTeamsFromUsers, + isGitHubEnterprise, + isSuspended, } from '../lib/github'; import { splitTeamSlug } from '../lib/util'; import { areGroupEntities, areUserEntities } from '../lib/guards'; import { + CacheService, LoggerService, SchedulerServiceTaskRunner, } from '@backstage/backend-plugin-api'; @@ -182,6 +187,21 @@ export interface GithubMultiOrgEntityProviderOptions { * Only for GitHub Enterprise instances. Will error if used against GitHub.com API. */ excludeSuspendedUsers?: boolean; + + /** + * Optional cache service used for conditional HTTP request caching when + * checking suspended users via the REST API. + */ + cache?: CacheService; + + /** + * When set to true alongside `excludeSuspendedUsers`, use the GitHub REST API + * to check for suspended users instead of the GraphQL `suspendedAt` field. + * REST responses are cached using conditional HTTP requests to minimize rate + * limit usage. + * @defaultValue false + */ + experimental_checkForSuspendedUsersWithRest?: boolean; } type CreateDeltaOperation = (entities: Entity[]) => { @@ -230,6 +250,9 @@ export class GithubMultiOrgEntityProvider implements EntityProvider { alwaysUseDefaultNamespace: options.alwaysUseDefaultNamespace, pageSizes: options.pageSizes, excludeSuspendedUsers: options.excludeSuspendedUsers, + cache: options.cache, + experimental_checkForSuspendedUsersWithRest: + options.experimental_checkForSuspendedUsersWithRest, }); provider.schedule(options.schedule); @@ -251,6 +274,8 @@ export class GithubMultiOrgEntityProvider implements EntityProvider { alwaysUseDefaultNamespace?: boolean; pageSizes?: Partial; excludeSuspendedUsers?: boolean; + cache?: CacheService; + experimental_checkForSuspendedUsersWithRest?: boolean; }, ) {} @@ -266,6 +291,45 @@ export class GithubMultiOrgEntityProvider implements EntityProvider { }; } + private get useRestSuspendedCheck(): boolean { + return ( + !!this.options.excludeSuspendedUsers && + !!this.options.experimental_checkForSuspendedUsersWithRest + ); + } + + private readonly restClients = new Map(); + + private getRestClient(org: string): Octokit { + let client = this.restClients.get(org); + if (!client) { + client = createRestClient({ + baseUrl: this.options.gitHubConfig.apiBaseUrl, + orgUrl: `${this.options.githubUrl}/${org}`, + credentialsProvider: this.options.githubCredentialsProvider, + logger: this.options.logger, + cache: this.options.cache, + }); + this.restClients.set(org, client); + } + return client; + } + + private isGitHubEnterprise = memoize((org: string) => + isGitHubEnterprise(this.getRestClient(org)), + ); + + private async shouldExclude(login: string, org: string): Promise { + if (!this.useRestSuspendedCheck) { + return false; + } + const restClient = this.getRestClient(org); + return ( + (await this.isGitHubEnterprise(org)) && + (await isSuspended(login, restClient, { org })) + ); + } + /** {@inheritdoc @backstage/plugin-catalog-node#EntityProvider.connect} */ async connect(connection: EntityProviderConnection) { this.connection = connection; @@ -317,6 +381,7 @@ export class GithubMultiOrgEntityProvider implements EntityProvider { this.options.userTransformer, pageSizes, this.options.excludeSuspendedUsers, + this.useRestSuspendedCheck ? this.getRestClient(org) : undefined, ); const { teams } = await getOrganizationTeams( @@ -487,6 +552,7 @@ export class GithubMultiOrgEntityProvider implements EntityProvider { this.options.userTransformer, pageSizes, this.options.excludeSuspendedUsers, + this.useRestSuspendedCheck ? this.getRestClient(org) : undefined, ); const { teams } = await getOrganizationTeams( @@ -556,6 +622,14 @@ export class GithubMultiOrgEntityProvider implements EntityProvider { node_id, } = event.membership.user; const org = event.organization.login; + + if ( + event.action === 'member_added' && + (await this.shouldExclude(login, org)) + ) { + return; + } + const { headers } = await this.options.githubCredentialsProvider.getCredentials({ url: `${this.options.githubUrl}/${org}`, @@ -729,6 +803,7 @@ export class GithubMultiOrgEntityProvider implements EntityProvider { this.options.userTransformer, pageSizes, this.options.excludeSuspendedUsers, + this.useRestSuspendedCheck ? this.getRestClient(org) : undefined, ); const usersFromChangedGroup = isGroupEntity(team) @@ -858,43 +933,49 @@ export class GithubMultiOrgEntityProvider implements EntityProvider { }, ); - const mutationEntities = [team]; + const addedEntities: Entity[] = [team]; + const removedEntities: Entity[] = []; if (user && isUserEntity(user)) { - const { orgs } = await getOrganizationsFromUser(client, login); - const userApplicableOrgs = orgs.filter(o => applicableOrgs.includes(o)); - for (const userOrg of userApplicableOrgs) { - const { headers: orgHeaders } = - await this.options.githubCredentialsProvider.getCredentials({ - url: `${this.options.githubUrl}/${userOrg}`, + if (await this.shouldExclude(login, org)) { + removedEntities.push(user); + } else { + const { orgs } = await getOrganizationsFromUser(client, login); + const userApplicableOrgs = orgs.filter(o => applicableOrgs.includes(o)); + for (const userOrg of userApplicableOrgs) { + const { headers: orgHeaders } = + await this.options.githubCredentialsProvider.getCredentials({ + url: `${this.options.githubUrl}/${userOrg}`, + }); + const orgClient = graphql.defaults({ + baseUrl: this.options.gitHubConfig.apiBaseUrl, + headers: orgHeaders, }); - const orgClient = graphql.defaults({ - baseUrl: this.options.gitHubConfig.apiBaseUrl, - headers: orgHeaders, - }); - const { teams } = await getOrganizationTeamsForUser( - orgClient, - userOrg, - login, - this.defaultMultiOrgTeamTransformer.bind(this), - pageSizes, - ); + const { teams } = await getOrganizationTeamsForUser( + orgClient, + userOrg, + login, + this.defaultMultiOrgTeamTransformer.bind(this), + pageSizes, + ); - if (areGroupEntities(teams)) { - assignGroupsToUser(user, teams); + if (areGroupEntities(teams)) { + assignGroupsToUser(user, teams); + } } - } - mutationEntities.push(user); + addedEntities.push(user); + } } - const { added, removed } = - this.createAddEntitiesOperation(mutationEntities); + const materializedAdd = this.createAddEntitiesOperation(addedEntities); + const materializedRemove = + this.createRemoveEntitiesOperation(removedEntities); await this.connection.applyMutation({ type: 'delta', - removed, - added, + removed: [...materializedAdd.removed, ...materializedRemove.removed], + added: [...materializedAdd.added, ...materializedRemove.added], }); } diff --git a/plugins/catalog-backend-module-github/src/providers/GithubOrgEntityProvider.test.ts b/plugins/catalog-backend-module-github/src/providers/GithubOrgEntityProvider.test.ts index 86b0090263..62bbb797b2 100644 --- a/plugins/catalog-backend-module-github/src/providers/GithubOrgEntityProvider.test.ts +++ b/plugins/catalog-backend-module-github/src/providers/GithubOrgEntityProvider.test.ts @@ -26,7 +26,12 @@ import { EventParams, } from '@backstage/plugin-events-node'; import { graphql } from '@octokit/graphql'; -import { createGraphqlClient } from '../lib/github'; +import { + createGraphqlClient, + createRestClient, + isGitHubEnterprise, + isSuspended, +} from '../lib/github'; import { withLocations } from '../lib/withLocations'; import { GithubOrgEntityProvider } from './GithubOrgEntityProvider'; @@ -34,6 +39,9 @@ jest.mock('@octokit/graphql'); jest.mock('../lib/github', () => ({ ...jest.requireActual('../lib/github'), createGraphqlClient: jest.fn(), + createRestClient: jest.fn(), + isGitHubEnterprise: jest.fn(), + isSuspended: jest.fn(), })); describe('GithubOrgEntityProvider', () => { @@ -1310,5 +1318,255 @@ describe('GithubOrgEntityProvider', () => { type: 'delta', }); }); + + it('should skip adding a suspended user on member_added event', async () => { + const entityProviderConnection: EntityProviderConnection = { + applyMutation: jest.fn(), + refresh: jest.fn(), + }; + + const logger = mockServices.logger.mock(); + const events = DefaultEventsService.create({ logger }); + const gitHubConfig: GithubIntegrationConfig = { + host: 'github.com', + }; + + const mockGetCredentials = jest.fn().mockReturnValue({ + headers: { token: 'blah' }, + type: 'app', + token: 'blah', + }); + + const githubCredentialsProvider: GithubCredentialsProvider = { + getCredentials: mockGetCredentials, + }; + + (createRestClient as jest.Mock).mockReturnValue({}); + (isGitHubEnterprise as jest.Mock).mockResolvedValue(true); + (isSuspended as jest.Mock).mockResolvedValue(true); + + const entityProvider = new GithubOrgEntityProvider({ + events, + id: 'my-id', + githubCredentialsProvider, + orgUrl: 'https://github.com/backstage', + gitHubConfig, + logger, + excludeSuspendedUsers: true, + experimental_checkForSuspendedUsersWithRest: true, + cache: mockServices.cache.mock(), + }); + + await entityProvider.connect(entityProviderConnection); + + const event: EventParams = { + topic: 'github.organization', + eventPayload: { + action: 'member_added', + membership: { + user: { + name: 'githubuser', + login: 'githubuser', + node_id: 'githubuserId', + avatar_url: 'https://avatars.githubusercontent.com/u/83820368', + email: 'user1@test.com', + }, + }, + organization: { + login: 'test-org', + }, + }, + }; + await events.publish(event); + + expect(entityProviderConnection.applyMutation).not.toHaveBeenCalled(); + expect(isSuspended).toHaveBeenCalledWith( + 'githubuser', + expect.anything(), + { org: 'test-org' }, + ); + }); + + it('should not skip member_added when experimental flag is off', async () => { + const entityProviderConnection: EntityProviderConnection = { + applyMutation: jest.fn(), + refresh: jest.fn(), + }; + + const logger = mockServices.logger.mock(); + const events = DefaultEventsService.create({ logger }); + const gitHubConfig: GithubIntegrationConfig = { + host: 'github.com', + }; + + const mockGetCredentials = jest.fn().mockReturnValue({ + headers: { token: 'blah' }, + type: 'app', + }); + + const githubCredentialsProvider: GithubCredentialsProvider = { + getCredentials: mockGetCredentials, + }; + + const entityProvider = new GithubOrgEntityProvider({ + events, + id: 'my-id', + githubCredentialsProvider, + orgUrl: 'https://github.com/backstage', + gitHubConfig, + logger, + excludeSuspendedUsers: true, + }); + + await entityProvider.connect(entityProviderConnection); + + const event: EventParams = { + topic: 'github.organization', + eventPayload: { + action: 'member_added', + membership: { + user: { + name: 'githubuser', + login: 'githubuser', + node_id: 'githubuserId', + avatar_url: 'https://avatars.githubusercontent.com/u/83820368', + email: 'user1@test.com', + }, + }, + organization: { + login: 'test-org', + }, + }, + }; + await events.publish(event); + + expect(entityProviderConnection.applyMutation).toHaveBeenCalledTimes(1); + expect(isSuspended).not.toHaveBeenCalled(); + }); + + it.each(['added', 'removed'])( + 'should exclude suspended user on membership %s event', + async action => { + const entityProviderConnection: EntityProviderConnection = { + applyMutation: jest.fn(), + refresh: jest.fn(), + }; + + const logger = mockServices.logger.mock(); + const events = DefaultEventsService.create({ logger }); + const gitHubConfig: GithubIntegrationConfig = { + host: 'github.com', + }; + + const mockGetCredentials = jest.fn().mockReturnValue({ + headers: { token: 'blah' }, + type: 'app', + token: 'blah', + }); + + const githubCredentialsProvider: GithubCredentialsProvider = { + getCredentials: mockGetCredentials, + }; + + (createRestClient as jest.Mock).mockReturnValue({}); + (isGitHubEnterprise as jest.Mock).mockResolvedValue(true); + (isSuspended as jest.Mock).mockResolvedValue(true); + + const entityProvider = new GithubOrgEntityProvider({ + events, + id: 'my-id', + githubCredentialsProvider, + orgUrl: 'https://github.com/backstage', + gitHubConfig, + logger, + excludeSuspendedUsers: true, + experimental_checkForSuspendedUsersWithRest: true, + cache: mockServices.cache.mock(), + }); + + const mockClient = jest.fn(); + + mockClient.mockResolvedValueOnce({ + organization: { + team: { + slug: 'team', + combinedSlug: 'blah/team', + name: 'Team', + description: 'The one and only team', + avatarUrl: 'http://example.com/team.jpeg', + parentTeam: { + slug: 'parent', + combinedSlug: '', + members: { pageInfo: { hasNextPage: false }, nodes: [] }, + }, + members: { + pageInfo: { hasNextPage: false }, + nodes: [{ login: 'a' }, { login: 'githubuser' }], + }, + }, + }, + }); + + (graphql.defaults as jest.Mock).mockReturnValue(mockClient); + await entityProvider.connect(entityProviderConnection); + + const event: EventParams = { + topic: 'github.membership', + eventPayload: { + action, + team: { + name: 'New Team', + slug: 'new-team', + description: 'description from the new team', + html_url: 'https://github.com/orgs/test-org/teams/new-team', + parent: { + slug: 'father-team', + }, + }, + member: { + login: 'githubuser', + avatar_url: 'e', + email: 'd', + name: 'githubuser', + node_id: 'githubuserId', + }, + organization: { + login: 'test-org', + }, + }, + }; + + await events.publish(event); + await new Promise(process.nextTick); + + expect(entityProviderConnection.applyMutation).toHaveBeenCalledTimes(1); + expect(entityProviderConnection.applyMutation).toHaveBeenCalledWith({ + added: [ + { + locationKey: 'github-org-provider:my-id', + entity: expect.objectContaining({ + kind: 'Group', + metadata: expect.objectContaining({ name: 'team' }), + }), + }, + ], + removed: [ + { + locationKey: 'github-org-provider:my-id', + entity: expect.objectContaining({ + kind: 'User', + metadata: expect.objectContaining({ name: 'githubuser' }), + }), + }, + ], + type: 'delta', + }); + expect(isSuspended).toHaveBeenCalledWith( + 'githubuser', + expect.anything(), + { org: 'backstage' }, + ); + }, + ); }); }); diff --git a/plugins/catalog-backend-module-github/src/providers/GithubOrgEntityProvider.ts b/plugins/catalog-backend-module-github/src/providers/GithubOrgEntityProvider.ts index ce9ae13af6..9e62a87a99 100644 --- a/plugins/catalog-backend-module-github/src/providers/GithubOrgEntityProvider.ts +++ b/plugins/catalog-backend-module-github/src/providers/GithubOrgEntityProvider.ts @@ -15,6 +15,7 @@ */ import { + CacheService, LoggerService, SchedulerServiceTaskRunner, } from '@backstage/backend-plugin-api'; @@ -32,6 +33,7 @@ import { EntityProviderConnection, } from '@backstage/plugin-catalog-node'; import { EventParams, EventsService } from '@backstage/plugin-events-node'; +import { Octokit } from '@octokit/core'; import { graphql } from '@octokit/graphql'; import { MembershipEvent, @@ -52,6 +54,7 @@ import { createAddEntitiesOperation, createGraphqlClient, createRemoveEntitiesOperation, + createRestClient, DEFAULT_PAGE_SIZES, DeferredEntitiesBuilder, getOrganizationTeam, @@ -61,6 +64,8 @@ import { getOrganizationUsers, GithubPageSizes, GithubTeam, + isGitHubEnterprise, + isSuspended, } from '../lib/github'; import { areGroupEntities, areUserEntities } from '../lib/guards'; import { @@ -70,6 +75,7 @@ import { } from '../lib/org'; import { parseGithubOrgUrl } from '../lib/util'; import { withLocations } from '../lib/withLocations'; +import { memoize } from 'lodash'; const EVENT_TOPICS = [ 'github.membership', @@ -150,6 +156,21 @@ export interface GithubOrgEntityProviderOptions { * Only for GitHub Enterprise instances. Will error if used against GitHub.com API. */ excludeSuspendedUsers?: boolean; + + /** + * Optional cache service used for conditional HTTP request caching when + * checking suspended users via the REST API. + */ + cache?: CacheService; + + /** + * When set to true alongside `excludeSuspendedUsers`, use the GitHub REST API + * to check for suspended users instead of the GraphQL `suspendedAt` field. + * REST responses are cached using conditional HTTP requests to minimize rate + * limit usage. + * @defaultValue false + */ + experimental_checkForSuspendedUsersWithRest?: boolean; } /** @@ -159,6 +180,7 @@ export interface GithubOrgEntityProviderOptions { */ export class GithubOrgEntityProvider implements EntityProvider { private readonly credentialsProvider: GithubCredentialsProvider; + private cachedRestClient?: Octokit; private connection?: EntityProviderConnection; private scheduleFn?: () => Promise; @@ -189,6 +211,9 @@ export class GithubOrgEntityProvider implements EntityProvider { events: options.events, pageSizes: options.pageSizes, excludeSuspendedUsers: options.excludeSuspendedUsers, + cache: options.cache, + experimental_checkForSuspendedUsersWithRest: + options.experimental_checkForSuspendedUsersWithRest, }); provider.schedule(options.schedule); @@ -208,6 +233,8 @@ export class GithubOrgEntityProvider implements EntityProvider { teamTransformer?: TeamTransformer; pageSizes?: Partial; excludeSuspendedUsers?: boolean; + cache?: CacheService; + experimental_checkForSuspendedUsersWithRest?: boolean; }, ) { this.credentialsProvider = @@ -227,6 +254,41 @@ export class GithubOrgEntityProvider implements EntityProvider { }; } + private get useRestSuspendedCheck(): boolean { + return ( + !!this.options.excludeSuspendedUsers && + !!this.options.experimental_checkForSuspendedUsersWithRest + ); + } + + private isGitHubEnterprise = memoize(() => + isGitHubEnterprise(this.getRestClient()), + ); + + private getRestClient(): Octokit { + if (!this.cachedRestClient) { + this.cachedRestClient = createRestClient({ + baseUrl: this.options.gitHubConfig.apiBaseUrl, + orgUrl: this.options.orgUrl, + credentialsProvider: this.credentialsProvider, + logger: this.options.logger, + cache: this.options.cache, + }); + } + return this.cachedRestClient; + } + + private async shouldExclude(login: string, org: string): Promise { + if (!this.useRestSuspendedCheck) { + return false; + } + const restClient = this.getRestClient(); + return ( + (await this.isGitHubEnterprise()) && + (await isSuspended(login, restClient, { org })) + ); + } + /** {@inheritdoc @backstage/plugin-catalog-node#EntityProvider.connect} */ async connect(connection: EntityProviderConnection) { this.connection = connection; @@ -263,6 +325,7 @@ export class GithubOrgEntityProvider implements EntityProvider { const { org } = parseGithubOrgUrl(this.options.orgUrl); const pageSizes = this.getPageSizes(); + const { users } = await getOrganizationUsers( client, org, @@ -270,6 +333,7 @@ export class GithubOrgEntityProvider implements EntityProvider { this.options.userTransformer, pageSizes, this.options.excludeSuspendedUsers, + this.useRestSuspendedCheck ? this.getRestClient() : undefined, ); const { teams } = await getOrganizationTeams( client, @@ -358,6 +422,7 @@ export class GithubOrgEntityProvider implements EntityProvider { await this.onMembershipChangedInOrganization( membershipEvent, addEntitiesOperation, + removeEntitiesOperation, ); } @@ -399,6 +464,7 @@ export class GithubOrgEntityProvider implements EntityProvider { this.options.userTransformer, pageSizes, this.options.excludeSuspendedUsers, + this.useRestSuspendedCheck ? this.getRestClient() : undefined, ); if (!isGroupEntity(team)) { @@ -461,7 +527,8 @@ export class GithubOrgEntityProvider implements EntityProvider { private async onMembershipChangedInOrganization( event: MembershipEvent, - createDeltaOperation: DeferredEntitiesBuilder, + addEntitiesOperation: DeferredEntitiesBuilder, + removeEntitiesOperation: DeferredEntitiesBuilder, ) { if (!this.connection) { throw new Error('Not initialized'); @@ -512,31 +579,47 @@ export class GithubOrgEntityProvider implements EntityProvider { }, ); - const mutationEntities: Entity[] = [team]; + const addedEntities: Entity[] = [team]; + const removedEntities: Entity[] = []; if (user && isUserEntity(user)) { - const teamTransformer = - this.options.teamTransformer || defaultOrganizationTeamTransformer; - const { teams } = await getOrganizationTeamsForUser( - client, - org, - login, - teamTransformer, - pageSizes, - ); + if (await this.shouldExclude(login, org)) { + removedEntities.push(user); + } else { + const teamTransformer = + this.options.teamTransformer || defaultOrganizationTeamTransformer; + const { teams } = await getOrganizationTeamsForUser( + client, + org, + login, + teamTransformer, + pageSizes, + ); - if (areGroupEntities(teams)) { - assignGroupsToUser(user, teams); + if (areGroupEntities(teams)) { + assignGroupsToUser(user, teams); + } + + addedEntities.push(user); } - - mutationEntities.push(user); } - const { added, removed } = createDeltaOperation(org, mutationEntities); + const materializedAddOperation = addEntitiesOperation(org, addedEntities); + const materializedRemoveOperation = removeEntitiesOperation( + org, + removedEntities, + ); + await this.connection.applyMutation({ type: 'delta', - removed, - added, + removed: [ + ...materializedAddOperation.removed, + ...materializedRemoveOperation.removed, + ], + added: [ + ...materializedAddOperation.added, + ...materializedRemoveOperation.added, + ], }); } @@ -607,6 +690,14 @@ export class GithubOrgEntityProvider implements EntityProvider { node_id, } = event.membership.user; const org = event.organization.login; + + if ( + event.action === 'member_added' && + (await this.shouldExclude(login, org)) + ) { + return; + } + const { headers } = await this.credentialsProvider.getCredentials({ url: this.options.orgUrl, });