Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refresh userinfo data on demand #877

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

marcoreni
Copy link

Fixes #846 #852

Checklist

  • This PR makes changes to the public API
  • I have included links for closing relevant issue numbers

This PR adds a new parameter "refreshUserInfo" to "loadUser", so that it's possible to refresh (and store) updated userinfo claims for the logged user. loadUser will refresh the token, if expired, otherwise it will simply reload userinfo.
In order for this to be enabled, loadUserInfo should be enabled in the settings.

NOTE: if the token is refreshed during the procedure, old claims that "disappeared" will be removed. Otherwise, they will be kept.

It also:

  • Adds a new configuration property "legacyMergeClaimsBehavior" (defaulted to true for backwards compatibility) that, if disabled, overwrites claims instead of transforming types.
  • Exposes a new getUserInfo method in OidcClient to perform the raw userinfo data retrieval.

LMKWYT

@codecov
Copy link

codecov bot commented Feb 8, 2023

Codecov Report

Attention: Patch coverage is 95.83333% with 3 lines in your changes missing coverage. Please review.

Project coverage is 78.11%. Comparing base (ebe7545) to head (885099d).
Report is 739 commits behind head on main.

Files with missing lines Patch % Lines
src/UserManager.ts 76.92% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #877      +/-   ##
==========================================
+ Coverage   77.69%   78.11%   +0.42%     
==========================================
  Files          44       45       +1     
  Lines        1690     1723      +33     
  Branches      331      338       +7     
==========================================
+ Hits         1313     1346      +33     
- Misses        340      341       +1     
+ Partials       37       36       -1     
Flag Coverage Δ
unittests 78.11% <95.83%> (+0.42%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pamapa
Copy link
Member

pamapa commented Feb 8, 2023

Thanks for providing this. I like the claims parts, yes it makes it easier when we split it in its own service. This code is almost ready to merge. With the refreshUserInfo i am not so happy yet because of the edge case handling (expired, signinSilent).

Idea:

  • keep refreshUserInfo in getUser, but without edge case handling, e..g the caller must take care the User is still valid... if not doing signinSilent, ...

Simple without edge cases:

if (refreshUserInfo) {
    logger.debug("refreshing user info");
    user.profile = await this._client.getUserInfo(user);
    await this.storeUser(user);
    logger.debug("user updated in storage");
}

@pamapa
Copy link
Member

pamapa commented Feb 8, 2023

Would be nice if you could split this up into two merge requests.

@marcoreni
Copy link
Author

keep refreshUserInfo in getUser, but without edge case handling, e..g the caller must take care the User is still valid... if not doing signinSilent, ...

What you added as an example was exactly my first implementation. After that, my reasoning was: UserManager is described as higher level API, so it should help users to obtain what they want without the hassle of multiple checks.

I can definitely simplify this. What do you think should happen if the token expired? We should throw an exception?

Would be nice if you could split this up into two merge requests.

Yeah no problem, you mean one for "splitting the claims service and changing the merge behavior" and the other for "adding the userinfo refresh behavior" ?

@mattsGitHub123
Copy link

@marcoreni Curious if your still working on this? I've got a use case for this functionality and would love to be able to use it.

@pamapa pamapa added the enhancement New feature or request label Apr 4, 2023
@pamapa pamapa changed the title feat: refresh userinfo data on demand refresh userinfo data on demand Apr 4, 2023
@tbm206
Copy link

tbm206 commented Apr 13, 2023

@marcoreni Curious if your still working on this? I've got a use case for this functionality and would love to be able to use it.

Same here

@marcoreni
Copy link
Author

Hi all,

sadly, we did not have time to wait on the discussion on #881, so we forked the library and implemented a basic version of the "on demand update" (that served our purposes). The implementation is available on these two commits but I think it's not ready for merge for the following reasons:
1- it's based on #881 which has been superseded by #904
2- it does not have tests
3- it does not integrate the changes requested above

I don't think I'll have time to tackle this again soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Load userinfo without refreshing tokens
4 participants