Skip to content

Think about changing how BindAuthenticator fetches attributes #8727

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

Open
ghost opened this issue Jun 18, 2020 · 1 comment
Open

Think about changing how BindAuthenticator fetches attributes #8727

ghost opened this issue Jun 18, 2020 · 1 comment
Labels
in: ldap An issue in spring-security-ldap type: enhancement A general enhancement

Comments

@ghost
Copy link

ghost commented Jun 18, 2020

As requested, I am splitting the long and unclear #8560 into several issues. This issue is about (thinking about) changing how BindAuthenticator fetches attributes.

Current behaviour

I've documented the behaviour in #8725. I suspect you'll agree that it's confusing.

Better behaviour

One of:

  • with both bind strategies, we fetch the attributes with the same
    permissions
  • we let the user decide which permissions to fetch attributes with

Context

Once upon a time, BindAuthenticator did use the same permissions -- the user's permissions -- regardless of the bind strategy employed. (This was the case as of a790c7e.) There was a suggestion (#1738) to change this, and later a pull request. Luke Taylor (who wrote the class) said, in the issue, that he would not change this behaviour because (I'm paraphrasing) it would complicate the class, and people who wanted something different could just write their own authenticators/authentication providers. Rob Winch denied the pull request with the same reasoning. This is, incidentally, entirely sensible.

Later, Cloudfoundry had an issue. They wanted to fetch attributes with the app user's permissions. At that point in time that was impossible without writing your own authenticator.

Cloudfoundry decided this was a Spring Security issue. They made a pull request which was accepted (6b436ff). The pull request changed what the search-and-bind strategy did, but did not change what the dnPatterns strategy did. That is why the present behaviour is inconsistent.

How to get better behaviour

First, note that it's not necessarily bad to do nothing. The behaviour is confusing, but it's not buggy. Also, while I'm not necessarily the only person to have run into it, I am (afaik) the only person to have posted an issue about it in 4 years, so it doesn't seem to bother your users much.

If you want to change it, I see several ways to do so:

  • always fetch with the user's permissions (i.e. revert 6b436ff)
  • always fetch with the app user's permissions (probably this entails doing a
    search with object scope when given dnPatterns)
  • give the user (of the class) more choice -- maybe let them configure
    multiple searches, where a search is a root and a scope and a filter and
    an attribute fetching strategy (this is a lot of breaking changes and
    complexity for not very much gain; see, once again, this)

The way I'd recommend is the first one. That pull request, I think, shouldn't have been accepted; after all, #1738, which makes essentially the same request, was declined.

@ghost ghost added status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels Jun 18, 2020
@rwinch rwinch added in: docs An issue in Documentation or samples and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 19, 2020
@rwinch
Copy link
Member

rwinch commented Jun 19, 2020

I think the ship has sailed on reverting the commit. We instead need to find a good way to allow users to customize the behavior.

@rwinch rwinch added in: ldap An issue in spring-security-ldap and removed in: docs An issue in Documentation or samples labels Jun 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: ldap An issue in spring-security-ldap type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

1 participant