Oidc-client-js: Problem merging claims when array with JSON

[Cronkan]

When having a claim of type IdentityServerConstants.ClaimValueTypes.Json (IdentityServer4) with an array of json-object the merging does not take array or JSON into consideration. The following happens:

UserInfoService.getClaims: claims received: { client_chains: Array(2)
0: {Id: 60, Name: "CHAIN 1"}
1: {Id: 59, Name: "CHAIN 2"}
client_stores: Array(1)
0: {Name: "STORE", Id: 1, IsAdmin: true} } 

Becomes:

ResponseValidator._processClaims: user info claims received, updated profile:
{ client_chains: {
Id: (2) [60, 59]
Name: (2) ["CHAIN 1", "CHAIN 2"]
}
client_stores: {
Id: 1
IsAdmin: true
Name: "STORE" } }

It should not remove the Array on client_chains and client_stores (as this could have multiple entries on some users) and should not merge together properties on a array with json-objects.

[Cronkan]

problem seems to be here:

    _mergeClaims(claims1, claims2) {
        var result = Object.assign({}, claims1);

        for (let name in claims2) {
            var values = claims2[name];
            if (!Array.isArray(values)) {
                values = [values];
            }

            for (let i = 0; i < values.length; i++) {
                let value = values[i];
                if (!result[name]) {
                    result[name] = value;
                }
                else if (Array.isArray(result[name])) {
                    if (result[name].indexOf(value) < 0) {
                        result[name].push(value);
                    }
                }
                else if (result[name] !== value) {
                    if (typeof value === 'object') {
                        result[name] = this._mergeClaims(result[name], value);
                    }
                    else {
                        result[name] = [result[name], value];
                    }
                }
            }
        }

        return result;
    }

Problem can be fixed by adding a setting mergeClaims and doing

response.profile = this._settings.mergeClaims ? this._mergeClaims(response.profile, claims) : Object.assign({}, response.profile, claims);

Although i dont know if the wanted behaviour is right in _mergeClaims? If that is changed it might break others solutions?

[brockallen]

Can you show me the JSON in the id_token or the response from the user info endpoint?

[dejan9393]

I'm running into a similar issue in v1.9.1, I believe the issue was non-existent on 1.6.1 as I only noticed the problem after recently upgrading. My userinfo endpoint result is:

{
  "sub": "123456",
  "name": "Test",
  "family_name": "User",
  "given_name": "Test User",
  "preferred_username": "test",
  "profile": "http://dev.example.com/admin/profile/test",
  "picture": "http://dev.example.com/public/image/123456",
  "email": "[email protected]",
  "email_verified": true,
  "phone_number": "[phone]",
  "phone_number_verified": false,
  "social": [
    {
      "provider": "facebook",
      "linked": false,
      "access_token": null
    },
    {
      "provider": "linkedin",
      "linked": false,
      "access_token": null
    },
    {
      "provider": "twitter",
      "linked": false,
      "access_token": null
    },
    {
      "provider": "google",
      "linked": false,
      "access_token": null
    },
    {
      "provider": "fitbit",
      "linked": false,
      "access_token": null
    },
    {
      "provider": "strava",
      "linked": false,
      "access_token": null
    }
  ]
}

the social claim gets merged and ends up looking like:

{
  "provider": [
    "facebook",
    "linkedin",
    "twitter",
    "google",
    "fitbit",
    "strava"
  ],
  "linked": false,
  "access_token": null
}

[dejan9393]

Has to do with the changes in this PR/commit: #754 https://github.com/IdentityModel/oidc-client-js/commit/510d2c569bbe3ba63ef35c188f96ed7e56c7431f

[brockallen]

@Cronkan

[Cronkan]

@brockallen It's the same as @dejan9393 but when different values they get grouped in arrays.
The array itself containg objects is replaced by one object, with grouped properties.

{
    "chains": [
        {
            "id": 111,
            "name": "OneChain"
        },
        {
            "id": 222,
            "name": "TwoChain"
        }
    ],
    "stores": [
        {
            "id": 1,
            "isAdmin": true,
            "name": "STORE"
        }
    ]
}

Becomes

{
    "chains": {
        "Id": [
            111,
            222
        ],
        "Name": [
            "OneChain",
            "TwoChain"
        ]
    },
    "stores": {
        "Id": 1,
        "IsAdmin": true,
        "Name": "STORE"
    }
}

[jakebeatty]

Appear to be having a similar issue that @dejan9393 had, but with v1.10.1.
My social claim is currently returning the same as his post merge.

[dejan9393]

Any update on this issue?

[cadobre]

I am experiencing the same issue in 1.10.1, after upgrading from 1.6.1.

The problem with mergeClaims is that it alters the claims in such a way that you cannot rebuild it to the original format, basically, it's losing data. Here is one example:

claims:

[
    {
      "GUID": "6ae5ae43-7540-470d-a081-2cb6abb82e4f",
      "Name": "Official",
      "Suspended": false,
      "Type": 2,
      "LinkUserUID": 43,
      "LinkOpUID": null
    },
    {
      "GUID": "f20b4d46-8632-4e23-ac78-483b1101b17e",
      "Name": "RO",
      "Suspended": false,
      "Type": 2,
      "LinkUserUID": 2,
      "LinkOpUID": null
    },
    {
      "GUID": "94ebf070-03b5-410c-ac33-e62442e7c652",
      "Name": "A+T",
      "Suspended": false,
      "Type": 2,
      "LinkUserUID": 1,
      "LinkOpUID": null
    },
    {
      "GUID": "9f6552ae-d526-4793-b2e9-676d98e883fe",
      "Name": "A+",
      "Suspended": false,
      "Type": 3,
      "LinkUserUID": 1,
      "LinkOpUID": null
    },
    {
      "GUID": "4966ad58-a662-44d9-9136-1b079ae264fb",
      "Name": "A",
      "Suspended": false,
      "Type": 4,
      "LinkUserUID": null,
      "LinkOpUID": 1
    },
    {
      "GUID": "15ac9280-7bbe-4a52-8aad-850439bf2058",
      "Name": "L",
      "Suspended": false,
      "Type": 1,
      "LinkUserUID": null,
      "LinkOpUID": 1
    }
  ]
}

After response.profile = _this2._mergeClaims(response.profile, claims) it becomes:

{
    "GUID": [
        "6ae5ae43-7540-470d-a081-2cb6abb82e4f",
        "f20b4d46-8632-4e23-ac78-483b1101b17e",
        "94ebf070-03b5-410c-ac33-e62442e7c652",
        "9f6552ae-d526-4793-b2e9-676d98e883fe",
        "4966ad58-a662-44d9-9136-1b079ae264fb",
        "15ac9280-7bbe-4a52-8aad-850439bf2058"
    ],
    "Name": [
        "Official",
        "RO",
        "A+T",
        "A+",
        "A",
        "L"
    ],
    "Suspended": false,
    "Type": [
        2,
        3,
        4,
        1
    ],
    "LinkUserUID": [
        43,
        2,
        1,
        null
    ],
    "LinkOpUID": 1
}

If I try to map back to the original format, it's not possible. See for example the arrays Type,LinkUserUID they have the length 4 instead of 6, so it's not possible to correlate them with arrays GUID,Name. I did not invest time in the internals of the function, but it looks like it's removing duplicate values from the arrays.

[sg3s]

Can confirm this is still an issue at this time.

Our "fix" was using string keys/serialise the data as an object instead of an array on the backend, then use Object.values(claimObjectValue) to get a straight array back again in js.

This will not be a possible fix for everyone - nor do we like it ourselves.

[Cronkan]

Yeah so my fix was to just have a setting to disable mergeClaims, but nothing happened to my PR so it's a bit outdated now. Why is it even needed @brockallen ?

[bbialas]

Hello, is there any progress on this topic? I can confirm, that the bug is there and claims are wrongly merged.

[bbialas]

I added my proposition of the fix. Please check my pull request with simplified merge logic: https://github.com/IdentityModel/oidc-client-js/pull/1261

To authors:
Please take a look at that issue asap, because it's a big blocker for the functionality. Thank you! :)

[sg3s]

We've analysed the function further here. I put the function up as an interview question to annotate it, and had it vetted by a couple of colleagues. There are more issues than just the merging of arrays.

PR #862 Evades the issue by just doing a shallow merge - which is a good option to have, but doesn't fix the code in question
PR #1261 Makes the function a shallow merge, combining but not deduplicating values in an array if they collide

We might be wrong, but the following snippet;

if (!result[name]) {
    result[name] = [];
}

Also causes attributes with falsy values (0 and false) in claims1 to be overwritten - which I don't think is something you want either.

So neither PR available to "fix" this is really complete in that sense.

The algorithm here to merge objects, retain non-duplicate values, seems fairly custom to fill a specific role but I think just a well-vetted deep merge should suffice. Maybe having an option to pass a merge function - with the current implementation being the default - could provide a good backward- and forward-compatible fix.

But I think it is up to @brockallen to determine what the actual behaviour should be / what use case it is supposed to fill.

[andrewperry]

That is why I noted that this merge introduced a bug back on https://github.com/IdentityModel/oidc-client-js/commit/510d2c569bbe3ba63ef35c188f96ed7e56c7431f

Can we please get this reverted or at least have the ability to switch it off @brockallen?

We are currently having to maintain a private fork, but it seems others would benefit from that version too if this can’t be resolved here.... which would be ideal!

We've analysed the function further here. I put the function up as an interview question to annotate it, and had it vetted by a couple of colleagues. There are more issues than just the merging of arrays.

PR #862 Evades the issue by just doing a shallow merge - which is a good option to have, but doesn't fix the code in question
PR #1261 Makes the function a shallow merge, combining but not deduplicating values in an array if they collide

We might be wrong, but the following snippet;

if (!result[name]) {
    result[name] = [];
}

Also causes attributes with falsy values (0 and false) in claims1 to be overwritten - which I don't think is something you want either.

So neither PR available to "fix" this is really complete in that sense.

The algorithm here to merge objects, retain non-duplicate values, seems fairly custom to fill a specific role but I think just a well-vetted deep merge should suffice. Maybe having an option to pass a merge function - with the current implementation being the default - could provide a good backward- and forward-compatible fix.

But I think it is up to @brockallen to determine what the actual behaviour should be / what use case it is supposed to fill.