You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by hguehl <gi...@git.apache.org> on 2016/08/14 14:51:52 UTC
[GitHub] incubator-guacamole-client pull request #57: GUACAMOLE-79: LDAPConnection ha...
GitHub user hguehl opened a pull request:
https://github.com/apache/incubator-guacamole-client/pull/57
GUACAMOLE-79: LDAPConnection has a size limit of 1000
This is a humble contribution for large Ldap directory (over 1000 accounts).
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/hguehl/incubator-guacamole-client master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-guacamole-client/pull/57.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 #57
----
commit df2fe1013cd2e276ba06754ba284838201fd0bee
Author: root <he...@gmail.com>
Date: 2016-08-14T14:44:09Z
GUACAMOLE-79: LDAPConnection has a size limit of 1000
----
---
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 #57: GUACAMOLE-79: LDAPConnection ha...
Posted by jmuehlner <gi...@git.apache.org>.
Github user jmuehlner commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/57#discussion_r74715220
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ConfigurationService.java ---
@@ -206,4 +206,22 @@ public EncryptionMethod getEncryptionMethod() throws GuacamoleException {
);
}
+ /**
+ * Returns Max results a ldap query can return
--- End diff --
It looks like maybe a few words were left out here? Did you mean to say "return, as specified with guacamole.properties"?
---
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 #57: GUACAMOLE-79: LDAPConnection ha...
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/57#discussion_r74715752
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ConfigurationService.java ---
@@ -206,4 +206,22 @@ public EncryptionMethod getEncryptionMethod() throws GuacamoleException {
);
}
+ /**
+ * Returns Max results a ldap query can return
--- End diff --
And in the same vein, "max" should similarly *not* be capitalized.
---
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 #57: GUACAMOLE-79: LDAPConnection ha...
Posted by jmuehlner <gi...@git.apache.org>.
Github user jmuehlner commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/57#discussion_r74715279
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ConfigurationService.java ---
@@ -206,4 +206,22 @@ public EncryptionMethod getEncryptionMethod() throws GuacamoleException {
);
}
+ /**
+ * Returns Max results a ldap query can return
--- End diff --
Also, when used in a comment, the acronym LDAP should be in all uppercase.
---
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 #57: GUACAMOLE-79: Adding an option ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-guacamole-client/pull/57
---
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 #57: GUACAMOLE-79: LDAPConnection ha...
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/57#discussion_r74715807
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/LDAPGuacamoleProperties.java ---
@@ -145,4 +145,17 @@ private LDAPGuacamoleProperties() {}
};
+ /**
+ * The maxResult a ldap search can return
+ *
--- End diff --
Why is this `*` misaligned from the rest?
---
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 #57: GUACAMOLE-79: LDAPConnection ha...
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/57#discussion_r74715832
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/LDAPGuacamoleProperties.java ---
@@ -145,4 +145,17 @@ private LDAPGuacamoleProperties() {}
};
+ /**
+ * The maxResult a ldap search can return
--- End diff --
And "The maximum number of results" would be better still. Just "max results" seems rather wonky to me.
---
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 #57: GUACAMOLE-79: Adding an option ...
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/57#discussion_r74814349
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/user/UserService.java ---
@@ -86,14 +87,18 @@ private void putAllUsers(Map<String, User> users, LDAPConnection ldapConnection,
String usernameAttribute) throws GuacamoleException {
try {
+ // Set search limits
+ LDAPSearchConstraints constraints = new LDAPSearchConstraints();
+ constraints.setMaxResults(confService.getMaxResults());
--- End diff --
Didn't notice this before, but please use 4-space indents (no tabs). I saw that these three lines weren't quite aligned with the surrounding code, and decided to download the patch to take a closer look. The misalignment is due to the tabs (see yellow highlight in screenshot below):
You might have to change your editor settings to get it to stop adding tabs instead of spaces.
Other than that, I see no further issues. With the exception of the tabs, all looks good to me, even the commit history. ;)
---
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 #57: GUACAMOLE-79: Adding an option ...
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/57#discussion_r74790860
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/LDAPGuacamoleProperties.java ---
@@ -145,4 +145,16 @@ private LDAPGuacamoleProperties() {}
};
+ /**
+ * The maximum number of results a LDAP query can return.
+ */
+ public static final IntegerGuacamoleProperty LDAP_MAX_SEARCH_RESULTS = new IntegerGuacamoleProperty() {
+
+ @Override
+ public String getName() { return "ldap-maxresults"; }
--- End diff --
Just noticed:
`LDAP_MAX_SEARCH_RESULTS` is very nice and readable, so why is the property just `ldap-maxresults`? (Nice dash following the "ldap", but not between "max" and "results"). For consistency with other properties used by Guacamole, this should at least be `ldap-max-results`, though given the nice `LDAP_MAX_SEARCH_RESULTS` name, I'd suggest `ldap-max-search-results` instead.
---
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 #57: GUACAMOLE-79: LDAPConnection ha...
Posted by jmuehlner <gi...@git.apache.org>.
Github user jmuehlner commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/57#discussion_r74715340
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/user/UserService.java ---
@@ -86,14 +87,18 @@ private void putAllUsers(Map<String, User> users, LDAPConnection ldapConnection,
String usernameAttribute) throws GuacamoleException {
try {
+ //change search limits
--- End diff --
For single line comments, we like to start with a capital letter, and leave a space after the slashes:
// This is a friendly comment
---
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 #57: GUACAMOLE-79: LDAPConnection ha...
Posted by jmuehlner <gi...@git.apache.org>.
Github user jmuehlner commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/57#discussion_r74715309
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/LDAPGuacamoleProperties.java ---
@@ -145,4 +145,17 @@ private LDAPGuacamoleProperties() {}
};
+ /**
+ * The maxResult a ldap search can return
--- End diff --
The formatting of this comment is a bit off (asterisks don't line up, capitalization, no period at the end of line).
Also, "The max results" would read better than "the maxResult".
---
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.
---