From 776ec144edf0f3a9a0ad6c7d1381263cac1b6365 Mon Sep 17 00:00:00 2001 From: Heikki Hellgren Date: Tue, 2 Apr 2024 09:12:29 +0300 Subject: [PATCH] fix: review comments and naming Signed-off-by: Heikki Hellgren --- .changeset/selfish-pigs-glow.md | 2 +- plugins/auth-backend/api-report.md | 9 +-- plugins/auth-backend/src/authPlugin.ts | 55 +++++++++---------- plugins/auth-backend/src/index.ts | 2 +- .../resolvers/CatalogAuthResolverContext.ts | 37 +++++++++++-- .../resolvers/DefaultAuthOwnershipResolver.ts | 43 --------------- .../auth-backend/src/lib/resolvers/index.ts | 6 +- plugins/auth-backend/src/providers/router.ts | 3 +- plugins/auth-node/api-report.md | 15 ++++- .../AuthOwnershipResolutionExtensionPoint.ts | 28 ++++++++++ .../extensions/AuthProvidersExtensionPoint.ts | 3 +- plugins/auth-node/src/extensions/index.ts | 5 ++ plugins/auth-node/src/types.ts | 4 +- 13 files changed, 115 insertions(+), 97 deletions(-) delete mode 100644 plugins/auth-backend/src/lib/resolvers/DefaultAuthOwnershipResolver.ts create mode 100644 plugins/auth-node/src/extensions/AuthOwnershipResolutionExtensionPoint.ts diff --git a/.changeset/selfish-pigs-glow.md b/.changeset/selfish-pigs-glow.md index ca29a22feb..c7808c0283 100644 --- a/.changeset/selfish-pigs-glow.md +++ b/.changeset/selfish-pigs-glow.md @@ -3,4 +3,4 @@ '@backstage/plugin-auth-node': patch --- -Allow overriding default ownership resolving +Allow overriding default ownership resolving with the new `AuthOwnershipResolutionExtensionPoint` diff --git a/plugins/auth-backend/api-report.md b/plugins/auth-backend/api-report.md index 0f537d3dc2..15a2aa690e 100644 --- a/plugins/auth-backend/api-report.md +++ b/plugins/auth-backend/api-report.md @@ -194,12 +194,6 @@ export function createOriginFilter(config: Config): (origin: string) => boolean; // @public (undocumented) export function createRouter(options: RouterOptions): Promise; -// @public -export class DefaultAuthOwnershipResolver implements AuthOwnershipResolver { - // (undocumented) - getOwnershipEntityRefs(entity: Entity): Promise; -} - // @public export const defaultAuthProviderFactories: { [providerId: string]: AuthProviderFactory_2; @@ -223,6 +217,9 @@ export type GcpIapResult = GcpIapResult_2; // @public @deprecated export type GcpIapTokenInfo = GcpIapTokenInfo_2; +// @public +export function getDefaultOwnershipEntityRefs(entity: Entity): string[]; + // @public (undocumented) export type GithubOAuthResult = { fullProfile: Profile; diff --git a/plugins/auth-backend/src/authPlugin.ts b/plugins/auth-backend/src/authPlugin.ts index f8e1f011b3..7687462d32 100644 --- a/plugins/auth-backend/src/authPlugin.ts +++ b/plugins/auth-backend/src/authPlugin.ts @@ -19,39 +19,14 @@ import { createBackendPlugin, } from '@backstage/backend-plugin-api'; import { + authOwnershipResolutionExtensionPoint, AuthOwnershipResolver, AuthProviderFactory, - AuthProviderRegistrationOptions, - AuthProvidersExtensionPoint, authProvidersExtensionPoint, } from '@backstage/plugin-auth-node'; import { catalogServiceRef } from '@backstage/plugin-catalog-node/alpha'; import { createRouter } from './service/router'; -class AuthProvidersExtensionPointImpl implements AuthProvidersExtensionPoint { - #providers = new Map(); - #ownershipResolver?: AuthOwnershipResolver; - registerProvider(options: AuthProviderRegistrationOptions): void { - const { providerId, factory } = options; - if (this.#providers.has(providerId)) { - throw new Error(`Auth provider '${providerId}' was already registered`); - } - this.#providers.set(providerId, factory); - } - - setAuthOwnershipResolver(ownershipResolver: AuthOwnershipResolver): void { - this.#ownershipResolver = ownershipResolver; - } - - get providers() { - return this.#providers; - } - - get ownershipResolver() { - return this.#ownershipResolver; - } -} - /** * Auth plugin * @@ -60,8 +35,28 @@ class AuthProvidersExtensionPointImpl implements AuthProvidersExtensionPoint { export const authPlugin = createBackendPlugin({ pluginId: 'auth', register(reg) { - const extensionPoint = new AuthProvidersExtensionPointImpl(); - reg.registerExtensionPoint(authProvidersExtensionPoint, extensionPoint); + const providers = new Map(); + let ownershipResolver: AuthOwnershipResolver | undefined = undefined; + + reg.registerExtensionPoint(authProvidersExtensionPoint, { + registerProvider({ providerId, factory }) { + if (providers.has(providerId)) { + throw new Error( + `Auth provider '${providerId}' was already registered`, + ); + } + providers.set(providerId, factory); + }, + }); + + reg.registerExtensionPoint(authOwnershipResolutionExtensionPoint, { + setAuthOwnershipResolver(resolver) { + if (ownershipResolver) { + throw new Error('Auth ownership resolver is already set'); + } + ownershipResolver = resolver; + }, + }); reg.registerInit({ deps: { @@ -95,9 +90,9 @@ export const authPlugin = createBackendPlugin({ auth, httpAuth, catalogApi, - providerFactories: Object.fromEntries(extensionPoint.providers), + providerFactories: Object.fromEntries(providers), disableDefaultProviderFactories: true, - ownershipResolver: extensionPoint.ownershipResolver, + ownershipResolver, }); httpRouter.addAuthPolicy({ path: '/', diff --git a/plugins/auth-backend/src/index.ts b/plugins/auth-backend/src/index.ts index d3688934c5..6c3f866031 100644 --- a/plugins/auth-backend/src/index.ts +++ b/plugins/auth-backend/src/index.ts @@ -34,4 +34,4 @@ export * from './lib/oauth'; export * from './lib/catalog'; -export { DefaultAuthOwnershipResolver } from './lib/resolvers'; +export { getDefaultOwnershipEntityRefs } from './lib/resolvers'; diff --git a/plugins/auth-backend/src/lib/resolvers/CatalogAuthResolverContext.ts b/plugins/auth-backend/src/lib/resolvers/CatalogAuthResolverContext.ts index be0cd0da97..c81ac4c355 100644 --- a/plugins/auth-backend/src/lib/resolvers/CatalogAuthResolverContext.ts +++ b/plugins/auth-backend/src/lib/resolvers/CatalogAuthResolverContext.ts @@ -20,6 +20,7 @@ import { DEFAULT_NAMESPACE, Entity, parseEntityRef, + RELATION_MEMBER_OF, stringifyEntityRef, } from '@backstage/catalog-model'; import { ConflictError, InputError, NotFoundError } from '@backstage/errors'; @@ -38,6 +39,25 @@ import { } from '@backstage/plugin-auth-node'; import { CatalogIdentityClient } from '../catalog'; +/** + * Uses the default ownership resolution logic to return an array + * of entity refs that the provided entity claims ownership through. + * + * A reference to the entity itself will also be included in the returned array. + * + * @public + */ +export function getDefaultOwnershipEntityRefs(entity: Entity) { + const membershipRefs = + entity.relations + ?.filter( + r => r.type === RELATION_MEMBER_OF && r.targetRef.startsWith('group:'), + ) + .map(r => r.targetRef) ?? []; + + return Array.from(new Set([stringifyEntityRef(entity), ...membershipRefs])); +} + /** * @internal */ @@ -50,7 +70,7 @@ export class CatalogAuthResolverContext implements AuthResolverContext { discovery: DiscoveryService; auth: AuthService; httpAuth: HttpAuthService; - ownershipResolver: AuthOwnershipResolver; + ownershipResolver?: AuthOwnershipResolver; }): CatalogAuthResolverContext { const catalogIdentityClient = new CatalogIdentityClient({ catalogApi: options.catalogApi, @@ -76,7 +96,7 @@ export class CatalogAuthResolverContext implements AuthResolverContext { public readonly catalogIdentityClient: CatalogIdentityClient, private readonly catalogApi: CatalogApi, private readonly auth: AuthService, - private readonly ownershipResolver: AuthOwnershipResolver, + private readonly ownershipResolver?: AuthOwnershipResolver, ) {} async issueToken(params: TokenParams) { @@ -144,14 +164,19 @@ export class CatalogAuthResolverContext implements AuthResolverContext { async signInWithCatalogUser(query: AuthResolverCatalogUserQuery) { const { entity } = await this.findCatalogUser(query); - const ownershipRefs = await this.ownershipResolver.getOwnershipEntityRefs( - entity, - ); + let ent: string[]; + if (this.ownershipResolver) { + const { ownershipEntityRefs } = + await this.ownershipResolver.resolveOwnershipEntityRefs(entity); + ent = ownershipEntityRefs; + } else { + ent = getDefaultOwnershipEntityRefs(entity); + } const token = await this.tokenIssuer.issueToken({ claims: { sub: stringifyEntityRef(entity), - ent: ownershipRefs, + ent, }, }); return { token }; diff --git a/plugins/auth-backend/src/lib/resolvers/DefaultAuthOwnershipResolver.ts b/plugins/auth-backend/src/lib/resolvers/DefaultAuthOwnershipResolver.ts deleted file mode 100644 index 3b6bab861f..0000000000 --- a/plugins/auth-backend/src/lib/resolvers/DefaultAuthOwnershipResolver.ts +++ /dev/null @@ -1,43 +0,0 @@ -/* - * Copyright 2024 The Backstage Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -import { AuthOwnershipResolver } from '@backstage/plugin-auth-node'; -import { - Entity, - RELATION_MEMBER_OF, - stringifyEntityRef, -} from '@backstage/catalog-model'; - -/** - * Uses the default ownership resolution logic to return an array - * of entity refs that the provided entity claims ownership through. - * - * A reference to the entity itself will also be included in the returned array. - * - * @public - */ -export class DefaultAuthOwnershipResolver implements AuthOwnershipResolver { - async getOwnershipEntityRefs(entity: Entity): Promise { - const membershipRefs = - entity.relations - ?.filter( - r => - r.type === RELATION_MEMBER_OF && r.targetRef.startsWith('group:'), - ) - .map(r => r.targetRef) ?? []; - - return Array.from(new Set([stringifyEntityRef(entity), ...membershipRefs])); - } -} diff --git a/plugins/auth-backend/src/lib/resolvers/index.ts b/plugins/auth-backend/src/lib/resolvers/index.ts index a9c132b386..c1ca59cb25 100644 --- a/plugins/auth-backend/src/lib/resolvers/index.ts +++ b/plugins/auth-backend/src/lib/resolvers/index.ts @@ -14,5 +14,7 @@ * limitations under the License. */ -export { CatalogAuthResolverContext } from './CatalogAuthResolverContext'; -export { DefaultAuthOwnershipResolver } from './DefaultAuthOwnershipResolver'; +export { + CatalogAuthResolverContext, + getDefaultOwnershipEntityRefs, +} from './CatalogAuthResolverContext'; diff --git a/plugins/auth-backend/src/providers/router.ts b/plugins/auth-backend/src/providers/router.ts index d2000288a7..d84133685c 100644 --- a/plugins/auth-backend/src/providers/router.ts +++ b/plugins/auth-backend/src/providers/router.ts @@ -35,7 +35,6 @@ import Router from 'express-promise-router'; import { Minimatch } from 'minimatch'; import { CatalogAuthResolverContext } from '../lib/resolvers/CatalogAuthResolverContext'; import { TokenIssuer } from '../identity/types'; -import { DefaultAuthOwnershipResolver } from '../lib/resolvers'; /** @public */ export type ProviderFactories = { [s: string]: AuthProviderFactory }; @@ -69,7 +68,7 @@ export function bindProviderRouters( tokenManager, tokenIssuer, catalogApi, - ownershipResolver = new DefaultAuthOwnershipResolver(), + ownershipResolver, } = options; const providersConfig = config.getOptionalConfig('auth.providers'); diff --git a/plugins/auth-node/api-report.md b/plugins/auth-node/api-report.md index 6a2e653d93..6a43d3b127 100644 --- a/plugins/auth-node/api-report.md +++ b/plugins/auth-node/api-report.md @@ -21,10 +21,21 @@ import { Strategy } from 'passport'; import { ZodSchema } from 'zod'; import { ZodTypeDef } from 'zod'; +// @public (undocumented) +export interface AuthOwnershipResolutionExtensionPoint { + // (undocumented) + setAuthOwnershipResolver(ownershipResolver: AuthOwnershipResolver): void; +} + +// @public (undocumented) +export const authOwnershipResolutionExtensionPoint: ExtensionPoint; + // @public export interface AuthOwnershipResolver { // (undocumented) - getOwnershipEntityRefs(entity: Entity): Promise; + resolveOwnershipEntityRefs(entity: Entity): Promise<{ + ownershipEntityRefs: string[]; + }>; } // @public @deprecated (undocumented) @@ -68,8 +79,6 @@ export interface AuthProviderRouteHandlers { export interface AuthProvidersExtensionPoint { // (undocumented) registerProvider(options: AuthProviderRegistrationOptions): void; - // (undocumented) - setAuthOwnershipResolver(ownershipResolver: AuthOwnershipResolver): void; } // @public (undocumented) diff --git a/plugins/auth-node/src/extensions/AuthOwnershipResolutionExtensionPoint.ts b/plugins/auth-node/src/extensions/AuthOwnershipResolutionExtensionPoint.ts new file mode 100644 index 0000000000..d6235a75e2 --- /dev/null +++ b/plugins/auth-node/src/extensions/AuthOwnershipResolutionExtensionPoint.ts @@ -0,0 +1,28 @@ +/* + * Copyright 2023 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +import { AuthOwnershipResolver } from '../types'; +import { createExtensionPoint } from '@backstage/backend-plugin-api'; + +/** @public */ +export interface AuthOwnershipResolutionExtensionPoint { + setAuthOwnershipResolver(ownershipResolver: AuthOwnershipResolver): void; +} + +/** @public */ +export const authOwnershipResolutionExtensionPoint = + createExtensionPoint({ + id: 'auth.ownershipResolution', + }); diff --git a/plugins/auth-node/src/extensions/AuthProvidersExtensionPoint.ts b/plugins/auth-node/src/extensions/AuthProvidersExtensionPoint.ts index e3396db4e0..b67d5be017 100644 --- a/plugins/auth-node/src/extensions/AuthProvidersExtensionPoint.ts +++ b/plugins/auth-node/src/extensions/AuthProvidersExtensionPoint.ts @@ -15,7 +15,7 @@ */ import { createExtensionPoint } from '@backstage/backend-plugin-api'; -import { AuthOwnershipResolver, AuthProviderFactory } from '../types'; +import { AuthProviderFactory } from '../types'; /** @public */ export interface AuthProviderRegistrationOptions { @@ -26,7 +26,6 @@ export interface AuthProviderRegistrationOptions { /** @public */ export interface AuthProvidersExtensionPoint { registerProvider(options: AuthProviderRegistrationOptions): void; - setAuthOwnershipResolver(ownershipResolver: AuthOwnershipResolver): void; } /** @public */ diff --git a/plugins/auth-node/src/extensions/index.ts b/plugins/auth-node/src/extensions/index.ts index 43e1751685..efca1623b3 100644 --- a/plugins/auth-node/src/extensions/index.ts +++ b/plugins/auth-node/src/extensions/index.ts @@ -19,3 +19,8 @@ export { type AuthProviderRegistrationOptions, type AuthProvidersExtensionPoint, } from './AuthProvidersExtensionPoint'; + +export { + authOwnershipResolutionExtensionPoint, + type AuthOwnershipResolutionExtensionPoint, +} from './AuthOwnershipResolutionExtensionPoint'; diff --git a/plugins/auth-node/src/types.ts b/plugins/auth-node/src/types.ts index 4dc5e570de..845fd7b0cb 100644 --- a/plugins/auth-node/src/types.ts +++ b/plugins/auth-node/src/types.ts @@ -169,7 +169,9 @@ export type AuthResolverContext = { * @public */ export interface AuthOwnershipResolver { - getOwnershipEntityRefs(entity: Entity): Promise; + resolveOwnershipEntityRefs( + entity: Entity, + ): Promise<{ ownershipEntityRefs: string[] }>; } /**