GUACAMOLE-300: Support posixGroup in LDAP Authentication and Group-ba… #245
Conversation
…sed Session Admission
I'm perfectly good with the changes you suggested here, but I would vote for making it configurable, as that would improve LDAP performance on searches, especially for larger trees. |
|
My only concerns would be with respect to the The
|
necouchman
left a comment
necouchman
left a comment
There was a problem hiding this comment.
Why does guacConfigGroup, a connection configuration mechanism specific to Guacamole, need to support memberUid? Why use member, as defined by groupOfNames
Isn't the search that's being modified not looking for guacConfigGroups, and only looking for actual groups:
"(&(!(objectClass=guacConfigGroup))(|(member=" + escapingService.escapeLDAPSearchFilter(userDN) + ")(memberUid=" + user.getCredentials().getUsername() + ")))"
?? It's generating the search filter which will ultimately be used to find connections accessible to the current user through group membership, and all this change really accomplishes is to make sure that it looks for both groupOfNames and posixGroup objects, correct? It's not actually examining guacConfigGroup objects. Or maybe I'm missing something?
@steffen-moser:
Need to add documentation for the one new parameter, as noted above. Also, I do think this should be configurable and not something that automatically gets added to every search, as I've already noted.
| * @throws LDAPException | ||
| * If an error occurs preventing retrieval of user groups. | ||
| * | ||
| * @throws GuacamoleException |
There was a problem hiding this comment.
New parameter needs to be added to the documentation.
|
Yup, that's correct. Never mind - I misread the query. |
| groupBaseDN, | ||
| LDAPConnection.SCOPE_SUB, | ||
| "(&(!(objectClass=guacConfigGroup))(member=" + escapingService.escapeLDAPSearchFilter(userDN) + "))", | ||
| "(&(!(objectClass=guacConfigGroup))(|(member=" + escapingService.escapeLDAPSearchFilter(userDN) + ")(memberUid=" + user.getCredentials().getUsername() + ")))", |
There was a problem hiding this comment.
The username value here will need to be escaped for inclusion in a filter.
|
@steffen-moser Any chance you can work on this a bit more to get it ready for merge? I'd like to set the final list of issues for inclusion in the version 1.0.0 release, and this looks like a good candidate to me if it's something you can spend a little more time on? |
|
Ping @steffen-moser - see last note, would like to include this in 1.0.0... |
|
@steffen-moser Merge base needs to be changed to staging/1.0.0 to be included in 1.0.0 release. |
|
@steffen-moser Ping... |
|
@steffen-moser Last call, else this will be pulled from the 1.0.0 release. |
|
Closed by #450 |
3 participants
…sed Session Admission
The change will extend the user lookup code by the ability to search not only through the groupOfNames but also through the posixGroup scheme. The piece of code seems to work with both schemes in my tests successfully.
I am not sure if there are any pitfalls when just combining the possible results. Maybe introducing a configuration flag to choose whether searching posixGroup or groupOfNames would be a better approach.