Split PermissionClient#authorize

Co-authored-by: Mike Lewis <mtlewis@users.noreply.github.com>
Signed-off-by: Vincenzo Scamporlino <me@vinzscam.dev>

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<EvaluatePermissionRequest>) => ({
-      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<DefinitivePolicyDecision>) => ({
+            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<ConditionalPolicyDecision>) => ({
+            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();
+    });
+  });
 });

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<PolicyDecision[]> {
+    return (await this.isEnabled(options?.token))
+      ? this.permissionClient.query(queries, options)
+      : queries.map(_ => ({ result: AuthorizeResult.ALLOW }));
+  }
 
   async authorize(
-    requests: EvaluatePermissionRequest[],
-    options?: AuthorizeRequestOptions,
-  ): Promise<EvaluatePermissionResponse[]> {
-    // 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<AuthorizePermissionResponse[]> {
+    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));
+  }
 }

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';

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.
  *