Skip to content

Conversation

sberyozkin
Copy link
Member

@sberyozkin sberyozkin commented May 22, 2025

This is a minor update to make it possible to get the user name using the same code with both authorization code and bearer token flows, when dealing with social provider tokens. I noticed it had to be tuned when working on the latest demo.

For example, given a GitHub token, if it is an authorization code flow login, then the user name can only be acquired from quarkus.oidc.UserInfo, as shown here, getting it from SecurityIdentity does not work.

But if it is a bearer access token, then using either UserInfo or SecurityIdentity (as shown here) works.

Ideally, the securityIdentity.getPrincipal().getName() option should also work for tokens acquired from OAuth2 social providers during the authorization code flow too.

The reason it currently does not is that in case of the code flow, an internal ID token is generated which is then used as a source for a principal name, while it is only available for OAuth2 providers in the UserInfo json.

So I did a minor update to fix it.

The updated test confirms it - note, that before, the augmentor was manually updating the principal to pick up the UserInfo property, for a test GitHub provider. It is no longer necessary if UserInfo contains the configured principal claim

@gastaldi gastaldi requested a review from Copilot May 22, 2025 15:28
Copy link
Contributor

@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 enables the OAuth2 code-flow to populate the principal name from a user-info claim (e.g., preferred_username) so that securityIdentity.getPrincipal().getName() works with social providers in both code and bearer flows.

  • Added a new token.principal-claim property for the GitHub provider in tests
  • Removed the custom augmentor override for the code-flow GitHub path
  • Extended the identity provider to copy the configured principal claim from UserInfo into the token JSON

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
integration-tests/oidc-wiremock/src/main/resources/application.properties Added quarkus.oidc.code-flow-user-info-github.token.principal-claim
integration-tests/oidc-wiremock/src/main/java/io/quarkus/it/keycloak/CustomSecurityIdentityAugmentor.java Removed the code-flow-user-info-github branch from the augmentor
extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcIdentityProvider.java Populates the principal claim from UserInfo into the token JSON
Comments suppressed due to low confidence (1)

integration-tests/oidc-wiremock/src/main/java/io/quarkus/it/keycloak/CustomSecurityIdentityAugmentor.java:45

  • [nitpick] Update or remove the surrounding comment/documentation to reflect that code-flow-user-info-github is now handled by configuration rather than the custom augmentor, preventing confusion.
|| routingContext.normalizedPath().endsWith("code-flow-user-info-github")

Comment on lines +466 to +470
final String principalClaim = resolvedContext.oidcConfig().token().principalClaim().orElse(null);
if (principalClaim != null && !tokenJson.containsKey(principalClaim) && userInfo != null
&& userInfo.contains(principalClaim)) {
tokenJson.put(principalClaim, userInfo.getString(principalClaim));
}
Copy link
Preview

Copilot AI May 22, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider using the Optional API (ifPresent) instead of orElse(null) to avoid nulls and improve readability, for example:

resolvedContext.oidcConfig()
  .token()
  .principalClaim()
  .ifPresent(claim -> { ... });
Suggested change
final String principalClaim = resolvedContext.oidcConfig().token().principalClaim().orElse(null);
if (principalClaim != null && !tokenJson.containsKey(principalClaim) && userInfo != null
&& userInfo.contains(principalClaim)) {
tokenJson.put(principalClaim, userInfo.getString(principalClaim));
}
resolvedContext.oidcConfig().token().principalClaim().ifPresent(principalClaim -> {
if (!tokenJson.containsKey(principalClaim) && userInfo != null
&& userInfo.contains(principalClaim)) {
tokenJson.put(principalClaim, userInfo.getString(principalClaim));
}
});

Copilot uses AI. Check for mistakes.

Copy link
Member Author

Choose a reason for hiding this comment

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

@gastaldi I guess I'll ignore this one though with the lambda expression :-)

@sberyozkin sberyozkin force-pushed the oidc_social_provider_misc_updates branch from 144848a to 51b8e04 Compare May 22, 2025 15:42
Copy link

quarkus-bot bot commented May 22, 2025

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 51b8e04.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@sberyozkin sberyozkin merged commit c179291 into quarkusio:main May 22, 2025
28 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.24 - main milestone May 22, 2025
@sberyozkin sberyozkin deleted the oidc_social_provider_misc_updates branch May 22, 2025 16:42
@gsmet gsmet modified the milestones: 3.24 - main, 3.23.1 Jun 3, 2025
@jmartisk jmartisk modified the milestones: 3.23.1, 3.20.2 Jun 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants