From 68c15741a79124d2c7c54256c940d67aa1aadd51 Mon Sep 17 00:00:00 2001 From: pamapa Date: Thu, 2 Nov 2023 13:41:02 +0100 Subject: [PATCH] feat: #1232 improve merge claims behavior --- docs/migration.md | 6 ++- docs/oidc-client-ts.api.md | 10 ++-- src/ClaimsService.test.ts | 107 ++++++++++++++++++++++--------------- src/ClaimsService.ts | 34 ++++++------ src/OidcClientSettings.ts | 14 ++--- 5 files changed, 99 insertions(+), 72 deletions(-) diff --git a/docs/migration.md b/docs/migration.md index 898870f5c..a7b8b4d14 100644 --- a/docs/migration.md +++ b/docs/migration.md @@ -1,6 +1,6 @@ -## oidc-client v2.4.0 → oidc-client-ts v3.0.0 +## oidc-client-ts v2.4.0 → oidc-client-ts v3.0.0 -The API is largely backwards-compatible. +The API is largely backwards-compatible. The merge claims behavior has been improved. ### [OidcClientSettings](https://authts.github.io/oidc-client-ts/interfaces/OidcClientSettings.html) @@ -8,6 +8,8 @@ The API is largely backwards-compatible. - `clockSkewInSeconds` unused since 2.0.0 - `userInfoJwtIssuer` unused since 2.0.0 - `refreshTokenCredentials` use `fetchRequestCredentials` since 2.1.0 +- the `mergeClaims` has been replaced by `mergeClaimsStrategy` + - if the previous behavior is needed `mergeClaimsStrategy: { array: "merge" }` can be used ## oidc-client v1.11.5 → oidc-client-ts v2.0.0 diff --git a/docs/oidc-client-ts.api.md b/docs/oidc-client-ts.api.md index 801fa087c..023b02407 100644 --- a/docs/oidc-client-ts.api.md +++ b/docs/oidc-client-ts.api.md @@ -358,7 +358,9 @@ export interface OidcClientSettings { filterProtocolClaims?: boolean | string[]; loadUserInfo?: boolean; max_age?: number; - mergeClaims?: boolean; + mergeClaimsStrategy?: { + array: "replace" | "merge"; + }; metadata?: Partial; metadataSeed?: Partial; // (undocumented) @@ -380,7 +382,7 @@ export interface OidcClientSettings { // @public export class OidcClientSettingsStore { - constructor({ authority, metadataUrl, metadata, signingKeys, metadataSeed, client_id, client_secret, response_type, scope, redirect_uri, post_logout_redirect_uri, client_authentication, prompt, display, max_age, ui_locales, acr_values, resource, response_mode, filterProtocolClaims, loadUserInfo, staleStateAgeInSeconds, mergeClaims, disablePKCE, stateStore, revokeTokenAdditionalContentTypes, fetchRequestCredentials, refreshTokenAllowedScope, extraQueryParams, extraTokenParams, extraHeaders, }: OidcClientSettings); + constructor({ authority, metadataUrl, metadata, signingKeys, metadataSeed, client_id, client_secret, response_type, scope, redirect_uri, post_logout_redirect_uri, client_authentication, prompt, display, max_age, ui_locales, acr_values, resource, response_mode, filterProtocolClaims, loadUserInfo, staleStateAgeInSeconds, mergeClaimsStrategy, disablePKCE, stateStore, revokeTokenAdditionalContentTypes, fetchRequestCredentials, refreshTokenAllowedScope, extraQueryParams, extraTokenParams, extraHeaders, }: OidcClientSettings); // (undocumented) readonly acr_values: string | undefined; // (undocumented) @@ -410,7 +412,9 @@ export class OidcClientSettingsStore { // (undocumented) readonly max_age: number | undefined; // (undocumented) - readonly mergeClaims: boolean; + readonly mergeClaimsStrategy: { + array: "replace" | "merge"; + }; // (undocumented) readonly metadata: Partial | undefined; // (undocumented) diff --git a/src/ClaimsService.test.ts b/src/ClaimsService.test.ts index 97ac7e666..ea2dee964 100644 --- a/src/ClaimsService.test.ts +++ b/src/ClaimsService.test.ts @@ -2,7 +2,7 @@ // Licensed under the Apache License, Version 2.0. See LICENSE in the project root for license information. import { ClaimsService } from "./ClaimsService"; -import type { OidcClientSettingsStore } from "./OidcClientSettings"; +import { OidcClientSettingsStore } from "./OidcClientSettings"; import type { UserProfile } from "./User"; describe("ClaimsService", () => { @@ -10,11 +10,11 @@ describe("ClaimsService", () => { let subject: ClaimsService; beforeEach(() => { - settings = { - authority: "op", - client_id: "client", - loadUserInfo: true, - } as OidcClientSettingsStore; + settings = new OidcClientSettingsStore({ + authority: "authority", + client_id: "client_id", + redirect_uri: "redirect_uri", + }); subject = new ClaimsService(settings); }); @@ -218,7 +218,7 @@ describe("ClaimsService", () => { expect(result).toEqual({ a: "apple", c: "carrot", b: "banana" }); }); - it("should not merge claims when claim types are objects", () => { + it("should merge claims when claim types are objects", () => { // arrange const c1 = { custom: { apple: "foo", pear: "bar" }, @@ -233,63 +233,59 @@ describe("ClaimsService", () => { // assert expect(result).toEqual({ - custom: [ - { apple: "foo", pear: "bar" }, - { apple: "foo", orange: "peel" }, - ], + custom: { apple: "foo", pear: "bar", orange: "peel" }, b: "banana", }); }); - it("should merge claims when claim types are objects when mergeClaims settings is true", () => { + it("should replace same claim types", () => { // arrange - Object.assign(settings, { mergeClaims: true }); - - const c1 = { - custom: { apple: "foo", pear: "bar" }, - } as unknown as UserProfile; - const c2 = { - custom: { apple: "foo", orange: "peel" }, - b: "banana", - }; + const c1 = { a: "apple", b: "banana" } as unknown as UserProfile; + const c2 = { a: "carrot" }; // act const result = subject.mergeClaims(c1, c2); // assert - expect(result).toEqual({ - custom: { apple: "foo", pear: "bar", orange: "peel" }, - b: "banana", - }); + expect(result).toEqual({ a: "carrot", b: "banana" }); }); - it("should merge same claim types into array", () => { + it("should remove duplicates when producing arrays", () => { // arrange const c1 = { a: "apple", b: "banana" } as unknown as UserProfile; - const c2 = { a: "carrot" }; + const c2 = { a: ["apple", "durian"] }; // act const result = subject.mergeClaims(c1, c2); // assert - expect(result).toEqual({ a: ["apple", "carrot"], b: "banana" }); + expect(result).toEqual({ a: ["apple", "durian"], b: "banana" }); }); - it("should merge arrays of same claim types into array", () => { + it("should merge arrays of same claim types into array (string vs. array) when mergeClaimsStrategy is 'merge'", () => { // arrange - const c1 = { a: "apple", b: "banana" } as unknown as UserProfile; - const c2 = { a: ["carrot", "durian"] }; + Object.assign(settings, { mergeClaimsStrategy: "merge" }); + const c1 = { + a: "apple", + b: "banana", + } as unknown as UserProfile; + const c2 = { + a: ["carrot", "durian"], + }; // act - let result = subject.mergeClaims(c1, c2); + const result = subject.mergeClaims(c1, c2); // assert expect(result).toEqual({ a: ["apple", "carrot", "durian"], b: "banana", }); + }); + it("should merge arrays of same claim types into array (array vs. array) when mergeClaimsStrategy is 'merge'", () => { // arrange + Object.assign(settings, { mergeClaimsStrategy: "merge" }); const d1 = { a: ["apple", "carrot"], b: "banana", @@ -297,15 +293,18 @@ describe("ClaimsService", () => { const d2 = { a: ["durian"] }; // act - result = subject.mergeClaims(d1, d2); + const result = subject.mergeClaims(d1, d2); // assert expect(result).toEqual({ a: ["apple", "carrot", "durian"], b: "banana", }); + }); + it("should merge arrays of same claim types into array (array vs. string) when mergeClaimsStrategy is 'merge'", () => { // arrange + Object.assign(settings, { mergeClaimsStrategy: "merge" }); const e1 = { a: ["apple", "carrot"], b: "banana", @@ -313,7 +312,7 @@ describe("ClaimsService", () => { const e2 = { a: "durian" }; // act - result = subject.mergeClaims(e1, e2); + const result = subject.mergeClaims(e1, e2); // assert expect(result).toEqual({ @@ -322,31 +321,51 @@ describe("ClaimsService", () => { }); }); - it("should remove duplicates when producing arrays", () => { - // arrange - const c1 = { a: "apple", b: "banana" } as unknown as UserProfile; - const c2 = { a: ["apple", "durian"] }; + it("should replace if type is different (array vs. string)", () => { + // arrange + const c1 = { + a: ["apple", "durian"], + b: "banana", + } as unknown as UserProfile; + const c2 = { a: "apple" }; // act const result = subject.mergeClaims(c1, c2); // assert - expect(result).toEqual({ a: ["apple", "durian"], b: "banana" }); + expect(result).toEqual({ a: "apple", b: "banana" }); }); - it("should not add if already present in array", () => { - // arrange + it("should replace if type is different (object vs. string)", () => { + // arrange const c1 = { - a: ["apple", "durian"], + custom: { a: "apple" }, b: "banana", } as unknown as UserProfile; - const c2 = { a: "apple" }; + const c2 = { custom: "apple" }; // act const result = subject.mergeClaims(c1, c2); // assert - expect(result).toEqual({ a: ["apple", "durian"], b: "banana" }); + expect(result).toEqual({ custom: "apple", b: "banana" }); + }); + + it("should replace if type is different (array vs. object)", () => { + // arrange + const c1 = { + custom: ["apple", "durian"], + b: "banana", + } as unknown as UserProfile; + const c2 = { + custom: { a: "apple" }, + } as unknown as UserProfile; + + // act + const result = subject.mergeClaims(c1, c2); + + // assert + expect(result).toEqual({ custom: { a: "apple" }, b: "banana" }); }); }); }); diff --git a/src/ClaimsService.ts b/src/ClaimsService.ts index 66752e65c..30c32c8e8 100644 --- a/src/ClaimsService.ts +++ b/src/ClaimsService.ts @@ -64,27 +64,27 @@ export class ClaimsService { return result; } + public mergeClaims(claims1: JwtClaims, claims2: JwtClaims): UserProfile; public mergeClaims(claims1: UserProfile, claims2: JwtClaims): UserProfile { const result = { ...claims1 }; - for (const [claim, values] of Object.entries(claims2)) { - for (const value of Array.isArray(values) ? values : [values]) { - const previousValue = result[claim]; - if (previousValue === undefined) { - result[claim] = value; - } - else if (Array.isArray(previousValue)) { - if (!previousValue.includes(value)) { - previousValue.push(value); - } - } - else if (result[claim] !== value) { - if (typeof value === "object" && this._settings.mergeClaims) { - result[claim] = this.mergeClaims(previousValue as UserProfile, value); - } - else { - result[claim] = [previousValue, value]; + if (result[claim] !== values) { + if (Array.isArray(result[claim]) || Array.isArray(values)) { + if (this._settings.mergeClaimsStrategy.array == "replace") { + result[claim] = values; + } else { + const mergedValues = Array.isArray(result[claim]) ? result[claim] as unknown[] : [result[claim]]; + for (const value of Array.isArray(values) ? values : [values]) { + if (!mergedValues.includes(value)) { + mergedValues.push(value); + } + } + result[claim] = mergedValues; } + } else if (typeof result[claim] === "object" && typeof values === "object") { + result[claim] = this.mergeClaims(result[claim] as JwtClaims, values as JwtClaims); + } else { + result[claim] = values; } } } diff --git a/src/OidcClientSettings.ts b/src/OidcClientSettings.ts index c07199743..2b664e5ce 100644 --- a/src/OidcClientSettings.ts +++ b/src/OidcClientSettings.ts @@ -87,10 +87,12 @@ export interface OidcClientSettings { staleStateAgeInSeconds?: number; /** - * Indicates if objects returned from the user info endpoint as claims (e.g. `address`) are merged into the claims from the id token as a single object. - * Otherwise, they are added to an array as distinct objects for the claim type. (default: false) + * Indicates how objects returned from the user info endpoint as claims (e.g. `address`) are merged into the claims from the + * id token as a single object. (default: `{ array: "replace" }`) + * - array: "replace": natives (string, int, float) and arrays are replaced, objects are merged as distinct objects + * - array: "merge": natives (string, int, float) are replaced, arrays and objects are merged as distinct objects */ - mergeClaims?: boolean; + mergeClaimsStrategy?: { array: "replace" | "merge" }; /** * Storage object used to persist interaction state (default: window.localStorage, InMemoryWebStorage iff no window). @@ -167,7 +169,7 @@ export class OidcClientSettingsStore { public readonly filterProtocolClaims: boolean | string[]; public readonly loadUserInfo: boolean; public readonly staleStateAgeInSeconds: number; - public readonly mergeClaims: boolean; + public readonly mergeClaimsStrategy: { array: "replace" | "merge" }; public readonly stateStore: StateStore; @@ -194,7 +196,7 @@ export class OidcClientSettingsStore { filterProtocolClaims = true, loadUserInfo = false, staleStateAgeInSeconds = DefaultStaleStateAgeInSeconds, - mergeClaims = false, + mergeClaimsStrategy = { array: "replace" }, disablePKCE = false, // other behavior stateStore, @@ -244,7 +246,7 @@ export class OidcClientSettingsStore { this.filterProtocolClaims = filterProtocolClaims ?? true; this.loadUserInfo = !!loadUserInfo; this.staleStateAgeInSeconds = staleStateAgeInSeconds; - this.mergeClaims = !!mergeClaims; + this.mergeClaimsStrategy = mergeClaimsStrategy; this.disablePKCE = !!disablePKCE; this.revokeTokenAdditionalContentTypes = revokeTokenAdditionalContentTypes;