Repository: authts/oidc-client-ts (ts)

Issue:
improve merge claims behavior
- In order to allow "refresh userinfo data on demand" #877 merging claims must be adapted in way way that we can do it multiple times.
- Changing the type of string to array by default (to preserve the previous different value) seems not a good idea. In case this is needed by some authz servers, we could add a special settings to have this behavior back (see [#881#issuecomment-1425834324](https://github.com/authts/oidc-client-ts/pull/881#issuecomment-1425834324)). `mergeClaimsStrategy?: "replace" | "merge"]`
- drop misleading `mergeClaims` setting, we always merge ...


Pull request:
improve merge claims behavior
<!-- Please link relevant issue numbers or provide context for this change -->
Closes/fixes #1232

### Checklist

- [x] This PR makes changes to the public API <!-- was the API report (docs/oidc-client-ts.api.md) updated by this PR? -->
- [x] I have included links for closing relevant issue numbers

This merge requests enables a fix for #846 and #852. This MR introduces the new merge behavior and moves all the claims related logic inside a dedicated service.
This code is based on the pre-work done by @marcoreni in #881!


Patch:
diff --git a/docs/migration.md b/docs/migration.md
index 898870f5..a7b8b4d1 100644
--- a/docs/migration.md
+++ b/docs/migration.md
@@ -1,6 +1,6 @@
-## oidc-client v2.4.0 &rarr; oidc-client-ts v3.0.0
+## oidc-client-ts v2.4.0 &rarr; 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 &rarr; oidc-client-ts v2.0.0
diff --git a/docs/oidc-client-ts.api.md b/docs/oidc-client-ts.api.md
index 801fa087..023b0240 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<OidcMetadata>;
     metadataSeed?: Partial<OidcMetadata>;
     // (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<OidcMetadata> | undefined;
     // (undocumented)
diff --git a/package-lock.json b/package-lock.json
index 362224fb..45c6883c 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -1476,9 +1476,9 @@
       }
     },
     "node_modules/@microsoft/api-extractor": {
-      "version": "7.38.2",
-      "resolved": "https://registry.npmjs.org/@microsoft/api-extractor/-/api-extractor-7.38.2.tgz",
-      "integrity": "sha512-JOARuhTwOcOMIU0O2czscoJy3ddVzIRhSA9/7T1ALuZSNphgWsPk+Bv4E7AnBDmTV4pP4lBNLtCxEHjjpWaytQ==",
+      "version": "7.38.1",
+      "resolved": "https://registry.npmjs.org/@microsoft/api-extractor/-/api-extractor-7.38.1.tgz",
+      "integrity": "sha512-Hxu/RrVpItQ4dzeMyfwlk4lGQFsXMoMS7bYU9YUrpW16hH04PXLRiTXJz77WhBiSGNtTuufz2xh6hWyXhC9JuQ==",
       "dev": true,
       "dependencies": {
         "@microsoft/api-extractor-model": "7.28.2",
@@ -1486,7 +1486,7 @@
         "@microsoft/tsdoc-config": "~0.16.1",
         "@rushstack/node-core-library": "3.61.0",
         "@rushstack/rig-package": "0.5.1",
-        "@rushstack/ts-command-line": "4.17.1",
+        "@rushstack/ts-command-line": "4.17.0",
         "colors": "~1.2.1",
         "lodash": "~4.17.15",
         "resolve": "~1.22.1",
@@ -1606,9 +1606,9 @@
       }
     },
     "node_modules/@rushstack/ts-command-line": {
-      "version": "4.17.1",
-      "resolved": "https://registry.npmjs.org/@rushstack/ts-command-line/-/ts-command-line-4.17.1.tgz",
-      "integrity": "sha512-2jweO1O57BYP5qdBGl6apJLB+aRIn5ccIRTPDyULh0KMwVzFqWtw6IZWt1qtUoZD/pD2RNkIOosH6Cq45rIYeg==",
+      "version": "4.17.0",
+      "resolved": "https://registry.npmjs.org/@rushstack/ts-command-line/-/ts-command-line-4.17.0.tgz",
+      "integrity": "sha512-1S0sXuEpZlzKTfvUqNs7Rg4leVkeLJc4Dn9cm+pSIn35a0Ztp5GxPN2gabD2G4RrQoQcJLLyVu+twzrJl1C0eA==",
       "dev": true,
       "dependencies": {
         "@types/argparse": "1.0.38",
diff --git a/src/ClaimsService.ts b/src/ClaimsService.ts
index 66752e65..30c32c8e 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 c0719974..2b664e5c 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;
 


Test patch:
diff --git a/src/ClaimsService.test.ts b/src/ClaimsService.test.ts
index 97ac7e66..ea2dee96 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" });
         });
     });
 });