From 6b8713df35c525edb4c1e327230ded591ee6878b Mon Sep 17 00:00:00 2001 From: Joon Park Date: Thu, 2 Dec 2021 10:26:44 +0000 Subject: [PATCH 01/24] Create ServerPermissionClient and add it to example backend Signed-off-by: Joon Park --- packages/backend/package.json | 3 + packages/backend/src/index.ts | 22 ++++++- packages/backend/src/plugins/permission.ts | 48 ++++++++++++++ packages/backend/src/types.ts | 2 + plugins/permission-node/package.json | 2 + .../src/ServerPermissionClient.ts | 62 +++++++++++++++++++ plugins/permission-node/src/index.ts | 1 + 7 files changed, 138 insertions(+), 2 deletions(-) create mode 100644 packages/backend/src/plugins/permission.ts create mode 100644 plugins/permission-node/src/ServerPermissionClient.ts diff --git a/packages/backend/package.json b/packages/backend/package.json index 80ae3b0998..d5249c28bc 100644 --- a/packages/backend/package.json +++ b/packages/backend/package.json @@ -39,6 +39,9 @@ "@backstage/plugin-jenkins-backend": "^0.1.9", "@backstage/plugin-kubernetes-backend": "^0.4.0", "@backstage/plugin-kafka-backend": "^0.2.12", + "@backstage/plugin-permission-backend": "^0.1.0", + "@backstage/plugin-permission-common": "^0.2.0", + "@backstage/plugin-permission-node": "^0.2.0", "@backstage/plugin-proxy-backend": "^0.2.14", "@backstage/plugin-rollbar-backend": "^0.1.16", "@backstage/plugin-scaffolder-backend": "^0.15.17", diff --git a/packages/backend/src/index.ts b/packages/backend/src/index.ts index 4426ed7767..ba38f8c427 100644 --- a/packages/backend/src/index.ts +++ b/packages/backend/src/index.ts @@ -55,13 +55,20 @@ import graphql from './plugins/graphql'; import app from './plugins/app'; import badges from './plugins/badges'; import jenkins from './plugins/jenkins'; +import permission from './plugins/permission'; import { PluginEnvironment } from './types'; +import { ServerPermissionClient } from '@backstage/plugin-permission-node'; function makeCreateEnv(config: Config) { const root = getRootLogger(); const reader = UrlReaders.default({ logger: root, config }); const discovery = SingleHostDiscovery.fromConfig(config); - const tokenManager = ServerTokenManager.noop(); + const tokenManager = ServerTokenManager.fromConfig(config); + const permissions = new ServerPermissionClient({ + discoveryApi: discovery, + configApi: config, + serverTokenManager: tokenManager, + }); root.info(`Created UrlReader ${reader}`); @@ -72,7 +79,16 @@ function makeCreateEnv(config: Config) { const logger = root.child({ type: 'plugin', plugin }); const database = databaseManager.forPlugin(plugin); const cache = cacheManager.forPlugin(plugin); - return { logger, cache, database, config, reader, discovery, tokenManager }; + return { + logger, + cache, + database, + config, + reader, + discovery, + tokenManager, + permissions, + }; }; } @@ -113,6 +129,7 @@ async function main() { const techInsightsEnv = useHotMemoize(module, () => createEnv('tech-insights'), ); + const permissionEnv = useHotMemoize(module, () => createEnv('permission')); const apiRouter = Router(); apiRouter.use('/catalog', await catalog(catalogEnv)); @@ -131,6 +148,7 @@ async function main() { apiRouter.use('/graphql', await graphql(graphqlEnv)); apiRouter.use('/badges', await badges(badgesEnv)); apiRouter.use('/jenkins', await jenkins(jenkinsEnv)); + apiRouter.use('/permission', await permission(permissionEnv)); apiRouter.use(notFoundHandler()); const service = createServiceBuilder(module) diff --git a/packages/backend/src/plugins/permission.ts b/packages/backend/src/plugins/permission.ts new file mode 100644 index 0000000000..71a9b90311 --- /dev/null +++ b/packages/backend/src/plugins/permission.ts @@ -0,0 +1,48 @@ +/* + * 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 { IdentityClient } from '@backstage/plugin-auth-backend'; +import { createRouter } from '@backstage/plugin-permission-backend'; +import { AuthorizeResult } from '@backstage/plugin-permission-common'; +import { + PermissionPolicy, + PolicyDecision, +} from '@backstage/plugin-permission-node'; +import { Router } from 'express'; +import { PluginEnvironment } from '../types'; + +class AllowAllPermissionPolicy implements PermissionPolicy { + async handle(): Promise { + return { + result: AuthorizeResult.ALLOW, + }; + } +} + +export default async function createPlugin( + env: PluginEnvironment, +): Promise { + const { logger, discovery } = env; + return await createRouter({ + logger, + discovery, + policy: new AllowAllPermissionPolicy(), + identity: new IdentityClient({ + discovery, + issuer: await discovery.getExternalBaseUrl('auth'), + }), + }); +} diff --git a/packages/backend/src/types.ts b/packages/backend/src/types.ts index 4be9c036b3..e3826d69ae 100644 --- a/packages/backend/src/types.ts +++ b/packages/backend/src/types.ts @@ -23,6 +23,7 @@ import { TokenManager, UrlReader, } from '@backstage/backend-common'; +import { ServerPermissionClient } from '@backstage/plugin-permission-node'; export type PluginEnvironment = { logger: Logger; @@ -32,4 +33,5 @@ export type PluginEnvironment = { reader: UrlReader; discovery: PluginEndpointDiscovery; tokenManager: TokenManager; + permissions: ServerPermissionClient; }; diff --git a/plugins/permission-node/package.json b/plugins/permission-node/package.json index 5e0a300873..7166ae5c31 100644 --- a/plugins/permission-node/package.json +++ b/plugins/permission-node/package.json @@ -29,6 +29,8 @@ "clean": "backstage-cli clean" }, "dependencies": { + "@backstage/backend-common": "^0.9.11", + "@backstage/config": "^0.1.11", "@backstage/plugin-auth-backend": "^0.5.0", "@backstage/plugin-permission-common": "^0.2.0", "@types/express": "^4.17.6", diff --git a/plugins/permission-node/src/ServerPermissionClient.ts b/plugins/permission-node/src/ServerPermissionClient.ts new file mode 100644 index 0000000000..1e54d73808 --- /dev/null +++ b/plugins/permission-node/src/ServerPermissionClient.ts @@ -0,0 +1,62 @@ +/* + * 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 { ServerTokenManager } from '@backstage/backend-common'; +import { Config } from '@backstage/config'; +import { + AuthorizeRequest, + AuthorizeRequestOptions, + AuthorizeResponse, + AuthorizeResult, + DiscoveryApi, + PermissionClient, +} from '@backstage/plugin-permission-common'; + +export class ServerPermissionClient extends PermissionClient { + private readonly serverTokenManager: ServerTokenManager; + + constructor(options: { + discoveryApi: DiscoveryApi; + configApi: Config; + serverTokenManager: ServerTokenManager; + }) { + const { discoveryApi, configApi, serverTokenManager } = options; + super({ discoveryApi, configApi }); + this.serverTokenManager = serverTokenManager; + } + + async authorize( + requests: AuthorizeRequest[], + options?: AuthorizeRequestOptions, + ): Promise { + if (await this.isValidServerToken(options?.token)) { + return requests.map(_ => ({ result: AuthorizeResult.ALLOW })); + } + return super.authorize(requests, options); + } + + private async isValidServerToken( + token: string | undefined, + ): Promise { + if (!token) { + return false; + } + return this.serverTokenManager + .authenticate(token) + .then(() => true) + .catch(() => false); + } +} diff --git a/plugins/permission-node/src/index.ts b/plugins/permission-node/src/index.ts index 39527bac71..32d71ddeb3 100644 --- a/plugins/permission-node/src/index.ts +++ b/plugins/permission-node/src/index.ts @@ -22,3 +22,4 @@ export * from './integration'; export * from './policy'; export * from './types'; +export { ServerPermissionClient } from './ServerPermissionClient'; From 3d15cde451569a41c1ca58e7acaec45415f7f0c9 Mon Sep 17 00:00:00 2001 From: Joon Park Date: Fri, 3 Dec 2021 11:03:29 +0000 Subject: [PATCH 02/24] Add permissionApiRef to app APIs Signed-off-by: Joon Park --- packages/app/package.json | 1 + packages/app/src/apis.ts | 17 +++++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/packages/app/package.json b/packages/app/package.json index a9e8e8ddc8..5dbb326538 100644 --- a/packages/app/package.json +++ b/packages/app/package.json @@ -35,6 +35,7 @@ "@backstage/plugin-newrelic": "^0.3.10", "@backstage/plugin-org": "^0.3.31", "@backstage/plugin-pagerduty": "0.3.19", + "@backstage/plugin-permission-react": "^0.1.1", "@backstage/plugin-rollbar": "^0.3.20", "@backstage/plugin-scaffolder": "^0.11.14", "@backstage/plugin-search": "^0.5.1", diff --git a/packages/app/src/apis.ts b/packages/app/src/apis.ts index d587a88402..f67a7b2649 100644 --- a/packages/app/src/apis.ts +++ b/packages/app/src/apis.ts @@ -31,9 +31,15 @@ import { AnyApiFactory, configApiRef, createApiFactory, + discoveryApiRef, errorApiRef, githubAuthApiRef, + identityApiRef, } from '@backstage/core-plugin-api'; +import { + permissionApiRef, + IdentityPermissionApi, +} from '@backstage/plugin-permission-react'; export const apis: AnyApiFactory[] = [ createApiFactory({ @@ -64,4 +70,15 @@ export const apis: AnyApiFactory[] = [ }), createApiFactory(costInsightsApiRef, new ExampleCostInsightsClient()), + + createApiFactory({ + api: permissionApiRef, + deps: { + discoveryApi: discoveryApiRef, + identityApi: identityApiRef, + configApi: configApiRef, + }, + factory: ({ configApi, discoveryApi, identityApi }) => + IdentityPermissionApi.create({ configApi, discoveryApi, identityApi }), + }), ]; From f786dc81407772b8b7ad1e60069dd58fca648792 Mon Sep 17 00:00:00 2001 From: Joon Park Date: Mon, 6 Dec 2021 18:12:07 +0000 Subject: [PATCH 03/24] Generate secret in development Signed-off-by: Joon Park --- packages/backend-common/api-report.md | 7 +- .../src/tokens/ServerTokenManager.test.ts | 220 ++++++++++-------- .../src/tokens/ServerTokenManager.ts | 51 ++-- packages/backend/src/index.ts | 2 +- plugins/permission-node/api-report.md | 24 +- 5 files changed, 190 insertions(+), 114 deletions(-) diff --git a/packages/backend-common/api-report.md b/packages/backend-common/api-report.md index d20f568e98..32d0df1148 100644 --- a/packages/backend-common/api-report.md +++ b/packages/backend-common/api-report.md @@ -491,13 +491,14 @@ export class ServerTokenManager implements TokenManager { // (undocumented) authenticate(token: string): Promise; // (undocumented) - static fromConfig(config: Config): ServerTokenManager; + static default(options: { + config: Config; + logger: Logger_2; + }): ServerTokenManager; // (undocumented) getToken(): Promise<{ token: string; }>; - // (undocumented) - static noop(): TokenManager; } // @public (undocumented) diff --git a/packages/backend-common/src/tokens/ServerTokenManager.test.ts b/packages/backend-common/src/tokens/ServerTokenManager.test.ts index fe724a0a4b..2bdd02c9f5 100644 --- a/packages/backend-common/src/tokens/ServerTokenManager.test.ts +++ b/packages/backend-common/src/tokens/ServerTokenManager.test.ts @@ -13,49 +13,68 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +import { getVoidLogger } from '../logging/voidLogger'; import { ConfigReader } from '@backstage/config'; -import { TokenManager } from './types'; import { ServerTokenManager } from './ServerTokenManager'; +import { Logger } from 'winston'; +import { JWK } from 'jose'; const emptyConfig = new ConfigReader({}); const configWithSecret = new ConfigReader({ backend: { auth: { keys: [{ secret: 'a-secret-key' }] } }, }); +const env = process.env; +let logger: Logger; describe('ServerTokenManager', () => { - it('should throw if secret in config does not exist', () => { - expect(() => ServerTokenManager.fromConfig(emptyConfig)).toThrowError(); + beforeEach(() => { + process.env = { ...env }; + logger = getVoidLogger(); + }); + + afterEach(() => { + process.env = env; }); describe('getToken', () => { - it('should return a token if secret in config exists', async () => { - const tokenManager = ServerTokenManager.fromConfig(configWithSecret); - expect((await tokenManager.getToken()).token).toBeDefined(); - }); - - it('should return a token string if using a noop TokenManager', async () => { - const tokenManager = ServerTokenManager.noop(); + it('should return a token', async () => { + const tokenManager = ServerTokenManager.default({ + config: configWithSecret, + logger, + }); expect((await tokenManager.getToken()).token).toBeDefined(); }); }); describe('authenticate', () => { it('should not throw if token is valid', async () => { - const tokenManager = ServerTokenManager.fromConfig(configWithSecret); + const tokenManager = ServerTokenManager.default({ + config: configWithSecret, + logger, + }); const { token } = await tokenManager.getToken(); await expect(tokenManager.authenticate(token)).resolves.not.toThrow(); }); it('should throw if token is invalid', async () => { - const tokenManager = ServerTokenManager.fromConfig(configWithSecret); + const tokenManager = ServerTokenManager.default({ + config: configWithSecret, + logger, + }); await expect( tokenManager.authenticate('random-string'), ).rejects.toThrowError(/invalid server token/i); }); it('should validate server tokens created by a different instance using the same secret', async () => { - const tokenManager1 = ServerTokenManager.fromConfig(configWithSecret); - const tokenManager2 = ServerTokenManager.fromConfig(configWithSecret); + const tokenManager1 = ServerTokenManager.default({ + config: configWithSecret, + logger, + }); + const tokenManager2 = ServerTokenManager.default({ + config: configWithSecret, + logger, + }); const { token } = await tokenManager1.getToken(); @@ -63,23 +82,26 @@ describe('ServerTokenManager', () => { }); it('should validate server tokens created using any of the secrets', async () => { - const tokenManager1 = ServerTokenManager.fromConfig( - new ConfigReader({ + const tokenManager1 = ServerTokenManager.default({ + config: new ConfigReader({ backend: { auth: { keys: [{ secret: 'a1b2c3' }] } }, }), - ); - const tokenManager2 = ServerTokenManager.fromConfig( - new ConfigReader({ + logger, + }); + const tokenManager2 = ServerTokenManager.default({ + config: new ConfigReader({ backend: { auth: { keys: [{ secret: 'd4e5f6' }] } }, }), - ); - const tokenManager3 = ServerTokenManager.fromConfig( - new ConfigReader({ + logger, + }); + const tokenManager3 = ServerTokenManager.default({ + config: new ConfigReader({ backend: { auth: { keys: [{ secret: 'a1b2c3' }, { secret: 'd4e5f6' }] }, }, }), - ); + logger, + }); const { token: token1 } = await tokenManager1.getToken(); await expect(tokenManager3.authenticate(token1)).resolves.not.toThrow(); @@ -89,16 +111,18 @@ describe('ServerTokenManager', () => { }); it('should throw for server tokens created using a different secret', async () => { - const tokenManager1 = ServerTokenManager.fromConfig( - new ConfigReader({ + const tokenManager1 = ServerTokenManager.default({ + config: new ConfigReader({ backend: { auth: { keys: [{ secret: 'a1b2c3' }] } }, }), - ); - const tokenManager2 = ServerTokenManager.fromConfig( - new ConfigReader({ + logger, + }); + const tokenManager2 = ServerTokenManager.default({ + config: new ConfigReader({ backend: { auth: { keys: [{ secret: 'd4e5f6' }] } }, }), - ); + logger, + }); const { token } = await tokenManager1.getToken(); @@ -107,83 +131,97 @@ describe('ServerTokenManager', () => { ); }); - it('should throw for server tokens created using a noop TokenManager', async () => { - const noopTokenManager = ServerTokenManager.noop(); - const tokenManager = ServerTokenManager.fromConfig( - new ConfigReader({ + it('should throw for server tokens created by a different generated secret', async () => { + (process.env as any).NODE_ENV = 'development'; + const tokenManager1 = ServerTokenManager.default({ + config: new ConfigReader({ backend: { auth: { keys: [{ secret: 'a1b2c3' }] } }, }), - ); + logger, + }); + const tokenManager2 = ServerTokenManager.default({ + config: emptyConfig, + logger, + }); - const { token } = await noopTokenManager.getToken(); + const { token } = await tokenManager2.getToken(); - await expect(tokenManager.authenticate(token)).rejects.toThrowError( + await expect(tokenManager1.authenticate(token)).rejects.toThrowError( /invalid server token/i, ); }); }); - describe('ServerTokenManager.fromConfig', () => { - it('should throw if backend auth configuration is missing', () => { - expect(() => - ServerTokenManager.fromConfig(new ConfigReader({})), - ).toThrow(); + describe('default', () => { + describe('NOVE_ENV === production', () => { + it('should throw if backend auth configuration is missing', () => { + expect(() => + ServerTokenManager.default({ config: emptyConfig, logger }), + ).toThrow(); + }); + + it('should throw if no keys are included in the configuration', () => { + expect(() => + ServerTokenManager.default({ + config: new ConfigReader({ + backend: { auth: { keys: [] } }, + }), + logger, + }), + ).toThrow(); + }); + + it('should throw if any key is missing a secret property', () => { + expect(() => + ServerTokenManager.default({ + config: new ConfigReader({ + backend: { + auth: { + keys: [{ secret: '123' }, {}, { secret: '789' }], + }, + }, + }), + logger, + }), + ).toThrow(); + }); }); - it('should throw if no keys are included in the configuration', () => { - expect(() => - ServerTokenManager.fromConfig( - new ConfigReader({ + describe('NOVE_ENV === development', () => { + const generateSyncSpy = jest.spyOn(JWK, 'generateSync'); + + beforeEach(() => { + (process.env as any).NODE_ENV = 'development'; + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + it('should generate a key if no config is provided', () => { + ServerTokenManager.default({ + config: emptyConfig, + logger, + }); + + expect(generateSyncSpy).toHaveBeenCalledWith('oct', 192); + }); + + it('should generate a key if no keys are provided in the configuration', () => { + ServerTokenManager.default({ + config: new ConfigReader({ backend: { auth: { keys: [] } }, }), - ), - ).toThrow(); - }); + logger, + }); - it('should throw if any key is missing a secret property', () => { - expect(() => - ServerTokenManager.fromConfig( - new ConfigReader({ - backend: { - auth: { - keys: [{ secret: '123' }, {}, { secret: '789' }], - }, - }, - }), - ), - ).toThrow(); - }); - }); + expect(generateSyncSpy).toHaveBeenCalledWith('oct', 192); + }); - describe('ServerTokenManager.noop', () => { - let noopTokenManager: TokenManager; - - beforeEach(() => { - noopTokenManager = ServerTokenManager.noop(); - }); - - it('should accept tokens it generates', async () => { - const { token } = await noopTokenManager.getToken(); - - await expect(noopTokenManager.authenticate(token)).resolves.not.toThrow(); - }); - - it('should accept tokens generated by other noop token managers', async () => { - const noopTokenManager2 = ServerTokenManager.noop(); - await expect( - noopTokenManager.authenticate( - ( - await noopTokenManager2.getToken() - ).token, - ), - ).resolves.not.toThrow(); - }); - - it('should accept signed tokens', async () => { - const tokenManager = ServerTokenManager.fromConfig(configWithSecret); - await expect( - noopTokenManager.authenticate((await tokenManager.getToken()).token), - ).resolves.not.toThrow(); + it('should use provided secrets if config is provided', () => { + ServerTokenManager.default({ config: configWithSecret, logger }); + expect(generateSyncSpy).not.toHaveBeenCalled(); + }); }); }); }); diff --git a/packages/backend-common/src/tokens/ServerTokenManager.ts b/packages/backend-common/src/tokens/ServerTokenManager.ts index 35a07509b6..a9633c7f77 100644 --- a/packages/backend-common/src/tokens/ServerTokenManager.ts +++ b/packages/backend-common/src/tokens/ServerTokenManager.ts @@ -18,14 +18,7 @@ import { JWKS, JWK, JWT } from 'jose'; import { Config } from '@backstage/config'; import { AuthenticationError } from '@backstage/errors'; import { TokenManager } from './types'; - -class NoopTokenManager implements TokenManager { - async getToken() { - return { token: '' }; - } - - async authenticate() {} -} +import { Logger } from 'winston'; /** * Creates and validates tokens for use during backend-to-backend @@ -37,20 +30,42 @@ export class ServerTokenManager implements TokenManager { private readonly verificationKeys: JWKS.KeyStore; private readonly signingKey: JWK.Key; - static noop(): TokenManager { - return new NoopTokenManager(); + static default(options: { config: Config; logger: Logger }) { + const { config, logger } = options; + + if (process.env.NODE_ENV === 'development') { + let secrets: string[] = []; + try { + secrets = this.getSecrets(config); + } catch { + // For development, if a secret has not been configured, we auto generate a secret instead of throwing. + } + + if (!secrets.length) { + const generatedDevOnlyKey = JWK.generateSync('oct', 24 * 8); + if (generatedDevOnlyKey.k === undefined) { + throw new Error('No key generated'); + } + logger.warn( + 'Generated a secret for backend-to-backend authentication: DEVELOPMENT USE ONLY. You must configure a secret before deploying to production.', + ); + return new ServerTokenManager([generatedDevOnlyKey.k]); + } + return new ServerTokenManager(secrets); + } + + const secrets = this.getSecrets(config); + return new ServerTokenManager(secrets); } - static fromConfig(config: Config) { - return new ServerTokenManager( - config - .getConfigArray('backend.auth.keys') - .map(key => key.getString('secret')), - ); + private static getSecrets(config: Config) { + return config + .getConfigArray('backend.auth.keys') + .map(key => key.getString('secret')); } - private constructor(secrets?: string[]) { - if (!secrets?.length) { + private constructor(secrets: string[]) { + if (!secrets.length) { throw new Error( 'No secrets provided when constructing ServerTokenManager', ); diff --git a/packages/backend/src/index.ts b/packages/backend/src/index.ts index ba38f8c427..78b48e581f 100644 --- a/packages/backend/src/index.ts +++ b/packages/backend/src/index.ts @@ -63,7 +63,7 @@ function makeCreateEnv(config: Config) { const root = getRootLogger(); const reader = UrlReaders.default({ logger: root, config }); const discovery = SingleHostDiscovery.fromConfig(config); - const tokenManager = ServerTokenManager.fromConfig(config); + const tokenManager = ServerTokenManager.default({ config, logger: root }); const permissions = new ServerPermissionClient({ discoveryApi: discovery, configApi: config, diff --git a/plugins/permission-node/api-report.md b/plugins/permission-node/api-report.md index 3df1386d4d..5e254d4a36 100644 --- a/plugins/permission-node/api-report.md +++ b/plugins/permission-node/api-report.md @@ -4,11 +4,17 @@ ```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 { BackstageIdentity } from '@backstage/plugin-auth-backend'; +import { Config } from '@backstage/config'; +import { DiscoveryApi } from '@backstage/plugin-permission-common'; +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'; // @public export type ApplyConditionsRequest = { @@ -119,4 +125,20 @@ export type PolicyDecision = result: AuthorizeResult.ALLOW | AuthorizeResult.DENY; } | ConditionalPolicyDecision; + +// Warning: (ae-missing-release-tag) "ServerPermissionClient" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) +// +// @public (undocumented) +export class ServerPermissionClient extends PermissionClient { + constructor(options: { + discoveryApi: DiscoveryApi; + configApi: Config; + serverTokenManager: ServerTokenManager; + }); + // (undocumented) + authorize( + requests: AuthorizeRequest[], + options?: AuthorizeRequestOptions, + ): Promise; +} ``` From abb7616345c98709b83a61d8e91dac1e16a1ba5c Mon Sep 17 00:00:00 2001 From: Joon Park Date: Tue, 7 Dec 2021 15:47:19 +0000 Subject: [PATCH 04/24] Add changeset and docs. Signed-off-by: Joon Park --- .changeset/sharp-peaches-begin.md | 8 ++++++++ docs/tutorials/backend-to-backend-auth.md | 12 ++++++------ plugins/permission-node/api-report.md | 4 +--- .../permission-node/src/ServerPermissionClient.ts | 5 +++++ 4 files changed, 20 insertions(+), 9 deletions(-) create mode 100644 .changeset/sharp-peaches-begin.md diff --git a/.changeset/sharp-peaches-begin.md b/.changeset/sharp-peaches-begin.md new file mode 100644 index 0000000000..63c452fbb1 --- /dev/null +++ b/.changeset/sharp-peaches-begin.md @@ -0,0 +1,8 @@ +--- +'@backstage/backend-common': minor +'example-app': patch +'example-backend': patch +'@backstage/plugin-permission-node': patch +--- + +Integrate permission framework into example app diff --git a/docs/tutorials/backend-to-backend-auth.md b/docs/tutorials/backend-to-backend-auth.md index 3cb72a4827..07de883172 100644 --- a/docs/tutorials/backend-to-backend-auth.md +++ b/docs/tutorials/backend-to-backend-auth.md @@ -22,13 +22,14 @@ resulting value. node -p 'require("crypto").randomBytes(24).toString("base64")' ``` +**NOTE**: For ease of development, we auto-generate a key for you if you haven't +configured a secret in dev mode. You _must set your own secret_ in order for +backend-to-backend authentication to work in production. + Requests originating from a backend plugin can be authenticated by decorating them with a backend token. Backend tokens can be generated using a `TokenManager`, which can be passed to plugin backends via the -`PluginEnvironment`. The `TokenManager` provided in new Backstage instances -generated by `create-app` is a stub, which returns empty tokens and accepts any -input string as valid. To enable backend-to-backend authentication, you'll need -to instantiate a new one using the secret from your config instead: +`PluginEnvironment`. ```diff // packages/backend/src/index.ts @@ -42,8 +43,7 @@ function makeCreateEnv(config: Config) { const cacheManager = CacheManager.fromConfig(config); const databaseManager = DatabaseManager.fromConfig(config); -- const tokenManager = ServerTokenManager.noop(); -+ const tokenManager = ServerTokenManager.fromConfig(config); ++ const tokenManager = ServerTokenManager.default({ config, logger: root }); ``` With this `tokenManager`, you can then generate a server token for requests: diff --git a/plugins/permission-node/api-report.md b/plugins/permission-node/api-report.md index 5e254d4a36..8e63daca83 100644 --- a/plugins/permission-node/api-report.md +++ b/plugins/permission-node/api-report.md @@ -126,9 +126,7 @@ export type PolicyDecision = } | ConditionalPolicyDecision; -// Warning: (ae-missing-release-tag) "ServerPermissionClient" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) -// -// @public (undocumented) +// @public export class ServerPermissionClient extends PermissionClient { constructor(options: { discoveryApi: DiscoveryApi; diff --git a/plugins/permission-node/src/ServerPermissionClient.ts b/plugins/permission-node/src/ServerPermissionClient.ts index 1e54d73808..0ef464c639 100644 --- a/plugins/permission-node/src/ServerPermissionClient.ts +++ b/plugins/permission-node/src/ServerPermissionClient.ts @@ -25,6 +25,11 @@ import { PermissionClient, } from '@backstage/plugin-permission-common'; +/** + * A server side {@link @backstage/plugin-permission-common#PermissionClient} + * that allows all backend-to-backend requests. + * @public + */ export class ServerPermissionClient extends PermissionClient { private readonly serverTokenManager: ServerTokenManager; From b3c67c7197292f471634389ccf92b51a51f1a7dc Mon Sep 17 00:00:00 2001 From: Joon Park Date: Wed, 8 Dec 2021 16:29:11 +0000 Subject: [PATCH 05/24] Remove noop from create app template Signed-off-by: Joon Park --- .../templates/default-app/packages/backend/src/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/create-app/templates/default-app/packages/backend/src/index.ts b/packages/create-app/templates/default-app/packages/backend/src/index.ts index 3f12122a3f..f191acf3db 100644 --- a/packages/create-app/templates/default-app/packages/backend/src/index.ts +++ b/packages/create-app/templates/default-app/packages/backend/src/index.ts @@ -38,7 +38,7 @@ function makeCreateEnv(config: Config) { const cacheManager = CacheManager.fromConfig(config); const databaseManager = DatabaseManager.fromConfig(config); - const tokenManager = ServerTokenManager.noop(); + const tokenManager = ServerTokenManager.default({ config, logger: root }); return (plugin: string): PluginEnvironment => { const logger = root.child({ type: 'plugin', plugin }); From 366ca3ebfbaaa321b0ed7708822dd675164ee082 Mon Sep 17 00:00:00 2001 From: Joon Park Date: Wed, 8 Dec 2021 17:01:48 +0000 Subject: [PATCH 06/24] Fix ts error Signed-off-by: Joon Park --- packages/backend/package.json | 2 +- plugins/permission-node/api-report.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/backend/package.json b/packages/backend/package.json index d5249c28bc..491ecedbb2 100644 --- a/packages/backend/package.json +++ b/packages/backend/package.json @@ -39,7 +39,7 @@ "@backstage/plugin-jenkins-backend": "^0.1.9", "@backstage/plugin-kubernetes-backend": "^0.4.0", "@backstage/plugin-kafka-backend": "^0.2.12", - "@backstage/plugin-permission-backend": "^0.1.0", + "@backstage/plugin-permission-backend": "^0.2.0", "@backstage/plugin-permission-common": "^0.2.0", "@backstage/plugin-permission-node": "^0.2.0", "@backstage/plugin-proxy-backend": "^0.2.14", diff --git a/plugins/permission-node/api-report.md b/plugins/permission-node/api-report.md index 8e63daca83..367bcd2386 100644 --- a/plugins/permission-node/api-report.md +++ b/plugins/permission-node/api-report.md @@ -7,7 +7,7 @@ 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 { BackstageIdentity } from '@backstage/plugin-auth-backend'; +import { BackstageIdentityResponse } from '@backstage/plugin-auth-backend'; import { Config } from '@backstage/config'; import { DiscoveryApi } from '@backstage/plugin-permission-common'; import { PermissionClient } from '@backstage/plugin-permission-common'; From 2f8a9b665f8e410ec073f58b1b20b9205fdcb7c3 Mon Sep 17 00:00:00 2001 From: Joon Park Date: Thu, 9 Dec 2021 17:19:15 +0000 Subject: [PATCH 07/24] Separate changesets Signed-off-by: Joon Park --- .changeset/lazy-files-check.md | 5 +++++ .changeset/sharp-peaches-begin.md | 8 -------- .changeset/wet-bikes-pull.md | 5 +++++ .../backend-common/src/tokens/ServerTokenManager.test.ts | 4 ++-- 4 files changed, 12 insertions(+), 10 deletions(-) create mode 100644 .changeset/lazy-files-check.md delete mode 100644 .changeset/sharp-peaches-begin.md create mode 100644 .changeset/wet-bikes-pull.md diff --git a/.changeset/lazy-files-check.md b/.changeset/lazy-files-check.md new file mode 100644 index 0000000000..30ae18dd7f --- /dev/null +++ b/.changeset/lazy-files-check.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-common': minor +--- + +Generate backend-to-backend secret for dev mode diff --git a/.changeset/sharp-peaches-begin.md b/.changeset/sharp-peaches-begin.md deleted file mode 100644 index 63c452fbb1..0000000000 --- a/.changeset/sharp-peaches-begin.md +++ /dev/null @@ -1,8 +0,0 @@ ---- -'@backstage/backend-common': minor -'example-app': patch -'example-backend': patch -'@backstage/plugin-permission-node': patch ---- - -Integrate permission framework into example app diff --git a/.changeset/wet-bikes-pull.md b/.changeset/wet-bikes-pull.md new file mode 100644 index 0000000000..dcc3c34890 --- /dev/null +++ b/.changeset/wet-bikes-pull.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-permission-node': patch +--- + +Add ServerPermissionClient to check for backend-to-backend tokens diff --git a/packages/backend-common/src/tokens/ServerTokenManager.test.ts b/packages/backend-common/src/tokens/ServerTokenManager.test.ts index 2bdd02c9f5..df74faf534 100644 --- a/packages/backend-common/src/tokens/ServerTokenManager.test.ts +++ b/packages/backend-common/src/tokens/ServerTokenManager.test.ts @@ -153,7 +153,7 @@ describe('ServerTokenManager', () => { }); describe('default', () => { - describe('NOVE_ENV === production', () => { + describe('NODE_ENV === production', () => { it('should throw if backend auth configuration is missing', () => { expect(() => ServerTokenManager.default({ config: emptyConfig, logger }), @@ -187,7 +187,7 @@ describe('ServerTokenManager', () => { }); }); - describe('NOVE_ENV === development', () => { + describe('NODE_ENV === development', () => { const generateSyncSpy = jest.spyOn(JWK, 'generateSync'); beforeEach(() => { From ea96ce244960e7c25eeabf2a7f115935f4bc1f8f Mon Sep 17 00:00:00 2001 From: Joon Park Date: Mon, 13 Dec 2021 11:07:23 +0000 Subject: [PATCH 08/24] Change ServerTokenManager method name back to fromConfig Signed-off-by: Joon Park --- docs/tutorials/backend-to-backend-auth.md | 2 +- packages/backend-common/api-report.md | 10 +- .../src/tokens/ServerTokenManager.test.ts | 99 +++++++++---------- .../src/tokens/ServerTokenManager.ts | 4 +- packages/backend/src/index.ts | 2 +- .../default-app/packages/backend/src/index.ts | 2 +- 6 files changed, 56 insertions(+), 63 deletions(-) diff --git a/docs/tutorials/backend-to-backend-auth.md b/docs/tutorials/backend-to-backend-auth.md index 07de883172..6163d4f592 100644 --- a/docs/tutorials/backend-to-backend-auth.md +++ b/docs/tutorials/backend-to-backend-auth.md @@ -43,7 +43,7 @@ function makeCreateEnv(config: Config) { const cacheManager = CacheManager.fromConfig(config); const databaseManager = DatabaseManager.fromConfig(config); -+ const tokenManager = ServerTokenManager.default({ config, logger: root }); ++ const tokenManager = ServerTokenManager.fromConfig(config, { logger: root }); ``` With this `tokenManager`, you can then generate a server token for requests: diff --git a/packages/backend-common/api-report.md b/packages/backend-common/api-report.md index 32d0df1148..cd44601d8c 100644 --- a/packages/backend-common/api-report.md +++ b/packages/backend-common/api-report.md @@ -491,10 +491,12 @@ export class ServerTokenManager implements TokenManager { // (undocumented) authenticate(token: string): Promise; // (undocumented) - static default(options: { - config: Config; - logger: Logger_2; - }): ServerTokenManager; + static fromConfig( + config: Config, + options: { + logger: Logger_2; + }, + ): ServerTokenManager; // (undocumented) getToken(): Promise<{ token: string; diff --git a/packages/backend-common/src/tokens/ServerTokenManager.test.ts b/packages/backend-common/src/tokens/ServerTokenManager.test.ts index df74faf534..fb82eada29 100644 --- a/packages/backend-common/src/tokens/ServerTokenManager.test.ts +++ b/packages/backend-common/src/tokens/ServerTokenManager.test.ts @@ -38,8 +38,7 @@ describe('ServerTokenManager', () => { describe('getToken', () => { it('should return a token', async () => { - const tokenManager = ServerTokenManager.default({ - config: configWithSecret, + const tokenManager = ServerTokenManager.fromConfig(configWithSecret, { logger, }); expect((await tokenManager.getToken()).token).toBeDefined(); @@ -48,8 +47,7 @@ describe('ServerTokenManager', () => { describe('authenticate', () => { it('should not throw if token is valid', async () => { - const tokenManager = ServerTokenManager.default({ - config: configWithSecret, + const tokenManager = ServerTokenManager.fromConfig(configWithSecret, { logger, }); const { token } = await tokenManager.getToken(); @@ -57,8 +55,7 @@ describe('ServerTokenManager', () => { }); it('should throw if token is invalid', async () => { - const tokenManager = ServerTokenManager.default({ - config: configWithSecret, + const tokenManager = ServerTokenManager.fromConfig(configWithSecret, { logger, }); await expect( @@ -67,12 +64,10 @@ describe('ServerTokenManager', () => { }); it('should validate server tokens created by a different instance using the same secret', async () => { - const tokenManager1 = ServerTokenManager.default({ - config: configWithSecret, + const tokenManager1 = ServerTokenManager.fromConfig(configWithSecret, { logger, }); - const tokenManager2 = ServerTokenManager.default({ - config: configWithSecret, + const tokenManager2 = ServerTokenManager.fromConfig(configWithSecret, { logger, }); @@ -82,26 +77,26 @@ describe('ServerTokenManager', () => { }); it('should validate server tokens created using any of the secrets', async () => { - const tokenManager1 = ServerTokenManager.default({ - config: new ConfigReader({ + const tokenManager1 = ServerTokenManager.fromConfig( + new ConfigReader({ backend: { auth: { keys: [{ secret: 'a1b2c3' }] } }, }), - logger, - }); - const tokenManager2 = ServerTokenManager.default({ - config: new ConfigReader({ + { logger }, + ); + const tokenManager2 = ServerTokenManager.fromConfig( + new ConfigReader({ backend: { auth: { keys: [{ secret: 'd4e5f6' }] } }, }), - logger, - }); - const tokenManager3 = ServerTokenManager.default({ - config: new ConfigReader({ + { logger }, + ); + const tokenManager3 = ServerTokenManager.fromConfig( + new ConfigReader({ backend: { auth: { keys: [{ secret: 'a1b2c3' }, { secret: 'd4e5f6' }] }, }, }), - logger, - }); + { logger }, + ); const { token: token1 } = await tokenManager1.getToken(); await expect(tokenManager3.authenticate(token1)).resolves.not.toThrow(); @@ -111,18 +106,18 @@ describe('ServerTokenManager', () => { }); it('should throw for server tokens created using a different secret', async () => { - const tokenManager1 = ServerTokenManager.default({ - config: new ConfigReader({ + const tokenManager1 = ServerTokenManager.fromConfig( + new ConfigReader({ backend: { auth: { keys: [{ secret: 'a1b2c3' }] } }, }), - logger, - }); - const tokenManager2 = ServerTokenManager.default({ - config: new ConfigReader({ + { logger }, + ); + const tokenManager2 = ServerTokenManager.fromConfig( + new ConfigReader({ backend: { auth: { keys: [{ secret: 'd4e5f6' }] } }, }), - logger, - }); + { logger }, + ); const { token } = await tokenManager1.getToken(); @@ -133,14 +128,13 @@ describe('ServerTokenManager', () => { it('should throw for server tokens created by a different generated secret', async () => { (process.env as any).NODE_ENV = 'development'; - const tokenManager1 = ServerTokenManager.default({ - config: new ConfigReader({ + const tokenManager1 = ServerTokenManager.fromConfig( + new ConfigReader({ backend: { auth: { keys: [{ secret: 'a1b2c3' }] } }, }), - logger, - }); - const tokenManager2 = ServerTokenManager.default({ - config: emptyConfig, + { logger }, + ); + const tokenManager2 = ServerTokenManager.fromConfig(emptyConfig, { logger, }); @@ -156,33 +150,33 @@ describe('ServerTokenManager', () => { describe('NODE_ENV === production', () => { it('should throw if backend auth configuration is missing', () => { expect(() => - ServerTokenManager.default({ config: emptyConfig, logger }), + ServerTokenManager.fromConfig(emptyConfig, { logger }), ).toThrow(); }); it('should throw if no keys are included in the configuration', () => { expect(() => - ServerTokenManager.default({ - config: new ConfigReader({ + ServerTokenManager.fromConfig( + new ConfigReader({ backend: { auth: { keys: [] } }, }), - logger, - }), + { logger }, + ), ).toThrow(); }); it('should throw if any key is missing a secret property', () => { expect(() => - ServerTokenManager.default({ - config: new ConfigReader({ + ServerTokenManager.fromConfig( + new ConfigReader({ backend: { auth: { keys: [{ secret: '123' }, {}, { secret: '789' }], }, }, }), - logger, - }), + { logger }, + ), ).toThrow(); }); }); @@ -199,27 +193,24 @@ describe('ServerTokenManager', () => { }); it('should generate a key if no config is provided', () => { - ServerTokenManager.default({ - config: emptyConfig, - logger, - }); + ServerTokenManager.fromConfig(emptyConfig, { logger }); expect(generateSyncSpy).toHaveBeenCalledWith('oct', 192); }); it('should generate a key if no keys are provided in the configuration', () => { - ServerTokenManager.default({ - config: new ConfigReader({ + ServerTokenManager.fromConfig( + new ConfigReader({ backend: { auth: { keys: [] } }, }), - logger, - }); + { logger }, + ); expect(generateSyncSpy).toHaveBeenCalledWith('oct', 192); }); it('should use provided secrets if config is provided', () => { - ServerTokenManager.default({ config: configWithSecret, logger }); + ServerTokenManager.fromConfig(configWithSecret, { logger }); expect(generateSyncSpy).not.toHaveBeenCalled(); }); }); diff --git a/packages/backend-common/src/tokens/ServerTokenManager.ts b/packages/backend-common/src/tokens/ServerTokenManager.ts index a9633c7f77..0b7ea49dce 100644 --- a/packages/backend-common/src/tokens/ServerTokenManager.ts +++ b/packages/backend-common/src/tokens/ServerTokenManager.ts @@ -30,8 +30,8 @@ export class ServerTokenManager implements TokenManager { private readonly verificationKeys: JWKS.KeyStore; private readonly signingKey: JWK.Key; - static default(options: { config: Config; logger: Logger }) { - const { config, logger } = options; + static fromConfig(config: Config, options: { logger: Logger }) { + const { logger } = options; if (process.env.NODE_ENV === 'development') { let secrets: string[] = []; diff --git a/packages/backend/src/index.ts b/packages/backend/src/index.ts index 78b48e581f..96def8d03f 100644 --- a/packages/backend/src/index.ts +++ b/packages/backend/src/index.ts @@ -63,7 +63,7 @@ function makeCreateEnv(config: Config) { const root = getRootLogger(); const reader = UrlReaders.default({ logger: root, config }); const discovery = SingleHostDiscovery.fromConfig(config); - const tokenManager = ServerTokenManager.default({ config, logger: root }); + const tokenManager = ServerTokenManager.fromConfig(config, { logger: root }); const permissions = new ServerPermissionClient({ discoveryApi: discovery, configApi: config, diff --git a/packages/create-app/templates/default-app/packages/backend/src/index.ts b/packages/create-app/templates/default-app/packages/backend/src/index.ts index f191acf3db..7a2d2a7a34 100644 --- a/packages/create-app/templates/default-app/packages/backend/src/index.ts +++ b/packages/create-app/templates/default-app/packages/backend/src/index.ts @@ -38,7 +38,7 @@ function makeCreateEnv(config: Config) { const cacheManager = CacheManager.fromConfig(config); const databaseManager = DatabaseManager.fromConfig(config); - const tokenManager = ServerTokenManager.default({ config, logger: root }); + const tokenManager = ServerTokenManager.fromConfig(config, { logger: root }); return (plugin: string): PluginEnvironment => { const logger = root.child({ type: 'plugin', plugin }); From 24dce3ca434123ec90701aa91e58373f142c6a00 Mon Sep 17 00:00:00 2001 From: Joon Park Date: Mon, 13 Dec 2021 14:10:10 +0000 Subject: [PATCH 09/24] Reintroduce noop token manager and refactor ServerPermissionClient Signed-off-by: Joon Park --- docs/tutorials/backend-to-backend-auth.md | 6 +- packages/backend-common/api-report.md | 2 + .../src/tokens/ServerTokenManager.test.ts | 58 ++++++++- .../src/tokens/ServerTokenManager.ts | 12 ++ packages/backend/src/index.ts | 2 +- .../default-app/packages/backend/src/index.ts | 2 +- plugins/permission-common/api-report.md | 4 + .../permission-common/src/PermissionClient.ts | 10 +- plugins/permission-node/api-report.md | 10 +- plugins/permission-node/package.json | 1 + .../src/ServerPermissionClient.test.ts | 118 ++++++++++++++++++ .../src/ServerPermissionClient.ts | 33 +++-- 12 files changed, 232 insertions(+), 26 deletions(-) create mode 100644 plugins/permission-node/src/ServerPermissionClient.test.ts diff --git a/docs/tutorials/backend-to-backend-auth.md b/docs/tutorials/backend-to-backend-auth.md index 6163d4f592..cd73a95c89 100644 --- a/docs/tutorials/backend-to-backend-auth.md +++ b/docs/tutorials/backend-to-backend-auth.md @@ -29,7 +29,10 @@ backend-to-backend authentication to work in production. Requests originating from a backend plugin can be authenticated by decorating them with a backend token. Backend tokens can be generated using a `TokenManager`, which can be passed to plugin backends via the -`PluginEnvironment`. +`PluginEnvironment`. The `TokenManager` provided in new Backstage instances +generated by `create-app` is a stub, which returns empty tokens and accepts any +input string as valid. To enable backend-to-backend authentication, you'll need +to instantiate a new one using the secret from your config instead: ```diff // packages/backend/src/index.ts @@ -43,6 +46,7 @@ function makeCreateEnv(config: Config) { const cacheManager = CacheManager.fromConfig(config); const databaseManager = DatabaseManager.fromConfig(config); +- const tokenManager = ServerTokenManager.noop(); + const tokenManager = ServerTokenManager.fromConfig(config, { logger: root }); ``` diff --git a/packages/backend-common/api-report.md b/packages/backend-common/api-report.md index cd44601d8c..b929251890 100644 --- a/packages/backend-common/api-report.md +++ b/packages/backend-common/api-report.md @@ -501,6 +501,8 @@ export class ServerTokenManager implements TokenManager { getToken(): Promise<{ token: string; }>; + // (undocumented) + static noop(): TokenManager; } // @public (undocumented) diff --git a/packages/backend-common/src/tokens/ServerTokenManager.test.ts b/packages/backend-common/src/tokens/ServerTokenManager.test.ts index fb82eada29..1799291544 100644 --- a/packages/backend-common/src/tokens/ServerTokenManager.test.ts +++ b/packages/backend-common/src/tokens/ServerTokenManager.test.ts @@ -18,6 +18,7 @@ import { ConfigReader } from '@backstage/config'; import { ServerTokenManager } from './ServerTokenManager'; import { Logger } from 'winston'; import { JWK } from 'jose'; +import { TokenManager } from './types'; const emptyConfig = new ConfigReader({}); const configWithSecret = new ConfigReader({ @@ -43,6 +44,11 @@ describe('ServerTokenManager', () => { }); expect((await tokenManager.getToken()).token).toBeDefined(); }); + + it('should return a token string if using a noop TokenManager', async () => { + const tokenManager = ServerTokenManager.noop(); + expect((await tokenManager.getToken()).token).toBeDefined(); + }); }); describe('authenticate', () => { @@ -126,6 +132,22 @@ describe('ServerTokenManager', () => { ); }); + it('should throw for server tokens created using a noop TokenManager', async () => { + const noopTokenManager = ServerTokenManager.noop(); + const tokenManager = ServerTokenManager.fromConfig( + new ConfigReader({ + backend: { auth: { keys: [{ secret: 'a1b2c3' }] } }, + }), + { logger }, + ); + + const { token } = await noopTokenManager.getToken(); + + await expect(tokenManager.authenticate(token)).rejects.toThrowError( + /invalid server token/i, + ); + }); + it('should throw for server tokens created by a different generated secret', async () => { (process.env as any).NODE_ENV = 'development'; const tokenManager1 = ServerTokenManager.fromConfig( @@ -146,7 +168,7 @@ describe('ServerTokenManager', () => { }); }); - describe('default', () => { + describe('fromConfig', () => { describe('NODE_ENV === production', () => { it('should throw if backend auth configuration is missing', () => { expect(() => @@ -215,4 +237,38 @@ describe('ServerTokenManager', () => { }); }); }); + + describe('ServerTokenManager.noop', () => { + let noopTokenManager: TokenManager; + + beforeEach(() => { + noopTokenManager = ServerTokenManager.noop(); + }); + + it('should accept tokens it generates', async () => { + const { token } = await noopTokenManager.getToken(); + + await expect(noopTokenManager.authenticate(token)).resolves.not.toThrow(); + }); + + it('should accept tokens generated by other noop token managers', async () => { + const noopTokenManager2 = ServerTokenManager.noop(); + await expect( + noopTokenManager.authenticate( + ( + await noopTokenManager2.getToken() + ).token, + ), + ).resolves.not.toThrow(); + }); + + it('should accept signed tokens', async () => { + const tokenManager = ServerTokenManager.fromConfig(configWithSecret, { + logger, + }); + await expect( + noopTokenManager.authenticate((await tokenManager.getToken()).token), + ).resolves.not.toThrow(); + }); + }); }); diff --git a/packages/backend-common/src/tokens/ServerTokenManager.ts b/packages/backend-common/src/tokens/ServerTokenManager.ts index 0b7ea49dce..7b50ff4300 100644 --- a/packages/backend-common/src/tokens/ServerTokenManager.ts +++ b/packages/backend-common/src/tokens/ServerTokenManager.ts @@ -20,6 +20,14 @@ import { AuthenticationError } from '@backstage/errors'; import { TokenManager } from './types'; import { Logger } from 'winston'; +class NoopTokenManager implements TokenManager { + async getToken() { + return { token: '' }; + } + + async authenticate() {} +} + /** * Creates and validates tokens for use during backend-to-backend * authentication. @@ -30,6 +38,10 @@ export class ServerTokenManager implements TokenManager { private readonly verificationKeys: JWKS.KeyStore; private readonly signingKey: JWK.Key; + static noop(): TokenManager { + return new NoopTokenManager(); + } + static fromConfig(config: Config, options: { logger: Logger }) { const { logger } = options; diff --git a/packages/backend/src/index.ts b/packages/backend/src/index.ts index 96def8d03f..c7d9d9b5ee 100644 --- a/packages/backend/src/index.ts +++ b/packages/backend/src/index.ts @@ -63,7 +63,7 @@ function makeCreateEnv(config: Config) { const root = getRootLogger(); const reader = UrlReaders.default({ logger: root, config }); const discovery = SingleHostDiscovery.fromConfig(config); - const tokenManager = ServerTokenManager.fromConfig(config, { logger: root }); + const tokenManager = ServerTokenManager.noop(); const permissions = new ServerPermissionClient({ discoveryApi: discovery, configApi: config, diff --git a/packages/create-app/templates/default-app/packages/backend/src/index.ts b/packages/create-app/templates/default-app/packages/backend/src/index.ts index 7a2d2a7a34..3f12122a3f 100644 --- a/packages/create-app/templates/default-app/packages/backend/src/index.ts +++ b/packages/create-app/templates/default-app/packages/backend/src/index.ts @@ -38,7 +38,7 @@ function makeCreateEnv(config: Config) { const cacheManager = CacheManager.fromConfig(config); const databaseManager = DatabaseManager.fromConfig(config); - const tokenManager = ServerTokenManager.fromConfig(config, { logger: root }); + const tokenManager = ServerTokenManager.noop(); return (plugin: string): PluginEnvironment => { const logger = root.child({ type: 'plugin', plugin }); diff --git a/plugins/permission-common/api-report.md b/plugins/permission-common/api-report.md index 043b27554c..cc2a433504 100644 --- a/plugins/permission-common/api-report.md +++ b/plugins/permission-common/api-report.md @@ -74,6 +74,10 @@ export class PermissionClient { requests: AuthorizeRequest[], options?: AuthorizeRequestOptions, ): Promise; + // (undocumented) + protected readonly enabled: boolean; + // (undocumented) + protected shouldBypass(_options?: AuthorizeRequestOptions): Promise; } // @public diff --git a/plugins/permission-common/src/PermissionClient.ts b/plugins/permission-common/src/PermissionClient.ts index f98d40a3c4..18e618d5f0 100644 --- a/plugins/permission-common/src/PermissionClient.ts +++ b/plugins/permission-common/src/PermissionClient.ts @@ -72,7 +72,7 @@ export type AuthorizeRequestOptions = { * @public */ export class PermissionClient { - private readonly enabled: boolean; + protected readonly enabled: boolean; private readonly discoveryApi: DiscoveryApi; constructor(options: { discoveryApi: DiscoveryApi; configApi: Config }) { @@ -106,7 +106,7 @@ export class PermissionClient { // 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. - if (!this.enabled) { + if (await this.shouldBypass(options)) { return requests.map(_ => ({ result: AuthorizeResult.ALLOW })); } @@ -141,6 +141,12 @@ export class PermissionClient { return identifiedRequests.map(request => responsesById[request.id]); } + protected async shouldBypass( + _options?: AuthorizeRequestOptions, + ): Promise { + return !this.enabled; + } + private getAuthorizationHeader(token?: string): Record { return token ? { Authorization: `Bearer ${token}` } : {}; } diff --git a/plugins/permission-node/api-report.md b/plugins/permission-node/api-report.md index 367bcd2386..90181925c4 100644 --- a/plugins/permission-node/api-report.md +++ b/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; + shouldBypass(options?: AuthorizeRequestOptions): Promise; } ``` diff --git a/plugins/permission-node/package.json b/plugins/permission-node/package.json index 7166ae5c31..92755e2012 100644 --- a/plugins/permission-node/package.json +++ b/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": [ diff --git a/plugins/permission-node/src/ServerPermissionClient.test.ts b/plugins/permission-node/src/ServerPermissionClient.test.ts new file mode 100644 index 0000000000..8efd6414a7 --- /dev/null +++ b/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) => ({ + 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.', + ); + }); +}); diff --git a/plugins/permission-node/src/ServerPermissionClient.ts b/plugins/permission-node/src/ServerPermissionClient.ts index 0ef464c639..37d75c8ebd 100644 --- a/plugins/permission-node/src/ServerPermissionClient.ts +++ b/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 { - if (await this.isValidServerToken(options?.token)) { - return requests.map(_ => ({ result: AuthorizeResult.ALLOW })); + async shouldBypass(options?: AuthorizeRequestOptions): Promise { + // 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( From 816e0e04f90b27a253f672fcbda1be445043eebf Mon Sep 17 00:00:00 2001 From: Joon Park Date: Tue, 14 Dec 2021 15:56:06 +0000 Subject: [PATCH 10/24] Address various comments round 1 Signed-off-by: Joon Park --- packages/app-defaults/package.json | 1 + packages/app-defaults/src/defaults/apis.ts | 15 +++++++ packages/app/package.json | 1 - packages/app/src/apis.ts | 17 ------- .../src/tokens/ServerTokenManager.ts | 44 +++++++------------ packages/backend/src/index.ts | 2 +- .../src/ServerPermissionClient.ts | 7 ++- 7 files changed, 40 insertions(+), 47 deletions(-) diff --git a/packages/app-defaults/package.json b/packages/app-defaults/package.json index fc1bca2104..80ff8dc29b 100644 --- a/packages/app-defaults/package.json +++ b/packages/app-defaults/package.json @@ -32,6 +32,7 @@ "@backstage/core-components": "^0.8.0", "@backstage/core-app-api": "^0.2.0", "@backstage/core-plugin-api": "^0.3.0", + "@backstage/plugin-permission-react": "^0.1.1", "@backstage/theme": "^0.2.14", "@material-ui/core": "^4.12.2", "@material-ui/icons": "^4.9.1", diff --git a/packages/app-defaults/src/defaults/apis.ts b/packages/app-defaults/src/defaults/apis.ts index 29aa62bdab..e56a27b81f 100644 --- a/packages/app-defaults/src/defaults/apis.ts +++ b/packages/app-defaults/src/defaults/apis.ts @@ -61,7 +61,12 @@ import { oidcAuthApiRef, bitbucketAuthApiRef, atlassianAuthApiRef, + identityApiRef, } from '@backstage/core-plugin-api'; +import { + permissionApiRef, + IdentityPermissionApi, +} from '@backstage/plugin-permission-react'; export const apis = [ createApiFactory({ @@ -296,4 +301,14 @@ export const apis = [ }); }, }), + createApiFactory({ + api: permissionApiRef, + deps: { + discoveryApi: discoveryApiRef, + identityApi: identityApiRef, + configApi: configApiRef, + }, + factory: ({ configApi, discoveryApi, identityApi }) => + IdentityPermissionApi.create({ configApi, discoveryApi, identityApi }), + }), ]; diff --git a/packages/app/package.json b/packages/app/package.json index 5dbb326538..a9e8e8ddc8 100644 --- a/packages/app/package.json +++ b/packages/app/package.json @@ -35,7 +35,6 @@ "@backstage/plugin-newrelic": "^0.3.10", "@backstage/plugin-org": "^0.3.31", "@backstage/plugin-pagerduty": "0.3.19", - "@backstage/plugin-permission-react": "^0.1.1", "@backstage/plugin-rollbar": "^0.3.20", "@backstage/plugin-scaffolder": "^0.11.14", "@backstage/plugin-search": "^0.5.1", diff --git a/packages/app/src/apis.ts b/packages/app/src/apis.ts index f67a7b2649..d587a88402 100644 --- a/packages/app/src/apis.ts +++ b/packages/app/src/apis.ts @@ -31,15 +31,9 @@ import { AnyApiFactory, configApiRef, createApiFactory, - discoveryApiRef, errorApiRef, githubAuthApiRef, - identityApiRef, } from '@backstage/core-plugin-api'; -import { - permissionApiRef, - IdentityPermissionApi, -} from '@backstage/plugin-permission-react'; export const apis: AnyApiFactory[] = [ createApiFactory({ @@ -70,15 +64,4 @@ export const apis: AnyApiFactory[] = [ }), createApiFactory(costInsightsApiRef, new ExampleCostInsightsClient()), - - createApiFactory({ - api: permissionApiRef, - deps: { - discoveryApi: discoveryApiRef, - identityApi: identityApiRef, - configApi: configApiRef, - }, - factory: ({ configApi, discoveryApi, identityApi }) => - IdentityPermissionApi.create({ configApi, discoveryApi, identityApi }), - }), ]; diff --git a/packages/backend-common/src/tokens/ServerTokenManager.ts b/packages/backend-common/src/tokens/ServerTokenManager.ts index 7b50ff4300..7fbd18e8c3 100644 --- a/packages/backend-common/src/tokens/ServerTokenManager.ts +++ b/packages/backend-common/src/tokens/ServerTokenManager.ts @@ -45,35 +45,25 @@ export class ServerTokenManager implements TokenManager { static fromConfig(config: Config, options: { logger: Logger }) { const { logger } = options; - if (process.env.NODE_ENV === 'development') { - let secrets: string[] = []; - try { - secrets = this.getSecrets(config); - } catch { - // For development, if a secret has not been configured, we auto generate a secret instead of throwing. - } - - if (!secrets.length) { - const generatedDevOnlyKey = JWK.generateSync('oct', 24 * 8); - if (generatedDevOnlyKey.k === undefined) { - throw new Error('No key generated'); - } - logger.warn( - 'Generated a secret for backend-to-backend authentication: DEVELOPMENT USE ONLY. You must configure a secret before deploying to production.', - ); - return new ServerTokenManager([generatedDevOnlyKey.k]); - } - return new ServerTokenManager(secrets); + const keys = config.getOptionalConfigArray('backend.auth.keys'); + if (keys?.length) { + return new ServerTokenManager(keys.map(key => key.getString('secret'))); + } + if (process.env.NODE_ENV !== 'development') { + throw new Error( + 'You must configure at least one key in backend.auth.keys for production.', + ); } - const secrets = this.getSecrets(config); - return new ServerTokenManager(secrets); - } - - private static getSecrets(config: Config) { - return config - .getConfigArray('backend.auth.keys') - .map(key => key.getString('secret')); + // For development, if a secret has not been configured, we auto generate a secret instead of throwing. + const generatedDevOnlyKey = JWK.generateSync('oct', 24 * 8); + if (generatedDevOnlyKey.k === undefined) { + throw new Error('No key generated'); + } + logger.warn( + 'Generated a secret for backend-to-backend authentication: DEVELOPMENT USE ONLY.', + ); + return new ServerTokenManager([generatedDevOnlyKey.k]); } private constructor(secrets: string[]) { diff --git a/packages/backend/src/index.ts b/packages/backend/src/index.ts index c7d9d9b5ee..96def8d03f 100644 --- a/packages/backend/src/index.ts +++ b/packages/backend/src/index.ts @@ -63,7 +63,7 @@ function makeCreateEnv(config: Config) { const root = getRootLogger(); const reader = UrlReaders.default({ logger: root, config }); const discovery = SingleHostDiscovery.fromConfig(config); - const tokenManager = ServerTokenManager.noop(); + const tokenManager = ServerTokenManager.fromConfig(config, { logger: root }); const permissions = new ServerPermissionClient({ discoveryApi: discovery, configApi: config, diff --git a/plugins/permission-node/src/ServerPermissionClient.ts b/plugins/permission-node/src/ServerPermissionClient.ts index 37d75c8ebd..8ded6e51e2 100644 --- a/plugins/permission-node/src/ServerPermissionClient.ts +++ b/plugins/permission-node/src/ServerPermissionClient.ts @@ -38,7 +38,12 @@ export class ServerPermissionClient extends PermissionClient { const { discoveryApi, configApi, serverTokenManager } = options; super({ discoveryApi, configApi }); - if (this.enabled && !(serverTokenManager instanceof ServerTokenManager)) { + if ( + this.enabled && + // TODO: Find a cleaner way of ensuring usage of SERVER token manager when + // permissions are enabled. + serverTokenManager instanceof ServerTokenManager.noop().constructor + ) { throw new Error( 'You must configure at least one key in backend.auth.keys if permissions are enabled.', ); From d1801d716689051d7ce92bded8ac6823150a061a Mon Sep 17 00:00:00 2001 From: Joon Park Date: Tue, 14 Dec 2021 16:28:05 +0000 Subject: [PATCH 11/24] Refactor ServerPermissionClient away from inheritance Signed-off-by: Joon Park --- .../permission-common/src/PermissionClient.ts | 24 +++------- plugins/permission-common/src/types/index.ts | 7 ++- .../permission-common/src/types/permission.ts | 17 +++++++ .../src/ServerPermissionClient.ts | 44 ++++++++++++------- 4 files changed, 58 insertions(+), 34 deletions(-) diff --git a/plugins/permission-common/src/PermissionClient.ts b/plugins/permission-common/src/PermissionClient.ts index 18e618d5f0..c61a9cadd9 100644 --- a/plugins/permission-common/src/PermissionClient.ts +++ b/plugins/permission-common/src/PermissionClient.ts @@ -28,6 +28,10 @@ import { PermissionCondition, } from './types/api'; import { DiscoveryApi } from './types/discovery'; +import { + PermissionClientInterface, + AuthorizeRequestOptions, +} from './types/permission'; const permissionCriteriaSchema: z.ZodSchema< PermissionCriteria @@ -59,20 +63,12 @@ const responseSchema = z.array( ), ); -/** - * Options for authorization requests; currently only an optional auth token. - * @public - */ -export type AuthorizeRequestOptions = { - token?: string; -}; - /** * An isomorphic client for requesting authorization for Backstage permissions. * @public */ -export class PermissionClient { - protected readonly enabled: boolean; +export class PermissionClient implements PermissionClientInterface { + private readonly enabled: boolean; private readonly discoveryApi: DiscoveryApi; constructor(options: { discoveryApi: DiscoveryApi; configApi: Config }) { @@ -106,7 +102,7 @@ export class PermissionClient { // 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. - if (await this.shouldBypass(options)) { + if (!this.enabled) { return requests.map(_ => ({ result: AuthorizeResult.ALLOW })); } @@ -141,12 +137,6 @@ export class PermissionClient { return identifiedRequests.map(request => responsesById[request.id]); } - protected async shouldBypass( - _options?: AuthorizeRequestOptions, - ): Promise { - return !this.enabled; - } - private getAuthorizationHeader(token?: string): Record { return token ? { Authorization: `Bearer ${token}` } : {}; } diff --git a/plugins/permission-common/src/types/index.ts b/plugins/permission-common/src/types/index.ts index e21fa19e8b..4d28cca7b5 100644 --- a/plugins/permission-common/src/types/index.ts +++ b/plugins/permission-common/src/types/index.ts @@ -23,4 +23,9 @@ export type { PermissionCriteria, } from './api'; export type { DiscoveryApi } from './discovery'; -export type { PermissionAttributes, Permission } from './permission'; +export type { + PermissionAttributes, + Permission, + PermissionClientInterface, + AuthorizeRequestOptions, +} from './permission'; diff --git a/plugins/permission-common/src/types/permission.ts b/plugins/permission-common/src/types/permission.ts index 181b36065f..a9c82d6a20 100644 --- a/plugins/permission-common/src/types/permission.ts +++ b/plugins/permission-common/src/types/permission.ts @@ -14,6 +14,8 @@ * limitations under the License. */ +import { AuthorizeRequest, AuthorizeResponse } from './api'; + /** * The attributes related to a given permission; these should be generic and widely applicable to * all permissions in the system. @@ -39,3 +41,18 @@ export type Permission = { attributes: PermissionAttributes; resourceType?: string; }; + +export interface PermissionClientInterface { + authorize( + requests: AuthorizeRequest[], + options?: AuthorizeRequestOptions, + ): Promise; +} + +/** + * Options for authorization requests; currently only an optional auth token. + * @public + */ +export type AuthorizeRequestOptions = { + token?: string; +}; diff --git a/plugins/permission-node/src/ServerPermissionClient.ts b/plugins/permission-node/src/ServerPermissionClient.ts index 8ded6e51e2..734ecd73bb 100644 --- a/plugins/permission-node/src/ServerPermissionClient.ts +++ b/plugins/permission-node/src/ServerPermissionClient.ts @@ -17,18 +17,25 @@ import { TokenManager, ServerTokenManager } from '@backstage/backend-common'; import { Config } from '@backstage/config'; import { + AuthorizeRequest, AuthorizeRequestOptions, + AuthorizeResponse, + AuthorizeResult, DiscoveryApi, PermissionClient, + PermissionClientInterface, } from '@backstage/plugin-permission-common'; /** - * A server side {@link @backstage/plugin-permission-common#PermissionClient} - * that allows all backend-to-backend requests. + * A thin wrapper around + * {@link @backstage/plugin-permission-common#PermissionClient} that allows all + * backend-to-backend requests. * @public */ -export class ServerPermissionClient extends PermissionClient { +export class ServerPermissionClient implements PermissionClientInterface { private readonly serverTokenManager: TokenManager; + private readonly permissionClient: PermissionClient; + private readonly permissionEnabled: boolean; constructor(options: { discoveryApi: DiscoveryApi; @@ -36,10 +43,12 @@ export class ServerPermissionClient extends PermissionClient { serverTokenManager: TokenManager; }) { const { discoveryApi, configApi, serverTokenManager } = options; - super({ discoveryApi, configApi }); + this.permissionClient = new PermissionClient({ discoveryApi, configApi }); + this.permissionEnabled = + options.configApi.getOptionalBoolean('permission.enabled') ?? false; if ( - this.enabled && + this.permissionEnabled && // TODO: Find a cleaner way of ensuring usage of SERVER token manager when // permissions are enabled. serverTokenManager instanceof ServerTokenManager.noop().constructor @@ -51,18 +60,21 @@ export class ServerPermissionClient extends PermissionClient { this.serverTokenManager = serverTokenManager; } - async shouldBypass(options?: AuthorizeRequestOptions): Promise { - // 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; + async authorize( + requests: AuthorizeRequest[], + 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 })); } - if (await this.isValidServerToken(options?.token)) { - return true; - } - return false; + return this.permissionClient.authorize(requests, options); } private async isValidServerToken( From 0e8ec6d97402d292431939f410618e57e57a9a1d Mon Sep 17 00:00:00 2001 From: Joon Park Date: Fri, 17 Dec 2021 14:19:34 +0000 Subject: [PATCH 12/24] Rename all the things Signed-off-by: Joon Park --- .changeset/real-geckos-work.md | 5 ++ packages/app-defaults/src/defaults/apis.ts | 10 ++-- packages/backend/src/index.ts | 8 +-- plugins/permission-common/api-report.md | 17 +++--- .../src/PermissionClient.test.ts | 14 ++--- .../permission-common/src/PermissionClient.ts | 14 ++--- plugins/permission-common/src/types/index.ts | 2 +- .../permission-common/src/types/permission.ts | 8 ++- plugins/permission-node/api-report.md | 23 ++++---- .../src/ServerPermissionClient.test.ts | 49 +++++++++-------- .../src/ServerPermissionClient.ts | 52 +++++++++++++------ plugins/permission-react/api-report.md | 6 +-- .../src/apis/IdentityPermissionApi.ts | 12 ++--- 13 files changed, 131 insertions(+), 89 deletions(-) create mode 100644 .changeset/real-geckos-work.md diff --git a/.changeset/real-geckos-work.md b/.changeset/real-geckos-work.md new file mode 100644 index 0000000000..a62b18e50f --- /dev/null +++ b/.changeset/real-geckos-work.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-permission-common': patch +--- + +Create PermissionAuthorizer interface for PermissionClients diff --git a/packages/app-defaults/src/defaults/apis.ts b/packages/app-defaults/src/defaults/apis.ts index e56a27b81f..3500afe32a 100644 --- a/packages/app-defaults/src/defaults/apis.ts +++ b/packages/app-defaults/src/defaults/apis.ts @@ -304,11 +304,11 @@ export const apis = [ createApiFactory({ api: permissionApiRef, deps: { - discoveryApi: discoveryApiRef, - identityApi: identityApiRef, - configApi: configApiRef, + discovery: discoveryApiRef, + identity: identityApiRef, + config: configApiRef, }, - factory: ({ configApi, discoveryApi, identityApi }) => - IdentityPermissionApi.create({ configApi, discoveryApi, identityApi }), + factory: ({ config, discovery, identity }) => + IdentityPermissionApi.create({ config, discovery, identity }), }), ]; diff --git a/packages/backend/src/index.ts b/packages/backend/src/index.ts index 96def8d03f..c398650588 100644 --- a/packages/backend/src/index.ts +++ b/packages/backend/src/index.ts @@ -64,10 +64,10 @@ function makeCreateEnv(config: Config) { const reader = UrlReaders.default({ logger: root, config }); const discovery = SingleHostDiscovery.fromConfig(config); const tokenManager = ServerTokenManager.fromConfig(config, { logger: root }); - const permissions = new ServerPermissionClient({ - discoveryApi: discovery, - configApi: config, - serverTokenManager: tokenManager, + const permissions = ServerPermissionClient.create({ + discovery, + config, + tokenManager, }); root.info(`Created UrlReader ${reader}`); diff --git a/plugins/permission-common/api-report.md b/plugins/permission-common/api-report.md index cc2a433504..70795d14ac 100644 --- a/plugins/permission-common/api-report.md +++ b/plugins/permission-common/api-report.md @@ -68,16 +68,21 @@ export type PermissionAttributes = { }; // @public -export class PermissionClient { - constructor(options: { discoveryApi: DiscoveryApi; configApi: Config }); +export interface PermissionAuthorizer { + // (undocumented) + authorize( + requests: AuthorizeRequest[], + options?: AuthorizeRequestOptions, + ): Promise; +} + +// @public +export class PermissionClient implements PermissionAuthorizer { + constructor(options: { discovery: DiscoveryApi; config: Config }); authorize( requests: AuthorizeRequest[], options?: AuthorizeRequestOptions, ): Promise; - // (undocumented) - protected readonly enabled: boolean; - // (undocumented) - protected shouldBypass(_options?: AuthorizeRequestOptions): Promise; } // @public diff --git a/plugins/permission-common/src/PermissionClient.test.ts b/plugins/permission-common/src/PermissionClient.test.ts index b2e66986a1..a432735a5e 100644 --- a/plugins/permission-common/src/PermissionClient.test.ts +++ b/plugins/permission-common/src/PermissionClient.test.ts @@ -26,14 +26,14 @@ const server = setupServer(); const token = 'fake-token'; const mockBaseUrl = 'http://backstage:9191/i-am-a-mock-base'; -const discoveryApi: DiscoveryApi = { +const discovery: DiscoveryApi = { async getBaseUrl() { return mockBaseUrl; }, }; const client: PermissionClient = new PermissionClient({ - discoveryApi, - configApi: new ConfigReader({ permission: { enabled: true } }), + discovery, + config: new ConfigReader({ permission: { enabled: true } }), }); const mockPermission: Permission = { @@ -158,8 +158,8 @@ describe('PermissionClient', () => { }, ); const disabled = new PermissionClient({ - discoveryApi, - configApi: new ConfigReader({ permission: { enabled: false } }), + discovery, + config: new ConfigReader({ permission: { enabled: false } }), }); const response = await disabled.authorize([mockAuthorizeRequest]); expect(response[0]).toEqual( @@ -180,8 +180,8 @@ describe('PermissionClient', () => { }, ); const disabled = new PermissionClient({ - discoveryApi, - configApi: new ConfigReader({}), + discovery, + config: new ConfigReader({}), }); const response = await disabled.authorize([mockAuthorizeRequest]); expect(response[0]).toEqual( diff --git a/plugins/permission-common/src/PermissionClient.ts b/plugins/permission-common/src/PermissionClient.ts index c61a9cadd9..62e5ec8172 100644 --- a/plugins/permission-common/src/PermissionClient.ts +++ b/plugins/permission-common/src/PermissionClient.ts @@ -29,7 +29,7 @@ import { } from './types/api'; import { DiscoveryApi } from './types/discovery'; import { - PermissionClientInterface, + PermissionAuthorizer, AuthorizeRequestOptions, } from './types/permission'; @@ -67,14 +67,14 @@ const responseSchema = z.array( * An isomorphic client for requesting authorization for Backstage permissions. * @public */ -export class PermissionClient implements PermissionClientInterface { +export class PermissionClient implements PermissionAuthorizer { private readonly enabled: boolean; - private readonly discoveryApi: DiscoveryApi; + private readonly discovery: DiscoveryApi; - constructor(options: { discoveryApi: DiscoveryApi; configApi: Config }) { - this.discoveryApi = options.discoveryApi; + constructor(options: { discovery: DiscoveryApi; config: Config }) { + this.discovery = options.discovery; this.enabled = - options.configApi.getOptionalBoolean('permission.enabled') ?? false; + options.config.getOptionalBoolean('permission.enabled') ?? false; } /** @@ -113,7 +113,7 @@ export class PermissionClient implements PermissionClientInterface { }), ); - const permissionApi = await this.discoveryApi.getBaseUrl('permission'); + const permissionApi = await this.discovery.getBaseUrl('permission'); const response = await fetch(`${permissionApi}/authorize`, { method: 'POST', body: JSON.stringify(identifiedRequests), diff --git a/plugins/permission-common/src/types/index.ts b/plugins/permission-common/src/types/index.ts index 4d28cca7b5..583a3577bd 100644 --- a/plugins/permission-common/src/types/index.ts +++ b/plugins/permission-common/src/types/index.ts @@ -26,6 +26,6 @@ export type { DiscoveryApi } from './discovery'; export type { PermissionAttributes, Permission, - PermissionClientInterface, + PermissionAuthorizer, AuthorizeRequestOptions, } from './permission'; diff --git a/plugins/permission-common/src/types/permission.ts b/plugins/permission-common/src/types/permission.ts index a9c82d6a20..246f1bec72 100644 --- a/plugins/permission-common/src/types/permission.ts +++ b/plugins/permission-common/src/types/permission.ts @@ -42,7 +42,11 @@ export type Permission = { resourceType?: string; }; -export interface PermissionClientInterface { +/** + * A client interacting with the permission backend can implement this authorizer interface. + * @public + */ +export interface PermissionAuthorizer { authorize( requests: AuthorizeRequest[], options?: AuthorizeRequestOptions, @@ -50,7 +54,7 @@ export interface PermissionClientInterface { } /** - * Options for authorization requests; currently only an optional auth token. + * Options for authorization requests. * @public */ export type AuthorizeRequestOptions = { diff --git a/plugins/permission-node/api-report.md b/plugins/permission-node/api-report.md index 90181925c4..f41350231f 100644 --- a/plugins/permission-node/api-report.md +++ b/plugins/permission-node/api-report.md @@ -5,13 +5,14 @@ ```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'; -import { DiscoveryApi } from '@backstage/plugin-permission-common'; -import { PermissionClient } 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 { PluginEndpointDiscovery } from '@backstage/backend-common'; import { Router } from 'express'; import { TokenManager } from '@backstage/backend-common'; @@ -126,13 +127,17 @@ export type PolicyDecision = | ConditionalPolicyDecision; // @public -export class ServerPermissionClient extends PermissionClient { - constructor(options: { - discoveryApi: DiscoveryApi; - configApi: Config; - serverTokenManager: TokenManager; - }); +export class ServerPermissionClient implements PermissionAuthorizer { // (undocumented) - shouldBypass(options?: AuthorizeRequestOptions): Promise; + authorize( + requests: AuthorizeRequest[], + options?: AuthorizeRequestOptions, + ): Promise; + // (undocumented) + static create(options: { + discovery: PluginEndpointDiscovery; + config: Config; + tokenManager: TokenManager; + }): ServerPermissionClient; } ``` diff --git a/plugins/permission-node/src/ServerPermissionClient.test.ts b/plugins/permission-node/src/ServerPermissionClient.test.ts index 8efd6414a7..5965247c16 100644 --- a/plugins/permission-node/src/ServerPermissionClient.test.ts +++ b/plugins/permission-node/src/ServerPermissionClient.test.ts @@ -16,14 +16,17 @@ 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 { + getVoidLogger, + PluginEndpointDiscovery, + ServerTokenManager, +} from '@backstage/backend-common'; import { setupServer } from 'msw/node'; import { RestContext, rest } from 'msw'; @@ -37,10 +40,13 @@ const mockAuthorizeHandler = jest.fn((req, res, { json }: RestContext) => { return res(json(responses)); }); const mockBaseUrl = 'http://backstage:9191/i-am-a-mock-base'; -const discoveryApi: DiscoveryApi = { +const discovery: PluginEndpointDiscovery = { async getBaseUrl() { return mockBaseUrl; }, + async getExternalBaseUrl() { + return mockBaseUrl; + }, }; const testPermission: Permission = { name: 'test.permission', @@ -62,10 +68,10 @@ describe('ServerPermissionClient', () => { afterEach(() => server.resetHandlers()); it('should bypass authorization if permissions are disabled', async () => { - const client = new ServerPermissionClient({ - discoveryApi, - configApi: new ConfigReader({}), - serverTokenManager: ServerTokenManager.noop(), + const client = ServerPermissionClient.create({ + discovery, + config: new ConfigReader({}), + tokenManager: ServerTokenManager.noop(), }); await client.authorize([{ permission: testPermission }]); @@ -75,10 +81,10 @@ describe('ServerPermissionClient', () => { 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, + const client = ServerPermissionClient.create({ + discovery, + config, + tokenManager, }); await client.authorize([{ permission: testPermission }], { @@ -90,10 +96,10 @@ describe('ServerPermissionClient', () => { 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, + const client = ServerPermissionClient.create({ + discovery, + config, + tokenManager, }); await client.authorize([{ permission: testPermission }], { @@ -104,13 +110,12 @@ describe('ServerPermissionClient', () => { }); 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(), - }), + expect(() => + ServerPermissionClient.create({ + discovery, + config, + tokenManager: ServerTokenManager.noop(), + }), ).toThrowError( 'You must configure at least one key in backend.auth.keys if permissions are enabled.', ); diff --git a/plugins/permission-node/src/ServerPermissionClient.ts b/plugins/permission-node/src/ServerPermissionClient.ts index 734ecd73bb..51c9bcf04f 100644 --- a/plugins/permission-node/src/ServerPermissionClient.ts +++ b/plugins/permission-node/src/ServerPermissionClient.ts @@ -14,16 +14,19 @@ * limitations under the License. */ -import { TokenManager, ServerTokenManager } from '@backstage/backend-common'; +import { + TokenManager, + ServerTokenManager, + PluginEndpointDiscovery, +} from '@backstage/backend-common'; import { Config } from '@backstage/config'; import { AuthorizeRequest, AuthorizeRequestOptions, AuthorizeResponse, AuthorizeResult, - DiscoveryApi, PermissionClient, - PermissionClientInterface, + PermissionAuthorizer, } from '@backstage/plugin-permission-common'; /** @@ -32,32 +35,47 @@ import { * backend-to-backend requests. * @public */ -export class ServerPermissionClient implements PermissionClientInterface { - private readonly serverTokenManager: TokenManager; +export class ServerPermissionClient implements PermissionAuthorizer { private readonly permissionClient: PermissionClient; + private readonly tokenManager: TokenManager; private readonly permissionEnabled: boolean; - constructor(options: { - discoveryApi: DiscoveryApi; - configApi: Config; - serverTokenManager: TokenManager; + static create(options: { + discovery: PluginEndpointDiscovery; + config: Config; + tokenManager: TokenManager; }) { - const { discoveryApi, configApi, serverTokenManager } = options; - this.permissionClient = new PermissionClient({ discoveryApi, configApi }); - this.permissionEnabled = - options.configApi.getOptionalBoolean('permission.enabled') ?? false; + const { discovery, config, tokenManager } = options; + const permissionClient = new PermissionClient({ discovery, config }); + const permissionEnabled = + config.getOptionalBoolean('permission.enabled') ?? false; if ( - this.permissionEnabled && + permissionEnabled && // TODO: Find a cleaner way of ensuring usage of SERVER token manager when // permissions are enabled. - serverTokenManager instanceof ServerTokenManager.noop().constructor + tokenManager instanceof ServerTokenManager.noop().constructor ) { throw new Error( 'You must configure at least one key in backend.auth.keys if permissions are enabled.', ); } - this.serverTokenManager = serverTokenManager; + + return new ServerPermissionClient({ + permissionClient, + tokenManager, + permissionEnabled, + }); + } + + private constructor(options: { + permissionClient: PermissionClient; + tokenManager: TokenManager; + permissionEnabled: boolean; + }) { + this.permissionClient = options.permissionClient; + this.tokenManager = options.tokenManager; + this.permissionEnabled = options.permissionEnabled; } async authorize( @@ -83,7 +101,7 @@ export class ServerPermissionClient implements PermissionClientInterface { if (!token) { return false; } - return this.serverTokenManager + return this.tokenManager .authenticate(token) .then(() => true) .catch(() => false); diff --git a/plugins/permission-react/api-report.md b/plugins/permission-react/api-report.md index c10317495a..799b3ad54f 100644 --- a/plugins/permission-react/api-report.md +++ b/plugins/permission-react/api-report.md @@ -27,9 +27,9 @@ export class IdentityPermissionApi implements PermissionApi { authorize(request: AuthorizeRequest): Promise; // (undocumented) static create(options: { - configApi: Config; - discoveryApi: DiscoveryApi; - identityApi: IdentityApi; + config: Config; + discovery: DiscoveryApi; + identity: IdentityApi; }): IdentityPermissionApi; } diff --git a/plugins/permission-react/src/apis/IdentityPermissionApi.ts b/plugins/permission-react/src/apis/IdentityPermissionApi.ts index 19de564f84..818eca17fa 100644 --- a/plugins/permission-react/src/apis/IdentityPermissionApi.ts +++ b/plugins/permission-react/src/apis/IdentityPermissionApi.ts @@ -35,13 +35,13 @@ export class IdentityPermissionApi implements PermissionApi { ) {} static create(options: { - configApi: Config; - discoveryApi: DiscoveryApi; - identityApi: IdentityApi; + config: Config; + discovery: DiscoveryApi; + identity: IdentityApi; }) { - const { configApi, discoveryApi, identityApi } = options; - const permissionClient = new PermissionClient({ discoveryApi, configApi }); - return new IdentityPermissionApi(permissionClient, identityApi); + const { config, discovery, identity } = options; + const permissionClient = new PermissionClient({ discovery, config }); + return new IdentityPermissionApi(permissionClient, identity); } async authorize(request: AuthorizeRequest): Promise { From f898c014ca2ec4eab0b9458c077f18897785fdb5 Mon Sep 17 00:00:00 2001 From: Joon Park Date: Fri, 17 Dec 2021 15:32:09 +0000 Subject: [PATCH 13/24] Add explicit instance variable to denote the given token manager's scope of authentication Signed-off-by: Joon Park --- packages/app-defaults/src/defaults/apis.ts | 1 - packages/backend-common/api-report.md | 3 +++ packages/backend-common/src/tokens/ServerTokenManager.ts | 3 +++ packages/backend-common/src/tokens/types.ts | 6 ++++++ .../src/search/DefaultCatalogCollator.test.ts | 1 + plugins/permission-node/src/ServerPermissionClient.ts | 8 +------- .../src/search/DefaultTechDocsCollator.test.ts | 2 ++ 7 files changed, 16 insertions(+), 8 deletions(-) diff --git a/packages/app-defaults/src/defaults/apis.ts b/packages/app-defaults/src/defaults/apis.ts index 3500afe32a..f26c298fb1 100644 --- a/packages/app-defaults/src/defaults/apis.ts +++ b/packages/app-defaults/src/defaults/apis.ts @@ -61,7 +61,6 @@ import { oidcAuthApiRef, bitbucketAuthApiRef, atlassianAuthApiRef, - identityApiRef, } from '@backstage/core-plugin-api'; import { permissionApiRef, diff --git a/packages/backend-common/api-report.md b/packages/backend-common/api-report.md index b929251890..c774e6baa3 100644 --- a/packages/backend-common/api-report.md +++ b/packages/backend-common/api-report.md @@ -502,6 +502,8 @@ export class ServerTokenManager implements TokenManager { token: string; }>; // (undocumented) + readonly isSecure: boolean; + // (undocumented) static noop(): TokenManager; } @@ -572,6 +574,7 @@ export interface TokenManager { getToken: () => Promise<{ token: string; }>; + isSecure: boolean; } // @public diff --git a/packages/backend-common/src/tokens/ServerTokenManager.ts b/packages/backend-common/src/tokens/ServerTokenManager.ts index 7fbd18e8c3..82ef5b8401 100644 --- a/packages/backend-common/src/tokens/ServerTokenManager.ts +++ b/packages/backend-common/src/tokens/ServerTokenManager.ts @@ -21,6 +21,8 @@ import { TokenManager } from './types'; import { Logger } from 'winston'; class NoopTokenManager implements TokenManager { + public readonly isSecure: boolean = false; + async getToken() { return { token: '' }; } @@ -37,6 +39,7 @@ class NoopTokenManager implements TokenManager { export class ServerTokenManager implements TokenManager { private readonly verificationKeys: JWKS.KeyStore; private readonly signingKey: JWK.Key; + public readonly isSecure: boolean = true; static noop(): TokenManager { return new NoopTokenManager(); diff --git a/packages/backend-common/src/tokens/types.ts b/packages/backend-common/src/tokens/types.ts index 1fea018db9..2be1936885 100644 --- a/packages/backend-common/src/tokens/types.ts +++ b/packages/backend-common/src/tokens/types.ts @@ -20,6 +20,12 @@ * @public */ export interface TokenManager { + /** + * This property should be true when the token manager is expected to only + * authenticate tokens created by itself, or an equivalently-constructed + * instance. + */ + isSecure: boolean; getToken: () => Promise<{ token: string }>; authenticate: (token: string) => Promise; } diff --git a/plugins/catalog-backend/src/search/DefaultCatalogCollator.test.ts b/plugins/catalog-backend/src/search/DefaultCatalogCollator.test.ts index 1360ca2647..81f728b862 100644 --- a/plugins/catalog-backend/src/search/DefaultCatalogCollator.test.ts +++ b/plugins/catalog-backend/src/search/DefaultCatalogCollator.test.ts @@ -67,6 +67,7 @@ describe('DefaultCatalogCollator', () => { getExternalBaseUrl: jest.fn(), }; mockTokenManager = { + isSecure: true, getToken: jest.fn().mockResolvedValue({ token: '' }), authenticate: jest.fn(), }; diff --git a/plugins/permission-node/src/ServerPermissionClient.ts b/plugins/permission-node/src/ServerPermissionClient.ts index 51c9bcf04f..e9438a9162 100644 --- a/plugins/permission-node/src/ServerPermissionClient.ts +++ b/plugins/permission-node/src/ServerPermissionClient.ts @@ -16,7 +16,6 @@ import { TokenManager, - ServerTokenManager, PluginEndpointDiscovery, } from '@backstage/backend-common'; import { Config } from '@backstage/config'; @@ -50,12 +49,7 @@ export class ServerPermissionClient implements PermissionAuthorizer { const permissionEnabled = config.getOptionalBoolean('permission.enabled') ?? false; - if ( - permissionEnabled && - // TODO: Find a cleaner way of ensuring usage of SERVER token manager when - // permissions are enabled. - tokenManager instanceof ServerTokenManager.noop().constructor - ) { + if (permissionEnabled && !tokenManager.isSecure) { throw new Error( 'You must configure at least one key in backend.auth.keys if permissions are enabled.', ); diff --git a/plugins/techdocs-backend/src/search/DefaultTechDocsCollator.test.ts b/plugins/techdocs-backend/src/search/DefaultTechDocsCollator.test.ts index 5f0bed55dd..6f38d5291d 100644 --- a/plugins/techdocs-backend/src/search/DefaultTechDocsCollator.test.ts +++ b/plugins/techdocs-backend/src/search/DefaultTechDocsCollator.test.ts @@ -99,6 +99,7 @@ describe('DefaultTechDocsCollator with legacyPathCasing configuration', () => { getExternalBaseUrl: jest.fn(), }; mockTokenManager = { + isSecure: true, getToken: jest.fn().mockResolvedValue({ token: '' }), authenticate: jest.fn(), }; @@ -165,6 +166,7 @@ describe('DefaultTechDocsCollator', () => { getExternalBaseUrl: jest.fn(), }; mockTokenManager = { + isSecure: true, getToken: jest.fn().mockResolvedValue({ token: '' }), authenticate: jest.fn(), }; From c829631b4af3ee18fe2e8b52563a49e5ba73257b Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Mon, 20 Dec 2021 17:34:25 +0000 Subject: [PATCH 14/24] permission-node: use filename import in ServerPermissionClient test suite Signed-off-by: MT Lewis --- plugins/permission-node/src/ServerPermissionClient.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/permission-node/src/ServerPermissionClient.test.ts b/plugins/permission-node/src/ServerPermissionClient.test.ts index 5965247c16..cd15ba7439 100644 --- a/plugins/permission-node/src/ServerPermissionClient.test.ts +++ b/plugins/permission-node/src/ServerPermissionClient.test.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { ServerPermissionClient } from '.'; +import { ServerPermissionClient } from './ServerPermissionClient'; import { Permission, Identified, From 20d10b57d6373b09510fbe964427849c613a1890 Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Tue, 21 Dec 2021 09:46:22 +0000 Subject: [PATCH 15/24] permission-node: rename static create method to fromConfig Signed-off-by: MT Lewis --- packages/backend/src/index.ts | 3 +-- plugins/permission-node/api-report.md | 12 +++++++----- .../src/ServerPermissionClient.test.ts | 12 ++++-------- .../permission-node/src/ServerPermissionClient.ts | 14 ++++++++------ 4 files changed, 20 insertions(+), 21 deletions(-) diff --git a/packages/backend/src/index.ts b/packages/backend/src/index.ts index c398650588..a83abc218c 100644 --- a/packages/backend/src/index.ts +++ b/packages/backend/src/index.ts @@ -64,9 +64,8 @@ function makeCreateEnv(config: Config) { const reader = UrlReaders.default({ logger: root, config }); const discovery = SingleHostDiscovery.fromConfig(config); const tokenManager = ServerTokenManager.fromConfig(config, { logger: root }); - const permissions = ServerPermissionClient.create({ + const permissions = ServerPermissionClient.fromConfig(config, { discovery, - config, tokenManager, }); diff --git a/plugins/permission-node/api-report.md b/plugins/permission-node/api-report.md index f41350231f..25a8be06c8 100644 --- a/plugins/permission-node/api-report.md +++ b/plugins/permission-node/api-report.md @@ -134,10 +134,12 @@ export class ServerPermissionClient implements PermissionAuthorizer { options?: AuthorizeRequestOptions, ): Promise; // (undocumented) - static create(options: { - discovery: PluginEndpointDiscovery; - config: Config; - tokenManager: TokenManager; - }): ServerPermissionClient; + static fromConfig( + config: Config, + options: { + discovery: PluginEndpointDiscovery; + tokenManager: TokenManager; + }, + ): ServerPermissionClient; } ``` diff --git a/plugins/permission-node/src/ServerPermissionClient.test.ts b/plugins/permission-node/src/ServerPermissionClient.test.ts index cd15ba7439..2c29ef8939 100644 --- a/plugins/permission-node/src/ServerPermissionClient.test.ts +++ b/plugins/permission-node/src/ServerPermissionClient.test.ts @@ -68,9 +68,8 @@ describe('ServerPermissionClient', () => { afterEach(() => server.resetHandlers()); it('should bypass authorization if permissions are disabled', async () => { - const client = ServerPermissionClient.create({ + const client = ServerPermissionClient.fromConfig(new ConfigReader({}), { discovery, - config: new ConfigReader({}), tokenManager: ServerTokenManager.noop(), }); @@ -81,9 +80,8 @@ describe('ServerPermissionClient', () => { it('should bypass authorization if permissions are enabled and request has valid server token', async () => { const tokenManager = ServerTokenManager.fromConfig(config, { logger }); - const client = ServerPermissionClient.create({ + const client = ServerPermissionClient.fromConfig(config, { discovery, - config, tokenManager, }); @@ -96,9 +94,8 @@ describe('ServerPermissionClient', () => { 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.create({ + const client = ServerPermissionClient.fromConfig(config, { discovery, - config, tokenManager, }); @@ -111,9 +108,8 @@ describe('ServerPermissionClient', () => { it('should error if permissions are enabled but a no-op token manager is configured', async () => { expect(() => - ServerPermissionClient.create({ + ServerPermissionClient.fromConfig(config, { discovery, - config, tokenManager: ServerTokenManager.noop(), }), ).toThrowError( diff --git a/plugins/permission-node/src/ServerPermissionClient.ts b/plugins/permission-node/src/ServerPermissionClient.ts index e9438a9162..a6f3488851 100644 --- a/plugins/permission-node/src/ServerPermissionClient.ts +++ b/plugins/permission-node/src/ServerPermissionClient.ts @@ -39,12 +39,14 @@ export class ServerPermissionClient implements PermissionAuthorizer { private readonly tokenManager: TokenManager; private readonly permissionEnabled: boolean; - static create(options: { - discovery: PluginEndpointDiscovery; - config: Config; - tokenManager: TokenManager; - }) { - const { discovery, config, tokenManager } = options; + static fromConfig( + config: Config, + options: { + discovery: PluginEndpointDiscovery; + tokenManager: TokenManager; + }, + ) { + const { discovery, tokenManager } = options; const permissionClient = new PermissionClient({ discovery, config }); const permissionEnabled = config.getOptionalBoolean('permission.enabled') ?? false; From bc9a205b864bb9fdde0b904ebc63faf9de8ea286 Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Tue, 21 Dec 2021 09:50:54 +0000 Subject: [PATCH 16/24] backend-common: remove isSecure property in favour of a property on the NoopServerTokenManager Signed-off-by: MT Lewis --- packages/backend-common/api-report.md | 3 --- packages/backend-common/src/tokens/ServerTokenManager.ts | 3 +-- packages/backend-common/src/tokens/types.ts | 6 ------ .../src/search/DefaultCatalogCollator.test.ts | 1 - plugins/permission-node/src/ServerPermissionClient.ts | 5 ++++- .../src/search/DefaultTechDocsCollator.test.ts | 2 -- 6 files changed, 5 insertions(+), 15 deletions(-) diff --git a/packages/backend-common/api-report.md b/packages/backend-common/api-report.md index c774e6baa3..b929251890 100644 --- a/packages/backend-common/api-report.md +++ b/packages/backend-common/api-report.md @@ -502,8 +502,6 @@ export class ServerTokenManager implements TokenManager { token: string; }>; // (undocumented) - readonly isSecure: boolean; - // (undocumented) static noop(): TokenManager; } @@ -574,7 +572,6 @@ export interface TokenManager { getToken: () => Promise<{ token: string; }>; - isSecure: boolean; } // @public diff --git a/packages/backend-common/src/tokens/ServerTokenManager.ts b/packages/backend-common/src/tokens/ServerTokenManager.ts index 82ef5b8401..a3481c2f97 100644 --- a/packages/backend-common/src/tokens/ServerTokenManager.ts +++ b/packages/backend-common/src/tokens/ServerTokenManager.ts @@ -21,7 +21,7 @@ import { TokenManager } from './types'; import { Logger } from 'winston'; class NoopTokenManager implements TokenManager { - public readonly isSecure: boolean = false; + public readonly isInsecureServerTokenManager: boolean = true; async getToken() { return { token: '' }; @@ -39,7 +39,6 @@ class NoopTokenManager implements TokenManager { export class ServerTokenManager implements TokenManager { private readonly verificationKeys: JWKS.KeyStore; private readonly signingKey: JWK.Key; - public readonly isSecure: boolean = true; static noop(): TokenManager { return new NoopTokenManager(); diff --git a/packages/backend-common/src/tokens/types.ts b/packages/backend-common/src/tokens/types.ts index 2be1936885..1fea018db9 100644 --- a/packages/backend-common/src/tokens/types.ts +++ b/packages/backend-common/src/tokens/types.ts @@ -20,12 +20,6 @@ * @public */ export interface TokenManager { - /** - * This property should be true when the token manager is expected to only - * authenticate tokens created by itself, or an equivalently-constructed - * instance. - */ - isSecure: boolean; getToken: () => Promise<{ token: string }>; authenticate: (token: string) => Promise; } diff --git a/plugins/catalog-backend/src/search/DefaultCatalogCollator.test.ts b/plugins/catalog-backend/src/search/DefaultCatalogCollator.test.ts index 81f728b862..1360ca2647 100644 --- a/plugins/catalog-backend/src/search/DefaultCatalogCollator.test.ts +++ b/plugins/catalog-backend/src/search/DefaultCatalogCollator.test.ts @@ -67,7 +67,6 @@ describe('DefaultCatalogCollator', () => { getExternalBaseUrl: jest.fn(), }; mockTokenManager = { - isSecure: true, getToken: jest.fn().mockResolvedValue({ token: '' }), authenticate: jest.fn(), }; diff --git a/plugins/permission-node/src/ServerPermissionClient.ts b/plugins/permission-node/src/ServerPermissionClient.ts index a6f3488851..c85cc7048b 100644 --- a/plugins/permission-node/src/ServerPermissionClient.ts +++ b/plugins/permission-node/src/ServerPermissionClient.ts @@ -51,7 +51,10 @@ export class ServerPermissionClient implements PermissionAuthorizer { const permissionEnabled = config.getOptionalBoolean('permission.enabled') ?? false; - if (permissionEnabled && !tokenManager.isSecure) { + if ( + permissionEnabled && + (tokenManager as any).isInsecureServerTokenManager + ) { throw new Error( 'You must configure at least one key in backend.auth.keys if permissions are enabled.', ); diff --git a/plugins/techdocs-backend/src/search/DefaultTechDocsCollator.test.ts b/plugins/techdocs-backend/src/search/DefaultTechDocsCollator.test.ts index 6f38d5291d..5f0bed55dd 100644 --- a/plugins/techdocs-backend/src/search/DefaultTechDocsCollator.test.ts +++ b/plugins/techdocs-backend/src/search/DefaultTechDocsCollator.test.ts @@ -99,7 +99,6 @@ describe('DefaultTechDocsCollator with legacyPathCasing configuration', () => { getExternalBaseUrl: jest.fn(), }; mockTokenManager = { - isSecure: true, getToken: jest.fn().mockResolvedValue({ token: '' }), authenticate: jest.fn(), }; @@ -166,7 +165,6 @@ describe('DefaultTechDocsCollator', () => { getExternalBaseUrl: jest.fn(), }; mockTokenManager = { - isSecure: true, getToken: jest.fn().mockResolvedValue({ token: '' }), authenticate: jest.fn(), }; From c1c7116f4b88e8fb13a97b99793fd636aa4bf74f Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Tue, 21 Dec 2021 10:08:19 +0000 Subject: [PATCH 17/24] backend-common: improve error message in ServerTokenManager Signed-off-by: MT Lewis --- packages/backend-common/src/tokens/ServerTokenManager.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/backend-common/src/tokens/ServerTokenManager.ts b/packages/backend-common/src/tokens/ServerTokenManager.ts index a3481c2f97..d6693edba1 100644 --- a/packages/backend-common/src/tokens/ServerTokenManager.ts +++ b/packages/backend-common/src/tokens/ServerTokenManager.ts @@ -60,7 +60,7 @@ export class ServerTokenManager implements TokenManager { // For development, if a secret has not been configured, we auto generate a secret instead of throwing. const generatedDevOnlyKey = JWK.generateSync('oct', 24 * 8); if (generatedDevOnlyKey.k === undefined) { - throw new Error('No key generated'); + throw new Error('Internal error, JWK key generation returned no data'); } logger.warn( 'Generated a secret for backend-to-backend authentication: DEVELOPMENT USE ONLY.', From 4320aea8794f200f7b8a2d26e55c44d73b497eb5 Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Tue, 21 Dec 2021 10:16:50 +0000 Subject: [PATCH 18/24] docs: add backend-to-backend auth tutorial to microsite Signed-off-by: MT Lewis --- microsite/sidebars.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/microsite/sidebars.json b/microsite/sidebars.json index 89eb282d14..6f9b9a00c3 100644 --- a/microsite/sidebars.json +++ b/microsite/sidebars.json @@ -270,7 +270,8 @@ "tutorials/migrating-away-from-core", "tutorials/configuring-plugin-databases", "tutorials/switching-sqlite-postgres", - "tutorials/using-backstage-proxy-within-plugin" + "tutorials/using-backstage-proxy-within-plugin", + "tutorials/backend-to-backend-auth" ], "Architecture Decision Records (ADRs)": [ "architecture-decisions/adrs-overview", From 6fe4f20cbdff7d60fdb1e57d94dadb2c66ce00f6 Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Tue, 21 Dec 2021 10:18:22 +0000 Subject: [PATCH 19/24] backend-common: expand secret generation changeset Signed-off-by: MT Lewis --- .changeset/lazy-files-check.md | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/.changeset/lazy-files-check.md b/.changeset/lazy-files-check.md index 30ae18dd7f..d8f1298a89 100644 --- a/.changeset/lazy-files-check.md +++ b/.changeset/lazy-files-check.md @@ -2,4 +2,13 @@ '@backstage/backend-common': minor --- -Generate backend-to-backend secret for dev mode +Auto-generate secrets for backend-to-backend auth in local development environments. + +When NODE_ENV is 'development', the ServerTokenManager will now generate a secret for backend-to-backend auth to make it simpler to work locally on Backstage instances that use backend-to-backend auth. A secret must still be manually configured as described in [the backend-to-backend auth tutorial](https://backstage.io/docs/tutorials/backend-to-backend-auth). + +After the change, the static `fromConfig` method on the `ServerTokenManager` requires a logger. + +```diff +- const tokenManager = ServerTokenManager.fromConfig(config); ++ const tokenManager = ServerTokenManager.fromConfig(config, { logger: root }); +``` From 1e49c23eb74ff853677872165a61ba7c5fd58f12 Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Tue, 21 Dec 2021 10:29:32 +0000 Subject: [PATCH 20/24] app-defaults: add changeset for inclusion of permission api in default apis Signed-off-by: MT Lewis --- .changeset/bright-ears-visit.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/bright-ears-visit.md diff --git a/.changeset/bright-ears-visit.md b/.changeset/bright-ears-visit.md new file mode 100644 index 0000000000..350cc5b094 --- /dev/null +++ b/.changeset/bright-ears-visit.md @@ -0,0 +1,5 @@ +--- +'@backstage/app-defaults': patch +--- + +Added an instance of PermissionApi to the apis included by default in createApp. From 482e4a4acd3bfffc70a03ff2c5d877565052ddfb Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Tue, 21 Dec 2021 10:42:11 +0000 Subject: [PATCH 21/24] permission-common: upgrade changeset to minor bump and describe breaking change Signed-off-by: MT Lewis --- .changeset/real-geckos-work.md | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/.changeset/real-geckos-work.md b/.changeset/real-geckos-work.md index a62b18e50f..549445e079 100644 --- a/.changeset/real-geckos-work.md +++ b/.changeset/real-geckos-work.md @@ -1,5 +1,15 @@ --- -'@backstage/plugin-permission-common': patch +'@backstage/plugin-permission-common': minor --- -Create PermissionAuthorizer interface for PermissionClients +- Add `PermissionAuthorizer` interface matching `PermissionClient` to allow alternative implementations like the `ServerPermissionClient` in @backstage/plugin-permission-node. + +Breaking Changes: + +- Remove "api" suffixes from constructor parameters in PermissionClient + +```diff + const { config, discovery } = options; +- const permissionClient = new PermissionClient({ discoveryApi: discovery, configApi: config }); ++ const permissionClient = new PermissionClient({ discovery, config }); +``` From f80c7232aeb38e7079d627104498e41dbbea5832 Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Tue, 21 Dec 2021 10:45:16 +0000 Subject: [PATCH 22/24] permission-node: expand ServerPermissionClient changeset Signed-off-by: MT Lewis --- .changeset/wet-bikes-pull.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.changeset/wet-bikes-pull.md b/.changeset/wet-bikes-pull.md index dcc3c34890..1c1c98b253 100644 --- a/.changeset/wet-bikes-pull.md +++ b/.changeset/wet-bikes-pull.md @@ -2,4 +2,6 @@ '@backstage/plugin-permission-node': patch --- -Add ServerPermissionClient to check for backend-to-backend tokens +Add `ServerPermissionClient`, which implements `PermissionAuthorizer` from @backstage/plugin-permission-common. This implementation skips authorization entirely when the supplied token is a valid backend-to-backend token, thereby allowing backend-to-backend systems to communicate without authorization. + +The `ServerPermissionClient` should always be used over the standard `PermissionClient` in plugin backends. From 70281a475b44d28d26626cf002d68a5433ac9aa3 Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Tue, 21 Dec 2021 10:49:06 +0000 Subject: [PATCH 23/24] permission-react: add changeset for IdentityPermissionApi.create signature change Signed-off-by: MT Lewis --- .changeset/famous-icons-pump.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 .changeset/famous-icons-pump.md diff --git a/.changeset/famous-icons-pump.md b/.changeset/famous-icons-pump.md new file mode 100644 index 0000000000..c2a5f07292 --- /dev/null +++ b/.changeset/famous-icons-pump.md @@ -0,0 +1,17 @@ +--- +'@backstage/plugin-permission-react': minor +--- + +Breaking Changes: + +- Remove "api" suffixes from constructor parameters in IdentityPermissionApi.create + +```diff + const { config, discovery, identity } = options; +- const permissionApi = IdentityPermissionApi.create({ +- configApi: config, +- discoveryApi: discovery, +- identityApi: identity +- }); ++ const permissionApi = IdentityPermissionApi.create({ config, discovery, identity }); +``` From ebed29d928130bac63e5bdf463846431b29c5610 Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Tue, 21 Dec 2021 11:19:46 +0000 Subject: [PATCH 24/24] backend-common: changeset clarification Signed-off-by: MT Lewis --- .changeset/lazy-files-check.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/lazy-files-check.md b/.changeset/lazy-files-check.md index d8f1298a89..54c53be434 100644 --- a/.changeset/lazy-files-check.md +++ b/.changeset/lazy-files-check.md @@ -4,7 +4,7 @@ Auto-generate secrets for backend-to-backend auth in local development environments. -When NODE_ENV is 'development', the ServerTokenManager will now generate a secret for backend-to-backend auth to make it simpler to work locally on Backstage instances that use backend-to-backend auth. A secret must still be manually configured as described in [the backend-to-backend auth tutorial](https://backstage.io/docs/tutorials/backend-to-backend-auth). +When NODE_ENV is 'development', the ServerTokenManager will now generate a secret for backend-to-backend auth to make it simpler to work locally on Backstage instances that use backend-to-backend auth. For production deployments, a secret must still be manually configured as described in [the backend-to-backend auth tutorial](https://backstage.io/docs/tutorials/backend-to-backend-auth). After the change, the static `fromConfig` method on the `ServerTokenManager` requires a logger.