From 2b07063d7777fe3ed0b01ae82085e1e6af9d2c0b Mon Sep 17 00:00:00 2001 From: Joe Porpeglia Date: Tue, 22 Mar 2022 21:44:47 -0400 Subject: [PATCH 01/24] Add PermissionEvaluator interface Signed-off-by: Joe Porpeglia --- .changeset/few-seas-fail.md | 8 +++ plugins/permission-common/api-report.md | 40 +++++++++++ plugins/permission-common/src/types/api.ts | 70 ++++++++++++++++++++ plugins/permission-common/src/types/index.ts | 6 ++ 4 files changed, 124 insertions(+) create mode 100644 .changeset/few-seas-fail.md diff --git a/.changeset/few-seas-fail.md b/.changeset/few-seas-fail.md new file mode 100644 index 0000000000..8ace0f3515 --- /dev/null +++ b/.changeset/few-seas-fail.md @@ -0,0 +1,8 @@ +--- +'@backstage/plugin-permission-common': patch +--- + +Added `PermissionEvaluator`, which will replace the existing `PermissionAuthorizer` interface. This new interface provides stronger type safety and validation by splitting `PermissionAuthorizer.authorize()` into two methods: + +- `authorize()`: Used when the caller requires a definitive decision. +- `query()`: Used when the caller can optimize the evaluation of any conditional decisions. For example, a plugin backend may want to use conditions in a database query instead of evaluating each resource in memory. diff --git a/plugins/permission-common/api-report.md b/plugins/permission-common/api-report.md index ab0c68470d..2f283c0d8d 100644 --- a/plugins/permission-common/api-report.md +++ b/plugins/permission-common/api-report.md @@ -15,6 +15,20 @@ export type AnyOfCriteria = { anyOf: NonEmptyArray>; }; +// @public +export type AuthorizePermissionRequest = + | { + permission: Exclude; + resourceRef?: never; + } + | { + permission: ResourcePermission; + resourceRef: string; + }; + +// @public +export type AuthorizePermissionResponse = DefinitivePolicyDecision; + // @public export type AuthorizeRequestOptions = { token?: string; @@ -78,6 +92,11 @@ export type EvaluatePermissionResponse = PolicyDecision; export type EvaluatePermissionResponseBatch = PermissionMessageBatch; +// @public +export type EvaluatorRequestOptions = { + token?: string; +}; + // @public export type IdentifiedPermissionMessage = T & { id: string; @@ -163,6 +182,18 @@ export type PermissionCriteria = | NotCriteria | TQuery; +// @public +export interface PermissionEvaluator { + authorize( + requests: AuthorizePermissionRequest[], + options?: EvaluatorRequestOptions, + ): Promise; + query( + requests: QueryPermissionRequest[], + options?: EvaluatorRequestOptions, + ): Promise; +} + // @public export type PermissionMessageBatch = { items: IdentifiedPermissionMessage[]; @@ -173,6 +204,15 @@ export type PolicyDecision = | DefinitivePolicyDecision | ConditionalPolicyDecision; +// @public +export type QueryPermissionRequest = { + permission: ResourcePermission; + resourceRef?: never; +}; + +// @public +export type QueryPermissionResponse = PolicyDecision; + // @public export type ResourcePermission = PermissionBase< diff --git a/plugins/permission-common/src/types/api.ts b/plugins/permission-common/src/types/api.ts index 6aaabf978d..1bc9af5ad8 100644 --- a/plugins/permission-common/src/types/api.ts +++ b/plugins/permission-common/src/types/api.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import { ResourcePermission } from '.'; import { Permission } from './permission'; /** @@ -182,3 +183,72 @@ export type EvaluatePermissionResponse = PolicyDecision; */ export type EvaluatePermissionResponseBatch = PermissionMessageBatch; + +/** + * Request object for {@link PermissionEvaluator.authorize}. If a {@link ResourcePermission} + * is provided, it must include a corresponding `resourceRef`. + * @public + */ +export type AuthorizePermissionRequest = + | { + permission: Exclude; + resourceRef?: never; + } + | { permission: ResourcePermission; resourceRef: string }; + +/** + * Response object for {@link PermissionEvaluator.authorize}. + * @public + */ +export type AuthorizePermissionResponse = DefinitivePolicyDecision; + +/** + * Request object for {@link PermissionEvaluator.query}. + * @public + */ +export type QueryPermissionRequest = { + permission: ResourcePermission; + resourceRef?: never; +}; + +/** + * Response object for {@link PermissionEvaluator.query}. + * @public + */ +export type QueryPermissionResponse = PolicyDecision; + +/** + * A client interacting with the permission backend can implement this evaluator interface. + * + * @public + */ +export interface PermissionEvaluator { + /** + * Evaluates {@link Permission | Permissions} and returns a definitive decision. + */ + authorize( + requests: AuthorizePermissionRequest[], + options?: EvaluatorRequestOptions, + ): Promise; + + /** + * Evaluates {@link ResourcePermission | ResourcePermissions} and returns both definitive and + * conditional decisions, depending on the configured + * {@link @backstage/plugin-permission-node#PermissionPolicy}. This method is useful when the + * caller needs more control over the processing of conditional decisions. For example, a plugin + * backend may want to use {@link PermissionCriteria | conditions} in a database query instead of + * evaluating each resource in memory. + */ + query( + requests: QueryPermissionRequest[], + options?: EvaluatorRequestOptions, + ): Promise; +} + +/** + * Options for {@link PermissionEvaluator} requests. + * @public + */ +export type EvaluatorRequestOptions = { + token?: string; +}; diff --git a/plugins/permission-common/src/types/index.ts b/plugins/permission-common/src/types/index.ts index 1a993a981c..f244a70b1a 100644 --- a/plugins/permission-common/src/types/index.ts +++ b/plugins/permission-common/src/types/index.ts @@ -22,6 +22,12 @@ export type { EvaluatePermissionResponseBatch, IdentifiedPermissionMessage, PermissionMessageBatch, + AuthorizePermissionRequest, + AuthorizePermissionResponse, + QueryPermissionRequest, + QueryPermissionResponse, + EvaluatorRequestOptions, + PermissionEvaluator, ConditionalPolicyDecision, DefinitivePolicyDecision, PolicyDecision, From 8960a2bfed11411c09f34a0e8acb090fe6d74cb2 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Wed, 23 Mar 2022 14:45:02 +0100 Subject: [PATCH 02/24] Split PermissionClient#authorize Co-authored-by: Mike Lewis Signed-off-by: Vincenzo Scamporlino --- .../src/PermissionClient.test.ts | 188 +++++++++++++++++- .../permission-common/src/PermissionClient.ts | 169 ++++++++++------ plugins/permission-common/src/types/api.ts | 14 ++ plugins/permission-common/src/types/index.ts | 1 + .../src/ServerPermissionClient.test.ts | 186 ++++++++++++----- .../src/ServerPermissionClient.ts | 48 +++-- plugins/permission-node/src/policy/index.ts | 2 +- plugins/permission-node/src/policy/types.ts | 16 +- 8 files changed, 473 insertions(+), 151 deletions(-) diff --git a/plugins/permission-common/src/PermissionClient.test.ts b/plugins/permission-common/src/PermissionClient.test.ts index 65a7c7fbbe..ec038e7f16 100644 --- a/plugins/permission-common/src/PermissionClient.test.ts +++ b/plugins/permission-common/src/PermissionClient.test.ts @@ -22,6 +22,7 @@ import { EvaluatePermissionRequest, AuthorizeResult, IdentifiedPermissionMessage, + ConditionalPolicyDecision, } from './types/api'; import { DiscoveryApi } from './types/discovery'; import { createPermission } from './permissions'; @@ -140,7 +141,7 @@ describe('PermissionClient', () => { ); await expect( client.authorize([mockAuthorizeQuery], { token }), - ).rejects.toThrowError(/Unexpected authorization response/i); + ).rejects.toThrowError(/items in response do not match request/i); }); it('should reject invalid responses', async () => { @@ -209,4 +210,189 @@ describe('PermissionClient', () => { expect(mockAuthorizeHandler).not.toBeCalled(); }); }); + + describe('query', () => { + const mockResourceAuthorizeQuery = { + permission: mockPermission, + }; + + const mockPolicyDecisionHandler = jest.fn( + (req, res, { json }: RestContext) => { + const responses = req.body.items.map( + (a: IdentifiedPermissionMessage) => ({ + id: a.id, + pluginId: 'test-plugin', + resourceType: 'test-resource', + result: AuthorizeResult.CONDITIONAL, + conditions: { + rule: 'FOO', + params: ['bar'], + }, + }), + ); + + return res(json({ items: responses })); + }, + ); + + beforeEach(() => { + server.use( + rest.post(`${mockBaseUrl}/authorize`, mockPolicyDecisionHandler), + ); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + it('should fetch entities from correct endpoint', async () => { + await client.query([mockResourceAuthorizeQuery]); + expect(mockPolicyDecisionHandler).toHaveBeenCalled(); + }); + + it('should include a request body', async () => { + await client.query([mockResourceAuthorizeQuery]); + + const request = mockPolicyDecisionHandler.mock.calls[0][0]; + + expect(request.body).toEqual({ + items: [ + expect.objectContaining({ + permission: mockPermission, + }), + ], + }); + }); + + it('should return the response from the fetch request', async () => { + const response = await client.query([mockResourceAuthorizeQuery]); + expect(response[0]).toEqual( + expect.objectContaining({ + result: AuthorizeResult.CONDITIONAL, + conditions: { + rule: 'FOO', + params: ['bar'], + }, + }), + ); + }); + + it('should not include authorization headers if no token is supplied', async () => { + await client.query([mockResourceAuthorizeQuery]); + + const request = mockPolicyDecisionHandler.mock.calls[0][0]; + expect(request.headers.has('authorization')).toEqual(false); + }); + + it('should include correctly-constructed authorization header if token is supplied', async () => { + await client.query([mockResourceAuthorizeQuery], { + token, + }); + + const request = mockPolicyDecisionHandler.mock.calls[0][0]; + expect(request.headers.get('authorization')).toEqual('Bearer fake-token'); + }); + + it('should forward response errors', async () => { + mockPolicyDecisionHandler.mockImplementationOnce( + (_req, res, { status }: RestContext) => { + return res(status(401)); + }, + ); + await expect( + client.query([mockResourceAuthorizeQuery], { + token, + }), + ).rejects.toThrowError(/request failed with 401/i); + }); + + it('should reject responses with missing ids', async () => { + mockPolicyDecisionHandler.mockImplementationOnce( + (_req, res, { json }: RestContext) => { + return res( + json({ + items: [{ id: 'wrong-id', result: AuthorizeResult.ALLOW }], + }), + ); + }, + ); + await expect( + client.query([mockResourceAuthorizeQuery], { + token, + }), + ).rejects.toThrowError(/items in response do not match request/i); + }); + + it('should reject invalid responses', async () => { + mockPolicyDecisionHandler.mockImplementationOnce( + (req, res, { json }: RestContext) => { + const responses = req.body.items.map( + (a: IdentifiedPermissionMessage) => ({ + id: a.id, + outcome: AuthorizeResult.ALLOW, + }), + ); + + return res(json({ items: responses })); + }, + ); + await expect( + client.query([mockResourceAuthorizeQuery], { + token, + }), + ).rejects.toThrowError(/invalid input/i); + }); + + it('should allow all when permission.enabled is false', async () => { + mockPolicyDecisionHandler.mockImplementationOnce( + (req, res, { json }: RestContext) => { + const responses = req.body.map( + (a: IdentifiedPermissionMessage) => ({ + id: a.id, + result: AuthorizeResult.DENY, + }), + ); + + return res(json({ items: responses })); + }, + ); + const disabled = new PermissionClient({ + discovery, + config: new ConfigReader({ permission: { enabled: false } }), + }); + const response = await disabled.query([mockResourceAuthorizeQuery], { + token, + }); + expect(response[0]).toEqual( + expect.objectContaining({ result: AuthorizeResult.ALLOW }), + ); + expect(mockPolicyDecisionHandler).not.toBeCalled(); + }); + + it('should allow all when permission.enabled is not configured', async () => { + mockPolicyDecisionHandler.mockImplementationOnce( + (req, res, { json }: RestContext) => { + const responses = req.body.map( + (a: IdentifiedPermissionMessage) => ({ + id: a.id, + outcome: AuthorizeResult.DENY, + }), + ); + + return res(json(responses)); + }, + ); + const disabled = new PermissionClient({ + discovery, + config: new ConfigReader({}), + }); + const response = await disabled.query([mockResourceAuthorizeQuery], { + token, + }); + expect(response[0]).toEqual( + expect.objectContaining({ result: AuthorizeResult.ALLOW }), + ); + expect(mockPolicyDecisionHandler).not.toBeCalled(); + }); + }); }); diff --git a/plugins/permission-common/src/PermissionClient.ts b/plugins/permission-common/src/PermissionClient.ts index 72f6b04567..9ca9bfe255 100644 --- a/plugins/permission-common/src/PermissionClient.ts +++ b/plugins/permission-common/src/PermissionClient.ts @@ -21,19 +21,20 @@ import * as uuid from 'uuid'; import { z } from 'zod'; import { AuthorizeResult, - EvaluatePermissionRequest, - EvaluatePermissionResponse, - IdentifiedPermissionMessage, + DefinitivePolicyDecision, + PermissionMessageBatch, PermissionCriteria, PermissionCondition, - EvaluatePermissionResponseBatch, - EvaluatePermissionRequestBatch, + PermissionEvaluator, + PolicyDecision, + QueryPermissionRequest, + AuthorizePermissionRequest, + EvaluatorRequestOptions, + AuthorizePermissionResponse, + QueryPermissionResponse, } from './types/api'; import { DiscoveryApi } from './types/discovery'; -import { - PermissionAuthorizer, - AuthorizeRequestOptions, -} from './types/permission'; +import { AuthorizeRequestOptions } from './types/permission'; const permissionCriteriaSchema: z.ZodSchema< PermissionCriteria @@ -58,30 +59,56 @@ const permissionCriteriaSchema: z.ZodSchema< .or(z.object({ not: permissionCriteriaSchema }).strict()), ); -const responseSchema = z.object({ - items: z.array( - z - .object({ - id: z.string(), - result: z - .literal(AuthorizeResult.ALLOW) - .or(z.literal(AuthorizeResult.DENY)), - }) - .or( - z.object({ - id: z.string(), - result: z.literal(AuthorizeResult.CONDITIONAL), - conditions: permissionCriteriaSchema, - }), +const authorizeDecisionSchema: z.ZodSchema = z.object( + { + result: z + .literal(AuthorizeResult.ALLOW) + .or(z.literal(AuthorizeResult.DENY)), + }, +); + +const policyDecisionSchema: z.ZodSchema = z.union([ + z.object({ + result: z + .literal(AuthorizeResult.ALLOW) + .or(z.literal(AuthorizeResult.DENY)), + }), + z.object({ + result: z.literal(AuthorizeResult.CONDITIONAL), + pluginId: z.string(), + resourceType: z.string(), + conditions: permissionCriteriaSchema, + }), +]); + +const responseSchema = ( + itemSchema: z.ZodSchema, + ids: Set, +): z.ZodSchema> => + z.object({ + items: z + .array( + z.intersection( + z.object({ + id: z.string(), + }), + itemSchema, + ), + ) + .refine( + items => + items.length === ids.size && items.every(({ id }) => ids.has(id)), + { + message: 'Items in response do not match request', + }, ), - ), -}); + }); /** * An isomorphic client for requesting authorization for Backstage permissions. * @public */ -export class PermissionClient implements PermissionAuthorizer { +export class PermissionClient implements PermissionEvaluator { private readonly enabled: boolean; private readonly discovery: DiscoveryApi; @@ -92,35 +119,67 @@ export class PermissionClient implements PermissionAuthorizer { } /** - * Request authorization from the permission-backend for the given set of permissions. + * Request authorization from the permission-backend for the given set of + * permissions. * - * Authorization requests check that a given Backstage user can perform a protected operation, - * potentially for a specific resource (such as a catalog entity). The Backstage identity token - * should be included in the `options` if available. + * @remarks * - * Permissions can be imported from plugins exposing them, such as `catalogEntityReadPermission`. + * Checks that a given Backstage user can perform a protected operation. When + * authorization is for a {@link ResourcePermission}s, a resourceRef + * corresponding to the resource should always be supplied along with the + * permission. The Backstage identity token should be included in the + * `options` if available. + * + * Permissions can be imported from plugins exposing them, such as + * `catalogEntityReadPermission`. + * + * For each query, the response will be either ALLOW or DENY. * - * The response will be either ALLOW or DENY when either the permission has no resourceType, or a - * resourceRef is provided in the request. For permissions with a resourceType, CONDITIONAL may be - * returned if no resourceRef is provided in the request. Conditional responses are intended only - * for backends which have access to the data source for permissioned resources, so that filters - * can be applied when loading collections of resources. * @public */ async authorize( - queries: EvaluatePermissionRequest[], - options?: AuthorizeRequestOptions, - ): Promise { + requests: AuthorizePermissionRequest[], + options?: EvaluatorRequestOptions, + ): Promise { // TODO(permissions): it would be great to provide some kind of typing guarantee that // conditional responses will only ever be returned for requests containing a resourceType // but no resourceRef. That way clients who aren't prepared to handle filtering according // to conditions can be guaranteed that they won't unexpectedly get a CONDITIONAL response. + return this.makeRequest(requests, authorizeDecisionSchema, options); + } + + /** + * Fetch the conditional authorization decisions for the given set of + * {@link ResourcePermission}s in order to apply the conditions to an upstream + * data source. + * + * @remarks + * + * For each query, the response will be either ALLOW, DENY, or CONDITIONAL. + * Conditional responses are intended only for backends which have access to + * the data source for permissioned resources, so that filters can be applied + * when loading collections of resources. + * + * @public + */ + async query( + queries: QueryPermissionRequest[], + options?: EvaluatorRequestOptions, + ): Promise { + return this.makeRequest(queries, policyDecisionSchema, options); + } + + private async makeRequest( + queries: TQuery[], + itemSchema: z.ZodSchema, + options?: AuthorizeRequestOptions, + ) { if (!this.enabled) { - return queries.map(_ => ({ result: AuthorizeResult.ALLOW })); + return queries.map(_ => ({ result: AuthorizeResult.ALLOW as const })); } - const request: EvaluatePermissionRequestBatch = { + const request = { items: queries.map(query => ({ id: uuid.v4(), ...query, @@ -141,12 +200,16 @@ export class PermissionClient implements PermissionAuthorizer { } const responseBody = await response.json(); - this.assertValidResponse(request, responseBody); - const responsesById = responseBody.items.reduce((acc, r) => { + const parsedResponse = responseSchema( + itemSchema, + new Set(request.items.map(({ id }) => id)), + ).parse(responseBody); + + const responsesById = parsedResponse.items.reduce((acc, r) => { acc[r.id] = r; return acc; - }, {} as Record>); + }, {} as Record); return request.items.map(query => responsesById[query.id]); } @@ -154,20 +217,4 @@ export class PermissionClient implements PermissionAuthorizer { private getAuthorizationHeader(token?: string): Record { return token ? { Authorization: `Bearer ${token}` } : {}; } - - private assertValidResponse( - request: EvaluatePermissionRequestBatch, - json: any, - ): asserts json is EvaluatePermissionResponseBatch { - const authorizedResponses = responseSchema.parse(json); - const responseIds = authorizedResponses.items.map(r => r.id); - const hasAllRequestIds = request.items.every(r => - responseIds.includes(r.id), - ); - if (!hasAllRequestIds) { - throw new Error( - 'Unexpected authorization response from permission-backend', - ); - } - } } diff --git a/plugins/permission-common/src/types/api.ts b/plugins/permission-common/src/types/api.ts index 1bc9af5ad8..c1996fc462 100644 --- a/plugins/permission-common/src/types/api.ts +++ b/plugins/permission-common/src/types/api.ts @@ -91,6 +91,20 @@ export type PolicyDecision = | DefinitivePolicyDecision | ConditionalPolicyDecision; +/** + * A query to be evaluated by the {@link PermissionPolicy}. + * + * @remarks + * + * Unlike other parts of the permission API, the policy does not accept a resource ref. This keeps + * the policy decoupled from the resource loading and condition applying logic. + * + * @public + */ +export type PolicyQuery = { + permission: Permission; +}; + /** * A condition returned with a CONDITIONAL authorization response. * diff --git a/plugins/permission-common/src/types/index.ts b/plugins/permission-common/src/types/index.ts index f244a70b1a..dd644ff345 100644 --- a/plugins/permission-common/src/types/index.ts +++ b/plugins/permission-common/src/types/index.ts @@ -36,6 +36,7 @@ export type { AllOfCriteria, AnyOfCriteria, NotCriteria, + PolicyQuery, } from './api'; export type { DiscoveryApi } from './discovery'; export type { diff --git a/plugins/permission-node/src/ServerPermissionClient.test.ts b/plugins/permission-node/src/ServerPermissionClient.test.ts index f5711ed4a0..2e3e5d988f 100644 --- a/plugins/permission-node/src/ServerPermissionClient.test.ts +++ b/plugins/permission-node/src/ServerPermissionClient.test.ts @@ -17,9 +17,10 @@ import { ServerPermissionClient } from './ServerPermissionClient'; import { IdentifiedPermissionMessage, - EvaluatePermissionRequest, AuthorizeResult, createPermission, + DefinitivePolicyDecision, + ConditionalPolicyDecision, } from '@backstage/plugin-permission-common'; import { ConfigReader } from '@backstage/config'; import { @@ -31,16 +32,7 @@ import { setupServer } from 'msw/node'; import { RestContext, rest } from 'msw'; const server = setupServer(); -const mockAuthorizeHandler = jest.fn((req, res, { json }: RestContext) => { - const responses = req.body.items.map( - (r: IdentifiedPermissionMessage) => ({ - id: r.id, - result: AuthorizeResult.ALLOW, - }), - ); - return res(json({ items: responses })); -}); const mockBaseUrl = 'http://backstage:9191/i-am-a-mock-base'; const discovery: PluginEndpointDiscovery = { async getBaseUrl() { @@ -50,10 +42,17 @@ const discovery: PluginEndpointDiscovery = { return mockBaseUrl; }, }; -const testPermission = createPermission({ +const testBasicPermission = createPermission({ name: 'test.permission', attributes: {}, }); + +const testResourcePermission = createPermission({ + name: 'test.permission-2', + attributes: {}, + resourceType: 'resource-type', +}); + const config = new ConfigReader({ permission: { enabled: true }, backend: { auth: { keys: [{ secret: 'a-secret-key' }] } }, @@ -63,49 +62,6 @@ const logger = getVoidLogger(); describe('ServerPermissionClient', () => { beforeAll(() => server.listen({ onUnhandledRequest: 'error' })); afterAll(() => server.close()); - beforeEach(() => { - server.use(rest.post(`${mockBaseUrl}/authorize`, mockAuthorizeHandler)); - }); - afterEach(() => server.resetHandlers()); - - it('should bypass authorization if permissions are disabled', async () => { - const client = ServerPermissionClient.fromConfig(new ConfigReader({}), { - discovery, - tokenManager: ServerTokenManager.noop(), - }); - - await client.authorize([{ permission: testPermission }]); - - expect(mockAuthorizeHandler).not.toHaveBeenCalled(); - }); - - it('should bypass authorization if permissions are enabled and request has valid server token', async () => { - const tokenManager = ServerTokenManager.fromConfig(config, { logger }); - const client = ServerPermissionClient.fromConfig(config, { - discovery, - tokenManager, - }); - - await client.authorize([{ permission: testPermission }], { - token: (await tokenManager.getToken()).token, - }); - - expect(mockAuthorizeHandler).not.toHaveBeenCalled(); - }); - - it('should authorize normally if permissions are enabled and request does not have valid server token', async () => { - const tokenManager = ServerTokenManager.fromConfig(config, { logger }); - const client = ServerPermissionClient.fromConfig(config, { - discovery, - tokenManager, - }); - - await client.authorize([{ permission: testPermission }], { - token: 'a-user-token', - }); - - expect(mockAuthorizeHandler).toHaveBeenCalled(); - }); it('should error if permissions are enabled but a no-op token manager is configured', async () => { expect(() => @@ -117,4 +73,126 @@ describe('ServerPermissionClient', () => { 'Backend-to-backend authentication must be configured before enabling permissions. Read more here https://backstage.io/docs/tutorials/backend-to-backend-auth', ); }); + + describe('authorize', () => { + let mockAuthorizeHandler: jest.Mock; + + beforeEach(() => { + mockAuthorizeHandler = jest.fn((req, res, { json }: RestContext) => { + const responses = req.body.items.map( + (r: IdentifiedPermissionMessage) => ({ + id: r.id, + result: AuthorizeResult.ALLOW, + }), + ); + + return res(json({ items: responses })); + }); + + server.use(rest.post(`${mockBaseUrl}/authorize`, mockAuthorizeHandler)); + }); + afterEach(() => server.resetHandlers()); + + it('should bypass the permission backend if permissions are disabled', async () => { + const client = ServerPermissionClient.fromConfig(new ConfigReader({}), { + discovery, + tokenManager: ServerTokenManager.noop(), + }); + + await client.authorize([ + { + permission: testBasicPermission, + }, + ]); + + expect(mockAuthorizeHandler).not.toHaveBeenCalled(); + }); + + it('should bypass the permission backend if permissions are enabled and request has valid server token', async () => { + const tokenManager = ServerTokenManager.fromConfig(config, { logger }); + const client = ServerPermissionClient.fromConfig(config, { + discovery, + tokenManager, + }); + + await client.authorize([{ permission: testBasicPermission }], { + token: (await tokenManager.getToken()).token, + }); + + expect(mockAuthorizeHandler).not.toHaveBeenCalled(); + }); + + it('should call the permission backend if permissions are enabled and request does not have valid server token', async () => { + const tokenManager = ServerTokenManager.fromConfig(config, { logger }); + const client = ServerPermissionClient.fromConfig(config, { + discovery, + tokenManager, + }); + + await client.authorize([{ permission: testBasicPermission }], { + token: 'a-user-token', + }); + + expect(mockAuthorizeHandler).toHaveBeenCalled(); + }); + }); + + describe('query', () => { + let mockAuthorizeHandler: jest.Mock; + + beforeEach(() => { + mockAuthorizeHandler = jest.fn((req, res, { json }: RestContext) => { + const responses = req.body.items.map( + (r: IdentifiedPermissionMessage) => ({ + id: r.id, + result: AuthorizeResult.ALLOW, + }), + ); + + return res(json({ items: responses })); + }); + + server.use(rest.post(`${mockBaseUrl}/authorize`, mockAuthorizeHandler)); + }); + afterEach(() => server.resetHandlers()); + + it('should bypass the permission backend if permissions are disabled', async () => { + const client = ServerPermissionClient.fromConfig(new ConfigReader({}), { + discovery, + tokenManager: ServerTokenManager.noop(), + }); + + await client.query([{ permission: testResourcePermission }]); + + expect(mockAuthorizeHandler).not.toHaveBeenCalled(); + }); + + it('should bypass the permission backend if permissions are enabled and request has valid server token', async () => { + const tokenManager = ServerTokenManager.fromConfig(config, { logger }); + const client = ServerPermissionClient.fromConfig(config, { + discovery, + tokenManager, + }); + + await client.query([{ permission: testResourcePermission }], { + token: (await tokenManager.getToken()).token, + }); + + expect(mockAuthorizeHandler).not.toHaveBeenCalled(); + }); + + it('should call the permission backend if permissions are enabled and request does not have valid server token', async () => { + const tokenManager = ServerTokenManager.fromConfig(config, { logger }); + const client = ServerPermissionClient.fromConfig(config, { + discovery, + tokenManager, + }); + + await client.query([{ permission: testResourcePermission }], { + token: 'a-user-token', + }); + + expect(mockAuthorizeHandler).toHaveBeenCalled(); + }); + }); }); diff --git a/plugins/permission-node/src/ServerPermissionClient.ts b/plugins/permission-node/src/ServerPermissionClient.ts index 56381dff17..2ea11e9e95 100644 --- a/plugins/permission-node/src/ServerPermissionClient.ts +++ b/plugins/permission-node/src/ServerPermissionClient.ts @@ -20,12 +20,14 @@ import { } from '@backstage/backend-common'; import { Config } from '@backstage/config'; import { - EvaluatePermissionRequest, - AuthorizeRequestOptions, - EvaluatePermissionResponse, AuthorizeResult, PermissionClient, - PermissionAuthorizer, + PermissionEvaluator, + AuthorizePermissionRequest, + EvaluatorRequestOptions, + AuthorizePermissionResponse, + PolicyDecision, + QueryPermissionRequest, } from '@backstage/plugin-permission-common'; /** @@ -34,7 +36,7 @@ import { * backend-to-backend requests. * @public */ -export class ServerPermissionClient implements PermissionAuthorizer { +export class ServerPermissionClient implements PermissionEvaluator { private readonly permissionClient: PermissionClient; private readonly tokenManager: TokenManager; private readonly permissionEnabled: boolean; @@ -76,22 +78,22 @@ export class ServerPermissionClient implements PermissionAuthorizer { this.tokenManager = options.tokenManager; this.permissionEnabled = options.permissionEnabled; } + async query( + queries: QueryPermissionRequest[], + options?: EvaluatorRequestOptions, + ): Promise { + return (await this.isEnabled(options?.token)) + ? this.permissionClient.query(queries, options) + : queries.map(_ => ({ result: AuthorizeResult.ALLOW })); + } async authorize( - requests: EvaluatePermissionRequest[], - options?: AuthorizeRequestOptions, - ): Promise { - // Check if permissions are enabled before validating the server token. That - // way when permissions are disabled, the noop token manager can be used - // without fouling up the logic inside the ServerPermissionClient, because - // the code path won't be reached. - if ( - !this.permissionEnabled || - (await this.isValidServerToken(options?.token)) - ) { - return requests.map(_ => ({ result: AuthorizeResult.ALLOW })); - } - return this.permissionClient.authorize(requests, options); + requests: AuthorizePermissionRequest[], + options?: EvaluatorRequestOptions, + ): Promise { + return (await this.isEnabled(options?.token)) + ? this.permissionClient.authorize(requests, options) + : requests.map(_ => ({ result: AuthorizeResult.ALLOW })); } private async isValidServerToken( @@ -105,4 +107,12 @@ export class ServerPermissionClient implements PermissionAuthorizer { .then(() => true) .catch(() => false); } + + private async isEnabled(token?: string) { + // Check if permissions are enabled before validating the server token. That + // way when permissions are disabled, the noop token manager can be used + // without fouling up the logic inside the ServerPermissionClient, because + // the code path won't be reached. + return this.permissionEnabled && !(await this.isValidServerToken(token)); + } } diff --git a/plugins/permission-node/src/policy/index.ts b/plugins/permission-node/src/policy/index.ts index f151cfb4fb..0b7e03cc7c 100644 --- a/plugins/permission-node/src/policy/index.ts +++ b/plugins/permission-node/src/policy/index.ts @@ -14,4 +14,4 @@ * limitations under the License. */ -export type { PermissionPolicy, PolicyQuery } from './types'; +export type { PermissionPolicy } from './types'; diff --git a/plugins/permission-node/src/policy/types.ts b/plugins/permission-node/src/policy/types.ts index 0f469cfbfe..992ad3a1cf 100644 --- a/plugins/permission-node/src/policy/types.ts +++ b/plugins/permission-node/src/policy/types.ts @@ -15,25 +15,11 @@ */ import { - Permission, PolicyDecision, + PolicyQuery, } from '@backstage/plugin-permission-common'; import { BackstageIdentityResponse } from '@backstage/plugin-auth-node'; -/** - * A query to be evaluated by the {@link PermissionPolicy}. - * - * @remarks - * - * Unlike other parts of the permission API, the policy does not accept a resource ref. This keeps - * the policy decoupled from the resource loading and condition applying logic. - * - * @public - */ -export type PolicyQuery = { - permission: Permission; -}; - /** * A policy to evaluate authorization requests for any permissioned action performed in Backstage. * From b1bbb9c7605ad8ed65878cdba5188666fd312888 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Thu, 24 Mar 2022 10:07:10 +0100 Subject: [PATCH 03/24] Fix IdentityPermissionApi typings Signed-off-by: Vincenzo Scamporlino --- .../src/apis/IdentityPermissionApi.ts | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/plugins/permission-react/src/apis/IdentityPermissionApi.ts b/plugins/permission-react/src/apis/IdentityPermissionApi.ts index 80d4eb8859..0bb46f5587 100644 --- a/plugins/permission-react/src/apis/IdentityPermissionApi.ts +++ b/plugins/permission-react/src/apis/IdentityPermissionApi.ts @@ -19,6 +19,7 @@ import { PermissionApi } from './PermissionApi'; import { EvaluatePermissionRequest, EvaluatePermissionResponse, + isResourcePermission, PermissionClient, } from '@backstage/plugin-permission-common'; import { Config } from '@backstage/config'; @@ -47,8 +48,21 @@ export class IdentityPermissionApi implements PermissionApi { async authorize( request: EvaluatePermissionRequest, ): Promise { + const { permission, resourceRef } = request; + if (isResourcePermission(permission)) { + if (!resourceRef) { + throw new Error( + 'A resourceRef should be provided when a ResourcePermission is used.', + ); + } + const response = await this.permissionClient.authorize( + [{ permission, resourceRef }], + await this.identityApi.getCredentials(), + ); + return response[0]; + } const response = await this.permissionClient.authorize( - [request], + [{ permission }], await this.identityApi.getCredentials(), ); return response[0]; From 2903c1fd5d9957bd704d60c8662f3652b5b8e31c Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Thu, 24 Mar 2022 14:20:32 +0100 Subject: [PATCH 04/24] Move PolicyQuery to permission-node Signed-off-by: Vincenzo Scamporlino --- plugins/permission-common/api-report.md | 12 +++++++---- plugins/permission-common/src/types/api.ts | 14 ------------- plugins/permission-common/src/types/index.ts | 1 - plugins/permission-node/api-report.md | 22 +++++++++++++------- plugins/permission-node/src/policy/index.ts | 2 +- plugins/permission-node/src/policy/types.ts | 16 +++++++++++++- 6 files changed, 38 insertions(+), 29 deletions(-) diff --git a/plugins/permission-common/api-report.md b/plugins/permission-common/api-report.md index 2f283c0d8d..e952425bbf 100644 --- a/plugins/permission-common/api-report.md +++ b/plugins/permission-common/api-report.md @@ -157,12 +157,16 @@ export type PermissionBase = { } & TFields; // @public -export class PermissionClient implements PermissionAuthorizer { +export class PermissionClient implements PermissionEvaluator { constructor(options: { discovery: DiscoveryApi; config: Config }); authorize( - queries: EvaluatePermissionRequest[], - options?: AuthorizeRequestOptions, - ): Promise; + requests: AuthorizePermissionRequest[], + options?: EvaluatorRequestOptions, + ): Promise; + query( + queries: QueryPermissionRequest[], + options?: EvaluatorRequestOptions, + ): Promise; } // @public diff --git a/plugins/permission-common/src/types/api.ts b/plugins/permission-common/src/types/api.ts index c1996fc462..1bc9af5ad8 100644 --- a/plugins/permission-common/src/types/api.ts +++ b/plugins/permission-common/src/types/api.ts @@ -91,20 +91,6 @@ export type PolicyDecision = | DefinitivePolicyDecision | ConditionalPolicyDecision; -/** - * A query to be evaluated by the {@link PermissionPolicy}. - * - * @remarks - * - * Unlike other parts of the permission API, the policy does not accept a resource ref. This keeps - * the policy decoupled from the resource loading and condition applying logic. - * - * @public - */ -export type PolicyQuery = { - permission: Permission; -}; - /** * A condition returned with a CONDITIONAL authorization response. * diff --git a/plugins/permission-common/src/types/index.ts b/plugins/permission-common/src/types/index.ts index dd644ff345..f244a70b1a 100644 --- a/plugins/permission-common/src/types/index.ts +++ b/plugins/permission-common/src/types/index.ts @@ -36,7 +36,6 @@ export type { AllOfCriteria, AnyOfCriteria, NotCriteria, - PolicyQuery, } from './api'; export type { DiscoveryApi } from './discovery'; export type { diff --git a/plugins/permission-node/api-report.md b/plugins/permission-node/api-report.md index 57379dd68b..3cf6b70d08 100644 --- a/plugins/permission-node/api-report.md +++ b/plugins/permission-node/api-report.md @@ -5,22 +5,23 @@ ```ts import { AllOfCriteria } from '@backstage/plugin-permission-common'; import { AnyOfCriteria } from '@backstage/plugin-permission-common'; -import { AuthorizeRequestOptions } from '@backstage/plugin-permission-common'; +import { AuthorizePermissionRequest } from '@backstage/plugin-permission-common'; +import { AuthorizePermissionResponse } from '@backstage/plugin-permission-common'; import { BackstageIdentityResponse } from '@backstage/plugin-auth-node'; import { ConditionalPolicyDecision } from '@backstage/plugin-permission-common'; import { Config } from '@backstage/config'; import { DefinitivePolicyDecision } from '@backstage/plugin-permission-common'; -import { EvaluatePermissionRequest } from '@backstage/plugin-permission-common'; -import { EvaluatePermissionResponse } from '@backstage/plugin-permission-common'; +import { EvaluatorRequestOptions } from '@backstage/plugin-permission-common'; import express from 'express'; import { IdentifiedPermissionMessage } from '@backstage/plugin-permission-common'; import { NotCriteria } from '@backstage/plugin-permission-common'; import { Permission } from '@backstage/plugin-permission-common'; -import { PermissionAuthorizer } from '@backstage/plugin-permission-common'; import { PermissionCondition } from '@backstage/plugin-permission-common'; import { PermissionCriteria } from '@backstage/plugin-permission-common'; +import { PermissionEvaluator } from '@backstage/plugin-permission-common'; import { PluginEndpointDiscovery } from '@backstage/backend-common'; import { PolicyDecision } from '@backstage/plugin-permission-common'; +import { QueryPermissionRequest } from '@backstage/plugin-permission-common'; import { ResourcePermission } from '@backstage/plugin-permission-common'; import { TokenManager } from '@backstage/backend-common'; @@ -178,12 +179,12 @@ export type PolicyQuery = { }; // @public -export class ServerPermissionClient implements PermissionAuthorizer { +export class ServerPermissionClient implements PermissionEvaluator { // (undocumented) authorize( - requests: EvaluatePermissionRequest[], - options?: AuthorizeRequestOptions, - ): Promise; + requests: AuthorizePermissionRequest[], + options?: EvaluatorRequestOptions, + ): Promise; // (undocumented) static fromConfig( config: Config, @@ -192,5 +193,10 @@ export class ServerPermissionClient implements PermissionAuthorizer { tokenManager: TokenManager; }, ): ServerPermissionClient; + // (undocumented) + query( + queries: QueryPermissionRequest[], + options?: EvaluatorRequestOptions, + ): Promise; } ``` diff --git a/plugins/permission-node/src/policy/index.ts b/plugins/permission-node/src/policy/index.ts index 0b7e03cc7c..f151cfb4fb 100644 --- a/plugins/permission-node/src/policy/index.ts +++ b/plugins/permission-node/src/policy/index.ts @@ -14,4 +14,4 @@ * limitations under the License. */ -export type { PermissionPolicy } from './types'; +export type { PermissionPolicy, PolicyQuery } from './types'; diff --git a/plugins/permission-node/src/policy/types.ts b/plugins/permission-node/src/policy/types.ts index 992ad3a1cf..0f469cfbfe 100644 --- a/plugins/permission-node/src/policy/types.ts +++ b/plugins/permission-node/src/policy/types.ts @@ -15,11 +15,25 @@ */ import { + Permission, PolicyDecision, - PolicyQuery, } from '@backstage/plugin-permission-common'; import { BackstageIdentityResponse } from '@backstage/plugin-auth-node'; +/** + * A query to be evaluated by the {@link PermissionPolicy}. + * + * @remarks + * + * Unlike other parts of the permission API, the policy does not accept a resource ref. This keeps + * the policy decoupled from the resource loading and condition applying logic. + * + * @public + */ +export type PolicyQuery = { + permission: Permission; +}; + /** * A policy to evaluate authorization requests for any permissioned action performed in Backstage. * From ef2dd617e3ae4d31d40a417f02590d94b8c4b879 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Mon, 28 Mar 2022 10:17:26 +0200 Subject: [PATCH 05/24] Fix comments Signed-off-by: Vincenzo Scamporlino --- .../permission-common/src/PermissionClient.ts | 51 +++---------------- plugins/permission-common/src/types/api.ts | 1 + 2 files changed, 9 insertions(+), 43 deletions(-) diff --git a/plugins/permission-common/src/PermissionClient.ts b/plugins/permission-common/src/PermissionClient.ts index 9ca9bfe255..3f7b459cb5 100644 --- a/plugins/permission-common/src/PermissionClient.ts +++ b/plugins/permission-common/src/PermissionClient.ts @@ -21,12 +21,10 @@ import * as uuid from 'uuid'; import { z } from 'zod'; import { AuthorizeResult, - DefinitivePolicyDecision, PermissionMessageBatch, PermissionCriteria, PermissionCondition, PermissionEvaluator, - PolicyDecision, QueryPermissionRequest, AuthorizePermissionRequest, EvaluatorRequestOptions, @@ -59,15 +57,14 @@ const permissionCriteriaSchema: z.ZodSchema< .or(z.object({ not: permissionCriteriaSchema }).strict()), ); -const authorizeDecisionSchema: z.ZodSchema = z.object( - { +const authorizeDecisionSchema: z.ZodSchema = + z.object({ result: z .literal(AuthorizeResult.ALLOW) .or(z.literal(AuthorizeResult.DENY)), - }, -); + }); -const policyDecisionSchema: z.ZodSchema = z.union([ +const policyDecisionSchema: z.ZodSchema = z.union([ z.object({ result: z .literal(AuthorizeResult.ALLOW) @@ -119,49 +116,17 @@ export class PermissionClient implements PermissionEvaluator { } /** - * Request authorization from the permission-backend for the given set of - * permissions. - * - * @remarks - * - * Checks that a given Backstage user can perform a protected operation. When - * authorization is for a {@link ResourcePermission}s, a resourceRef - * corresponding to the resource should always be supplied along with the - * permission. The Backstage identity token should be included in the - * `options` if available. - * - * Permissions can be imported from plugins exposing them, such as - * `catalogEntityReadPermission`. - * - * For each query, the response will be either ALLOW or DENY. - * - * @public + * {@inheritdoc PermissionEvaluator.authorize} */ async authorize( requests: AuthorizePermissionRequest[], options?: EvaluatorRequestOptions, ): Promise { - // TODO(permissions): it would be great to provide some kind of typing guarantee that - // conditional responses will only ever be returned for requests containing a resourceType - // but no resourceRef. That way clients who aren't prepared to handle filtering according - // to conditions can be guaranteed that they won't unexpectedly get a CONDITIONAL response. - return this.makeRequest(requests, authorizeDecisionSchema, options); } /** - * Fetch the conditional authorization decisions for the given set of - * {@link ResourcePermission}s in order to apply the conditions to an upstream - * data source. - * - * @remarks - * - * For each query, the response will be either ALLOW, DENY, or CONDITIONAL. - * Conditional responses are intended only for backends which have access to - * the data source for permissioned resources, so that filters can be applied - * when loading collections of resources. - * - * @public + * {@inheritdoc PermissionEvaluator.query} */ async query( queries: QueryPermissionRequest[], @@ -179,7 +144,7 @@ export class PermissionClient implements PermissionEvaluator { return queries.map(_ => ({ result: AuthorizeResult.ALLOW as const })); } - const request = { + const request: PermissionMessageBatch = { items: queries.map(query => ({ id: uuid.v4(), ...query, @@ -209,7 +174,7 @@ export class PermissionClient implements PermissionEvaluator { const responsesById = parsedResponse.items.reduce((acc, r) => { acc[r.id] = r; return acc; - }, {} as Record); + }, {} as Record>); return request.items.map(query => responsesById[query.id]); } diff --git a/plugins/permission-common/src/types/api.ts b/plugins/permission-common/src/types/api.ts index 1bc9af5ad8..0e4cf782e1 100644 --- a/plugins/permission-common/src/types/api.ts +++ b/plugins/permission-common/src/types/api.ts @@ -247,6 +247,7 @@ export interface PermissionEvaluator { /** * Options for {@link PermissionEvaluator} requests. + * The Backstage identity token should be defined if available. * @public */ export type EvaluatorRequestOptions = { From 23646e51a51065e8773f5fe4b56bbe4e5f1ccbf9 Mon Sep 17 00:00:00 2001 From: Mike Lewis Date: Tue, 8 Mar 2022 13:16:16 +0000 Subject: [PATCH 06/24] catalog-backend: use new PermissionAuthorizer#policyDecision method Signed-off-by: Mike Lewis --- .changeset/cuddly-turtles-sleep.md | 5 +++++ .../service/AuthorizedEntitiesCatalog.test.ts | 21 ++++++++++--------- .../src/service/AuthorizedEntitiesCatalog.ts | 6 +++--- .../service/AuthorizedLocationService.test.ts | 1 + .../service/AuthorizedRefreshService.test.ts | 1 + 5 files changed, 21 insertions(+), 13 deletions(-) create mode 100644 .changeset/cuddly-turtles-sleep.md diff --git a/.changeset/cuddly-turtles-sleep.md b/.changeset/cuddly-turtles-sleep.md new file mode 100644 index 0000000000..bb1a925506 --- /dev/null +++ b/.changeset/cuddly-turtles-sleep.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-backend': patch +--- + +Use new `PermissionEvaluator#query` method when retrieving permission conditions. diff --git a/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.test.ts b/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.test.ts index b29e1121b3..16d3070c6d 100644 --- a/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.test.ts +++ b/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.test.ts @@ -30,6 +30,7 @@ describe('AuthorizedEntitiesCatalog', () => { }; const fakePermissionApi = { authorize: jest.fn(), + policyDecision: jest.fn(), }; const createCatalog = (...rules: CatalogPermissionRule[]) => @@ -45,7 +46,7 @@ describe('AuthorizedEntitiesCatalog', () => { describe('entities', () => { it('returns empty response on DENY', async () => { - fakePermissionApi.authorize.mockResolvedValue([ + fakePermissionApi.policyDecision.mockResolvedValue([ { result: AuthorizeResult.DENY }, ]); const catalog = createCatalog(); @@ -61,7 +62,7 @@ describe('AuthorizedEntitiesCatalog', () => { }); it('calls underlying catalog method with correct filter on CONDITIONAL', async () => { - fakePermissionApi.authorize.mockResolvedValue([ + fakePermissionApi.policyDecision.mockResolvedValue([ { result: AuthorizeResult.CONDITIONAL, conditions: { rule: 'IS_ENTITY_KIND', params: [['b']] }, @@ -78,7 +79,7 @@ describe('AuthorizedEntitiesCatalog', () => { }); it('calls underlying catalog method on ALLOW', async () => { - fakePermissionApi.authorize.mockResolvedValue([ + fakePermissionApi.policyDecision.mockResolvedValue([ { result: AuthorizeResult.ALLOW }, ]); const catalog = createCatalog(); @@ -98,7 +99,7 @@ describe('AuthorizedEntitiesCatalog', () => { { kind: 'component', namespace: 'default', name: 'my-component' }, ], }); - fakePermissionApi.authorize.mockResolvedValue([ + fakePermissionApi.policyDecision.mockResolvedValue([ { result: AuthorizeResult.DENY }, ]); const catalog = new AuthorizedEntitiesCatalog( @@ -113,7 +114,7 @@ describe('AuthorizedEntitiesCatalog', () => { }); it('throws error on CONDITIONAL authorization that evaluates to 0 entities', async () => { - fakePermissionApi.authorize.mockResolvedValue([ + fakePermissionApi.policyDecision.mockResolvedValue([ { result: AuthorizeResult.CONDITIONAL, conditions: { rule: 'IS_ENTITY_KIND', params: [['b']] }, @@ -132,7 +133,7 @@ describe('AuthorizedEntitiesCatalog', () => { }); it('calls underlying catalog method on CONDITIONAL authorization that evaluates to nonzero entities', async () => { - fakePermissionApi.authorize.mockResolvedValue([ + fakePermissionApi.policyDecision.mockResolvedValue([ { result: AuthorizeResult.CONDITIONAL, conditions: { rule: 'IS_ENTITY_KIND', params: [['b']] }, @@ -158,7 +159,7 @@ describe('AuthorizedEntitiesCatalog', () => { { kind: 'component', namespace: 'default', name: 'my-component' }, ], }); - fakePermissionApi.authorize.mockResolvedValue([ + fakePermissionApi.policyDecision.mockResolvedValue([ { result: AuthorizeResult.ALLOW }, ]); const catalog = new AuthorizedEntitiesCatalog( @@ -252,7 +253,7 @@ describe('AuthorizedEntitiesCatalog', () => { describe('facets', () => { it('returns empty response on DENY', async () => { - fakePermissionApi.authorize.mockResolvedValue([ + fakePermissionApi.policyDecision.mockResolvedValue([ { result: AuthorizeResult.DENY }, ]); const catalog = createCatalog(); @@ -268,7 +269,7 @@ describe('AuthorizedEntitiesCatalog', () => { }); it('calls underlying catalog method with correct filter on CONDITIONAL', async () => { - fakePermissionApi.authorize.mockResolvedValue([ + fakePermissionApi.policyDecision.mockResolvedValue([ { result: AuthorizeResult.CONDITIONAL, conditions: { rule: 'IS_ENTITY_KIND', params: [['b']] }, @@ -286,7 +287,7 @@ describe('AuthorizedEntitiesCatalog', () => { }); it('calls underlying catalog method on ALLOW', async () => { - fakePermissionApi.authorize.mockResolvedValue([ + fakePermissionApi.policyDecision.mockResolvedValue([ { result: AuthorizeResult.ALLOW }, ]); const catalog = createCatalog(); diff --git a/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.ts b/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.ts index 2f94e23303..fe1bb579f5 100644 --- a/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.ts +++ b/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.ts @@ -45,7 +45,7 @@ export class AuthorizedEntitiesCatalog implements EntitiesCatalog { async entities(request?: EntitiesRequest): Promise { const authorizeDecision = ( - await this.permissionApi.authorize( + await this.permissionApi.policyDecision( [{ permission: catalogEntityReadPermission }], { token: request?.authorizationToken }, ) @@ -78,7 +78,7 @@ export class AuthorizedEntitiesCatalog implements EntitiesCatalog { options?: { authorizationToken?: string }, ): Promise { const authorizeResponse = ( - await this.permissionApi.authorize( + await this.permissionApi.policyDecision( [{ permission: catalogEntityDeletePermission }], { token: options?.authorizationToken }, ) @@ -155,7 +155,7 @@ export class AuthorizedEntitiesCatalog implements EntitiesCatalog { async facets(request: EntityFacetsRequest): Promise { const authorizeDecision = ( - await this.permissionApi.authorize( + await this.permissionApi.policyDecision( [{ permission: catalogEntityReadPermission }], { token: request?.authorizationToken }, ) diff --git a/plugins/catalog-backend/src/service/AuthorizedLocationService.test.ts b/plugins/catalog-backend/src/service/AuthorizedLocationService.test.ts index b27ca92924..3db6bff208 100644 --- a/plugins/catalog-backend/src/service/AuthorizedLocationService.test.ts +++ b/plugins/catalog-backend/src/service/AuthorizedLocationService.test.ts @@ -27,6 +27,7 @@ describe('AuthorizedLocationService', () => { }; const fakePermissionApi = { authorize: jest.fn(), + policyDecision: jest.fn(), }; const mockAllow = () => { diff --git a/plugins/catalog-backend/src/service/AuthorizedRefreshService.test.ts b/plugins/catalog-backend/src/service/AuthorizedRefreshService.test.ts index 82bedec573..d1c4fe3fc1 100644 --- a/plugins/catalog-backend/src/service/AuthorizedRefreshService.test.ts +++ b/plugins/catalog-backend/src/service/AuthorizedRefreshService.test.ts @@ -25,6 +25,7 @@ describe('AuthorizedRefreshService', () => { }; const permissionApi = { authorize: jest.fn(), + policyDecision: jest.fn(), }; afterEach(() => { From b831d53972f7f3dfd88c442fb1d3164809c16ad9 Mon Sep 17 00:00:00 2001 From: Mike Lewis Date: Tue, 8 Mar 2022 13:18:24 +0000 Subject: [PATCH 07/24] jenkins-backend: update PermissionAuthorizer mock in jenkinsApi test suite Signed-off-by: Mike Lewis --- .changeset/spicy-dingos-serve.md | 5 +++++ plugins/jenkins-backend/src/service/jenkinsApi.test.ts | 1 + 2 files changed, 6 insertions(+) create mode 100644 .changeset/spicy-dingos-serve.md diff --git a/.changeset/spicy-dingos-serve.md b/.changeset/spicy-dingos-serve.md new file mode 100644 index 0000000000..282818b49d --- /dev/null +++ b/.changeset/spicy-dingos-serve.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-jenkins-backend': patch +--- + +Add `policyDecision` method to `PermissionClient` mock in jenkinsApi test suite. diff --git a/plugins/jenkins-backend/src/service/jenkinsApi.test.ts b/plugins/jenkins-backend/src/service/jenkinsApi.test.ts index a5453b5ebe..e11ed32c54 100644 --- a/plugins/jenkins-backend/src/service/jenkinsApi.test.ts +++ b/plugins/jenkins-backend/src/service/jenkinsApi.test.ts @@ -49,6 +49,7 @@ const fakePermissionApi = { result: AuthorizeResult.ALLOW, }, ]), + policyDecision: jest.fn(), }; describe('JenkinsApi', () => { From 3c8cfaaa800cc6ba84150b3021d7460baf4ee4d0 Mon Sep 17 00:00:00 2001 From: Mike Lewis Date: Tue, 8 Mar 2022 13:20:35 +0000 Subject: [PATCH 08/24] search-backend: use new policyDecision method in AuthorizedSearchEngine and test suites Signed-off-by: Mike Lewis --- .changeset/eight-cobras-think.md | 5 + .../service/AuthorizedSearchEngine.test.ts | 155 +++++++++++------- .../src/service/AuthorizedSearchEngine.ts | 32 +++- .../search-backend/src/service/router.test.ts | 3 + 4 files changed, 127 insertions(+), 68 deletions(-) create mode 100644 .changeset/eight-cobras-think.md diff --git a/.changeset/eight-cobras-think.md b/.changeset/eight-cobras-think.md new file mode 100644 index 0000000000..df661d1a0a --- /dev/null +++ b/.changeset/eight-cobras-think.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-search-backend': patch +--- + +Use new `PermissionAuthorizer#policyDecision` method in `AuthorizedSearchEngine` and test suites. diff --git a/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts b/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts index 47503312fb..8a7662c1f4 100644 --- a/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts +++ b/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts @@ -20,6 +20,8 @@ import { AuthorizeResult, createPermission, PermissionAuthorizer, + PolicyDecision, + PermissionEvaluator, } from '@backstage/plugin-permission-common'; import { DocumentTypeInfo, @@ -69,12 +71,15 @@ describe('AuthorizedSearchEngine', () => { query: mockedQuery, }; - const mockedAuthorize: jest.MockedFunction< - PermissionAuthorizer['authorize'] + const mockedAuthorize: jest.MockedFunction = + jest.fn(); + const mockedPermissionQuery: jest.MockedFunction< + PermissionEvaluator['query'] > = jest.fn(); - const permissionAuthorizer: PermissionAuthorizer = { + const permissionAuthorizer: PermissionEvaluator = { authorize: mockedAuthorize, + query: mockedPermissionQuery, }; const defaultTypes: Record = { @@ -117,7 +122,8 @@ describe('AuthorizedSearchEngine', () => { const options = { token: 'token' }; - const allowAll: PermissionAuthorizer['authorize'] = async queries => { + const allowAll: PermissionAuthorizer['authorize'] & + PermissionEvaluator['query'] = async queries => { return queries.map(() => ({ result: AuthorizeResult.ALLOW, })); @@ -126,11 +132,12 @@ describe('AuthorizedSearchEngine', () => { beforeEach(() => { mockedQuery.mockReset(); mockedAuthorize.mockClear(); + mockedPermissionQuery.mockClear(); }); it('should forward the parameters correctly', async () => { mockedQuery.mockImplementation(async () => ({ results })); - mockedAuthorize.mockImplementation(allowAll); + const filters = { just: 1, a: 2, filter: 3 }; await authorizedSearchEngine.query( { term: 'term', filters, types: ['one', 'two'] }, @@ -148,7 +155,8 @@ describe('AuthorizedSearchEngine', () => { it('should forward the default types if none are passed', async () => { mockedQuery.mockImplementation(async () => ({ results })); - mockedAuthorize.mockImplementation(allowAll); + mockedPermissionQuery.mockImplementation(allowAll); + await authorizedSearchEngine.query({ term: '' }, options); expect(mockedQuery).toHaveBeenCalledWith( { term: '', types: ['users', 'templates', 'services', 'groups'] }, @@ -158,17 +166,17 @@ describe('AuthorizedSearchEngine', () => { it('should return all the results if all queries are allowed', async () => { mockedQuery.mockImplementation(async () => ({ results })); - mockedAuthorize.mockImplementation(allowAll); + mockedPermissionQuery.mockImplementation(allowAll); await expect( authorizedSearchEngine.query({ term: '' }, options), ).resolves.toEqual({ results }); - expect(mockedAuthorize).toHaveBeenCalledTimes(1); + expect(mockedPermissionQuery).toHaveBeenCalledTimes(1); }); it('should batch authorized requests', async () => { mockedQuery.mockImplementation(async () => ({ results })); - mockedAuthorize.mockImplementation(allowAll); + mockedPermissionQuery.mockImplementation(allowAll); await authorizedSearchEngine.query( { term: '', types: [typeUsers, typeTemplates] }, @@ -178,8 +186,8 @@ describe('AuthorizedSearchEngine', () => { { term: '', types: ['users', 'templates'] }, { token: 'token' }, ); - expect(mockedAuthorize).toHaveBeenCalledTimes(1); - expect(mockedAuthorize).toHaveBeenLastCalledWith( + expect(mockedPermissionQuery).toHaveBeenCalledTimes(1); + expect(mockedPermissionQuery).toHaveBeenLastCalledWith( [ { permission: defaultTypes[typeUsers].visibilityPermission }, { permission: defaultTypes[typeTemplates].visibilityPermission }, @@ -190,7 +198,7 @@ describe('AuthorizedSearchEngine', () => { it('should skip sending request for types that are not allowed', async () => { mockedQuery.mockImplementation(async () => ({ results })); - mockedAuthorize.mockImplementation(async queries => { + mockedPermissionQuery.mockImplementation(async queries => { return queries.map(query => { if ( query.permission.name === @@ -213,7 +221,7 @@ describe('AuthorizedSearchEngine', () => { { token: 'token' }, ); - expect(mockedAuthorize).toHaveBeenCalledTimes(1); + expect(mockedPermissionQuery).toHaveBeenCalledTimes(1); }); it('should perform result-by-result filtering', async () => { @@ -226,21 +234,12 @@ describe('AuthorizedSearchEngine', () => { results: resultsWithAuth, })); - const userToBeReturned = 8; - - mockedAuthorize.mockImplementation(async queries => + mockedPermissionQuery.mockImplementation(async queries => queries.map(query => { if ( query.permission.name === defaultTypes.users.visibilityPermission?.name ) { - if (query.resourceRef) { - return { - result: query.resourceRef.endsWith(userToBeReturned.toString()) - ? AuthorizeResult.ALLOW - : AuthorizeResult.DENY, - }; - } return { result: AuthorizeResult.CONDITIONAL, } as EvaluatePermissionResponse; @@ -252,9 +251,20 @@ describe('AuthorizedSearchEngine', () => { }), ); + mockedAuthorize.mockImplementation(async queries => + queries.map(query => { + return { + result: + query.resourceRef! === `users_doc_8` + ? AuthorizeResult.ALLOW + : AuthorizeResult.DENY, + }; + }), + ); + await expect( authorizedSearchEngine.query({ term: '' }, options), - ).resolves.toEqual({ results: [usersWithAuth[userToBeReturned]] }); + ).resolves.toEqual({ results: [usersWithAuth[8]] }); expect(mockedQuery).toHaveBeenCalledWith( { term: '', types: ['users'] }, @@ -284,27 +294,27 @@ describe('AuthorizedSearchEngine', () => { results: searchResults, })); - mockedAuthorize.mockImplementation(async queries => - queries.map(query => { - if (query.resourceRef) { - return { - result: AuthorizeResult.ALLOW, - }; - } + mockedPermissionQuery.mockImplementation(async queries => + queries.map( + _ => + ({ + result: AuthorizeResult.CONDITIONAL, + } as EvaluatePermissionResponse), + ), + ); - return { - result: AuthorizeResult.CONDITIONAL, - } as EvaluatePermissionResponse; - }), + mockedAuthorize.mockImplementation(async queries => + queries.map(_ => ({ + result: AuthorizeResult.ALLOW, + })), ); await expect( authorizedSearchEngine.query({ term: '', types: ['templates'] }, options), ).resolves.toEqual({ results: searchResults }); - expect(mockedAuthorize).toHaveBeenCalledTimes(2); - expect(mockedAuthorize).toHaveBeenNthCalledWith( - 1, + expect(mockedPermissionQuery).toHaveBeenCalledTimes(1); + expect(mockedPermissionQuery).toHaveBeenCalledWith( [ { permission: expect.objectContaining({ @@ -314,8 +324,8 @@ describe('AuthorizedSearchEngine', () => { ], { token: 'token' }, ); - expect(mockedAuthorize).toHaveBeenNthCalledWith( - 2, + expect(mockedAuthorize).toHaveBeenCalledTimes(1); + expect(mockedAuthorize).toHaveBeenCalledWith( [ { permission: expect.objectContaining({ @@ -329,7 +339,16 @@ describe('AuthorizedSearchEngine', () => { }); it('should perform search until the target number of results is reached', async () => { - mockedAuthorize.mockImplementation(async queries => + mockedPermissionQuery.mockImplementation(async queries => + queries.map( + _ => + ({ + result: AuthorizeResult.CONDITIONAL, + } as PolicyDecision), + ), + ); + + mockedPermissionQuery.mockImplementation(async queries => queries.map(query => { if (query.resourceRef) { return { @@ -342,6 +361,12 @@ describe('AuthorizedSearchEngine', () => { }), ); + mockedAuthorize.mockImplementation(async queries => + queries.map(_ => ({ + result: AuthorizeResult.ALLOW, + })), + ); + const usersWithAuth = generateSampleResults(typeUsers, true); const templatesWithAuth = generateSampleResults(typeTemplates, true); const servicesWithAuth = generateSampleResults(typeServices, true); @@ -405,20 +430,22 @@ describe('AuthorizedSearchEngine', () => { }); it('should perform search until the target number of results is reached, excluding unauthorized results', async () => { + mockedPermissionQuery.mockImplementation(async queries => + queries.map( + _ => + ({ + result: AuthorizeResult.CONDITIONAL, + } as PolicyDecision), + ), + ); + mockedAuthorize.mockImplementation(async queries => - queries.map(query => { - if (query.resourceRef) { - return { - result: - query.permission.name === 'search.services.read' - ? AuthorizeResult.DENY - : AuthorizeResult.ALLOW, - }; - } - return { - result: AuthorizeResult.CONDITIONAL, - } as EvaluatePermissionResponse; - }), + queries.map(query => ({ + result: + query.permission.name === 'search.services.read' + ? AuthorizeResult.DENY + : AuthorizeResult.ALLOW, + })), ); const usersWithAuth = generateSampleResults(typeUsers, true); @@ -494,15 +521,19 @@ describe('AuthorizedSearchEngine', () => { }); it('should discard results until the target cursor is reached', async () => { + mockedPermissionQuery.mockImplementation(async queries => + queries.map( + _ => + ({ + result: AuthorizeResult.CONDITIONAL, + } as PolicyDecision), + ), + ); + mockedAuthorize.mockImplementation(async queries => - queries.map(query => { - if (query.resourceRef) { - return { result: AuthorizeResult.ALLOW }; - } - return { - result: AuthorizeResult.CONDITIONAL, - } as EvaluatePermissionResponse; - }), + queries.map(_ => ({ + result: AuthorizeResult.ALLOW, + })), ); const usersWithAuth = generateSampleResults(typeUsers, true); diff --git a/plugins/search-backend/src/service/AuthorizedSearchEngine.ts b/plugins/search-backend/src/service/AuthorizedSearchEngine.ts index a529bf04c6..2397734089 100644 --- a/plugins/search-backend/src/service/AuthorizedSearchEngine.ts +++ b/plugins/search-backend/src/service/AuthorizedSearchEngine.ts @@ -22,7 +22,9 @@ import { EvaluatePermissionRequest, AuthorizeResult, isResourcePermission, - PermissionAuthorizer, + PermissionEvaluator, + AuthorizePermissionRequest, + QueryPermissionRequest, } from '@backstage/plugin-permission-common'; import { DocumentTypeInfo, @@ -67,7 +69,7 @@ export class AuthorizedSearchEngine implements SearchEngine { constructor( private readonly searchEngine: SearchEngine, private readonly types: Record, - private readonly permissions: PermissionAuthorizer, + private readonly permissions: PermissionEvaluator, config: Config, ) { this.queryLatencyBudgetMs = @@ -89,8 +91,16 @@ export class AuthorizedSearchEngine implements SearchEngine { ): Promise { const queryStartTime = Date.now(); + const conditionFetcher = new DataLoader( + (requests: readonly QueryPermissionRequest[]) => + this.permissions.query(requests.slice(), options), + { + cacheKeyFn: ({ permission: { name } }) => name, + }, + ); + const authorizer = new DataLoader( - (requests: readonly EvaluatePermissionRequest[]) => + (requests: readonly AuthorizePermissionRequest[]) => this.permissions.authorize(requests.slice(), options), { // Serialize the permission name and resourceRef as @@ -100,6 +110,7 @@ export class AuthorizedSearchEngine implements SearchEngine { qs.stringify({ name, resourceRef }), }, ); + const requestedTypes = query.types || Object.keys(this.types); const typeDecisions = zipObject( @@ -108,9 +119,18 @@ export class AuthorizedSearchEngine implements SearchEngine { requestedTypes.map(type => { const permission = this.types[type]?.visibilityPermission; - return permission - ? authorizer.load({ permission }) - : { result: AuthorizeResult.ALLOW as const }; + // No permission configured for this document type - always allow. + if (!permission) { + return { result: AuthorizeResult.ALLOW as const }; + } + + // Resource permission supplied, so we need to check for conditional decisions. + if (isResourcePermission(permission)) { + return conditionFetcher.load({ permission }); + } + + // Non-resource permission supplied - we can perform a standard authorization. + return authorizer.load({ permission }); }), ), ); diff --git a/plugins/search-backend/src/service/router.test.ts b/plugins/search-backend/src/service/router.test.ts index 2efab91256..e93f0dd671 100644 --- a/plugins/search-backend/src/service/router.test.ts +++ b/plugins/search-backend/src/service/router.test.ts @@ -30,6 +30,9 @@ const mockPermissionAuthorizer: PermissionAuthorizer = { authorize: () => { throw new Error('Not implemented'); }, + policyDecision: () => { + throw new Error('Not implemented'); + }, }; describe('createRouter', () => { From dc8037213cd1291ab4b0f197409b0470d2f08825 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Mon, 28 Mar 2022 12:16:27 +0200 Subject: [PATCH 09/24] Use PermissionEvaluator Signed-off-by: Vincenzo Scamporlino --- plugins/catalog-backend/api-report.md | 4 ++-- .../service/AuthorizedEntitiesCatalog.test.ts | 22 +++++++++---------- .../src/service/AuthorizedEntitiesCatalog.ts | 10 ++++----- .../src/service/CatalogBuilder.ts | 4 ++-- .../src/PermissionClient.test.ts | 2 ++ plugins/search-backend/api-report.md | 4 ++-- .../search-backend/src/service/router.test.ts | 6 ++--- plugins/search-backend/src/service/router.ts | 4 ++-- 8 files changed, 29 insertions(+), 27 deletions(-) diff --git a/plugins/catalog-backend/api-report.md b/plugins/catalog-backend/api-report.md index d44e744bb6..7efdfec424 100644 --- a/plugins/catalog-backend/api-report.md +++ b/plugins/catalog-backend/api-report.md @@ -19,9 +19,9 @@ import { JsonValue } from '@backstage/types'; import { LocationEntityV1alpha1 } from '@backstage/catalog-model'; import { Logger } from 'winston'; import { Permission } from '@backstage/plugin-permission-common'; -import { PermissionAuthorizer } from '@backstage/plugin-permission-common'; import { PermissionCondition } from '@backstage/plugin-permission-common'; import { PermissionCriteria } from '@backstage/plugin-permission-common'; +import { PermissionEvaluator } from '@backstage/plugin-permission-common'; import { PermissionRule } from '@backstage/plugin-permission-node'; import { PluginDatabaseManager } from '@backstage/backend-common'; import { PluginEndpointDiscovery } from '@backstage/backend-common'; @@ -181,7 +181,7 @@ export type CatalogEnvironment = { database: PluginDatabaseManager; config: Config; reader: UrlReader; - permissions: PermissionAuthorizer; + permissions: PermissionEvaluator; }; // @alpha diff --git a/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.test.ts b/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.test.ts index 16d3070c6d..dbdcde11c4 100644 --- a/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.test.ts +++ b/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.test.ts @@ -30,7 +30,7 @@ describe('AuthorizedEntitiesCatalog', () => { }; const fakePermissionApi = { authorize: jest.fn(), - policyDecision: jest.fn(), + query: jest.fn(), }; const createCatalog = (...rules: CatalogPermissionRule[]) => @@ -46,7 +46,7 @@ describe('AuthorizedEntitiesCatalog', () => { describe('entities', () => { it('returns empty response on DENY', async () => { - fakePermissionApi.policyDecision.mockResolvedValue([ + fakePermissionApi.query.mockResolvedValue([ { result: AuthorizeResult.DENY }, ]); const catalog = createCatalog(); @@ -62,7 +62,7 @@ describe('AuthorizedEntitiesCatalog', () => { }); it('calls underlying catalog method with correct filter on CONDITIONAL', async () => { - fakePermissionApi.policyDecision.mockResolvedValue([ + fakePermissionApi.query.mockResolvedValue([ { result: AuthorizeResult.CONDITIONAL, conditions: { rule: 'IS_ENTITY_KIND', params: [['b']] }, @@ -79,7 +79,7 @@ describe('AuthorizedEntitiesCatalog', () => { }); it('calls underlying catalog method on ALLOW', async () => { - fakePermissionApi.policyDecision.mockResolvedValue([ + fakePermissionApi.query.mockResolvedValue([ { result: AuthorizeResult.ALLOW }, ]); const catalog = createCatalog(); @@ -99,7 +99,7 @@ describe('AuthorizedEntitiesCatalog', () => { { kind: 'component', namespace: 'default', name: 'my-component' }, ], }); - fakePermissionApi.policyDecision.mockResolvedValue([ + fakePermissionApi.query.mockResolvedValue([ { result: AuthorizeResult.DENY }, ]); const catalog = new AuthorizedEntitiesCatalog( @@ -114,7 +114,7 @@ describe('AuthorizedEntitiesCatalog', () => { }); it('throws error on CONDITIONAL authorization that evaluates to 0 entities', async () => { - fakePermissionApi.policyDecision.mockResolvedValue([ + fakePermissionApi.query.mockResolvedValue([ { result: AuthorizeResult.CONDITIONAL, conditions: { rule: 'IS_ENTITY_KIND', params: [['b']] }, @@ -133,7 +133,7 @@ describe('AuthorizedEntitiesCatalog', () => { }); it('calls underlying catalog method on CONDITIONAL authorization that evaluates to nonzero entities', async () => { - fakePermissionApi.policyDecision.mockResolvedValue([ + fakePermissionApi.query.mockResolvedValue([ { result: AuthorizeResult.CONDITIONAL, conditions: { rule: 'IS_ENTITY_KIND', params: [['b']] }, @@ -159,7 +159,7 @@ describe('AuthorizedEntitiesCatalog', () => { { kind: 'component', namespace: 'default', name: 'my-component' }, ], }); - fakePermissionApi.policyDecision.mockResolvedValue([ + fakePermissionApi.query.mockResolvedValue([ { result: AuthorizeResult.ALLOW }, ]); const catalog = new AuthorizedEntitiesCatalog( @@ -253,7 +253,7 @@ describe('AuthorizedEntitiesCatalog', () => { describe('facets', () => { it('returns empty response on DENY', async () => { - fakePermissionApi.policyDecision.mockResolvedValue([ + fakePermissionApi.query.mockResolvedValue([ { result: AuthorizeResult.DENY }, ]); const catalog = createCatalog(); @@ -269,7 +269,7 @@ describe('AuthorizedEntitiesCatalog', () => { }); it('calls underlying catalog method with correct filter on CONDITIONAL', async () => { - fakePermissionApi.policyDecision.mockResolvedValue([ + fakePermissionApi.query.mockResolvedValue([ { result: AuthorizeResult.CONDITIONAL, conditions: { rule: 'IS_ENTITY_KIND', params: [['b']] }, @@ -287,7 +287,7 @@ describe('AuthorizedEntitiesCatalog', () => { }); it('calls underlying catalog method on ALLOW', async () => { - fakePermissionApi.policyDecision.mockResolvedValue([ + fakePermissionApi.query.mockResolvedValue([ { result: AuthorizeResult.ALLOW }, ]); const catalog = createCatalog(); diff --git a/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.ts b/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.ts index fe1bb579f5..fcd05f95d8 100644 --- a/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.ts +++ b/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.ts @@ -22,7 +22,7 @@ import { import { Entity, stringifyEntityRef } from '@backstage/catalog-model'; import { AuthorizeResult, - PermissionAuthorizer, + PermissionEvaluator, } from '@backstage/plugin-permission-common'; import { ConditionTransformer } from '@backstage/plugin-permission-node'; import { @@ -39,13 +39,13 @@ import { basicEntityFilter } from './request/basicEntityFilter'; export class AuthorizedEntitiesCatalog implements EntitiesCatalog { constructor( private readonly entitiesCatalog: EntitiesCatalog, - private readonly permissionApi: PermissionAuthorizer, + private readonly permissionApi: PermissionEvaluator, private readonly transformConditions: ConditionTransformer, ) {} async entities(request?: EntitiesRequest): Promise { const authorizeDecision = ( - await this.permissionApi.policyDecision( + await this.permissionApi.query( [{ permission: catalogEntityReadPermission }], { token: request?.authorizationToken }, ) @@ -78,7 +78,7 @@ export class AuthorizedEntitiesCatalog implements EntitiesCatalog { options?: { authorizationToken?: string }, ): Promise { const authorizeResponse = ( - await this.permissionApi.policyDecision( + await this.permissionApi.query( [{ permission: catalogEntityDeletePermission }], { token: options?.authorizationToken }, ) @@ -155,7 +155,7 @@ export class AuthorizedEntitiesCatalog implements EntitiesCatalog { async facets(request: EntityFacetsRequest): Promise { const authorizeDecision = ( - await this.permissionApi.policyDecision( + await this.permissionApi.query( [{ permission: catalogEntityReadPermission }], { token: request?.authorizationToken }, ) diff --git a/plugins/catalog-backend/src/service/CatalogBuilder.ts b/plugins/catalog-backend/src/service/CatalogBuilder.ts index 075fcd6a5d..e9135393ba 100644 --- a/plugins/catalog-backend/src/service/CatalogBuilder.ts +++ b/plugins/catalog-backend/src/service/CatalogBuilder.ts @@ -79,7 +79,7 @@ import { CatalogPermissionRule, permissionRules as catalogPermissionRules, } from '../permissions/rules'; -import { PermissionAuthorizer } from '@backstage/plugin-permission-common'; +import { PermissionEvaluator } from '@backstage/plugin-permission-common'; import { createConditionTransformer, createPermissionIntegrationRouter, @@ -95,7 +95,7 @@ export type CatalogEnvironment = { database: PluginDatabaseManager; config: Config; reader: UrlReader; - permissions: PermissionAuthorizer; + permissions: PermissionEvaluator; }; /** diff --git a/plugins/permission-common/src/PermissionClient.test.ts b/plugins/permission-common/src/PermissionClient.test.ts index ec038e7f16..8da30185b7 100644 --- a/plugins/permission-common/src/PermissionClient.test.ts +++ b/plugins/permission-common/src/PermissionClient.test.ts @@ -225,6 +225,7 @@ describe('PermissionClient', () => { resourceType: 'test-resource', result: AuthorizeResult.CONDITIONAL, conditions: { + resourceType: 'test-resource', rule: 'FOO', params: ['bar'], }, @@ -271,6 +272,7 @@ describe('PermissionClient', () => { result: AuthorizeResult.CONDITIONAL, conditions: { rule: 'FOO', + resourceType: 'test-resource', params: ['bar'], }, }), diff --git a/plugins/search-backend/api-report.md b/plugins/search-backend/api-report.md index 3c7f9c2754..dc14d2f15b 100644 --- a/plugins/search-backend/api-report.md +++ b/plugins/search-backend/api-report.md @@ -7,7 +7,7 @@ import { Config } from '@backstage/config'; import { DocumentTypeInfo } from '@backstage/plugin-search-common'; import express from 'express'; import { Logger } from 'winston'; -import { PermissionAuthorizer } from '@backstage/plugin-permission-common'; +import { PermissionEvaluator } from '@backstage/plugin-permission-common'; import { SearchEngine } from '@backstage/plugin-search-backend-node'; // Warning: (ae-missing-release-tag) "createRouter" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) @@ -21,7 +21,7 @@ export function createRouter(options: RouterOptions): Promise; export type RouterOptions = { engine: SearchEngine; types: Record; - permissions: PermissionAuthorizer; + permissions: PermissionEvaluator; config: Config; logger: Logger; }; diff --git a/plugins/search-backend/src/service/router.test.ts b/plugins/search-backend/src/service/router.test.ts index e93f0dd671..85773fb3b8 100644 --- a/plugins/search-backend/src/service/router.test.ts +++ b/plugins/search-backend/src/service/router.test.ts @@ -16,7 +16,7 @@ import { getVoidLogger } from '@backstage/backend-common'; import { ConfigReader } from '@backstage/config'; -import { PermissionAuthorizer } from '@backstage/plugin-permission-common'; +import { PermissionEvaluator } from '@backstage/plugin-permission-common'; import { IndexBuilder, SearchEngine, @@ -26,11 +26,11 @@ import request from 'supertest'; import { createRouter } from './router'; -const mockPermissionAuthorizer: PermissionAuthorizer = { +const mockPermissionAuthorizer: PermissionEvaluator = { authorize: () => { throw new Error('Not implemented'); }, - policyDecision: () => { + query: () => { throw new Error('Not implemented'); }, }; diff --git a/plugins/search-backend/src/service/router.ts b/plugins/search-backend/src/service/router.ts index ff91465cf4..6867c382df 100644 --- a/plugins/search-backend/src/service/router.ts +++ b/plugins/search-backend/src/service/router.ts @@ -23,7 +23,7 @@ import { InputError } from '@backstage/errors'; import { Config } from '@backstage/config'; import { JsonObject, JsonValue } from '@backstage/types'; import { getBearerTokenFromAuthorizationHeader } from '@backstage/plugin-auth-node'; -import { PermissionAuthorizer } from '@backstage/plugin-permission-common'; +import { PermissionEvaluator } from '@backstage/plugin-permission-common'; import { DocumentTypeInfo, IndexableResultSet, @@ -50,7 +50,7 @@ const jsonObjectSchema: z.ZodSchema = z.lazy(() => { export type RouterOptions = { engine: SearchEngine; types: Record; - permissions: PermissionAuthorizer; + permissions: PermissionEvaluator; config: Config; logger: Logger; }; From 322b69e46a7bedff7e60f1b103dac5f1120feea3 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Mon, 28 Mar 2022 16:01:28 +0200 Subject: [PATCH 10/24] Add changesets Signed-off-by: Vincenzo Scamporlino --- .changeset/eight-cobras-think.md | 2 +- .changeset/four-dolphins-report.md | 5 +++++ .changeset/soft-rice-remember.md | 5 +++++ .changeset/spicy-dingos-serve.md | 5 ----- 4 files changed, 11 insertions(+), 6 deletions(-) create mode 100644 .changeset/four-dolphins-report.md create mode 100644 .changeset/soft-rice-remember.md delete mode 100644 .changeset/spicy-dingos-serve.md diff --git a/.changeset/eight-cobras-think.md b/.changeset/eight-cobras-think.md index df661d1a0a..854e4fcec8 100644 --- a/.changeset/eight-cobras-think.md +++ b/.changeset/eight-cobras-think.md @@ -2,4 +2,4 @@ '@backstage/plugin-search-backend': patch --- -Use new `PermissionAuthorizer#policyDecision` method in `AuthorizedSearchEngine` and test suites. +Fix typing when invoking `PermissionClient#authorize` diff --git a/.changeset/four-dolphins-report.md b/.changeset/four-dolphins-report.md new file mode 100644 index 0000000000..52bbbac1e8 --- /dev/null +++ b/.changeset/four-dolphins-report.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-permission-node': patch +--- + +Use new `PermissionEvaluator#query` method in `ServerPermissionClient` and test suites. diff --git a/.changeset/soft-rice-remember.md b/.changeset/soft-rice-remember.md new file mode 100644 index 0000000000..27e4c0a35d --- /dev/null +++ b/.changeset/soft-rice-remember.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-permission-react': patch +--- + +Fix typing when invoking `PermissionClient#authorize` diff --git a/.changeset/spicy-dingos-serve.md b/.changeset/spicy-dingos-serve.md deleted file mode 100644 index 282818b49d..0000000000 --- a/.changeset/spicy-dingos-serve.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -'@backstage/plugin-jenkins-backend': patch ---- - -Add `policyDecision` method to `PermissionClient` mock in jenkinsApi test suite. From 06d32493e65f4c329544486f5f924f5f25ffbf27 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Mon, 28 Mar 2022 16:29:24 +0200 Subject: [PATCH 11/24] Fix integration tests Signed-off-by: Vincenzo Scamporlino --- .../templates/default-app/packages/backend/src/types.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/create-app/templates/default-app/packages/backend/src/types.ts b/packages/create-app/templates/default-app/packages/backend/src/types.ts index 0862b0e874..7984b7361f 100644 --- a/packages/create-app/templates/default-app/packages/backend/src/types.ts +++ b/packages/create-app/templates/default-app/packages/backend/src/types.ts @@ -8,7 +8,7 @@ import { UrlReader, } from '@backstage/backend-common'; import { PluginTaskScheduler } from '@backstage/backend-tasks'; -import { PermissionAuthorizer } from '@backstage/plugin-permission-common'; +import { ServerPermissionClient } from '@backstage/plugin-permission-node'; export type PluginEnvironment = { logger: Logger; @@ -19,5 +19,5 @@ export type PluginEnvironment = { discovery: PluginEndpointDiscovery; tokenManager: TokenManager; scheduler: PluginTaskScheduler; - permissions: PermissionAuthorizer; + permissions: ServerPermissionClient; }; From 1882dbda2b1fe792e8589443c1a65283edcc2d65 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Mon, 28 Mar 2022 22:43:52 +0200 Subject: [PATCH 12/24] Add create-app changeset Signed-off-by: Vincenzo Scamporlino --- .changeset/rare-parents-pretend.md | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 .changeset/rare-parents-pretend.md diff --git a/.changeset/rare-parents-pretend.md b/.changeset/rare-parents-pretend.md new file mode 100644 index 0000000000..b483666e66 --- /dev/null +++ b/.changeset/rare-parents-pretend.md @@ -0,0 +1,21 @@ +--- +'@backstage/create-app': patch +--- + +Use `ServerPermissionClient` instead of `PermissionAuthorizer`. + +Apply the following to `packages/backend/src/types.ts`: + +```diff +- import { PermissionAuthorizer } from '@backstage/plugin-permission-common'; ++ import { ServerPermissionClient } from '@backstage/plugin-permission-node'; + + export type PluginEnvironment = { + ... + discovery: PluginEndpointDiscovery; + tokenManager: TokenManager; + scheduler: PluginTaskScheduler; +- permissions: PermissionAuthorizer; ++ permissions: ServerPermissionClient; + }; +``` From e7f28d8152ca726841f5045c813d33524f757da7 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Mon, 28 Mar 2022 22:58:43 +0200 Subject: [PATCH 13/24] IdentityPermissionApi: strict typing Signed-off-by: Vincenzo Scamporlino --- .changeset/soft-rice-remember.md | 2 +- plugins/permission-react/api-report.md | 6 +++-- .../src/apis/IdentityPermissionApi.ts | 24 ++++--------------- 3 files changed, 10 insertions(+), 22 deletions(-) diff --git a/.changeset/soft-rice-remember.md b/.changeset/soft-rice-remember.md index 27e4c0a35d..bcb2860d18 100644 --- a/.changeset/soft-rice-remember.md +++ b/.changeset/soft-rice-remember.md @@ -2,4 +2,4 @@ '@backstage/plugin-permission-react': patch --- -Fix typing when invoking `PermissionClient#authorize` +Make `IdentityPermissionApi#authorize` typing more strict, using `AuthorizePermissionRequest` and `AuthorizePermissionResponse`. diff --git a/plugins/permission-react/api-report.md b/plugins/permission-react/api-report.md index 17b29a463a..12f1300c70 100644 --- a/plugins/permission-react/api-report.md +++ b/plugins/permission-react/api-report.md @@ -4,6 +4,8 @@ ```ts import { ApiRef } from '@backstage/core-plugin-api'; +import { AuthorizePermissionRequest } from '@backstage/plugin-permission-common'; +import { AuthorizePermissionResponse } from '@backstage/plugin-permission-common'; import { ComponentProps } from 'react'; import { Config } from '@backstage/config'; import { DiscoveryApi } from '@backstage/core-plugin-api'; @@ -26,8 +28,8 @@ export type AsyncPermissionResult = { export class IdentityPermissionApi implements PermissionApi { // (undocumented) authorize( - request: EvaluatePermissionRequest, - ): Promise; + request: AuthorizePermissionRequest, + ): Promise; // (undocumented) static create(options: { config: Config; diff --git a/plugins/permission-react/src/apis/IdentityPermissionApi.ts b/plugins/permission-react/src/apis/IdentityPermissionApi.ts index 0bb46f5587..1fe295c9f2 100644 --- a/plugins/permission-react/src/apis/IdentityPermissionApi.ts +++ b/plugins/permission-react/src/apis/IdentityPermissionApi.ts @@ -17,9 +17,8 @@ import { DiscoveryApi, IdentityApi } from '@backstage/core-plugin-api'; import { PermissionApi } from './PermissionApi'; import { - EvaluatePermissionRequest, - EvaluatePermissionResponse, - isResourcePermission, + AuthorizePermissionRequest, + AuthorizePermissionResponse, PermissionClient, } from '@backstage/plugin-permission-common'; import { Config } from '@backstage/config'; @@ -46,23 +45,10 @@ export class IdentityPermissionApi implements PermissionApi { } async authorize( - request: EvaluatePermissionRequest, - ): Promise { - const { permission, resourceRef } = request; - if (isResourcePermission(permission)) { - if (!resourceRef) { - throw new Error( - 'A resourceRef should be provided when a ResourcePermission is used.', - ); - } - const response = await this.permissionClient.authorize( - [{ permission, resourceRef }], - await this.identityApi.getCredentials(), - ); - return response[0]; - } + request: AuthorizePermissionRequest, + ): Promise { const response = await this.permissionClient.authorize( - [{ permission }], + [request], await this.identityApi.getCredentials(), ); return response[0]; From a119dfbbd5c422773e6efce546032baf4b95e614 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Wed, 30 Mar 2022 14:58:51 +0200 Subject: [PATCH 14/24] Apply suggestions from code review Co-authored-by: Joe Porpeglia Signed-off-by: Vincenzo Scamporlino --- .changeset/soft-rice-remember.md | 2 +- .../service/AuthorizedLocationService.test.ts | 2 +- .../service/AuthorizedRefreshService.test.ts | 2 +- .../src/service/jenkinsApi.test.ts | 2 +- .../permission-common/src/PermissionClient.ts | 37 +++++++++++-------- 5 files changed, 25 insertions(+), 20 deletions(-) diff --git a/.changeset/soft-rice-remember.md b/.changeset/soft-rice-remember.md index bcb2860d18..bf207a0dd5 100644 --- a/.changeset/soft-rice-remember.md +++ b/.changeset/soft-rice-remember.md @@ -2,4 +2,4 @@ '@backstage/plugin-permission-react': patch --- -Make `IdentityPermissionApi#authorize` typing more strict, using `AuthorizePermissionRequest` and `AuthorizePermissionResponse`. +**BREAKING:** Make `IdentityPermissionApi#authorize` typing more strict, using `AuthorizePermissionRequest` and `AuthorizePermissionResponse`. diff --git a/plugins/catalog-backend/src/service/AuthorizedLocationService.test.ts b/plugins/catalog-backend/src/service/AuthorizedLocationService.test.ts index 3db6bff208..151ba1c3e8 100644 --- a/plugins/catalog-backend/src/service/AuthorizedLocationService.test.ts +++ b/plugins/catalog-backend/src/service/AuthorizedLocationService.test.ts @@ -27,7 +27,7 @@ describe('AuthorizedLocationService', () => { }; const fakePermissionApi = { authorize: jest.fn(), - policyDecision: jest.fn(), + query: jest.fn(), }; const mockAllow = () => { diff --git a/plugins/catalog-backend/src/service/AuthorizedRefreshService.test.ts b/plugins/catalog-backend/src/service/AuthorizedRefreshService.test.ts index d1c4fe3fc1..5d0a192470 100644 --- a/plugins/catalog-backend/src/service/AuthorizedRefreshService.test.ts +++ b/plugins/catalog-backend/src/service/AuthorizedRefreshService.test.ts @@ -25,7 +25,7 @@ describe('AuthorizedRefreshService', () => { }; const permissionApi = { authorize: jest.fn(), - policyDecision: jest.fn(), + query: jest.fn(), }; afterEach(() => { diff --git a/plugins/jenkins-backend/src/service/jenkinsApi.test.ts b/plugins/jenkins-backend/src/service/jenkinsApi.test.ts index e11ed32c54..7c4214f714 100644 --- a/plugins/jenkins-backend/src/service/jenkinsApi.test.ts +++ b/plugins/jenkins-backend/src/service/jenkinsApi.test.ts @@ -49,7 +49,7 @@ const fakePermissionApi = { result: AuthorizeResult.ALLOW, }, ]), - policyDecision: jest.fn(), + query: jest.fn(), }; describe('JenkinsApi', () => { diff --git a/plugins/permission-common/src/PermissionClient.ts b/plugins/permission-common/src/PermissionClient.ts index 3f7b459cb5..07d60a8e65 100644 --- a/plugins/permission-common/src/PermissionClient.ts +++ b/plugins/permission-common/src/PermissionClient.ts @@ -57,26 +57,27 @@ const permissionCriteriaSchema: z.ZodSchema< .or(z.object({ not: permissionCriteriaSchema }).strict()), ); -const authorizeDecisionSchema: z.ZodSchema = +const authorizePermissionResponseSchema: z.ZodSchema = z.object({ result: z .literal(AuthorizeResult.ALLOW) .or(z.literal(AuthorizeResult.DENY)), }); -const policyDecisionSchema: z.ZodSchema = z.union([ - z.object({ - result: z - .literal(AuthorizeResult.ALLOW) - .or(z.literal(AuthorizeResult.DENY)), - }), - z.object({ - result: z.literal(AuthorizeResult.CONDITIONAL), - pluginId: z.string(), - resourceType: z.string(), - conditions: permissionCriteriaSchema, - }), -]); +const queryPermissionResponseSchema: z.ZodSchema = + z.union([ + z.object({ + result: z + .literal(AuthorizeResult.ALLOW) + .or(z.literal(AuthorizeResult.DENY)), + }), + z.object({ + result: z.literal(AuthorizeResult.CONDITIONAL), + pluginId: z.string(), + resourceType: z.string(), + conditions: permissionCriteriaSchema, + }), + ]); const responseSchema = ( itemSchema: z.ZodSchema, @@ -122,7 +123,11 @@ export class PermissionClient implements PermissionEvaluator { requests: AuthorizePermissionRequest[], options?: EvaluatorRequestOptions, ): Promise { - return this.makeRequest(requests, authorizeDecisionSchema, options); + return this.makeRequest( + requests, + authorizePermissionResponseSchema, + options, + ); } /** @@ -132,7 +137,7 @@ export class PermissionClient implements PermissionEvaluator { queries: QueryPermissionRequest[], options?: EvaluatorRequestOptions, ): Promise { - return this.makeRequest(queries, policyDecisionSchema, options); + return this.makeRequest(queries, queryPermissionResponseSchema, options); } private async makeRequest( From afb7535c2eb3c45b136cb41b7b831391d7b755af Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Thu, 31 Mar 2022 10:00:05 +0200 Subject: [PATCH 15/24] Use PermissionEvaluator instead of ServerPermissionClient Signed-off-by: Vincenzo Scamporlino --- .changeset/four-dolphins-report.md | 4 ++-- .changeset/rare-parents-pretend.md | 6 +++--- packages/backend/src/types.ts | 4 ++-- .../templates/default-app/packages/backend/src/types.ts | 4 ++-- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/.changeset/four-dolphins-report.md b/.changeset/four-dolphins-report.md index 52bbbac1e8..ff0178c9f6 100644 --- a/.changeset/four-dolphins-report.md +++ b/.changeset/four-dolphins-report.md @@ -1,5 +1,5 @@ --- -'@backstage/plugin-permission-node': patch +'@backstage/plugin-permission-node': minor --- -Use new `PermissionEvaluator#query` method in `ServerPermissionClient` and test suites. +**BREAKING:** `ServerPermissionClient` now implements `PermissionEvaluator`, which moves out the capabilities for evaluating conditional decisions from `authorize()` to `query()` method. diff --git a/.changeset/rare-parents-pretend.md b/.changeset/rare-parents-pretend.md index b483666e66..cb97c6999e 100644 --- a/.changeset/rare-parents-pretend.md +++ b/.changeset/rare-parents-pretend.md @@ -2,13 +2,13 @@ '@backstage/create-app': patch --- -Use `ServerPermissionClient` instead of `PermissionAuthorizer`. +Use `PermissionEvaluator` instead of `PermissionAuthorizer`. Apply the following to `packages/backend/src/types.ts`: ```diff - import { PermissionAuthorizer } from '@backstage/plugin-permission-common'; -+ import { ServerPermissionClient } from '@backstage/plugin-permission-node'; ++ import { PermissionEvaluator } from '@backstage/plugin-permission-common'; export type PluginEnvironment = { ... @@ -16,6 +16,6 @@ Apply the following to `packages/backend/src/types.ts`: tokenManager: TokenManager; scheduler: PluginTaskScheduler; - permissions: PermissionAuthorizer; -+ permissions: ServerPermissionClient; ++ permissions: PermissionEvaluator; }; ``` diff --git a/packages/backend/src/types.ts b/packages/backend/src/types.ts index 0b2543c4f5..d1f62d833b 100644 --- a/packages/backend/src/types.ts +++ b/packages/backend/src/types.ts @@ -23,8 +23,8 @@ import { TokenManager, UrlReader, } from '@backstage/backend-common'; -import { ServerPermissionClient } from '@backstage/plugin-permission-node'; import { PluginTaskScheduler } from '@backstage/backend-tasks'; +import { PermissionEvaluator } from '@backstage/plugin-permission-common'; export type PluginEnvironment = { logger: Logger; @@ -34,6 +34,6 @@ export type PluginEnvironment = { reader: UrlReader; discovery: PluginEndpointDiscovery; tokenManager: TokenManager; - permissions: ServerPermissionClient; + permissions: PermissionEvaluator; scheduler: PluginTaskScheduler; }; diff --git a/packages/create-app/templates/default-app/packages/backend/src/types.ts b/packages/create-app/templates/default-app/packages/backend/src/types.ts index 7984b7361f..8e0a86404b 100644 --- a/packages/create-app/templates/default-app/packages/backend/src/types.ts +++ b/packages/create-app/templates/default-app/packages/backend/src/types.ts @@ -8,7 +8,7 @@ import { UrlReader, } from '@backstage/backend-common'; import { PluginTaskScheduler } from '@backstage/backend-tasks'; -import { ServerPermissionClient } from '@backstage/plugin-permission-node'; +import { PermissionEvaluator } from '@backstage/plugin-permission-common'; export type PluginEnvironment = { logger: Logger; @@ -19,5 +19,5 @@ export type PluginEnvironment = { discovery: PluginEndpointDiscovery; tokenManager: TokenManager; scheduler: PluginTaskScheduler; - permissions: ServerPermissionClient; + permissions: PermissionEvaluator; }; From 1917923ab82e9995d783ab203e43c2ba0d0859a7 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Thu, 31 Mar 2022 10:01:35 +0200 Subject: [PATCH 16/24] Jenkins: use PermissionEvaluator instead of PermissionAuthorizer Signed-off-by: Vincenzo Scamporlino --- .changeset/fresh-boxes-pull.md | 5 +++++ plugins/jenkins-backend/api-report.md | 4 ++-- plugins/jenkins-backend/src/service/jenkinsApi.ts | 4 ++-- plugins/jenkins-backend/src/service/router.ts | 4 ++-- 4 files changed, 11 insertions(+), 6 deletions(-) create mode 100644 .changeset/fresh-boxes-pull.md diff --git a/.changeset/fresh-boxes-pull.md b/.changeset/fresh-boxes-pull.md new file mode 100644 index 0000000000..87bff5c3a7 --- /dev/null +++ b/.changeset/fresh-boxes-pull.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-jenkins-backend': patch +--- + +Use PermissionEvaluator instead of PermissionAuthorizer diff --git a/plugins/jenkins-backend/api-report.md b/plugins/jenkins-backend/api-report.md index 37e8f80a80..315187e669 100644 --- a/plugins/jenkins-backend/api-report.md +++ b/plugins/jenkins-backend/api-report.md @@ -8,7 +8,7 @@ import { CompoundEntityRef } from '@backstage/catalog-model'; import { Config } from '@backstage/config'; import express from 'express'; import { Logger } from 'winston'; -import { PermissionAuthorizer } from '@backstage/plugin-permission-common'; +import { PermissionEvaluator } from '@backstage/plugin-permission-common'; // Warning: (ae-missing-release-tag) "createRouter" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) // @@ -98,6 +98,6 @@ export interface RouterOptions { // (undocumented) logger: Logger; // (undocumented) - permissions?: PermissionAuthorizer; + permissions?: PermissionEvaluator; } ``` diff --git a/plugins/jenkins-backend/src/service/jenkinsApi.ts b/plugins/jenkins-backend/src/service/jenkinsApi.ts index 38ab20b17b..8069f57692 100644 --- a/plugins/jenkins-backend/src/service/jenkinsApi.ts +++ b/plugins/jenkins-backend/src/service/jenkinsApi.ts @@ -25,7 +25,7 @@ import type { } from '../types'; import { AuthorizeResult, - PermissionAuthorizer, + PermissionEvaluator, } from '@backstage/plugin-permission-common'; import { jenkinsExecutePermission } from '@backstage/plugin-jenkins-common'; import { NotAllowedError } from '@backstage/errors'; @@ -64,7 +64,7 @@ export class JenkinsApiImpl { ${JenkinsApiImpl.jobTreeSpec} ]{0,50}`; - constructor(private readonly permissionApi?: PermissionAuthorizer) {} + constructor(private readonly permissionApi?: PermissionEvaluator) {} /** * Get a list of projects for the given JenkinsInfo. diff --git a/plugins/jenkins-backend/src/service/router.ts b/plugins/jenkins-backend/src/service/router.ts index 03462b923f..1ad8f22b25 100644 --- a/plugins/jenkins-backend/src/service/router.ts +++ b/plugins/jenkins-backend/src/service/router.ts @@ -20,14 +20,14 @@ import Router from 'express-promise-router'; import { Logger } from 'winston'; import { JenkinsInfoProvider } from './jenkinsInfoProvider'; import { JenkinsApiImpl } from './jenkinsApi'; -import { PermissionAuthorizer } from '@backstage/plugin-permission-common'; +import { PermissionEvaluator } from '@backstage/plugin-permission-common'; import { getBearerTokenFromAuthorizationHeader } from '@backstage/plugin-auth-node'; import { stringifyEntityRef } from '@backstage/catalog-model'; export interface RouterOptions { logger: Logger; jenkinsInfoProvider: JenkinsInfoProvider; - permissions?: PermissionAuthorizer; + permissions?: PermissionEvaluator; } export async function createRouter( From 6dd85d588adc81536ba7060e6e4f999d381dadd6 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Thu, 31 Mar 2022 10:02:23 +0200 Subject: [PATCH 17/24] catalog-backend: use new PermissionEvaluator instead of PermissionAuthorizer Signed-off-by: Vincenzo Scamporlino --- .../catalog-backend/src/service/AuthorizedLocationService.ts | 4 ++-- .../catalog-backend/src/service/AuthorizedRefreshService.ts | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/plugins/catalog-backend/src/service/AuthorizedLocationService.ts b/plugins/catalog-backend/src/service/AuthorizedLocationService.ts index 077f56c737..a73597649e 100644 --- a/plugins/catalog-backend/src/service/AuthorizedLocationService.ts +++ b/plugins/catalog-backend/src/service/AuthorizedLocationService.ts @@ -24,14 +24,14 @@ import { } from '@backstage/plugin-catalog-common'; import { AuthorizeResult, - PermissionAuthorizer, + PermissionEvaluator, } from '@backstage/plugin-permission-common'; import { LocationInput, LocationService } from './types'; export class AuthorizedLocationService implements LocationService { constructor( private readonly locationService: LocationService, - private readonly permissionApi: PermissionAuthorizer, + private readonly permissionApi: PermissionEvaluator, ) {} async createLocation( diff --git a/plugins/catalog-backend/src/service/AuthorizedRefreshService.ts b/plugins/catalog-backend/src/service/AuthorizedRefreshService.ts index 17dfb58c54..8634fbf86d 100644 --- a/plugins/catalog-backend/src/service/AuthorizedRefreshService.ts +++ b/plugins/catalog-backend/src/service/AuthorizedRefreshService.ts @@ -18,14 +18,14 @@ import { NotAllowedError } from '@backstage/errors'; import { catalogEntityRefreshPermission } from '@backstage/plugin-catalog-common'; import { AuthorizeResult, - PermissionAuthorizer, + PermissionEvaluator, } from '@backstage/plugin-permission-common'; import { RefreshOptions, RefreshService } from './types'; export class AuthorizedRefreshService implements RefreshService { constructor( private readonly service: RefreshService, - private readonly permissionApi: PermissionAuthorizer, + private readonly permissionApi: PermissionEvaluator, ) {} async refresh(options: RefreshOptions) { From 8f4792a1962fd60e981aef730d0335a1801806d8 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Thu, 31 Mar 2022 10:09:29 +0200 Subject: [PATCH 18/24] Mark PermissionAuthorizer as deprecated Signed-off-by: Vincenzo Scamporlino --- plugins/permission-common/api-report.md | 2 +- plugins/permission-common/src/types/permission.ts | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/plugins/permission-common/api-report.md b/plugins/permission-common/api-report.md index e952425bbf..e33449c3d6 100644 --- a/plugins/permission-common/api-report.md +++ b/plugins/permission-common/api-report.md @@ -139,7 +139,7 @@ export type PermissionAttributes = { action?: 'create' | 'read' | 'update' | 'delete'; }; -// @public +// @public @deprecated export interface PermissionAuthorizer { // (undocumented) authorize( diff --git a/plugins/permission-common/src/types/permission.ts b/plugins/permission-common/src/types/permission.ts index 34e7884793..ab9f888605 100644 --- a/plugins/permission-common/src/types/permission.ts +++ b/plugins/permission-common/src/types/permission.ts @@ -91,6 +91,7 @@ export type ResourcePermission = /** * A client interacting with the permission backend can implement this authorizer interface. * @public + * @deprecated Use PermissionEvaluator instead */ export interface PermissionAuthorizer { authorize( From 8b27170d3024d2b1a3f446e0cbc6a1a4abcdf2c9 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Thu, 31 Mar 2022 10:10:08 +0200 Subject: [PATCH 19/24] search-backend: Use PermissionEvaluator instead of PermissionAuthorizer Signed-off-by: Vincenzo Scamporlino --- .changeset/eight-cobras-think.md | 2 +- .../src/service/AuthorizedSearchEngine.test.ts | 7 +++---- plugins/search-backend/src/service/router.test.ts | 6 +++--- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/.changeset/eight-cobras-think.md b/.changeset/eight-cobras-think.md index 854e4fcec8..3a9ec54a90 100644 --- a/.changeset/eight-cobras-think.md +++ b/.changeset/eight-cobras-think.md @@ -2,4 +2,4 @@ '@backstage/plugin-search-backend': patch --- -Fix typing when invoking `PermissionClient#authorize` +Use `PermissionEvaluator` instead of `PermissionAuthorizer`. diff --git a/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts b/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts index 8a7662c1f4..5f666b3a26 100644 --- a/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts +++ b/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts @@ -19,7 +19,6 @@ import { EvaluatePermissionResponse, AuthorizeResult, createPermission, - PermissionAuthorizer, PolicyDecision, PermissionEvaluator, } from '@backstage/plugin-permission-common'; @@ -77,7 +76,7 @@ describe('AuthorizedSearchEngine', () => { PermissionEvaluator['query'] > = jest.fn(); - const permissionAuthorizer: PermissionEvaluator = { + const permissionEvaluator: PermissionEvaluator = { authorize: mockedAuthorize, query: mockedPermissionQuery, }; @@ -116,13 +115,13 @@ describe('AuthorizedSearchEngine', () => { const authorizedSearchEngine = new AuthorizedSearchEngine( searchEngine, defaultTypes, - permissionAuthorizer, + permissionEvaluator, new ConfigReader({}), ); const options = { token: 'token' }; - const allowAll: PermissionAuthorizer['authorize'] & + const allowAll: PermissionEvaluator['authorize'] & PermissionEvaluator['query'] = async queries => { return queries.map(() => ({ result: AuthorizeResult.ALLOW, diff --git a/plugins/search-backend/src/service/router.test.ts b/plugins/search-backend/src/service/router.test.ts index 85773fb3b8..673556f450 100644 --- a/plugins/search-backend/src/service/router.test.ts +++ b/plugins/search-backend/src/service/router.test.ts @@ -26,7 +26,7 @@ import request from 'supertest'; import { createRouter } from './router'; -const mockPermissionAuthorizer: PermissionEvaluator = { +const mockPermissionEvaluator: PermissionEvaluator = { authorize: () => { throw new Error('Not implemented'); }, @@ -62,7 +62,7 @@ describe('createRouter', () => { 'second-type': {}, }, config: new ConfigReader({ permissions: { enabled: false } }), - permissions: mockPermissionAuthorizer, + permissions: mockPermissionEvaluator, logger, }); app = express().use(router); @@ -167,7 +167,7 @@ describe('createRouter', () => { engine: indexBuilder.getSearchEngine(), types: indexBuilder.getDocumentTypes(), config: new ConfigReader({ permissions: { enabled: false } }), - permissions: mockPermissionAuthorizer, + permissions: mockPermissionEvaluator, logger, }); app = express().use(router); From 173aadff5b4be95d84ac75222c4123947031674e Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Tue, 5 Apr 2022 12:47:28 +0200 Subject: [PATCH 20/24] Avoid PermissionEvaluator breaking changes Signed-off-by: Vincenzo Scamporlino --- packages/backend/src/types.ts | 7 ++-- .../src/service/CatalogBuilder.ts | 25 ++++++++++--- plugins/jenkins-backend/src/service/router.ts | 24 ++++++++++--- .../permission-common/src/permissions/util.ts | 36 ++++++++++++++++++- plugins/search-backend/src/service/router.ts | 25 +++++++++++-- 5 files changed, 102 insertions(+), 15 deletions(-) diff --git a/packages/backend/src/types.ts b/packages/backend/src/types.ts index d1f62d833b..3e47b1a523 100644 --- a/packages/backend/src/types.ts +++ b/packages/backend/src/types.ts @@ -24,7 +24,10 @@ import { UrlReader, } from '@backstage/backend-common'; import { PluginTaskScheduler } from '@backstage/backend-tasks'; -import { PermissionEvaluator } from '@backstage/plugin-permission-common'; +import { + PermissionAuthorizer, + PermissionEvaluator, +} from '@backstage/plugin-permission-common'; export type PluginEnvironment = { logger: Logger; @@ -34,6 +37,6 @@ export type PluginEnvironment = { reader: UrlReader; discovery: PluginEndpointDiscovery; tokenManager: TokenManager; - permissions: PermissionEvaluator; + permissions: PermissionEvaluator | PermissionAuthorizer; scheduler: PluginTaskScheduler; }; diff --git a/plugins/catalog-backend/src/service/CatalogBuilder.ts b/plugins/catalog-backend/src/service/CatalogBuilder.ts index e9135393ba..90ffc467c2 100644 --- a/plugins/catalog-backend/src/service/CatalogBuilder.ts +++ b/plugins/catalog-backend/src/service/CatalogBuilder.ts @@ -79,7 +79,11 @@ import { CatalogPermissionRule, permissionRules as catalogPermissionRules, } from '../permissions/rules'; -import { PermissionEvaluator } from '@backstage/plugin-permission-common'; +import { + PermissionAuthorizer, + PermissionEvaluator, + toPermissionEvaluator, +} from '@backstage/plugin-permission-common'; import { createConditionTransformer, createPermissionIntegrationRouter, @@ -95,7 +99,7 @@ export type CatalogEnvironment = { database: PluginDatabaseManager; config: Config; reader: UrlReader; - permissions: PermissionEvaluator; + permissions: PermissionEvaluator | PermissionAuthorizer; }; /** @@ -376,9 +380,20 @@ export class CatalogBuilder { policy, }); const unauthorizedEntitiesCatalog = new DefaultEntitiesCatalog(dbClient); + + let permissionEvaluator: PermissionEvaluator; + if (!permissions.hasOwnProperty('query')) { + logger.warn( + 'PermissionAuthorizer is deprecated. Please use PermissionEvaluator instead of PermissionAuthorizer in catalog.ts', + ); + permissionEvaluator = toPermissionEvaluator(permissions); + } else { + permissionEvaluator = permissions as PermissionEvaluator; + } + const entitiesCatalog = new AuthorizedEntitiesCatalog( unauthorizedEntitiesCatalog, - permissions, + permissionEvaluator, createConditionTransformer(this.permissionRules), ); const permissionIntegrationRouter = createPermissionIntegrationRouter({ @@ -428,11 +443,11 @@ export class CatalogBuilder { this.locationAnalyzer ?? new RepoLocationAnalyzer(logger, integrations); const locationService = new AuthorizedLocationService( new DefaultLocationService(locationStore, orchestrator), - permissions, + permissionEvaluator, ); const refreshService = new AuthorizedRefreshService( new DefaultRefreshService({ database: processingDatabase }), - permissions, + permissionEvaluator, ); const router = await createRouter({ entitiesCatalog, diff --git a/plugins/jenkins-backend/src/service/router.ts b/plugins/jenkins-backend/src/service/router.ts index 1ad8f22b25..8787be70df 100644 --- a/plugins/jenkins-backend/src/service/router.ts +++ b/plugins/jenkins-backend/src/service/router.ts @@ -20,22 +20,38 @@ import Router from 'express-promise-router'; import { Logger } from 'winston'; import { JenkinsInfoProvider } from './jenkinsInfoProvider'; import { JenkinsApiImpl } from './jenkinsApi'; -import { PermissionEvaluator } from '@backstage/plugin-permission-common'; +import { + PermissionAuthorizer, + PermissionEvaluator, + toPermissionEvaluator, +} from '@backstage/plugin-permission-common'; import { getBearerTokenFromAuthorizationHeader } from '@backstage/plugin-auth-node'; import { stringifyEntityRef } from '@backstage/catalog-model'; export interface RouterOptions { logger: Logger; jenkinsInfoProvider: JenkinsInfoProvider; - permissions?: PermissionEvaluator; + permissions?: PermissionEvaluator | PermissionAuthorizer; } export async function createRouter( options: RouterOptions, ): Promise { - const { jenkinsInfoProvider } = options; + const { jenkinsInfoProvider, permissions, logger } = options; - const jenkinsApi = new JenkinsApiImpl(options.permissions); + let permissionEvaluator: PermissionEvaluator | undefined; + if (permissions?.hasOwnProperty('query')) { + permissionEvaluator = permissions as PermissionEvaluator; + } else { + logger.warn( + 'PermissionAuthorizer is deprecated. Please use PermissionEvaluator instead of PermissionAuthorizer in your jenkins.ts', + ); + permissionEvaluator = permissions + ? toPermissionEvaluator(permissions) + : undefined; + } + + const jenkinsApi = new JenkinsApiImpl(permissionEvaluator); const router = Router(); router.use(express.json()); diff --git a/plugins/permission-common/src/permissions/util.ts b/plugins/permission-common/src/permissions/util.ts index 1a5ed434a7..b40b582149 100644 --- a/plugins/permission-common/src/permissions/util.ts +++ b/plugins/permission-common/src/permissions/util.ts @@ -14,7 +14,18 @@ * limitations under the License. */ -import { Permission, ResourcePermission } from '../types'; +import { + AuthorizePermissionRequest, + AuthorizePermissionResponse, + DefinitivePolicyDecision, + EvaluatorRequestOptions, + Permission, + PermissionAuthorizer, + PermissionEvaluator, + QueryPermissionRequest, + QueryPermissionResponse, + ResourcePermission, +} from '../types'; /** * Check if the two parameters are equivalent permissions. @@ -75,3 +86,26 @@ export function isUpdatePermission(permission: Permission) { export function isDeletePermission(permission: Permission) { return permission.attributes.action === 'delete'; } + +export function toPermissionEvaluator( + permissionAuthorizer: PermissionAuthorizer, +): PermissionEvaluator { + return { + authorize: async ( + requests: AuthorizePermissionRequest[], + options?: EvaluatorRequestOptions, + ): Promise => { + const response = await permissionAuthorizer.authorize(requests, options); + + return response as DefinitivePolicyDecision[]; + }, + query( + requests: QueryPermissionRequest[], + options?: EvaluatorRequestOptions, + ): Promise { + // @ts-expect-error + const parsedRequests: AuthorizePermissionRequest[] = requests; + return permissionAuthorizer.authorize(parsedRequests, options); + }, + }; +} diff --git a/plugins/search-backend/src/service/router.ts b/plugins/search-backend/src/service/router.ts index 6867c382df..bd956a9ff8 100644 --- a/plugins/search-backend/src/service/router.ts +++ b/plugins/search-backend/src/service/router.ts @@ -23,7 +23,11 @@ import { InputError } from '@backstage/errors'; import { Config } from '@backstage/config'; import { JsonObject, JsonValue } from '@backstage/types'; import { getBearerTokenFromAuthorizationHeader } from '@backstage/plugin-auth-node'; -import { PermissionEvaluator } from '@backstage/plugin-permission-common'; +import { + PermissionAuthorizer, + PermissionEvaluator, + toPermissionEvaluator, +} from '@backstage/plugin-permission-common'; import { DocumentTypeInfo, IndexableResultSet, @@ -50,7 +54,7 @@ const jsonObjectSchema: z.ZodSchema = z.lazy(() => { export type RouterOptions = { engine: SearchEngine; types: Record; - permissions: PermissionEvaluator; + permissions: PermissionEvaluator | PermissionAuthorizer; config: Config; logger: Logger; }; @@ -71,8 +75,23 @@ export async function createRouter( pageCursor: z.string().optional(), }); + let permissionEvaluator: PermissionEvaluator; + if (!permissions.hasOwnProperty('query')) { + logger.warn( + 'PermissionAuthorizer is deprecated. Please use PermissionEvaluator instead of PermissionAuthorizer in search.ts', + ); + permissionEvaluator = toPermissionEvaluator(permissions); + } else { + permissionEvaluator = permissions as PermissionEvaluator; + } + const engine = config.getOptionalBoolean('permission.enabled') - ? new AuthorizedSearchEngine(inputEngine, types, permissions, config) + ? new AuthorizedSearchEngine( + inputEngine, + types, + permissionEvaluator, + config, + ) : inputEngine; const filterResultSet = ({ results, ...resultSet }: SearchResultSet) => ({ From b4af8664b5ab1943c913a27cfdddeccc08edfaed Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Tue, 5 Apr 2022 16:05:25 +0200 Subject: [PATCH 21/24] api report and minor fixes Signed-off-by: Vincenzo Scamporlino --- plugins/catalog-backend/api-report.md | 3 ++- plugins/catalog-backend/src/service/CatalogBuilder.ts | 8 ++++---- plugins/jenkins-backend/api-report.md | 3 ++- plugins/jenkins-backend/src/service/router.ts | 4 ++-- plugins/permission-common/api-report.md | 5 +++++ plugins/permission-common/src/permissions/util.ts | 9 +++++++-- plugins/search-backend/api-report.md | 3 ++- plugins/search-backend/src/service/router.ts | 8 ++++---- 8 files changed, 28 insertions(+), 15 deletions(-) diff --git a/plugins/catalog-backend/api-report.md b/plugins/catalog-backend/api-report.md index 7efdfec424..a770d6289e 100644 --- a/plugins/catalog-backend/api-report.md +++ b/plugins/catalog-backend/api-report.md @@ -19,6 +19,7 @@ import { JsonValue } from '@backstage/types'; import { LocationEntityV1alpha1 } from '@backstage/catalog-model'; import { Logger } from 'winston'; import { Permission } from '@backstage/plugin-permission-common'; +import { PermissionAuthorizer } from '@backstage/plugin-permission-common'; import { PermissionCondition } from '@backstage/plugin-permission-common'; import { PermissionCriteria } from '@backstage/plugin-permission-common'; import { PermissionEvaluator } from '@backstage/plugin-permission-common'; @@ -181,7 +182,7 @@ export type CatalogEnvironment = { database: PluginDatabaseManager; config: Config; reader: UrlReader; - permissions: PermissionEvaluator; + permissions: PermissionEvaluator | PermissionAuthorizer; }; // @alpha diff --git a/plugins/catalog-backend/src/service/CatalogBuilder.ts b/plugins/catalog-backend/src/service/CatalogBuilder.ts index 90ffc467c2..f5f00cd0fc 100644 --- a/plugins/catalog-backend/src/service/CatalogBuilder.ts +++ b/plugins/catalog-backend/src/service/CatalogBuilder.ts @@ -382,13 +382,13 @@ export class CatalogBuilder { const unauthorizedEntitiesCatalog = new DefaultEntitiesCatalog(dbClient); let permissionEvaluator: PermissionEvaluator; - if (!permissions.hasOwnProperty('query')) { + if ('query' in permissions) { + permissionEvaluator = permissions as PermissionEvaluator; + } else { logger.warn( - 'PermissionAuthorizer is deprecated. Please use PermissionEvaluator instead of PermissionAuthorizer in catalog.ts', + 'PermissionAuthorizer is deprecated. Please use an instance of PermissionEvaluator instead of PermissionAuthorizer in PluginEnvironment#permissions', ); permissionEvaluator = toPermissionEvaluator(permissions); - } else { - permissionEvaluator = permissions as PermissionEvaluator; } const entitiesCatalog = new AuthorizedEntitiesCatalog( diff --git a/plugins/jenkins-backend/api-report.md b/plugins/jenkins-backend/api-report.md index 315187e669..029f2c7af3 100644 --- a/plugins/jenkins-backend/api-report.md +++ b/plugins/jenkins-backend/api-report.md @@ -8,6 +8,7 @@ import { CompoundEntityRef } from '@backstage/catalog-model'; import { Config } from '@backstage/config'; import express from 'express'; import { Logger } from 'winston'; +import { PermissionAuthorizer } from '@backstage/plugin-permission-common'; import { PermissionEvaluator } from '@backstage/plugin-permission-common'; // Warning: (ae-missing-release-tag) "createRouter" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) @@ -98,6 +99,6 @@ export interface RouterOptions { // (undocumented) logger: Logger; // (undocumented) - permissions?: PermissionEvaluator; + permissions?: PermissionEvaluator | PermissionAuthorizer; } ``` diff --git a/plugins/jenkins-backend/src/service/router.ts b/plugins/jenkins-backend/src/service/router.ts index 8787be70df..f6bd21c633 100644 --- a/plugins/jenkins-backend/src/service/router.ts +++ b/plugins/jenkins-backend/src/service/router.ts @@ -40,11 +40,11 @@ export async function createRouter( const { jenkinsInfoProvider, permissions, logger } = options; let permissionEvaluator: PermissionEvaluator | undefined; - if (permissions?.hasOwnProperty('query')) { + if (permissions && 'query' in permissions) { permissionEvaluator = permissions as PermissionEvaluator; } else { logger.warn( - 'PermissionAuthorizer is deprecated. Please use PermissionEvaluator instead of PermissionAuthorizer in your jenkins.ts', + 'PermissionAuthorizer is deprecated. Please use an instance of PermissionEvaluator instead of PermissionAuthorizer in PluginEnvironment#permissions', ); permissionEvaluator = permissions ? toPermissionEvaluator(permissions) diff --git a/plugins/permission-common/api-report.md b/plugins/permission-common/api-report.md index e33449c3d6..41abb62cf6 100644 --- a/plugins/permission-common/api-report.md +++ b/plugins/permission-common/api-report.md @@ -225,4 +225,9 @@ export type ResourcePermission = resourceType: TResourceType; } >; + +// @public +export function toPermissionEvaluator( + permissionAuthorizer: PermissionAuthorizer, +): PermissionEvaluator; ``` diff --git a/plugins/permission-common/src/permissions/util.ts b/plugins/permission-common/src/permissions/util.ts index b40b582149..6e0aca457b 100644 --- a/plugins/permission-common/src/permissions/util.ts +++ b/plugins/permission-common/src/permissions/util.ts @@ -87,6 +87,11 @@ export function isDeletePermission(permission: Permission) { return permission.attributes.action === 'delete'; } +/** + * Convert {@link PermissionAuthorizer} to {@link PermissionEvaluator}. + * + * @public + */ export function toPermissionEvaluator( permissionAuthorizer: PermissionAuthorizer, ): PermissionEvaluator { @@ -103,8 +108,8 @@ export function toPermissionEvaluator( requests: QueryPermissionRequest[], options?: EvaluatorRequestOptions, ): Promise { - // @ts-expect-error - const parsedRequests: AuthorizePermissionRequest[] = requests; + const parsedRequests = + requests as unknown as AuthorizePermissionRequest[]; return permissionAuthorizer.authorize(parsedRequests, options); }, }; diff --git a/plugins/search-backend/api-report.md b/plugins/search-backend/api-report.md index dc14d2f15b..c5c8eb7a13 100644 --- a/plugins/search-backend/api-report.md +++ b/plugins/search-backend/api-report.md @@ -7,6 +7,7 @@ import { Config } from '@backstage/config'; import { DocumentTypeInfo } from '@backstage/plugin-search-common'; import express from 'express'; import { Logger } from 'winston'; +import { PermissionAuthorizer } from '@backstage/plugin-permission-common'; import { PermissionEvaluator } from '@backstage/plugin-permission-common'; import { SearchEngine } from '@backstage/plugin-search-backend-node'; @@ -21,7 +22,7 @@ export function createRouter(options: RouterOptions): Promise; export type RouterOptions = { engine: SearchEngine; types: Record; - permissions: PermissionEvaluator; + permissions: PermissionEvaluator | PermissionAuthorizer; config: Config; logger: Logger; }; diff --git a/plugins/search-backend/src/service/router.ts b/plugins/search-backend/src/service/router.ts index bd956a9ff8..11f0eb22e6 100644 --- a/plugins/search-backend/src/service/router.ts +++ b/plugins/search-backend/src/service/router.ts @@ -76,13 +76,13 @@ export async function createRouter( }); let permissionEvaluator: PermissionEvaluator; - if (!permissions.hasOwnProperty('query')) { + if ('query' in permissions) { + permissionEvaluator = permissions as PermissionEvaluator; + } else { logger.warn( - 'PermissionAuthorizer is deprecated. Please use PermissionEvaluator instead of PermissionAuthorizer in search.ts', + 'PermissionAuthorizer is deprecated. Please use an instance of PermissionEvaluator instead of PermissionAuthorizer in PluginEnvironment#permissions', ); permissionEvaluator = toPermissionEvaluator(permissions); - } else { - permissionEvaluator = permissions as PermissionEvaluator; } const engine = config.getOptionalBoolean('permission.enabled') From 6ca16bdf2db7a588a9874c6945e38631609e8a1d Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Wed, 6 Apr 2022 14:44:58 +0200 Subject: [PATCH 22/24] Apply suggestions from code review Co-authored-by: Joe Porpeglia Signed-off-by: Vincenzo Scamporlino --- plugins/permission-common/src/types/permission.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/permission-common/src/types/permission.ts b/plugins/permission-common/src/types/permission.ts index ab9f888605..eb1c0eb50d 100644 --- a/plugins/permission-common/src/types/permission.ts +++ b/plugins/permission-common/src/types/permission.ts @@ -91,7 +91,7 @@ export type ResourcePermission = /** * A client interacting with the permission backend can implement this authorizer interface. * @public - * @deprecated Use PermissionEvaluator instead + * @deprecated Use {@link @backstage/plugin-permission-common#PermissionEvaluator} instead */ export interface PermissionAuthorizer { authorize( From 3f41ada39d2b693070b034f44bb16aa56120e6f7 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Wed, 6 Apr 2022 14:47:06 +0200 Subject: [PATCH 23/24] Update create-app changeset Signed-off-by: Vincenzo Scamporlino --- .changeset/rare-parents-pretend.md | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/.changeset/rare-parents-pretend.md b/.changeset/rare-parents-pretend.md index cb97c6999e..4c8952ad14 100644 --- a/.changeset/rare-parents-pretend.md +++ b/.changeset/rare-parents-pretend.md @@ -2,20 +2,23 @@ '@backstage/create-app': patch --- -Use `PermissionEvaluator` instead of `PermissionAuthorizer`. +Accept `PermissionEvaluator` together with the deprecated `PermissionAuthorizer`. Apply the following to `packages/backend/src/types.ts`: ```diff -- import { PermissionAuthorizer } from '@backstage/plugin-permission-common'; -+ import { PermissionEvaluator } from '@backstage/plugin-permission-common'; +- import { ServerPermissionClient } from '@backstage/plugin-permission-node'; ++ import { ++ PermissionAuthorizer, ++ PermissionEvaluator, ++ } from '@backstage/plugin-permission-common'; export type PluginEnvironment = { ... discovery: PluginEndpointDiscovery; tokenManager: TokenManager; scheduler: PluginTaskScheduler; -- permissions: PermissionAuthorizer; -+ permissions: PermissionEvaluator; +- permissions: ServerPermissionClient; ++ permissions: PermissionEvaluator | PermissionAuthorizer; }; ``` From 63902fcc1787d7824ef6b5d735611e14f8a53987 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Thu, 7 Apr 2022 20:25:53 +0200 Subject: [PATCH 24/24] PermissionEvaluator: rename query to authorizeConditional Signed-off-by: Vincenzo Scamporlino --- .changeset/cuddly-turtles-sleep.md | 2 +- .changeset/eight-cobras-think.md | 2 +- .changeset/few-seas-fail.md | 2 +- .changeset/four-dolphins-report.md | 2 +- .changeset/fresh-boxes-pull.md | 2 +- .changeset/rare-parents-pretend.md | 13 ++-- .../service/AuthorizedEntitiesCatalog.test.ts | 22 +++---- .../src/service/AuthorizedEntitiesCatalog.ts | 6 +- .../service/AuthorizedLocationService.test.ts | 2 +- .../src/service/jenkinsApi.test.ts | 2 +- plugins/permission-common/api-report.md | 4 +- .../src/PermissionClient.test.ts | 62 +++++++++++-------- .../permission-common/src/PermissionClient.ts | 4 +- .../permission-common/src/permissions/util.ts | 2 +- plugins/permission-common/src/types/api.ts | 6 +- plugins/permission-node/api-report.md | 10 +-- .../src/ServerPermissionClient.test.ts | 24 ++++--- .../src/ServerPermissionClient.ts | 5 +- .../service/AuthorizedSearchEngine.test.ts | 6 +- .../src/service/AuthorizedSearchEngine.ts | 2 +- .../search-backend/src/service/router.test.ts | 2 +- 21 files changed, 98 insertions(+), 84 deletions(-) diff --git a/.changeset/cuddly-turtles-sleep.md b/.changeset/cuddly-turtles-sleep.md index bb1a925506..a63cd44232 100644 --- a/.changeset/cuddly-turtles-sleep.md +++ b/.changeset/cuddly-turtles-sleep.md @@ -2,4 +2,4 @@ '@backstage/plugin-catalog-backend': patch --- -Use new `PermissionEvaluator#query` method when retrieving permission conditions. +Use new `PermissionEvaluator#authorizeConditional` method when retrieving permission conditions. diff --git a/.changeset/eight-cobras-think.md b/.changeset/eight-cobras-think.md index 3a9ec54a90..af1c7cd113 100644 --- a/.changeset/eight-cobras-think.md +++ b/.changeset/eight-cobras-think.md @@ -2,4 +2,4 @@ '@backstage/plugin-search-backend': patch --- -Use `PermissionEvaluator` instead of `PermissionAuthorizer`. +Use `PermissionEvaluator` instead of `PermissionAuthorizer`, which is now deprecated. diff --git a/.changeset/few-seas-fail.md b/.changeset/few-seas-fail.md index 8ace0f3515..c5d9db4d47 100644 --- a/.changeset/few-seas-fail.md +++ b/.changeset/few-seas-fail.md @@ -5,4 +5,4 @@ Added `PermissionEvaluator`, which will replace the existing `PermissionAuthorizer` interface. This new interface provides stronger type safety and validation by splitting `PermissionAuthorizer.authorize()` into two methods: - `authorize()`: Used when the caller requires a definitive decision. -- `query()`: Used when the caller can optimize the evaluation of any conditional decisions. For example, a plugin backend may want to use conditions in a database query instead of evaluating each resource in memory. +- `authorizeConditional()`: Used when the caller can optimize the evaluation of any conditional decisions. For example, a plugin backend may want to use conditions in a database query instead of evaluating each resource in memory. diff --git a/.changeset/four-dolphins-report.md b/.changeset/four-dolphins-report.md index ff0178c9f6..fa3f06c532 100644 --- a/.changeset/four-dolphins-report.md +++ b/.changeset/four-dolphins-report.md @@ -2,4 +2,4 @@ '@backstage/plugin-permission-node': minor --- -**BREAKING:** `ServerPermissionClient` now implements `PermissionEvaluator`, which moves out the capabilities for evaluating conditional decisions from `authorize()` to `query()` method. +**BREAKING:** `ServerPermissionClient` now implements `PermissionEvaluator`, which moves out the capabilities for evaluating conditional decisions from `authorize()` to `authorizeConditional()` method. diff --git a/.changeset/fresh-boxes-pull.md b/.changeset/fresh-boxes-pull.md index 87bff5c3a7..e73d7e25c2 100644 --- a/.changeset/fresh-boxes-pull.md +++ b/.changeset/fresh-boxes-pull.md @@ -2,4 +2,4 @@ '@backstage/plugin-jenkins-backend': patch --- -Use PermissionEvaluator instead of PermissionAuthorizer +Use `PermissionEvaluator` instead of `PermissionAuthorizer`, which is now deprecated. diff --git a/.changeset/rare-parents-pretend.md b/.changeset/rare-parents-pretend.md index 4c8952ad14..f211a85efe 100644 --- a/.changeset/rare-parents-pretend.md +++ b/.changeset/rare-parents-pretend.md @@ -2,23 +2,20 @@ '@backstage/create-app': patch --- -Accept `PermissionEvaluator` together with the deprecated `PermissionAuthorizer`. +Accept `PermissionEvaluator` instead of the deprecated `PermissionAuthorizer`. Apply the following to `packages/backend/src/types.ts`: ```diff -- import { ServerPermissionClient } from '@backstage/plugin-permission-node'; -+ import { -+ PermissionAuthorizer, -+ PermissionEvaluator, -+ } from '@backstage/plugin-permission-common'; +- import { PermissionAuthorizer } from '@backstage/plugin-permission-common'; ++ import { PermissionEvaluator } from '@backstage/plugin-permission-common'; export type PluginEnvironment = { ... discovery: PluginEndpointDiscovery; tokenManager: TokenManager; scheduler: PluginTaskScheduler; -- permissions: ServerPermissionClient; -+ permissions: PermissionEvaluator | PermissionAuthorizer; +- permissions: PermissionAuthorizer; ++ permissions: PermissionEvaluator; }; ``` diff --git a/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.test.ts b/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.test.ts index dbdcde11c4..51edd903e6 100644 --- a/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.test.ts +++ b/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.test.ts @@ -30,7 +30,7 @@ describe('AuthorizedEntitiesCatalog', () => { }; const fakePermissionApi = { authorize: jest.fn(), - query: jest.fn(), + authorizeConditional: jest.fn(), }; const createCatalog = (...rules: CatalogPermissionRule[]) => @@ -46,7 +46,7 @@ describe('AuthorizedEntitiesCatalog', () => { describe('entities', () => { it('returns empty response on DENY', async () => { - fakePermissionApi.query.mockResolvedValue([ + fakePermissionApi.authorizeConditional.mockResolvedValue([ { result: AuthorizeResult.DENY }, ]); const catalog = createCatalog(); @@ -62,7 +62,7 @@ describe('AuthorizedEntitiesCatalog', () => { }); it('calls underlying catalog method with correct filter on CONDITIONAL', async () => { - fakePermissionApi.query.mockResolvedValue([ + fakePermissionApi.authorizeConditional.mockResolvedValue([ { result: AuthorizeResult.CONDITIONAL, conditions: { rule: 'IS_ENTITY_KIND', params: [['b']] }, @@ -79,7 +79,7 @@ describe('AuthorizedEntitiesCatalog', () => { }); it('calls underlying catalog method on ALLOW', async () => { - fakePermissionApi.query.mockResolvedValue([ + fakePermissionApi.authorizeConditional.mockResolvedValue([ { result: AuthorizeResult.ALLOW }, ]); const catalog = createCatalog(); @@ -99,7 +99,7 @@ describe('AuthorizedEntitiesCatalog', () => { { kind: 'component', namespace: 'default', name: 'my-component' }, ], }); - fakePermissionApi.query.mockResolvedValue([ + fakePermissionApi.authorizeConditional.mockResolvedValue([ { result: AuthorizeResult.DENY }, ]); const catalog = new AuthorizedEntitiesCatalog( @@ -114,7 +114,7 @@ describe('AuthorizedEntitiesCatalog', () => { }); it('throws error on CONDITIONAL authorization that evaluates to 0 entities', async () => { - fakePermissionApi.query.mockResolvedValue([ + fakePermissionApi.authorizeConditional.mockResolvedValue([ { result: AuthorizeResult.CONDITIONAL, conditions: { rule: 'IS_ENTITY_KIND', params: [['b']] }, @@ -133,7 +133,7 @@ describe('AuthorizedEntitiesCatalog', () => { }); it('calls underlying catalog method on CONDITIONAL authorization that evaluates to nonzero entities', async () => { - fakePermissionApi.query.mockResolvedValue([ + fakePermissionApi.authorizeConditional.mockResolvedValue([ { result: AuthorizeResult.CONDITIONAL, conditions: { rule: 'IS_ENTITY_KIND', params: [['b']] }, @@ -159,7 +159,7 @@ describe('AuthorizedEntitiesCatalog', () => { { kind: 'component', namespace: 'default', name: 'my-component' }, ], }); - fakePermissionApi.query.mockResolvedValue([ + fakePermissionApi.authorizeConditional.mockResolvedValue([ { result: AuthorizeResult.ALLOW }, ]); const catalog = new AuthorizedEntitiesCatalog( @@ -253,7 +253,7 @@ describe('AuthorizedEntitiesCatalog', () => { describe('facets', () => { it('returns empty response on DENY', async () => { - fakePermissionApi.query.mockResolvedValue([ + fakePermissionApi.authorizeConditional.mockResolvedValue([ { result: AuthorizeResult.DENY }, ]); const catalog = createCatalog(); @@ -269,7 +269,7 @@ describe('AuthorizedEntitiesCatalog', () => { }); it('calls underlying catalog method with correct filter on CONDITIONAL', async () => { - fakePermissionApi.query.mockResolvedValue([ + fakePermissionApi.authorizeConditional.mockResolvedValue([ { result: AuthorizeResult.CONDITIONAL, conditions: { rule: 'IS_ENTITY_KIND', params: [['b']] }, @@ -287,7 +287,7 @@ describe('AuthorizedEntitiesCatalog', () => { }); it('calls underlying catalog method on ALLOW', async () => { - fakePermissionApi.query.mockResolvedValue([ + fakePermissionApi.authorizeConditional.mockResolvedValue([ { result: AuthorizeResult.ALLOW }, ]); const catalog = createCatalog(); diff --git a/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.ts b/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.ts index fcd05f95d8..0e861a39e1 100644 --- a/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.ts +++ b/plugins/catalog-backend/src/service/AuthorizedEntitiesCatalog.ts @@ -45,7 +45,7 @@ export class AuthorizedEntitiesCatalog implements EntitiesCatalog { async entities(request?: EntitiesRequest): Promise { const authorizeDecision = ( - await this.permissionApi.query( + await this.permissionApi.authorizeConditional( [{ permission: catalogEntityReadPermission }], { token: request?.authorizationToken }, ) @@ -78,7 +78,7 @@ export class AuthorizedEntitiesCatalog implements EntitiesCatalog { options?: { authorizationToken?: string }, ): Promise { const authorizeResponse = ( - await this.permissionApi.query( + await this.permissionApi.authorizeConditional( [{ permission: catalogEntityDeletePermission }], { token: options?.authorizationToken }, ) @@ -155,7 +155,7 @@ export class AuthorizedEntitiesCatalog implements EntitiesCatalog { async facets(request: EntityFacetsRequest): Promise { const authorizeDecision = ( - await this.permissionApi.query( + await this.permissionApi.authorizeConditional( [{ permission: catalogEntityReadPermission }], { token: request?.authorizationToken }, ) diff --git a/plugins/catalog-backend/src/service/AuthorizedLocationService.test.ts b/plugins/catalog-backend/src/service/AuthorizedLocationService.test.ts index 151ba1c3e8..dc8719bb0e 100644 --- a/plugins/catalog-backend/src/service/AuthorizedLocationService.test.ts +++ b/plugins/catalog-backend/src/service/AuthorizedLocationService.test.ts @@ -27,7 +27,7 @@ describe('AuthorizedLocationService', () => { }; const fakePermissionApi = { authorize: jest.fn(), - query: jest.fn(), + authorizeConditional: jest.fn(), }; const mockAllow = () => { diff --git a/plugins/jenkins-backend/src/service/jenkinsApi.test.ts b/plugins/jenkins-backend/src/service/jenkinsApi.test.ts index 7c4214f714..29da414470 100644 --- a/plugins/jenkins-backend/src/service/jenkinsApi.test.ts +++ b/plugins/jenkins-backend/src/service/jenkinsApi.test.ts @@ -49,7 +49,7 @@ const fakePermissionApi = { result: AuthorizeResult.ALLOW, }, ]), - query: jest.fn(), + authorizeConditional: jest.fn(), }; describe('JenkinsApi', () => { diff --git a/plugins/permission-common/api-report.md b/plugins/permission-common/api-report.md index 41abb62cf6..c0c351d6cd 100644 --- a/plugins/permission-common/api-report.md +++ b/plugins/permission-common/api-report.md @@ -163,7 +163,7 @@ export class PermissionClient implements PermissionEvaluator { requests: AuthorizePermissionRequest[], options?: EvaluatorRequestOptions, ): Promise; - query( + authorizeConditional( queries: QueryPermissionRequest[], options?: EvaluatorRequestOptions, ): Promise; @@ -192,7 +192,7 @@ export interface PermissionEvaluator { requests: AuthorizePermissionRequest[], options?: EvaluatorRequestOptions, ): Promise; - query( + authorizeConditional( requests: QueryPermissionRequest[], options?: EvaluatorRequestOptions, ): Promise; diff --git a/plugins/permission-common/src/PermissionClient.test.ts b/plugins/permission-common/src/PermissionClient.test.ts index 8da30185b7..503ce2768d 100644 --- a/plugins/permission-common/src/PermissionClient.test.ts +++ b/plugins/permission-common/src/PermissionClient.test.ts @@ -53,7 +53,7 @@ describe('PermissionClient', () => { afterEach(() => server.resetHandlers()); describe('authorize', () => { - const mockAuthorizeQuery = { + const mockAuthorizeConditional = { permission: mockPermission, resourceRef: 'foo:bar', }; @@ -78,12 +78,12 @@ describe('PermissionClient', () => { }); it('should fetch entities from correct endpoint', async () => { - await client.authorize([mockAuthorizeQuery]); + await client.authorize([mockAuthorizeConditional]); expect(mockAuthorizeHandler).toHaveBeenCalled(); }); it('should include a request body', async () => { - await client.authorize([mockAuthorizeQuery]); + await client.authorize([mockAuthorizeConditional]); const request = mockAuthorizeHandler.mock.calls[0][0]; @@ -98,21 +98,21 @@ describe('PermissionClient', () => { }); it('should return the response from the fetch request', async () => { - const response = await client.authorize([mockAuthorizeQuery]); + const response = await client.authorize([mockAuthorizeConditional]); expect(response[0]).toEqual( expect.objectContaining({ result: AuthorizeResult.ALLOW }), ); }); it('should not include authorization headers if no token is supplied', async () => { - await client.authorize([mockAuthorizeQuery]); + await client.authorize([mockAuthorizeConditional]); const request = mockAuthorizeHandler.mock.calls[0][0]; expect(request.headers.has('authorization')).toEqual(false); }); it('should include correctly-constructed authorization header if token is supplied', async () => { - await client.authorize([mockAuthorizeQuery], { token }); + await client.authorize([mockAuthorizeConditional], { token }); const request = mockAuthorizeHandler.mock.calls[0][0]; expect(request.headers.get('authorization')).toEqual('Bearer fake-token'); @@ -125,7 +125,7 @@ describe('PermissionClient', () => { }, ); await expect( - client.authorize([mockAuthorizeQuery], { token }), + client.authorize([mockAuthorizeConditional], { token }), ).rejects.toThrowError(/request failed with 401/i); }); @@ -140,7 +140,7 @@ describe('PermissionClient', () => { }, ); await expect( - client.authorize([mockAuthorizeQuery], { token }), + client.authorize([mockAuthorizeConditional], { token }), ).rejects.toThrowError(/items in response do not match request/i); }); @@ -158,7 +158,7 @@ describe('PermissionClient', () => { }, ); await expect( - client.authorize([mockAuthorizeQuery], { token }), + client.authorize([mockAuthorizeConditional], { token }), ).rejects.toThrowError(/invalid input/i); }); @@ -179,7 +179,7 @@ describe('PermissionClient', () => { discovery, config: new ConfigReader({ permission: { enabled: false } }), }); - const response = await disabled.authorize([mockAuthorizeQuery]); + const response = await disabled.authorize([mockAuthorizeConditional]); expect(response[0]).toEqual( expect.objectContaining({ result: AuthorizeResult.ALLOW }), ); @@ -203,7 +203,7 @@ describe('PermissionClient', () => { discovery, config: new ConfigReader({}), }); - const response = await disabled.authorize([mockAuthorizeQuery]); + const response = await disabled.authorize([mockAuthorizeConditional]); expect(response[0]).toEqual( expect.objectContaining({ result: AuthorizeResult.ALLOW }), ); @@ -211,8 +211,8 @@ describe('PermissionClient', () => { }); }); - describe('query', () => { - const mockResourceAuthorizeQuery = { + describe('authorizeConditional', () => { + const mockResourceAuthorizeConditional = { permission: mockPermission, }; @@ -247,12 +247,12 @@ describe('PermissionClient', () => { }); it('should fetch entities from correct endpoint', async () => { - await client.query([mockResourceAuthorizeQuery]); + await client.authorizeConditional([mockResourceAuthorizeConditional]); expect(mockPolicyDecisionHandler).toHaveBeenCalled(); }); it('should include a request body', async () => { - await client.query([mockResourceAuthorizeQuery]); + await client.authorizeConditional([mockResourceAuthorizeConditional]); const request = mockPolicyDecisionHandler.mock.calls[0][0]; @@ -266,7 +266,9 @@ describe('PermissionClient', () => { }); it('should return the response from the fetch request', async () => { - const response = await client.query([mockResourceAuthorizeQuery]); + const response = await client.authorizeConditional([ + mockResourceAuthorizeConditional, + ]); expect(response[0]).toEqual( expect.objectContaining({ result: AuthorizeResult.CONDITIONAL, @@ -280,14 +282,14 @@ describe('PermissionClient', () => { }); it('should not include authorization headers if no token is supplied', async () => { - await client.query([mockResourceAuthorizeQuery]); + await client.authorizeConditional([mockResourceAuthorizeConditional]); const request = mockPolicyDecisionHandler.mock.calls[0][0]; expect(request.headers.has('authorization')).toEqual(false); }); it('should include correctly-constructed authorization header if token is supplied', async () => { - await client.query([mockResourceAuthorizeQuery], { + await client.authorizeConditional([mockResourceAuthorizeConditional], { token, }); @@ -302,7 +304,7 @@ describe('PermissionClient', () => { }, ); await expect( - client.query([mockResourceAuthorizeQuery], { + client.authorizeConditional([mockResourceAuthorizeConditional], { token, }), ).rejects.toThrowError(/request failed with 401/i); @@ -319,7 +321,7 @@ describe('PermissionClient', () => { }, ); await expect( - client.query([mockResourceAuthorizeQuery], { + client.authorizeConditional([mockResourceAuthorizeConditional], { token, }), ).rejects.toThrowError(/items in response do not match request/i); @@ -339,7 +341,7 @@ describe('PermissionClient', () => { }, ); await expect( - client.query([mockResourceAuthorizeQuery], { + client.authorizeConditional([mockResourceAuthorizeConditional], { token, }), ).rejects.toThrowError(/invalid input/i); @@ -362,9 +364,12 @@ describe('PermissionClient', () => { discovery, config: new ConfigReader({ permission: { enabled: false } }), }); - const response = await disabled.query([mockResourceAuthorizeQuery], { - token, - }); + const response = await disabled.authorizeConditional( + [mockResourceAuthorizeConditional], + { + token, + }, + ); expect(response[0]).toEqual( expect.objectContaining({ result: AuthorizeResult.ALLOW }), ); @@ -388,9 +393,12 @@ describe('PermissionClient', () => { discovery, config: new ConfigReader({}), }); - const response = await disabled.query([mockResourceAuthorizeQuery], { - token, - }); + const response = await disabled.authorizeConditional( + [mockResourceAuthorizeConditional], + { + token, + }, + ); 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 07d60a8e65..223209c69f 100644 --- a/plugins/permission-common/src/PermissionClient.ts +++ b/plugins/permission-common/src/PermissionClient.ts @@ -131,9 +131,9 @@ export class PermissionClient implements PermissionEvaluator { } /** - * {@inheritdoc PermissionEvaluator.query} + * {@inheritdoc PermissionEvaluator.authorizeConditional} */ - async query( + async authorizeConditional( queries: QueryPermissionRequest[], options?: EvaluatorRequestOptions, ): Promise { diff --git a/plugins/permission-common/src/permissions/util.ts b/plugins/permission-common/src/permissions/util.ts index 6e0aca457b..5cc9d9c08a 100644 --- a/plugins/permission-common/src/permissions/util.ts +++ b/plugins/permission-common/src/permissions/util.ts @@ -104,7 +104,7 @@ export function toPermissionEvaluator( return response as DefinitivePolicyDecision[]; }, - query( + authorizeConditional( requests: QueryPermissionRequest[], options?: EvaluatorRequestOptions, ): Promise { diff --git a/plugins/permission-common/src/types/api.ts b/plugins/permission-common/src/types/api.ts index 0e4cf782e1..f29a639d18 100644 --- a/plugins/permission-common/src/types/api.ts +++ b/plugins/permission-common/src/types/api.ts @@ -203,7 +203,7 @@ export type AuthorizePermissionRequest = export type AuthorizePermissionResponse = DefinitivePolicyDecision; /** - * Request object for {@link PermissionEvaluator.query}. + * Request object for {@link PermissionEvaluator.authorizeConditional}. * @public */ export type QueryPermissionRequest = { @@ -212,7 +212,7 @@ export type QueryPermissionRequest = { }; /** - * Response object for {@link PermissionEvaluator.query}. + * Response object for {@link PermissionEvaluator.authorizeConditional}. * @public */ export type QueryPermissionResponse = PolicyDecision; @@ -239,7 +239,7 @@ export interface PermissionEvaluator { * backend may want to use {@link PermissionCriteria | conditions} in a database query instead of * evaluating each resource in memory. */ - query( + authorizeConditional( requests: QueryPermissionRequest[], options?: EvaluatorRequestOptions, ): Promise; diff --git a/plugins/permission-node/api-report.md b/plugins/permission-node/api-report.md index 3cf6b70d08..b2e4ff45b8 100644 --- a/plugins/permission-node/api-report.md +++ b/plugins/permission-node/api-report.md @@ -186,6 +186,11 @@ export class ServerPermissionClient implements PermissionEvaluator { options?: EvaluatorRequestOptions, ): Promise; // (undocumented) + authorizeConditional( + queries: QueryPermissionRequest[], + options?: EvaluatorRequestOptions, + ): Promise; + // (undocumented) static fromConfig( config: Config, options: { @@ -193,10 +198,5 @@ export class ServerPermissionClient implements PermissionEvaluator { tokenManager: TokenManager; }, ): ServerPermissionClient; - // (undocumented) - query( - queries: QueryPermissionRequest[], - options?: EvaluatorRequestOptions, - ): Promise; } ``` diff --git a/plugins/permission-node/src/ServerPermissionClient.test.ts b/plugins/permission-node/src/ServerPermissionClient.test.ts index 2e3e5d988f..e531bd3556 100644 --- a/plugins/permission-node/src/ServerPermissionClient.test.ts +++ b/plugins/permission-node/src/ServerPermissionClient.test.ts @@ -137,7 +137,7 @@ describe('ServerPermissionClient', () => { }); }); - describe('query', () => { + describe('authorizeConditional', () => { let mockAuthorizeHandler: jest.Mock; beforeEach(() => { @@ -162,7 +162,9 @@ describe('ServerPermissionClient', () => { tokenManager: ServerTokenManager.noop(), }); - await client.query([{ permission: testResourcePermission }]); + await client.authorizeConditional([ + { permission: testResourcePermission }, + ]); expect(mockAuthorizeHandler).not.toHaveBeenCalled(); }); @@ -174,9 +176,12 @@ describe('ServerPermissionClient', () => { tokenManager, }); - await client.query([{ permission: testResourcePermission }], { - token: (await tokenManager.getToken()).token, - }); + await client.authorizeConditional( + [{ permission: testResourcePermission }], + { + token: (await tokenManager.getToken()).token, + }, + ); expect(mockAuthorizeHandler).not.toHaveBeenCalled(); }); @@ -188,9 +193,12 @@ describe('ServerPermissionClient', () => { tokenManager, }); - await client.query([{ permission: testResourcePermission }], { - token: 'a-user-token', - }); + await client.authorizeConditional( + [{ permission: testResourcePermission }], + { + token: 'a-user-token', + }, + ); expect(mockAuthorizeHandler).toHaveBeenCalled(); }); diff --git a/plugins/permission-node/src/ServerPermissionClient.ts b/plugins/permission-node/src/ServerPermissionClient.ts index 2ea11e9e95..664e5a8309 100644 --- a/plugins/permission-node/src/ServerPermissionClient.ts +++ b/plugins/permission-node/src/ServerPermissionClient.ts @@ -78,12 +78,13 @@ export class ServerPermissionClient implements PermissionEvaluator { this.tokenManager = options.tokenManager; this.permissionEnabled = options.permissionEnabled; } - async query( + + async authorizeConditional( queries: QueryPermissionRequest[], options?: EvaluatorRequestOptions, ): Promise { return (await this.isEnabled(options?.token)) - ? this.permissionClient.query(queries, options) + ? this.permissionClient.authorizeConditional(queries, options) : queries.map(_ => ({ result: AuthorizeResult.ALLOW })); } diff --git a/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts b/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts index 5f666b3a26..256608fd82 100644 --- a/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts +++ b/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts @@ -73,12 +73,12 @@ describe('AuthorizedSearchEngine', () => { const mockedAuthorize: jest.MockedFunction = jest.fn(); const mockedPermissionQuery: jest.MockedFunction< - PermissionEvaluator['query'] + PermissionEvaluator['authorizeConditional'] > = jest.fn(); const permissionEvaluator: PermissionEvaluator = { authorize: mockedAuthorize, - query: mockedPermissionQuery, + authorizeConditional: mockedPermissionQuery, }; const defaultTypes: Record = { @@ -122,7 +122,7 @@ describe('AuthorizedSearchEngine', () => { const options = { token: 'token' }; const allowAll: PermissionEvaluator['authorize'] & - PermissionEvaluator['query'] = async queries => { + PermissionEvaluator['authorizeConditional'] = async queries => { return queries.map(() => ({ result: AuthorizeResult.ALLOW, })); diff --git a/plugins/search-backend/src/service/AuthorizedSearchEngine.ts b/plugins/search-backend/src/service/AuthorizedSearchEngine.ts index 2397734089..a74cdf8eef 100644 --- a/plugins/search-backend/src/service/AuthorizedSearchEngine.ts +++ b/plugins/search-backend/src/service/AuthorizedSearchEngine.ts @@ -93,7 +93,7 @@ export class AuthorizedSearchEngine implements SearchEngine { const conditionFetcher = new DataLoader( (requests: readonly QueryPermissionRequest[]) => - this.permissions.query(requests.slice(), options), + this.permissions.authorizeConditional(requests.slice(), options), { cacheKeyFn: ({ permission: { name } }) => name, }, diff --git a/plugins/search-backend/src/service/router.test.ts b/plugins/search-backend/src/service/router.test.ts index 673556f450..26d901e519 100644 --- a/plugins/search-backend/src/service/router.test.ts +++ b/plugins/search-backend/src/service/router.test.ts @@ -30,7 +30,7 @@ const mockPermissionEvaluator: PermissionEvaluator = { authorize: () => { throw new Error('Not implemented'); }, - query: () => { + authorizeConditional: () => { throw new Error('Not implemented'); }, };