8000 [Ldap] Use discovered DNs for the authentication bind by junowilderness · Pull Request #16827 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Ldap] Use discovered DNs for the authentication bind #16827

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[Ldap] Use discovered DNs for the authentication bind #16827

wants to merge 1 commit into from

Conversation

junowilderness
Copy link
Contributor
Q A
Bug fix? no
New feature? yes - makes the authentication more flexible
BC breaks? yes - when finished, would invalidate the need for the dn_string configuration setting, which has not yet been removed.
Deprecations? no
Tests pass? no - still need fixing
Fixed tickets #16823
License MIT
Doc PR TBD

@junowilderness junowilderness mentioned this pull request Dec 3, 2015
Closed
@fabpot
Copy link
Member
fabpot commented Jan 25, 2016

@csarrazi Can you have a look at this one?

@csarrazi
Copy link
Contributor

I have nothing against the principle, but instead of adding a new interface, and a getUserDn() method, which is specific to LDAP (and no other mechanism), I would rather work with a specialised User class. This way, we could have more specific logic, and even move toward parsing LDAP attributes.

This will be made much easier when #15994 will have been merged.

@csarrazi
Copy link
Contributor

The DN is available from the entry's data, so we should rather set the username to be the user's DN in the UserProvider.

@csarrazi
Copy link
Contributor

Thus, no need to create a new user class, or change the authentication provider, a simple modification of the user provider should be enough.

@junowilderness
Copy link
Contributor Author

If time permits, today.

@junowilderness
Copy link
Contributor Author

This is not complete. We may wish to remove dn_string. But I think this is close to what you intend.

@csarrazi
Copy link
Contributor

The problem is that this will break compatibility with any user provider which does not fetch users from an Ldap server.

This is the actual goal of dn_string. By default, it is equal to {username}, but you can customize it to add the rest of the DN. Something like cn={username},dc=domain,dc=tld or {username}@domain.tld.

The code for the authentication provider needs to work with both:

  • the Ldap user provider
  • third-party user providers

This means that the dn_string should be needed, and that we actually need to do things in two steps:

  • First, try to authenticate using the username from the token, and the dn_string. In this case, your DN string would be cn={username},dc=domain,dc=tld or {username}@domain.tld (your own custom value).
  • Then, try to authenticate using the user provider's username, and the dn_string. In this case, you would configure your DN string to simply be {username} (the default).

Regardless, the user provider part is correct. You just need to adapt the authentication provider, and we should be okay.

@fabpot
Copy link
Member
fabpot commented Mar 4, 2016

@cilefen Will you have time to finish this one in the coming days/weeks? I would love to be able to merge this one before 3.1 code freeze. Thanks.

@junowilderness
Copy link
Contributor Author

@fabpot I regret out of my ignorance I have difficulty with the development process of the project, and although I appreciate the detailed reviews, I am confused about the direction to take, and I have little time to commit to giving this the attention it deserves.

@csarrazi
Copy link
Contributor
csarrazi commented Mar 4, 2016

@cilefen @fabpot Want me to take over the rest of the PR?

@fabpot
Copy link
Member
fabpot commented Jun 9, 2016

@csarrazi Do you have time to take over this one?

@csarrazi
Copy link
Contributor
csarrazi commented Jun 9, 2016

@fabpot Can do, but not until next week, as I am quite busy these days.

@fabpot
Copy link
Member
fabpot commented Jun 9, 2016

@csarrazi Thanks!

@fabpot
Copy link
Member
fabpot commented Jun 13, 2016

Closing for now, waiting for @csarrazi to fix the original issue.

71C3
@fabpot fabpot closed this Jun 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Feature Ldap Status: Needs Work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@junowilderness @fabpot @csarrazi @javiereguiluz @xabbuh @carsonbot
0