Skip to content

AD-bind-principal: New abstraction for ActiveDirectory LDAP auth#246

Closed
Turbots wants to merge 1 commit intospring-projects:masterfrom
Turbots:AD-bind-principal
Closed

AD-bind-principal: New abstraction for ActiveDirectory LDAP auth#246
Turbots wants to merge 1 commit intospring-projects:masterfrom
Turbots:AD-bind-principal

Conversation

@Turbots
Copy link
Copy Markdown

@Turbots Turbots commented Dec 17, 2015

Most use cases don't require another implementation than the standard
way of binding and searching for a user in Active Directory.

However, sometimes there is a different implementation needed
for creating the principal string for binding a user and the principal
string for searching a user.

The new abstract class basically contains the same methods
as the old final class and contains two abstract methods
for creating a bindPrincipal and searchPrincipal string, respectively.

The new abstract class allows for users to create a different
implementation according to their needs, based on the given
authentication token (or a subclass of the authentication token).

@Turbots
AD-bind-principal: New abstraction for ActiveDirectory LDAP auth
7a9d59f 
Most use cases don't require another implementation than the standard
way of binding and searching for a user in Active Directory.

However, sometimes there is a different implementation needed
for creating the principal string for binding a user and the principal
string for searching a user.

The new abstract class allows for users to create a different
implementation according to their needs, based on the given
authentication token (or a subclass of the authentication token).
@Turbots
Copy link
Copy Markdown
Author

Turbots commented Dec 17, 2015

I have signed and agree to the terms of the SpringSource Individual Contributor License Agreement.

@pivotal-issuemaster
Copy link
Copy Markdown

pivotal-issuemaster commented Jun 9, 2016

@Turbots Please sign the Contributor License Agreement!

@Turbots
Copy link
Copy Markdown
Author

Turbots commented Jun 9, 2016

I have signed the (new?) contributor license agreement

@pivotal-issuemaster
Copy link
Copy Markdown

pivotal-issuemaster commented Jun 15, 2016

@Turbots 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
Copy link
Copy Markdown

pivotal-issuemaster commented Jun 15, 2016

@Turbots Thank you for signing the Contributor License Agreement!

@rwinch rwinch modified the milestone: 4.2.0 M1 Aug 15, 2016
@jgrandja jgrandja self-assigned this Aug 31, 2016
@jgrandja
Copy link
Copy Markdown
Contributor

jgrandja commented Sep 1, 2016

Hi @Turbots. Thank you for submitting this PR !

This is definitely a feature users are asking for so we appreciate you submitting your proposal.

The AbstractActiveDirectoryLdapAuthenticationProvider implementation you provided will definitely allow the user to adapt the user/CN used for the bind operation as well will allow the same/different user/CN for the search operation.

I'm not sure if you had a chance to look at the other implementation of AbstractLdapAuthenticationProvider being LdapAuthenticationProvider?

This specific implementation uses the strategy LdapAuthenticator, for example, BindAuthenticator. I'm wondering if this strategy is the way to go for ActiveDirectoryLdapAuthenticationProvider?

An option would be to have a default implementation of LdapAuthenticator similar to the BindAuthenticator. It would either use the credentials from the incoming Authentication when authenticate() is called OR the credentials passed into the constructor at creation time. The latter option would suit the use case where the same 'system account' is used each time for the bind operation.

Furthermore, the LdapAuthenticator implementation could leverage a LdapUserSearch strategy for the search operation. The existing FilterBasedLdapUserSearch can be reused and it allows the user to provide a custom searchFilter.

What are your thoughts on this implementation strategy?

@jgrandja jgrandja added the status: waiting-for-feedback We need additional information before we can continue label Sep 1, 2016
@jgrandja jgrandja mentioned this pull request Sep 19, 2016
Closed
@jgrandja
Copy link
Copy Markdown
Contributor

jgrandja commented Sep 19, 2016

Thanks again for submitting this PR @Turbots.

We are currently re-thinking our approach to an Active Directory AuthenticationProvider implementation. Our direction is to deprecate ActiveDirectoryLdapAuthenticationProvider and leverage the existing LdapAuthenticationProvider and provide a custom strategy of LdapAuthenticator for Active Directory.

The work has already started in #4064 if you would like to track the progress and provide any input/feedback.

I'm going to close this PR as we will address your input/feedback in #4064.
Thank you.

@jgrandja jgrandja closed this Sep 19, 2016
@jgrandja jgrandja added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-feedback We need additional information before we can continue labels Sep 19, 2016
@jgrandja jgrandja removed this from the 4.2.0 M1 milestone Sep 19, 2016
@Turbots Turbots deleted the AD-bind-principal branch September 20, 2016 23:44
@Turbots
Copy link
Copy Markdown
Author

Turbots commented Sep 20, 2016

Thanks for the feedback! I'm glad to see people are continuing (or at least improving) upon my work 👍

I'll keep an eye on the new issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@jgrandja jgrandja

Labels

status: declined A suggestion or change that we don't feel we should currently apply

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Turbots @pivotal-issuemaster @jgrandja @rwinch