From 5df4ebaada192e65a6b9becff464c027cb3bf3a5 Mon Sep 17 00:00:00 2001 From: Kerry Date: Tue, 11 Jul 2023 14:20:19 +1200 Subject: [PATCH] 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 | 246 ++++++++++++++++++++++--------- spec/unit/oidc/validate.spec.ts | 6 +- src/oidc/authorize.ts | 169 +++++++++++---------- src/oidc/error.ts | 1 + src/oidc/validate.ts | 69 ++++++++- 5 files changed, 331 insertions(+), 160 deletions(-) diff --git a/spec/unit/oidc/authorize.spec.ts b/spec/unit/oidc/authorize.spec.ts index 773d7071d..ffd22148d 100644 --- a/spec/unit/oidc/authorize.spec.ts +++ b/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): 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 = (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 => { + 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, { - clientId, - codeVerifier, - redirectUri, - delegatedAuthConfig, - nonce, - }); + const state = await setupState(); + const codeVerifier = getValueFromStorage(state, "code_verifier"); + await completeAuthorizationCodeGrant(code, state); - expect(fetchMock).toHaveBeenCalledWith(tokenEndpoint, { - method: Method.Post, - headers: { "Content-Type": "application/x-www-form-urlencoded" }, - body: `grant_type=authorization_code&client_id=${clientId}&code_verifier=${codeVerifier}&redirect_uri=https%3A%2F%2Ftest.com&code=${code}`, - }); + expect(fetchMock).toHaveBeenCalledWith( + metadata.token_endpoint, + expect.objectContaining({ + 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, { - clientId, - codeVerifier, - redirectUri, - delegatedAuthConfig, - nonce, - }); + const state = await setupState(); + const scope = getValueFromStorage(state, "scope"); + const result = await completeAuthorizationCodeGrant(code, state); - 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, - body: JSON.stringify(tokenResponse), + headers: { + "Content-Type": "application/json", + }, + ...tokenResponse, }, { overwriteRoutes: true }, ); - const result = await completeAuthorizationCodeGrant(code, { - clientId, - codeVerifier, - redirectUri, - delegatedAuthConfig, - nonce, + const result = await completeAuthorizationCodeGrant(code, state); + + expect(result).toEqual({ + homeserverUrl, + identityServerUrl, + 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).toEqual(validBearerTokenResponse); - expect(result.token_type).toEqual("Bearer"); + expect(result.tokenResponse.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(() => - completeAuthorizationCodeGrant(code, { - clientId, - codeVerifier, - redirectUri, - delegatedAuthConfig, - nonce, - }), - ).rejects.toThrow(new Error(OidcError.CodeExchangeFailed)); + await expect(() => completeAuthorizationCodeGrant(code, state)).rejects.toThrow( + new Error(OidcError.MissingOrInvalidStoredState), + ); + }); + + it("should throw with code exchange failed error when request fails", async () => { + const state = await setupState(); + fetchMock.post( + 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, - { status: 200, body: JSON.stringify(invalidBearerTokenResponse) }, + metadata.token_endpoint, + { + headers: { + "Content-Type": "application/json", + }, + ...invalidBearerTokenResponse, + }, { overwriteRoutes: true }, ); - await expect(() => - completeAuthorizationCodeGrant(code, { - clientId, - codeVerifier, - redirectUri, - delegatedAuthConfig, - nonce, - }), - ).rejects.toThrow(new Error(OidcError.InvalidBearerTokenResponse)); + await expect(() => completeAuthorizationCodeGrant(code, state)).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(() => - completeAuthorizationCodeGrant(code, { - clientId, - codeVerifier, - redirectUri, - delegatedAuthConfig, - nonce, - }), - ).rejects.toThrow(new Error(OidcError.InvalidIdToken)); + await expect(() => completeAuthorizationCodeGrant(code, state)).rejects.toThrow( + new Error(OidcError.InvalidIdToken), + ); }); }); }); diff --git a/spec/unit/oidc/validate.spec.ts b/spec/unit/oidc/validate.spec.ts index f177d0d5a..91b6985f0 100644 --- a/spec/unit/oidc/validate.spec.ts +++ b/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( - "OIDC issuer configuration: response_types_supported is invalid. code is required.", - ); + expect(logger.error).toHaveBeenCalledWith("Missing or invalid property: authorization_endpoint"); + expect(logger.error).toHaveBeenCalledWith("Invalid property: response_types_supported. code is required."); }); it("should return validated issuer config", () => { diff --git a/src/oidc/authorize.ts b/src/oidc/authorize.ts index 8dca760c5..df802aa0a 100644 --- a/src/oidc/authorize.ts +++ b/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 => - !!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 ({ - ...response, - token_type: "Bearer", -}); - -const isValidBearerTokenResponse = (response: unknown): response is WireBearerTokenResponse => - isResponseObject(response) && - typeof response["token_type"] === "string" && - // token_type is case insensitive, some OPs return `token_type: "bearer"` - 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"); +const normalizeBearerTokenResponseTokenType = (response: SigninResponse): BearerTokenResponse => + ({ + id_token: response.id_token, + scope: response.scope, + expires_at: response.expires_at, + refresh_token: response.refresh_token, + access_token: response.access_token, + token_type: "Bearer", + } as BearerTokenResponse); /** + * @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, - { - clientId, - codeVerifier, - redirectUri, - delegatedAuthConfig, - nonce, - }: { - clientId: string; - codeVerifier: string; - redirectUri: string; - delegatedAuthConfig: IDelegatedAuthConfig & ValidatedIssuerConfig; - nonce: string; - }, -): Promise => { - const params = new URLSearchParams(); - 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(); + state: string, +): Promise<{ + oidcClientSettings: IDelegatedAuthConfig & { clientId: string }; + tokenResponse: BearerTokenResponse; + homeserverUrl: string; + identityServerUrl?: string; +}> => { + /** + * Element Web strips and changes the url on starting the app + * Use the code and state from query params to rebuild a url + * so that oidc-client can parse it + */ + const reconstructedUrl = new URL(window.location.origin); + reconstructedUrl.searchParams.append("code", code); + reconstructedUrl.searchParams.append("state", state); - 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, { - method: Method.Post, - headers, - body: metadata, - }); + const stateStore = new WebStorageStateStore({ prefix: "mx_oidc_", store: window.sessionStorage }); - 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); }; diff --git a/src/oidc/error.ts b/src/oidc/error.ts index 233ae34c5..c71e80830 100644 --- a/src/oidc/error.ts +++ b/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.", } diff --git a/src/oidc/validate.ts b/src/oidc/validate.ts index 1db4ba854..ef3e7c26e 100644 --- a/src/oidc/validate.ts +++ b/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 => !!value && typeof value === "object" && !Array.isArray(value); const requiredStringProperty = (wellKnown: Record, 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, 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, key: string) const requiredArrayValue = (wellKnown: Record, 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); + } +}