You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by alt36 <gi...@git.apache.org> on 2017/03/17 14:32:29 UTC
[GitHub] incubator-guacamole-client pull request #129: GUACAMOLE-243: Ignore LDAPRefe...
GitHub user alt36 opened a pull request:
https://github.com/apache/incubator-guacamole-client/pull/129
GUACAMOLE-243: Ignore LDAPReferralExceptions thrown when iterating over LDAPSearchResults
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/alt36/incubator-guacamole-client ignore-ldap-referrals
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-guacamole-client/pull/129.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #129
----
commit c426b322290b8af763fd956f08869760bbc057a1
Author: Adam Thorn <al...@cam.ac.uk>
Date: 2017-03-17T14:28:55Z
GUACAMOLE-243: Ignore LDAPReferralExceptions thrown when iterating over LDAPSearchResults
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #129: GUACAMOLE-243: Ignore LDAPRefe...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/129#discussion_r106697645
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/user/UserService.java ---
@@ -101,21 +102,24 @@ private void putAllUsers(Map<String, User> users, LDAPConnection ldapConnection,
// Read all visible users
while (results.hasMore()) {
-
- LDAPEntry entry = results.next();
-
- // Get username from record
- LDAPAttribute username = entry.getAttribute(usernameAttribute);
- if (username == null) {
- logger.warn("Queried user is missing the username attribute \"{}\".", usernameAttribute);
- continue;
+ try {
+ LDAPEntry entry = results.next();
+
+ // Get username from record
+ LDAPAttribute username = entry.getAttribute(usernameAttribute);
+ if (username == null) {
+ logger.warn("Queried user is missing the username attribute \"{}\".", usernameAttribute);
+ continue;
+ }
+
+ // Store user using their username as the identifier
+ String identifier = username.getStringValue();
+ if (users.put(identifier, new SimpleUser(identifier)) != null)
+ logger.warn("Possibly ambiguous user account: \"{}\".", identifier);
+
+ } catch (LDAPReferralException e) {
+ logger.debug("Ignoring LDAP Referral: \"{}\".", e.toString());
--- End diff --
As mentioned in the comments on [GUACAMOLE-243](https://issues.apache.org/jira/browse/GUACAMOLE-243), ignoring all LDAP referrals is not really a solution unless there is an option to follow them.
Warning that an LDAP referral has occurred and isn't being followed is good, but:
1. Such a warning would naturally need to be at the "warning" log level.
2. The details of the exception should be logged at the "debug" level (ie: `logger.debug("Some contextual message about what just happened.", e)`, *not* `e.toString()` or there won't be any stacktraces at the debug level).
Normally, non-debug log messages use `e.getMessage()` to pull some additional human-readable context while excluding the stacktrace, but if the `LDAPReferralException` will only ever occur for non-followed referrals, that may be just noise here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #129: GUACAMOLE-243: Ignore LDAPRefe...
Posted by alt36 <gi...@git.apache.org>.
Github user alt36 closed the pull request at:
https://github.com/apache/incubator-guacamole-client/pull/129
---
[GitHub] incubator-guacamole-client pull request #129: GUACAMOLE-243: Ignore LDAPRefe...
Posted by alt36 <gi...@git.apache.org>.
Github user alt36 commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/129#discussion_r106703061
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/user/UserService.java ---
@@ -101,21 +102,24 @@ private void putAllUsers(Map<String, User> users, LDAPConnection ldapConnection,
// Read all visible users
while (results.hasMore()) {
-
- LDAPEntry entry = results.next();
-
- // Get username from record
- LDAPAttribute username = entry.getAttribute(usernameAttribute);
- if (username == null) {
- logger.warn("Queried user is missing the username attribute \"{}\".", usernameAttribute);
- continue;
+ try {
+ LDAPEntry entry = results.next();
+
+ // Get username from record
+ LDAPAttribute username = entry.getAttribute(usernameAttribute);
+ if (username == null) {
+ logger.warn("Queried user is missing the username attribute \"{}\".", usernameAttribute);
+ continue;
+ }
+
+ // Store user using their username as the identifier
+ String identifier = username.getStringValue();
+ if (users.put(identifier, new SimpleUser(identifier)) != null)
+ logger.warn("Possibly ambiguous user account: \"{}\".", identifier);
+
+ } catch (LDAPReferralException e) {
--- End diff --
Thanks for the feedback - I honestly thought I was following what I'd seen elsewhere in the code, but evidently I wasn't paying close enough attention! I'll read the style guide more carefully next time :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #129: GUACAMOLE-243: Ignore LDAPRefe...
Posted by alt36 <gi...@git.apache.org>.
Github user alt36 commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/129#discussion_r106701681
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/user/UserService.java ---
@@ -259,8 +263,13 @@ private String generateLDAPQuery(String username)
// Add all DNs for found users
while (results.hasMore()) {
- LDAPEntry entry = results.next();
- userDNs.add(entry.getDN());
+ try {
+ LDAPEntry entry = results.next();
+ userDNs.add(entry.getDN());
+ } catch (LDAPReferralException e) {
+ logger.debug("Ignoring LDAP Referral: \"{}\".", e.toString());
--- End diff --
Fair enough - my thought process was that at present, it is simply not possible to log in via LDAP if a referral is returned. This makes it impossible to use LDAP auth against an AD where the search base dn is the root of the AD, because at present the LDAPReferralException will be rethrown and not dealt with in any way. This was a v0.1 attempt to at least improve the situation a little. I don't disagree that including the capability to (probably optionally) follow the referral is a better solution, but I'm afraid I'll have to leave that to someone more familiar with the code, then.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #129: GUACAMOLE-243: Ignore LDAPRefe...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/129#discussion_r106696635
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/user/UserService.java ---
@@ -101,21 +102,24 @@ private void putAllUsers(Map<String, User> users, LDAPConnection ldapConnection,
// Read all visible users
while (results.hasMore()) {
-
- LDAPEntry entry = results.next();
-
- // Get username from record
- LDAPAttribute username = entry.getAttribute(usernameAttribute);
- if (username == null) {
- logger.warn("Queried user is missing the username attribute \"{}\".", usernameAttribute);
- continue;
+ try {
+ LDAPEntry entry = results.next();
+
+ // Get username from record
+ LDAPAttribute username = entry.getAttribute(usernameAttribute);
+ if (username == null) {
+ logger.warn("Queried user is missing the username attribute \"{}\".", usernameAttribute);
+ continue;
+ }
+
+ // Store user using their username as the identifier
+ String identifier = username.getStringValue();
+ if (users.put(identifier, new SimpleUser(identifier)) != null)
+ logger.warn("Possibly ambiguous user account: \"{}\".", identifier);
+
+ } catch (LDAPReferralException e) {
--- End diff --
Please do not cuddle the `catch`. For more information, see the general style of other code in the codebase, or the overview of the style we use at http://guacamole.incubator.apache.org/guac-style/.
The most important thing here style-wise is consistency.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #129: GUACAMOLE-243: Ignore LDAPRefe...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/129#discussion_r106697719
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/user/UserService.java ---
@@ -259,8 +263,13 @@ private String generateLDAPQuery(String username)
// Add all DNs for found users
while (results.hasMore()) {
- LDAPEntry entry = results.next();
- userDNs.add(entry.getDN());
+ try {
+ LDAPEntry entry = results.next();
+ userDNs.add(entry.getDN());
+ } catch (LDAPReferralException e) {
+ logger.debug("Ignoring LDAP Referral: \"{}\".", e.toString());
--- End diff --
As above, probably not a good idea to just ignore all referrals.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #129: GUACAMOLE-243: Ignore LDAPRefe...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/129#discussion_r106696780
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/user/UserService.java ---
@@ -259,8 +263,13 @@ private String generateLDAPQuery(String username)
// Add all DNs for found users
while (results.hasMore()) {
- LDAPEntry entry = results.next();
- userDNs.add(entry.getDN());
+ try {
+ LDAPEntry entry = results.next();
+ userDNs.add(entry.getDN());
+ } catch (LDAPReferralException e) {
--- End diff --
Same thing here - please do not cuddle the `catch`. See other code in the codebase for examples of style.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---