Codecov Report

Attention: Patch coverage is 33.00000% with 67 lines in your changes are missing coverage. Please review.

Project coverage is 69.18%. Comparing base (e327bf2) to head (1a5e4f2).
Report is 7 commits behind head on master.

@@             Coverage Diff              @@
##             master     #310      +/-   ##
============================================
- Coverage     73.35%   69.18%   -4.18%     
- Complexity      211      220       +9     
============================================
  Files            10       11       +1     
  Lines           882      980      +98     
  Branches        124      138      +14     
============================================
+ Hits            647      678      +31     
- Misses          172      232      +60     
- Partials         63       70       +7     

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

Hi @michael-doubez, this is a working draft where the most basic and common functionality works:

• Added "Enable/disable refresh token" support - a simple checkbox when "manual config", detected when "automatic config" by querying "supproted_grant_types" discovery response attribute
• Added "strict" mode (disabled by default) that indicates whether an expired access token and missing refresh token, or simply failed refresh request (due to various reasons) will result in "401" or we simply ignore it
• Added configurable clock skew that prolongs lifetime of an access token for a pre-configured amount of seconds to adjust for any time sync issues. Defaults to 60 seconds
• Any updated roles/groups are reflected on every token refresh, as they should be

Now, before I move on to update documentation and write some tests, I'd like to get some feedback if the approach is correct. In addition, I have additional points that I have noticed but not sure if I should handle or how to handle at all.

• First and most obvious point: In the build pipeline there is a warning that "accessToken" and "refreshToken" look sensitive and may be stored in plain text files. I have no idea where the SecurityContext is stored, so do you think we should mark the tokens as credentials?
• There is a concurrency issue where two parallel requests will trigger two token refresh requests at the same time. I am unable to find a good place for a synchronization object that does not involve a manual clean up. Should I take care about this? If yes, do you happen to know if there's any place that I could place a synchronization primitive such as timestamp of first request that triggered the refresh, and to let any subsequent parallel requests just skip the refresh as it's "being handled"? Is there any "self-cleaning" cache used elsewhere in the project that I could hook into?

Just for fun, without any activity I keep my jenkins tab opened and UI triggers a lot of requests every x seconds, resulting in at least 2 token refresh requests without any activity

• When a Token is expired, no refresh can be performed, and strict mode is on, should we simply redirect the user to the logout URL or do we clean up the session manually and simply "halt"?

Any additional feedback is welcome.

Hello,

Thanks for the work. You can ignore the warnings about secret leaking, they are mostly irrelevant.
I'll check the concurrency issue but if the information is local to a user context, there shouldn't be any issue.

There are a lot of features in the PR. I would break them down into:

• handle session timeout: if refresh/offline token is unavailable or expired, how to clear session information and request re-login
• handle additional (refresh) token lifetime : how to request it if available, as you did. It should be store in session information such as to be available globally, it may also be revoked at logout because lifetime is typically longer than access token
• handle refresh of user info

I should be able to review in depth and (hopefuly) provide relevant feedback next Sunday.

Hello,

I believe I have handled most of the use-cases except the logout. The question remains on how do we proceed.

Answering the points in the order they were raised

• Easiest way to do this is to request log out at IdP. This is an already supported feature. This will invalidate the refresh token, and next refresh will fail. This use-case has been handled and works without clearing the Jenkins related session stuff (as of yet - we have to agree on how to proceed)
• Refresh tokens may be extended indefinitely as long as the session is active on the IdP. I wouldn't go beyond what was done here as described in step 1 - refresh token returns "invalid_grant" -> assume RT is expired or revoked -> destroy the user session. User may be logged elsewhere out of IdP and detecting this is a bit more bothersome than I'd like to go into - but there are standards on how to handle this scenario (see below)
• Refresh of user info and ID token has already been incorporated in the MR and I re-used the existing userinfo request code and IdToken decoding (without the validations - but that can be re-used if needed, as well)

Back to the first point, we can implement additional features if needed, namely

• We implement Token Revocation on manual logout when redirect to IdP has been disabled https://datatracker.ietf.org/doc/html/rfc7009
• We add support for OpenID Front and/or Back Channel Logout https://openid.net/specs/openid-connect-frontchannel-1_0.html / https://openid.net/specs/openid-connect-backchannel-1_0.html
• We add support OpenID Session Management and check_session_iframe, although I'm not sure how widely it's supported. Not even latest Spring Security supports it https://openid.net/specs/openid-connect-session-1_0.html

@krezovic the change looks good to me.

What bothers me most is the idtoken kept in the session information. It should be removed and kept only in the credentials but that can be ironed out later.