Add fleet-user creation check#47566
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds an enterprise integration test to ensure a team-scoped admin cannot create a user with team roles in fleets/teams they don’t administer (even if one of the requested team roles is allowed), covering both the “admin create” and “API-only create” endpoints that flow through svc.CreateUser.
Changes:
- Added
TestTeamAdminCannotCreateUserInOtherTeamsto validate cross-team user creation is rejected with403 Forbidden. - Verified the forbidden request does not create the user (admin create path) and that a team admin can still create a user scoped only to their own team.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // A fleet-admin must not be able to create a user with roles in teams they don't administer, | ||
| // even when one team in the payload is their own. | ||
| // | ||
| // Both the admin create path (POST /users) and the API-only create | ||
| // path (POST /users/api_only) funnel through svc.CreateUser, so both are | ||
| // exercised here. |
WalkthroughThis PR adds a new integration test 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.43.0)server/service/integration_enterprise_test.goThanks 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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
server/service/integration_enterprise_test.go (2)
5148-5158: 💤 Low valueConsider cleaning up the team admin user.
The test creates a
teamAdminuser directly via the datastore but never deletes it. While integration test databases may be reset between runs, explicit cleanup improves test hygiene.♻️ Add cleanup for the team admin user
_, err = s.ds.NewUser(ctx, teamAdmin) require.NoError(t, err) + defer func() { + s.token = s.getTestAdminToken() + require.NoError(t, s.ds.DeleteUser(ctx, teamAdmin.ID)) + }() // Act as the team1 admin for the rest of the test. s.token = s.getTestToken(teamAdmin.Email, test.GoodPassword)Note: Adjust based on whether
s.ds.DeleteUserexists or if a different cleanup method is appropriate.🤖 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/integration_enterprise_test.go` around lines 5148 - 5158, Add explicit cleanup for the teamAdmin user created in the test: after you finish using teamAdmin call the datastore delete method (e.g. s.ds.DeleteUser(ctx, teamAdmin.ID)) and assert success (require.NoError) to remove the user; if s.ds.DeleteUser does not exist, call the appropriate teardown method (e.g. RemoveUser/DestroyUser or delete via SQL helper) and assert no error—this ensures the user created by NewUser (teamAdmin) is removed at the end of the test.
5190-5202: 💤 Low valueConsider cleaning up the successfully created user.
The sanity check creates a user that succeeds (line 5193), but the test doesn't clean up this resource. Adding cleanup improves test isolation.
♻️ Add cleanup for the successfully created user
}, http.StatusOK, &resp) require.NotNil(t, resp.User) + defer func() { + s.token = s.getTestAdminToken() + require.NoError(t, s.ds.DeleteUser(ctx, resp.User.ID)) + }() }Note: Since this is the last operation in the test, you may alternatively add the cleanup before the function returns rather than using defer.
🤖 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/integration_enterprise_test.go` around lines 5190 - 5202, The test creates a user into resp (createUserResponse) via s.DoJSON but never removes it; after the successful creation capture resp.User.ID (or resp.User) and delete the created user to avoid leaking state — either immediately defer a cleanup call (e.g., call the existing user-deletion helper or s.DoJSON with the users delete endpoint and the created ID) or perform the delete before the test returns; update the block containing the s.DoJSON POST that populates resp so it runs the corresponding delete using that ID.
🤖 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.
Inline comments:
In `@server/service/integration_enterprise_test.go`:
- Around line 5180-5188: The API-only endpoint test calls s.DoJSON("POST",
"/api/latest/fleet/users/api_only", createAPIOnlyUserRequest...) and asserts
http.StatusForbidden but does not verify the user wasn't persisted; add a
persistence check after that call similar to the admin-path check: attempt to
lookup the user (by email if createAPIOnlyUserRequest included one, or by name
using the same query used earlier in the file) and assert the user does not
exist (or that the lookup returns empty/not found), using the same helper/query
used in the admin test to ensure symmetry with the admin endpoint verification;
if API-only users require a different lookup, use that mechanism instead.
---
Nitpick comments:
In `@server/service/integration_enterprise_test.go`:
- Around line 5148-5158: Add explicit cleanup for the teamAdmin user created in
the test: after you finish using teamAdmin call the datastore delete method
(e.g. s.ds.DeleteUser(ctx, teamAdmin.ID)) and assert success (require.NoError)
to remove the user; if s.ds.DeleteUser does not exist, call the appropriate
teardown method (e.g. RemoveUser/DestroyUser or delete via SQL helper) and
assert no error—this ensures the user created by NewUser (teamAdmin) is removed
at the end of the test.
- Around line 5190-5202: The test creates a user into resp (createUserResponse)
via s.DoJSON but never removes it; after the successful creation capture
resp.User.ID (or resp.User) and delete the created user to avoid leaking state —
either immediately defer a cleanup call (e.g., call the existing user-deletion
helper or s.DoJSON with the users delete endpoint and the created ID) or perform
the delete before the test returns; update the block containing the s.DoJSON
POST that populates resp so it runs the corresponding delete using that ID.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a1fd76da-c93f-4c99-85d2-573ace97b431
📒 Files selected for processing (1)
server/service/integration_enterprise_test.go
| // API-only create path inherits the same authorization, so it must reject | ||
| // the cross-team payload too. | ||
| s.DoJSON("POST", "/api/latest/fleet/users/api_only", createAPIOnlyUserRequest{ | ||
| Name: new(t.Name() + "_crossteam_api"), | ||
| Fleets: &[]fleetsPayload{ | ||
| {ID: team1.ID, Role: fleet.RoleAdmin}, | ||
| {ID: team2.ID, Role: fleet.RoleAdmin}, | ||
| }, | ||
| }, http.StatusForbidden, &resp) |
There was a problem hiding this comment.
Add persistence verification for the API-only forbidden request.
The admin endpoint test (lines 5176-5178) verifies the forbidden user was not persisted, but the API-only endpoint test has no equivalent check. While both funnel through the same authorization layer, asymmetric verification weakens regression protection.
✅ Add a persistence check after the API-only request
If the createAPIOnlyUserRequest includes an email or if API-only users can be queried by name, add a verification similar to lines 5176-5178:
s.DoJSON("POST", "/api/latest/fleet/users/api_only", createAPIOnlyUserRequest{
Name: new(t.Name() + "_crossteam_api"),
Fleets: &[]fleetsPayload{
{ID: team1.ID, Role: fleet.RoleAdmin},
{ID: team2.ID, Role: fleet.RoleAdmin},
},
}, http.StatusForbidden, &resp)
+
+ // Verify the forbidden API-only request also didn't persist a user.
+ // Adjust the query method based on how API-only users can be retrieved.
+ _, err = s.ds.UserByEmail(ctx, t.Name()+"_crossteam_api@example.com")
+ require.True(t, fleet.IsNotFound(err))Note: Adjust the verification method if API-only users don't have email addresses or require a different lookup mechanism.
🤖 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/integration_enterprise_test.go` around lines 5180 - 5188, The
API-only endpoint test calls s.DoJSON("POST",
"/api/latest/fleet/users/api_only", createAPIOnlyUserRequest...) and asserts
http.StatusForbidden but does not verify the user wasn't persisted; add a
persistence check after that call similar to the admin-path check: attempt to
lookup the user (by email if createAPIOnlyUserRequest included one, or by name
using the same query used earlier in the file) and assert the user does not
exist (or that the lookup returns empty/not found), using the same helper/query
used in the admin test to ensure symmetry with the admin endpoint verification;
if API-only users require a different lookup, use that mechanism instead.
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #47566 +/- ##
==========================================
+ Coverage 67.15% 67.18% +0.03%
==========================================
Files 3547 3548 +1
Lines 228778 228829 +51
Branches 11907 11907
==========================================
+ Hits 153634 153750 +116
+ Misses 61291 61243 -48
+ Partials 13853 13836 -17
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:
|
|
Test failure is unrelated (FMA panic nil). This is just adding a test. |
Summary by CodeRabbit