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

align OIDC username attribute name lookup #1204

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mdaur
Copy link

@mdaur mdaur commented Nov 12, 2021

Signed-off-by: Martin Daur [email protected]
This fix aligns the behaviour hence OIDC username attribute name lookup at these two places:

  • OidcBearerTokenAuthenticationFilter/doFilter
  • JwtAuthoritiesOidcUserService/loadUser

Additionally, this fix checks the existence of the claim which is set by the Spring user-name-attribute property. Doing so allows OIDC scenarios in which the userNameAttributeName might not be available in case of different OAuth flows. E.g. in Azure AD preferredUsername is available for OAuth 2.0 auth code grant and OAuth 2.0 device code flow but it is not available for Oauth 2.0 client credentials grants with shared secrets or certificates.

To align with the Spring user-name-attribute property (spring.security.oauth2.client.provider) it makes sense to return the name of the OidcUser and not hardcoded the preferredUser (see SpringSecurityAuditorAware).

For backward compatibly you may configure the provider as follows (including the benefit to have a fallback to the sub as the username if there is no such claim):
spring.security.oauth2.client.provider.azure.user-name-attribute=preferred_username

Without these adjustments you may end up with "NULL" usernames at various places (e.g. Created by, Last modified by), which also breaks the UI.

@hawkbit-bot
Copy link

Can one of the admins verify this patch?

@pdomineaux
Copy link

Hello,
Is this pull request will fix the issue with the null in the initiated_by when trying to associate 'distribution set' to taget?

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.

3 participants