Open
Conversation
juan-fdz-hawa
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #43440 +/- ##
==========================================
+ Coverage 66.90% 66.91% +0.01%
==========================================
Files 2594 2598 +4
Lines 207772 208512 +740
Branches 9208 9208
==========================================
+ Hits 139006 139533 +527
- Misses 56131 56277 +146
- Partials 12635 12702 +67
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Added POST /users/api_only endpoint for creating API-only users.
- Added PATCH /users/api_only/{id} for updating existing API-only users.
- Updated `fleetctl user create --api-only` removing email/password field requirements.
juan-fdz-hawa
force-pushed
the
42882_42880_42884_allow_creation_of_api_only_users
branch
from
5a4a9ea to
a7ff10b
Compare
juan-fdz-hawa
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
juan-fdz-hawa
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
juan-fdz-hawa
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds API-only user management: new endpoints POST /api/version/fleet/users/api_only and PATCH /api/version/fleet/users/api_only/{id}; expands User and UserPayload with api_endpoints and persists/loads per-user API endpoint permissions (DB schema and migration remove author_id from user_api_endpoints); introduces Service.ModifyAPIOnlyUser and client/CLI support (fleetctl create --api-only no longer requires email/password); enforces validation and license-gating for api_endpoints; adds helpers for random email/password generation; and introduces extensive unit and integration tests covering free vs premium behaviors. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@changes/42882-42880-42884-allow-creation-of-api-only-users`:
- Around line 1-3: The changelog currently lists POST /users/api_only, PATCH
/users/api_only/{id}, and CLI changes but omits the GET-side contract change —
update the release notes to also call out that the GET /users/{id} response now
includes the new api_endpoints field; mention the exact endpoint name GET
/users/{id} and the added response property api_endpoints so API consumers know
the response shape has changed.
In `@cmd/fleetctl/fleetctl/user.go`:
- Around line 127-150: When apiOnly is true the code currently ignores any SSO
or MFA flags; add an explicit validation before calling CreateAPIOnlyUser to
reject incompatible flag combinations by returning an error if --api-only is set
together with the SSO or MFA flags. Locate the apiOnly branch and check the
SSO/MFA flag variables (the same variables parsed for the non-API path, e.g.,
sso and mfa or their equivalents) and if either is set return a descriptive
fmt.Errorf explaining that --sso and --mfa cannot be used with --api-only; keep
the rest of the CreateAPIOnlyUser flow (including the sessionKey handling)
unchanged.
In
`@server/datastore/mysql/migrations/tables/20260411000000_DropAuthorIdFromUserApiEndpoints.go`:
- Around line 28-29: The ALTER TABLE DDL built in the tx.Exec(fmt.Sprintf(...))
call that runs `ALTER TABLE user_api_endpoints DROP FOREIGN KEY %s` uses the raw
constraintName and can break if the FK identifier needs quoting; update the code
that constructs the SQL for DROP FOREIGN KEY in this migration (the
fmt.Sprintf/tx.Exec invocation referencing constraintName) to safely quote the
identifier with backticks and escape any backticks in constraintName before
interpolation so the resulting SQL uses `\`<escaped-constraint>\`` rather than
the raw value, preventing syntax errors and SQL injection risks.
In `@server/fleet/users.go`:
- Around line 60-62: The APIEndpoints field currently uses
`json:"api_endpoints,omitempty"`, which hides both nil and empty slices in
responses; update the struct field `APIEndpoints []APIEndpointRef` to either
remove `omitempty` so the field is always present (empty array for non-API-only
users) or change the type to a pointer `*[]APIEndpointRef` if you need explicit
nil vs empty-array semantics; adjust any serializers/tests that expect the
absent field accordingly and keep the field name `APIEndpoints` and type
`APIEndpointRef` consistent.
In `@server/service/integration_enterprise_test.go`:
- Around line 28591-28599: The test updates a user's teams but doesn't assert
that the previous global role was cleared; after the PATCH call that sets
"fleets" via s.DoJSON (using patchURL) and inspects respTeam (patchResp), add an
assertion that respTeam.User.GlobalRole (or respTeam.User.global_role in JSON)
is empty/nil to ensure the global role was removed when switching to team
(fleet) role.
- Around line 28675-28680: The test currently hard-codes adminID := uint(1) and
then calls s.DoJSON("/api/latest/fleet/users/%d", adminID) which couples the
test to seed data; instead, create a fresh regular user within the test (via the
existing user creation helper or an API POST to /api/latest/fleet/users),
capture the returned ID, and use that ID in the subsequent s.DoJSON GET; update
references to adminID and assertions on getAdminResp (type getUserResp and
fields User.APIOnly and User.APIEndpoints) to use the newly created user's ID so
the test no longer relies on seeded users.
In `@server/service/users.go`:
- Around line 249-268: Early returns in the user creation flow can exit before
authorization is marked checked, causing the transport to report an auth-skipped
error; before each early return in the GenerateRandomPwd error branch, the
viewer.FromContext failure branch, and the GenerateRandomEmail error branch,
call setAuthCheckedOnPreAuthErr(ctx) to mark auth as checked (i.e., invoke
setAuthCheckedOnPreAuthErr(ctx) just prior to returning the createUserResponse
in the error paths around GenerateRandomPwd, viewer.FromContext, and
GenerateRandomEmail).
- Around line 325-329: The authorization check in ModifyAPIOnlyUser currently
calls svc.authz.Authorize(ctx, &fleet.User{}, fleet.ActionWrite) before loading
the target user, which strips any team context and incorrectly denies
team-scoped updates; instead, first load the target user by calling the existing
user lookup (e.g., retrieve via svc.ds.User(...)/svc.UserByID) and then call
svc.authz.Authorize(ctx, loadedUser, fleet.ActionWrite) so the authorizer can
evaluate team scope; if you must keep the pre-check to prevent ID enumeration,
normalize the error semantics so callers receive the same 404/403 outcome
regardless of whether the user exists or authorization fails rather than
authorizing on an empty fleet.User.
- Around line 201-208: CreateUser currently only validates API endpoints when
p.APIOnly is true, allowing non-API-only creates to include p.APIEndpoints;
enforce the same invariant as ModifyUser by rejecting APIEndpoints when APIOnly
is absent or false. In the CreateUser path around the p.APIOnly /
validateAPIEndpointRefs logic, add a guard that if p.APIEndpoints is non-nil (or
non-empty) and (p.APIOnly == nil || !*p.APIOnly) you return the same
validation/error used by ModifyUser; keep using validateAPIEndpointRefs only for
the API-only branch and mirror ModifyUser’s error type/message to ensure
consistent behavior.
🪄 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: b079f96d-2820-4342-a966-11e59800c47c
📒 Files selected for processing (16)
changes/42882-42880-42884-allow-creation-of-api-only-userscmd/fleetctl/fleetctl/user.gocmd/fleetctl/fleetctl/users_test.goserver/datastore/mysql/migrations/tables/20260411000000_DropAuthorIdFromUserApiEndpoints.goserver/datastore/mysql/schema.sqlserver/datastore/mysql/users.goserver/fleet/service.goserver/fleet/users.goserver/mock/service/service_mock.goserver/service/client_users.goserver/service/handler.goserver/service/integration_core_test.goserver/service/integration_enterprise_test.goserver/service/users.goserver/utils.goserver/utils_test.go
server/datastore/mysql/migrations/tables/20260411000000_DropAuthorIdFromUserApiEndpoints.go
Outdated
Show resolved
Hide resolved
juan-fdz-hawa
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
juan-fdz-hawa
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
juan-fdz-hawa
commented
Apr 13, 2026
juan-fdz-hawa
commented
Apr 13, 2026
juan-fdz-hawa
commented
Apr 13, 2026
server/datastore/mysql/migrations/tables/20260411000000_DropAuthorIdFromUserApiEndpoints.go
Outdated
Show resolved
Hide resolved
juan-fdz-hawa
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
cmd/fleetctl/fleetctl/user.go (1)
127-153:⚠️ Potential issue | 🟡 Minor
--ssois still silently ignored in the--api-onlypath.Line 127 returns before the normal SSO/password validation runs, so
fleetctl user create --api-only --ssosucceeds while discarding the caller's--ssorequest. Reject it alongside--mfa.💡 Suggested fix
if apiOnly { - if mfa { - return errors.New("--mfa cannot be used with --api-only") + if sso || mfa { + return errors.New("--sso and --mfa cannot be used with --api-only") } sessionKey, err := client.CreateAPIOnlyUser(name, globalRole, teams) if err != nil { return fmt.Errorf("Failed to create user: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/fleetctl/fleetctl/user.go` around lines 127 - 153, In the api-only branch of the user creation flow (the block that checks apiOnly in the function handling the user create command in user.go), add validation to reject the --sso flag just like --mfa is rejected: before calling client.CreateAPIOnlyUser(name, globalRole, teams) check if sso (the parsed flag variable for SSO) is true and return an error such as "--sso cannot be used with --api-only"; ensure this validation lives alongside the existing mfa check so the command fails fast and does not silently ignore SSO when apiOnly is set.
🧹 Nitpick comments (2)
server/datastore/mysql/migrations/tables/20260411000000_DropAuthorIdFromUserApiEndpoints.go (1)
37-38: WrapDROP COLUMNerrors with operation context.Line 38 returns the raw error; adding context makes migration failures easier to diagnose.
💡 Suggested tweak
- _, err = tx.Exec(`ALTER TABLE user_api_endpoints DROP COLUMN author_id`) - return err + if _, err = tx.Exec(`ALTER TABLE user_api_endpoints DROP COLUMN author_id`); err != nil { + return fmt.Errorf("drop user_api_endpoints.author_id column: %w", err) + } + return nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/migrations/tables/20260411000000_DropAuthorIdFromUserApiEndpoints.go` around lines 37 - 38, The migration returns the raw error from tx.Exec when running ALTER TABLE user_api_endpoints DROP COLUMN author_id; change the error handling in the migration function (where tx.Exec is called) to wrap the returned error with contextual information (e.g., reference the operation "drop author_id from user_api_endpoints" and include the original error) so failures are easier to diagnose; locate the tx.Exec call that executes `ALTER TABLE user_api_endpoints DROP COLUMN author_id` and replace the direct return of err with a wrapped error using fmt.Errorf(..., %w) or errors.Wrap.server/service/users_test.go (1)
1707-1722: Tighten the assertion so this test proves theapi_onlyguard fired.Right now it only checks
InvalidArgumentError, so an unrelated validation failure would also satisfy the test. Consider asserting the error mentionsapi_onlyand that no save path is invoked.♻️ Suggested test hardening
t.Run("cannot promote non-API user to API-only via api_only:true", func(t *testing.T) { adminUser := newAdminTestUser(nil) regularUser := newAdminTestUser(&adminTestUserOpts{id: 2, email: "regular@example.com", apiOnly: false}) ds, svc, ctx := setupAdminTestContext(t, adminUser) setupModifyUserMocks(ds, regularUser) + ds.SaveUserFunc = func(ctx context.Context, u *fleet.User) error { + t.Fatal("SaveUser should not be called when api_only mutation is rejected") + return nil + } _, err := svc.ModifyUser(ctx, regularUser.ID, fleet.UserPayload{APIOnly: new(true)}) require.Error(t, err) var argErr *fleet.InvalidArgumentError require.ErrorAs(t, err, &argErr) + require.Contains(t, err.Error(), "api_only") })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/users_test.go` around lines 1707 - 1722, The test should not only assert InvalidArgumentError from svc.ModifyUser but also ensure the api_only guard triggered: update the test case that calls svc.ModifyUser(..., fleet.UserPayload{APIOnly: new(false)}) to assert the returned error message contains "api_only" (or the exact field name used in validation) and additionally assert the mock datastore (ds) save/update method (e.g., SaveUser/UpdateUser) was not invoked—use the mock's "NotCalled" or expectation checks set up in setupModifyUserMocks so the test proves no save path was executed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/fleetctl/fleetctl/user.go`:
- Around line 40-43: Restore the conditional validation so that --email is
required unless --api-only is set: in the user creation command handler (the
action function that reads emailFlagName and apiOnlyFlagName), add a pre-run or
initial check that if the api-only flag is false/empty and the value for
emailFlagName is empty, return a user-facing error indicating email is required;
keep the flag definition non-required but enforce the conditional requirement
programmatically in the command's action/validate step (referencing
emailFlagName and apiOnlyFlagName to locate the values).
In `@server/service/integration_enterprise_test.go`:
- Around line 28761-28775: After creating the regular user (regularUserResp),
also call the list endpoint "GET /api/latest/fleet/users" and assert the created
user's entry has APIOnly false and APIEndpoints empty; specifically, add a GET
request similar to the existing one but storing into a list response (e.g.
getUsersResp) and locate the user by ID from regularUserResp.User.ID, then
require.False on that user's APIOnly and require.Empty on that user's
APIEndpoints to match the per-user GET assertions; use the same request helper
(s.DoJSON) and assertion style as the existing getAdminResp checks.
- Around line 28416-28429: The test case "more than 100 api_endpoints" currently
generates non-catalog paths in the eps slice (eps[i] =
map[string]any{"method":"GET","path":fmt.Sprintf("/api/v1/fleet/path/%d", i)})
which can trigger catalog validation errors instead of the intended max-length
error; update the generated paths in that test (and the similar case around the
other occurrence) to use a valid catalog endpoint path (for example
fmt.Sprintf("/api/v1/fleet/queries/%d" or another known catalog endpoint used
elsewhere in tests) so failures come from the api_endpoints length cap; keep the
same eps construction and only change the fmt.Sprintf path string in both
places.
In `@server/service/users.go`:
- Around line 44-57: The createUserEndpoint currently rejects req.APIEndpoints
but still forwards req.APIOnly into svc.CreateUser; update createUserEndpoint to
also reject API-only creation by checking req.APIOnly (e.g., if req.APIOnly !=
nil or true depending on its type) and return a createUserResponse with
fleet.NewInvalidArgumentError("api_only", "This endpoint does not accept
API-only user creation") while calling setAuthCheckedOnPreAuthErr(ctx) before
returning, instead of passing req.APIOnly into CreateUser; locate
createUserEndpoint, createUserRequest, req.APIOnly and svc.CreateUser to apply
this change.
- Around line 377-388: The invite signup endpoint createUserFromInviteEndpoint
currently blocks APIEndpoints but does not guard against the APIOnly field,
allowing API-only users to be created; add the same explicit check used
elsewhere (lines referenced in review) by inspecting req.APIOnly in
createUserFromInviteEndpoint, call setAuthCheckedOnPreAuthErr(ctx) and return a
createUserResponse with fleet.NewInvalidArgumentError("api_only", "This endpoint
does not accept api_only users") (or similar message) so VerifyInviteCreate() /
NewUser() cannot create API-only accounts from this transport; ensure the check
mirrors the pattern used at the other transport guard
(setAuthCheckedOnPreAuthErr + createUserResponse error) and reference req,
APIOnly, createUserFromInviteEndpoint, createUserResponse,
setAuthCheckedOnPreAuthErr, and NewUser when making the change.
---
Duplicate comments:
In `@cmd/fleetctl/fleetctl/user.go`:
- Around line 127-153: In the api-only branch of the user creation flow (the
block that checks apiOnly in the function handling the user create command in
user.go), add validation to reject the --sso flag just like --mfa is rejected:
before calling client.CreateAPIOnlyUser(name, globalRole, teams) check if sso
(the parsed flag variable for SSO) is true and return an error such as "--sso
cannot be used with --api-only"; ensure this validation lives alongside the
existing mfa check so the command fails fast and does not silently ignore SSO
when apiOnly is set.
---
Nitpick comments:
In
`@server/datastore/mysql/migrations/tables/20260411000000_DropAuthorIdFromUserApiEndpoints.go`:
- Around line 37-38: The migration returns the raw error from tx.Exec when
running ALTER TABLE user_api_endpoints DROP COLUMN author_id; change the error
handling in the migration function (where tx.Exec is called) to wrap the
returned error with contextual information (e.g., reference the operation "drop
author_id from user_api_endpoints" and include the original error) so failures
are easier to diagnose; locate the tx.Exec call that executes `ALTER TABLE
user_api_endpoints DROP COLUMN author_id` and replace the direct return of err
with a wrapped error using fmt.Errorf(..., %w) or errors.Wrap.
In `@server/service/users_test.go`:
- Around line 1707-1722: The test should not only assert InvalidArgumentError
from svc.ModifyUser but also ensure the api_only guard triggered: update the
test case that calls svc.ModifyUser(..., fleet.UserPayload{APIOnly: new(false)})
to assert the returned error message contains "api_only" (or the exact field
name used in validation) and additionally assert the mock datastore (ds)
save/update method (e.g., SaveUser/UpdateUser) was not invoked—use the mock's
"NotCalled" or expectation checks set up in setupModifyUserMocks so the test
proves no save path was executed.
🪄 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: ebefe68c-6886-41bb-b93d-cd37c309b22d
📒 Files selected for processing (11)
changes/42882-42880-42884-allow-creation-of-api-only-userscmd/fleetctl/fleetctl/user.goserver/api_endpoints/api_endpoints.goserver/datastore/mysql/migrations/tables/20260411000000_DropAuthorIdFromUserApiEndpoints.goserver/datastore/mysql/users.goserver/fleet/service.goserver/service/client_users.goserver/service/integration_core_test.goserver/service/integration_enterprise_test.goserver/service/users.goserver/service/users_test.go
✅ Files skipped from review due to trivial changes (2)
- server/api_endpoints/api_endpoints.go
- server/datastore/mysql/users.go
🚧 Files skipped from review as they are similar to previous changes (3)
- changes/42882-42880-42884-allow-creation-of-api-only-users
- server/service/client_users.go
- server/service/integration_core_test.go
juan-fdz-hawa
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/service/users.go (1)
79-91: Consider simplifying the validation condition for clarity.The nested condition is functionally correct but hard to parse at a glance. The intent is: reject if
requireNonEmptyis true, OR if an explicit empty array was provided (i.e.,refs != nilbut empty).♻️ Optional: Clearer condition structure
func validateAPIEndpointRefs(ctx context.Context, refs *[]fleet.APIEndpointRef, requireNonEmpty bool) error { - if refs == nil || len(*refs) == 0 { - if requireNonEmpty || refs != nil { - return ctxerr.Wrap( - ctx, - fleet.NewInvalidArgumentError( - "api_endpoints", - "At least one API endpoint must be specified for API only users", - ), - ) - } - return nil - } + // If field is not provided and we don't require it, nothing to validate. + if refs == nil && !requireNonEmpty { + return nil + } + // Otherwise, require at least one entry (reject nil when required, or explicit empty array always). + if refs == nil || len(*refs) == 0 { + return ctxerr.Wrap( + ctx, + fleet.NewInvalidArgumentError( + "api_endpoints", + "At least one API endpoint must be specified for API only users", + ), + ) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/users.go` around lines 79 - 91, The validation logic in validateAPIEndpointRefs is correct but hard to read; split the nil check and the empty-slice check for clarity: first, if refs == nil then return an error only when requireNonEmpty is true otherwise return nil; second, if len(*refs) == 0 then always return the same ctxerr.Wrap(fleet.NewInvalidArgumentError("api_endpoints", "...")) error (since refs is non-nil but empty); keep the same error message and use the same ctx, refs, and requireNonEmpty symbols so callers behave identically.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/service/users.go`:
- Around line 79-91: The validation logic in validateAPIEndpointRefs is correct
but hard to read; split the nil check and the empty-slice check for clarity:
first, if refs == nil then return an error only when requireNonEmpty is true
otherwise return nil; second, if len(*refs) == 0 then always return the same
ctxerr.Wrap(fleet.NewInvalidArgumentError("api_endpoints", "...")) error (since
refs is non-nil but empty); keep the same error message and use the same ctx,
refs, and requireNonEmpty symbols so callers behave identically.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 428d8420-6d1e-498a-b98e-c31a7de7dc24
📒 Files selected for processing (3)
server/service/integration_core_test.goserver/service/integration_enterprise_test.goserver/service/users.go
✅ Files skipped from review due to trivial changes (1)
- server/service/integration_core_test.go
juan-fdz-hawa
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/service/integration_core_test.go (1)
432-503: Tighten negative-case assertions inTestCreateAPIOnlyUser.Right now failures assert status only. Please also assert error content/code so a wrong validation path returning the same status doesn’t pass silently.
♻️ Suggested refactor
cases := []struct { name string body map[string]any wantStatus int + wantErrSubstr string verify func(t *testing.T, resp createAPIOnlyUserResponse) }{ { name: "missing name", body: map[string]any{"global_role": "observer"}, wantStatus: http.StatusUnprocessableEntity, + wantErrSubstr: "name", }, @@ for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { var resp createAPIOnlyUserResponse s.DoJSON("POST", "/api/latest/fleet/users/api_only", tc.body, tc.wantStatus, &resp) + if tc.wantErrSubstr != "" { + require.Contains(t, strings.ToLower(resp.Err), strings.ToLower(tc.wantErrSubstr)) + } if tc.verify != nil { tc.verify(t, resp) } }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/integration_core_test.go` around lines 432 - 503, The negative tests in the table only assert HTTP status; update TestCreateAPIOnlyUser so each negative-case entry (the cases slice used with s.DoJSON and createAPIOnlyUserResponse) also includes expected error details (e.g., an expected error code or message field) and assert those after the request; for failing cases call s.DoJSON (or a variant) to unmarshal into the API error response struct (createAPIOnlyUserResponse currently used for success) or a dedicated error struct, then assert the error.code/error.message equals the expected value for that case (add fields like wantErrorCode/wantErrorMessage to the case entries and check them when tc.verify == nil) to ensure the test validates the error payload as well as the HTTP status.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/service/users.go`:
- Around line 353-378: Add an upfront check in ModifyAPIOnlyUser to ensure the
loaded target user is actually API-only before calling ModifyUser: after
retrieving target via svc.ds.UserByID and after authorization, verify
target.APIOnly (or the equivalent field) is true and if not return a contextual
InvalidArgument error (e.g. ctxerr.Wrap(ctx, fleet.NewInvalidArgumentError("id",
"user is not an API-only user"))) so callers of the /users/api_only/{id}
endpoint get a clear, immediate error instead of the downstream "cannot change
api_only status" failure.
---
Nitpick comments:
In `@server/service/integration_core_test.go`:
- Around line 432-503: The negative tests in the table only assert HTTP status;
update TestCreateAPIOnlyUser so each negative-case entry (the cases slice used
with s.DoJSON and createAPIOnlyUserResponse) also includes expected error
details (e.g., an expected error code or message field) and assert those after
the request; for failing cases call s.DoJSON (or a variant) to unmarshal into
the API error response struct (createAPIOnlyUserResponse currently used for
success) or a dedicated error struct, then assert the error.code/error.message
equals the expected value for that case (add fields like
wantErrorCode/wantErrorMessage to the case entries and check them when tc.verify
== nil) to ensure the test validates the error payload as well as the HTTP
status.
🪄 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: 38dcd331-2ff3-47d7-80a9-575b756941e0
📒 Files selected for processing (2)
server/service/integration_core_test.goserver/service/users.go
juan-fdz-hawa
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
server/service/users.go (2)
353-378:⚠️ Potential issue | 🟡 MinorAdd upfront check that target user is API-only.
The method correctly authorizes against the loaded target user (addressing the team-scoped auth concern). However, it doesn't verify that the target is an API-only user before delegating to
ModifyUser. If called with a regular user's ID, it will fail with "cannot change api_only status of a user" (line 692-694), which is confusing for callers using the/users/api_only/{id}endpoint.Suggested fix
func (svc *Service) ModifyAPIOnlyUser(ctx context.Context, userID uint, p fleet.UserPayload) (*fleet.User, error) { target, err := svc.ds.UserByID(ctx, userID) if err != nil { setAuthCheckedOnPreAuthErr(ctx) return nil, ctxerr.Wrap(ctx, err) } if err := svc.authz.Authorize(ctx, target, fleet.ActionWrite); err != nil { return nil, err } + if !target.APIOnly { + return nil, ctxerr.Wrap(ctx, fleet.NewInvalidArgumentError("id", "user is not an API-only user")) + } + vc, ok := viewer.FromContext(ctx)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/users.go` around lines 353 - 378, ModifyAPIOnlyUser currently doesn't verify the loaded target is an API-only user and can produce confusing errors downstream; after loading target via svc.ds.UserByID and before authorizing/early-return checks (or immediately after authorization), check the target's API-only flag (e.g., target.APIOnly or equivalent field) and return a clear invalid-argument error (use fleet.NewInvalidArgumentError("id", "<clear message>")) if the target is not API-only, so callers of the /users/api_only/{id} endpoint get a meaningful error instead of the generic ModifyUser message.
44-57:⚠️ Potential issue | 🟡 MinorReject
api_onlyon the generic create endpoint.The guard only rejects
api_endpointsbut still allowsapi_only: trueto pass through toCreateUser, where it would successfully create an API-only user (with auto-added wildcard endpoints). This bypasses the dedicated/users/api_onlyendpoint and creates inconsistency in the API surface.Suggested fix
func createUserEndpoint(ctx context.Context, request interface{}, svc fleet.Service) (fleet.Errorer, error) { req := request.(*createUserRequest) + if req.APIOnly != nil { + setAuthCheckedOnPreAuthErr(ctx) + return createUserResponse{ + Err: fleet.NewInvalidArgumentError( + "api_only", + "This endpoint does not accept API only values", + ), + }, nil + } + if req.APIEndpoints != nil { setAuthCheckedOnPreAuthErr(ctx) return createUserResponse{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/users.go` around lines 44 - 57, The generic createUserEndpoint currently only rejects APIEndpoints but not api_only; update createUserEndpoint to also detect and reject the api_only flag on req (e.g., check req.APIOnly or req.ApiOnly) by calling setAuthCheckedOnPreAuthErr(ctx) and returning a createUserResponse with fleet.NewInvalidArgumentError("api_only", "Use the /users/api_only endpoint to create API-only users") so api_only requests are routed to the dedicated Create API-only flow instead of passing through to svc.CreateUser.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@server/service/users.go`:
- Around line 353-378: ModifyAPIOnlyUser currently doesn't verify the loaded
target is an API-only user and can produce confusing errors downstream; after
loading target via svc.ds.UserByID and before authorizing/early-return checks
(or immediately after authorization), check the target's API-only flag (e.g.,
target.APIOnly or equivalent field) and return a clear invalid-argument error
(use fleet.NewInvalidArgumentError("id", "<clear message>")) if the target is
not API-only, so callers of the /users/api_only/{id} endpoint get a meaningful
error instead of the generic ModifyUser message.
- Around line 44-57: The generic createUserEndpoint currently only rejects
APIEndpoints but not api_only; update createUserEndpoint to also detect and
reject the api_only flag on req (e.g., check req.APIOnly or req.ApiOnly) by
calling setAuthCheckedOnPreAuthErr(ctx) and returning a createUserResponse with
fleet.NewInvalidArgumentError("api_only", "Use the /users/api_only endpoint to
create API-only users") so api_only requests are routed to the dedicated Create
API-only flow instead of passing through to svc.CreateUser.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cabdbb19-9ec5-4404-93c0-cc7b887e6c17
📒 Files selected for processing (1)
server/service/users.go
juan-fdz-hawa
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
nulmete
reviewed
Apr 13, 2026
Member
nulmete
left a comment
nulmete
left a comment
There was a problem hiding this comment.
LGTM overall 👍 I only have a couple clarifying questions, nothing blocking.
Haven't looked at tests yet and I saw a change was just pushed before I submitted my review 😬
juan-fdz-hawa
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
lucasmrod
reviewed
Apr 13, 2026
Member
There was a problem hiding this comment.
Given this is unreleased, do we need a separate migration? (Why not fix the existing migration for the feature)
cmd/fleetctl/fleetctl/user.go
Outdated
| } | ||
|
|
||
| fmt.Fprintln(c.App.Writer, "Successfully created new user!") | ||
| if appCfg, cfgErr := client.GetAppConfig(); cfgErr == nil && |
Member
There was a problem hiding this comment.
Should we print somthing if cfgErr != nil?
server/datastore/mysql/users.go
Outdated
| return err | ||
| } | ||
|
|
||
| if user.APIEndpoints != nil { |
Member
There was a problem hiding this comment.
Suggested change
| if user.APIEndpoints != nil { | |
| if user.APIOnly && user.APIEndpoints != nil { |
| // - Field absent → Present=false: no change to the user's current endpoints. | ||
| // - Field is null → Present=true, Value=nil: clear all entries (full access). | ||
| // - Field is an array → Present=true, Value=[...]: replace with specific entries. | ||
| type OptionalAPIEndpoints struct { |
Member
There was a problem hiding this comment.
IIRC we have optjson package for this use case.
Member
There was a problem hiding this comment.
But I think this is a-ok as is, it's clear.
| ue.GET("/api/_version_/fleet/users", listUsersEndpoint, listUsersRequest{}) | ||
| ue.POST("/api/_version_/fleet/users/admin", createUserEndpoint, createUserRequest{}) | ||
| ue.POST("/api/_version_/fleet/users/api_only", createAPIOnlyUserEndpoint, createAPIOnlyUserRequest{}) | ||
| ue.PATCH("/api/_version_/fleet/users/api_only/{id:[0-9]+}", modifyAPIOnlyUserEndpoint, modifyAPIOnlyUserRequest{}) |
Member
There was a problem hiding this comment.
Asking here instead of Slack. Why do we need a new PATCH instead of using the existing one for users.
server/service/users.go
Outdated
| type createAPIOnlyUserRequest struct { | ||
| Name *string `json:"name,omitempty"` | ||
| GlobalRole *string `json:"global_role,omitempty"` | ||
| Teams *[]fleet.UserTeam `json:"teams,omitempty" renameto:"fleets"` |
Member
There was a problem hiding this comment.
Do we need the teams alias on new endpoints?
| if err != nil { | ||
| setAuthCheckedOnPreAuthErr(ctx) | ||
| return nil, ctxerr.Wrap(ctx, err) | ||
| } |
Member
There was a problem hiding this comment.
Maybe check that target is indeed APIOnly?
Comment on lines
+664
to
+665
| if p.APIOnly != nil && *p.APIOnly && len(teamRoles) > 0 { | ||
| return nil, fleet.ErrMissingLicense |
Member
There was a problem hiding this comment.
Same as the other place, this might be caught already by fleet.PremiumRolesPresent.
Contributor
Author
There was a problem hiding this comment.
fleet.PremiumRolesPresent only validates against premium roles which is a subset of all the roles that are available, what we are checking here is that if you are creating an API only user you are not allowed to use team roles (regardless of whether they are premium or not).
Comment on lines
+685
to
+689
| // Changing endpoint permissions is a privileged operation — same level as | ||
| // changing roles. This prevents an API-only user from expanding their own access. | ||
| if err := svc.authz.Authorize(ctx, user, fleet.ActionWriteRole); err != nil { | ||
| return nil, err | ||
| } |
Member
There was a problem hiding this comment.
Good idea. I will be checking all of this as part of #42887
server/utils.go
Outdated
| ) | ||
|
|
||
| // GenerateRandomEmail generates a random email using baseEmail as the base. | ||
| // For example: GenerateRandomEmail('juan@fleetdm.com') -> 'juan+somerandomtext@fleetdm.com' |
Member
There was a problem hiding this comment.
Nit: Please remove the fleetdm.com address (IIRC we don't want those to reduce spam).
juan-fdz-hawa
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issues:
POST /api/v1/fleet/users/api_only#42882PATCH /api/v1/fleet/users/:idandGET /api/v1/fleet/users/:id#42884Changes
fleetctl user create --api-onlyremoving email/password field requirements.Checklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Testing
Summary by CodeRabbit
New Features
CLI
fleetctl user create --api-onlyno longer requires email/password and prints the API token on creation.Behavior
Tests