From 85b9e1ae608faaa202104bb9b59a5e7441442fe6 Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Wed, 5 Jan 2022 12:35:31 +0000 Subject: [PATCH] permission-backend: use dataloader in PermissionIntegrationClient Signed-off-by: MT Lewis --- .../PermissionIntegrationClient.test.ts | 76 +++++++++++++++++++ .../service/PermissionIntegrationClient.ts | 56 +++++++------- 2 files changed, 102 insertions(+), 30 deletions(-) diff --git a/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts b/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts index 4697277377..cc1de05127 100644 --- a/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts +++ b/plugins/permission-backend/src/service/PermissionIntegrationClient.test.ts @@ -484,5 +484,81 @@ describe('PermissionIntegrationClient', () => { expect(plugin1Router).toHaveBeenCalledTimes(1); expect(plugin2Router).toHaveBeenCalledTimes(1); }); + + it('leaves conditional results without resourceRefs unchanged', async () => { + await expect( + client.applyConditions([ + { + id: '123', + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-1', + resourceRef: 'testResource1', + resourceType: 'test-resource', + conditions: { rule: 'RULE_1', params: ['yes'] }, + }, + { + id: '234', + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-2', + resourceRef: 'testResource1', + resourceType: 'test-resource', + conditions: { rule: 'RULE_1', params: ['no'] }, + }, + { + id: '345', + result: AuthorizeResult.ALLOW, + }, + { + id: '456', + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-1', + resourceRef: 'testResource1', + resourceType: 'test-resource', + conditions: { rule: 'RULE_1', params: ['no'] }, + }, + { + id: '567', + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-2', + resourceRef: 'testResource1', + resourceType: 'test-resource', + conditions: { rule: 'RULE_1', params: ['yes'] }, + }, + { + id: '789', + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-2', + resourceType: 'test-resource', + conditions: { + anyOf: [ + { rule: 'RULE_1', params: ['yes'] }, + { rule: 'RULE_2', params: ['yes'] }, + ], + }, + }, + ]), + ).resolves.toEqual([ + { id: '123', result: AuthorizeResult.ALLOW }, + { id: '234', result: AuthorizeResult.DENY }, + { id: '345', result: AuthorizeResult.ALLOW }, + { id: '456', result: AuthorizeResult.DENY }, + { id: '567', result: AuthorizeResult.ALLOW }, + { + id: '789', + result: AuthorizeResult.CONDITIONAL, + pluginId: 'plugin-2', + resourceType: 'test-resource', + conditions: { + anyOf: [ + { rule: 'RULE_1', params: ['yes'] }, + { rule: 'RULE_2', params: ['yes'] }, + ], + }, + }, + ]); + + expect(plugin1Router).toHaveBeenCalledTimes(1); + expect(plugin2Router).toHaveBeenCalledTimes(1); + }); }); }); diff --git a/plugins/permission-backend/src/service/PermissionIntegrationClient.ts b/plugins/permission-backend/src/service/PermissionIntegrationClient.ts index 644cac1b38..520e8b19ae 100644 --- a/plugins/permission-backend/src/service/PermissionIntegrationClient.ts +++ b/plugins/permission-backend/src/service/PermissionIntegrationClient.ts @@ -14,9 +14,10 @@ * limitations under the License. */ -import { groupBy, keyBy } from 'lodash'; +import { memoize } from 'lodash'; import fetch from 'node-fetch'; import { z } from 'zod'; +import DataLoader from 'dataloader'; import { PluginEndpointDiscovery } from '@backstage/backend-common'; import { AuthorizeResponse, @@ -25,6 +26,7 @@ import { } from '@backstage/plugin-permission-common'; import { ApplyConditionsResponse, + ApplyConditionsResponseEntry, ConditionalPolicyDecision, PolicyDecision, } from '@backstage/plugin-permission-node'; @@ -38,12 +40,10 @@ const responseSchema = z.array( }), ); -export type ResourcePolicyDecision = Identified< - PolicyDecision & { resourceRef?: string } ->; - -type ConditionalResourcePolicyDecision = Identified< - ConditionalPolicyDecision & { resourceRef: string } +type ResourceDecision = Identified< + T & { + resourceRef?: string; + } >; export class PermissionIntegrationClient { @@ -54,23 +54,34 @@ export class PermissionIntegrationClient { } async applyConditions( - decisions: ResourcePolicyDecision[], + decisions: ResourceDecision[], authHeader?: string, ): Promise[]> { - const responses = await Promise.all( - this.groupRequestsByPluginId(decisions).map(([pluginId, requests]) => - this.makeRequest(pluginId, requests, authHeader), - ), + const loaderFor = memoize( + (pluginId: string) => + new DataLoader< + Identified, + ApplyConditionsResponseEntry + >(requests => this.makeRequest(pluginId, requests, authHeader)), ); - const responseIndex = keyBy(responses.flat(), 'id'); + return Promise.all( + decisions.map(decision => { + if ( + decision.result !== AuthorizeResult.CONDITIONAL || + !decision.resourceRef + ) { + return decision; + } - return decisions.map(decision => responseIndex[decision.id] ?? decision); + return loaderFor(decision.pluginId).load(decision); + }), + ); } private async makeRequest( pluginId: string, - decisions: ConditionalResourcePolicyDecision[], + decisions: readonly ResourceDecision[], authHeader?: string, ): Promise { const endpoint = `${await this.discovery.getBaseUrl( @@ -101,19 +112,4 @@ export class PermissionIntegrationClient { return responseSchema.parse(await response.json()); } - - private groupRequestsByPluginId( - decisions: ResourcePolicyDecision[], - ): [string, ConditionalResourcePolicyDecision[]][] { - return Object.entries( - groupBy( - decisions.filter( - (decision): decision is ConditionalResourcePolicyDecision => - decision.result === AuthorizeResult.CONDITIONAL && - !!decision.resourceRef, - ), - 'pluginId', - ), - ); - } }