Conversation
Signed-off-by: Carlos Feria <2582866+carlosthe19916@users.noreply.github.com>
Reviewer's guide (collapsed on small PRs)Reviewer's GuideDisables the OIDC client’s /loaduser call by setting loadUserInfo to false in the client configuration, relying on user information from the token response instead to prevent flow failures when /loaduser is unavailable (e.g., in Azure). Sequence diagram for OIDC login flow with loadUserInfo disabledsequenceDiagram
actor User
participant WebApp
participant OIDCProvider
User->>WebApp: Initiate login
WebApp->>OIDCProvider: Authorization request (response_type=code)
OIDCProvider-->>WebApp: Authorization code
WebApp->>OIDCProvider: Token request (code)
OIDCProvider-->>WebApp: Token response (includes user info)
alt loadUserInfo_true (previous behavior)
WebApp->>OIDCProvider: GET /loaduser
alt loaduser_success
OIDCProvider-->>WebApp: User profile
WebApp-->>User: Authenticated session established
else loaduser_failure
OIDCProvider-->>WebApp: Error
WebApp-->>User: OIDC flow fails
end
else loadUserInfo_false (current behavior)
Note right of WebApp: /loaduser is not called
WebApp-->>User: Authenticated session established using token user info
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider adding a brief code comment next to
loadUserInfo: falseexplaining the Azure/Keycloak rationale so future maintainers understand why this deviates from the library’s default behavior. - If other OIDC providers may still require
/loaduser, you might want to makeloadUserInfoconfigurable (e.g., via ENV) rather than hard-coding it tofalseto avoid regressions for non-Keycloak setups.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider adding a brief code comment next to `loadUserInfo: false` explaining the Azure/Keycloak rationale so future maintainers understand why this deviates from the library’s default behavior.
- If other OIDC providers may still require `/loaduser`, you might want to make `loadUserInfo` configurable (e.g., via ENV) rather than hard-coding it to `false` to avoid regressions for non-Keycloak setups.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #975 +/- ##
==========================================
- Coverage 66.52% 66.50% -0.02%
==========================================
Files 218 218
Lines 3827 3828 +1
Branches 873 873
==========================================
Hits 2546 2546
- Misses 947 950 +3
+ Partials 334 332 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi Carlos, I discussed the work required to have RHPTA working with Azure Entra ID this morning and we need an additional Helm Chart property OIDC_USER_INFO that is a boolean and it defaults to true. Can you please update this PR so that the loadUserInfo setting is set from the Helm Chart property? Phil |
Signed-off-by: Carlos Feria <2582866+carlosthe19916@users.noreply.github.com>
|
@PhilipCattanach ok, I made changes so there is a new Env Due to the current architecture where the UI is served by the Actix Rust server, once this PR is merged a change will need to be done in the backend to inject this new env. So the flow will be:
|
Signed-off-by: Carlos Feria <2582866+carlosthe19916@users.noreply.github.com> (cherry picked from commit 2007d2c)
|
Successfully created backport PR for |
Fixes: https://redhat.atlassian.net/browse/TC-3623
loadUserInfo=trueforces the endpoint /loaduser to be fetched and due to internals of the library the whole oidc flow fails in case /loaduser fails.In Keycloak it is not needed to call /loaduser because the username is part of the /token response already so there is no harm on disabling it for be benefit of making it work in Azure
Summary by Sourcery
Bug Fixes: