Skip to content

GUACAMOLE-2224: Fix effective user group expansion for SAML/SSO groups#1170

Open
tkuhlengel wants to merge 2 commits intoapache:mainfrom
tkuhlengel:GUACAMOLE-2224
Open

GUACAMOLE-2224: Fix effective user group expansion for SAML/SSO groups#1170
tkuhlengel wants to merge 2 commits intoapache:mainfrom
tkuhlengel:GUACAMOLE-2224

Conversation

@tkuhlengel
Copy link
Copy Markdown

@tkuhlengel tkuhlengel commented Feb 20, 2026

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:

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:

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:

@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.

Copilot AI review requested due to automatic review settings February 20, 2026 02:42
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 from EntityService as 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 Sets import

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.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants