From 9cbb270aef79d7b642a51906df58343b4a97b3d3 Mon Sep 17 00:00:00 2001 From: Mike Lewis Date: Fri, 4 Mar 2022 14:51:53 +0000 Subject: [PATCH] permissions: add a discriminator type to Permission Signed-off-by: Mike Lewis --- .changeset/pink-horses-cough.md | 7 +++ .changeset/unlucky-schools-heal.md | 3 +- .../PermissionApi/MockPermissionApi.test.ts | 10 +++- .../src/service/router.test.ts | 57 ++++++++++++++++++- .../permission-backend/src/service/router.ts | 41 ++++++++----- plugins/permission-common/api-report.md | 23 +++++--- .../src/permissions/createPermission.ts | 21 ++++++- plugins/permission-common/src/types/index.ts | 1 + .../permission-common/src/types/permission.ts | 48 +++++++++------- .../src/components/PermissionedRoute.test.tsx | 5 +- .../src/hooks/usePermission.test.tsx | 11 ++-- .../service/AuthorizedSearchEngine.test.ts | 17 +++--- 12 files changed, 181 insertions(+), 63 deletions(-) create mode 100644 .changeset/pink-horses-cough.md diff --git a/.changeset/pink-horses-cough.md b/.changeset/pink-horses-cough.md new file mode 100644 index 0000000000..dc49ba6b83 --- /dev/null +++ b/.changeset/pink-horses-cough.md @@ -0,0 +1,7 @@ +--- +'@backstage/plugin-permission-react': patch +'@backstage/plugin-search-backend': patch +'@backstage/test-utils': patch +--- + +Use `createPermission` helper to create valid `Permission` objects in test suites. diff --git a/.changeset/unlucky-schools-heal.md b/.changeset/unlucky-schools-heal.md index 1f2930b29a..55daf4e65c 100644 --- a/.changeset/unlucky-schools-heal.md +++ b/.changeset/unlucky-schools-heal.md @@ -2,4 +2,5 @@ '@backstage/plugin-permission-backend': patch --- -Add more specific check for policies which return conditional decisions for non-resource permissions. +- Add more specific check for policies which return conditional decisions for non-resource permissions. +- Refine permission validation in authorize endpoint to differentiate between `BasicPermission` and `ResourcePermission` instances. diff --git a/packages/test-utils/src/testUtils/apis/PermissionApi/MockPermissionApi.test.ts b/packages/test-utils/src/testUtils/apis/PermissionApi/MockPermissionApi.test.ts index 88e7c995f5..b617d89cc4 100644 --- a/packages/test-utils/src/testUtils/apis/PermissionApi/MockPermissionApi.test.ts +++ b/packages/test-utils/src/testUtils/apis/PermissionApi/MockPermissionApi.test.ts @@ -16,7 +16,7 @@ import { AuthorizeResult, - Permission, + createPermission, } from '@backstage/plugin-permission-common'; import { MockPermissionApi } from './MockPermissionApi'; @@ -25,7 +25,9 @@ describe('MockPermissionApi', () => { const api = new MockPermissionApi(); await expect( - api.authorize({ permission: { name: 'permission.1' } as Permission }), + api.authorize({ + permission: createPermission({ name: 'permission.1', attributes: {} }), + }), ).resolves.toEqual({ result: AuthorizeResult.ALLOW }); }); @@ -37,7 +39,9 @@ describe('MockPermissionApi', () => { ); await expect( - api.authorize({ permission: { name: 'permission.2' } as Permission }), + api.authorize({ + permission: createPermission({ name: 'permission.2', attributes: {} }), + }), ).resolves.toEqual({ result: AuthorizeResult.DENY }); }); }); diff --git a/plugins/permission-backend/src/service/router.test.ts b/plugins/permission-backend/src/service/router.test.ts index b98b32b01c..6c6ad14af4 100644 --- a/plugins/permission-backend/src/service/router.test.ts +++ b/plugins/permission-backend/src/service/router.test.ts @@ -110,6 +110,7 @@ describe('createRouter', () => { { id: '123', permission: { + type: 'basic', name: 'test.permission1', attributes: {}, }, @@ -117,6 +118,7 @@ describe('createRouter', () => { { id: '234', permission: { + type: 'basic', name: 'test.permission2', attributes: {}, }, @@ -129,6 +131,7 @@ describe('createRouter', () => { expect(policy.handle).toHaveBeenCalledWith( { permission: { + type: 'basic', name: 'test.permission1', attributes: {}, }, @@ -138,6 +141,7 @@ describe('createRouter', () => { expect(policy.handle).toHaveBeenCalledWith( { permission: { + type: 'basic', name: 'test.permission2', attributes: {}, }, @@ -163,6 +167,7 @@ describe('createRouter', () => { { id: '123', permission: { + type: 'basic', name: 'test.permission', attributes: {}, }, @@ -174,6 +179,7 @@ describe('createRouter', () => { expect(policy.handle).toHaveBeenCalledWith( { permission: { + type: 'basic', name: 'test.permission', attributes: {}, }, @@ -201,6 +207,7 @@ describe('createRouter', () => { { id: '123', permission: { + type: 'resource', name: 'test.permission', resourceType: 'test-resource-1', attributes: {}, @@ -258,6 +265,7 @@ describe('createRouter', () => { { id: '123', permission: { + type: 'resource', name: 'test.permission.1', resourceType: 'test-resource-1', attributes: {}, @@ -267,6 +275,7 @@ describe('createRouter', () => { { id: '234', permission: { + type: 'resource', name: 'test.permission.2', resourceType: 'test-resource-2', attributes: {}, @@ -276,6 +285,7 @@ describe('createRouter', () => { { id: '345', permission: { + type: 'resource', name: 'test.permission.3', resourceType: 'test-resource-1', attributes: {}, @@ -285,6 +295,7 @@ describe('createRouter', () => { { id: '456', permission: { + type: 'resource', name: 'test.permission.4', resourceType: 'test-resource-2', attributes: {}, @@ -384,6 +395,7 @@ describe('createRouter', () => { { id: '123', permission: { + type: 'resource', name: 'test.permission.1', resourceType: 'test-resource-1', attributes: {}, @@ -393,6 +405,7 @@ describe('createRouter', () => { { id: '234', permission: { + type: 'resource', name: 'test.permission.2', resourceType: 'test-resource-2', attributes: {}, @@ -402,6 +415,7 @@ describe('createRouter', () => { { id: '345', permission: { + type: 'resource', name: 'test.permission.3', resourceType: 'test-resource-1', attributes: {}, @@ -411,6 +425,7 @@ describe('createRouter', () => { { id: '456', permission: { + type: 'resource', name: 'test.permission.4', resourceType: 'test-resource-1', attributes: {}, @@ -420,6 +435,7 @@ describe('createRouter', () => { { id: '567', permission: { + type: 'resource', name: 'test.permission.5', resourceType: 'test-resource-2', attributes: {}, @@ -429,6 +445,7 @@ describe('createRouter', () => { { id: '678', permission: { + type: 'basic', name: 'test.permission.6', attributes: {}, }, @@ -519,6 +536,7 @@ describe('createRouter', () => { { id: '123', permission: { + type: 'resource', name: 'test.permission.1', resourceType: 'test-resource-1', attributes: {}, @@ -528,6 +546,7 @@ describe('createRouter', () => { { id: '234', permission: { + type: 'resource', name: 'test.permission.2', resourceType: 'test-resource-2', attributes: {}, @@ -537,6 +556,7 @@ describe('createRouter', () => { { id: '345', permission: { + type: 'resource', name: 'test.permission.3', resourceType: 'test-resource-1', attributes: {}, @@ -546,6 +566,7 @@ describe('createRouter', () => { { id: '456', permission: { + type: 'resource', name: 'test.permission.4', resourceType: 'test-resource-1', attributes: {}, @@ -630,6 +651,7 @@ describe('createRouter', () => { id: '123', resourceRef: 'test/resource', permission: { + type: 'resource', name: 'test.permission', resourceType: 'test-resource-1', attributes: {}, @@ -639,6 +661,7 @@ describe('createRouter', () => { id: '234', resourceRef: 'test/resource', permission: { + type: 'resource', name: 'test.permission', resourceType: 'test-resource-1', attributes: {}, @@ -687,10 +710,37 @@ describe('createRouter', () => { undefined, '', {}, - [{ permission: { name: 'test.permission', attributes: {} } }], - { items: [{ permission: { name: 'test.permission', attributes: {} } }] }, + [ + { + permission: { + type: 'basic', + name: 'test.permission', + attributes: {}, + }, + }, + ], + { + items: [ + { + permission: { + type: 'basic', + name: 'test.permission', + attributes: {}, + }, + }, + ], + }, { items: [{ id: '123' }] }, - { items: [{ id: '123', permission: { name: 'test.permission' } }] }, + { + items: [ + { + id: '123', + permission: { name: 'test.permission', attributes: {} }, + }, + ], + }, + { items: [{ id: '123', permission: { type: 'basic', attributes: {} } }] }, + { items: [{ id: '123', permission: { type: 'basic' } }] }, { items: [ { id: '123', permission: { attributes: { invalid: 'attribute' } } }, @@ -724,6 +774,7 @@ describe('createRouter', () => { { id: '123', permission: { + type: 'resource', name: 'test.permission', resourceType: 'test-resource-1', attributes: {}, diff --git a/plugins/permission-backend/src/service/router.ts b/plugins/permission-backend/src/service/router.ts index 4b5b8f5abb..034ff0b9ed 100644 --- a/plugins/permission-backend/src/service/router.ts +++ b/plugins/permission-backend/src/service/router.ts @@ -36,6 +36,7 @@ import { AuthorizeRequest, AuthorizeResponse, isResourcePermission, + PermissionAttributes, } from '@backstage/plugin-permission-common'; import { ApplyConditionsRequestEntry, @@ -47,23 +48,35 @@ import { memoize } from 'lodash'; import DataLoader from 'dataloader'; import { Config } from '@backstage/config'; +const attributesSchema: z.ZodSchema = z.object({ + action: z + .union([ + z.literal('create'), + z.literal('read'), + z.literal('update'), + z.literal('delete'), + ]) + .optional(), +}); + +const permissionSchema = z.union([ + z.object({ + type: z.literal('basic'), + name: z.string(), + attributes: attributesSchema, + }), + z.object({ + type: z.literal('resource'), + name: z.string(), + attributes: attributesSchema, + resourceType: z.string(), + }), +]); + const querySchema: z.ZodSchema> = z.object({ id: z.string(), resourceRef: z.string().optional(), - permission: z.object({ - name: z.string(), - resourceType: z.string().optional(), - attributes: z.object({ - action: z - .union([ - z.literal('create'), - z.literal('read'), - z.literal('update'), - z.literal('delete'), - ]) - .optional(), - }), - }), + permission: permissionSchema, }); const requestSchema: z.ZodSchema = z.object({ diff --git a/plugins/permission-common/api-report.md b/plugins/permission-common/api-report.md index d58d89f9cc..77ab94b4e3 100644 --- a/plugins/permission-common/api-report.md +++ b/plugins/permission-common/api-report.md @@ -54,10 +54,7 @@ export enum AuthorizeResult { } // @public -export type BasicPermission = { - name: string; - attributes: PermissionAttributes; -}; +export type BasicPermission = PermissionBase<'basic', {}>; // @public export function createPermission(input: { @@ -122,6 +119,14 @@ export interface PermissionAuthorizer { ): Promise; } +// @public +export type PermissionBase = { + name: string; + attributes: PermissionAttributes; +} & { + type: TType; +} & TFields; + // @public export class PermissionClient implements PermissionAuthorizer { constructor(options: { discovery: DiscoveryApi; config: Config }); @@ -145,7 +150,11 @@ export type PermissionCriteria = | TQuery; // @public -export type ResourcePermission = BasicPermission & { - resourceType: T; -}; +export type ResourcePermission = + PermissionBase< + 'resource', + { + resourceType: TResourceType; + } + >; ``` diff --git a/plugins/permission-common/src/permissions/createPermission.ts b/plugins/permission-common/src/permissions/createPermission.ts index b3e918ac85..586403ceb4 100644 --- a/plugins/permission-common/src/permissions/createPermission.ts +++ b/plugins/permission-common/src/permissions/createPermission.ts @@ -41,10 +41,27 @@ export function createPermission(input: { name: string; attributes: PermissionAttributes; }): BasicPermission; -export function createPermission(input: { +export function createPermission({ + name, + attributes, + resourceType, +}: { name: string; attributes: PermissionAttributes; resourceType?: string; }): Permission { - return input; + if (resourceType) { + return { + type: 'resource', + name, + attributes, + resourceType, + }; + } + + return { + type: 'basic', + name, + attributes, + }; } diff --git a/plugins/permission-common/src/types/index.ts b/plugins/permission-common/src/types/index.ts index 2fdb553f09..71ef7695b8 100644 --- a/plugins/permission-common/src/types/index.ts +++ b/plugins/permission-common/src/types/index.ts @@ -33,6 +33,7 @@ export type { PermissionAttributes, Permission, PermissionAuthorizer, + PermissionBase, ResourcePermission, AuthorizeRequestOptions, } from './permission'; diff --git a/plugins/permission-common/src/types/permission.ts b/plugins/permission-common/src/types/permission.ts index 0777e24eb4..ff4e1e1e21 100644 --- a/plugins/permission-common/src/types/permission.ts +++ b/plugins/permission-common/src/types/permission.ts @@ -25,6 +25,23 @@ export type PermissionAttributes = { action?: 'create' | 'read' | 'update' | 'delete'; }; +/** + * Generic type for building {@link Permission} types. + * @public + */ +export type PermissionBase = { + /** + * The name of the permission. + */ + name: string; + /** + * {@link PermissionAttributes} which describe characteristics of the permission, to help + * policy authors make consistent decisions for similar permissions without referring to them + * all by name. + */ + attributes: PermissionAttributes; +} & { type: TType } & TFields; + /** * A permission that can be checked through authorization. * @@ -44,31 +61,24 @@ export type Permission = BasicPermission | ResourcePermission; * A standard {@link Permission} with no additional capabilities or restrictions. * @public */ -export type BasicPermission = { - /** - * The name of the permission. - */ - name: string; - /** - * {@link PermissionAttributes} which describe characteristics of the permission, to help - * policy authors make consistent decisions for similar permissions without referring to them - * all by name. - */ - attributes: PermissionAttributes; -}; +export type BasicPermission = PermissionBase<'basic', {}>; /** * ResourcePermissions are {@link Permission}s that can be authorized based on * characteristics of a resource such a catalog entity. * @public */ -export type ResourcePermission = BasicPermission & { - /** - * Denotes the type of the resource whose resourceRef should be passed when - * authorizing. - */ - resourceType: T; -}; +export type ResourcePermission = + PermissionBase< + 'resource', + { + /** + * Denotes the type of the resource whose resourceRef should be passed when + * authorizing. + */ + resourceType: TResourceType; + } + >; /** * A client interacting with the permission backend can implement this authorizer interface. diff --git a/plugins/permission-react/src/components/PermissionedRoute.test.tsx b/plugins/permission-react/src/components/PermissionedRoute.test.tsx index 3d7d1b7af7..45909b44de 100644 --- a/plugins/permission-react/src/components/PermissionedRoute.test.tsx +++ b/plugins/permission-react/src/components/PermissionedRoute.test.tsx @@ -18,6 +18,7 @@ import React from 'react'; import { PermissionedRoute } from '.'; import { usePermission } from '../hooks'; import { renderInTestApp } from '@backstage/test-utils'; +import { createPermission } from '@backstage/plugin-permission-common'; jest.mock('../hooks', () => ({ usePermission: jest.fn(), @@ -26,10 +27,10 @@ const mockUsePermission = usePermission as jest.MockedFunction< typeof usePermission >; -const permission = { +const permission = createPermission({ name: 'access.something', attributes: { action: 'read' as const }, -}; +}); describe('PermissionedRoute', () => { it('Does not render when loading', async () => { diff --git a/plugins/permission-react/src/hooks/usePermission.test.tsx b/plugins/permission-react/src/hooks/usePermission.test.tsx index d6c5b2ed33..f67f961dd2 100644 --- a/plugins/permission-react/src/hooks/usePermission.test.tsx +++ b/plugins/permission-react/src/hooks/usePermission.test.tsx @@ -17,15 +17,18 @@ import React, { FC } from 'react'; import { render } from '@testing-library/react'; import { usePermission } from './usePermission'; -import { AuthorizeResult } from '@backstage/plugin-permission-common'; +import { + AuthorizeResult, + createPermission, +} from '@backstage/plugin-permission-common'; import { TestApiProvider } from '@backstage/test-utils'; import { PermissionApi, permissionApiRef } from '../apis'; import { SWRConfig } from 'swr'; -const permission = { +const permission = createPermission({ name: 'access.something', - attributes: { action: 'read' as const }, -}; + attributes: { action: 'read' }, +}); const TestComponent: FC = () => { const { loading, allowed, error } = usePermission(permission); diff --git a/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts b/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts index ba63f8484e..76b3cf5184 100644 --- a/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts +++ b/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts @@ -18,6 +18,7 @@ import { ConfigReader } from '@backstage/config'; import { AuthorizeDecision, AuthorizeResult, + createPermission, PermissionAuthorizer, } from '@backstage/plugin-permission-common'; import { @@ -78,28 +79,28 @@ describe('AuthorizedSearchEngine', () => { const defaultTypes: Record = { [typeUsers]: { - visibilityPermission: { + visibilityPermission: createPermission({ name: 'search.users.read', attributes: { action: 'read' }, - }, + }), }, [typeTemplates]: { - visibilityPermission: { + visibilityPermission: createPermission({ name: 'search.templates.read', attributes: { action: 'read' }, - }, + }), }, [typeServices]: { - visibilityPermission: { + visibilityPermission: createPermission({ name: 'search.services.read', attributes: { action: 'read' }, - }, + }), }, [typeGroups]: { - visibilityPermission: { + visibilityPermission: createPermission({ name: 'search.groups.read', attributes: { action: 'read' }, - }, + }), }, };