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

fix(OIDC): Use new access_token after refresh #4853

Merged
merged 2 commits into from
Apr 8, 2025
Merged

Conversation

elawende
Copy link
Contributor

Context

When using OIDC authorisation with automaticSilentRenew: true, OHIF keeps using the old OIDC access_token after the token has been refreshed. Requests with an expired access_token fail with a 401 or 403 response.

See https://community.ohif.org/t/fix-user-state-not-updating-during-token-refresh-in-oidc-with-keycloak-and-automaticsilentrenew-true-configuration/1065

Changes & Results

In OpenIdConnectRoutes.tsx, set the user in the userAuthenticationService after a new token is available.

Testing

Use a setup with OIDC and automaticSilentRenew: true. When starting OHIF, a token will be requested and the response of the token endpoint will look like this:

{
    "access_token": "eyJ...VOT3",
    "token_type": "bearer",
    "expires_in": 300,
    "refresh_token": "eyJh...g1xB",
    "id_token": "eyJh...gq5X"
}

Wait until the access token has expired (based on expires_in, which are seconds) and then perform a search. The request should have an authorization header with the new acccess token (from the last refresh) instead of the expired one.

Please note that the value of expires_in varies among OIDC providers. It may be 5 minutes (as required by IHE's IUA profile, this is also the default value in Keycloak), but it may also be hours or even days.

Checklist

PR

  • My Pull Request title is descriptive, accurate and follows the
    semantic-release format and guidelines.

Code

  • My code has been well-documented (function documentation, inline comments,
    etc.)

Public Documentation Updates

  • The documentation page has been updated as necessary for any public API
    additions or removals.

Tested Environment

  • OS: Debian 12 (server), Windows 11 (client)
  • Node version: 20
  • Browser: Chrome 134.0.6998.89, Firefox 136.0.1

Copy link

netlify bot commented Mar 14, 2025

Deploy Preview for ohif-dev ready!

Name Link
🔨 Latest commit 6d91a25
🔍 Latest deploy log https://app.netlify.com/sites/ohif-dev/deploys/67dae00cb9b4990008d87302
😎 Deploy Preview https://deploy-preview-4853--ohif-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Mar 14, 2025

Deploy Preview for ohif-platform-docs failed. Why did it fail? →

Name Link
🔨 Latest commit 6d91a25
🔍 Latest deploy log https://app.netlify.com/sites/ohif-platform-docs/deploys/67dae00cf90b4700089cb0fb

@sedghi sedghi requested a review from Copilot March 19, 2025 15:47
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR addresses the issue where OHIF continued using an expired access token by updating the user in the authentication service after a token refresh.

  • Added a useEffect hook that registers a userLoadedHandler to update the userAuthenticationService when a new token is loaded.
  • Ensured proper cleanup of the event handler on component unmount.

@andylinton
Copy link

Any way I can implement this?

@elawende
Copy link
Contributor Author

elawende commented Apr 8, 2025

@andylinton I'm currently using a fork with this fix, and ran my own build.
@sedghi Is the merge blocked because of the failing deploy/netlify? I did not experience build issues on my local env and can't think of a way how the "Can't resolve '@radix-ui/react-hover-card'" error is related to my change. Is there something I can do to push this PR further?

@sedghi
Copy link
Member

sedghi commented Apr 8, 2025

I will merge today, sorry

@sedghi sedghi merged commit cfa202e into OHIF:master Apr 8, 2025
4 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants