From 430b3f7a8f88e965587b292971b7008ff8bc8a67 Mon Sep 17 00:00:00 2001 From: Mike Lewis Date: Fri, 19 Nov 2021 09:35:09 +0000 Subject: [PATCH 1/3] permission-common: accept configApi in PermissionClient constructor Moving the responsibility of pulling the right configuration key inside the PermissionClient keeps it as the responsibility of permission-common, whereas putting it on the outside means anyone constructing a PermissionClient has to know which key to use. Signed-off-by: Mike Lewis --- plugins/permission-common/api-report.md | 4 +++- plugins/permission-common/package.json | 1 + plugins/permission-common/src/PermissionClient.test.ts | 8 ++++++-- plugins/permission-common/src/PermissionClient.ts | 5 +++-- 4 files changed, 13 insertions(+), 5 deletions(-) diff --git a/plugins/permission-common/api-report.md b/plugins/permission-common/api-report.md index cbf1877c75..043b27554c 100644 --- a/plugins/permission-common/api-report.md +++ b/plugins/permission-common/api-report.md @@ -3,6 +3,8 @@ > Do not edit this file. It is a report generated by [API Extractor](https://api-extractor.com/). ```ts +import { Config } from '@backstage/config'; + // @public export type AuthorizeRequest = { permission: Permission; @@ -67,7 +69,7 @@ export type PermissionAttributes = { // @public export class PermissionClient { - constructor(options: { discoveryApi: DiscoveryApi; enabled?: boolean }); + constructor(options: { discoveryApi: DiscoveryApi; configApi: Config }); authorize( requests: AuthorizeRequest[], options?: AuthorizeRequestOptions, diff --git a/plugins/permission-common/package.json b/plugins/permission-common/package.json index 95a0d9d3c3..9e1d60f782 100644 --- a/plugins/permission-common/package.json +++ b/plugins/permission-common/package.json @@ -38,6 +38,7 @@ "url": "https://github.com/backstage/backstage/issues" }, "dependencies": { + "@backstage/config": "^0.1.11", "@backstage/errors": "^0.1.2", "cross-fetch": "^3.0.6", "uuid": "^8.0.0", diff --git a/plugins/permission-common/src/PermissionClient.test.ts b/plugins/permission-common/src/PermissionClient.test.ts index 06e2ae0561..8d7933d967 100644 --- a/plugins/permission-common/src/PermissionClient.test.ts +++ b/plugins/permission-common/src/PermissionClient.test.ts @@ -16,6 +16,7 @@ import { RestContext, rest } from 'msw'; import { setupServer } from 'msw/node'; +import { ConfigReader } from '@backstage/config'; import { PermissionClient } from './PermissionClient'; import { AuthorizeRequest, AuthorizeResult, Identified } from './types/api'; import { DiscoveryApi } from './types/discovery'; @@ -32,7 +33,7 @@ const discoveryApi: DiscoveryApi = { }; const client: PermissionClient = new PermissionClient({ discoveryApi, - enabled: true, + configApi: new ConfigReader({ permission: { enabled: true } }), }); const mockPermission: Permission = { @@ -156,7 +157,10 @@ describe('PermissionClient', () => { return res(json(responses)); }, ); - const disabled = new PermissionClient({ discoveryApi, enabled: false }); + const disabled = new PermissionClient({ + discoveryApi, + configApi: new ConfigReader({ permission: { enabled: false } }), + }); const response = await disabled.authorize([mockAuthorizeRequest]); expect(response[0]).toEqual( expect.objectContaining({ result: AuthorizeResult.ALLOW }), diff --git a/plugins/permission-common/src/PermissionClient.ts b/plugins/permission-common/src/PermissionClient.ts index 5b17ccd333..aa1cde9ba1 100644 --- a/plugins/permission-common/src/PermissionClient.ts +++ b/plugins/permission-common/src/PermissionClient.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import { Config } from '@backstage/config'; import { ResponseError } from '@backstage/errors'; import fetch from 'cross-fetch'; import * as uuid from 'uuid'; @@ -74,9 +75,9 @@ export class PermissionClient { private readonly enabled: boolean; private readonly discoveryApi: DiscoveryApi; - constructor(options: { discoveryApi: DiscoveryApi; enabled?: boolean }) { + constructor(options: { discoveryApi: DiscoveryApi; configApi: Config }) { this.discoveryApi = options.discoveryApi; - this.enabled = options.enabled ?? false; + this.enabled = options.configApi.getBoolean('permission.enabled'); } /** From d1dcd7e309486570627baaf4801d19d9b250ce77 Mon Sep 17 00:00:00 2001 From: Mike Lewis Date: Fri, 19 Nov 2021 09:34:21 +0000 Subject: [PATCH 2/3] permission-common: make permission.enabled config optional Signed-off-by: Mike Lewis --- .../src/PermissionClient.test.ts | 24 ++++++++++++++++++- .../permission-common/src/PermissionClient.ts | 3 ++- 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/plugins/permission-common/src/PermissionClient.test.ts b/plugins/permission-common/src/PermissionClient.test.ts index 8d7933d967..b2e66986a1 100644 --- a/plugins/permission-common/src/PermissionClient.test.ts +++ b/plugins/permission-common/src/PermissionClient.test.ts @@ -146,7 +146,7 @@ describe('PermissionClient', () => { ).rejects.toThrowError(/invalid input/i); }); - it('should allow all when authorization is not enabled', async () => { + it('should allow all when permission.enabled is false', async () => { mockAuthorizeHandler.mockImplementationOnce( (req, res, { json }: RestContext) => { const responses = req.body.map((a: Identified) => ({ @@ -167,5 +167,27 @@ describe('PermissionClient', () => { ); expect(mockAuthorizeHandler).not.toBeCalled(); }); + + it('should allow all when permission.enabled is not configured', async () => { + mockAuthorizeHandler.mockImplementationOnce( + (req, res, { json }: RestContext) => { + const responses = req.body.map((a: Identified) => ({ + id: a.id, + outcome: AuthorizeResult.DENY, + })); + + return res(json(responses)); + }, + ); + const disabled = new PermissionClient({ + discoveryApi, + configApi: new ConfigReader({}), + }); + const response = await disabled.authorize([mockAuthorizeRequest]); + expect(response[0]).toEqual( + expect.objectContaining({ result: AuthorizeResult.ALLOW }), + ); + expect(mockAuthorizeHandler).not.toBeCalled(); + }); }); }); diff --git a/plugins/permission-common/src/PermissionClient.ts b/plugins/permission-common/src/PermissionClient.ts index aa1cde9ba1..f98d40a3c4 100644 --- a/plugins/permission-common/src/PermissionClient.ts +++ b/plugins/permission-common/src/PermissionClient.ts @@ -77,7 +77,8 @@ export class PermissionClient { constructor(options: { discoveryApi: DiscoveryApi; configApi: Config }) { this.discoveryApi = options.discoveryApi; - this.enabled = options.configApi.getBoolean('permission.enabled'); + this.enabled = + options.configApi.getOptionalBoolean('permission.enabled') ?? false; } /** From 92439056fbe089749f123d52b0de07bf150ff100 Mon Sep 17 00:00:00 2001 From: Mike Lewis Date: Fri, 19 Nov 2021 09:35:47 +0000 Subject: [PATCH 3/3] add changeset for new PermissionClient constructor signature Signed-off-by: Mike Lewis --- .changeset/purple-grapes-attack.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/purple-grapes-attack.md diff --git a/.changeset/purple-grapes-attack.md b/.changeset/purple-grapes-attack.md new file mode 100644 index 0000000000..a2a99690e9 --- /dev/null +++ b/.changeset/purple-grapes-attack.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-permission-common': minor +--- + +Accept configApi rather than enabled flag in PermissionClient constructor.