Reintroduce noop token manager and refactor ServerPermissionClient

Signed-off-by: Joon Park <joonp@spotify.com>

plugins/permission-node/api-report.md

@@ -5,7 +5,6 @@
 ```ts
 import { AuthorizeRequest } from '@backstage/plugin-permission-common';
 import { AuthorizeRequestOptions } from '@backstage/plugin-permission-common';
-import { AuthorizeResponse } from '@backstage/plugin-permission-common';
 import { AuthorizeResult } from '@backstage/plugin-permission-common';
 import { BackstageIdentityResponse } from '@backstage/plugin-auth-backend';
 import { Config } from '@backstage/config';
@@ -14,7 +13,7 @@ import { PermissionClient } from '@backstage/plugin-permission-common';
 import { PermissionCondition } from '@backstage/plugin-permission-common';
 import { PermissionCriteria } from '@backstage/plugin-permission-common';
 import { Router } from 'express';
-import { ServerTokenManager } from '@backstage/backend-common';
+import { TokenManager } from '@backstage/backend-common';
 
 // @public
 export type ApplyConditionsRequest = {
@@ -131,12 +130,9 @@ export class ServerPermissionClient extends PermissionClient {
   constructor(options: {
     discoveryApi: DiscoveryApi;
     configApi: Config;
-    serverTokenManager: ServerTokenManager;
+    serverTokenManager: TokenManager;
   });
   // (undocumented)
-  authorize(
-    requests: AuthorizeRequest[],
-    options?: AuthorizeRequestOptions,
-  ): Promise<AuthorizeResponse[]>;
+  shouldBypass(options?: AuthorizeRequestOptions): Promise<boolean>;
 }
 ```

plugins/permission-node/package.json

@@ -40,6 +40,7 @@
   "devDependencies": {
     "@backstage/cli": "^0.10.1",
     "@types/supertest": "^2.0.8",
+    "msw": "^0.35.0",
     "supertest": "^6.1.3"
   },
   "files": [

plugins/permission-node/src/ServerPermissionClient.test.ts

@@ -0,0 +1,118 @@
+/*
+ * Copyright 2021 The Backstage Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { ServerPermissionClient } from '.';
+import {
+  DiscoveryApi,
+  Permission,
+  Identified,
+  AuthorizeRequest,
+  AuthorizeResult,
+} from '@backstage/plugin-permission-common';
+import { ConfigReader } from '@backstage/config';
+import { getVoidLogger, ServerTokenManager } from '@backstage/backend-common';
+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.map((r: Identified<AuthorizeRequest>) => ({
+    id: r.id,
+    result: AuthorizeResult.ALLOW,
+  }));
+
+  return res(json(responses));
+});
+const mockBaseUrl = 'http://backstage:9191/i-am-a-mock-base';
+const discoveryApi: DiscoveryApi = {
+  async getBaseUrl() {
+    return mockBaseUrl;
+  },
+};
+const testPermission: Permission = {
+  name: 'test.permission',
+  attributes: {},
+  resourceType: 'test-resource',
+};
+const config = new ConfigReader({
+  permission: { enabled: true },
+  backend: { auth: { keys: [{ secret: 'a-secret-key' }] } },
+});
+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 = new ServerPermissionClient({
+      discoveryApi,
+      configApi: new ConfigReader({}),
+      serverTokenManager: 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 = new ServerPermissionClient({
+      discoveryApi,
+      configApi: config,
+      serverTokenManager: 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 = new ServerPermissionClient({
+      discoveryApi,
+      configApi: config,
+      serverTokenManager: 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(
+      () =>
+        new ServerPermissionClient({
+          discoveryApi,
+          configApi: config,
+          serverTokenManager: ServerTokenManager.noop(),
+        }),
+    ).toThrowError(
+      'You must configure at least one key in backend.auth.keys if permissions are enabled.',
+    );
+  });
+});

plugins/permission-node/src/ServerPermissionClient.ts

@@ -14,13 +14,10 @@
  * limitations under the License.
  */
 
-import { ServerTokenManager } from '@backstage/backend-common';
+import { TokenManager, ServerTokenManager } from '@backstage/backend-common';
 import { Config } from '@backstage/config';
 import {
-  AuthorizeRequest,
   AuthorizeRequestOptions,
-  AuthorizeResponse,
-  AuthorizeResult,
   DiscoveryApi,
   PermissionClient,
 } from '@backstage/plugin-permission-common';
@@ -31,26 +28,36 @@ import {
  * @public
  */
 export class ServerPermissionClient extends PermissionClient {
-  private readonly serverTokenManager: ServerTokenManager;
+  private readonly serverTokenManager: TokenManager;
 
   constructor(options: {
     discoveryApi: DiscoveryApi;
     configApi: Config;
-    serverTokenManager: ServerTokenManager;
+    serverTokenManager: TokenManager;
   }) {
     const { discoveryApi, configApi, serverTokenManager } = options;
     super({ discoveryApi, configApi });
+
+    if (this.enabled && !(serverTokenManager instanceof ServerTokenManager)) {
+      throw new Error(
+        'You must configure at least one key in backend.auth.keys if permissions are enabled.',
+      );
+    }
     this.serverTokenManager = serverTokenManager;
   }
 
-  async authorize(
-    requests: AuthorizeRequest[],
-    options?: AuthorizeRequestOptions,
-  ): Promise<AuthorizeResponse[]> {
-    if (await this.isValidServerToken(options?.token)) {
-      return requests.map(_ => ({ result: AuthorizeResult.ALLOW }));
+  async shouldBypass(options?: AuthorizeRequestOptions): Promise<boolean> {
+    // Call super first in order to 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 (await super.shouldBypass(options)) {
+      return true;
     }
-    return super.authorize(requests, options);
+    if (await this.isValidServerToken(options?.token)) {
+      return true;
+    }
+    return false;
   }
 
   private async isValidServerToken(