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

Use username mapper to resolve username when creating a default user #40

Merged

Conversation

denniscoorn
Copy link
Contributor

This pull request originates from the situation where I had the force option enabled in the firewall and then wanted to control the username resolving by setting my own list of attributes on the light_saml_sp.username_mapper configuration.

This wasn't working and it turned out that the username mapper was never used when the LightsSamlSpAuthenticationProvider wants to create a default user. In fact, the logic used in the createDefaultUser method looks very similar to the logic of the SimpleUsernameMapper. As far as my interpretation of this bundle goes there's no necessity why this logic should be duplicated and could possibly cause unexpected behavior, as described above.

That's why I'm proposing to use the username mapper for resolving the username when a default user should be created. In my opinion this change has three main benefits:

  • There's less unexpected behavior because there's a single source of 'truth' when it comes to resolving a username.
  • The configuration options to manipulate the username mapper can now also be used when creating a default user.
  • Because the logic for resolving a username is more or less the same between the removed lines of code and the SimpleUsernameMapper, this will most likely not affect current implementations.

Would like to know what you think of it!

* Username mapper is being used for resolving the username when a default user should be created
* Two small adjustment to the LightsSamlSpAuthenticationProviderTest to make sure the username mapper is being called without breaking other behavior
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 98.98% when pulling 325b99a on Karify:feature/create-default-user-username-mapper into 2cb00e7 on lightSAML:master.

@tmilos
Copy link
Member

tmilos commented Mar 3, 2017

Yes, we could use it, had ago such idea but lost it. Thanks

@tmilos tmilos merged commit f0018bb into lightSAML:master Mar 3, 2017
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.

None yet

3 participants