From e7851efa9efd7f5b054d9dd6b7e6e853c8b382ec Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Tue, 30 Nov 2021 14:48:56 +0000 Subject: [PATCH] permission-node: rename and adjust policy return type to reduce nesting Signed-off-by: MT Lewis --- .changeset/hungry-impalas-wave.md | 6 +++++ .../src/service/router.test.ts | 16 +++++-------- .../permission-backend/src/service/router.ts | 11 ++++----- plugins/permission-node/api-report.md | 24 ++++++++----------- .../createConditionExports.test.ts | 8 +++++-- .../src/integration/createConditionExports.ts | 13 +++++----- plugins/permission-node/src/policy/index.ts | 4 ++-- plugins/permission-node/src/policy/types.ts | 16 ++++++------- 8 files changed, 49 insertions(+), 49 deletions(-) create mode 100644 .changeset/hungry-impalas-wave.md diff --git a/.changeset/hungry-impalas-wave.md b/.changeset/hungry-impalas-wave.md new file mode 100644 index 0000000000..376b9fc341 --- /dev/null +++ b/.changeset/hungry-impalas-wave.md @@ -0,0 +1,6 @@ +--- +'@backstage/plugin-permission-node': minor +'@backstage/plugin-permission-backend': patch +--- + +Rename and adjust permission policy return type to reduce nesting diff --git a/plugins/permission-backend/src/service/router.test.ts b/plugins/permission-backend/src/service/router.test.ts index 78e7121da6..26fa7ef8a6 100644 --- a/plugins/permission-backend/src/service/router.test.ts +++ b/plugins/permission-backend/src/service/router.test.ts @@ -161,12 +161,10 @@ describe('createRouter', () => { beforeEach(() => { policy.handle.mockReturnValueOnce({ result: AuthorizeResult.CONDITIONAL, + pluginId: 'test-plugin', + resourceType: 'test-resource-1', conditions: { - pluginId: 'test-plugin', - resourceType: 'test-resource-1', - conditions: { - anyOf: [{ rule: 'test-rule', params: ['abc'] }], - }, + anyOf: [{ rule: 'test-rule', params: ['abc'] }], }, }); }); @@ -265,11 +263,9 @@ describe('createRouter', () => { it('returns a 500 error if the policy returns a different resourceType', async () => { policy.handle.mockReturnValueOnce({ result: AuthorizeResult.CONDITIONAL, - conditions: { - pluginId: 'test-plugin', - resourceType: 'test-resource-2', - conditions: {}, - }, + pluginId: 'test-plugin', + resourceType: 'test-resource-2', + conditions: {}, }); const response = await request(app) diff --git a/plugins/permission-backend/src/service/router.ts b/plugins/permission-backend/src/service/router.ts index 8caba23b50..b85e7feb85 100644 --- a/plugins/permission-backend/src/service/router.ts +++ b/plugins/permission-backend/src/service/router.ts @@ -80,7 +80,7 @@ const handleRequest = async ( if (response.result === AuthorizeResult.CONDITIONAL) { // Sanity check that any resource provided matches the one expected by the permission - if (request.permission.resourceType !== response.conditions.resourceType) { + if (request.permission.resourceType !== response.resourceType) { throw new Error( `Invalid resource conditions returned from permission policy for permission ${request.permission.name}`, ); @@ -92,7 +92,9 @@ const handleRequest = async ( ...(await permissionIntegrationClient.applyConditions( { resourceRef, - ...response.conditions, + pluginId: response.pluginId, + resourceType: response.resourceType, + conditions: response.conditions, }, authHeader, )), @@ -102,10 +104,7 @@ const handleRequest = async ( return { id, result: AuthorizeResult.CONDITIONAL, - // TODO(mtlewis): this .conditions.conditions situation is a bit awkward. I think it's - // worth exploring a bit of reorganization of the ConditionalPolicyResult type so that - // the naming of property chains like this makes a bit more sense. - conditions: response.conditions.conditions, + conditions: response.conditions, }; } diff --git a/plugins/permission-node/api-report.md b/plugins/permission-node/api-report.md index e5fe9a7087..75c45a9250 100644 --- a/plugins/permission-node/api-report.md +++ b/plugins/permission-node/api-report.md @@ -32,13 +32,11 @@ export type Condition = TRule extends PermissionRule< : never; // @public -export type ConditionalPolicyResult = { +export type ConditionalPolicyDecision = { result: AuthorizeResult.CONDITIONAL; - conditions: { - pluginId: string; - resourceType: string; - conditions: PermissionCriteria; - }; + pluginId: string; + resourceType: string; + conditions: PermissionCriteria; }; // @public @@ -63,11 +61,9 @@ export const createConditionExports: < rules: TRules; }) => { conditions: Conditions; - createConditions: (conditions: PermissionCriteria) => { - pluginId: string; - resourceType: string; - conditions: PermissionCriteria; - }; + createPolicyDecision: ( + conditions: PermissionCriteria, + ) => ConditionalPolicyDecision; }; // @public @@ -103,7 +99,7 @@ export interface PermissionPolicy { handle( request: PolicyAuthorizeRequest, user?: BackstageIdentity, - ): Promise; + ): Promise; } // @public @@ -122,9 +118,9 @@ export type PermissionRule< export type PolicyAuthorizeRequest = Omit; // @public -export type PolicyResult = +export type PolicyDecision = | { result: AuthorizeResult.ALLOW | AuthorizeResult.DENY; } - | ConditionalPolicyResult; + | ConditionalPolicyDecision; ``` diff --git a/plugins/permission-node/src/integration/createConditionExports.test.ts b/plugins/permission-node/src/integration/createConditionExports.test.ts index 6f0f8f8632..a849d82268 100644 --- a/plugins/permission-node/src/integration/createConditionExports.test.ts +++ b/plugins/permission-node/src/integration/createConditionExports.test.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import { AuthorizeResult } from '@backstage/plugin-permission-common'; import { createConditionExports } from './createConditionExports'; const testIntegration = () => @@ -63,11 +64,14 @@ describe('createConditionExports', () => { describe('createConditions', () => { it('wraps conditions in an object with resourceType and pluginId', () => { - const { createConditions } = testIntegration(); + const { createPolicyDecision } = testIntegration(); expect( - createConditions({ allOf: [{ rule: 'testRule1', params: ['a', 1] }] }), + createPolicyDecision({ + allOf: [{ rule: 'testRule1', params: ['a', 1] }], + }), ).toEqual({ + result: AuthorizeResult.CONDITIONAL, pluginId: 'test-plugin', resourceType: 'test-resource', conditions: { diff --git a/plugins/permission-node/src/integration/createConditionExports.ts b/plugins/permission-node/src/integration/createConditionExports.ts index 30cff6628a..fd351128ed 100644 --- a/plugins/permission-node/src/integration/createConditionExports.ts +++ b/plugins/permission-node/src/integration/createConditionExports.ts @@ -15,9 +15,11 @@ */ import { + AuthorizeResult, PermissionCondition, PermissionCriteria, } from '@backstage/plugin-permission-common'; +import { ConditionalPolicyDecision } from '../policy'; import { PermissionRule } from '../types'; import { createConditionFactory } from './createConditionFactory'; @@ -73,11 +75,9 @@ export const createConditionExports = < rules: TRules; }): { conditions: Conditions; - createConditions: (conditions: PermissionCriteria) => { - pluginId: string; - resourceType: string; - conditions: PermissionCriteria; - }; + createPolicyDecision: ( + conditions: PermissionCriteria, + ) => ConditionalPolicyDecision; } => { const { pluginId, resourceType, rules } = options; @@ -89,9 +89,10 @@ export const createConditionExports = < }), {} as Conditions, ), - createConditions: ( + createPolicyDecision: ( conditions: PermissionCriteria, ) => ({ + result: AuthorizeResult.CONDITIONAL, pluginId, resourceType, conditions, diff --git a/plugins/permission-node/src/policy/index.ts b/plugins/permission-node/src/policy/index.ts index f04c2a094c..c8216989a1 100644 --- a/plugins/permission-node/src/policy/index.ts +++ b/plugins/permission-node/src/policy/index.ts @@ -15,8 +15,8 @@ */ export type { - ConditionalPolicyResult, + ConditionalPolicyDecision, PermissionPolicy, PolicyAuthorizeRequest, - PolicyResult, + PolicyDecision, } from './types'; diff --git a/plugins/permission-node/src/policy/types.ts b/plugins/permission-node/src/policy/types.ts index 44380bad26..3548d051f6 100644 --- a/plugins/permission-node/src/policy/types.ts +++ b/plugins/permission-node/src/policy/types.ts @@ -48,13 +48,11 @@ export type PolicyAuthorizeRequest = Omit; * identifiers needed to evaluate the returned conditions. * @public */ -export type ConditionalPolicyResult = { +export type ConditionalPolicyDecision = { result: AuthorizeResult.CONDITIONAL; - conditions: { - pluginId: string; - resourceType: string; - conditions: PermissionCriteria; - }; + pluginId: string; + resourceType: string; + conditions: PermissionCriteria; }; /** @@ -62,9 +60,9 @@ export type ConditionalPolicyResult = { * * @public */ -export type PolicyResult = +export type PolicyDecision = | { result: AuthorizeResult.ALLOW | AuthorizeResult.DENY } - | ConditionalPolicyResult; + | ConditionalPolicyDecision; /** * A policy to evaluate authorization requests for any permissioned action performed in Backstage. @@ -86,5 +84,5 @@ export interface PermissionPolicy { handle( request: PolicyAuthorizeRequest, user?: BackstageIdentity, - ): Promise; + ): Promise; }