OIDC: add friendly errors (#11184)

* add delegatedauthentication to validated server config * dynamic client registration functions * test OP registration functions * add stubbed nativeOidc flow setup in Login * cover more error cases in Login * tidy * test dynamic client registration in Login * comment oidc_static_clients * register oidc inside Login.getFlows * strict fixes * remove unused code * and imports * comments * comments 2 * util functions to get static client id * check static client ids in login flow * remove dead code * OidcRegistrationClientMetadata type * navigate to oidc authorize url * exchange code for token * navigate to oidc authorize url * navigate to oidc authorize url * test * adjust for js-sdk code * login with oidc native flow: messy version * tidy * update test for response_mode query * tidy up some TODOs * use new types * add identityServerUrl to stored params * unit test completeOidcLogin * test tokenlogin * strict * whitespace * tidy * unit test oidc login flow in MatrixChat * strict * tidy * extract success/failure handlers from token login function * typo * use for no homeserver error dialog too * reuse post-token login functions, test * shuffle testing utils around * shuffle testing utils around * i18n * tidy * Update src/Lifecycle.ts Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * tidy * comment * update tests for id token validation * move try again responsibility * prettier * add friendly error messages for oidc authorization failures * i18n * update for new translations, tidy --------- Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
matrix-org · Oct 19, 2023 · 1d9c24e · 1d9c24e
1 parent eb7ce66
commit 1d9c24e
Show file tree

Hide file tree

Showing 6 changed files with 84 additions and 11 deletions.
diff --git a/src/Lifecycle.ts b/src/Lifecycle.ts
@@ -65,6 +65,7 @@ import { OverwriteLoginPayload } from "./dispatcher/payloads/OverwriteLoginPaylo
 import { SdkContextClass } from "./contexts/SDKContext";
 import { messageForLoginError } from "./utils/ErrorUtils";
 import { completeOidcLogin } from "./utils/oidc/authorize";
+import { getOidcErrorMessage } from "./utils/oidc/error";
 import { OidcClientStore } from "./stores/oidc/OidcClientStore";
 import {
     getStoredOidcClientId,
@@ -306,8 +307,7 @@ async function attemptOidcNativeLogin(queryParams: QueryDict): Promise<boolean>
     } catch (error) {
         logger.error("Failed to login via OIDC", error);
 
-        // TODO(kerrya) nice error messages https://github.com/vector-im/element-web/issues/25665
-        await onFailedDelegatedAuthLogin(_t("auth|oidc|error_generic"));
+        await onFailedDelegatedAuthLogin(getOidcErrorMessage(error as Error));
         return false;
     }
 }

diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json
@@ -228,9 +228,10 @@
         "msisdn_field_required_invalid": "Enter phone number",
         "no_hs_url_provided": "No homeserver URL provided",
         "oidc": {
-            "error_generic": "Something went wrong.",
             "error_title": "We couldn't log you in",
-            "logout_redirect_warning": "You will be redirected to your server's authentication provider to complete sign out."
+            "generic_auth_error": "Something went wrong during authentication. Go to the sign in page and try again.",
+            "logout_redirect_warning": "You will be redirected to your server's authentication provider to complete sign out.",
+            "missing_or_invalid_stored_state": "We asked the browser to remember which homeserver you use to let you sign in, but unfortunately your browser has forgotten it. Go to the sign in page and try again."
         },
         "password_field_keep_going_prompt": "Keep going…",
         "password_field_label": "Enter password",

diff --git a/src/utils/oidc/authorize.ts b/src/utils/oidc/authorize.ts
@@ -20,6 +20,8 @@ import { OidcClientConfig } from "matrix-js-sdk/src/matrix";
 import { randomString } from "matrix-js-sdk/src/randomstring";
 import { IdTokenClaims } from "oidc-client-ts";
 
+import { OidcClientError } from "./error";
+
 /**
  * Start OIDC authorization code flow
  * Generates auth params, stores them in session storage and
@@ -68,7 +70,7 @@ const getCodeAndStateFromQueryParams = (queryParams: QueryDict): { code: string;
     const state = queryParams["state"];
 
     if (!code || typeof code !== "string" || !state || typeof state !== "string") {
-        throw new Error("Invalid query parameters for OIDC native login. `code` and `state` are required.");
+        throw new Error(OidcClientError.InvalidQueryParameters);
     }
     return { code, state };
 };

diff --git a/src/utils/oidc/error.ts b/src/utils/oidc/error.ts
@@ -0,0 +1,47 @@
+/*
+Copyright 2023 The Matrix.org Foundation C.I.C.
+
+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 { ReactNode } from "react";
+import { OidcError } from "matrix-js-sdk/src/oidc/error";
+
+import { _t } from "../../languageHandler";
+
+/**
+ * Errors thrown by EW during OIDC native flow authentication.
+ * Intended to be logged, not read by users.
+ */
+export enum OidcClientError {
+    InvalidQueryParameters = "Invalid query parameters for OIDC native login. `code` and `state` are required.",
+}
+
+/**
+ * Get a friendly translated error message for user consumption
+ * based on error encountered during authentication
+ * @param error
+ * @returns a friendly translated error message for user consumption
+ */
+export const getOidcErrorMessage = (error: Error): string | ReactNode => {
+    switch (error.message) {
+        case OidcError.MissingOrInvalidStoredState:
+            return _t("auth|oidc|missing_or_invalid_stored_state");
+        case OidcClientError.InvalidQueryParameters:
+        case OidcError.CodeExchangeFailed:
+        case OidcError.InvalidBearerTokenResponse:
+        case OidcError.InvalidIdToken:
+        default:
+            return _t("auth|oidc|generic_auth_error");
+    }
+};
diff --git a/test/components/structures/MatrixChat-test.tsx b/test/components/structures/MatrixChat-test.tsx
@@ -44,6 +44,7 @@ import {
     unmockClientPeg,
 } from "../../test-utils";
 import * as leaveRoomUtils from "../../../src/utils/leave-behaviour";
+import { OidcClientError } from "../../../src/utils/oidc/error";
 import * as voiceBroadcastUtils from "../../../src/voice-broadcast/utils/cleanUpBroadcasts";
 import LegacyCallHandler from "../../../src/LegacyCallHandler";
 import { CallStore } from "../../../src/stores/CallStore";
@@ -915,10 +916,13 @@ describe("<MatrixChat />", () => {
 
         let loginClient!: ReturnType<typeof getMockClientWithEventEmitter>;
 
-        // for now when OIDC fails for any reason we just bump back to welcome
-        // error handling screens in https://github.com/vector-im/element-web/issues/25665
-        const expectOIDCError = async (): Promise<void> => {
+        const expectOIDCError = async (
+            errorMessage = "Something went wrong during authentication. Go to the sign in page and try again.",
+        ): Promise<void> => {
             await flushPromises();
+            const dialog = await screen.findByRole("dialog");
+
+            expect(within(dialog).getByText(errorMessage)).toBeInTheDocument();
             // just check we're back on welcome page
             expect(document.querySelector(".mx_Welcome")!).toBeInTheDocument();
         };
@@ -972,7 +976,7 @@ describe("<MatrixChat />", () => {
 
             expect(logger.error).toHaveBeenCalledWith(
                 "Failed to login via OIDC",
-                new Error("Invalid query parameters for OIDC native login. `code` and `state` are required."),
+                new Error(OidcClientError.InvalidQueryParameters),
             );
 
             await expectOIDCError();
@@ -1027,6 +1031,24 @@ describe("<MatrixChat />", () => {
                 mocked(completeAuthorizationCodeGrant).mockRejectedValue(new Error(OidcError.CodeExchangeFailed));
             });
 
+            it("should log and return to welcome page with correct error when login state is not found", async () => {
+                mocked(completeAuthorizationCodeGrant).mockRejectedValue(
+                    new Error(OidcError.MissingOrInvalidStoredState),
+                );
+                getComponent({ realQueryParams });
+
+                await flushPromises();
+
+                expect(logger.error).toHaveBeenCalledWith(
+                    "Failed to login via OIDC",
+                    new Error(OidcError.MissingOrInvalidStoredState),
+                );
+
+                await expectOIDCError(
+                    "We asked the browser to remember which homeserver you use to let you sign in, but unfortunately your browser has forgotten it. Go to the sign in page and try again.",
+                );
+            });
+
             it("should log and return to welcome page", async () => {
                 getComponent({ realQueryParams });
 

diff --git a/test/utils/oidc/authorize-test.ts b/test/utils/oidc/authorize-test.ts
@@ -22,6 +22,7 @@ import { mocked } from "jest-mock";
 
 import { completeOidcLogin, startOidcLogin } from "../../../src/utils/oidc/authorize";
 import { makeDelegatedAuthConfig } from "../../test-utils/oidc";
+import { OidcClientError } from "../../../src/utils/oidc/error";
 
 jest.unmock("matrix-js-sdk/src/randomstring");
 
@@ -125,8 +126,8 @@ describe("OIDC authorization", () => {
         });
 
         it("should throw when query params do not include state and code", async () => {
-            await expect(completeOidcLogin({})).rejects.toThrow(
-                "Invalid query parameters for OIDC native login. `code` and `state` are required.",
+            await expect(async () => await completeOidcLogin({})).rejects.toThrow(
+                OidcClientError.InvalidQueryParameters,
             );
         });