Filter cross-team memberships from user list responses#48890
Conversation
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
This PR fixes an information disclosure in the team-scoped GET /api/latest/fleet/users endpoint by ensuring that, for team-scoped requesters, each returned user’s teams list is filtered to only the teams the requester has a role in (while global-role requesters still see all teams).
Changes:
- Filter
user.TeamsinlistUsersEndpointto the requester’s authorized team scope (global roles unchanged). - Add endpoint-level tests covering team-scoped filtering and global-role passthrough.
- (Unreviewable here) Updates to
handbook/company/product-groups.mdandchanges/16691-cross-team-user-list-teams-authzwere excluded by policy.
Reviewed changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| server/service/users.go | Adds response-layer filtering of user.Teams based on requester scope in listUsersEndpoint. |
| server/service/users_test.go | Adds regression tests to ensure cross-team memberships aren’t disclosed to team-scoped admins, while global admins see all teams. |
| handbook/company/product-groups.md | Content excluded from diff by policy (not reviewable). |
| changes/16691-cross-team-user-list-teams-authz | Content excluded from diff by policy (not reviewable). |
Files excluded by content exclusion policy (2)
- changes/16691-cross-team-user-list-teams-authz
- handbook/company/product-groups.md
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WalkthroughThis change updates Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/service/users_test.go (1)
1915-1952: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding a case for a requester with mixed team roles.
Neither test covers a requester who administers the queried team but also holds a non-admin role (e.g., observer) in another team the shared user belongs to. Adding this case would validate the "any role" vs "administers" concern raised in
users.go'sfilterUserTeamsToRequesterScope.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/service/users_test.go` around lines 1915 - 1952, Add a test in TestListUsersFiltersTeamsToRequesterScope that covers a requester who administers the queried team but also has a non-admin role in another team the returned user belongs to, so you exercise the mixed-role edge case in listUsersEndpoint and filterUserTeamsToRequesterScope. Reuse the existing sharedUserTeams setup, but make the viewer user include team 1 as admin and team 2 as a non-admin role, then assert the listed user is still filtered to only the requested team scope. This will verify the behavior around “any role” versus “administers” using the existing listUsersResponse and fleet.UserTeam symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/service/users_test.go`:
- Around line 1915-1952: Add a test in TestListUsersFiltersTeamsToRequesterScope
that covers a requester who administers the queried team but also has a
non-admin role in another team the returned user belongs to, so you exercise the
mixed-role edge case in listUsersEndpoint and filterUserTeamsToRequesterScope.
Reuse the existing sharedUserTeams setup, but make the viewer user include team
1 as admin and team 2 as a non-admin role, then assert the listed user is still
filtered to only the requested team scope. This will verify the behavior around
“any role” versus “administers” using the existing listUsersResponse and
fleet.UserTeam symbols.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 89b53071-9b8b-44ae-979b-bbb82bd39c9d
⛔ Files ignored due to path filters (1)
handbook/company/product-groups.mdis excluded by!**/*.md
📒 Files selected for processing (3)
changes/16691-cross-team-user-list-teams-authzserver/service/users.goserver/service/users_test.go
A team-scoped admin listing users of a team they administer received the
full team membership (IDs, names, roles) of any user shared with other
teams, disclosing teams the requester has no role in. The single-user
GET /users/{id} endpoint already blocks this via authz; the list endpoint
authorizes against a synthetic single-team object and returned unfiltered
teams loaded by the datastore.
Filter each returned user's teams to the requester's scope at the response
layer. Global roles are unchanged. Filtering is kept out of Service.User
because ModifyUser and password reset reuse it and read Teams to compute
write diffs.
For fleetdm/confidential#16691
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #48890 +/- ##
========================================
Coverage 68.05% 68.05%
========================================
Files 3684 3690 +6
Lines 234062 234970 +908
Branches 12463 12463
========================================
+ Hits 159294 159914 +620
- Misses 60452 60658 +206
- Partials 14316 14398 +82
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
lukeheath
force-pushed
the
fix-list-users-cross-team-teams-leak
branch
from
32c433c to
19abd1f
Compare
lucasmrod
left a comment
lucasmrod
left a comment
There was a problem hiding this comment.
Looks good. Left some comments.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/service/users.go (1)
482-493: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard
requesterbefore dereferencingviewer.FromContextonly guarantees aViewer, not a populatedViewer.User, so this helper can still panic onrequester.HasAnyGlobalRole(). Add a nil check before filtering teams.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/service/users.go` around lines 482 - 493, The helper filterUserTeamsToRequesterScope can panic when requester is nil because it calls requester.HasAnyGlobalRole() before any guard. Update this function to handle a nil requester up front, returning the appropriate filtered result without dereferencing requester, and keep the existing team-role filtering logic intact for non-nil users.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@server/service/users.go`:
- Around line 482-493: The helper filterUserTeamsToRequesterScope can panic when
requester is nil because it calls requester.HasAnyGlobalRole() before any guard.
Update this function to handle a nil requester up front, returning the
appropriate filtered result without dereferencing requester, and keep the
existing team-role filtering logic intact for non-nil users.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e58f46c8-6852-4c6d-b9fc-ca90369bdb04
📒 Files selected for processing (1)
server/service/users.go
From Lucas:
Summary
A team-scoped admin listing users of a team they administer (
GET /api/latest/fleet/users?team_id=A) received the full team membership — team IDs, names, and roles — of any user also shared with other teams, disclosing teams the requester has no role in.The single-user
GET /users/{id}endpoint already blocks this: its authorization requires the requester to administer every team the target belongs to. The list endpoint authorizes against a synthetic single-team object (correct, so team admins can manage their members), but then returned each user's complete team list as loaded by the datastore.This filters each returned user's teams down to the requester's scope at the response layer. Requesters with any global role are unchanged (they're authorized to see all teams).
Why the response layer, not
Service.UserModifyUserand the password-reset flow reuseService.Userand readuser.Teamsto compute write diffs. Filtering there would silently drop team memberships on edits, so the filter is applied inlistUsersEndpointonly.GET /users/{id}is intentionally not changed — it is not exploitable (authz already requires admin-of-all-the-target's-teams), and its legitimate readers should keep seeing the full team list.Testing
TestListUsersFiltersTeamsToRequesterScope— team-1 admin listing team 1 sees only team 1 for a user shared with {1,2}.TestListUsersGlobalRequesterSeesAllTeams— global admin sees all teams.TestUserAuth/TestAuthorizeUserpass unchanged (no authz regression).Fixes fleetdm/confidential#16691
🤖 Generated with Claude Code
Summary by CodeRabbit