Added middleware for api-only users auth#43772
Conversation
Fixes #42885 Added new middleware (APIOnlyEndpointCheck) that enforces 403 for API-only users whose request either isn't in the API endpoint catalog or falls outside their configured per-user endpoint restrictions.
juan-fdz-hawa
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #43772 +/- ##
========================================
Coverage 66.91% 66.92%
========================================
Files 2600 2602 +2
Lines 208643 209035 +392
Branches 9342 9342
========================================
+ Hits 139608 139890 +282
- Misses 56326 56399 +73
- Partials 12709 12746 +37
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:
|
juan-fdz-hawa
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis PR implements HTTP 403 denial enforcement for API-only users when their requests do not match configured endpoint restrictions. A new middleware component checks whether a request exists in the API endpoint catalog and validates against the user's allowed endpoint list if configured. The implementation includes utilities to capture Gorilla/mux route templates in request context, catalog membership checking via endpoint fingerprint, and updates to endpoint wiring to integrate the new middleware before authentication. Tests validate middleware behavior, pass-through logic for non-API-only users, and enforcement of endpoint restrictions. 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 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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
server/service/integration_enterprise_test.go (1)
28989-29041: Consider adding coverage for a catalog-miss 403 case.The current subtests exercise: (a) no restrictions + catalog endpoint = 200, (b) restrictions + allow-listed endpoint = 200 / non-allow-listed catalog endpoint = 403, and (c) non-api-only user unaffected. A useful additional case would be an api-only user (with or without
api_endpoints) hitting a path that is NOT in the API endpoint catalog at all, to lock in the "not in catalog → 403" behavior called out in the PR objectives. Without it, a regression that accidentally allows non-catalog paths through for api-only users would not be caught here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/integration_enterprise_test.go` around lines 28989 - 29041, Add a new subtest inside TestAPIOnlyUserEndpointMiddleware to assert that requests to paths not present in the API endpoint catalog return 403 for api-only users: use the existing createAPIOnlyUser helper to create a token (with and/or without api_endpoints) and call s.Do with a clearly non-catalog path (e.g. "/api/latest/fleet/nonexistent" or "/no/such/endpoint") expecting http.StatusForbidden; place this alongside the other t.Run cases so the catalog-miss → 403 behavior is covered by the test suite and references the TestAPIOnlyUserEndpointMiddleware, createAPIOnlyUser, and s.Do helpers.server/service/middleware/auth/api_only.go (1)
77-82: Optional: precompute allow-list fingerprints.
fleet.NewAPIEndpointFromTplruns a regex replace and string split per user endpoint on every request. For users with non-trivial allow-lists this recomputes the same fingerprints on each call. Iffleet.User.APIEndpointsis loaded once per request (or cached), consider computing the fingerprint set once upstream (e.g., on user load) and exposing a membership check, then this loop becomes an O(1) map lookup. Non-blocking for this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/middleware/auth/api_only.go` around lines 77 - 82, The loop repeatedly calls fleet.NewAPIEndpointFromTpl(...).Fingerprint() for every request which is expensive; modify the user loading code (where v.User is constructed) to precompute and store a set/map of endpoint fingerprints (e.g., User.APIEndpointFingerprints map[string]struct{}) derived from User.APIEndpoints using fleet.NewAPIEndpointFromTpl(...).Fingerprint(), and then change this middleware loop to perform an O(1 membership check against that map (check fp in v.User.APIEndpointFingerprints) instead of recomputing fingerprints; keep function names like NewAPIEndpointFromTpl, Fingerprint, and the middleware variables v.User and fp to locate the change.
🤖 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/middleware/auth/api_only_test.go`:
- Line 137: The tests pass a value-typed target (&fleet.PermissionError{}) to
require.ErrorAs but PermissionError implements error only on *PermissionError,
which causes errors.As to panic; fix each occurrence (the require.ErrorAs calls
at the locations noted) by using a pointer-to-pointer target (e.g., declare var
perr *fleet.PermissionError and pass &perr) or by passing
(*fleet.PermissionError)(nil) as the target so errors.As can match the
*fleet.PermissionError type; update the require.ErrorAs calls referencing
fleet.PermissionError accordingly.
---
Nitpick comments:
In `@server/service/integration_enterprise_test.go`:
- Around line 28989-29041: Add a new subtest inside
TestAPIOnlyUserEndpointMiddleware to assert that requests to paths not present
in the API endpoint catalog return 403 for api-only users: use the existing
createAPIOnlyUser helper to create a token (with and/or without api_endpoints)
and call s.Do with a clearly non-catalog path (e.g.
"/api/latest/fleet/nonexistent" or "/no/such/endpoint") expecting
http.StatusForbidden; place this alongside the other t.Run cases so the
catalog-miss → 403 behavior is covered by the test suite and references the
TestAPIOnlyUserEndpointMiddleware, createAPIOnlyUser, and s.Do helpers.
In `@server/service/middleware/auth/api_only.go`:
- Around line 77-82: The loop repeatedly calls
fleet.NewAPIEndpointFromTpl(...).Fingerprint() for every request which is
expensive; modify the user loading code (where v.User is constructed) to
precompute and store a set/map of endpoint fingerprints (e.g.,
User.APIEndpointFingerprints map[string]struct{}) derived from User.APIEndpoints
using fleet.NewAPIEndpointFromTpl(...).Fingerprint(), and then change this
middleware loop to perform an O(1 membership check against that map (check fp in
v.User.APIEndpointFingerprints) instead of recomputing fingerprints; keep
function names like NewAPIEndpointFromTpl, Fingerprint, and the middleware
variables v.User and fp to locate the change.
🪄 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: 64710039-c1cd-464e-b203-02ca86e0ff30
📒 Files selected for processing (10)
changes/42885-api-only-endpoints-middlewarecmd/fleet/serve.goserver/activity/internal/service/endpoint_utils.goserver/api_endpoints/api_endpoints.goserver/mdm/android/service/endpoint_utils.goserver/service/endpoint_utils.goserver/service/integration_core_test.goserver/service/integration_enterprise_test.goserver/service/middleware/auth/api_only.goserver/service/middleware/auth/api_only_test.go
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
![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.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/service/middleware/auth/api_only.go (1)
49-52: Minor: consider denying explicitly on missing method/template rather than relying on catalog miss.Today, a missing
kithttp.ContextKeyRequestMethodor missing route template produces an empty-path fingerprint that happens not to be in the catalog, which is then denied. This is fine defensively, but it couples correctness of a security check to the contents of the catalog. A short explicit guard (e.g.,if requestMethod == "" || routeTemplate == "" { return nil, permissionDenied(ctx) }) makes intent clear and produces a better debugging signal if wiring regresses.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/middleware/auth/api_only.go` around lines 49 - 52, The code currently derives requestMethod via kithttp.ContextKeyRequestMethod and routeTemplate via eu.RouteTemplateFromContext and then computes fp with fleet.NewAPIEndpointFromTpl(...).Fingerprint(); add an explicit guard that checks if requestMethod == "" || routeTemplate == "" and in that case immediately return the existing permissionDenied(ctx) result instead of relying on a catalog miss—update the block around where requestMethod, routeTemplate and fp are computed (the call sites of kithttp.ContextKeyRequestMethod, eu.RouteTemplateFromContext and fleet.NewAPIEndpointFromTpl) to perform this check and early-return to make the intent and error signal explicit.
🤖 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/fleet/users.go`:
- Around line 83-108: The apiEndpointFingerprintSet cache can become stale and
suffers a data race on lazy init; fix by making the cache always-in-sync and
removing the unsynchronized lazy-write: stop writing u.apiEndpointFingerprintSet
in APIEndpointFingerprintSet(), instead ensure every mutation of APIEndpoints
goes through a single setter that updates both u.APIEndpoints and
u.apiEndpointFingerprintSet (e.g., add SetAPIEndpoints([]APIEndpoint) or make
APIEndpoints private and provide a setter), update call sites that currently
assign user.APIEndpoints directly (UserPayload.User() and ModifyAPIOnlyUser) to
use that setter, and change APIEndpointFingerprintSet() to only return the
precomputed map (no lazy init/write). This ensures no concurrent writes and no
stale cache.
---
Nitpick comments:
In `@server/service/middleware/auth/api_only.go`:
- Around line 49-52: The code currently derives requestMethod via
kithttp.ContextKeyRequestMethod and routeTemplate via
eu.RouteTemplateFromContext and then computes fp with
fleet.NewAPIEndpointFromTpl(...).Fingerprint(); add an explicit guard that
checks if requestMethod == "" || routeTemplate == "" and in that case
immediately return the existing permissionDenied(ctx) result instead of relying
on a catalog miss—update the block around where requestMethod, routeTemplate and
fp are computed (the call sites of kithttp.ContextKeyRequestMethod,
eu.RouteTemplateFromContext and fleet.NewAPIEndpointFromTpl) to perform this
check and early-return to make the intent and error signal explicit.
🪄 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: 9b1dadb6-e695-4362-8c7f-c4ca79b4391f
📒 Files selected for processing (8)
cmd/fleet/serve.goserver/activity/internal/service/endpoint_utils.goserver/datastore/mysql/users.goserver/fleet/users.goserver/platform/endpointer/route_template.goserver/service/integration_enterprise_test.goserver/service/middleware/auth/api_only.goserver/service/middleware/auth/api_only_test.go
✅ Files skipped from review due to trivial changes (1)
- cmd/fleet/serve.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/service/integration_enterprise_test.go
lucasmrod
left a comment
lucasmrod
left a comment
There was a problem hiding this comment.
LGTM! Just some nit comments.
| // apiEndpointFingerprintSet is a pre-computed fingerprint set of APIEndpoints, | ||
| // populated by SetAPIEndpointFingerprintSet at load time. | ||
| apiEndpointFingerprintSet map[string]struct{} |
There was a problem hiding this comment.
Does it need a mutex for protection?
There was a problem hiding this comment.
I'll remove the map lookup - I think it's a premature optimization not worth the added complexity.
| for _, u := range users { | ||
| if u.APIOnly { | ||
| u.APIEndpoints = byUserID[u.ID] | ||
| u.SetAPIEndpointFingerprintSet() |
There was a problem hiding this comment.
Not needed given APIEndpointFingerprintSet will pre-compute before returning if empty?
juan-fdz-hawa
had a problem deploying
to
Docker Hub
GitHub Actions
Error
Co-authored-by: Lucas Manuel Rodriguez <lucas@fleetdm.com>
juan-fdz-hawa
had a problem deploying
to
Docker Hub
GitHub Actions
Error
Co-authored-by: Lucas Manuel Rodriguez <lucas@fleetdm.com>
juan-fdz-hawa
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/service/middleware/auth/api_only.go (1)
63-68: Optional: precompute allow-list fingerprints to avoid O(n) work per request.For API-only users with a non-empty
APIEndpoints, every request rebuilds a fingerprint for each allow-listed endpoint viaNewAPIEndpointFromTpl(...).Fingerprint(). If users commonly have more than a handful of entries, caching the fingerprint set on theUser(or computing it once at load time) turns this into an O(1) map lookup and removes repeated string allocation/normalization on the hot path.♻️ Sketch
- // Check whether the requested endpoint matches any of the user's allowed endpoints. - for _, ep := range v.User.APIEndpoints { - if fleet.NewAPIEndpointFromTpl(ep.Method, ep.Path).Fingerprint() == fp { - return next(ctx, request) - } - } + // Prefer a prebuilt set on the user, e.g. v.User.APIEndpointFingerprints() + // that memoizes the map once per user load. + if _, allowed := v.User.APIEndpointFingerprints()[fp]; allowed { + return next(ctx, request) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/middleware/auth/api_only.go` around lines 63 - 68, The loop that calls fleet.NewAPIEndpointFromTpl(...).Fingerprint() for every request is costly; change it to use a precomputed fingerprint set on the user (e.g., add a field like User.APIEndpointFingerprints map[string]struct{} or []string populated once when the User is loaded or first validated) and replace the O(n) loop in the middleware with a constant-time map lookup against that set; update the code paths that construct User.APIEndpoints to also populate the fingerprint set (or lazily populate it on first access with synchronization) and then have the middleware compare the request fingerprint (from Fingerprint()) against User.APIEndpointFingerprints instead of rebuilding fingerprints each request.
🤖 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/middleware/auth/api_only.go`:
- Around line 63-68: The loop that calls
fleet.NewAPIEndpointFromTpl(...).Fingerprint() for every request is costly;
change it to use a precomputed fingerprint set on the user (e.g., add a field
like User.APIEndpointFingerprints map[string]struct{} or []string populated once
when the User is loaded or first validated) and replace the O(n) loop in the
middleware with a constant-time map lookup against that set; update the code
paths that construct User.APIEndpoints to also populate the fingerprint set (or
lazily populate it on first access with synchronization) and then have the
middleware compare the request fingerprint (from Fingerprint()) against
User.APIEndpointFingerprints instead of rebuilding fingerprints each request.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2834b334-e334-4991-ab07-33c1726bff3c
📒 Files selected for processing (4)
server/activity/internal/service/endpoint_utils.goserver/fleet/api_endpoints.goserver/service/integration_core_test.goserver/service/middleware/auth/api_only.go
✅ Files skipped from review due to trivial changes (1)
- server/fleet/api_endpoints.go
2 participants
Fixes #42885
Added new middleware (APIOnlyEndpointCheck) that enforces 403 for API-only users whose request either isn't in the API endpoint catalog or falls outside their configured per-user endpoint restrictions.
Related issue: Resolves #
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
Summary by CodeRabbit
New Features
Bug Fixes