From 8542af998a323480b80805b70aead82572c8cc69 Mon Sep 17 00:00:00 2001 From: Stephen Glass Date: Sun, 28 Jul 2024 03:15:00 -0400 Subject: [PATCH 01/28] fix errors with auth redirect flow Signed-off-by: Stephen Glass --- packages/core-app-api/package.json | 57 ++++++++++--------- .../auth/microsoft/MicrosoftAuth.ts | 4 ++ .../implementations/auth/oauth2/OAuth2.ts | 11 +++- .../implementations/auth/saml/SamlAuth.ts | 9 ++- packages/core-app-api/src/lib/index.ts | 1 + .../core-app-api/src/lib/signInAuthError.ts | 29 ++++++++++ .../src/layout/SignInPage/SignInPage.tsx | 19 ++++++- .../src/apis/definitions/auth.ts | 5 ++ .../createCookieAuthErrorMiddleware.ts | 44 ++++++++++++++ plugins/auth-backend/src/service/router.ts | 8 ++- .../src/oauth/createAuthErrorCookie.ts | 56 ++++++++++++++++++ .../src/oauth/createOAuthRouteHandlers.ts | 28 +++++++-- 12 files changed, 232 insertions(+), 39 deletions(-) create mode 100644 packages/core-app-api/src/lib/signInAuthError.ts create mode 100644 plugins/auth-backend/src/service/createCookieAuthErrorMiddleware.ts create mode 100644 plugins/auth-node/src/oauth/createAuthErrorCookie.ts diff --git a/packages/core-app-api/package.json b/packages/core-app-api/package.json index 5f297b16d5..31b3cd22fc 100644 --- a/packages/core-app-api/package.json +++ b/packages/core-app-api/package.json @@ -1,14 +1,30 @@ { "name": "@backstage/core-app-api", - "description": "Core app API used by Backstage apps", "version": "1.14.0", + "description": "Core app API used by Backstage apps", + "backstage": { + "role": "web-library" + }, "publishConfig": { "access": "public" }, + "keywords": [ + "backstage" + ], + "homepage": "https://backstage.io", + "repository": { + "type": "git", + "url": "https://github.com/backstage/backstage", + "directory": "packages/core-app-api" + }, + "license": "Apache-2.0", + "sideEffects": false, "exports": { ".": "./src/index.ts", "./package.json": "./package.json" }, + "main": "src/index.ts", + "types": "src/index.ts", "typesVersions": { "*": { "package.json": [ @@ -16,34 +32,23 @@ ] } }, - "backstage": { - "role": "web-library" - }, - "homepage": "https://backstage.io", - "repository": { - "type": "git", - "url": "https://github.com/backstage/backstage", - "directory": "packages/core-app-api" - }, - "keywords": [ - "backstage" + "files": [ + "dist", + "config.d.ts" ], - "license": "Apache-2.0", - "main": "src/index.ts", - "types": "src/index.ts", - "sideEffects": false, "scripts": { "build": "backstage-cli package build", + "clean": "backstage-cli package clean", "lint": "backstage-cli package lint", - "test": "backstage-cli package test", "prepack": "backstage-cli package prepack", "postpack": "backstage-cli package postpack", - "clean": "backstage-cli package clean", - "start": "backstage-cli package start" + "start": "backstage-cli package start", + "test": "backstage-cli package test" }, "dependencies": { "@backstage/config": "workspace:^", "@backstage/core-plugin-api": "workspace:^", + "@backstage/errors": "workspace:^", "@backstage/types": "workspace:^", "@backstage/version-bridge": "workspace:^", "@types/prop-types": "^15.7.3", @@ -56,11 +61,6 @@ "zen-observable": "^0.10.0", "zod": "^3.22.4" }, - "peerDependencies": { - "react": "^16.13.1 || ^17.0.0 || ^18.0.0", - "react-dom": "^16.13.1 || ^17.0.0 || ^18.0.0", - "react-router-dom": "6.0.0-beta.0 || ^6.3.0" - }, "devDependencies": { "@backstage/cli": "workspace:^", "@backstage/test-utils": "workspace:^", @@ -76,9 +76,10 @@ "react-router-dom-stable": "npm:react-router-dom@^6.3.0", "react-router-stable": "npm:react-router@^6.3.0" }, - "files": [ - "dist", - "config.d.ts" - ], + "peerDependencies": { + "react": "^16.13.1 || ^17.0.0 || ^18.0.0", + "react-dom": "^16.13.1 || ^17.0.0 || ^18.0.0", + "react-router-dom": "6.0.0-beta.0 || ^6.3.0" + }, "configSchema": "config.d.ts" } diff --git a/packages/core-app-api/src/apis/implementations/auth/microsoft/MicrosoftAuth.ts b/packages/core-app-api/src/apis/implementations/auth/microsoft/MicrosoftAuth.ts index c57c65d830..049022702e 100644 --- a/packages/core-app-api/src/apis/implementations/auth/microsoft/MicrosoftAuth.ts +++ b/packages/core-app-api/src/apis/implementations/auth/microsoft/MicrosoftAuth.ts @@ -175,6 +175,10 @@ export default class MicrosoftAuth { return this.microsoftGraph().signOut(); } + getSignInAuthError() { + return this.microsoftGraph().getSignInAuthError(); + } + sessionState$() { return this.microsoftGraph().sessionState$(); } diff --git a/packages/core-app-api/src/apis/implementations/auth/oauth2/OAuth2.ts b/packages/core-app-api/src/apis/implementations/auth/oauth2/OAuth2.ts index dbad031652..1ac5199b8f 100644 --- a/packages/core-app-api/src/apis/implementations/auth/oauth2/OAuth2.ts +++ b/packages/core-app-api/src/apis/implementations/auth/oauth2/OAuth2.ts @@ -31,10 +31,12 @@ import { SessionApi, BackstageIdentityApi, BackstageUserIdentity, + DiscoveryApi, } from '@backstage/core-plugin-api'; import { Observable } from '@backstage/types'; import { OAuth2Session } from './types'; import { OAuthApiCreateOptions } from '../types'; +import { getSignInAuthError } from '../../../../lib'; /** * OAuth2 create options. @@ -152,18 +154,21 @@ export default class OAuth2 }, }); - return new OAuth2({ sessionManager, scopeTransform }); + return new OAuth2({ sessionManager, scopeTransform, discoveryApi }); } private readonly sessionManager: SessionManager; private readonly scopeTransform: (scopes: string[]) => string[]; + private readonly discoveryApi: DiscoveryApi; private constructor(options: { sessionManager: SessionManager; scopeTransform: (scopes: string[]) => string[]; + discoveryApi: DiscoveryApi; }) { this.sessionManager = options.sessionManager; this.scopeTransform = options.scopeTransform; + this.discoveryApi = options.discoveryApi; } async signIn() { @@ -224,4 +229,8 @@ export default class OAuth2 return new Set(scopeTransform(scopeList)); } + + async getSignInAuthError() { + return getSignInAuthError(this.discoveryApi); + } } diff --git a/packages/core-app-api/src/apis/implementations/auth/saml/SamlAuth.ts b/packages/core-app-api/src/apis/implementations/auth/saml/SamlAuth.ts index dad62f1d06..325be93c4c 100644 --- a/packages/core-app-api/src/apis/implementations/auth/saml/SamlAuth.ts +++ b/packages/core-app-api/src/apis/implementations/auth/saml/SamlAuth.ts @@ -22,6 +22,7 @@ import { SessionApi, SessionState, BackstageIdentityResponse, + DiscoveryApi, } from '@backstage/core-plugin-api'; import { Observable } from '@backstage/types'; import { DirectAuthConnector } from '../../../../lib/AuthConnector'; @@ -32,6 +33,7 @@ import { import { SessionManager } from '../../../../lib/AuthSessionManager/types'; import { AuthApiCreateOptions } from '../types'; import { SamlSession, samlSessionSchema } from './types'; +import { getSignInAuthError } from '../../../../lib'; export type SamlAuthResponse = { profile: ProfileInfo; @@ -75,7 +77,7 @@ export default class SamlAuth schema: samlSessionSchema, }); - return new SamlAuth(authSessionStore); + return new SamlAuth(authSessionStore, discoveryApi); } sessionState$(): Observable { @@ -84,6 +86,7 @@ export default class SamlAuth private constructor( private readonly sessionManager: SessionManager, + private readonly discoveryApi: DiscoveryApi, ) {} async signIn() { @@ -104,4 +107,8 @@ export default class SamlAuth const session = await this.sessionManager.getSession(options); return session?.profile; } + + async getSignInAuthError() { + return getSignInAuthError(this.discoveryApi); + } } diff --git a/packages/core-app-api/src/lib/index.ts b/packages/core-app-api/src/lib/index.ts index 1327aab4c2..d5aa2b9e3b 100644 --- a/packages/core-app-api/src/lib/index.ts +++ b/packages/core-app-api/src/lib/index.ts @@ -16,5 +16,6 @@ export * from './subjects'; export * from './loginPopup'; +export * from './signInAuthError'; export * from './AuthConnector'; export * from './AuthSessionManager'; diff --git a/packages/core-app-api/src/lib/signInAuthError.ts b/packages/core-app-api/src/lib/signInAuthError.ts new file mode 100644 index 0000000000..a86a55ee4c --- /dev/null +++ b/packages/core-app-api/src/lib/signInAuthError.ts @@ -0,0 +1,29 @@ +/* + * Copyright 2024 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 { DiscoveryApi } from '@backstage/core-plugin-api'; +import { deserializeError } from '@backstage/errors'; + +export async function getSignInAuthError( + discoveryApi: DiscoveryApi, +): Promise { + const baseUrl = await discoveryApi.getBaseUrl('auth'); + const response = await fetch(`${baseUrl}/.backstage/error`, { + credentials: 'include', + }); + const data = await response.json(); + + return data ? deserializeError(data) : undefined; +} diff --git a/packages/core-components/src/layout/SignInPage/SignInPage.tsx b/packages/core-components/src/layout/SignInPage/SignInPage.tsx index 95d96b701a..207d7e664e 100644 --- a/packages/core-components/src/layout/SignInPage/SignInPage.tsx +++ b/packages/core-components/src/layout/SignInPage/SignInPage.tsx @@ -25,7 +25,7 @@ import Button from '@material-ui/core/Button'; import Grid from '@material-ui/core/Grid'; import Typography from '@material-ui/core/Typography'; import React, { useState } from 'react'; -import { useMountEffect } from '@react-hookz/web'; +import { useAsync, useMountEffect } from '@react-hookz/web'; import { Progress } from '../../components/Progress'; import { Content } from '../Content/Content'; import { ContentHeader } from '../ContentHeader/ContentHeader'; @@ -37,6 +37,7 @@ import { GridItem, useStyles } from './styles'; import { IdentityProviders, SignInProviderConfig } from './types'; import { coreComponentsTranslationRef } from '../../translation'; import { useTranslationRef } from '@backstage/core-plugin-api/alpha'; +import { useSearchParams } from 'react-router-dom'; type MultiSignInPageProps = SignInPageProps & { providers: IdentityProviders; @@ -106,6 +107,8 @@ export const SingleSignInPage = ({ // displayed for a split second when the user is already logged-in. const [showLoginPage, setShowLoginPage] = useState(false); + const [searchParams, _setSearchParams] = useSearchParams(); + type LoginOpts = { checkExisting?: boolean; showPopup?: boolean }; const login = async ({ checkExisting, showPopup }: LoginOpts) => { try { @@ -152,7 +155,19 @@ export const SingleSignInPage = ({ } }; - useMountEffect(() => login({ checkExisting: true })); + const [_state, actions] = useAsync(async () => { + if (searchParams.get('error') !== 'false') { + const errorResponse = await authApi.getSignInAuthError(); + if (errorResponse) { + setError(errorResponse); + } + } + }); + + useMountEffect(() => { + actions.execute(); + login({ checkExisting: true }); + }); return showLoginPage ? ( diff --git a/packages/core-plugin-api/src/apis/definitions/auth.ts b/packages/core-plugin-api/src/apis/definitions/auth.ts index d89544cf68..c29d965890 100644 --- a/packages/core-plugin-api/src/apis/definitions/auth.ts +++ b/packages/core-plugin-api/src/apis/definitions/auth.ts @@ -291,6 +291,11 @@ export type SessionApi = { */ signOut(): Promise; + /** + * Get any caught errors during the auth redirect flow + */ + getSignInAuthError(): Promise; + /** * Observe the current state of the auth session. Emits the current state on subscription. */ diff --git a/plugins/auth-backend/src/service/createCookieAuthErrorMiddleware.ts b/plugins/auth-backend/src/service/createCookieAuthErrorMiddleware.ts new file mode 100644 index 0000000000..9b1d20431d --- /dev/null +++ b/plugins/auth-backend/src/service/createCookieAuthErrorMiddleware.ts @@ -0,0 +1,44 @@ +/* + * Copyright 2024 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 Router from 'express-promise-router'; + +const AUTH_ERROR_COOKIE = 'auth-error'; + +/** + * @public + * Creates a middleware that can be used to get auth errors in redirect flow + */ +export function createCookieAuthErrorMiddleware(authUrl: string) { + const router = Router(); + + router.get('/.backstage/error', async (req, res) => { + const error = req.cookies[AUTH_ERROR_COOKIE]; + if (error) { + const { hostname: domain } = new URL(authUrl); + + res.clearCookie('auth-error', { + path: '/api/auth/.backstage/error', + domain, + }); + res.status(200).json(error); + } else { + res.status(404); + } + }); + + return router; +} diff --git a/plugins/auth-backend/src/service/router.ts b/plugins/auth-backend/src/service/router.ts index 4bb42c3abd..0f6e7d201a 100644 --- a/plugins/auth-backend/src/service/router.ts +++ b/plugins/auth-backend/src/service/router.ts @@ -48,6 +48,7 @@ import { StaticTokenIssuer } from '../identity/StaticTokenIssuer'; import { StaticKeyStore } from '../identity/StaticKeyStore'; import { Config } from '@backstage/config'; import { bindProviderRouters, ProviderFactories } from '../providers/router'; +import { createCookieAuthErrorMiddleware } from './createCookieAuthErrorMiddleware'; /** @public */ export interface RouterOptions { @@ -170,10 +171,15 @@ export async function createRouter( }); // Gives a more helpful error message than a plain 404 - router.use('/:provider/', req => { + router.use('/:provider/', (req, _, next) => { const { provider } = req.params; + if (provider.startsWith('.backstage')) { + return next('route'); + } throw new NotFoundError(`Unknown auth provider '${provider}'`); }); + router.use(createCookieAuthErrorMiddleware(authUrl)); + return router; } diff --git a/plugins/auth-node/src/oauth/createAuthErrorCookie.ts b/plugins/auth-node/src/oauth/createAuthErrorCookie.ts new file mode 100644 index 0000000000..05b1c99cee --- /dev/null +++ b/plugins/auth-node/src/oauth/createAuthErrorCookie.ts @@ -0,0 +1,56 @@ +/* + * Copyright 2024 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 { Response } from 'express'; +import { CookieConfigurer } from '../types'; +import { serializeError } from '@backstage/errors'; + +const ONE_MINUTE_MS = 60 * 1000; + +const AUTH_ERROR_COOKIE = 'auth-error'; + +function configureAuthErrorCookie(redirectUrl: string, appOrigin: string) { + const { hostname: domain, pathname: path, protocol } = new URL(redirectUrl); + const secure = protocol === 'https:'; + + // For situations where the auth-backend is running on a + // different domain than the app, we set the SameSite attribute + // to 'none' to allow third-party access to the cookie, but + // only if it's in a secure context (https). + let sameSite: ReturnType['sameSite'] = 'lax'; + if (new URL(appOrigin).hostname !== domain && secure) { + sameSite = 'none'; + } + + return { domain, path, secure, sameSite }; +} + +export function createAuthErrorCookie( + res: Response, + origin: string, + options: { + error: Error; + redirectUrl: string; + }, +) { + const { error, redirectUrl } = options; + const jsonData = serializeError(error); + + res.cookie(AUTH_ERROR_COOKIE, jsonData, { + maxAge: ONE_MINUTE_MS, + httpOnly: true, + ...configureAuthErrorCookie(redirectUrl, origin), + }); +} diff --git a/plugins/auth-node/src/oauth/createOAuthRouteHandlers.ts b/plugins/auth-node/src/oauth/createOAuthRouteHandlers.ts index 82bfa2f02c..649c094dba 100644 --- a/plugins/auth-node/src/oauth/createOAuthRouteHandlers.ts +++ b/plugins/auth-node/src/oauth/createOAuthRouteHandlers.ts @@ -42,6 +42,7 @@ import { import { OAuthAuthenticator, OAuthAuthenticatorResult } from './types'; import { Config } from '@backstage/config'; import { CookieScopeManager } from './CookieScopeManager'; +import { createAuthErrorCookie } from './createAuthErrorCookie'; /** @public */ export interface OAuthRouteHandlersOptions { @@ -164,9 +165,10 @@ export function createOAuthRouteHandlers( res: express.Response, ): Promise { let origin = defaultAppOrigin; + let state; try { - const state = decodeOAuthState(req.query.state?.toString() ?? ''); + state = decodeOAuthState(req.query.state?.toString() ?? ''); if (state.origin) { try { @@ -248,11 +250,25 @@ export function createOAuthRouteHandlers( const { name, message } = isError(error) ? error : new Error('Encountered invalid error'); // Being a bit safe and not forwarding the bad value - // post error message back to popup if failure - sendWebMessageResponse(res, origin, { - type: 'authorization_response', - error: { name, message }, - }); + + if (state?.flow === 'redirect' && state?.redirectUrl) { + createAuthErrorCookie(res, state?.redirectUrl, { + error: { name, message }, + redirectUrl: `${baseUrl}/.backstage/error`, + }); + + const redirectUrl = new URL(state.redirectUrl); + redirectUrl.searchParams.set('error', 'true'); + + // set the error in a cookie and redirect user back to sign in where the error can be rendered + res.redirect(redirectUrl.toString()); + } else { + // post error message back to popup if failure + sendWebMessageResponse(res, origin, { + type: 'authorization_response', + error: { name, message }, + }); + } } }, From 5c11d3f97039785cc52bb8c68b54483b8d4aba85 Mon Sep 17 00:00:00 2001 From: Stephen Glass Date: Sun, 28 Jul 2024 03:21:05 -0400 Subject: [PATCH 02/28] fix formatting Signed-off-by: Stephen Glass --- packages/core-app-api/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core-app-api/package.json b/packages/core-app-api/package.json index 31b3cd22fc..547dd47654 100644 --- a/packages/core-app-api/package.json +++ b/packages/core-app-api/package.json @@ -1,6 +1,6 @@ { "name": "@backstage/core-app-api", - "version": "1.14.0", + "version": "1.14.1-next.0", "description": "Core app API used by Backstage apps", "backstage": { "role": "web-library" From 0625085c2fe946cf7091f7633b15c29fde435ee6 Mon Sep 17 00:00:00 2001 From: Stephen Glass Date: Sun, 28 Jul 2024 03:29:31 -0400 Subject: [PATCH 03/28] update clear cookie to use var name Signed-off-by: Stephen Glass --- .../auth-backend/src/service/createCookieAuthErrorMiddleware.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/auth-backend/src/service/createCookieAuthErrorMiddleware.ts b/plugins/auth-backend/src/service/createCookieAuthErrorMiddleware.ts index 9b1d20431d..cf7645b081 100644 --- a/plugins/auth-backend/src/service/createCookieAuthErrorMiddleware.ts +++ b/plugins/auth-backend/src/service/createCookieAuthErrorMiddleware.ts @@ -30,7 +30,7 @@ export function createCookieAuthErrorMiddleware(authUrl: string) { if (error) { const { hostname: domain } = new URL(authUrl); - res.clearCookie('auth-error', { + res.clearCookie(AUTH_ERROR_COOKIE, { path: '/api/auth/.backstage/error', domain, }); From 31edacd9b153209670a5e97a998c434e19d07a41 Mon Sep 17 00:00:00 2001 From: Stephen Glass Date: Sun, 28 Jul 2024 03:29:58 -0400 Subject: [PATCH 04/28] yarn lock Signed-off-by: Stephen Glass --- yarn.lock | 1 + 1 file changed, 1 insertion(+) diff --git a/yarn.lock b/yarn.lock index c6a1d2139a..c3d1f83de5 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4123,6 +4123,7 @@ __metadata: "@backstage/cli": "workspace:^" "@backstage/config": "workspace:^" "@backstage/core-plugin-api": "workspace:^" + "@backstage/errors": "workspace:^" "@backstage/test-utils": "workspace:^" "@backstage/types": "workspace:^" "@backstage/version-bridge": "workspace:^" From e1caa7bc85b886f6e585ed9b66747ff199d751b6 Mon Sep 17 00:00:00 2001 From: Stephen Glass Date: Sun, 28 Jul 2024 14:12:12 -0400 Subject: [PATCH 05/28] create auth error api ref Signed-off-by: Stephen Glass --- packages/app-defaults/src/defaults/apis.ts | 9 ++++ .../AuthErrorApi/SignInAuthErrorApi.ts | 41 +++++++++++++++++ .../implementations/AuthErrorApi/index.ts} | 14 +----- .../auth/microsoft/MicrosoftAuth.ts | 4 -- .../implementations/auth/oauth2/OAuth2.ts | 11 +---- .../implementations/auth/saml/SamlAuth.ts | 9 +--- .../src/apis/implementations/index.ts | 1 + packages/core-app-api/src/lib/index.ts | 1 - .../src/layout/SignInPage/SignInPage.tsx | 8 ++-- .../src/layout/SignInPage/providers.tsx | 17 +++++++ .../src/apis/definitions/AuthErrorApi.ts | 45 +++++++++++++++++++ .../src/apis/definitions/auth.ts | 5 --- .../src/apis/definitions/index.ts | 1 + 13 files changed, 122 insertions(+), 44 deletions(-) create mode 100644 packages/core-app-api/src/apis/implementations/AuthErrorApi/SignInAuthErrorApi.ts rename packages/core-app-api/src/{lib/signInAuthError.ts => apis/implementations/AuthErrorApi/index.ts} (56%) create mode 100644 packages/core-plugin-api/src/apis/definitions/AuthErrorApi.ts diff --git a/packages/app-defaults/src/defaults/apis.ts b/packages/app-defaults/src/defaults/apis.ts index 4e9e1a492c..84902b9f52 100644 --- a/packages/app-defaults/src/defaults/apis.ts +++ b/packages/app-defaults/src/defaults/apis.ts @@ -35,6 +35,7 @@ import { createFetchApi, FetchMiddlewares, VMwareCloudAuth, + SignInAuthErrorApi, } from '@backstage/core-app-api'; import { @@ -58,6 +59,7 @@ import { bitbucketServerAuthApiRef, atlassianAuthApiRef, vmwareCloudAuthApiRef, + authErrorApiRef, } from '@backstage/core-plugin-api'; import { permissionApiRef, @@ -287,4 +289,11 @@ export const apis = [ factory: ({ config, discovery, identity }) => IdentityPermissionApi.create({ config, discovery, identity }), }), + createApiFactory({ + api: authErrorApiRef, + deps: { + discovery: discoveryApiRef, + }, + factory: ({ discovery }) => SignInAuthErrorApi.create({ discovery }), + }), ]; diff --git a/packages/core-app-api/src/apis/implementations/AuthErrorApi/SignInAuthErrorApi.ts b/packages/core-app-api/src/apis/implementations/AuthErrorApi/SignInAuthErrorApi.ts new file mode 100644 index 0000000000..0197f253ab --- /dev/null +++ b/packages/core-app-api/src/apis/implementations/AuthErrorApi/SignInAuthErrorApi.ts @@ -0,0 +1,41 @@ +/* + * 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 { DiscoveryApi, AuthErrorApi } from '@backstage/core-plugin-api'; +import { deserializeError } from '@backstage/errors'; + +/** + * The default implementation of the AuthErrorApi, which simply calls auth error endpoint. + * @public + */ +export class SignInAuthErrorApi implements AuthErrorApi { + private constructor(private readonly discoveryApi: DiscoveryApi) {} + + static create(options: { discovery: DiscoveryApi }) { + const { discovery } = options; + return new SignInAuthErrorApi(discovery); + } + + async getSignInAuthError(): Promise { + const baseUrl = await this.discoveryApi.getBaseUrl('auth'); + const response = await fetch(`${baseUrl}/.backstage/error`, { + credentials: 'include', + }); + const data = await response.json(); + + return data ? deserializeError(data) : undefined; + } +} diff --git a/packages/core-app-api/src/lib/signInAuthError.ts b/packages/core-app-api/src/apis/implementations/AuthErrorApi/index.ts similarity index 56% rename from packages/core-app-api/src/lib/signInAuthError.ts rename to packages/core-app-api/src/apis/implementations/AuthErrorApi/index.ts index a86a55ee4c..6f3a3ae269 100644 --- a/packages/core-app-api/src/lib/signInAuthError.ts +++ b/packages/core-app-api/src/apis/implementations/AuthErrorApi/index.ts @@ -13,17 +13,5 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import { DiscoveryApi } from '@backstage/core-plugin-api'; -import { deserializeError } from '@backstage/errors'; -export async function getSignInAuthError( - discoveryApi: DiscoveryApi, -): Promise { - const baseUrl = await discoveryApi.getBaseUrl('auth'); - const response = await fetch(`${baseUrl}/.backstage/error`, { - credentials: 'include', - }); - const data = await response.json(); - - return data ? deserializeError(data) : undefined; -} +export { SignInAuthErrorApi } from './SignInAuthErrorApi'; diff --git a/packages/core-app-api/src/apis/implementations/auth/microsoft/MicrosoftAuth.ts b/packages/core-app-api/src/apis/implementations/auth/microsoft/MicrosoftAuth.ts index 049022702e..c57c65d830 100644 --- a/packages/core-app-api/src/apis/implementations/auth/microsoft/MicrosoftAuth.ts +++ b/packages/core-app-api/src/apis/implementations/auth/microsoft/MicrosoftAuth.ts @@ -175,10 +175,6 @@ export default class MicrosoftAuth { return this.microsoftGraph().signOut(); } - getSignInAuthError() { - return this.microsoftGraph().getSignInAuthError(); - } - sessionState$() { return this.microsoftGraph().sessionState$(); } diff --git a/packages/core-app-api/src/apis/implementations/auth/oauth2/OAuth2.ts b/packages/core-app-api/src/apis/implementations/auth/oauth2/OAuth2.ts index 1ac5199b8f..dbad031652 100644 --- a/packages/core-app-api/src/apis/implementations/auth/oauth2/OAuth2.ts +++ b/packages/core-app-api/src/apis/implementations/auth/oauth2/OAuth2.ts @@ -31,12 +31,10 @@ import { SessionApi, BackstageIdentityApi, BackstageUserIdentity, - DiscoveryApi, } from '@backstage/core-plugin-api'; import { Observable } from '@backstage/types'; import { OAuth2Session } from './types'; import { OAuthApiCreateOptions } from '../types'; -import { getSignInAuthError } from '../../../../lib'; /** * OAuth2 create options. @@ -154,21 +152,18 @@ export default class OAuth2 }, }); - return new OAuth2({ sessionManager, scopeTransform, discoveryApi }); + return new OAuth2({ sessionManager, scopeTransform }); } private readonly sessionManager: SessionManager; private readonly scopeTransform: (scopes: string[]) => string[]; - private readonly discoveryApi: DiscoveryApi; private constructor(options: { sessionManager: SessionManager; scopeTransform: (scopes: string[]) => string[]; - discoveryApi: DiscoveryApi; }) { this.sessionManager = options.sessionManager; this.scopeTransform = options.scopeTransform; - this.discoveryApi = options.discoveryApi; } async signIn() { @@ -229,8 +224,4 @@ export default class OAuth2 return new Set(scopeTransform(scopeList)); } - - async getSignInAuthError() { - return getSignInAuthError(this.discoveryApi); - } } diff --git a/packages/core-app-api/src/apis/implementations/auth/saml/SamlAuth.ts b/packages/core-app-api/src/apis/implementations/auth/saml/SamlAuth.ts index 325be93c4c..dad62f1d06 100644 --- a/packages/core-app-api/src/apis/implementations/auth/saml/SamlAuth.ts +++ b/packages/core-app-api/src/apis/implementations/auth/saml/SamlAuth.ts @@ -22,7 +22,6 @@ import { SessionApi, SessionState, BackstageIdentityResponse, - DiscoveryApi, } from '@backstage/core-plugin-api'; import { Observable } from '@backstage/types'; import { DirectAuthConnector } from '../../../../lib/AuthConnector'; @@ -33,7 +32,6 @@ import { import { SessionManager } from '../../../../lib/AuthSessionManager/types'; import { AuthApiCreateOptions } from '../types'; import { SamlSession, samlSessionSchema } from './types'; -import { getSignInAuthError } from '../../../../lib'; export type SamlAuthResponse = { profile: ProfileInfo; @@ -77,7 +75,7 @@ export default class SamlAuth schema: samlSessionSchema, }); - return new SamlAuth(authSessionStore, discoveryApi); + return new SamlAuth(authSessionStore); } sessionState$(): Observable { @@ -86,7 +84,6 @@ export default class SamlAuth private constructor( private readonly sessionManager: SessionManager, - private readonly discoveryApi: DiscoveryApi, ) {} async signIn() { @@ -107,8 +104,4 @@ export default class SamlAuth const session = await this.sessionManager.getSession(options); return session?.profile; } - - async getSignInAuthError() { - return getSignInAuthError(this.discoveryApi); - } } diff --git a/packages/core-app-api/src/apis/implementations/index.ts b/packages/core-app-api/src/apis/implementations/index.ts index 1c79d3d164..7643dfcd82 100644 --- a/packages/core-app-api/src/apis/implementations/index.ts +++ b/packages/core-app-api/src/apis/implementations/index.ts @@ -30,3 +30,4 @@ export * from './FeatureFlagsApi'; export * from './FetchApi'; export * from './OAuthRequestApi'; export * from './StorageApi'; +export * from './AuthErrorApi'; diff --git a/packages/core-app-api/src/lib/index.ts b/packages/core-app-api/src/lib/index.ts index d5aa2b9e3b..1327aab4c2 100644 --- a/packages/core-app-api/src/lib/index.ts +++ b/packages/core-app-api/src/lib/index.ts @@ -16,6 +16,5 @@ export * from './subjects'; export * from './loginPopup'; -export * from './signInAuthError'; export * from './AuthConnector'; export * from './AuthSessionManager'; diff --git a/packages/core-components/src/layout/SignInPage/SignInPage.tsx b/packages/core-components/src/layout/SignInPage/SignInPage.tsx index 207d7e664e..f0af0c5d35 100644 --- a/packages/core-components/src/layout/SignInPage/SignInPage.tsx +++ b/packages/core-components/src/layout/SignInPage/SignInPage.tsx @@ -19,6 +19,7 @@ import { configApiRef, SignInPageProps, useApi, + authErrorApiRef, } from '@backstage/core-plugin-api'; import { UserIdentity } from './UserIdentity'; import Button from '@material-ui/core/Button'; @@ -98,6 +99,7 @@ export const SingleSignInPage = ({ const classes = useStyles(); const authApi = useApi(provider.apiRef); const configApi = useApi(configApiRef); + const authErrorApi = useApi(authErrorApiRef); const { t } = useTranslationRef(coreComponentsTranslationRef); const [error, setError] = useState(); @@ -155,9 +157,9 @@ export const SingleSignInPage = ({ } }; - const [_state, actions] = useAsync(async () => { + const [_, { execute }] = useAsync(async () => { if (searchParams.get('error') !== 'false') { - const errorResponse = await authApi.getSignInAuthError(); + const errorResponse = await authErrorApi.getSignInAuthError(); if (errorResponse) { setError(errorResponse); } @@ -165,7 +167,7 @@ export const SingleSignInPage = ({ }); useMountEffect(() => { - actions.execute(); + execute(); login({ checkExisting: true }); }); diff --git a/packages/core-components/src/layout/SignInPage/providers.tsx b/packages/core-components/src/layout/SignInPage/providers.tsx index e456a6948b..9b73cfc561 100644 --- a/packages/core-components/src/layout/SignInPage/providers.tsx +++ b/packages/core-components/src/layout/SignInPage/providers.tsx @@ -21,6 +21,7 @@ import { useApiHolder, errorApiRef, IdentityApi, + authErrorApiRef, } from '@backstage/core-plugin-api'; import { IdentityProviders, @@ -31,6 +32,8 @@ import { commonProvider } from './commonProvider'; import { guestProvider } from './guestProvider'; import { customProvider } from './customProvider'; import { IdentityApiSignOutProxy } from './IdentityApiSignOutProxy'; +import { useSearchParams } from 'react-router-dom'; +import { useMountEffect, useAsync } from '@react-hookz/web'; const PROVIDER_STORAGE_KEY = '@backstage/core:SignInPage:provider'; @@ -86,8 +89,22 @@ export const useSignInProviders = ( ) => { const errorApi = useApi(errorApiRef); const apiHolder = useApiHolder(); + const authErrorApi = useApi(authErrorApiRef); const [loading, setLoading] = useState(true); + const [searchParams, _setSearchParams] = useSearchParams(); + + const [_, { execute }] = useAsync(async () => { + if (searchParams.get('error') !== 'false') { + const errorResponse = await authErrorApi.getSignInAuthError(); + if (errorResponse) { + errorApi.post(errorResponse); + } + } + }); + + useMountEffect(execute); + // This decorates the result with sign out logic from this hook const handleWrappedResult = useCallback( (identityApi: IdentityApi) => { diff --git a/packages/core-plugin-api/src/apis/definitions/AuthErrorApi.ts b/packages/core-plugin-api/src/apis/definitions/AuthErrorApi.ts new file mode 100644 index 0000000000..57c6b1088f --- /dev/null +++ b/packages/core-plugin-api/src/apis/definitions/AuthErrorApi.ts @@ -0,0 +1,45 @@ +/* + * Copyright 2024 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 { ApiRef, createApiRef } from '../system'; + +/** + * A wrapper for retrieving any errors caught in authentication redirect flow + * + * @public + */ +export type AuthErrorApi = { + /** + * Get any caught errors during the auth redirect flow + */ + getSignInAuthError(): Promise; +}; + +/** + * The {@link ApiRef} of {@link AuthErrorApi}. + * + * @remarks + * + * This is a wrapper that uses fetch to retrieve any caught errors + * in the authentication redirect flow process. This API calls a + * Backstage endpoint to read any error stored in authentication error + * cookie and returns it to the user. + * + * @public + */ +export const authErrorApiRef: ApiRef = createApiRef({ + id: 'core.authError', +}); diff --git a/packages/core-plugin-api/src/apis/definitions/auth.ts b/packages/core-plugin-api/src/apis/definitions/auth.ts index c29d965890..d89544cf68 100644 --- a/packages/core-plugin-api/src/apis/definitions/auth.ts +++ b/packages/core-plugin-api/src/apis/definitions/auth.ts @@ -291,11 +291,6 @@ export type SessionApi = { */ signOut(): Promise; - /** - * Get any caught errors during the auth redirect flow - */ - getSignInAuthError(): Promise; - /** * Observe the current state of the auth session. Emits the current state on subscription. */ diff --git a/packages/core-plugin-api/src/apis/definitions/index.ts b/packages/core-plugin-api/src/apis/definitions/index.ts index 67d442587d..406dc1980e 100644 --- a/packages/core-plugin-api/src/apis/definitions/index.ts +++ b/packages/core-plugin-api/src/apis/definitions/index.ts @@ -33,3 +33,4 @@ export * from './FetchApi'; export * from './IdentityApi'; export * from './OAuthRequestApi'; export * from './StorageApi'; +export * from './AuthErrorApi'; From 40064678e0e5b4f24ff9558ba3efd700c0e9cd07 Mon Sep 17 00:00:00 2001 From: Stephen Glass Date: Sun, 28 Jul 2024 14:15:08 -0400 Subject: [PATCH 06/28] rename async execute Signed-off-by: Stephen Glass --- packages/core-components/src/layout/SignInPage/SignInPage.tsx | 4 ++-- packages/core-components/src/layout/SignInPage/providers.tsx | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/core-components/src/layout/SignInPage/SignInPage.tsx b/packages/core-components/src/layout/SignInPage/SignInPage.tsx index f0af0c5d35..a0d5ec232e 100644 --- a/packages/core-components/src/layout/SignInPage/SignInPage.tsx +++ b/packages/core-components/src/layout/SignInPage/SignInPage.tsx @@ -157,7 +157,7 @@ export const SingleSignInPage = ({ } }; - const [_, { execute }] = useAsync(async () => { + const [_, { execute: checkAuthErrors }] = useAsync(async () => { if (searchParams.get('error') !== 'false') { const errorResponse = await authErrorApi.getSignInAuthError(); if (errorResponse) { @@ -167,7 +167,7 @@ export const SingleSignInPage = ({ }); useMountEffect(() => { - execute(); + checkAuthErrors(); login({ checkExisting: true }); }); diff --git a/packages/core-components/src/layout/SignInPage/providers.tsx b/packages/core-components/src/layout/SignInPage/providers.tsx index 9b73cfc561..4b6c251e7b 100644 --- a/packages/core-components/src/layout/SignInPage/providers.tsx +++ b/packages/core-components/src/layout/SignInPage/providers.tsx @@ -94,7 +94,7 @@ export const useSignInProviders = ( const [searchParams, _setSearchParams] = useSearchParams(); - const [_, { execute }] = useAsync(async () => { + const [_, { execute: checkAuthErrors }] = useAsync(async () => { if (searchParams.get('error') !== 'false') { const errorResponse = await authErrorApi.getSignInAuthError(); if (errorResponse) { @@ -103,7 +103,7 @@ export const useSignInProviders = ( } }); - useMountEffect(execute); + useMountEffect(checkAuthErrors); // This decorates the result with sign out logic from this hook const handleWrappedResult = useCallback( From 5d8649d7757c665ca76b9fc687d3b0e28c98f4da Mon Sep 17 00:00:00 2001 From: Stephen Glass Date: Sun, 28 Jul 2024 23:28:13 -0400 Subject: [PATCH 07/28] update param name Signed-off-by: Stephen Glass --- .../implementations/AuthErrorApi/SignInAuthErrorApi.ts | 3 +++ plugins/auth-node/src/oauth/createAuthErrorCookie.ts | 10 +++++----- .../auth-node/src/oauth/createOAuthRouteHandlers.ts | 2 +- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/packages/core-app-api/src/apis/implementations/AuthErrorApi/SignInAuthErrorApi.ts b/packages/core-app-api/src/apis/implementations/AuthErrorApi/SignInAuthErrorApi.ts index 0197f253ab..2f2ba9cb09 100644 --- a/packages/core-app-api/src/apis/implementations/AuthErrorApi/SignInAuthErrorApi.ts +++ b/packages/core-app-api/src/apis/implementations/AuthErrorApi/SignInAuthErrorApi.ts @@ -31,6 +31,9 @@ export class SignInAuthErrorApi implements AuthErrorApi { async getSignInAuthError(): Promise { const baseUrl = await this.discoveryApi.getBaseUrl('auth'); + + // use native fetch instead of depending on fetchApi because + // we are not signed in and are calling an unauthenticated endpoint const response = await fetch(`${baseUrl}/.backstage/error`, { credentials: 'include', }); diff --git a/plugins/auth-node/src/oauth/createAuthErrorCookie.ts b/plugins/auth-node/src/oauth/createAuthErrorCookie.ts index 05b1c99cee..61914e7c05 100644 --- a/plugins/auth-node/src/oauth/createAuthErrorCookie.ts +++ b/plugins/auth-node/src/oauth/createAuthErrorCookie.ts @@ -21,8 +21,8 @@ const ONE_MINUTE_MS = 60 * 1000; const AUTH_ERROR_COOKIE = 'auth-error'; -function configureAuthErrorCookie(redirectUrl: string, appOrigin: string) { - const { hostname: domain, pathname: path, protocol } = new URL(redirectUrl); +function configureAuthErrorCookie(apiUrl: string, appOrigin: string) { + const { hostname: domain, pathname: path, protocol } = new URL(apiUrl); const secure = protocol === 'https:'; // For situations where the auth-backend is running on a @@ -42,15 +42,15 @@ export function createAuthErrorCookie( origin: string, options: { error: Error; - redirectUrl: string; + apiUrl: string; }, ) { - const { error, redirectUrl } = options; + const { error, apiUrl } = options; const jsonData = serializeError(error); res.cookie(AUTH_ERROR_COOKIE, jsonData, { maxAge: ONE_MINUTE_MS, httpOnly: true, - ...configureAuthErrorCookie(redirectUrl, origin), + ...configureAuthErrorCookie(apiUrl, origin), }); } diff --git a/plugins/auth-node/src/oauth/createOAuthRouteHandlers.ts b/plugins/auth-node/src/oauth/createOAuthRouteHandlers.ts index 649c094dba..847dc4844f 100644 --- a/plugins/auth-node/src/oauth/createOAuthRouteHandlers.ts +++ b/plugins/auth-node/src/oauth/createOAuthRouteHandlers.ts @@ -254,7 +254,7 @@ export function createOAuthRouteHandlers( if (state?.flow === 'redirect' && state?.redirectUrl) { createAuthErrorCookie(res, state?.redirectUrl, { error: { name, message }, - redirectUrl: `${baseUrl}/.backstage/error`, + apiUrl: `${baseUrl}/.backstage/error`, }); const redirectUrl = new URL(state.redirectUrl); From 41b0d71313f5804b890f39d5f258325ab838a718 Mon Sep 17 00:00:00 2001 From: Stephen Glass Date: Sun, 28 Jul 2024 23:54:30 -0400 Subject: [PATCH 08/28] fix warning on cookie clear Signed-off-by: Stephen Glass --- .../src/apis/definitions/AuthErrorApi.ts | 2 +- .../src/service/createCookieAuthErrorMiddleware.ts | 12 ++++++++++-- plugins/auth-backend/src/service/router.ts | 2 +- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/packages/core-plugin-api/src/apis/definitions/AuthErrorApi.ts b/packages/core-plugin-api/src/apis/definitions/AuthErrorApi.ts index 57c6b1088f..558c1202a8 100644 --- a/packages/core-plugin-api/src/apis/definitions/AuthErrorApi.ts +++ b/packages/core-plugin-api/src/apis/definitions/AuthErrorApi.ts @@ -41,5 +41,5 @@ export type AuthErrorApi = { * @public */ export const authErrorApiRef: ApiRef = createApiRef({ - id: 'core.authError', + id: 'core.auth-error', }); diff --git a/plugins/auth-backend/src/service/createCookieAuthErrorMiddleware.ts b/plugins/auth-backend/src/service/createCookieAuthErrorMiddleware.ts index cf7645b081..325126d831 100644 --- a/plugins/auth-backend/src/service/createCookieAuthErrorMiddleware.ts +++ b/plugins/auth-backend/src/service/createCookieAuthErrorMiddleware.ts @@ -22,17 +22,25 @@ const AUTH_ERROR_COOKIE = 'auth-error'; * @public * Creates a middleware that can be used to get auth errors in redirect flow */ -export function createCookieAuthErrorMiddleware(authUrl: string) { +export function createCookieAuthErrorMiddleware( + appUrl: string, + authUrl: string, +) { const router = Router(); router.get('/.backstage/error', async (req, res) => { const error = req.cookies[AUTH_ERROR_COOKIE]; if (error) { - const { hostname: domain } = new URL(authUrl); + const { hostname: domain, protocol } = new URL(authUrl); + const secure = protocol === 'https:'; + const sameSite = + new URL(appUrl).hostname !== domain && secure ? 'none' : 'lax'; res.clearCookie(AUTH_ERROR_COOKIE, { path: '/api/auth/.backstage/error', domain, + sameSite, + secure, }); res.status(200).json(error); } else { diff --git a/plugins/auth-backend/src/service/router.ts b/plugins/auth-backend/src/service/router.ts index 0f6e7d201a..b1952755ca 100644 --- a/plugins/auth-backend/src/service/router.ts +++ b/plugins/auth-backend/src/service/router.ts @@ -179,7 +179,7 @@ export async function createRouter( throw new NotFoundError(`Unknown auth provider '${provider}'`); }); - router.use(createCookieAuthErrorMiddleware(authUrl)); + router.use(createCookieAuthErrorMiddleware(appUrl, authUrl)); return router; } From 672a4b4876db0eb3ba8756503b1252e42f49cf01 Mon Sep 17 00:00:00 2001 From: Stephen Glass Date: Mon, 29 Jul 2024 00:06:51 -0400 Subject: [PATCH 09/28] update middleware order Signed-off-by: Stephen Glass --- .../implementations/AuthErrorApi/SignInAuthErrorApi.ts | 2 +- plugins/auth-backend/src/service/router.ts | 9 +++------ 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/packages/core-app-api/src/apis/implementations/AuthErrorApi/SignInAuthErrorApi.ts b/packages/core-app-api/src/apis/implementations/AuthErrorApi/SignInAuthErrorApi.ts index 2f2ba9cb09..77b3c53f14 100644 --- a/packages/core-app-api/src/apis/implementations/AuthErrorApi/SignInAuthErrorApi.ts +++ b/packages/core-app-api/src/apis/implementations/AuthErrorApi/SignInAuthErrorApi.ts @@ -1,5 +1,5 @@ /* - * Copyright 2021 The Backstage Authors + * Copyright 2024 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. diff --git a/plugins/auth-backend/src/service/router.ts b/plugins/auth-backend/src/service/router.ts index b1952755ca..57f01cfba1 100644 --- a/plugins/auth-backend/src/service/router.ts +++ b/plugins/auth-backend/src/service/router.ts @@ -170,16 +170,13 @@ export async function createRouter( userInfoDatabaseHandler, }); + router.use(createCookieAuthErrorMiddleware(appUrl, authUrl)); + // Gives a more helpful error message than a plain 404 - router.use('/:provider/', (req, _, next) => { + router.use('/:provider/', req => { const { provider } = req.params; - if (provider.startsWith('.backstage')) { - return next('route'); - } throw new NotFoundError(`Unknown auth provider '${provider}'`); }); - router.use(createCookieAuthErrorMiddleware(appUrl, authUrl)); - return router; } From 17c9a1a330b509817d749f1944d91d58483a6efe Mon Sep 17 00:00:00 2001 From: Stephen Glass Date: Mon, 29 Jul 2024 00:45:41 -0400 Subject: [PATCH 10/28] add test Signed-off-by: Stephen Glass --- .../src/oauth/createAuthErrorCookie.ts | 3 +-- .../oauth/createOAuthRouteHandlers.test.ts | 24 +++++++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/plugins/auth-node/src/oauth/createAuthErrorCookie.ts b/plugins/auth-node/src/oauth/createAuthErrorCookie.ts index 61914e7c05..46ef4c4a1c 100644 --- a/plugins/auth-node/src/oauth/createAuthErrorCookie.ts +++ b/plugins/auth-node/src/oauth/createAuthErrorCookie.ts @@ -14,7 +14,6 @@ * limitations under the License. */ import { Response } from 'express'; -import { CookieConfigurer } from '../types'; import { serializeError } from '@backstage/errors'; const ONE_MINUTE_MS = 60 * 1000; @@ -29,7 +28,7 @@ function configureAuthErrorCookie(apiUrl: string, appOrigin: string) { // different domain than the app, we set the SameSite attribute // to 'none' to allow third-party access to the cookie, but // only if it's in a secure context (https). - let sameSite: ReturnType['sameSite'] = 'lax'; + let sameSite: 'lax' | 'none' = 'lax'; if (new URL(appOrigin).hostname !== domain && secure) { sameSite = 'none'; } diff --git a/plugins/auth-node/src/oauth/createOAuthRouteHandlers.test.ts b/plugins/auth-node/src/oauth/createOAuthRouteHandlers.test.ts index 7da5519655..2f3eb360cc 100644 --- a/plugins/auth-node/src/oauth/createOAuthRouteHandlers.test.ts +++ b/plugins/auth-node/src/oauth/createOAuthRouteHandlers.test.ts @@ -742,5 +742,29 @@ describe('createOAuthRouteHandlers', () => { }, }); }); + + it('should set cookie on caught error redirect', async () => { + const app = wrapInApp(createOAuthRouteHandlers(baseConfig)); + const res = await request(app) + .get('/my-provider/handler/frame') + .query({ + state: encodeOAuthState({ + env: 'development', + nonce: '123', + flow: 'redirect', + redirectUrl: 'http://localhost:3000', + }), + }); + + // redirects on error with auth error cookie + expect(res.status).toBe(302); + const setCookieHeader = res.header['set-cookie']; + expect(setCookieHeader).toBeDefined(); + + const authErrorCookie = setCookieHeader.find((cookie: string) => + cookie.startsWith('auth-error='), + ); + expect(authErrorCookie).toBeDefined(); + }); }); }); From 155b9018980f71a3d88cd83bde430000656f6fd2 Mon Sep 17 00:00:00 2001 From: Stephen Glass Date: Mon, 29 Jul 2024 00:47:32 -0400 Subject: [PATCH 11/28] update test name Signed-off-by: Stephen Glass --- plugins/auth-node/src/oauth/createOAuthRouteHandlers.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/auth-node/src/oauth/createOAuthRouteHandlers.test.ts b/plugins/auth-node/src/oauth/createOAuthRouteHandlers.test.ts index 2f3eb360cc..b54de50881 100644 --- a/plugins/auth-node/src/oauth/createOAuthRouteHandlers.test.ts +++ b/plugins/auth-node/src/oauth/createOAuthRouteHandlers.test.ts @@ -743,7 +743,7 @@ describe('createOAuthRouteHandlers', () => { }); }); - it('should set cookie on caught error redirect', async () => { + it('should set cookie and redirect on caught error', async () => { const app = wrapInApp(createOAuthRouteHandlers(baseConfig)); const res = await request(app) .get('/my-provider/handler/frame') From cc2642d69633e20561280bd048fe71b0a5cd1840 Mon Sep 17 00:00:00 2001 From: Stephen Glass Date: Mon, 29 Jul 2024 14:23:13 -0400 Subject: [PATCH 12/28] add tests Signed-off-by: Stephen Glass --- .../AuthErrorApi/SignInAuthErrorApi.test.ts | 94 +++++++++++++++++++ .../createCookieAuthErrorMiddleware.test.ts | 67 +++++++++++++ .../createCookieAuthErrorMiddleware.ts | 2 +- 3 files changed, 162 insertions(+), 1 deletion(-) create mode 100644 packages/core-app-api/src/apis/implementations/AuthErrorApi/SignInAuthErrorApi.test.ts create mode 100644 plugins/auth-backend/src/service/createCookieAuthErrorMiddleware.test.ts diff --git a/packages/core-app-api/src/apis/implementations/AuthErrorApi/SignInAuthErrorApi.test.ts b/packages/core-app-api/src/apis/implementations/AuthErrorApi/SignInAuthErrorApi.test.ts new file mode 100644 index 0000000000..c4d6fe83de --- /dev/null +++ b/packages/core-app-api/src/apis/implementations/AuthErrorApi/SignInAuthErrorApi.test.ts @@ -0,0 +1,94 @@ +/* + * Copyright 2024 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 { DiscoveryApi } from '@backstage/core-plugin-api'; +import { serializeError } from '@backstage/errors'; +import { SignInAuthErrorApi } from './SignInAuthErrorApi'; + +describe('SignInAuthErrorApi', () => { + const mockDiscoveryApi = { + getBaseUrl: jest.fn(), + } as unknown as jest.Mocked; + + beforeEach(() => { + jest.resetAllMocks(); + }); + + it('should create an instance of SignInAuthErrorApi', async () => { + const api = SignInAuthErrorApi.create({ + discovery: mockDiscoveryApi, + }); + + mockDiscoveryApi.getBaseUrl.mockResolvedValue( + 'http://localhost:7007/api/auth', + ); + + expect(api).toBeInstanceOf(SignInAuthErrorApi); + }); + + it('should return an error when the cookie returns an error object', async () => { + const errorObject = { + name: 'TestError', + message: 'This is a test error', + }; + const serializedError = serializeError(errorObject); + mockDiscoveryApi.getBaseUrl.mockResolvedValue( + 'http://localhost:7000/api/auth', + ); + + global.fetch = jest.fn().mockResolvedValue({ + ok: true, + json: jest.fn().mockResolvedValue(serializedError), + } as unknown as Response); + + const api = SignInAuthErrorApi.create({ discovery: mockDiscoveryApi }); + const error = await api.getSignInAuthError(); + + expect(mockDiscoveryApi.getBaseUrl).toHaveBeenCalledWith('auth'); + expect(fetch).toHaveBeenCalledWith( + 'http://localhost:7000/api/auth/.backstage/error', + { + credentials: 'include', + }, + ); + expect(error).toBeInstanceOf(Error); + expect((error as Error).name).toEqual(errorObject.name); + expect((error as Error).message).toEqual(errorObject.message); + }); + + it('should return undefined when the backend does not return an error object', async () => { + mockDiscoveryApi.getBaseUrl.mockResolvedValue( + 'http://localhost:7000/api/auth', + ); + + global.fetch = jest.fn().mockResolvedValue({ + ok: true, + json: jest.fn().mockResolvedValue(undefined), + } as unknown as Response); + + const api = SignInAuthErrorApi.create({ discovery: mockDiscoveryApi }); + const error = await api.getSignInAuthError(); + + expect(mockDiscoveryApi.getBaseUrl).toHaveBeenCalledWith('auth'); + expect(fetch).toHaveBeenCalledWith( + 'http://localhost:7000/api/auth/.backstage/error', + { + credentials: 'include', + }, + ); + expect(error).toBeUndefined(); + }); +}); diff --git a/plugins/auth-backend/src/service/createCookieAuthErrorMiddleware.test.ts b/plugins/auth-backend/src/service/createCookieAuthErrorMiddleware.test.ts new file mode 100644 index 0000000000..1d270a953f --- /dev/null +++ b/plugins/auth-backend/src/service/createCookieAuthErrorMiddleware.test.ts @@ -0,0 +1,67 @@ +/* + * Copyright 2024 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 express from 'express'; +import request from 'supertest'; +import { createCookieAuthErrorMiddleware } from './createCookieAuthErrorMiddleware'; + +const AUTH_ERROR_COOKIE = 'auth-error'; + +describe('createCookieAuthErrorMiddleware', () => { + let app: express.Express; + + beforeEach(() => { + app = express(); + app.use(express.json()); + app.use(express.urlencoded({ extended: true })); + + // Mock cookie parser middleware + app.use((req, _, next) => { + req.cookies = {}; + const cookieHeader = req.headers.cookie; + if (cookieHeader) { + const cookies = cookieHeader.split(';'); + cookies.forEach(cookie => { + const [name, ...rest] = cookie.split('='); + req.cookies[name.trim()] = decodeURIComponent(rest.join('=')); + }); + } + next(); + }); + + app.use( + createCookieAuthErrorMiddleware( + 'http://localhost:3000', + 'http://localhost:7000', + ), + ); + }); + + it('should return cookie content if error cookie exists', async () => { + const error = 'test'; + const res = await request(app) + .get('/.backstage/error') + .set('Cookie', `${AUTH_ERROR_COOKIE}=${encodeURIComponent(error)}`); + + expect(res.status).toBe(200); + expect(res.body).toEqual('test'); + }); + + it('should return 404 if error cookie does not exist', async () => { + const res = await request(app).get('/.backstage/error'); + expect(res.status).toBe(404); + }); +}); diff --git a/plugins/auth-backend/src/service/createCookieAuthErrorMiddleware.ts b/plugins/auth-backend/src/service/createCookieAuthErrorMiddleware.ts index 325126d831..30724fc3cd 100644 --- a/plugins/auth-backend/src/service/createCookieAuthErrorMiddleware.ts +++ b/plugins/auth-backend/src/service/createCookieAuthErrorMiddleware.ts @@ -44,7 +44,7 @@ export function createCookieAuthErrorMiddleware( }); res.status(200).json(error); } else { - res.status(404); + res.status(404).end(); } }); From b73387a1118bd7b3cce712753a7820073b6d0a97 Mon Sep 17 00:00:00 2001 From: Stephen Glass Date: Mon, 29 Jul 2024 14:38:48 -0400 Subject: [PATCH 13/28] fix formatting Signed-off-by: Stephen Glass --- packages/core-app-api/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core-app-api/package.json b/packages/core-app-api/package.json index e68faa0b6b..6025fd5677 100644 --- a/packages/core-app-api/package.json +++ b/packages/core-app-api/package.json @@ -82,4 +82,4 @@ "config.d.ts" ], "configSchema": "config.d.ts" -} \ No newline at end of file +} From e4ad29ad8b4d84590b81519f5382ad7e15383e1c Mon Sep 17 00:00:00 2001 From: Stephen Glass Date: Mon, 29 Jul 2024 15:16:31 -0400 Subject: [PATCH 14/28] add changeset Signed-off-by: Stephen Glass --- .changeset/violet-beds-promise.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 .changeset/violet-beds-promise.md diff --git a/.changeset/violet-beds-promise.md b/.changeset/violet-beds-promise.md new file mode 100644 index 0000000000..9fa03646c4 --- /dev/null +++ b/.changeset/violet-beds-promise.md @@ -0,0 +1,17 @@ +--- +'@backstage/core-components': patch +'@backstage/core-plugin-api': patch +'@backstage/app-defaults': patch +'@backstage/core-app-api': patch +'@backstage/plugin-auth-backend': patch +'@backstage/plugin-auth-node': patch +--- + +Fix error handling using authentication redirect flow via `enableExperimentalRedirectFlow` config. If an error is caught during authentication, the user is redirected back to app origin with `?error=true` query parameter. A cookie is also set in the redirect which contains the error message. The error can be fetched from any custom sign in page using the new `AuthErrorApi` and `getSignInError()` implementation. Example: + +```ts +import { useApi, authErrorApiRef } from '@backstage/core-plugin-api'; + +const authErrorApi = useApi(authErrorApiRef); +const errorResponse = await authErrorApi.getSignInAuthError(); +``` From b6591fa45a0fd23a1d713b21d11cacef64f516f8 Mon Sep 17 00:00:00 2001 From: Stephen Glass Date: Mon, 29 Jul 2024 15:39:29 -0400 Subject: [PATCH 15/28] update changeset Signed-off-by: Stephen Glass --- .changeset/violet-beds-promise.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/violet-beds-promise.md b/.changeset/violet-beds-promise.md index 9fa03646c4..ea5f478543 100644 --- a/.changeset/violet-beds-promise.md +++ b/.changeset/violet-beds-promise.md @@ -7,7 +7,7 @@ '@backstage/plugin-auth-node': patch --- -Fix error handling using authentication redirect flow via `enableExperimentalRedirectFlow` config. If an error is caught during authentication, the user is redirected back to app origin with `?error=true` query parameter. A cookie is also set in the redirect which contains the error message. The error can be fetched from any custom sign in page using the new `AuthErrorApi` and `getSignInError()` implementation. Example: +Fix error handling using authentication redirect flow via `enableExperimentalRedirectFlow` config. If an error is caught during authentication, the user is redirected back to app origin with `?error=true` query parameter. A cookie is also set in the redirect which contains the error message. The error can be fetched from any custom sign in page using the new `AuthErrorApi`. Example: ```ts import { useApi, authErrorApiRef } from '@backstage/core-plugin-api'; From d576de7f9e2313270d0f9750c6169ffa9663e3d1 Mon Sep 17 00:00:00 2001 From: Stephen Glass Date: Mon, 29 Jul 2024 16:11:58 -0400 Subject: [PATCH 16/28] build api reports Signed-off-by: Stephen Glass --- packages/core-app-api/api-report.md | 9 +++++++++ packages/core-plugin-api/api-report.md | 8 ++++++++ 2 files changed, 17 insertions(+) diff --git a/packages/core-app-api/api-report.md b/packages/core-app-api/api-report.md index 8b8cc991c8..ae9d709c88 100644 --- a/packages/core-app-api/api-report.md +++ b/packages/core-app-api/api-report.md @@ -16,6 +16,7 @@ import { AppConfig } from '@backstage/config'; import { AppTheme } from '@backstage/core-plugin-api'; import { AppThemeApi } from '@backstage/core-plugin-api'; import { atlassianAuthApiRef } from '@backstage/core-plugin-api'; +import { AuthErrorApi } from '@backstage/core-plugin-api'; import { AuthProviderInfo } from '@backstage/core-plugin-api'; import { AuthRequestOptions } from '@backstage/core-plugin-api'; import { BackstageIdentityApi } from '@backstage/core-plugin-api'; @@ -635,6 +636,14 @@ export class SamlAuth signOut(): Promise; } +// @public +export class SignInAuthErrorApi implements AuthErrorApi { + // (undocumented) + static create(options: { discovery: DiscoveryApi }): SignInAuthErrorApi; + // (undocumented) + getSignInAuthError(): Promise; +} + // @public export type SignInPageProps = PropsWithChildren<{ onSignInSuccess(identityApi: IdentityApi): void; diff --git a/packages/core-plugin-api/api-report.md b/packages/core-plugin-api/api-report.md index 134b338c92..22e254cc29 100644 --- a/packages/core-plugin-api/api-report.md +++ b/packages/core-plugin-api/api-report.md @@ -194,6 +194,14 @@ export function attachComponentData

( data: unknown, ): void; +// @public +export type AuthErrorApi = { + getSignInAuthError(): Promise; +}; + +// @public +export const authErrorApiRef: ApiRef; + // @public export type AuthProviderInfo = { id: string; From 7fd44657053a75b337f79c7fb41349e7e7fca9cc Mon Sep 17 00:00:00 2001 From: Stephen Glass Date: Mon, 29 Jul 2024 16:39:23 -0400 Subject: [PATCH 17/28] fix createApp test case Signed-off-by: Stephen Glass --- packages/frontend-app-api/src/wiring/createApp.test.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/frontend-app-api/src/wiring/createApp.test.tsx b/packages/frontend-app-api/src/wiring/createApp.test.tsx index 94254a2e01..6bff2d6d10 100644 --- a/packages/frontend-app-api/src/wiring/createApp.test.tsx +++ b/packages/frontend-app-api/src/wiring/createApp.test.tsx @@ -293,6 +293,7 @@ describe('createApp', () => { + ] " `); From cf15b1f53d894e8960495cbe8265b3453e22c15d Mon Sep 17 00:00:00 2001 From: Stephen Glass Date: Mon, 29 Jul 2024 17:12:28 -0400 Subject: [PATCH 18/28] use cookie parser in test instead of mock Signed-off-by: Stephen Glass --- .../createCookieAuthErrorMiddleware.test.ts | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/plugins/auth-backend/src/service/createCookieAuthErrorMiddleware.test.ts b/plugins/auth-backend/src/service/createCookieAuthErrorMiddleware.test.ts index 1d270a953f..f0c547842a 100644 --- a/plugins/auth-backend/src/service/createCookieAuthErrorMiddleware.test.ts +++ b/plugins/auth-backend/src/service/createCookieAuthErrorMiddleware.test.ts @@ -16,6 +16,7 @@ import express from 'express'; import request from 'supertest'; +import cookieParser from 'cookie-parser'; import { createCookieAuthErrorMiddleware } from './createCookieAuthErrorMiddleware'; const AUTH_ERROR_COOKIE = 'auth-error'; @@ -28,19 +29,7 @@ describe('createCookieAuthErrorMiddleware', () => { app.use(express.json()); app.use(express.urlencoded({ extended: true })); - // Mock cookie parser middleware - app.use((req, _, next) => { - req.cookies = {}; - const cookieHeader = req.headers.cookie; - if (cookieHeader) { - const cookies = cookieHeader.split(';'); - cookies.forEach(cookie => { - const [name, ...rest] = cookie.split('='); - req.cookies[name.trim()] = decodeURIComponent(rest.join('=')); - }); - } - next(); - }); + app.use(cookieParser()); app.use( createCookieAuthErrorMiddleware( From 32fe26a57ff6ad31f2543275967b630dd37113f3 Mon Sep 17 00:00:00 2001 From: Stephen Glass Date: Thu, 1 Aug 2024 09:51:30 -0400 Subject: [PATCH 19/28] fix redirect loop using auto prop in signin component Signed-off-by: Stephen Glass --- .../core-components/src/layout/SignInPage/SignInPage.tsx | 7 +++++-- .../core-components/src/layout/SignInPage/providers.tsx | 5 ++++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/packages/core-components/src/layout/SignInPage/SignInPage.tsx b/packages/core-components/src/layout/SignInPage/SignInPage.tsx index a0d5ec232e..932266cdc7 100644 --- a/packages/core-components/src/layout/SignInPage/SignInPage.tsx +++ b/packages/core-components/src/layout/SignInPage/SignInPage.tsx @@ -109,7 +109,10 @@ export const SingleSignInPage = ({ // displayed for a split second when the user is already logged-in. const [showLoginPage, setShowLoginPage] = useState(false); + // User was redirected back to sign in page with error from auth redirect flow const [searchParams, _setSearchParams] = useSearchParams(); + const errorParam = searchParams.get('error'); + const hasErrorSearchParam = errorParam !== 'false' && errorParam !== null; type LoginOpts = { checkExisting?: boolean; showPopup?: boolean }; const login = async ({ checkExisting, showPopup }: LoginOpts) => { @@ -123,7 +126,7 @@ export const SingleSignInPage = ({ } // If no session exists, show the sign-in page - if (!identityResponse && (showPopup || auto)) { + if (!identityResponse && (showPopup || auto) && !hasErrorSearchParam) { // Unless auto is set to true, this step should not happen. // When user intentionally clicks the Sign In button, autoShowPopup is set to true setShowLoginPage(true); @@ -158,7 +161,7 @@ export const SingleSignInPage = ({ }; const [_, { execute: checkAuthErrors }] = useAsync(async () => { - if (searchParams.get('error') !== 'false') { + if (hasErrorSearchParam) { const errorResponse = await authErrorApi.getSignInAuthError(); if (errorResponse) { setError(errorResponse); diff --git a/packages/core-components/src/layout/SignInPage/providers.tsx b/packages/core-components/src/layout/SignInPage/providers.tsx index 4b6c251e7b..1cf4ad54bf 100644 --- a/packages/core-components/src/layout/SignInPage/providers.tsx +++ b/packages/core-components/src/layout/SignInPage/providers.tsx @@ -92,10 +92,13 @@ export const useSignInProviders = ( const authErrorApi = useApi(authErrorApiRef); const [loading, setLoading] = useState(true); + // User was redirected back to sign in page with error from auth redirect flow const [searchParams, _setSearchParams] = useSearchParams(); + const errorParam = searchParams.get('error'); + const hasErrorSearchParam = errorParam !== 'false' && errorParam !== null; const [_, { execute: checkAuthErrors }] = useAsync(async () => { - if (searchParams.get('error') !== 'false') { + if (hasErrorSearchParam) { const errorResponse = await authErrorApi.getSignInAuthError(); if (errorResponse) { errorApi.post(errorResponse); From d8e6b69baeb028abb87ce1d86013d7cea12f0a4d Mon Sep 17 00:00:00 2001 From: Stephen Glass Date: Tue, 13 Aug 2024 23:43:41 -0400 Subject: [PATCH 20/28] change api auth utility to hook Signed-off-by: Stephen Glass --- packages/app-defaults/src/defaults/apis.ts | 9 --- packages/core-app-api/api-report.md | 9 --- packages/core-app-api/package.json | 1 - .../src/apis/implementations/index.ts | 1 - packages/core-components/package.json | 1 + .../src/layout/SignInPage/SignInPage.tsx | 27 ++++--- .../src/layout/SignInPage/providers.tsx | 29 +++---- packages/core-plugin-api/api-report.md | 8 -- .../src/apis/definitions/AuthErrorApi.ts | 45 ----------- .../src/apis/definitions/index.ts | 1 - plugins/auth-react/src/hooks/index.ts | 1 + .../src/hooks/useSignInAuthError/index.tsx | 2 +- .../useSignInAuthError.test.tsx | 76 ++++++++++--------- .../useSignInAuthError/useSignInAuthError.tsx | 29 ++++--- yarn.lock | 2 +- 15 files changed, 86 insertions(+), 155 deletions(-) delete mode 100644 packages/core-plugin-api/src/apis/definitions/AuthErrorApi.ts rename packages/core-app-api/src/apis/implementations/AuthErrorApi/index.ts => plugins/auth-react/src/hooks/useSignInAuthError/index.tsx (91%) rename packages/core-app-api/src/apis/implementations/AuthErrorApi/SignInAuthErrorApi.test.ts => plugins/auth-react/src/hooks/useSignInAuthError/useSignInAuthError.test.tsx (53%) rename packages/core-app-api/src/apis/implementations/AuthErrorApi/SignInAuthErrorApi.ts => plugins/auth-react/src/hooks/useSignInAuthError/useSignInAuthError.tsx (59%) diff --git a/packages/app-defaults/src/defaults/apis.ts b/packages/app-defaults/src/defaults/apis.ts index 84902b9f52..4e9e1a492c 100644 --- a/packages/app-defaults/src/defaults/apis.ts +++ b/packages/app-defaults/src/defaults/apis.ts @@ -35,7 +35,6 @@ import { createFetchApi, FetchMiddlewares, VMwareCloudAuth, - SignInAuthErrorApi, } from '@backstage/core-app-api'; import { @@ -59,7 +58,6 @@ import { bitbucketServerAuthApiRef, atlassianAuthApiRef, vmwareCloudAuthApiRef, - authErrorApiRef, } from '@backstage/core-plugin-api'; import { permissionApiRef, @@ -289,11 +287,4 @@ export const apis = [ factory: ({ config, discovery, identity }) => IdentityPermissionApi.create({ config, discovery, identity }), }), - createApiFactory({ - api: authErrorApiRef, - deps: { - discovery: discoveryApiRef, - }, - factory: ({ discovery }) => SignInAuthErrorApi.create({ discovery }), - }), ]; diff --git a/packages/core-app-api/api-report.md b/packages/core-app-api/api-report.md index ae9d709c88..8b8cc991c8 100644 --- a/packages/core-app-api/api-report.md +++ b/packages/core-app-api/api-report.md @@ -16,7 +16,6 @@ import { AppConfig } from '@backstage/config'; import { AppTheme } from '@backstage/core-plugin-api'; import { AppThemeApi } from '@backstage/core-plugin-api'; import { atlassianAuthApiRef } from '@backstage/core-plugin-api'; -import { AuthErrorApi } from '@backstage/core-plugin-api'; import { AuthProviderInfo } from '@backstage/core-plugin-api'; import { AuthRequestOptions } from '@backstage/core-plugin-api'; import { BackstageIdentityApi } from '@backstage/core-plugin-api'; @@ -636,14 +635,6 @@ export class SamlAuth signOut(): Promise; } -// @public -export class SignInAuthErrorApi implements AuthErrorApi { - // (undocumented) - static create(options: { discovery: DiscoveryApi }): SignInAuthErrorApi; - // (undocumented) - getSignInAuthError(): Promise; -} - // @public export type SignInPageProps = PropsWithChildren<{ onSignInSuccess(identityApi: IdentityApi): void; diff --git a/packages/core-app-api/package.json b/packages/core-app-api/package.json index 6025fd5677..0f78b11f98 100644 --- a/packages/core-app-api/package.json +++ b/packages/core-app-api/package.json @@ -44,7 +44,6 @@ "dependencies": { "@backstage/config": "workspace:^", "@backstage/core-plugin-api": "workspace:^", - "@backstage/errors": "workspace:^", "@backstage/types": "workspace:^", "@backstage/version-bridge": "workspace:^", "@types/prop-types": "^15.7.3", diff --git a/packages/core-app-api/src/apis/implementations/index.ts b/packages/core-app-api/src/apis/implementations/index.ts index 7643dfcd82..1c79d3d164 100644 --- a/packages/core-app-api/src/apis/implementations/index.ts +++ b/packages/core-app-api/src/apis/implementations/index.ts @@ -30,4 +30,3 @@ export * from './FeatureFlagsApi'; export * from './FetchApi'; export * from './OAuthRequestApi'; export * from './StorageApi'; -export * from './AuthErrorApi'; diff --git a/packages/core-components/package.json b/packages/core-components/package.json index a2688a0037..49cecfbb21 100644 --- a/packages/core-components/package.json +++ b/packages/core-components/package.json @@ -57,6 +57,7 @@ "@backstage/config": "workspace:^", "@backstage/core-plugin-api": "workspace:^", "@backstage/errors": "workspace:^", + "@backstage/plugin-auth-react": "workspace:^", "@backstage/theme": "workspace:^", "@backstage/version-bridge": "workspace:^", "@date-io/core": "^1.3.13", diff --git a/packages/core-components/src/layout/SignInPage/SignInPage.tsx b/packages/core-components/src/layout/SignInPage/SignInPage.tsx index 932266cdc7..d7e8d1fcf6 100644 --- a/packages/core-components/src/layout/SignInPage/SignInPage.tsx +++ b/packages/core-components/src/layout/SignInPage/SignInPage.tsx @@ -19,14 +19,13 @@ import { configApiRef, SignInPageProps, useApi, - authErrorApiRef, } from '@backstage/core-plugin-api'; import { UserIdentity } from './UserIdentity'; import Button from '@material-ui/core/Button'; import Grid from '@material-ui/core/Grid'; import Typography from '@material-ui/core/Typography'; -import React, { useState } from 'react'; -import { useAsync, useMountEffect } from '@react-hookz/web'; +import React, { useEffect, useState } from 'react'; +import { useMountEffect } from '@react-hookz/web'; import { Progress } from '../../components/Progress'; import { Content } from '../Content/Content'; import { ContentHeader } from '../ContentHeader/ContentHeader'; @@ -39,6 +38,7 @@ import { IdentityProviders, SignInProviderConfig } from './types'; import { coreComponentsTranslationRef } from '../../translation'; import { useTranslationRef } from '@backstage/core-plugin-api/alpha'; import { useSearchParams } from 'react-router-dom'; +import { useSignInAuthError } from '@backstage/plugin-auth-react'; type MultiSignInPageProps = SignInPageProps & { providers: IdentityProviders; @@ -99,7 +99,7 @@ export const SingleSignInPage = ({ const classes = useStyles(); const authApi = useApi(provider.apiRef); const configApi = useApi(configApiRef); - const authErrorApi = useApi(authErrorApiRef); + const { error: signInError, checkAuthError } = useSignInAuthError(); const { t } = useTranslationRef(coreComponentsTranslationRef); const [error, setError] = useState(); @@ -160,20 +160,19 @@ export const SingleSignInPage = ({ } }; - const [_, { execute: checkAuthErrors }] = useAsync(async () => { - if (hasErrorSearchParam) { - const errorResponse = await authErrorApi.getSignInAuthError(); - if (errorResponse) { - setError(errorResponse); - } - } - }); - useMountEffect(() => { - checkAuthErrors(); + if (hasErrorSearchParam) { + checkAuthError(); + } login({ checkExisting: true }); }); + useEffect(() => { + if (signInError) { + setError(signInError); + } + }, [signInError]); + return showLoginPage ? (

diff --git a/packages/core-components/src/layout/SignInPage/providers.tsx b/packages/core-components/src/layout/SignInPage/providers.tsx index 1cf4ad54bf..8d34d8d9de 100644 --- a/packages/core-components/src/layout/SignInPage/providers.tsx +++ b/packages/core-components/src/layout/SignInPage/providers.tsx @@ -14,14 +14,19 @@ * limitations under the License. */ -import React, { useLayoutEffect, useState, useMemo, useCallback } from 'react'; +import React, { + useLayoutEffect, + useState, + useMemo, + useCallback, + useEffect, +} from 'react'; import { SignInPageProps, useApi, useApiHolder, errorApiRef, IdentityApi, - authErrorApiRef, } from '@backstage/core-plugin-api'; import { IdentityProviders, @@ -33,7 +38,8 @@ import { guestProvider } from './guestProvider'; import { customProvider } from './customProvider'; import { IdentityApiSignOutProxy } from './IdentityApiSignOutProxy'; import { useSearchParams } from 'react-router-dom'; -import { useMountEffect, useAsync } from '@react-hookz/web'; +import { useMountEffect } from '@react-hookz/web'; +import { useSignInAuthError } from '@backstage/plugin-auth-react'; const PROVIDER_STORAGE_KEY = '@backstage/core:SignInPage:provider'; @@ -89,7 +95,7 @@ export const useSignInProviders = ( ) => { const errorApi = useApi(errorApiRef); const apiHolder = useApiHolder(); - const authErrorApi = useApi(authErrorApiRef); + const { error: signInError, checkAuthError } = useSignInAuthError(); const [loading, setLoading] = useState(true); // User was redirected back to sign in page with error from auth redirect flow @@ -97,16 +103,13 @@ export const useSignInProviders = ( const errorParam = searchParams.get('error'); const hasErrorSearchParam = errorParam !== 'false' && errorParam !== null; - const [_, { execute: checkAuthErrors }] = useAsync(async () => { - if (hasErrorSearchParam) { - const errorResponse = await authErrorApi.getSignInAuthError(); - if (errorResponse) { - errorApi.post(errorResponse); - } - } - }); + useMountEffect(() => hasErrorSearchParam && checkAuthError()); - useMountEffect(checkAuthErrors); + useEffect(() => { + if (signInError) { + errorApi.post(signInError); + } + }, [errorApi, signInError]); // This decorates the result with sign out logic from this hook const handleWrappedResult = useCallback( diff --git a/packages/core-plugin-api/api-report.md b/packages/core-plugin-api/api-report.md index 22e254cc29..134b338c92 100644 --- a/packages/core-plugin-api/api-report.md +++ b/packages/core-plugin-api/api-report.md @@ -194,14 +194,6 @@ export function attachComponentData

( data: unknown, ): void; -// @public -export type AuthErrorApi = { - getSignInAuthError(): Promise; -}; - -// @public -export const authErrorApiRef: ApiRef; - // @public export type AuthProviderInfo = { id: string; diff --git a/packages/core-plugin-api/src/apis/definitions/AuthErrorApi.ts b/packages/core-plugin-api/src/apis/definitions/AuthErrorApi.ts deleted file mode 100644 index 558c1202a8..0000000000 --- a/packages/core-plugin-api/src/apis/definitions/AuthErrorApi.ts +++ /dev/null @@ -1,45 +0,0 @@ -/* - * Copyright 2024 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 { ApiRef, createApiRef } from '../system'; - -/** - * A wrapper for retrieving any errors caught in authentication redirect flow - * - * @public - */ -export type AuthErrorApi = { - /** - * Get any caught errors during the auth redirect flow - */ - getSignInAuthError(): Promise; -}; - -/** - * The {@link ApiRef} of {@link AuthErrorApi}. - * - * @remarks - * - * This is a wrapper that uses fetch to retrieve any caught errors - * in the authentication redirect flow process. This API calls a - * Backstage endpoint to read any error stored in authentication error - * cookie and returns it to the user. - * - * @public - */ -export const authErrorApiRef: ApiRef = createApiRef({ - id: 'core.auth-error', -}); diff --git a/packages/core-plugin-api/src/apis/definitions/index.ts b/packages/core-plugin-api/src/apis/definitions/index.ts index 406dc1980e..67d442587d 100644 --- a/packages/core-plugin-api/src/apis/definitions/index.ts +++ b/packages/core-plugin-api/src/apis/definitions/index.ts @@ -33,4 +33,3 @@ export * from './FetchApi'; export * from './IdentityApi'; export * from './OAuthRequestApi'; export * from './StorageApi'; -export * from './AuthErrorApi'; diff --git a/plugins/auth-react/src/hooks/index.ts b/plugins/auth-react/src/hooks/index.ts index 1257334498..0e87bab67f 100644 --- a/plugins/auth-react/src/hooks/index.ts +++ b/plugins/auth-react/src/hooks/index.ts @@ -18,3 +18,4 @@ // which hooks are public API and should be exported from the package. export * from './useCookieAuthRefresh'; +export * from './useSignInAuthError'; diff --git a/packages/core-app-api/src/apis/implementations/AuthErrorApi/index.ts b/plugins/auth-react/src/hooks/useSignInAuthError/index.tsx similarity index 91% rename from packages/core-app-api/src/apis/implementations/AuthErrorApi/index.ts rename to plugins/auth-react/src/hooks/useSignInAuthError/index.tsx index 6f3a3ae269..549b5b9f1f 100644 --- a/packages/core-app-api/src/apis/implementations/AuthErrorApi/index.ts +++ b/plugins/auth-react/src/hooks/useSignInAuthError/index.tsx @@ -14,4 +14,4 @@ * limitations under the License. */ -export { SignInAuthErrorApi } from './SignInAuthErrorApi'; +export { useSignInAuthError } from './useSignInAuthError'; diff --git a/packages/core-app-api/src/apis/implementations/AuthErrorApi/SignInAuthErrorApi.test.ts b/plugins/auth-react/src/hooks/useSignInAuthError/useSignInAuthError.test.tsx similarity index 53% rename from packages/core-app-api/src/apis/implementations/AuthErrorApi/SignInAuthErrorApi.test.ts rename to plugins/auth-react/src/hooks/useSignInAuthError/useSignInAuthError.test.tsx index c4d6fe83de..a5ac57ec83 100644 --- a/packages/core-app-api/src/apis/implementations/AuthErrorApi/SignInAuthErrorApi.test.ts +++ b/plugins/auth-react/src/hooks/useSignInAuthError/useSignInAuthError.test.tsx @@ -14,29 +14,20 @@ * limitations under the License. */ -import { DiscoveryApi } from '@backstage/core-plugin-api'; +import React from 'react'; +import { act, renderHook } from '@testing-library/react'; +import { discoveryApiRef } from '@backstage/core-plugin-api'; +import { TestApiProvider } from '@backstage/test-utils'; +import { useSignInAuthError } from './useSignInAuthError'; import { serializeError } from '@backstage/errors'; -import { SignInAuthErrorApi } from './SignInAuthErrorApi'; -describe('SignInAuthErrorApi', () => { - const mockDiscoveryApi = { - getBaseUrl: jest.fn(), - } as unknown as jest.Mocked; +describe('useCookieAuthRefresh', () => { + const discoveryApiMock = { + getBaseUrl: jest.fn().mockResolvedValue('http://localhost:7000/api/auth'), + }; beforeEach(() => { - jest.resetAllMocks(); - }); - - it('should create an instance of SignInAuthErrorApi', async () => { - const api = SignInAuthErrorApi.create({ - discovery: mockDiscoveryApi, - }); - - mockDiscoveryApi.getBaseUrl.mockResolvedValue( - 'http://localhost:7007/api/auth', - ); - - expect(api).toBeInstanceOf(SignInAuthErrorApi); + jest.clearAllMocks(); }); it('should return an error when the cookie returns an error object', async () => { @@ -45,50 +36,63 @@ describe('SignInAuthErrorApi', () => { message: 'This is a test error', }; const serializedError = serializeError(errorObject); - mockDiscoveryApi.getBaseUrl.mockResolvedValue( - 'http://localhost:7000/api/auth', - ); global.fetch = jest.fn().mockResolvedValue({ ok: true, json: jest.fn().mockResolvedValue(serializedError), } as unknown as Response); - const api = SignInAuthErrorApi.create({ discovery: mockDiscoveryApi }); - const error = await api.getSignInAuthError(); + const { result } = renderHook(() => useSignInAuthError(), { + wrapper: ({ children }) => ( + + {children} + + ), + }); - expect(mockDiscoveryApi.getBaseUrl).toHaveBeenCalledWith('auth'); + await act(async () => { + result.current.checkAuthError(); + }); + + expect(discoveryApiMock.getBaseUrl).toHaveBeenCalledWith('auth'); expect(fetch).toHaveBeenCalledWith( 'http://localhost:7000/api/auth/.backstage/error', { credentials: 'include', }, ); - expect(error).toBeInstanceOf(Error); - expect((error as Error).name).toEqual(errorObject.name); - expect((error as Error).message).toEqual(errorObject.message); + expect(result.current.error).toBeInstanceOf(Error); + expect((result.current.error as Error).name).toEqual(errorObject.name); + expect((result.current.error as Error).message).toEqual( + errorObject.message, + ); }); it('should return undefined when the backend does not return an error object', async () => { - mockDiscoveryApi.getBaseUrl.mockResolvedValue( - 'http://localhost:7000/api/auth', - ); - global.fetch = jest.fn().mockResolvedValue({ ok: true, json: jest.fn().mockResolvedValue(undefined), } as unknown as Response); - const api = SignInAuthErrorApi.create({ discovery: mockDiscoveryApi }); - const error = await api.getSignInAuthError(); + const { result } = renderHook(() => useSignInAuthError(), { + wrapper: ({ children }) => ( + + {children} + + ), + }); - expect(mockDiscoveryApi.getBaseUrl).toHaveBeenCalledWith('auth'); + await act(async () => { + result.current.checkAuthError(); + }); + + expect(discoveryApiMock.getBaseUrl).toHaveBeenCalledWith('auth'); expect(fetch).toHaveBeenCalledWith( 'http://localhost:7000/api/auth/.backstage/error', { credentials: 'include', }, ); - expect(error).toBeUndefined(); + expect(result.current.error).toBeUndefined(); }); }); diff --git a/packages/core-app-api/src/apis/implementations/AuthErrorApi/SignInAuthErrorApi.ts b/plugins/auth-react/src/hooks/useSignInAuthError/useSignInAuthError.tsx similarity index 59% rename from packages/core-app-api/src/apis/implementations/AuthErrorApi/SignInAuthErrorApi.ts rename to plugins/auth-react/src/hooks/useSignInAuthError/useSignInAuthError.tsx index 77b3c53f14..8d4af1f555 100644 --- a/packages/core-app-api/src/apis/implementations/AuthErrorApi/SignInAuthErrorApi.ts +++ b/plugins/auth-react/src/hooks/useSignInAuthError/useSignInAuthError.tsx @@ -14,25 +14,20 @@ * limitations under the License. */ -import { DiscoveryApi, AuthErrorApi } from '@backstage/core-plugin-api'; +import { discoveryApiRef, useApi } from '@backstage/core-plugin-api'; +import { useAsync } from '@react-hookz/web'; import { deserializeError } from '@backstage/errors'; -/** - * The default implementation of the AuthErrorApi, which simply calls auth error endpoint. - * @public - */ -export class SignInAuthErrorApi implements AuthErrorApi { - private constructor(private readonly discoveryApi: DiscoveryApi) {} +export function useSignInAuthError(): { + error: Error | undefined; + checkAuthError: () => void; +} { + const discoveryApi = useApi(discoveryApiRef); - static create(options: { discovery: DiscoveryApi }) { - const { discovery } = options; - return new SignInAuthErrorApi(discovery); - } + const [state, { execute: checkAuthError }] = useAsync(async () => { + const baseUrl = await discoveryApi.getBaseUrl('auth'); - async getSignInAuthError(): Promise { - const baseUrl = await this.discoveryApi.getBaseUrl('auth'); - - // use native fetch instead of depending on fetchApi because + // use native fetch instead of fetchApi because // we are not signed in and are calling an unauthenticated endpoint const response = await fetch(`${baseUrl}/.backstage/error`, { credentials: 'include', @@ -40,5 +35,7 @@ export class SignInAuthErrorApi implements AuthErrorApi { const data = await response.json(); return data ? deserializeError(data) : undefined; - } + }); + + return { error: state.result, checkAuthError }; } diff --git a/yarn.lock b/yarn.lock index cb41faa20f..dfd1cffd9e 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4123,7 +4123,6 @@ __metadata: "@backstage/cli": "workspace:^" "@backstage/config": "workspace:^" "@backstage/core-plugin-api": "workspace:^" - "@backstage/errors": "workspace:^" "@backstage/test-utils": "workspace:^" "@backstage/types": "workspace:^" "@backstage/version-bridge": "workspace:^" @@ -4288,6 +4287,7 @@ __metadata: "@backstage/core-app-api": "workspace:^" "@backstage/core-plugin-api": "workspace:^" "@backstage/errors": "workspace:^" + "@backstage/plugin-auth-react": "workspace:^" "@backstage/test-utils": "workspace:^" "@backstage/theme": "workspace:^" "@backstage/version-bridge": "workspace:^" From d2757e9ca02152d1506d27c012019b7c9edb0651 Mon Sep 17 00:00:00 2001 From: Stephen Glass Date: Tue, 13 Aug 2024 23:44:14 -0400 Subject: [PATCH 21/28] update test Signed-off-by: Stephen Glass --- packages/frontend-app-api/src/wiring/createApp.test.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/frontend-app-api/src/wiring/createApp.test.tsx b/packages/frontend-app-api/src/wiring/createApp.test.tsx index 6bff2d6d10..94254a2e01 100644 --- a/packages/frontend-app-api/src/wiring/createApp.test.tsx +++ b/packages/frontend-app-api/src/wiring/createApp.test.tsx @@ -293,7 +293,6 @@ describe('createApp', () => { - ] " `); From f82952433fdb8e98eaa943a6ccc173a92e8f39e8 Mon Sep 17 00:00:00 2001 From: Stephen Glass Date: Tue, 13 Aug 2024 23:52:51 -0400 Subject: [PATCH 22/28] update changeset Signed-off-by: Stephen Glass --- .changeset/violet-beds-promise.md | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/.changeset/violet-beds-promise.md b/.changeset/violet-beds-promise.md index ea5f478543..5fd4890ffa 100644 --- a/.changeset/violet-beds-promise.md +++ b/.changeset/violet-beds-promise.md @@ -1,17 +1,14 @@ --- '@backstage/core-components': patch -'@backstage/core-plugin-api': patch -'@backstage/app-defaults': patch -'@backstage/core-app-api': patch '@backstage/plugin-auth-backend': patch '@backstage/plugin-auth-node': patch +'@backstage/plugin-auth-react': patch --- -Fix error handling using authentication redirect flow via `enableExperimentalRedirectFlow` config. If an error is caught during authentication, the user is redirected back to app origin with `?error=true` query parameter. A cookie is also set in the redirect which contains the error message. The error can be fetched from any custom sign in page using the new `AuthErrorApi`. Example: +Fix error handling using authentication redirect flow via `enableExperimentalRedirectFlow` config. If an error is caught during authentication, the user is redirected back to app origin with `?error=true` query parameter. A cookie is also set in the redirect which contains the error message. The error can be fetched from any custom sign in page using the new `useSignInAuthError` hook.: ```ts -import { useApi, authErrorApiRef } from '@backstage/core-plugin-api'; +import { useSignInAuthError } from '@backstage/plugin-auth-react'; -const authErrorApi = useApi(authErrorApiRef); -const errorResponse = await authErrorApi.getSignInAuthError(); +const { error, checkAuthError } = useSignInAuthError(); ``` From 2ae5f4b2f72a729b61b51698c9693dc810f93da4 Mon Sep 17 00:00:00 2001 From: Stephen Glass Date: Wed, 14 Aug 2024 00:11:46 -0400 Subject: [PATCH 23/28] api report Signed-off-by: Stephen Glass --- plugins/auth-react/api-report.md | 6 ++++++ .../src/hooks/useSignInAuthError/useSignInAuthError.tsx | 4 ++++ 2 files changed, 10 insertions(+) diff --git a/plugins/auth-react/api-report.md b/plugins/auth-react/api-report.md index d5206261d2..52e6c6bc3a 100644 --- a/plugins/auth-react/api-report.md +++ b/plugins/auth-react/api-report.md @@ -36,4 +36,10 @@ export function useCookieAuthRefresh(options: { pluginId: string }): expiresAt: string; }; }; + +// @public +export function useSignInAuthError(): { + error: Error | undefined; + checkAuthError: () => void; +}; ``` diff --git a/plugins/auth-react/src/hooks/useSignInAuthError/useSignInAuthError.tsx b/plugins/auth-react/src/hooks/useSignInAuthError/useSignInAuthError.tsx index 8d4af1f555..fe146fedca 100644 --- a/plugins/auth-react/src/hooks/useSignInAuthError/useSignInAuthError.tsx +++ b/plugins/auth-react/src/hooks/useSignInAuthError/useSignInAuthError.tsx @@ -18,6 +18,10 @@ import { discoveryApiRef, useApi } from '@backstage/core-plugin-api'; import { useAsync } from '@react-hookz/web'; import { deserializeError } from '@backstage/errors'; +/** + * @public + * A hook that will fetch any sign in auth error in redirect auth flow + */ export function useSignInAuthError(): { error: Error | undefined; checkAuthError: () => void; From cd7a503bbc8360297db2f10443851fa237524cac Mon Sep 17 00:00:00 2001 From: Stephen Glass Date: Wed, 14 Aug 2024 20:58:32 -0400 Subject: [PATCH 24/28] trigger ci Signed-off-by: Stephen Glass --- .changeset/violet-beds-promise.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/violet-beds-promise.md b/.changeset/violet-beds-promise.md index 5fd4890ffa..7632ea68c7 100644 --- a/.changeset/violet-beds-promise.md +++ b/.changeset/violet-beds-promise.md @@ -5,7 +5,7 @@ '@backstage/plugin-auth-react': patch --- -Fix error handling using authentication redirect flow via `enableExperimentalRedirectFlow` config. If an error is caught during authentication, the user is redirected back to app origin with `?error=true` query parameter. A cookie is also set in the redirect which contains the error message. The error can be fetched from any custom sign in page using the new `useSignInAuthError` hook.: +Fix authentication error handling using redirect flow via `enableExperimentalRedirectFlow` config. If an error is caught during authentication, the user is redirected back to app origin with `?error=true` query parameter. A cookie is also set in the redirect which contains the error message. The error can be fetched from any custom sign in page using the new `useSignInAuthError` hook.: ```ts import { useSignInAuthError } from '@backstage/plugin-auth-react'; From 65a5939be6b82d5b75626a4862f5461f81ad4a52 Mon Sep 17 00:00:00 2001 From: Stephen Glass Date: Wed, 14 Aug 2024 20:59:23 -0400 Subject: [PATCH 25/28] fix typo Signed-off-by: Stephen Glass --- .changeset/violet-beds-promise.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/violet-beds-promise.md b/.changeset/violet-beds-promise.md index 7632ea68c7..ee002d8319 100644 --- a/.changeset/violet-beds-promise.md +++ b/.changeset/violet-beds-promise.md @@ -5,7 +5,7 @@ '@backstage/plugin-auth-react': patch --- -Fix authentication error handling using redirect flow via `enableExperimentalRedirectFlow` config. If an error is caught during authentication, the user is redirected back to app origin with `?error=true` query parameter. A cookie is also set in the redirect which contains the error message. The error can be fetched from any custom sign in page using the new `useSignInAuthError` hook.: +Fix authentication error handling using redirect flow via `enableExperimentalRedirectFlow` config. If an error is caught during authentication, the user is redirected back to app origin with `?error=true` query parameter. A cookie is also set in the redirect which contains the error message. The error can be fetched from any custom sign in page using the new `useSignInAuthError` hook. ```ts import { useSignInAuthError } from '@backstage/plugin-auth-react'; From d964eb1a030d5271b848d644d79692c77ef8df41 Mon Sep 17 00:00:00 2001 From: Stephen Glass Date: Fri, 23 Aug 2024 10:22:14 -0400 Subject: [PATCH 26/28] fix test suite name for auth error hook Signed-off-by: Stephen Glass --- .../src/hooks/useSignInAuthError/useSignInAuthError.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/auth-react/src/hooks/useSignInAuthError/useSignInAuthError.test.tsx b/plugins/auth-react/src/hooks/useSignInAuthError/useSignInAuthError.test.tsx index a5ac57ec83..0c67c419f8 100644 --- a/plugins/auth-react/src/hooks/useSignInAuthError/useSignInAuthError.test.tsx +++ b/plugins/auth-react/src/hooks/useSignInAuthError/useSignInAuthError.test.tsx @@ -21,7 +21,7 @@ import { TestApiProvider } from '@backstage/test-utils'; import { useSignInAuthError } from './useSignInAuthError'; import { serializeError } from '@backstage/errors'; -describe('useCookieAuthRefresh', () => { +describe('useSignInAuthError', () => { const discoveryApiMock = { getBaseUrl: jest.fn().mockResolvedValue('http://localhost:7000/api/auth'), }; From 4935d29d151f335b1c1cb8a50e76fac4f0b8f9e9 Mon Sep 17 00:00:00 2001 From: Stephen Glass Date: Tue, 1 Oct 2024 23:12:10 -0400 Subject: [PATCH 27/28] change code to use search params instead of cookie Signed-off-by: Stephen Glass --- .changeset/violet-beds-promise.md | 10 +- packages/core-components/package.json | 1 - .../src/layout/SignInPage/SignInPage.tsx | 17 +--- .../src/layout/SignInPage/providers.tsx | 32 +++--- .../createCookieAuthErrorMiddleware.test.ts | 56 ----------- .../createCookieAuthErrorMiddleware.ts | 52 ---------- plugins/auth-backend/src/service/router.ts | 3 - .../src/oauth/createAuthErrorCookie.ts | 55 ----------- .../oauth/createOAuthRouteHandlers.test.ts | 20 ++-- .../src/oauth/createOAuthRouteHandlers.ts | 8 +- plugins/auth-react/api-report.md | 6 -- plugins/auth-react/src/hooks/index.ts | 1 - .../src/hooks/useSignInAuthError/index.tsx | 17 ---- .../useSignInAuthError.test.tsx | 98 ------------------- .../useSignInAuthError/useSignInAuthError.tsx | 45 --------- yarn.lock | 1 - 16 files changed, 34 insertions(+), 388 deletions(-) delete mode 100644 plugins/auth-backend/src/service/createCookieAuthErrorMiddleware.test.ts delete mode 100644 plugins/auth-backend/src/service/createCookieAuthErrorMiddleware.ts delete mode 100644 plugins/auth-node/src/oauth/createAuthErrorCookie.ts delete mode 100644 plugins/auth-react/src/hooks/useSignInAuthError/index.tsx delete mode 100644 plugins/auth-react/src/hooks/useSignInAuthError/useSignInAuthError.test.tsx delete mode 100644 plugins/auth-react/src/hooks/useSignInAuthError/useSignInAuthError.tsx diff --git a/.changeset/violet-beds-promise.md b/.changeset/violet-beds-promise.md index ee002d8319..b5369d4c56 100644 --- a/.changeset/violet-beds-promise.md +++ b/.changeset/violet-beds-promise.md @@ -1,14 +1,6 @@ --- '@backstage/core-components': patch -'@backstage/plugin-auth-backend': patch '@backstage/plugin-auth-node': patch -'@backstage/plugin-auth-react': patch --- -Fix authentication error handling using redirect flow via `enableExperimentalRedirectFlow` config. If an error is caught during authentication, the user is redirected back to app origin with `?error=true` query parameter. A cookie is also set in the redirect which contains the error message. The error can be fetched from any custom sign in page using the new `useSignInAuthError` hook. - -```ts -import { useSignInAuthError } from '@backstage/plugin-auth-react'; - -const { error, checkAuthError } = useSignInAuthError(); -``` +Fix authentication error handling using redirect flow via `enableExperimentalRedirectFlow` config. If an error is caught during authentication, the user is redirected back to app origin with `error` query parameter containing the error message. diff --git a/packages/core-components/package.json b/packages/core-components/package.json index 49cecfbb21..a2688a0037 100644 --- a/packages/core-components/package.json +++ b/packages/core-components/package.json @@ -57,7 +57,6 @@ "@backstage/config": "workspace:^", "@backstage/core-plugin-api": "workspace:^", "@backstage/errors": "workspace:^", - "@backstage/plugin-auth-react": "workspace:^", "@backstage/theme": "workspace:^", "@backstage/version-bridge": "workspace:^", "@date-io/core": "^1.3.13", diff --git a/packages/core-components/src/layout/SignInPage/SignInPage.tsx b/packages/core-components/src/layout/SignInPage/SignInPage.tsx index d7e8d1fcf6..51e8b62199 100644 --- a/packages/core-components/src/layout/SignInPage/SignInPage.tsx +++ b/packages/core-components/src/layout/SignInPage/SignInPage.tsx @@ -24,7 +24,7 @@ import { UserIdentity } from './UserIdentity'; import Button from '@material-ui/core/Button'; import Grid from '@material-ui/core/Grid'; import Typography from '@material-ui/core/Typography'; -import React, { useEffect, useState } from 'react'; +import React, { useState } from 'react'; import { useMountEffect } from '@react-hookz/web'; import { Progress } from '../../components/Progress'; import { Content } from '../Content/Content'; @@ -38,7 +38,6 @@ import { IdentityProviders, SignInProviderConfig } from './types'; import { coreComponentsTranslationRef } from '../../translation'; import { useTranslationRef } from '@backstage/core-plugin-api/alpha'; import { useSearchParams } from 'react-router-dom'; -import { useSignInAuthError } from '@backstage/plugin-auth-react'; type MultiSignInPageProps = SignInPageProps & { providers: IdentityProviders; @@ -99,7 +98,6 @@ export const SingleSignInPage = ({ const classes = useStyles(); const authApi = useApi(provider.apiRef); const configApi = useApi(configApiRef); - const { error: signInError, checkAuthError } = useSignInAuthError(); const { t } = useTranslationRef(coreComponentsTranslationRef); const [error, setError] = useState(); @@ -112,7 +110,6 @@ export const SingleSignInPage = ({ // User was redirected back to sign in page with error from auth redirect flow const [searchParams, _setSearchParams] = useSearchParams(); const errorParam = searchParams.get('error'); - const hasErrorSearchParam = errorParam !== 'false' && errorParam !== null; type LoginOpts = { checkExisting?: boolean; showPopup?: boolean }; const login = async ({ checkExisting, showPopup }: LoginOpts) => { @@ -126,7 +123,7 @@ export const SingleSignInPage = ({ } // If no session exists, show the sign-in page - if (!identityResponse && (showPopup || auto) && !hasErrorSearchParam) { + if (!identityResponse && (showPopup || auto) && !errorParam) { // Unless auto is set to true, this step should not happen. // When user intentionally clicks the Sign In button, autoShowPopup is set to true setShowLoginPage(true); @@ -161,18 +158,12 @@ export const SingleSignInPage = ({ }; useMountEffect(() => { - if (hasErrorSearchParam) { - checkAuthError(); + if (errorParam) { + setError(new Error(decodeURIComponent(errorParam))); } login({ checkExisting: true }); }); - useEffect(() => { - if (signInError) { - setError(signInError); - } - }, [signInError]); - return showLoginPage ? (

diff --git a/packages/core-components/src/layout/SignInPage/providers.tsx b/packages/core-components/src/layout/SignInPage/providers.tsx index 8d34d8d9de..9cb2fa3c9f 100644 --- a/packages/core-components/src/layout/SignInPage/providers.tsx +++ b/packages/core-components/src/layout/SignInPage/providers.tsx @@ -14,13 +14,7 @@ * limitations under the License. */ -import React, { - useLayoutEffect, - useState, - useMemo, - useCallback, - useEffect, -} from 'react'; +import React, { useLayoutEffect, useState, useMemo, useCallback } from 'react'; import { SignInPageProps, useApi, @@ -39,7 +33,9 @@ import { customProvider } from './customProvider'; import { IdentityApiSignOutProxy } from './IdentityApiSignOutProxy'; import { useSearchParams } from 'react-router-dom'; import { useMountEffect } from '@react-hookz/web'; -import { useSignInAuthError } from '@backstage/plugin-auth-react'; +import { ForwardedError } from '@backstage/errors'; +import { coreComponentsTranslationRef } from '../../translation'; +import { useTranslationRef } from '@backstage/core-plugin-api/alpha'; const PROVIDER_STORAGE_KEY = '@backstage/core:SignInPage:provider'; @@ -95,21 +91,23 @@ export const useSignInProviders = ( ) => { const errorApi = useApi(errorApiRef); const apiHolder = useApiHolder(); - const { error: signInError, checkAuthError } = useSignInAuthError(); const [loading, setLoading] = useState(true); + const { t } = useTranslationRef(coreComponentsTranslationRef); // User was redirected back to sign in page with error from auth redirect flow const [searchParams, _setSearchParams] = useSearchParams(); - const errorParam = searchParams.get('error'); - const hasErrorSearchParam = errorParam !== 'false' && errorParam !== null; - useMountEffect(() => hasErrorSearchParam && checkAuthError()); - - useEffect(() => { - if (signInError) { - errorApi.post(signInError); + useMountEffect(() => { + const errorParam = searchParams.get('error'); + if (errorParam) { + errorApi.post( + new ForwardedError(t('signIn.loginFailed'), { + name: 'Error', + message: decodeURIComponent(errorParam), + }), + ); } - }, [errorApi, signInError]); + }); // This decorates the result with sign out logic from this hook const handleWrappedResult = useCallback( diff --git a/plugins/auth-backend/src/service/createCookieAuthErrorMiddleware.test.ts b/plugins/auth-backend/src/service/createCookieAuthErrorMiddleware.test.ts deleted file mode 100644 index f0c547842a..0000000000 --- a/plugins/auth-backend/src/service/createCookieAuthErrorMiddleware.test.ts +++ /dev/null @@ -1,56 +0,0 @@ -/* - * Copyright 2024 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 express from 'express'; -import request from 'supertest'; -import cookieParser from 'cookie-parser'; -import { createCookieAuthErrorMiddleware } from './createCookieAuthErrorMiddleware'; - -const AUTH_ERROR_COOKIE = 'auth-error'; - -describe('createCookieAuthErrorMiddleware', () => { - let app: express.Express; - - beforeEach(() => { - app = express(); - app.use(express.json()); - app.use(express.urlencoded({ extended: true })); - - app.use(cookieParser()); - - app.use( - createCookieAuthErrorMiddleware( - 'http://localhost:3000', - 'http://localhost:7000', - ), - ); - }); - - it('should return cookie content if error cookie exists', async () => { - const error = 'test'; - const res = await request(app) - .get('/.backstage/error') - .set('Cookie', `${AUTH_ERROR_COOKIE}=${encodeURIComponent(error)}`); - - expect(res.status).toBe(200); - expect(res.body).toEqual('test'); - }); - - it('should return 404 if error cookie does not exist', async () => { - const res = await request(app).get('/.backstage/error'); - expect(res.status).toBe(404); - }); -}); diff --git a/plugins/auth-backend/src/service/createCookieAuthErrorMiddleware.ts b/plugins/auth-backend/src/service/createCookieAuthErrorMiddleware.ts deleted file mode 100644 index 30724fc3cd..0000000000 --- a/plugins/auth-backend/src/service/createCookieAuthErrorMiddleware.ts +++ /dev/null @@ -1,52 +0,0 @@ -/* - * Copyright 2024 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 Router from 'express-promise-router'; - -const AUTH_ERROR_COOKIE = 'auth-error'; - -/** - * @public - * Creates a middleware that can be used to get auth errors in redirect flow - */ -export function createCookieAuthErrorMiddleware( - appUrl: string, - authUrl: string, -) { - const router = Router(); - - router.get('/.backstage/error', async (req, res) => { - const error = req.cookies[AUTH_ERROR_COOKIE]; - if (error) { - const { hostname: domain, protocol } = new URL(authUrl); - const secure = protocol === 'https:'; - const sameSite = - new URL(appUrl).hostname !== domain && secure ? 'none' : 'lax'; - - res.clearCookie(AUTH_ERROR_COOKIE, { - path: '/api/auth/.backstage/error', - domain, - sameSite, - secure, - }); - res.status(200).json(error); - } else { - res.status(404).end(); - } - }); - - return router; -} diff --git a/plugins/auth-backend/src/service/router.ts b/plugins/auth-backend/src/service/router.ts index 57f01cfba1..4bb42c3abd 100644 --- a/plugins/auth-backend/src/service/router.ts +++ b/plugins/auth-backend/src/service/router.ts @@ -48,7 +48,6 @@ import { StaticTokenIssuer } from '../identity/StaticTokenIssuer'; import { StaticKeyStore } from '../identity/StaticKeyStore'; import { Config } from '@backstage/config'; import { bindProviderRouters, ProviderFactories } from '../providers/router'; -import { createCookieAuthErrorMiddleware } from './createCookieAuthErrorMiddleware'; /** @public */ export interface RouterOptions { @@ -170,8 +169,6 @@ export async function createRouter( userInfoDatabaseHandler, }); - router.use(createCookieAuthErrorMiddleware(appUrl, authUrl)); - // Gives a more helpful error message than a plain 404 router.use('/:provider/', req => { const { provider } = req.params; diff --git a/plugins/auth-node/src/oauth/createAuthErrorCookie.ts b/plugins/auth-node/src/oauth/createAuthErrorCookie.ts deleted file mode 100644 index 46ef4c4a1c..0000000000 --- a/plugins/auth-node/src/oauth/createAuthErrorCookie.ts +++ /dev/null @@ -1,55 +0,0 @@ -/* - * Copyright 2024 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 { Response } from 'express'; -import { serializeError } from '@backstage/errors'; - -const ONE_MINUTE_MS = 60 * 1000; - -const AUTH_ERROR_COOKIE = 'auth-error'; - -function configureAuthErrorCookie(apiUrl: string, appOrigin: string) { - const { hostname: domain, pathname: path, protocol } = new URL(apiUrl); - const secure = protocol === 'https:'; - - // For situations where the auth-backend is running on a - // different domain than the app, we set the SameSite attribute - // to 'none' to allow third-party access to the cookie, but - // only if it's in a secure context (https). - let sameSite: 'lax' | 'none' = 'lax'; - if (new URL(appOrigin).hostname !== domain && secure) { - sameSite = 'none'; - } - - return { domain, path, secure, sameSite }; -} - -export function createAuthErrorCookie( - res: Response, - origin: string, - options: { - error: Error; - apiUrl: string; - }, -) { - const { error, apiUrl } = options; - const jsonData = serializeError(error); - - res.cookie(AUTH_ERROR_COOKIE, jsonData, { - maxAge: ONE_MINUTE_MS, - httpOnly: true, - ...configureAuthErrorCookie(apiUrl, origin), - }); -} diff --git a/plugins/auth-node/src/oauth/createOAuthRouteHandlers.test.ts b/plugins/auth-node/src/oauth/createOAuthRouteHandlers.test.ts index b54de50881..e2155df444 100644 --- a/plugins/auth-node/src/oauth/createOAuthRouteHandlers.test.ts +++ b/plugins/auth-node/src/oauth/createOAuthRouteHandlers.test.ts @@ -743,7 +743,7 @@ describe('createOAuthRouteHandlers', () => { }); }); - it('should set cookie and redirect on caught error', async () => { + it('should set error search param and redirect on caught error', async () => { const app = wrapInApp(createOAuthRouteHandlers(baseConfig)); const res = await request(app) .get('/my-provider/handler/frame') @@ -756,15 +756,21 @@ describe('createOAuthRouteHandlers', () => { }), }); - // redirects on error with auth error cookie + // Check if it redirects on error expect(res.status).toBe(302); - const setCookieHeader = res.header['set-cookie']; - expect(setCookieHeader).toBeDefined(); - const authErrorCookie = setCookieHeader.find((cookie: string) => - cookie.startsWith('auth-error='), + // Extract the redirect URL from the location header + const redirectLocation = res.header.location; + expect(redirectLocation).toBeDefined(); + + // Create a URL object from the redirect location + const redirectUrl = new URL(redirectLocation); + + // Verify that the 'error' search param is set with the encoded error message + const errorMessage = redirectUrl.searchParams.get('error'); + expect(errorMessage).toBe( + encodeURIComponent('Auth response is missing cookie nonce'), ); - expect(authErrorCookie).toBeDefined(); }); }); }); diff --git a/plugins/auth-node/src/oauth/createOAuthRouteHandlers.ts b/plugins/auth-node/src/oauth/createOAuthRouteHandlers.ts index 847dc4844f..46f8fc3eca 100644 --- a/plugins/auth-node/src/oauth/createOAuthRouteHandlers.ts +++ b/plugins/auth-node/src/oauth/createOAuthRouteHandlers.ts @@ -42,7 +42,6 @@ import { import { OAuthAuthenticator, OAuthAuthenticatorResult } from './types'; import { Config } from '@backstage/config'; import { CookieScopeManager } from './CookieScopeManager'; -import { createAuthErrorCookie } from './createAuthErrorCookie'; /** @public */ export interface OAuthRouteHandlersOptions { @@ -252,13 +251,8 @@ export function createOAuthRouteHandlers( : new Error('Encountered invalid error'); // Being a bit safe and not forwarding the bad value if (state?.flow === 'redirect' && state?.redirectUrl) { - createAuthErrorCookie(res, state?.redirectUrl, { - error: { name, message }, - apiUrl: `${baseUrl}/.backstage/error`, - }); - const redirectUrl = new URL(state.redirectUrl); - redirectUrl.searchParams.set('error', 'true'); + redirectUrl.searchParams.set('error', encodeURIComponent(message)); // set the error in a cookie and redirect user back to sign in where the error can be rendered res.redirect(redirectUrl.toString()); diff --git a/plugins/auth-react/api-report.md b/plugins/auth-react/api-report.md index 52e6c6bc3a..d5206261d2 100644 --- a/plugins/auth-react/api-report.md +++ b/plugins/auth-react/api-report.md @@ -36,10 +36,4 @@ export function useCookieAuthRefresh(options: { pluginId: string }): expiresAt: string; }; }; - -// @public -export function useSignInAuthError(): { - error: Error | undefined; - checkAuthError: () => void; -}; ``` diff --git a/plugins/auth-react/src/hooks/index.ts b/plugins/auth-react/src/hooks/index.ts index 0e87bab67f..1257334498 100644 --- a/plugins/auth-react/src/hooks/index.ts +++ b/plugins/auth-react/src/hooks/index.ts @@ -18,4 +18,3 @@ // which hooks are public API and should be exported from the package. export * from './useCookieAuthRefresh'; -export * from './useSignInAuthError'; diff --git a/plugins/auth-react/src/hooks/useSignInAuthError/index.tsx b/plugins/auth-react/src/hooks/useSignInAuthError/index.tsx deleted file mode 100644 index 549b5b9f1f..0000000000 --- a/plugins/auth-react/src/hooks/useSignInAuthError/index.tsx +++ /dev/null @@ -1,17 +0,0 @@ -/* - * Copyright 2024 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. - */ - -export { useSignInAuthError } from './useSignInAuthError'; diff --git a/plugins/auth-react/src/hooks/useSignInAuthError/useSignInAuthError.test.tsx b/plugins/auth-react/src/hooks/useSignInAuthError/useSignInAuthError.test.tsx deleted file mode 100644 index 0c67c419f8..0000000000 --- a/plugins/auth-react/src/hooks/useSignInAuthError/useSignInAuthError.test.tsx +++ /dev/null @@ -1,98 +0,0 @@ -/* - * Copyright 2024 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 React from 'react'; -import { act, renderHook } from '@testing-library/react'; -import { discoveryApiRef } from '@backstage/core-plugin-api'; -import { TestApiProvider } from '@backstage/test-utils'; -import { useSignInAuthError } from './useSignInAuthError'; -import { serializeError } from '@backstage/errors'; - -describe('useSignInAuthError', () => { - const discoveryApiMock = { - getBaseUrl: jest.fn().mockResolvedValue('http://localhost:7000/api/auth'), - }; - - beforeEach(() => { - jest.clearAllMocks(); - }); - - it('should return an error when the cookie returns an error object', async () => { - const errorObject = { - name: 'TestError', - message: 'This is a test error', - }; - const serializedError = serializeError(errorObject); - - global.fetch = jest.fn().mockResolvedValue({ - ok: true, - json: jest.fn().mockResolvedValue(serializedError), - } as unknown as Response); - - const { result } = renderHook(() => useSignInAuthError(), { - wrapper: ({ children }) => ( - - {children} - - ), - }); - - await act(async () => { - result.current.checkAuthError(); - }); - - expect(discoveryApiMock.getBaseUrl).toHaveBeenCalledWith('auth'); - expect(fetch).toHaveBeenCalledWith( - 'http://localhost:7000/api/auth/.backstage/error', - { - credentials: 'include', - }, - ); - expect(result.current.error).toBeInstanceOf(Error); - expect((result.current.error as Error).name).toEqual(errorObject.name); - expect((result.current.error as Error).message).toEqual( - errorObject.message, - ); - }); - - it('should return undefined when the backend does not return an error object', async () => { - global.fetch = jest.fn().mockResolvedValue({ - ok: true, - json: jest.fn().mockResolvedValue(undefined), - } as unknown as Response); - - const { result } = renderHook(() => useSignInAuthError(), { - wrapper: ({ children }) => ( - - {children} - - ), - }); - - await act(async () => { - result.current.checkAuthError(); - }); - - expect(discoveryApiMock.getBaseUrl).toHaveBeenCalledWith('auth'); - expect(fetch).toHaveBeenCalledWith( - 'http://localhost:7000/api/auth/.backstage/error', - { - credentials: 'include', - }, - ); - expect(result.current.error).toBeUndefined(); - }); -}); diff --git a/plugins/auth-react/src/hooks/useSignInAuthError/useSignInAuthError.tsx b/plugins/auth-react/src/hooks/useSignInAuthError/useSignInAuthError.tsx deleted file mode 100644 index fe146fedca..0000000000 --- a/plugins/auth-react/src/hooks/useSignInAuthError/useSignInAuthError.tsx +++ /dev/null @@ -1,45 +0,0 @@ -/* - * Copyright 2024 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 { discoveryApiRef, useApi } from '@backstage/core-plugin-api'; -import { useAsync } from '@react-hookz/web'; -import { deserializeError } from '@backstage/errors'; - -/** - * @public - * A hook that will fetch any sign in auth error in redirect auth flow - */ -export function useSignInAuthError(): { - error: Error | undefined; - checkAuthError: () => void; -} { - const discoveryApi = useApi(discoveryApiRef); - - const [state, { execute: checkAuthError }] = useAsync(async () => { - const baseUrl = await discoveryApi.getBaseUrl('auth'); - - // use native fetch instead of fetchApi because - // we are not signed in and are calling an unauthenticated endpoint - const response = await fetch(`${baseUrl}/.backstage/error`, { - credentials: 'include', - }); - const data = await response.json(); - - return data ? deserializeError(data) : undefined; - }); - - return { error: state.result, checkAuthError }; -} diff --git a/yarn.lock b/yarn.lock index dfd1cffd9e..155c0eeb9e 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4287,7 +4287,6 @@ __metadata: "@backstage/core-app-api": "workspace:^" "@backstage/core-plugin-api": "workspace:^" "@backstage/errors": "workspace:^" - "@backstage/plugin-auth-react": "workspace:^" "@backstage/test-utils": "workspace:^" "@backstage/theme": "workspace:^" "@backstage/version-bridge": "workspace:^" From 5e5e4a850c1a779012b9644181cc805655964ee1 Mon Sep 17 00:00:00 2001 From: Stephen Glass Date: Tue, 8 Oct 2024 09:25:02 -0400 Subject: [PATCH 28/28] fix redirect error encoding Signed-off-by: Stephen Glass --- .../core-components/src/layout/SignInPage/SignInPage.tsx | 2 +- packages/core-components/src/layout/SignInPage/providers.tsx | 5 +---- plugins/auth-node/src/oauth/createOAuthRouteHandlers.test.ts | 4 +--- plugins/auth-node/src/oauth/createOAuthRouteHandlers.ts | 2 +- 4 files changed, 4 insertions(+), 9 deletions(-) diff --git a/packages/core-components/src/layout/SignInPage/SignInPage.tsx b/packages/core-components/src/layout/SignInPage/SignInPage.tsx index 8e1249f83e..c3139f9316 100644 --- a/packages/core-components/src/layout/SignInPage/SignInPage.tsx +++ b/packages/core-components/src/layout/SignInPage/SignInPage.tsx @@ -167,7 +167,7 @@ export const SingleSignInPage = ({ useMountEffect(() => { if (errorParam) { - setError(new Error(decodeURIComponent(errorParam))); + setError(new Error(errorParam)); } login({ checkExisting: true }); }); diff --git a/packages/core-components/src/layout/SignInPage/providers.tsx b/packages/core-components/src/layout/SignInPage/providers.tsx index 9cb2fa3c9f..c1c3b744c1 100644 --- a/packages/core-components/src/layout/SignInPage/providers.tsx +++ b/packages/core-components/src/layout/SignInPage/providers.tsx @@ -101,10 +101,7 @@ export const useSignInProviders = ( const errorParam = searchParams.get('error'); if (errorParam) { errorApi.post( - new ForwardedError(t('signIn.loginFailed'), { - name: 'Error', - message: decodeURIComponent(errorParam), - }), + new ForwardedError(t('signIn.loginFailed'), new Error(errorParam)), ); } }); diff --git a/plugins/auth-node/src/oauth/createOAuthRouteHandlers.test.ts b/plugins/auth-node/src/oauth/createOAuthRouteHandlers.test.ts index e2155df444..edf816cbee 100644 --- a/plugins/auth-node/src/oauth/createOAuthRouteHandlers.test.ts +++ b/plugins/auth-node/src/oauth/createOAuthRouteHandlers.test.ts @@ -768,9 +768,7 @@ describe('createOAuthRouteHandlers', () => { // Verify that the 'error' search param is set with the encoded error message const errorMessage = redirectUrl.searchParams.get('error'); - expect(errorMessage).toBe( - encodeURIComponent('Auth response is missing cookie nonce'), - ); + expect(errorMessage).toBe('Auth response is missing cookie nonce'); }); }); }); diff --git a/plugins/auth-node/src/oauth/createOAuthRouteHandlers.ts b/plugins/auth-node/src/oauth/createOAuthRouteHandlers.ts index 46f8fc3eca..53ae4101d3 100644 --- a/plugins/auth-node/src/oauth/createOAuthRouteHandlers.ts +++ b/plugins/auth-node/src/oauth/createOAuthRouteHandlers.ts @@ -252,7 +252,7 @@ export function createOAuthRouteHandlers( if (state?.flow === 'redirect' && state?.redirectUrl) { const redirectUrl = new URL(state.redirectUrl); - redirectUrl.searchParams.set('error', encodeURIComponent(message)); + redirectUrl.searchParams.set('error', message); // set the error in a cookie and redirect user back to sign in where the error can be rendered res.redirect(redirectUrl.toString());