Authenticating SAML user by UserDetailService #7550

herrminni · 2019-10-20T08:58:14Z

Added support for authenticating a SAML user by UserDetailsService.
This enhances the AuthenticationProvider by adding UserDetail metadata
to the principal and also set authorities like roles.

Closes gh-7465

Added support for authenticating a SAML user by UserDetailsService. This enhances the AuthenticationProvider by adding UserDetail metadata to the principal and also set authorities like roles. Closes spring-projectsgh-7465

pivotal-issuemaster · 2019-10-20T08:58:16Z

@herrminni Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

pivotal-issuemaster · 2019-10-20T08:58:53Z

@herrminni Thank you for signing the Contributor License Agreement!

fhanik

Thank you very much for contributing. It's much appreciated
I will work with you through this review.

fhanik · 2019-11-05T19:09:03Z

...g/springframework/security/config/annotation/web/configurers/saml2/Saml2LoginConfigurer.java

+		OpenSamlAuthenticationProvider provider = new OpenSamlAuthenticationProvider();
+		if (this.userDetailsService != null) {
+			provider.setUserDetailsService(this.userDetailsService);
+		}


I like that you can explicitly set it. In addition to this, what if there is a

@Bean UserDetailsService

in the context. We should pick up that bean too

Hello Fillip,
thank you for your review!

I like your idea of automatically using the UserDetailsService from @Bean registry, if this is present.

However my implementation of manually setting (or maybe overriding) to a specific UserDetailsService was explicitly intended by me:
This way you would be able to have two different UserDetailsServices at the same time and handle SAML authentication in a different way. For example, you could explicitly create a new User as a sideeffect, if that one was not known in a local database. The other UserDetailsService could be used to only authenticate already known users an authenticate by username+password.

Maybe we could combine your and my idea:
User the spring @Bean UserDetailsService as default and also be able to overrride it with the method I already included (setUserDetailsService)?

fhanik · 2019-11-05T19:11:34Z

...fig/saml2login/src/main/java/org/springframework/security/samples/config/SecurityConfig.java

@@ -40,27 +45,43 @@
 public class SecurityConfig extends WebSecurityConfigurerAdapter {

 	RelyingPartyRegistration getSaml2AuthenticationConfiguration() throws Exception {
-		//remote IDP entity ID
+		// remote IDP entity ID


Nothing wrong with reformatting as part of a PR.
Would you be able to put that into a separate commit, one that doesn't change functionality, so that reviewers can focus on what actually changed?

Hi Fillip,
I used the suggested formatter config for Eclipse, which is part of spring-security from the file etc/eclipse/eclipse-code-formatter.xml.
Reverting the automatic formatting for the files I changed would have been too much manual work, and I had many local test-builds that failed because of the formatting. So I decided to commit the files with all the automatic changes from the eclipse-code-formatter that went through the test.

fhanik · 2019-11-05T19:12:51Z

...fig/saml2login/src/main/java/org/springframework/security/samples/config/SecurityConfig.java

+				.assertionConsumerServiceUrlTemplate(acsUrlTemplate).build();
+	}
+
+	private UserDetailsService userDetailsServiceMockup() {


We'd like to keep the samples to bare minimum.
I will work with you to find a better place for this configuration and how to test it (I'm working on that right now)

Sure, it just felt right for me, to add a sample.
Feel free to create a second example, that just uses the UserDetailService.
Let me know, if I can help!

mballoni · 2019-11-14T12:11:13Z

Wow I was looking exactly for it!
In case it gets merged soon, at which version it would be available ?

mballoni · 2019-11-25T15:56:33Z

@herrminni are you still planning on working on this PR?
I can pick up from were you left if you prefer.

herrminni · 2019-11-26T10:16:35Z

@herrminni are you still planning on working on this PR?
I can pick up from were you left if you prefer.

@mballoni As you can see, I replied to the review comments of @fhanik
I'm not sure, how the review process will go on now.
Please feel free to add/change the PR yourself. I am happy to support you and Fillip on this

mballoni · 2019-12-02T16:55:55Z

@herrminni Thank you for pointing it out!

@fhanik fixing the merge conflicts would be enough for this PR or should we look deeper into some of those feedbacks?

fhanik · 2019-12-02T17:02:27Z

I think we'll be able to simplify this PR a lot after these two get merged

Those two PRs will get us

A testing framework for testing changes to Saml2LoginConfigurer
A Saml2AuthenticatedPrincipal interface that we will be able to extend
The ability to configure the authentication manager via a resolver

Once those two get through, we can rework this one, is that ok?

mballoni · 2019-12-02T17:29:47Z

Thank you for your quick response @fhanik .

Yes, that works for me.
Let me know if I can help with something.

I appreciate.

mballoni · 2020-01-09T16:56:34Z

@fhanik hi!
Now that both of those issues were merged can we continue with this one?
Can I pick up this PR and take core of the merges or should I wait/do something else?

Thank you, I appreciate your patience.

fhanik · 2020-01-09T17:21:49Z

hi @mballoni - yes, please pick it up. I'll work with you on the review process.

DEEPAKRDEEPS · 2020-02-23T16:16:34Z

Can I know when this will be get released?

fhanik · 2020-02-24T20:42:26Z

@mballoni If you're still interested, take a look at the Oauth2Login implementation.

The user details get loaded here[1]

Creating an implementation that makes similar choices to this implementation will make the code base easy to follow.

I'm closing this PR as the code has diverted away from this. We can attach the work to:
#8010

[1]

spring-security/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/authentication/OAuth2LoginAuthenticationProvider.java

Lines 116 to 117 in 3b89754

    
           OAuth2User oauth2User = this.userService.loadUser(new OAuth2UserRequest( 
        
           		authorizationCodeAuthentication.getClientRegistration(), accessToken, additionalParameters));

Authenticating SAML user by UserDetailService

b1f2946

Added support for authenticating a SAML user by UserDetailsService. This enhances the AuthenticationProvider by adding UserDetail metadata to the principal and also set authorities like roles. Closes spring-projectsgh-7465

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 20, 2019

eleftherias assigned fhanik Nov 5, 2019

fhanik self-requested a review November 5, 2019 19:03

fhanik reviewed Nov 5, 2019

View reviewed changes

fhanik added in: saml2 An issue in SAML2 modules type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Dec 2, 2019

fhanik closed this Feb 24, 2020

geirhe mentioned this pull request Nov 15, 2023

Gh 7750 implement jdbcuserpassworddetailsmanager #14148

Closed

geirhe added a commit to geirhe/spring-security that referenced this pull request Jan 21, 2024

spring-projectsgh-7550: gradle format

2c917f6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Authenticating SAML user by UserDetailService #7550

Authenticating SAML user by UserDetailService #7550

herrminni commented Oct 20, 2019

pivotal-issuemaster commented Oct 20, 2019

pivotal-issuemaster commented Oct 20, 2019

fhanik left a comment

fhanik Nov 5, 2019

herrminni Nov 5, 2019 •

edited

fhanik Nov 5, 2019

herrminni Nov 5, 2019

fhanik Nov 5, 2019

herrminni Nov 5, 2019

mballoni commented Nov 14, 2019

mballoni commented Nov 25, 2019

herrminni commented Nov 26, 2019

mballoni commented Dec 2, 2019

fhanik commented Dec 2, 2019

mballoni commented Dec 2, 2019

mballoni commented Jan 9, 2020

fhanik commented Jan 9, 2020

DEEPAKRDEEPS commented Feb 23, 2020

fhanik commented Feb 24, 2020

Authenticating SAML user by UserDetailService #7550

Authenticating SAML user by UserDetailService #7550

Conversation

herrminni commented Oct 20, 2019

pivotal-issuemaster commented Oct 20, 2019

pivotal-issuemaster commented Oct 20, 2019

fhanik left a comment

Choose a reason for hiding this comment

fhanik Nov 5, 2019

Choose a reason for hiding this comment

herrminni Nov 5, 2019 • edited

Choose a reason for hiding this comment

fhanik Nov 5, 2019

Choose a reason for hiding this comment

herrminni Nov 5, 2019

Choose a reason for hiding this comment

fhanik Nov 5, 2019

Choose a reason for hiding this comment

herrminni Nov 5, 2019

Choose a reason for hiding this comment

mballoni commented Nov 14, 2019

mballoni commented Nov 25, 2019

herrminni commented Nov 26, 2019

mballoni commented Dec 2, 2019

fhanik commented Dec 2, 2019

mballoni commented Dec 2, 2019

mballoni commented Jan 9, 2020

fhanik commented Jan 9, 2020

DEEPAKRDEEPS commented Feb 23, 2020

fhanik commented Feb 24, 2020

herrminni Nov 5, 2019 •

edited