OIDC: Log in (#3554)

* use oidc-client-ts during oidc discovery

* export new type for auth config

* deprecate generateAuthorizationUrl in favour of generateOidcAuthorizationUrl

* testing util for oidc configurations

* test generateOidcAuthorizationUrl

* lint

* test discovery

* dont pass whole client wellknown to oidc validation funcs

* add nonce

* use oidc-client-ts for oidc response

* validate user state and update tests

* use oidc-client-ts for code exchange

* use oidc-client-ts in completing auth grant

* use client userState for homeserver

* more comments

spec/unit/oidc/authorize.spec.ts

@@ -46,8 +46,12 @@ describe("oidc authorization", () => {
     const clientId = "xyz789";
     const baseUrl = "https://test.com";
 
+    // 14.03.2022 16:15
+    const now = 1647270879403;
+
     beforeAll(() => {
         jest.spyOn(logger, "warn");
+        jest.setSystemTime(now);
 
         fetchMock.get(delegatedAuthConfig.issuer + ".well-known/openid-configuration", mockOpenIdConfiguration());
     });
@@ -133,7 +137,8 @@ describe("oidc authorization", () => {
     });
 
     describe("completeAuthorizationCodeGrant", () => {
-        const codeVerifier = "abc123";
+        const homeserverUrl = "https://server.org/";
+        const identityServerUrl = "https://id.org/";
         const nonce = "test-nonce";
         const redirectUri = baseUrl;
         const code = "auth_code_xyz";
@@ -142,9 +147,11 @@ describe("oidc authorization", () => {
             access_token: "test_access_token",
             refresh_token: "test_refresh_token",
             id_token: "valid.id.token",
-            expires_in: 12345,
+            expires_in: 300,
         };
 
+        const metadata = mockOpenIdConfiguration();
+
         const validDecodedIdToken = {
             // nonce matches
             nonce,
@@ -153,131 +160,236 @@ describe("oidc authorization", () => {
             // audience is this client
             aud: clientId,
             // issuer matches
-            iss: delegatedAuthConfig.issuer,
+            iss: metadata.issuer,
+            sub: "123",
+        };
+
+        const mockSessionStorage = (state: Record<string, unknown>): void => {
+            jest.spyOn(sessionStorage.__proto__, "getItem").mockImplementation((key: unknown) => {
+                return state[key as string] ?? null;
+            });
+            jest.spyOn(sessionStorage.__proto__, "setItem").mockImplementation(
+                // @ts-ignore mock type
+                (key: string, value: unknown) => (state[key] = value),
+            );
+            jest.spyOn(sessionStorage.__proto__, "removeItem").mockImplementation((key: unknown) => {
+                const { [key as string]: value, ...newState } = state;
+                state = newState;
+                return value;
+            });
+        };
+
+        const getValueFromStorage = <T = string>(state: string, key: string): T => {
+            const storedState = window.sessionStorage.getItem(`mx_oidc_${state}`)!;
+            return JSON.parse(storedState)[key] as unknown as T;
+        };
+
+        /**
+         * These tests kind of integration test oidc auth, by using `generateOidcAuthorizationUrl` and mocked storage
+         * to mock the use case of initiating oidc auth, putting state in storage, redirecting to OP,
+         * then returning and using state to verfiy.
+         * Returns random state string used to access storage
+         * @param params
+         */
+        const setupState = async (params = {}): Promise<string> => {
+            const url = await generateOidcAuthorizationUrl({
+                metadata,
+                redirectUri,
+                clientId,
+                homeserverUrl,
+                identityServerUrl,
+                nonce,
+                ...params,
+            });
+
+            const state = new URL(url).searchParams.get("state")!;
+
+            // add the scope with correct deviceId to the mocked bearer token response
+            const scope = getValueFromStorage(state, "scope");
+            fetchMock.post(metadata.token_endpoint, {
+                status: 200,
+                headers: {
+                    "Content-Type": "application/json",
+                },
+                ...validBearerTokenResponse,
+                scope,
+            });
+
+            return state;
         };
 
         beforeEach(() => {
             fetchMock.mockClear();
             fetchMock.resetBehavior();
 
-            fetchMock.post(tokenEndpoint, {
+            fetchMock.get(`${metadata.issuer}.well-known/openid-configuration`, metadata);
+            fetchMock.get(`${metadata.issuer}jwks`, {
                 status: 200,
-                body: JSON.stringify(validBearerTokenResponse),
+                headers: {
+                    "Content-Type": "application/json",
+                },
+                keys: [],
             });
 
+            mockSessionStorage({});
+
             mocked(jwtDecode).mockReturnValue(validDecodedIdToken);
         });
 
         it("should make correct request to the token endpoint", async () => {
-            await completeAuthorizationCodeGrant(code, {
+            const state = await setupState();
-                clientId,
+            const codeVerifier = getValueFromStorage(state, "code_verifier");
-                codeVerifier,
+            await completeAuthorizationCodeGrant(code, state);
-                redirectUri,
-                delegatedAuthConfig,
-                nonce,
-            });
 
-            expect(fetchMock).toHaveBeenCalledWith(tokenEndpoint, {
+            expect(fetchMock).toHaveBeenCalledWith(
-                method: Method.Post,
+                metadata.token_endpoint,
-                headers: { "Content-Type": "application/x-www-form-urlencoded" },
+                expect.objectContaining({
-                body: `grant_type=authorization_code&client_id=${clientId}&code_verifier=${codeVerifier}&redirect_uri=https%3A%2F%2Ftest.com&code=${code}`,
+                    method: Method.Post,
-            });
+                    credentials: "same-origin",
+                    headers: {
+                        "Accept": "application/json",
+                        "Content-Type": "application/x-www-form-urlencoded",
+                    },
+                }),
+            );
+
+            // check body is correctly formed
+            const queryParams = fetchMock.mock.calls.find(([endpoint]) => endpoint === metadata.token_endpoint)![1]!
+                .body as URLSearchParams;
+            expect(queryParams.get("grant_type")).toEqual("authorization_code");
+            expect(queryParams.get("client_id")).toEqual(clientId);
+            expect(queryParams.get("code_verifier")).toEqual(codeVerifier);
+            expect(queryParams.get("redirect_uri")).toEqual(redirectUri);
+            expect(queryParams.get("code")).toEqual(code);
         });
 
         it("should return with valid bearer token", async () => {
-            const result = await completeAuthorizationCodeGrant(code, {
+            const state = await setupState();
-                clientId,
+            const scope = getValueFromStorage(state, "scope");
-                codeVerifier,
+            const result = await completeAuthorizationCodeGrant(code, state);
-                redirectUri,
-                delegatedAuthConfig,
-                nonce,
-            });
 
-            expect(result).toEqual(validBearerTokenResponse);
+            expect(result).toEqual({
+                homeserverUrl,
+                identityServerUrl,
+                oidcClientSettings: {
+                    clientId,
+                    issuer: metadata.issuer,
+                },
+                tokenResponse: {
+                    access_token: validBearerTokenResponse.access_token,
+                    id_token: validBearerTokenResponse.id_token,
+                    refresh_token: validBearerTokenResponse.refresh_token,
+                    token_type: validBearerTokenResponse.token_type,
+                    // this value is slightly unstable because it uses the clock
+                    expires_at: result.tokenResponse.expires_at,
+                    scope,
+                },
+            });
         });
 
         it("should return with valid bearer token where token_type is lowercase", async () => {
+            const state = await setupState();
+            const scope = getValueFromStorage(state, "scope");
             const tokenResponse = {
                 ...validBearerTokenResponse,
+                scope,
                 token_type: "bearer",
             };
             fetchMock.post(
                 tokenEndpoint,
                 {
-                    status: 200,
+                    headers: {
-                    body: JSON.stringify(tokenResponse),
+                        "Content-Type": "application/json",
+                    },
+                    ...tokenResponse,
                 },
                 { overwriteRoutes: true },
             );
 
-            const result = await completeAuthorizationCodeGrant(code, {
+            const result = await completeAuthorizationCodeGrant(code, state);
-                clientId,
+
-                codeVerifier,
+            expect(result).toEqual({
-                redirectUri,
+                homeserverUrl,
-                delegatedAuthConfig,
+                identityServerUrl,
-                nonce,
+                oidcClientSettings: {
+                    clientId,
+                    issuer: metadata.issuer,
+                },
+                // results in token that uses 'Bearer' token type
+                tokenResponse: {
+                    access_token: validBearerTokenResponse.access_token,
+                    id_token: validBearerTokenResponse.id_token,
+                    refresh_token: validBearerTokenResponse.refresh_token,
+                    token_type: "Bearer",
+                    // this value is slightly unstable because it uses the clock
+                    expires_at: result.tokenResponse.expires_at,
+                    scope,
+                },
             });
 
-            // results in token that uses 'Bearer' token type
+            expect(result.tokenResponse.token_type).toEqual("Bearer");
-            expect(result).toEqual(validBearerTokenResponse);
-            expect(result.token_type).toEqual("Bearer");
         });
 
-        it("should throw with code exchange failed error when request fails", async () => {
+        it("should throw when state is not found in storage", async () => {
+            // don't setup sessionStorage with expected state
+            const state = "abc123";
             fetchMock.post(
-                tokenEndpoint,
+                metadata.token_endpoint,
                 {
                     status: 500,
                 },
                 { overwriteRoutes: true },
             );
-            await expect(() =>
+            await expect(() => completeAuthorizationCodeGrant(code, state)).rejects.toThrow(
-                completeAuthorizationCodeGrant(code, {
+                new Error(OidcError.MissingOrInvalidStoredState),
-                    clientId,
+            );
-                    codeVerifier,
+        });
-                    redirectUri,
+
-                    delegatedAuthConfig,
+        it("should throw with code exchange failed error when request fails", async () => {
-                    nonce,
+            const state = await setupState();
-                }),
+            fetchMock.post(
-            ).rejects.toThrow(new Error(OidcError.CodeExchangeFailed));
+                metadata.token_endpoint,
+                {
+                    status: 500,
+                },
+                { overwriteRoutes: true },
+            );
+            await expect(() => completeAuthorizationCodeGrant(code, state)).rejects.toThrow(
+                new Error(OidcError.CodeExchangeFailed),
+            );
         });
 
         it("should throw invalid token error when token is invalid", async () => {
+            const state = await setupState();
             const invalidBearerTokenResponse = {
                 ...validBearerTokenResponse,
                 access_token: null,
             };
             fetchMock.post(
-                tokenEndpoint,
+                metadata.token_endpoint,
-                { status: 200, body: JSON.stringify(invalidBearerTokenResponse) },
+                {
+                    headers: {
+                        "Content-Type": "application/json",
+                    },
+                    ...invalidBearerTokenResponse,
+                },
                 { overwriteRoutes: true },
             );
-            await expect(() =>
+            await expect(() => completeAuthorizationCodeGrant(code, state)).rejects.toThrow(
-                completeAuthorizationCodeGrant(code, {
+                new Error(OidcError.InvalidBearerTokenResponse),
-                    clientId,
+            );
-                    codeVerifier,
-                    redirectUri,
-                    delegatedAuthConfig,
-                    nonce,
-                }),
-            ).rejects.toThrow(new Error(OidcError.InvalidBearerTokenResponse));
         });
 
         it("should throw invalid id token error when id_token is invalid", async () => {
+            const state = await setupState();
             mocked(jwtDecode).mockReturnValue({
                 ...validDecodedIdToken,
                 // invalid audience
                 aud: "something-else",
             });
-            await expect(() =>
+            await expect(() => completeAuthorizationCodeGrant(code, state)).rejects.toThrow(
-                completeAuthorizationCodeGrant(code, {
+                new Error(OidcError.InvalidIdToken),
-                    clientId,
+            );
-                    codeVerifier,
-                    redirectUri,
-                    delegatedAuthConfig,
-                    nonce,
-                }),
-            ).rejects.toThrow(new Error(OidcError.InvalidIdToken));
         });
     });
 });

spec/unit/oidc/validate.spec.ts

@@ -148,10 +148,8 @@ describe("validateOIDCIssuerWellKnown", () => {
                 response_types_supported: [],
             });
         }).toThrow(OidcError.OpSupport);
-        expect(logger.error).toHaveBeenCalledWith("OIDC issuer configuration: authorization_endpoint is invalid");
+        expect(logger.error).toHaveBeenCalledWith("Missing or invalid property: authorization_endpoint");
-        expect(logger.error).toHaveBeenCalledWith(
+        expect(logger.error).toHaveBeenCalledWith("Invalid property: response_types_supported. code is required.");
-            "OIDC issuer configuration: response_types_supported is invalid. code is required.",
-        );
     });
 
     it("should return validated issuer config", () => {

src/oidc/authorize.ts

@@ -14,15 +14,24 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-import { OidcClient, WebStorageStateStore } from "oidc-client-ts";
+import { Log, OidcClient, SigninResponse, SigninState, WebStorageStateStore } from "oidc-client-ts";
 
 import { IDelegatedAuthConfig } from "../client";
-import { Method } from "../http-api";
 import { subtleCrypto, TextEncoder } from "../crypto/crypto";
 import { logger } from "../logger";
 import { randomString } from "../randomstring";
 import { OidcError } from "./error";
-import { validateIdToken, ValidatedIssuerConfig, ValidatedIssuerMetadata, UserState } from "./validate";
+import {
+    validateIdToken,
+    ValidatedIssuerMetadata,
+    validateStoredUserState,
+    UserState,
+    BearerTokenResponse,
+    validateBearerTokenResponse,
+} from "./validate";
+
+// reexport for backwards compatibility
+export type { BearerTokenResponse };
 
 /**
  * Authorization parameters which are used in the authentication request of an OIDC auth code flow.
@@ -152,37 +161,6 @@ export const generateOidcAuthorizationUrl = async ({
     return request.url;
 };
 
-/**
- * The expected response type from the token endpoint during authorization code flow
- * Normalized to always use capitalized 'Bearer' for token_type
- *
- * See https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.4,
- * https://openid.net/specs/openid-connect-basic-1_0.html#TokenOK.
- */
-export type BearerTokenResponse = {
-    token_type: "Bearer";
-    access_token: string;
-    scope: string;
-    refresh_token?: string;
-    expires_in?: number;
-    id_token?: string;
-};
-
-/**
- * Expected response type from the token endpoint during authorization code flow
- * as it comes over the wire.
- * Should be normalized to use capital case 'Bearer' for token_type property
- *
- * See https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.4,
- * https://openid.net/specs/openid-connect-basic-1_0.html#TokenOK.
- */
-type WireBearerTokenResponse = BearerTokenResponse & {
-    token_type: "Bearer" | "bearer";
-};
-
-const isResponseObject = (response: unknown): response is Record<string, unknown> =>
-    !!response && typeof response === "object";
-
 /**
  * Normalize token_type to use capital case to make consuming the token response easier
  * token_type is case insensitive, and it is spec-compliant for OPs to return token_type: "bearer"
@@ -192,21 +170,18 @@ const isResponseObject = (response: unknown): response is Record<string, unknown
  * @param response - validated token response
  * @returns response with token_type set to 'Bearer'
  */
-const normalizeBearerTokenResponseTokenType = (response: WireBearerTokenResponse): BearerTokenResponse => ({
+const normalizeBearerTokenResponseTokenType = (response: SigninResponse): BearerTokenResponse =>
-    ...response,
+    ({
-    token_type: "Bearer",
+        id_token: response.id_token,
-});
+        scope: response.scope,
-
+        expires_at: response.expires_at,
-const isValidBearerTokenResponse = (response: unknown): response is WireBearerTokenResponse =>
+        refresh_token: response.refresh_token,
-    isResponseObject(response) &&
+        access_token: response.access_token,
-    typeof response["token_type"] === "string" &&
+        token_type: "Bearer",
-    // token_type is case insensitive, some OPs return `token_type: "bearer"`
+    } as BearerTokenResponse);
-    response["token_type"].toLowerCase() === "bearer" &&
-    typeof response["access_token"] === "string" &&
-    (!("refresh_token" in response) || typeof response["refresh_token"] === "string") &&
-    (!("expires_in" in response) || typeof response["expires_in"] === "number");
 
 /**
+ * @experimental
  * Attempt to exchange authorization code for bearer token.
  *
  * Takes the authorization code returned by the OpenID Provider via the authorization URL, and makes a
@@ -219,47 +194,71 @@ const isValidBearerTokenResponse = (response: unknown): response is WireBearerTo
  */
 export const completeAuthorizationCodeGrant = async (
     code: string,
-    {
+    state: string,
-        clientId,
+): Promise<{
-        codeVerifier,
+    oidcClientSettings: IDelegatedAuthConfig & { clientId: string };
-        redirectUri,
+    tokenResponse: BearerTokenResponse;
-        delegatedAuthConfig,
+    homeserverUrl: string;
-        nonce,
+    identityServerUrl?: string;
-    }: {
+}> => {
-        clientId: string;
+    /**
-        codeVerifier: string;
+     * Element Web strips and changes the url on starting the app
-        redirectUri: string;
+     * Use the code and state from query params to rebuild a url
-        delegatedAuthConfig: IDelegatedAuthConfig & ValidatedIssuerConfig;
+     * so that oidc-client can parse it
-        nonce: string;
+     */
-    },
+    const reconstructedUrl = new URL(window.location.origin);
-): Promise<BearerTokenResponse> => {
+    reconstructedUrl.searchParams.append("code", code);
-    const params = new URLSearchParams();
+    reconstructedUrl.searchParams.append("state", state);
-    params.append("grant_type", "authorization_code");
-    params.append("client_id", clientId);
-    params.append("code_verifier", codeVerifier);
-    params.append("redirect_uri", redirectUri);
-    params.append("code", code);
-    const metadata = params.toString();
 
-    const headers = { "Content-Type": "application/x-www-form-urlencoded" };
+    // set oidc-client to use our logger
+    Log.setLogger(logger);
+    try {
+        const response = new SigninResponse(reconstructedUrl.searchParams);
 
-    const response = await fetch(delegatedAuthConfig.tokenEndpoint, {
+        const stateStore = new WebStorageStateStore({ prefix: "mx_oidc_", store: window.sessionStorage });
-        method: Method.Post,
-        headers,
-        body: metadata,
-    });
 
-    if (response.status >= 400) {
+        // retrieve the state we put in storage at the start of oidc auth flow
+        const stateString = await stateStore.get(response.state!);
+        if (!stateString) {
+            throw new Error(OidcError.MissingOrInvalidStoredState);
+        }
+
+        // hydrate the sign in state and create a client
+        // the stored sign in state includes oidc configuration we set at the start of the oidc login flow
+        const signInState = SigninState.fromStorageString(stateString);
+        const client = new OidcClient({ ...signInState, stateStore });
+
+        // validate the code and state, and attempt to swap the code for tokens
+        const signinResponse = await client.processSigninResponse(reconstructedUrl.href);
+
+        // extra values we stored at the start of the login flow
+        // used to complete login in the client
+        const userState = signinResponse.userState;
+        validateStoredUserState(userState);
+
+        // throws when response is invalid
+        validateBearerTokenResponse(signinResponse);
+        // throws when token is invalid
+        validateIdToken(signinResponse.id_token, client.settings.authority, client.settings.client_id, userState.nonce);
+        const normalizedTokenResponse = normalizeBearerTokenResponseTokenType(signinResponse);
+
+        return {
+            oidcClientSettings: {
+                clientId: client.settings.client_id,
+                issuer: client.settings.authority,
+            },
+            tokenResponse: normalizedTokenResponse,
+            homeserverUrl: userState.homeserverUrl,
+            identityServerUrl: userState.identityServerUrl,
+        };
+    } catch (error) {
+        logger.error("Oidc login failed", error);
+        const errorType = (error as Error).message;
+
+        // rethrow errors that we recognise
+        if (Object.values(OidcError).includes(errorType as any)) {
+            throw error;
+        }
         throw new Error(OidcError.CodeExchangeFailed);
     }
-
-    const token = await response.json();
-
-    if (isValidBearerTokenResponse(token)) {
-        // throws when token is invalid
-        validateIdToken(token.id_token, delegatedAuthConfig.issuer, clientId, nonce);
-        return normalizeBearerTokenResponseTokenType(token);
-    }
-
-    throw new Error(OidcError.InvalidBearerTokenResponse);
 };

src/oidc/error.ts

@@ -25,4 +25,5 @@ export enum OidcError {
     CodeExchangeFailed = "Failed to exchange code for token",
     InvalidBearerTokenResponse = "Invalid bearer token response",
     InvalidIdToken = "Invalid ID token",
+    MissingOrInvalidStoredState = "State required to finish logging in is not found in storage.",
 }

src/oidc/validate.ts

@@ -15,7 +15,7 @@ limitations under the License.
 */
 
 import jwtDecode from "jwt-decode";
-import { OidcMetadata } from "oidc-client-ts";
+import { OidcMetadata, SigninResponse } from "oidc-client-ts";
 
 import { IDelegatedAuthConfig } from "../client";
 import { logger } from "../logger";
@@ -62,14 +62,14 @@ const isRecord = (value: unknown): value is Record<string, unknown> =>
     !!value && typeof value === "object" && !Array.isArray(value);
 const requiredStringProperty = (wellKnown: Record<string, unknown>, key: string): boolean => {
     if (!wellKnown[key] || !optionalStringProperty(wellKnown, key)) {
-        logger.error(`OIDC issuer configuration: ${key} is invalid`);
+        logger.error(`Missing or invalid property: ${key}`);
         return false;
     }
     return true;
 };
 const optionalStringProperty = (wellKnown: Record<string, unknown>, key: string): boolean => {
     if (!!wellKnown[key] && typeof wellKnown[key] !== "string") {
-        logger.error(`OIDC issuer configuration: ${key} is invalid`);
+        logger.error(`Invalid property: ${key}`);
         return false;
     }
     return true;
@@ -77,7 +77,7 @@ const optionalStringProperty = (wellKnown: Record<string, unknown>, key: string)
 const requiredArrayValue = (wellKnown: Record<string, unknown>, key: string, value: any): boolean => {
     const array = wellKnown[key];
     if (!array || !Array.isArray(array) || !array.includes(value)) {
-        logger.error(`OIDC issuer configuration: ${key} is invalid. ${value} is required.`);
+        logger.error(`Invalid property: ${key}. ${value} is required.`);
         return false;
     }
     return true;
@@ -245,3 +245,64 @@ export type UserState = {
      */
     nonce: string;
 };
+/**
+ * Validate stored user state exists and is valid
+ * @param userState - userState returned by oidcClient.processSigninResponse
+ * @throws when userState is invalid
+ */
+export function validateStoredUserState(userState: unknown): asserts userState is UserState {
+    if (!isRecord(userState)) {
+        logger.error("Stored user state not found");
+        throw new Error(OidcError.MissingOrInvalidStoredState);
+    }
+    const isInvalid = [
+        requiredStringProperty(userState, "homeserverUrl"),
+        requiredStringProperty(userState, "nonce"),
+        optionalStringProperty(userState, "identityServerUrl"),
+    ].some((isValid) => !isValid);
+
+    if (isInvalid) {
+        throw new Error(OidcError.MissingOrInvalidStoredState);
+    }
+}
+
+/**
+ * The expected response type from the token endpoint during authorization code flow
+ * Normalized to always use capitalized 'Bearer' for token_type
+ *
+ * See https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.4,
+ * https://openid.net/specs/openid-connect-basic-1_0.html#TokenOK.
+ */
+export type BearerTokenResponse = {
+    token_type: "Bearer";
+    access_token: string;
+    scope: string;
+    refresh_token?: string;
+    expires_in?: number;
+    // from oidc-client-ts
+    expires_at?: number;
+    id_token?: string;
+};
+
+/**
+ * Make required properties required in type
+ */
+type ValidSignInResponse = SigninResponse &
+    BearerTokenResponse & {
+        token_type: "Bearer" | "bearer";
+    };
+
+const isValidBearerTokenResponse = (response: unknown): response is ValidSignInResponse =>
+    isRecord(response) &&
+    requiredStringProperty(response, "token_type") &&
+    // token_type is case insensitive, some OPs return `token_type: "bearer"`
+    (response["token_type"] as string).toLowerCase() === "bearer" &&
+    requiredStringProperty(response, "access_token") &&
+    requiredStringProperty(response, "refresh_token") &&
+    (!("expires_in" in response) || typeof response["expires_in"] === "number");
+
+export function validateBearerTokenResponse(response: unknown): asserts response is ValidSignInResponse {
+    if (!isValidBearerTokenResponse(response)) {
+        throw new Error(OidcError.InvalidBearerTokenResponse);
+    }
+}