You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by "tkuhlengel (via GitHub)" <gi...@apache.org> on 2026/02/20 02:42:55 UTC
[PR] GUACAMOLE-2224: Fix effective user group expansion for SAML/SSO groups [guacamole-client]
tkuhlengel opened a new pull request, #1170:
URL: https://github.com/apache/guacamole-client/pull/1170
PR corresponding to https://issues.apache.org/jira/browse/GUACAMOLE-2224
## Problem
When a user authenticates via an external SSO provider (e.g. SAML with Azure AD), the identity provider asserts group membership by name. These names are stored on the `AuthenticatedUser` and surfaced through `getEffectiveUserGroups()`.
The previous implementation in `ModeledAuthenticatedUser` resolved this with a flat union:
```java
return Sets.union(user.getEffectiveUserGroups(), super.getEffectiveUserGroups());
```
This combined the user's database group memberships with the raw SSO claims, but made no attempt to walk the database group hierarchy. If `child-group` (matched by SAML claim) was a member of `parent-group` in the database, `parent-group` was never included — and any permissions granted only to `parent-group` were invisible to the SSO user.
Database-native users were unaffected because their memberships are stored as `entity_id` references already handled by the existing recursive expansion in `EntityService`.
---
## Changes
### `ModeledPermissions.expandEffectiveGroups()` — guacamole-auth-jdbc-base
A new method delegates to `EntityService.retrieveEffectiveGroups()`, passing caller-provided external group names as additional seeds for recursive DB group expansion:
```java
public Set<String> expandEffectiveGroups(Collection<String> externalEffectiveGroups) {
return entityService.retrieveEffectiveGroups(this, externalEffectiveGroups);
}
```
This exposes the existing recursive CTE logic as a callable entry point on the permissions object.
### `ModeledAuthenticatedUser.getEffectiveUserGroups()` — guacamole-auth-jdbc-base
The flat union is replaced with a call to `expandEffectiveGroups()`, passing the raw SSO claims as seeds:
```java
@Override
public Set<String> getEffectiveUserGroups() {
return user.expandEffectiveGroups(super.getEffectiveUserGroups());
}
```
`super.getEffectiveUserGroups()` returns the SSO group name claims from the originating `AuthenticatedUser`. These are now fed into the recursive expansion, so any ancestor groups of those claims in the database are included in the result — making inherited permissions visible to SSO users.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-2224: Fix effective user group expansion for SAML/SSO groups [guacamole-client]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
necouchman commented on code in PR #1170:
URL: https://github.com/apache/guacamole-client/pull/1170#discussion_r3484080180
##########
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/ModeledAuthenticatedUser.java:
##########
@@ -170,11 +169,21 @@ public String getIdentifier() {
public void setIdentifier(String identifier) {
user.setIdentifier(identifier);
}
-
+
+
+ /**
+ * Expands a user's groups through the parents in database group hierarchy
+ * so that parent groups of external groups (e.g. SAML/SSO group claims)
+ * are included. This also covers the user's own direct
+ * DB memberships (via entity_id) and skeleton users with null entity_id.
+ *
+ * @return
+ * The set of effective groups for this user, whether inherited or
+ * direct.
+ */
@Override
public Set<String> getEffectiveUserGroups() {
- return Sets.union(user.getEffectiveUserGroups(),
- super.getEffectiveUserGroups());
+ return user.expandEffectiveGroups(super.getEffectiveUserGroups());
Review Comment:
I think injecting `entityService` into the `ModeledAuthenticatedUser` class would be the way I'd prefer to go - it seems to me that would result, overall, in easier-to-follow code, rather than having to jump from one class to another and then on to another.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-2224: Fix effective user group expansion for SAML/SSO groups [guacamole-client]
Posted by "tkuhlengel (via GitHub)" <gi...@apache.org>.
tkuhlengel commented on PR #1170:
URL: https://github.com/apache/guacamole-client/pull/1170#issuecomment-4289631088
This is ready for review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-2224: Fix effective user group expansion for SAML/SSO groups [guacamole-client]
Posted by "tkuhlengel (via GitHub)" <gi...@apache.org>.
tkuhlengel commented on PR #1170:
URL: https://github.com/apache/guacamole-client/pull/1170#issuecomment-4520264952
Ready for followup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-2224: Fix effective user group expansion for SAML/SSO groups [guacamole-client]
Posted by "tkuhlengel (via GitHub)" <gi...@apache.org>.
tkuhlengel commented on PR #1170:
URL: https://github.com/apache/guacamole-client/pull/1170#issuecomment-4290366888
Fixed the commit issue.
Replied to the review point.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-2224: Fix effective user group expansion for SAML/SSO groups [guacamole-client]
Posted by "tkuhlengel (via GitHub)" <gi...@apache.org>.
tkuhlengel commented on PR #1170:
URL: https://github.com/apache/guacamole-client/pull/1170#issuecomment-5317107130
> > @necouchman I think I finished the rebase correctly. Please let me know if you need something different from the rebase or if it needs to be squashed.
> > I assumed that any changes that weren't mine were intended to be as they were from `staging/1.6.1`
>
> Could you please drop the commits coming from the main branch?
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-2224: Fix effective user group expansion for SAML/SSO groups [guacamole-client]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1170:
URL: https://github.com/apache/guacamole-client/pull/1170#discussion_r2831094869
##########
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/ModeledAuthenticatedUser.java:
##########
@@ -170,11 +169,21 @@ public String getIdentifier() {
public void setIdentifier(String identifier) {
user.setIdentifier(identifier);
}
-
+
+
+ /**
+ * Expands a users groups through the parents in database group hierarchy
Review Comment:
Grammar issue: "a users groups" should be "a user's groups" (possessive).
```suggestion
* Expands a user's groups through the parents in database group hierarchy
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-2224: Fix effective user group expansion for SAML/SSO groups [guacamole-client]
Posted by "corentin-soriano (via GitHub)" <gi...@apache.org>.
corentin-soriano commented on PR #1170:
URL: https://github.com/apache/guacamole-client/pull/1170#issuecomment-5313657777
> @necouchman I think I finished the rebase correctly. Please let me know if you need something different from the rebase or if it needs to be squashed.
>
> I assumed that any changes that weren't mine were intended to be as they were from `staging/1.6.1`
Could you please drop the commits coming from the main branch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-2224: Fix effective user group expansion for SAML/SSO groups [guacamole-client]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
necouchman commented on code in PR #1170:
URL: https://github.com/apache/guacamole-client/pull/1170#discussion_r3118772774
##########
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/ModeledAuthenticatedUser.java:
##########
@@ -170,11 +169,21 @@ public String getIdentifier() {
public void setIdentifier(String identifier) {
user.setIdentifier(identifier);
}
-
+
+
+ /**
+ * Expands a user's groups through the parents in database group hierarchy
+ * so that parent groups of external groups (e.g. SAML/SSO group claims)
+ * are included. This also covers the user's own direct
+ * DB memberships (via entity_id) and skeleton users with null entity_id.
+ *
+ * @return
+ * The set of effective groups for this user, whether inherited or
+ * direct.
+ */
@Override
public Set<String> getEffectiveUserGroups() {
- return Sets.union(user.getEffectiveUserGroups(),
- super.getEffectiveUserGroups());
+ return user.expandEffectiveGroups(super.getEffectiveUserGroups());
Review Comment:
Rather than creating a new `expandEffectiveGroups()` method in the `ModeledUser` class, is there any reason not to just call the `entityService.retrieveEffectiveGroups()` method, here, and add it to the `Sets.union()` call?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-2224: Fix effective user group expansion for SAML/SSO groups [guacamole-client]
Posted by "tkuhlengel (via GitHub)" <gi...@apache.org>.
tkuhlengel commented on code in PR #1170:
URL: https://github.com/apache/guacamole-client/pull/1170#discussion_r3668970703
##########
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/ModeledAuthenticatedUser.java:
##########
@@ -170,11 +169,21 @@ public String getIdentifier() {
public void setIdentifier(String identifier) {
user.setIdentifier(identifier);
}
-
+
+
+ /**
+ * Expands a user's groups through the parents in database group hierarchy
+ * so that parent groups of external groups (e.g. SAML/SSO group claims)
+ * are included. This also covers the user's own direct
+ * DB memberships (via entity_id) and skeleton users with null entity_id.
+ *
+ * @return
+ * The set of effective groups for this user, whether inherited or
+ * direct.
+ */
@Override
public Set<String> getEffectiveUserGroups() {
- return Sets.union(user.getEffectiveUserGroups(),
- super.getEffectiveUserGroups());
+ return user.expandEffectiveGroups(super.getEffectiveUserGroups());
Review Comment:
Addressed in the latest commit, though it is a bit bigger of a changeset.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-2224: Fix effective user group expansion for SAML/SSO groups [guacamole-client]
Posted by "tkuhlengel (via GitHub)" <gi...@apache.org>.
tkuhlengel commented on PR #1170:
URL: https://github.com/apache/guacamole-client/pull/1170#issuecomment-5308395835
@necouchman I think I finished the rebase correctly. Please let me know if you need something different from the rebase or if it needs to be squashed.
I assumed that any changes that weren't mine were intended to be as they were from `staging/1.6.1`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-2224: Fix effective user group expansion for SAML/SSO groups [guacamole-client]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
necouchman commented on PR #1170:
URL: https://github.com/apache/guacamole-client/pull/1170#issuecomment-4289986148
Thanks @tkuhlengel, I'll try to take a look. FYI, your "Typo fix" commit needs to either be properly tagged with the GUACAMOLE-2224 issue, or needs to be squashed into the other one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-2224: Fix effective user group expansion for SAML/SSO groups [guacamole-client]
Posted by "tkuhlengel (via GitHub)" <gi...@apache.org>.
tkuhlengel commented on code in PR #1170:
URL: https://github.com/apache/guacamole-client/pull/1170#discussion_r3119013898
##########
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/ModeledAuthenticatedUser.java:
##########
@@ -170,11 +169,21 @@ public String getIdentifier() {
public void setIdentifier(String identifier) {
user.setIdentifier(identifier);
}
-
+
+
+ /**
+ * Expands a user's groups through the parents in database group hierarchy
+ * so that parent groups of external groups (e.g. SAML/SSO group claims)
+ * are included. This also covers the user's own direct
+ * DB memberships (via entity_id) and skeleton users with null entity_id.
+ *
+ * @return
+ * The set of effective groups for this user, whether inherited or
+ * direct.
+ */
@Override
public Set<String> getEffectiveUserGroups() {
- return Sets.union(user.getEffectiveUserGroups(),
- super.getEffectiveUserGroups());
+ return user.expandEffectiveGroups(super.getEffectiveUserGroups());
Review Comment:
I think `ModeledAuthenticatedUser` doesn't have access to `entityService`. It's injected in `ModeledPermissions`, which `ModeledUser` inherits from. The thin `expandEffectiveGroups()` wrapper keeps the service access encapsulated without changing any permissions or inheritance.
To call `entityService` directly here I'd need to either inject it into `ModeledAuthenticatedUser` or add a getter method on `ModeledUser`.
I can try to go that route if you want. The wrapper `user.expandEffectiveGroups()` seemed like the most compact option.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GUACAMOLE-2224: Fix effective user group expansion for SAML/SSO groups [guacamole-client]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
necouchman commented on PR #1170:
URL: https://github.com/apache/guacamole-client/pull/1170#issuecomment-5207980982
Thanks, @tkuhlengel - this looks good. Could you rebase against `staging/1.6.1` and we'll get it into this next release?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org