From b7fadd89203b52da7f29e6fc03418851d10d7f76 Mon Sep 17 00:00:00 2001 From: Badisi Date: Fri, 13 Jan 2023 21:22:34 +0100 Subject: [PATCH] fix(#780): filterProtocolClaims deletes properties required by the IdTokenClaims type --- docs/oidc-client-ts.api.md | 6 +-- src/Claims.ts | 2 +- src/OidcClientSettings.test.ts | 11 ++++ src/OidcClientSettings.ts | 12 +++-- src/ResponseValidator.test.ts | 99 ++++++++++++++++++++++++++++++++-- src/ResponseValidator.ts | 32 +++++++---- 6 files changed, 141 insertions(+), 21 deletions(-) diff --git a/docs/oidc-client-ts.api.md b/docs/oidc-client-ts.api.md index bd66c08dd..01a10a26a 100644 --- a/docs/oidc-client-ts.api.md +++ b/docs/oidc-client-ts.api.md @@ -130,7 +130,7 @@ export type ExtraSignoutRequestArgs = Pick, Mandatory { +export interface IdTokenClaims extends Mandatory, Mandatory, Pick { // (undocumented) [claim: string]: unknown; acr?: string; @@ -344,7 +344,7 @@ export interface OidcClientSettings { // (undocumented) extraTokenParams?: Record; fetchRequestCredentials?: RequestCredentials; - filterProtocolClaims?: boolean; + filterProtocolClaims?: boolean | string[]; loadUserInfo?: boolean; max_age?: number; mergeClaims?: boolean; @@ -394,7 +394,7 @@ export class OidcClientSettingsStore { // (undocumented) readonly fetchRequestCredentials: RequestCredentials; // (undocumented) - readonly filterProtocolClaims: boolean; + readonly filterProtocolClaims: boolean | string[]; // (undocumented) readonly loadUserInfo: boolean; // (undocumented) diff --git a/src/Claims.ts b/src/Claims.ts index 3e34b6546..bfb3af4d2 100644 --- a/src/Claims.ts +++ b/src/Claims.ts @@ -106,7 +106,7 @@ export interface JwtClaims { * @public * @see https://openid.net/specs/openid-connect-core-1_0.html#IDToken */ -export interface IdTokenClaims extends Mandatory, Mandatory { +export interface IdTokenClaims extends Mandatory, Mandatory, Pick { [claim: string]: unknown; /** Time when the End-User authentication occurred. Its value is a JSON number representing the number of seconds from 1970-01-01T0:0:0Z as measured in UTC until the date/time. When a max_age request is made or when auth_time is requested as an Essential Claim, then this Claim is REQUIRED; otherwise, its inclusion is OPTIONAL. (The auth_time Claim semantically corresponds to the OpenID 2.0 PAPE [OpenID.PAPE] auth_time response parameter.) */ diff --git a/src/OidcClientSettings.test.ts b/src/OidcClientSettings.test.ts index 07dd57f64..4a05c13d0 100644 --- a/src/OidcClientSettings.test.ts +++ b/src/OidcClientSettings.test.ts @@ -320,6 +320,17 @@ describe("OidcClientSettings", () => { // assert expect(subject.filterProtocolClaims).toEqual(false); + + // act + subject = new OidcClientSettingsStore({ + authority: "authority", + client_id: "client", + redirect_uri: "redirect", + filterProtocolClaims: ["a", "b", "c"], + }); + + // assert + expect(subject.filterProtocolClaims).toEqual(["a", "b", "c"]); }); }); diff --git a/src/OidcClientSettings.ts b/src/OidcClientSettings.ts index f85393c98..2c569b8c1 100644 --- a/src/OidcClientSettings.ts +++ b/src/OidcClientSettings.ts @@ -71,8 +71,12 @@ export interface OidcClientSettings { /** optional protocol param (default: "query") */ response_mode?: "query" | "fragment"; - /** Should OIDC protocol claims be removed from profile (default: true) */ - filterProtocolClaims?: boolean; + /** + * Should optional OIDC protocol claims be removed from profile or specify the ones to be removed (default: true) + * When true, the following claims are removed by default: ["nbf", "jti", "auth_time", "nonce", "acr", "amr", "azp", "at_hash"] + * When specifying claims, the following claims are not allowed: ["sub", "iss", "aud", "exp", "iat"] + */ + filterProtocolClaims?: boolean | string[]; /** Flag to control if additional identity data is loaded from the user info endpoint in order to populate the user's profile (default: false) */ loadUserInfo?: boolean; /** Number (in seconds) indicating the age of state entries in storage for authorize requests that are considered abandoned and thus can be cleaned up (default: 900) */ @@ -153,7 +157,7 @@ export class OidcClientSettingsStore { public readonly response_mode: "query" | "fragment"; // behavior flags - public readonly filterProtocolClaims: boolean; + public readonly filterProtocolClaims: boolean | string[]; public readonly loadUserInfo: boolean; public readonly staleStateAgeInSeconds: number; public readonly clockSkewInSeconds: number; @@ -229,7 +233,7 @@ export class OidcClientSettingsStore { this.resource = resource; this.response_mode = response_mode; - this.filterProtocolClaims = !!filterProtocolClaims; + this.filterProtocolClaims = filterProtocolClaims ?? true; this.loadUserInfo = !!loadUserInfo; this.staleStateAgeInSeconds = staleStateAgeInSeconds; this.clockSkewInSeconds = clockSkewInSeconds; diff --git a/src/ResponseValidator.test.ts b/src/ResponseValidator.test.ts index 54133cc0f..7f86a3224 100644 --- a/src/ResponseValidator.test.ts +++ b/src/ResponseValidator.test.ts @@ -235,6 +235,7 @@ describe("ResponseValidator", () => { jest.spyOn(JwtUtils, "decode").mockReturnValue({ sub: "sub", iss: "iss", + acr: "acr", a: "apple", b: "banana", }); @@ -244,7 +245,7 @@ describe("ResponseValidator", () => { await subject.validateSigninResponse(stubResponse, stubState); // assert - expect(stubResponse.profile).not.toHaveProperty("iss"); + expect(stubResponse.profile).not.toHaveProperty("acr"); }); it("should not filter protocol claims if not OIDC", async () => { @@ -718,8 +719,8 @@ describe("ResponseValidator", () => { aud: "some_aud", iss: "issuer", sub: "123", email: "foo@gmail.com", role: ["admin", "dev"], - at_hash: "athash", - iat: 5, nbf: 10, exp: 20, + iat: 5, exp: 20, + nbf: 10, at_hash: "athash", }; // act @@ -728,8 +729,10 @@ describe("ResponseValidator", () => { // assert expect(result).toEqual({ foo: 1, bar: "test", + aud: "some_aud", iss: "issuer", sub: "123", email: "foo@gmail.com", role: ["admin", "dev"], + iat: 5, exp: 20, }); }); @@ -758,5 +761,95 @@ describe("ResponseValidator", () => { iat: 5, nbf: 10, exp: 20, }); }); + + it("should filter protocol claims if specified in settings", () => { + // arrange + Object.assign(settings, { filterProtocolClaims: ["foo", "bar", "role", "nbf", "email"] }); + const claims = { + foo: 1, bar: "test", + aud: "some_aud", iss: "issuer", + sub: "123", email: "foo@gmail.com", + role: ["admin", "dev"], + iat: 5, exp: 20, + nbf: 10, at_hash: "athash", + }; + + // act + const result = subject["_filterProtocolClaims"](claims); + + // assert + expect(result).toEqual({ + aud: "some_aud", iss: "issuer", + sub: "123", + iat: 5, exp: 20, + at_hash: "athash", + }); + }); + + it("should filter only protocol claims defined by default by the library", () => { + // arrange + Object.assign(settings, { filterProtocolClaims: true }); + const defaultProtocolClaims = { + nbf: 3, jti: "jti", + auth_time: 123, + nonce: "nonce", + acr: "acr", + amr: "amr", + azp: "azp", + at_hash: "athash", + }; + const claims = { + foo: 1, bar: "test", + aud: "some_aud", iss: "issuer", + sub: "123", email: "foo@gmail.com", + role: ["admin", "dev"], + iat: 5, exp: 20, + }; + + // act + const result = subject["_filterProtocolClaims"]({ ...defaultProtocolClaims, ...claims }); + + // assert + expect(result).toEqual(claims); + }); + + it("should not filter protocol claims that are required by the library", () => { + // arrange + Object.assign(settings, { filterProtocolClaims: true }); + const internalRequiredProtocolClaims = { + sub: "sub", + iss: "issuer", + aud: "some_aud", + exp: 20, + iat: 5, + }; + const claims = { + foo: 1, bar: "test", + email: "foo@gmail.com", + role: ["admin", "dev"], + nbf: 10, + }; + + // act + let items = { ...internalRequiredProtocolClaims, ...claims }; + let result = subject["_filterProtocolClaims"](items); + + // assert + // nbf is part of the claims that should be filtered by the library by default, so we need to remove it + delete (items as Partial).nbf; + expect(result).toEqual(items); + + // ... even if specified in settings + + // arrange + Object.assign(settings, { filterProtocolClaims: ["sub", "iss", "aud", "exp", "iat"] }); + + // act + items = { ...internalRequiredProtocolClaims, ...claims }; + result = subject["_filterProtocolClaims"](items); + + // assert + expect(result).toEqual(items); + }); }); }); diff --git a/src/ResponseValidator.ts b/src/ResponseValidator.ts index e811472e2..637a3a156 100644 --- a/src/ResponseValidator.ts +++ b/src/ResponseValidator.ts @@ -16,6 +16,7 @@ import type { RefreshState } from "./RefreshState"; import type { JwtClaims, IdTokenClaims } from "./Claims"; /** + * Protocol claims that could be removed by default from profile. * Derived from the following sets of claims: * - {@link https://datatracker.ietf.org/doc/html/rfc7519.html#section-4.1} * - {@link https://openid.net/specs/openid-connect-core-1_0.html#IDToken} @@ -23,23 +24,25 @@ import type { JwtClaims, IdTokenClaims } from "./Claims"; * * @internal */ -const ProtocolClaims = [ - "iss", - // "sub" should never be excluded, we need access to it internally - "aud", - "exp", +const DefaultProtocolClaims = [ "nbf", - "iat", "jti", "auth_time", "nonce", "acr", "amr", "azp", - // https://openid.net/specs/openid-connect-core-1_0.html#CodeIDToken - "at_hash", + "at_hash", // https://openid.net/specs/openid-connect-core-1_0.html#CodeIDToken ] as const; +/** + * Protocol claims that should never be removed from profile. + * "sub" is needed internally and others should remain required as per the OIDC specs. + * + * @internal + */ +const InternalRequiredProtocolClaims = ["sub", "iss", "aud", "exp", "iat"]; + /** * @internal */ @@ -226,8 +229,17 @@ export class ResponseValidator { const result = { ...claims }; if (this._settings.filterProtocolClaims) { - for (const type of ProtocolClaims) { - delete result[type]; + let protocolClaims; + if (Array.isArray(this._settings.filterProtocolClaims)) { + protocolClaims = this._settings.filterProtocolClaims; + } else { + protocolClaims = DefaultProtocolClaims; + } + + for (const claim of protocolClaims) { + if (!InternalRequiredProtocolClaims.includes(claim)) { + delete result[claim]; + } } }