Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194163526
--- Diff:
extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java
---
@@ -221,6 +230,14 @@ public AuthenticatedUser authenticateUser(Credentials
credentials)
throw new GuacamoleInvalidCredentialsException("Permission
denied.", CredentialsInfo.USERNAME_PASSWORD);
try {
+ try {
--- End diff --
Two things, here...
First, I don't think there's any reason to nest these `try` statements, is
there? This may actually be moot (see comment below on the
`getLDAPAttributes()` method), but you should be able to do something like:
try {
// Code to authenticate and retrieve attributes
}
catch (LDAPException e) {
throw...
}
finally {
ldapService.disconnect...
}
I've definitely encountered instances where the nesting was required, but I
don't think this is one of them?
Second thing, I would actually put the attribute retrieval *after* the code
to authenticate the user. It probably isn't worth retrieving the attributes if
the authentication fails, so I'd only do that if authentication succeeds, and,
thus, after.
---
