From 9f9b5a983410c0fcc72b78485f128762e7de8eb3 Mon Sep 17 00:00:00 2001 From: Patrick Date: Sun, 20 Oct 2024 21:36:34 -0400 Subject: [PATCH] fix(rbac): clean up some options for policy server --- .../src/service/policies-rest-api.test.ts | 15 +++----- .../src/service/policies-rest-api.ts | 34 +++++++---------- .../src/service/policy-builder.ts | 38 +++++++++++-------- 3 files changed, 42 insertions(+), 45 deletions(-) diff --git a/plugins/rbac-backend/src/service/policies-rest-api.test.ts b/plugins/rbac-backend/src/service/policies-rest-api.test.ts index 5899c0172fc..e41c73340ef 100644 --- a/plugins/rbac-backend/src/service/policies-rest-api.test.ts +++ b/plugins/rbac-backend/src/service/policies-rest-api.test.ts @@ -1,7 +1,6 @@ import { MiddlewareFactory } from '@backstage/backend-defaults/rootHttpRouter'; import { mockCredentials, mockServices } from '@backstage/backend-test-utils'; import { InputError } from '@backstage/errors'; -import type { RouterOptions } from '@backstage/plugin-permission-backend'; import { AuthorizeResult } from '@backstage/plugin-permission-common'; import type { MetadataResponse } from '@backstage/plugin-permission-node'; @@ -35,6 +34,7 @@ import { PluginPermissionMetadataCollector, } from './plugin-endpoints'; import { PoliciesServer } from './policies-rest-api'; +import { RBACRouterOptions } from './policy-builder'; jest.setTimeout(60000); @@ -126,6 +126,7 @@ const mockHttpAuth = mockServices.httpAuth({ }); const mockAuth = mockServices.auth(); const credentials = mockCredentials.user('user:default/guest'); +const mockUserInfo = mockServices.userInfo(); const conditions: RoleConditionalPolicyDecision[] = [ { @@ -267,7 +268,7 @@ describe('REST policies api', () => { mockHttpAuth.credentials = jest.fn().mockImplementation(() => credentials); - const options: RouterOptions = { + const options: RBACRouterOptions = { config: config, logger, discovery: mockDiscovery, @@ -284,15 +285,13 @@ describe('REST policies api', () => { pluginPermissionMetadataCollectorMock as PluginPermissionMetadataCollector, mockAuth, ), + userInfo: mockUserInfo, }; server = new PoliciesServer( mockPermissionEvaluator, options, mockEnforcer as EnforcerDelegate, - config, - mockHttpAuth, - mockAuth, conditionalStorage, pluginPermissionMetadataCollectorMock as PluginPermissionMetadataCollector, roleMetadataStorageMock, @@ -3476,7 +3475,7 @@ describe('REST policies api', () => { { result: AuthorizeResult.ALLOW }, ]); - const options: RouterOptions = { + const options: RBACRouterOptions = { config: config, logger, discovery: mockDiscovery, @@ -3493,15 +3492,13 @@ describe('REST policies api', () => { pluginPermissionMetadataCollectorMock as PluginPermissionMetadataCollector, mockAuth, ), + userInfo: mockUserInfo, }; server = new PoliciesServer( mockPermissionEvaluator, options, mockEnforcer as EnforcerDelegate, - config, - mockHttpAuth, - mockAuth, conditionalStorage, pluginPermissionMetadataCollectorMock as PluginPermissionMetadataCollector, roleMetadataStorageMock, diff --git a/plugins/rbac-backend/src/service/policies-rest-api.ts b/plugins/rbac-backend/src/service/policies-rest-api.ts index 98a6ffe6a5a..73dcde49f9c 100644 --- a/plugins/rbac-backend/src/service/policies-rest-api.ts +++ b/plugins/rbac-backend/src/service/policies-rest-api.ts @@ -1,9 +1,4 @@ -import type { - AuthService, - HttpAuthService, - PermissionsService, -} from '@backstage/backend-plugin-api'; -import type { Config } from '@backstage/config'; +import type { PermissionsService } from '@backstage/backend-plugin-api'; import { ConflictError, InputError, @@ -11,10 +6,7 @@ import { NotFoundError, ServiceUnavailableError, } from '@backstage/errors'; -import { - createRouter, - RouterOptions, -} from '@backstage/plugin-permission-backend'; +import { createRouter } from '@backstage/plugin-permission-backend'; import { AuthorizeResult, PolicyDecision, @@ -76,15 +68,13 @@ import { } from '../validation/policies-validation'; import { EnforcerDelegate } from './enforcer-delegate'; import { PluginPermissionMetadataCollector } from './plugin-endpoints'; +import { RBACRouterOptions } from './policy-builder'; export class PoliciesServer { constructor( private readonly permissions: PermissionsService, - private readonly options: RouterOptions, + private readonly options: RBACRouterOptions, private readonly enforcer: EnforcerDelegate, - private readonly config: Config, - private readonly httpAuth: HttpAuthService, - private readonly auth: AuthService, private readonly conditionalStorage: ConditionalStorage, private readonly pluginPermMetaData: PluginPermissionMetadataCollector, private readonly roleMetadata: RoleMetadataStorage, @@ -96,13 +86,13 @@ export class PoliciesServer { request: Request, permission: ResourcePermission, ): Promise { - const credentials = await this.httpAuth.credentials(request, { + const credentials = await this.options.httpAuth.credentials(request, { allow: ['user', 'service'], }); // allow service to service communication, but only with read permission if ( - this.auth.isPrincipal(credentials, 'service') && + this.options.auth.isPrincipal(credentials, 'service') && permission !== policyEntityReadPermission ) { throw new NotAllowedError( @@ -138,7 +128,7 @@ export class PoliciesServer { router.use(permissionsIntegrationRouter); const isPluginEnabled = - this.config.getOptionalBoolean('permission.enabled'); + this.options.config.getOptionalBoolean('permission.enabled'); if (!isPluginEnabled) { return router; } @@ -767,7 +757,9 @@ export class PoliciesServer { throw new NotAllowedError(); // 403 } - const body = await this.pluginPermMetaData.getPluginPolicies(this.auth); + const body = await this.pluginPermMetaData.getPluginPolicies( + this.options.auth, + ); await this.aLog.auditLog({ message: `Return list plugin policies`, @@ -792,7 +784,7 @@ export class PoliciesServer { } const body = await this.pluginPermMetaData.getPluginConditionRules( - this.auth, + this.options.auth, ); await this.aLog.auditLog({ @@ -861,7 +853,7 @@ export class PoliciesServer { const conditionToCreate = await processConditionMapping( roleConditionPolicy, this.pluginPermMetaData, - this.auth, + this.options.auth, ); const id = @@ -982,7 +974,7 @@ export class PoliciesServer { const conditionToUpdate = await processConditionMapping( roleConditionPolicy, this.pluginPermMetaData, - this.auth, + this.options.auth, ); await this.conditionalStorage.updateCondition(id, conditionToUpdate); diff --git a/plugins/rbac-backend/src/service/policy-builder.ts b/plugins/rbac-backend/src/service/policy-builder.ts index f41b711eb0f..7297a37f8f0 100644 --- a/plugins/rbac-backend/src/service/policy-builder.ts +++ b/plugins/rbac-backend/src/service/policy-builder.ts @@ -9,7 +9,6 @@ import type { } from '@backstage/backend-plugin-api'; import { CatalogClient } from '@backstage/catalog-client'; import type { Config } from '@backstage/config'; -import type { RouterOptions } from '@backstage/plugin-permission-backend'; import type { PermissionEvaluator } from '@backstage/plugin-permission-common'; import { PermissionPolicy } from '@backstage/plugin-permission-node'; @@ -35,18 +34,30 @@ import { MODEL } from './permission-model'; import { PluginPermissionMetadataCollector } from './plugin-endpoints'; import { PoliciesServer } from './policies-rest-api'; +export type EnvOptions = { + config: Config; + logger: LoggerService; + discovery: DiscoveryService; + permissions: PermissionEvaluator; + auth: AuthService; + httpAuth: HttpAuthService; + userInfo: UserInfoService; + lifecycle: LifecycleService; +}; + +export type RBACRouterOptions = { + config: Config; + logger: LoggerService; + discovery: DiscoveryService; + policy: PermissionPolicy; + auth: AuthService; + httpAuth: HttpAuthService; + userInfo: UserInfoService; +}; + export class PolicyBuilder { public static async build( - env: { - config: Config; - logger: LoggerService; - discovery: DiscoveryService; - permissions: PermissionEvaluator; - auth: AuthService; - httpAuth: HttpAuthService; - userInfo: UserInfoService; - lifecycle: LifecycleService; - }, + env: EnvOptions, pluginIdProvider: PluginIdProvider = { getPluginIds: () => [] }, rbacProviders?: Array, ): Promise { @@ -157,7 +168,7 @@ export class PolicyBuilder { policy = new AllowAllPolicy(); } - const options: RouterOptions = { + const options: RBACRouterOptions = { config: env.config, logger: env.logger, discovery: env.discovery, @@ -171,9 +182,6 @@ export class PolicyBuilder { env.permissions, options, enforcerDelegate, - env.config, - env.httpAuth, - env.auth, conditionStorage, pluginPermMetaData, roleMetadataStorage,