Skip to content

Commit

Permalink
fix(authts#780): filterProtocolClaims deletes properties required by …
Browse files Browse the repository at this point in the history
…the IdTokenClaims type
  • Loading branch information
Badisi committed Jan 16, 2023
1 parent 696c17a commit b7fadd8
Show file tree
Hide file tree
Showing 6 changed files with 141 additions and 21 deletions.
6 changes: 3 additions & 3 deletions docs/oidc-client-ts.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ export type ExtraSignoutRequestArgs = Pick<CreateSignoutRequestArgs, "extraQuery
// Warning: (ae-forgotten-export) The symbol "Mandatory" needs to be exported by the entry point index.d.ts
//
// @public
export interface IdTokenClaims extends Mandatory<OidcStandardClaims, "sub">, Mandatory<JwtClaims, "iss" | "sub" | "aud" | "exp" | "iat"> {
export interface IdTokenClaims extends Mandatory<OidcStandardClaims, "sub">, Mandatory<JwtClaims, "iss" | "sub" | "aud" | "exp" | "iat">, Pick<JwtClaims, "nbf" | "jti"> {
// (undocumented)
[claim: string]: unknown;
acr?: string;
Expand Down Expand Up @@ -344,7 +344,7 @@ export interface OidcClientSettings {
// (undocumented)
extraTokenParams?: Record<string, unknown>;
fetchRequestCredentials?: RequestCredentials;
filterProtocolClaims?: boolean;
filterProtocolClaims?: boolean | string[];
loadUserInfo?: boolean;
max_age?: number;
mergeClaims?: boolean;
Expand Down Expand Up @@ -394,7 +394,7 @@ export class OidcClientSettingsStore {
// (undocumented)
readonly fetchRequestCredentials: RequestCredentials;
// (undocumented)
readonly filterProtocolClaims: boolean;
readonly filterProtocolClaims: boolean | string[];
// (undocumented)
readonly loadUserInfo: boolean;
// (undocumented)
Expand Down
2 changes: 1 addition & 1 deletion src/Claims.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<OidcStandardClaims, "sub">, Mandatory<JwtClaims, "iss" | "sub" | "aud" | "exp" | "iat"> {
export interface IdTokenClaims extends Mandatory<OidcStandardClaims, "sub">, Mandatory<JwtClaims, "iss" | "sub" | "aud" | "exp" | "iat">, Pick<JwtClaims, "nbf" | "jti"> {
[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.) */
Expand Down
11 changes: 11 additions & 0 deletions src/OidcClientSettings.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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"]);
});
});

Expand Down
12 changes: 8 additions & 4 deletions src/OidcClientSettings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) */
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
99 changes: 96 additions & 3 deletions src/ResponseValidator.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@ describe("ResponseValidator", () => {
jest.spyOn(JwtUtils, "decode").mockReturnValue({
sub: "sub",
iss: "iss",
acr: "acr",
a: "apple",
b: "banana",
});
Expand All @@ -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 () => {
Expand Down Expand Up @@ -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
Expand All @@ -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,
});
});

Expand Down Expand Up @@ -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<typeof items>).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);
});
});
});
32 changes: 22 additions & 10 deletions src/ResponseValidator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,30 +16,33 @@ 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}
* - {@link https://openid.net/specs/openid-connect-core-1_0.html#CodeIDToken}
*
* @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
*/
Expand Down Expand Up @@ -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];
}
}
}

Expand Down

0 comments on commit b7fadd8

Please sign in to comment.