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

Add support for nested property names in oauth2.providers.userNameAttribute #14186

Closed
leeavital opened this issue Nov 22, 2023 · 7 comments
Closed
Assignees
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement

Comments

@leeavital
Copy link

Expected Behavior

I was trying to configure my app to use the pagerduty oauth2 provider, I was using the following configuration:

spring:
  security:
    oauth2:
      client:
        registration:
          pagerduty:
            provider: pagerduty
            clientId: <redacted>
            clientSecret: <redacted>
            authorizationGrantType: authorization_code
            redirectUri: "{baseUrl}/login/oauth2/code/{registrationId}"
            clientAuthenticationMethod: client_secret_post

        provider:
          pagerduty:
            authorizationUri: "https://identity.pagerduty.com/oauth/authorize"
            tokenUri: "https://identity.pagerduty.com/oauth/token"
            userInfoUri: "https://api.pagerduty.com/users/me"
            userNameAttribute:  user.email

The pagerduty /me api returns users with all the interesting properties nested under the user field, like so:

{
  "user": {
    "id": ...,
    "email": "fancy@pants.com",

I tried setting userNameAttribute: user.email expecting the name field to be extracted as the name property on the user object. But I get an error from DefaultOAuth2User: "Missing attribute 'user.email' in attributes.

Current Behavior

Ideally a user would successfully be extracted, and login would be successful.

Context

I wound up exposing a custom OAuth2UserService class, but it's 90% of a copy paste of DefaultOAuth2UserService, and this seems like something that another user info API might reasonably do.

@leeavital leeavital added status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels Nov 22, 2023
@jzheaux
Copy link
Contributor

jzheaux commented Nov 27, 2023

Hi, @leeavital, thanks for the suggestion. I think this sounds reasonable.

I believe three steps are needed to make this work.

First, DefaultOAuth2User should have a new constructor and deprecate the existing one. The new one should look like this:

public DefaultOAuth2User(Map<String, Object> attributes, Collection<? extends GrantedAuthority> authorities, String name)

Second, Spring Security should update existing production code (not tests) to use the new constructor.

Third, DefaultOAuth2UserService should change to process the userNameAttribute as a simple SpEL expression like so:

SimpleEvaluationContext context = SimpleEvaluationContext
		.forPropertyAccessors(new MapAccessor())
		.withRootObject(userAttributes).build();
SpelExpressionParser parser = new SpelExpressionParser();
Expression expression = parser.parseExpression(userNameAttributeName);
String name = (String) expression.getValue(context);
return new DefaultOAuth2User(userAttributes, authorities, name);

Would you be interested in submitting a PR to make these changes? If not, I can make the changes instead and invite you to review the PR.

@jzheaux jzheaux self-assigned this Nov 27, 2023
@jzheaux jzheaux added status: waiting-for-feedback We need additional information before we can continue in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) and removed status: waiting-for-triage An issue we've not yet triaged labels Nov 27, 2023
@spring-projects-issues
Copy link

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

@spring-projects-issues spring-projects-issues added the status: feedback-reminder We've sent a reminder that we need additional information before we can continue label Dec 4, 2023
@ahmd-nabil
Copy link
Contributor

@jzheaux I think I can do that. is it ok if I work on it ?

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue status: feedback-reminder We've sent a reminder that we need additional information before we can continue labels Dec 8, 2023
@jzheaux
Copy link
Contributor

jzheaux commented Dec 8, 2023

Sounds good, @ahmd-nabil

@jzheaux jzheaux removed the status: feedback-provided Feedback has been provided label Dec 8, 2023
ahmd-nabil added a commit to ahmd-nabil/spring-security that referenced this issue Dec 13, 2023
Closes spring-projectsgh-14186

Signed-off-by: ahmd-nabil <ahm3dnabil99@gmail.com>
ahmd-nabil added a commit to ahmd-nabil/spring-security that referenced this issue Jan 19, 2024
ahmd-nabil added a commit to ahmd-nabil/spring-security that referenced this issue Jan 23, 2024
Closes spring-projectsgh-14186

Signed-off-by: ahmd-nabil <ahm3dnabil99@gmail.com>
ahmd-nabil added a commit to ahmd-nabil/spring-security that referenced this issue Jan 23, 2024
Closes spring-projectsgh-14186

Signed-off-by: ahmd-nabil <ahm3dnabil99@gmail.com>
ahmd-nabil added a commit to ahmd-nabil/spring-security that referenced this issue Jan 23, 2024
Closes spring-projectsgh-14186

Signed-off-by: ahmd-nabil <ahm3dnabil99@gmail.com>
@jzheaux
Copy link
Contributor

jzheaux commented Jan 30, 2024

Closing in favor of #14265

@jzheaux jzheaux closed this as completed Jan 30, 2024
jzheaux added a commit to ahmd-nabil/spring-security that referenced this issue Jan 30, 2024
- Add Tests
- Add Reactive Support

Issue spring-projectsgh-14186
jzheaux pushed a commit that referenced this issue Jan 30, 2024
Closes gh-14186

Signed-off-by: ahmd-nabil <ahm3dnabil99@gmail.com>
jzheaux added a commit that referenced this issue Jan 30, 2024
- Add Tests
- Add Reactive Support

Issue gh-14186
@Julius-Babies
Copy link

Julius-Babies commented Apr 14, 2024

Hello,
when setting my user-name-attribute to data.email, which is the correct key for my case[^1], I still get the exception Missing attribute 'data.email' in attributes. The error occurred in ...core.user.DefaultOAuth2User. I took a look at your PR but I discovered, that you didn't change this file. My spring version is at 3.2.4. Did I misconfigure something or can I force it to use your class which should be inside org...oauth2.client.userinfo....?

When adding a breakpoint in IntelliJ, I discovered, that the returned object for my userinfo is a LinkedHashMap containing more LinkedHashMaps, so the returned data by the server is correct.

I don't know if this issue covered DefaultOAuth2User.java. I suggest defining some separator character (maybe /, I don't which chars are allowed in JSON keys) and searching for them recursively.


1: This is what the data looks like:

{
  "data": {
    "email": "users email address",
    "more": "data"
  },
  "meta": {
    "other": "data"
  }
}

@Julius-Babies
Copy link

Ok, sorry for this, I didn't realise that this wasn't included in the Spring Boot Starter release. Here is a working example with this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

5 participants