Skip to content

Impersonating Troubleshooters can now impersonate users and groups#7465

Open
labkey-adam wants to merge 11 commits intodevelopfrom
fb_impersonation
Open

Impersonating Troubleshooters can now impersonate users and groups#7465
labkey-adam wants to merge 11 commits intodevelopfrom
fb_impersonation

Conversation

@labkey-adam
Copy link
Contributor

@labkey-adam labkey-adam commented Mar 2, 2026

Rationale

https://github.com/LabKey/internal-issues/issues/837

Changes

  • The main functional change is that Impersonating Troubleshooters can now impersonate users and groups in the root. If they happen to have read permissions in a project or folder, they can impersonate users, groups, and roles there as well.
  • Internally, I've reworked impersonation permissions checking to eliminate duplicate checks. A new ImpersonatePermission class is now used instead of AdminPermission as the primary check. Action-level and manager permission checking has been removed; all permission checking is performed by choke points in each impersonation context for simplicity and consistency.
  • ImpersonationTest junit test has been added to exercise and verify user, group, and role impersonation permissions rules.
  • App Admins can now impersonate groups that are assigned privileged roles (Site Admin, Platform Developer, Impersonating Troubleshooter), although any privileged roles are filtered out by GroupImpersonationContext. This matches the behavior of when they impersonate a privileged user.
  • Project Admins can now impersonate any user who has read permissions in the project. This eliminates one use of the antiquated "project users" notion. This filtering is a convenience and not a security restriction. (I can grant any user read permission in my project and then impersonate them.) For partitioned sites like Atlas, showing the whole user list to a project admin would be inconvenient. Plus, there's not much value in impersonating a user who doesn't have read permissions in the project.
  • I also renamed a method, class name, and term for clarity:
    • AbstractRootContainerRole.isAvailableEverywhere() --> isApplicableOutsideRoot()
    • CanImpersonatePrivilegedSiteRolesPermission --> ImpersonatePrivilegedSiteRolesPermission
    • wild-card or wild card --> wildcard

@labkey-adam labkey-adam marked this pull request as draft March 2, 2026 21:14
@labkey-adam labkey-adam marked this pull request as ready for review March 4, 2026 19:15
.filter(Role::isAssignable)
.filter(role -> role.isApplicable(policy, c))
.filter(role -> !role.isPrivileged() || canImpersonatePrivilegedRoles);
.filter(role -> !role.isPrivileged() || canImpersonatePrivilegedRoles)
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix up mismatch between container used for c.hasPermission(ImpersonationPermission) and role.isApplicable(c)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After further investigation, switching this to always pass in and check project (or root if null). AuthFilter checks perms based on the project stashed in the factory that's in session; it has no idea what the current folder is. That means we can impersonate roles only if they're applicable at the root or project level (never those only applicable in a folder). But no one has complained...

Copy link
Contributor

Choose a reason for hiding this comment

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

Great, maybe add a comment to memorialize the thought?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@labkey-jeckels labkey-jeckels left a comment

Choose a reason for hiding this comment

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

One suggestion and two questions from automated code review tool. See what you think.

@labkey-matthewb labkey-matthewb self-requested a review March 5, 2026 23:47
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