Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/304#discussion_r197604999
--- Diff:
extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java
---
@@ -283,7 +284,12 @@ public AuthenticatedUser authenticateUser(Credentials
credentials)
try {
// Get LDAP attributes by querying LDAP
LDAPEntry userEntry = ldapConnection.read(userDN, attrArray);
+ if (userEntry == null)
+ return null;
--- End diff --
It's definitely a good thing IMHO that `Attributes` does not define its
behavior for `null`, as there's little need to impose that burden on all that
implement that interface. This specific function here does intentionally return
`null`, though, and so that return value should be documented.
> Thoughts on whether its better to go back and define `null` behavior or
return an empty `Map` (which I'm guessing is the other option)?
Returning an empty map would definitely be a reasonable option, and would
remove the need to document a `null` return value (as there would cease to be a
`null` return value). As doing that would also remove the need for the caller
to be aware of the possible distinction between `null` ("I can't retrieve
anything") and an empty map ("I successfully retrieved nothing"), this would be
my preference, unless you think there is some value in maintaining that
distinction.
---
