diff --git a/.changeset/new-dryers-cough-2.md b/.changeset/new-dryers-cough-2.md new file mode 100644 index 0000000000..436bb3629c --- /dev/null +++ b/.changeset/new-dryers-cough-2.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-auth-backend-module-oidc-provider': patch +--- + +Simplify the `start` method in the `authenticator` to just return the helper promise diff --git a/.changeset/new-dryers-cough.md b/.changeset/new-dryers-cough.md new file mode 100644 index 0000000000..5be2dd761a --- /dev/null +++ b/.changeset/new-dryers-cough.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-auth-node': patch +--- + +Add an `error` handler to the `strategy` to reject the `executeRedirectStrategy` diff --git a/plugins/auth-backend-module-oidc-provider/src/authenticator.ts b/plugins/auth-backend-module-oidc-provider/src/authenticator.ts index efcf1b61dd..c396ea7034 100644 --- a/plugins/auth-backend-module-oidc-provider/src/authenticator.ts +++ b/plugins/auth-backend-module-oidc-provider/src/authenticator.ts @@ -135,7 +135,7 @@ export const oidcAuthenticator = createOAuthAuthenticator({ async start(input, ctx) { const { initializedPrompt, promise } = ctx; - const { helper, strategy } = await promise; + const { helper } = await promise; const options: Record = { scope: input.scope, state: input.state, @@ -146,14 +146,8 @@ export const oidcAuthenticator = createOAuthAuthenticator({ options.prompt = prompt; } - return new Promise((resolve, reject) => { - strategy.error = reject; - - return helper - .start(input, { - ...options, - }) - .then(resolve); + return helper.start(input, { + ...options, }); }, diff --git a/plugins/auth-node/src/oauth/PassportOAuthAuthenticatorHelper.test.ts b/plugins/auth-node/src/oauth/PassportOAuthAuthenticatorHelper.test.ts new file mode 100644 index 0000000000..6694ed8784 --- /dev/null +++ b/plugins/auth-node/src/oauth/PassportOAuthAuthenticatorHelper.test.ts @@ -0,0 +1,48 @@ +/* + * Copyright 2020 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 { Strategy } from 'passport'; +import { PassportOAuthAuthenticatorHelper } from './PassportOAuthAuthenticatorHelper'; + +class FailureStrategy extends Strategy { + public override authenticate(_: any, __: any) { + this.error(new Error('failure')); + } +} + +describe('PassportOAuthAuthenticatorHelper', () => { + describe('start', () => { + it('should gracefully handle errors if unable to start', async () => { + const helper = PassportOAuthAuthenticatorHelper.from( + new FailureStrategy(), + ); + await expect(helper.start({} as any, {})).rejects.toThrow( + 'Authentication failed, failure', + ); + }); + }); + + describe('authenticate', () => { + it('should gracefully handle errors if unable to authenticate', async () => { + const helper = PassportOAuthAuthenticatorHelper.from( + new FailureStrategy(), + ); + await expect(helper.authenticate({} as any, {})).rejects.toThrow( + 'Authentication failed, failure', + ); + }); + }); +}); diff --git a/plugins/auth-node/src/passport/PassportHelpers.ts b/plugins/auth-node/src/passport/PassportHelpers.ts index 9c9198fbf1..ab692c3b67 100644 --- a/plugins/auth-node/src/passport/PassportHelpers.ts +++ b/plugins/auth-node/src/passport/PassportHelpers.ts @@ -105,8 +105,11 @@ export class PassportHelpers { */ status?: number; }> { - return new Promise(resolve => { + return new Promise((resolve, reject) => { const strategy = Object.create(providerStrategy); + strategy.error = (error: Error) => { + reject(new Error(`Authentication failed, ${error.message ?? ''}`)); + }; strategy.redirect = (url: string, status?: number) => { resolve({ url, status: status ?? undefined }); };