BindAuthenticator's attribute-related functionality should be better documented (designed?)

[ghost]

This is really long, but you can stop after the "what I want you to do" section if you want; I figured after doing the work I did I might as well write down all I learned somewhere.

I'll be glad to provide Java code if the explanations become unclear at any point.

What I wanted to do
I've got an LDAP server where some of the user nodes' attributes are readable by the anonymous user but some are only readable by the users themselves. I'm on Linux, so I'm using openldap; the access control part of the configuration looks like

access to * attrs=sn
    by self 
    by * none
  
access to * attrs=userPassword
    by anonymous auth
    by * none
  
access to *
    by * read

When a user authenticates, the application is supposed to show her the attribute which only she can read (sn, in the configuration above).

The problem
This is pretty easy to do, actually. You set userDnPatterns (either on a BindAuthenticator which you define as a Bean, or using LdapAuthenticationProviderConfigurer) and then the application will fetch attributes after authenticating. If your LDAP server is configured as above, you have access to the sn attribute. All is wonderful.

The reason I'm writing this is that if you don't want to use userDnPatterns but want to do a search instead (ie you use BindAuthenticator.setUserSearch or the corresponding method of LdapAuthenticationProviderConfigurer instead of using BindAuthenticator.setUserDnPatterns), this is not easy to do anymore. In fact it is completely impossible to do, as far as I can tell, without defining your own authenticator. It took me hours to arrive at this conclusion, because (again as far as I can tell) this behaviour is not documented.

What I want you to do
Document it. (A good place would be here, since it mentions both authenticators and attributes and is thus easily findable by someone Ctrl-Fing the reference.) Or maybe change it -- more on that below.

Further explanation of the problem
OK, so why does BindAuthenticator behave so differently depending on whether you use setUserSearch or setUserDnPatterns? References to lines of code are to spring-security-ldap-5.3.2.RELEASE.

If we have set userDnPatterns, then in BindAuthenticator.java, lines 84-90, we bind with the user's dn and return all attributes that the user can read. (Part of the reason I wasted so much time is that I originally skipped over these lines...)

If we haven't, then in BindAuthenticator.java, lines 94-98, we first do a search for the user. This search will return some attributes. Then we call BindAuthenticator.bindWithDn; therein (in line 130, to be exact) we notice that the search returned some attributes, and so don't bother fetching any other attributes. But of course the search only fetched the attributes that the anonymous user is allowed to read, and so (given an LDAP server with the access controls given above) sn will not get returned.

One more possible solution
If you decide that this behaviour is unintuitive enough to change rather than document, what I'd do is always request zero attributes when searching and always request all of them after binding. Honestly I'm not sure why this isn't the case already; that's why there's a "(designed?)" in the issue's title. (Maybe because it requires more communication with the server? But authentication is not anyone's bottleneck, surely.) ETA: ok, so this commit explains the reasoning behind it.

...return the union of what the two searches returned, and ignore failures on the second search? I'm kind of reaching here. Maybe just ignore me.

Further notes, maybe an actual bug or two
I originally thought that I'd solve my problem if I did BindAuthenticator.setUserSearch to a search which requested no attributes (ie contained a SearchControls which had an empty attributesToReturn), but that didn't work, because (on my system, anyway) such a search will nevertheless return the objectClass attribute. IMO this is a bug, but I don't know (and don't know enough to find out) whether it's Spring's fault or the fault of (the openjdk implementation of) some low-level Java LDAP API or the fault of openldap. ETA: And I don't even know whether it IS a bug! I'm new to LDAP. Can you tell?

Also, if we have set both userDnPatterns and userSearch, then userSearch is completely ignored, as far as attributes are concerned: we return all attributes the user can read, even if (in the search) we requested fewer/none of them. I wouldn't necessarily call this a bug, but it is perhaps unintuitive enough to document. (Probably in the javadoc for BindAuthenticator.setUserSearch.)

[rwinch]

Thanks for the excellent and well thought out feedback. Could you create multiple tickets for each item? This will allow things to move more quickly. For example, I think the documentation updates will be pretty easy to get in. We can then try to figure out if any behavior changes need to be made and what they are.

[ghost]

Hm, yeah, there are definitely multiple issues here. (You're very welcome, by the way.) Here's what I'd do:

1. Issue for documenting how fetching attributes with BindAuthenticator works
2. Issue for documenting how BindAuthenticator.setUserSearch works (in particular, what bits of the search are ignored)
3. Issue for debating whether changing how BindAuthenticator fetches attributes is worth it
4. Issue for debating whether changing how BindAuthenticator treats its user search is worth it

(The issue for which I say "IMO this is a bug" above I'd probably just leave, honestly; I'm too unclear on what is supposed to happen there -- in particular, I have too little experience with LDAP to know what other tools do and why.)

I'd also make a little github repo you can clone to follow along with my descriptions of the behavior; unfortunately whoever you assign (if anyone) will also need an LDAP server on a machine somewhere -- no in-memory LDAP server I can find supports access control.

Is this all OK with you? I'd like a brief confirmation before I fill your issue board full of stuff which is ultimately not all that important.

[rwinch]

Thank you for your thoughts. I think the way you split up the issues sounds good.

In terms of the samples needing LDAP installed, I think if you can provide clear instructions for the setup that is fine. The easier you make this, the faster we will be able to help. One option might be to point to a docker image and load the data from a file in the sample repository.

[ghost]

So, uh, sorry that this took a literal eternity. I had a fairly busy three weeks, and splitting this up (well) wasn't no work. I should be able to respond more quickly in the future.

The repo I promise above is linked in all but one of the issues.

This issue, you can close; I'm not closing it myself, because if you want to tell me something which spans several of the issues I just created (e. g. you can't get the repo to work, or you can but you want Docker after all) this is probably a good place to do it.