Currently loadUserInfo is tight to the SigninResponse within ResponseValidator._processClaims, but that could berefactored and that feature could be made accessible independent of the sign-in process...

I may be able to contribute to this, but I think there are several ways to do it.

The first two that came to mind are:

1. Add an optional parameter to UserManager.getUser (eg. updateUserInfo?: boolean) and, if the user is logged and the parameter is set, invoke a new method that refresh the data and store the updated data.
2. Create a separate method UserManager.getUserInfo that retrieves the "user info claims" from storage and/or from remote (with an "update" parameter like before)

I think that some refactor is required, because AFAIU the UserInfoService is currently strictly related to the _processClaims (and nested) methods of ResponseValidator, but it should be separate from that.

Let me know if you have a different view on how this may be solved.

EDIT: probably we should also take into consideration token refreshing. Right now, since the user is always local, there's no need to refresh the token. Maybe, if the "user info" is loaded remotely, we should handle the "expired tokens" scenario and trigger a silentSignin

The first open makes most sense.

BTW: This library already support silent renewing of the access token / id token, that path also takes into account to load the additional user info if needed via SigninResponse.

I'm struggling to understand the reason behind

With this, whenever a value change, the claim is transformed into an array and the new value is added to the previous.

Besides the feature I'm working on, I think this may already cause problems (probably #790 is due to this). For example, imagine the following scenario:

1. settings = { [...], loadUserInfo: true, automaticSilentRenew: true }
2. Perform signin
3. Go to Identity Provider and change some attributes (eg change nickname and update thumbnail)
4. Wait for token expiration
5. UserManager will automatically refresh the token, but AFAIU nickname and thumbnail claims will now contain old and new values.

What is the reason behind this? Changing this behavior is a Breaking Change, so I don't think it can be changed easily, but at the same time we wound need additional parameters and logics to handle a new "overrideClaimsOnRefresh" param (and also knowing which claims should be overwritten and which should be concatenated)

I'm struggling to understand the reason behind

oidc-client-ts/src/ResponseValidator.ts

Line 219 in de93b53

result[claim] = [previousValue, value];

Agree on that, i saw this as well. That whole code is not optimal as it makes of an none array (result[claim]) an array. This code is already present in the older library oidc-client. And as you said changing would mean a breaking chance. Maybe we can hide it behind a setting, which defaults to current implementation and when we do a 3.x, we can drop this kind of code/settings?

Yeah, we can do a "legacyMergeClaimsBehavior" that is defaulted to true in this version and will be set to false in the next.

If the new behavior is enabled, I think it should be something like

Given "old" and "new" claims
1. Loop over "old" claims. For each claim, if the value is NOT existing on "new" and 
   it's not one of the required claims, delete it.
2. Loop over "new" claims. For each claim
   a. if the value is an object and "mergeClaims" is enabled, recurse on the object
   b. otherwise, if the value it's different (or non existent), update it (or add it) on the result.

The first loop is something I'm struggling with. I think it's necessary to handle the scenario where, in the updated data, something has been removed for any reason, but at the same time I fear losing important data that it's not actually "required" by the standard.

The first loop is something I'm struggling with. I think it's necessary to handle the scenario where, in the updated data, something has been removed for any reason, but at the same time I fear losing important data that it's not actually "required" by the standard.

Yes, we should definitely not remove stuff that was present in the initial claims.

My understand is that we should preserve initial data and only update, enrich what is new. Important is to keep the data type as is (e.g. previous: object -> merged should be still an object, etc...). The current implementation makes e.g. an string to an array of string, which is very unexpected.

Merging:

• object is trivial: update existing, preserver previous, add new
• if the object is an array i am not so sure whats the best: add new values or replace the whole array though
• string/int (non objects): only update existing

What i also do not like in that code path is the recursion, maybe we can get ride of it by using a stack-based algorithm (e.g. https://haacked.com/archive/2007/03/04/Replacing_Recursion_With_a_Stack.aspx/).

Yes, we should definitely not remove stuff that was present in the initial claims.
My understand is that we should preserve initial data and only update, enrich what is new. Important is to keep the data type as is (e.g. previous: object -> merged should be still an object, etc...). The current implementation makes e.g. an string to an array of string, which is very unexpected.

@pamapa I agree with you, but what can be considered "initial claim"? If loadUserInfo is enabled, "userInfo" claims will be merged from the start and right now "userInfoClaims" and "tokenClaims" are not stored separately.

Imagine a scenario where the user "deletes" their profile picture. The claim (obtained via userInfo) should be disappear / be nulled upon refresh...

A variant of this scenario is where the user "adds" their profile picture. In the old data there's no claim, but in the new data we will have it.

if the object is an array i am not so sure whats the best: add new values or replace the whole array though

As per array merging, I don't have a real use case to think about this. Thinking about hypotetical cases, for example "liked profiles on facebook", the array should be replaced. Do you have any scenario where concatenating new values would be better?

What i also do not like in that code path is the recursion, maybe we can get ride of it by using a stack-based algorithm

I didn't know about stack algorithms, I'll investigate this

..., but what can be considered "initial claim"? If loadUserInfo is enabled, "userInfo" claims will be merged from the start and right now "userInfoClaims" and "tokenClaims" are not stored separately.

initial claim: what is first coming from the id token, then enriched with the user info, if desired.

Is work being done or planned for addressing this issue?

Hey @tbm206 , I started working on the issue but then some other priorities at work happened.

I hope to be able to work on this during this week.

Thanks @marcoreni. Much appreciated

Hi @marcoreni. I see there was some momentum in the linked PRs? When do you expect the work to be merged?

This can now be achieved much simpler, when doing something like (un-tested) within a merge request:

in UserManager.ts

public refreshUserInfo() {
  const logger = this._logger.create("refreshUserInfo");
  const user = await this._loadUser();
  if (user) {
    user.profile = await this._client.getUserInfo(user.access_token, user.profile);
    await this.storeUser(user);
    logger.info("refreshed user info");
    return user; 
  }  
  
  logger.info("user not found in storage");
  return null;
}

To check for user and user.expired can be done in the application code...
Maybe we need to add a check to have the correct claim merge behavior (default: { array: "replace" })