From 3b494fa900a510146322ab68e16f5000cb8ef42e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Madureira?= Date: Tue, 11 Feb 2025 12:00:34 +0000 Subject: [PATCH 1/7] Handle errors during an oauth start stage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: João Madureira --- .../PassportOAuthAuthenticatorHelper.test.ts | 44 +++++++++++++++++++ .../auth-node/src/passport/PassportHelpers.ts | 5 ++- 2 files changed, 48 insertions(+), 1 deletion(-) create mode 100644 plugins/auth-node/src/oauth/PassportOAuthAuthenticatorHelper.test.ts 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..46bcf55bb9 --- /dev/null +++ b/plugins/auth-node/src/oauth/PassportOAuthAuthenticatorHelper.test.ts @@ -0,0 +1,44 @@ +/* + * 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 }); }; From 5838e0210105ca0167de679a663fec2505d99258 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Madureira?= Date: Tue, 11 Feb 2025 13:46:23 +0000 Subject: [PATCH 2/7] Fixed some styling issues MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: João Madureira --- .../src/oauth/PassportOAuthAuthenticatorHelper.test.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/plugins/auth-node/src/oauth/PassportOAuthAuthenticatorHelper.test.ts b/plugins/auth-node/src/oauth/PassportOAuthAuthenticatorHelper.test.ts index 46bcf55bb9..6694ed8784 100644 --- a/plugins/auth-node/src/oauth/PassportOAuthAuthenticatorHelper.test.ts +++ b/plugins/auth-node/src/oauth/PassportOAuthAuthenticatorHelper.test.ts @@ -26,7 +26,9 @@ class FailureStrategy extends Strategy { describe('PassportOAuthAuthenticatorHelper', () => { describe('start', () => { it('should gracefully handle errors if unable to start', async () => { - const helper = PassportOAuthAuthenticatorHelper.from(new FailureStrategy()); + const helper = PassportOAuthAuthenticatorHelper.from( + new FailureStrategy(), + ); await expect(helper.start({} as any, {})).rejects.toThrow( 'Authentication failed, failure', ); @@ -35,7 +37,9 @@ describe('PassportOAuthAuthenticatorHelper', () => { describe('authenticate', () => { it('should gracefully handle errors if unable to authenticate', async () => { - const helper = PassportOAuthAuthenticatorHelper.from(new FailureStrategy()); + const helper = PassportOAuthAuthenticatorHelper.from( + new FailureStrategy(), + ); await expect(helper.authenticate({} as any, {})).rejects.toThrow( 'Authentication failed, failure', ); From cb316fc8e04ba2786c73d6fc9f07e1d29ca97c24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Madureira?= Date: Tue, 11 Feb 2025 14:39:32 +0000 Subject: [PATCH 3/7] Added missing changeset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: João Madureira --- .changeset/new-dryers-cough.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/new-dryers-cough.md diff --git a/.changeset/new-dryers-cough.md b/.changeset/new-dryers-cough.md new file mode 100644 index 0000000000..9f62ac1ee5 --- /dev/null +++ b/.changeset/new-dryers-cough.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-auth-node': minor +--- + +Fixed a potential issue when using the Backstage's `PassportOAuthAuthenticatorHelper` to implement a custom OAuth Authenticator. The issue occurs during the start stage of the authorization process when the custom `passport.Strategy` calls the `error()` to propagate an error message. Because the `error` function wasn't being set Backstage would throw a `this.error is not a function` error thus hiding the original cause. From fec0a8956aa68492554b9434787a48c8eb046a63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Madureira?= Date: Fri, 14 Feb 2025 15:33:33 +0000 Subject: [PATCH 4/7] Switch to patch changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: João Madureira --- .changeset/new-dryers-cough.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/new-dryers-cough.md b/.changeset/new-dryers-cough.md index 9f62ac1ee5..f2d6260e08 100644 --- a/.changeset/new-dryers-cough.md +++ b/.changeset/new-dryers-cough.md @@ -1,5 +1,5 @@ --- -'@backstage/plugin-auth-node': minor +'@backstage/plugin-auth-node': patch --- Fixed a potential issue when using the Backstage's `PassportOAuthAuthenticatorHelper` to implement a custom OAuth Authenticator. The issue occurs during the start stage of the authorization process when the custom `passport.Strategy` calls the `error()` to propagate an error message. Because the `error` function wasn't being set Backstage would throw a `this.error is not a function` error thus hiding the original cause. From 497ed901e124b8d76de70d8ed2393ce865d45aff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Madureira?= Date: Mon, 17 Feb 2025 15:13:35 +0000 Subject: [PATCH 5/7] Fixed OIDC failing tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: João Madureira --- plugins/auth-backend-module-oidc-provider/src/authenticator.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/plugins/auth-backend-module-oidc-provider/src/authenticator.ts b/plugins/auth-backend-module-oidc-provider/src/authenticator.ts index efcf1b61dd..cb01639685 100644 --- a/plugins/auth-backend-module-oidc-provider/src/authenticator.ts +++ b/plugins/auth-backend-module-oidc-provider/src/authenticator.ts @@ -153,7 +153,8 @@ export const oidcAuthenticator = createOAuthAuthenticator({ .start(input, { ...options, }) - .then(resolve); + .then(resolve) + .catch(reject); }); }, From edb88e21d2601481a158e8782a54df88c8085ddf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Madureira?= Date: Mon, 17 Feb 2025 16:03:06 +0000 Subject: [PATCH 6/7] Updated changeset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: João Madureira --- .changeset/new-dryers-cough.md | 1 + 1 file changed, 1 insertion(+) diff --git a/.changeset/new-dryers-cough.md b/.changeset/new-dryers-cough.md index f2d6260e08..39a972e71f 100644 --- a/.changeset/new-dryers-cough.md +++ b/.changeset/new-dryers-cough.md @@ -1,4 +1,5 @@ --- +'@backstage/plugin-auth-backend-module-oidc-provider': patch '@backstage/plugin-auth-node': patch --- From 0afc4f21d89ef7c8fc1bee8758773d2caba657c8 Mon Sep 17 00:00:00 2001 From: blam Date: Tue, 18 Feb 2025 15:09:27 +0100 Subject: [PATCH 7/7] chore: cleanup changeset and refactor Signed-off-by: blam --- .changeset/new-dryers-cough-2.md | 5 +++++ .changeset/new-dryers-cough.md | 3 +-- .../src/authenticator.ts | 13 +++---------- 3 files changed, 9 insertions(+), 12 deletions(-) create mode 100644 .changeset/new-dryers-cough-2.md 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 index 39a972e71f..5be2dd761a 100644 --- a/.changeset/new-dryers-cough.md +++ b/.changeset/new-dryers-cough.md @@ -1,6 +1,5 @@ --- -'@backstage/plugin-auth-backend-module-oidc-provider': patch '@backstage/plugin-auth-node': patch --- -Fixed a potential issue when using the Backstage's `PassportOAuthAuthenticatorHelper` to implement a custom OAuth Authenticator. The issue occurs during the start stage of the authorization process when the custom `passport.Strategy` calls the `error()` to propagate an error message. Because the `error` function wasn't being set Backstage would throw a `this.error is not a function` error thus hiding the original cause. +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 cb01639685..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,15 +146,8 @@ export const oidcAuthenticator = createOAuthAuthenticator({ options.prompt = prompt; } - return new Promise((resolve, reject) => { - strategy.error = reject; - - return helper - .start(input, { - ...options, - }) - .then(resolve) - .catch(reject); + return helper.start(input, { + ...options, }); },