From 41f0c6f982164cfe59a2ca9a8cd49145f01f9034 Mon Sep 17 00:00:00 2001 From: arnevogt Date: Thu, 26 Sep 2024 11:31:14 +0200 Subject: [PATCH] introduce error state for authentication (#47, #66) Co-authored-by: Michael Beckemeyer --- .changeset/dry-apes-sparkle.md | 41 ++++++++++ .changeset/moody-panthers-count.md | 5 ++ pnpm-lock.yaml | 3 + .../KeycloakAuthPlugin.ts | 9 +- .../authentication/AuthServiceImpl.test.ts | 5 ++ .../authentication/ForceAuth.test.tsx | 82 ++++++++++++++++++- src/packages/authentication/ForceAuth.tsx | 68 +++++++++++++++ src/packages/authentication/api.ts | 15 +++- src/packages/authentication/build.config.mjs | 1 + src/packages/authentication/i18n/de.yaml | 2 + src/packages/authentication/i18n/en.yaml | 2 + src/packages/authentication/package.json | 1 + src/packages/local-storage/README.md | 4 +- src/samples/auth-sample/auth-app/AppUI.tsx | 17 +++- .../auth-app/auth-plugin/LoginMask.tsx | 11 ++- .../auth-app/auth-plugin/TestAuthPlugin.ts | 9 ++ .../auth-sample/auth-app/build.config.mjs | 1 + src/samples/auth-sample/auth-app/i18n/de.yaml | 0 src/samples/auth-sample/auth-app/i18n/en.yaml | 0 src/samples/keycloak-sample/AppUI.tsx | 10 ++- 20 files changed, 270 insertions(+), 16 deletions(-) create mode 100644 .changeset/dry-apes-sparkle.md create mode 100644 .changeset/moody-panthers-count.md create mode 100644 src/packages/authentication/i18n/de.yaml create mode 100644 src/packages/authentication/i18n/en.yaml create mode 100644 src/samples/auth-sample/auth-app/i18n/de.yaml create mode 100644 src/samples/auth-sample/auth-app/i18n/en.yaml diff --git a/.changeset/dry-apes-sparkle.md b/.changeset/dry-apes-sparkle.md new file mode 100644 index 00000000..3260df08 --- /dev/null +++ b/.changeset/dry-apes-sparkle.md @@ -0,0 +1,41 @@ +--- +"@open-pioneer/authentication": minor +--- + +Introduce new authentication state `AuthStateAuthenticationError` + +Error state is supposed to be used for errors that occur during the authentication process (e.g. lost connection to authentication backend) rather than for failed login attempts (e.g. invalid credentials) + +`ForceAuth` component provides two mechanisms to render a fallback component if an authentication error occurs. + +`errorFallback` option takes an abitrary react component that is rendered in case of an error. The error object can be accessed via the ErrorFallbackPros. + +```jsx + + App Content + + + function ErrorFallback(props: ErrorFallbackProps) { + return ( + <> + {props.error.message} + + ); + } +``` + +If additional inputs or state must be accessed from within the error fallback component the `renderErrorFallback` option should be used. + +```jsx +const userName = "user1"; + ( + <> + Could not login {userName} + {e.message} + + )}> + App Content + +``` + +The `renderErrorFallback` property takes precedence over the `errorFallback` property. diff --git a/.changeset/moody-panthers-count.md b/.changeset/moody-panthers-count.md new file mode 100644 index 00000000..d1f63ea6 --- /dev/null +++ b/.changeset/moody-panthers-count.md @@ -0,0 +1,5 @@ +--- +"@open-pioneer/authentication-keycloak": patch +--- + +Use error state to communicate keycloak exceptions diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index bd3d964c..cee18357 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -303,6 +303,9 @@ importers: src/packages/authentication: dependencies: + '@open-pioneer/chakra-integration': + specifier: workspace:^ + version: link:../chakra-integration '@open-pioneer/core': specifier: workspace:^ version: link:../core diff --git a/src/packages/authentication-keycloak/KeycloakAuthPlugin.ts b/src/packages/authentication-keycloak/KeycloakAuthPlugin.ts index be8fa473..a89c20e3 100644 --- a/src/packages/authentication-keycloak/KeycloakAuthPlugin.ts +++ b/src/packages/authentication-keycloak/KeycloakAuthPlugin.ts @@ -76,14 +76,9 @@ export class KeycloakAuthPlugin throw new Error("Failed to construct keycloak instance", { cause: e }); } this.#init().catch((e) => { - // TODO: Handle error - // - // Stay in pending state when an error happens. - // There is currently no useful way to signal an error using the plugin API, - // going into 'not-authenticated' could lead to unexpected behavior (e.g. redirect loops). - // See https://github.com/open-pioneer/trails-core-packages/issues/47 this.#updateState({ - kind: "pending" + kind: "error", + error: e }); this.#notifier.notify({ level: "error", diff --git a/src/packages/authentication/AuthServiceImpl.test.ts b/src/packages/authentication/AuthServiceImpl.test.ts index 6283f855..d0771477 100644 --- a/src/packages/authentication/AuthServiceImpl.test.ts +++ b/src/packages/authentication/AuthServiceImpl.test.ts @@ -31,6 +31,7 @@ it("forwards the authentication plugin's state changes", async () => { } }); plugin.$setAuthState({ kind: "not-authenticated" }); + plugin.$setAuthState({ kind: "error", error: new Error("server error") }); expect(observedStates).toMatchInlineSnapshot(` [ @@ -49,6 +50,10 @@ it("forwards the authentication plugin's state changes", async () => { { "kind": "not-authenticated", }, + { + "error": [Error: server error], + "kind": "error", + }, ] `); }); diff --git a/src/packages/authentication/ForceAuth.test.tsx b/src/packages/authentication/ForceAuth.test.tsx index 643ea635..d6bb10c2 100644 --- a/src/packages/authentication/ForceAuth.test.tsx +++ b/src/packages/authentication/ForceAuth.test.tsx @@ -4,8 +4,9 @@ import { EventEmitter } from "@open-pioneer/core"; import { PackageContextProvider } from "@open-pioneer/test-utils/react"; import { act, render, screen, waitFor } from "@testing-library/react"; import { expect, it } from "vitest"; -import { ForceAuth } from "./ForceAuth"; +import { ErrorFallbackProps, ForceAuth } from "./ForceAuth"; import { AuthEvents, AuthService, AuthState, LoginBehavior, SessionInfo } from "./api"; +import { Box } from "@open-pioneer/chakra-integration"; it("renders children if the user is authenticated", async () => { const mocks = { @@ -203,6 +204,85 @@ it("calls a login effect if present", async () => { }); }); +it("renders the error fallback if authentication state is erroneous", async () => { + const error = new Error("authentication failed"); + const mocks = { + services: { + "authentication.AuthService": new TestAuthService({ + kind: "error", + error: error + }) + } + }; + + function ErrorFallback(props: ErrorFallbackProps) { + return {props.error.message}; + } + + render( + + + + ); + + const result = await screen.findByTestId("ErrorFallback-box"); + expect(result.innerHTML).toEqual(error.message); +}); + +it("uses the renderErrorFallback property if authentication state is erroneous", async () => { + const testInput = "test input"; + const mocks = { + services: { + "authentication.AuthService": new TestAuthService({ + kind: "error", + error: new Error("authentication failed") + }) + } + }; + + render( + + {testInput}} + > + + ); + + const result = await screen.findByTestId("ErrorFallback-box"); + expect(result.innerHTML).toEqual(testInput); +}); + +it("should use renderErrorFallback property rather than errorFallback property if both are provided", async () => { + const renderErrorFallbackInner = "renderErrorFallback"; + const errorFallbackInner = "errorFallback"; + const mocks = { + services: { + "authentication.AuthService": new TestAuthService({ + kind: "error", + error: new Error("authentication failed") + }) + } + }; + + function ErrorFallback() { + return {errorFallbackInner}; + } + + render( + + ( + {renderErrorFallbackInner} + )} + > + + ); + + const result = await screen.findByTestId("ErrorFallback-box"); + expect(result.innerHTML).toEqual(renderErrorFallbackInner); +}); + class TestAuthService extends EventEmitter implements AuthService { #currentState: AuthState; #behavior: LoginBehavior; diff --git a/src/packages/authentication/ForceAuth.tsx b/src/packages/authentication/ForceAuth.tsx index fa8f26e1..e12f0ef1 100644 --- a/src/packages/authentication/ForceAuth.tsx +++ b/src/packages/authentication/ForceAuth.tsx @@ -9,6 +9,8 @@ import { AuthService } from "./api"; import { useAuthState } from "./useAuthState"; +import { Box } from "@open-pioneer/chakra-integration"; +import { useIntl } from "open-pioneer:react-hooks"; /** * Properties for the ForceAuth component. @@ -48,10 +50,63 @@ export interface ForceAuthProps { */ renderFallback?: (AuthFallback: ComponentType>) => ReactNode; + /** + * This component is rendered as fallback if an error occurs during authentication (e.g authentication backend is not available). + * The actual error that occured is accesible from within the fallback component via {@link ErrorFallbackProps} + * + * Example: + * + * ```jsx + * + * App Content + * + * + * function ErrorFallback(props: ErrorFallbackProps) { + * return ( + * <> + * {props.error.message} + * + * ); + * } + * ``` + */ + errorFallback?: ComponentType; + + /** + * This property can be used to customize rendering of the error fallback. + * The `renderErrorFallback` should be used if inputs other than {@link ErrorFallbackProps} are to be used in the error fallback. + * + * NOTE: `renderErrorFallback` takes precedence before {@link errorFallback}. + * + * Example: + * + * ```jsx + * const userName = "user1"; + * ( + * <> + * Could not login {userName} + * {e.message} + * + * )}> + * App Content + * + * ``` + * + * @param error the error that occured during authentication + */ + renderErrorFallback?: (error: Error) => ReactNode; + /** The children are rendered if the current user is authenticated. */ children?: ReactNode; } +/** + * `ErrorFallbackProps` properties indicate the error that happened in the authentication process. + */ +export interface ErrorFallbackProps { + error: Error; +} + /** * `ForceAuth` renders its children if the current user is authenticated. * If the user is not authenticated, a `AuthFallback` will be presented to the user. @@ -77,6 +132,7 @@ export interface ForceAuthProps { export const ForceAuth: FC = (props) => { const authService = useService("authentication.AuthService"); const state = useAuthState(authService); + const intl = useIntl(); // Extract login behavior from service (only when needed). const behavior = useMemo(() => { @@ -106,6 +162,18 @@ export const ForceAuth: FC = (props) => { } return ; } + case "error": + if (props.renderErrorFallback) { + return props.renderErrorFallback(state.error); + } else if (props.errorFallback) { + return ; + } else { + return ( + + {intl.formatMessage({ id: "auth-error" })} + + ); + } case "authenticated": return <>{props.children}; } diff --git a/src/packages/authentication/api.ts b/src/packages/authentication/api.ts index 440c8fe7..2d88511d 100644 --- a/src/packages/authentication/api.ts +++ b/src/packages/authentication/api.ts @@ -38,7 +38,11 @@ export interface SessionInfo { * NOTE: Future versions of this package may define additional states. * Your code should contain sensible fallback or error logic. */ -export type AuthState = AuthStatePending | AuthStateNotAuthenticated | AuthStateAuthenticated; +export type AuthState = + | AuthStatePending + | AuthStateNotAuthenticated + | AuthStateAuthenticated + | AuthStateAuthenticationError; /** * This state is active when the authentication service @@ -55,6 +59,15 @@ export interface AuthStateNotAuthenticated { kind: "not-authenticated"; } +/** + * This state indicates an error during authentication. + * This state should used for errors in the authentication workflow (e.g. backend unavailable) rather than failed login attempts (e.g. invalid credentials). + */ +export interface AuthStateAuthenticationError { + kind: "error"; + error: Error; +} + /** * The user is authenticated and its session attributes * can be retrieved. diff --git a/src/packages/authentication/build.config.mjs b/src/packages/authentication/build.config.mjs index 0882eba7..3a6fc7e7 100644 --- a/src/packages/authentication/build.config.mjs +++ b/src/packages/authentication/build.config.mjs @@ -4,6 +4,7 @@ import { defineBuildConfig } from "@open-pioneer/build-support"; export default defineBuildConfig({ entryPoints: ["index"], + i18n: ["en", "de"], services: { AuthServiceImpl: { provides: "authentication.AuthService", diff --git a/src/packages/authentication/i18n/de.yaml b/src/packages/authentication/i18n/de.yaml new file mode 100644 index 00000000..ad113add --- /dev/null +++ b/src/packages/authentication/i18n/de.yaml @@ -0,0 +1,2 @@ +messages: + auth-error: "Bei der Authentifizierung ist ein Fehler aufgetreten." diff --git a/src/packages/authentication/i18n/en.yaml b/src/packages/authentication/i18n/en.yaml new file mode 100644 index 00000000..62d84c93 --- /dev/null +++ b/src/packages/authentication/i18n/en.yaml @@ -0,0 +1,2 @@ +messages: + auth-error: "An error occurred during authentication." diff --git a/src/packages/authentication/package.json b/src/packages/authentication/package.json index 230325cd..eb7d9132 100644 --- a/src/packages/authentication/package.json +++ b/src/packages/authentication/package.json @@ -19,6 +19,7 @@ "peerDependencies": { "@open-pioneer/core": "workspace:^", "@open-pioneer/runtime": "workspace:^", + "@open-pioneer/chakra-integration": "workspace:^", "react": "catalog:", "react-use": "catalog:" }, diff --git a/src/packages/local-storage/README.md b/src/packages/local-storage/README.md index 87ad44e0..de49cf3f 100644 --- a/src/packages/local-storage/README.md +++ b/src/packages/local-storage/README.md @@ -86,8 +86,8 @@ namespace.set("my-state", "some-value-to-save"); ### Configuration -| Name | Type | Description | -| ----------- | ------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Name | Type | Description | +| ----------- | ------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | | `storageId` | String | The key under which the persistent data is saved. This value should be configured to a reasonably unique value to avoid clashes with other applications at the same origin. Defaults to `trails-state` (with a warning). | ### Implementation notes diff --git a/src/samples/auth-sample/auth-app/AppUI.tsx b/src/samples/auth-sample/auth-app/AppUI.tsx index 22a7f253..aecc4aba 100644 --- a/src/samples/auth-sample/auth-app/AppUI.tsx +++ b/src/samples/auth-sample/auth-app/AppUI.tsx @@ -1,12 +1,12 @@ // SPDX-FileCopyrightText: 2023 Open Pioneer project (https://github.com/open-pioneer) // SPDX-License-Identifier: Apache-2.0 import { ForceAuth } from "@open-pioneer/authentication"; -import { Container, Flex, Heading } from "@open-pioneer/chakra-integration"; +import { Box, Button, Container, Flex, Heading } from "@open-pioneer/chakra-integration"; import { LogoutButton } from "./LogoutButton"; export function AppUI() { return ( - + Authenticated This is the actual content of the app. Authentication was successful. @@ -17,3 +17,16 @@ export function AppUI() { ); } + +export function ErrorFallback(props: { error: Error }) { + return ( + <> + + {props.error.message} + + + + ); +} diff --git a/src/samples/auth-sample/auth-app/auth-plugin/LoginMask.tsx b/src/samples/auth-sample/auth-app/auth-plugin/LoginMask.tsx index 200eb512..53b2f194 100644 --- a/src/samples/auth-sample/auth-app/auth-plugin/LoginMask.tsx +++ b/src/samples/auth-sample/auth-app/auth-plugin/LoginMask.tsx @@ -10,6 +10,7 @@ import { FormControl, FormLabel, Heading, + HStack, Input, InputGroup, InputRightElement, @@ -21,9 +22,10 @@ import { useState } from "react"; interface LoginMaskProps { wasLoggedIn: boolean; doLogin: (userName: string, password: string) => string | undefined; + doFail: () => void; } -export function LoginMask({ doLogin, wasLoggedIn }: LoginMaskProps) { +export function LoginMask({ doLogin, doFail, wasLoggedIn }: LoginMaskProps) { const [userName, setUserName] = useState(""); const [password, setPassword] = useState(""); const [showPassword, setShowPassword] = useState(false); @@ -94,7 +96,12 @@ export function LoginMask({ doLogin, wasLoggedIn }: LoginMaskProps) { - + + + + ); diff --git a/src/samples/auth-sample/auth-app/auth-plugin/TestAuthPlugin.ts b/src/samples/auth-sample/auth-app/auth-plugin/TestAuthPlugin.ts index 00409fb0..66e2631d 100644 --- a/src/samples/auth-sample/auth-app/auth-plugin/TestAuthPlugin.ts +++ b/src/samples/auth-sample/auth-app/auth-plugin/TestAuthPlugin.ts @@ -60,11 +60,20 @@ export class TestAuthPlugin extends EventEmitter implements Se } }; + const doFail = () => { + this.#state = { + kind: "error", + error: new Error("Login failed!") + }; + this.emit("changed"); + }; + // This component is rendered when the user is not logged in, for example // by the `` component. const AuthFallback = () => createElement(LoginMask, { doLogin: doLogin, + doFail: doFail, wasLoggedIn: this.#wasLoggedIn }); return { diff --git a/src/samples/auth-sample/auth-app/build.config.mjs b/src/samples/auth-sample/auth-app/build.config.mjs index a50250ca..7f369532 100644 --- a/src/samples/auth-sample/auth-app/build.config.mjs +++ b/src/samples/auth-sample/auth-app/build.config.mjs @@ -3,6 +3,7 @@ import { defineBuildConfig } from "@open-pioneer/build-support"; export default defineBuildConfig({ + i18n: ["en", "de"], services: { TestAuthPlugin: { provides: "authentication.AuthPlugin" diff --git a/src/samples/auth-sample/auth-app/i18n/de.yaml b/src/samples/auth-sample/auth-app/i18n/de.yaml new file mode 100644 index 00000000..e69de29b diff --git a/src/samples/auth-sample/auth-app/i18n/en.yaml b/src/samples/auth-sample/auth-app/i18n/en.yaml new file mode 100644 index 00000000..e69de29b diff --git a/src/samples/keycloak-sample/AppUI.tsx b/src/samples/keycloak-sample/AppUI.tsx index b4eff604..097a80af 100644 --- a/src/samples/keycloak-sample/AppUI.tsx +++ b/src/samples/keycloak-sample/AppUI.tsx @@ -2,6 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 import { AuthService, ForceAuth, useAuthState } from "@open-pioneer/authentication"; import { + Box, Button, Code, Container, @@ -78,7 +79,14 @@ export function AppUI() { - + ( + <> + An Error occured while trying to login! + {e.message} + + )} + >