GUACAMOLE-2224: Fix effective user group expansion for SAML/SSO groups#1170
GUACAMOLE-2224: Fix effective user group expansion for SAML/SSO groups#1170tkuhlengel wants to merge 2 commits intoapache:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request fixes a bug where SSO users (e.g., via SAML/Azure AD) did not receive permissions from parent groups in the database group hierarchy. When an SSO provider asserted group membership by name, the previous implementation simply unioned those names with database groups using Sets.union(), without recursively expanding parent groups. This meant that permissions granted only to parent groups were invisible to SSO users, while database-native users were unaffected because their memberships use entity_id references handled by existing recursive expansion logic.
Changes:
- Added
ModeledPermissions.expandEffectiveGroups()method to expose the existing recursive group expansion logic fromEntityServiceas a callable entry point - Modified
ModeledAuthenticatedUser.getEffectiveUserGroups()to call the new expansion method, passing SSO group claims as seeds for recursive database group expansion - Removed unused Guava
Setsimport
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| ModeledPermissions.java | Adds new expandEffectiveGroups() method that delegates to EntityService.retrieveEffectiveGroups() to recursively expand external group names through the database group hierarchy |
| ModeledAuthenticatedUser.java | Updates getEffectiveUserGroups() to use the new expansion method instead of flat union, and removes unused Guava import |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
AuthenticatedUserand surfaced throughgetEffectiveUserGroups().The previous implementation in
ModeledAuthenticatedUserresolved this with a flat union: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 ofparent-groupin the database,parent-groupwas never included — and any permissions granted only toparent-groupwere invisible to the SSO user.Database-native users were unaffected because their memberships are stored as
entity_idreferences already handled by the existing recursive expansion inEntityService.Changes
ModeledPermissions.expandEffectiveGroups()— guacamole-auth-jdbc-baseA new method delegates to
EntityService.retrieveEffectiveGroups(), passing caller-provided external group names as additional seeds for recursive DB group expansion:This exposes the existing recursive CTE logic as a callable entry point on the permissions object.
ModeledAuthenticatedUser.getEffectiveUserGroups()— guacamole-auth-jdbc-baseThe flat union is replaced with a call to
expandEffectiveGroups(), passing the raw SSO claims as seeds:super.getEffectiveUserGroups()returns the SSO group name claims from the originatingAuthenticatedUser. 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.