Distinguish API-only endpoint-restriction 403s from role-based permission denials - #50816
Distinguish API-only endpoint-restriction 403s from role-based permission denials#50816lukeheath wants to merge 3 commits into
Conversation
Requests denied by an API-only user's endpoint allowlist now return a distinct 403 message and log the route template and denial reason at info level, so these denials are identifiable in server logs and error responses instead of looking like role-based permission denials. Fixes #50813
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAPI-only endpoint restriction denials now return a distinct 403 message. Catalog, allowlist, and missing-context failures use specialized denial handling. The middleware logs the route and denial reason at info level. Middleware and enterprise integration tests verify the response message and structured logging fields. A changelog entry documents the behavior. Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.45.0)server/service/integration_enterprise_test.goast-grep timed out on this file 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 |
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 improves debuggability of API-only user endpoint restriction denials by making them distinguishable from role-based permission denials, both in the 403 response body and in request logging.
Changes:
- Return a 403
UserMessageError(wrapping aPermissionError) for endpoint-restriction denials so the denial reason survives JSON encoding. - Force request logging to
INFOand attach structured extras (denied_by,denial_reason,route) when endpoint restrictions deny a request. - Update/extend unit tests to assert the new error type/message and the logging context fields.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| server/service/middleware/auth/api_only.go | Adds distinct endpoint-restriction denial error + info-level log extras for API-only restrictions. |
| server/service/middleware/auth/api_only_test.go | Updates assertions to the new UserMessageError and adds a test for log level/extras on denial. |
| changes/50813-distinguish-api-only-endpoint-restriction-403s | User-visible changes entry (content excluded from review output by policy). |
Files excluded by content exclusion policy (1)
- changes/50813-distinguish-api-only-endpoint-restriction-403s
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| requestMethod, _ := ctx.Value(kithttp.ContextKeyRequestMethod).(string) | ||
| routeTemplate, _ := eu.RouteTemplateFromContext(ctx) | ||
|
|
||
| fp := fleet.NewAPIEndpointFromTpl(requestMethod, routeTemplate).Fingerprint() | ||
|
|
||
| if !isInCatalog(fp) { | ||
| return nil, permissionDenied(ctx) | ||
| return nil, endpointRestrictionDenied(ctx, routeTemplate, "endpoint not in API endpoint catalog") | ||
| } |
There was a problem hiding this comment.
Fixed in 7d60f3e. The middleware now fails closed with a distinct denial_reason ("request method or route template missing from request context") before computing the fingerprint, instead of reporting a misleading catalog miss with an empty route. Covered by a new case in the log-extras subtest.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #50816 +/- ##
=======================================
Coverage 68.52% 68.53%
=======================================
Files 3977 3977
Lines 256094 256153 +59
Branches 13658 13658
=======================================
+ Hits 175489 175549 +60
+ Misses 64987 64986 -1
Partials 15618 15618
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:
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![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
🤖 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/middleware/auth/api_only_test.go`:
- Around line 51-62: Extend the endpoint restriction denial tests around
requireEndpointRestrictionDenied with a request-level assertion that executes
the middleware flow and verifies the HTTP response status is
http.StatusForbidden and the response body contains
EndpointRestrictionDeniedMessage. Keep the existing error-level assertions
unchanged and use the established request/response test helpers.
🪄 Autofix
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 Plus
Run ID: 860a921b-a5ec-4d9b-860a-06c03ef42f29
📒 Files selected for processing (3)
changes/50813-distinguish-api-only-endpoint-restriction-403sserver/service/middleware/auth/api_only.goserver/service/middleware/auth/api_only_test.go
| // requireEndpointRestrictionDenied asserts that err is the 403 returned when an | ||
| // API-only user's endpoint restrictions deny a request: a UserMessageError | ||
| // carrying EndpointRestrictionDeniedMessage, distinguishable from a role-based | ||
| // permission denial. | ||
| func requireEndpointRestrictionDenied(t *testing.T, err error) { | ||
| t.Helper() | ||
| require.Error(t, err) | ||
| var umErr *fleet.UserMessageError | ||
| require.ErrorAs(t, err, &umErr) | ||
| require.Equal(t, http.StatusForbidden, umErr.StatusCode()) | ||
| require.Equal(t, EndpointRestrictionDeniedMessage, umErr.UserMessage()) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the changed test before inspecting encoder-related call sites.
ast-grep outline server/service/middleware/auth/api_only_test.go --items all
# Locate the error type, its encoder, and request-level tests that cover it.
rg -n -C 5 --type go \
'NewUserMessageError|UserMessageError|UserMessage\(\)|StatusCode\(\)|PermissionError' \
serverRepository: fleetdm/fleet
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== api_only_test.go relevant lines =="
sed -n '1,140p' server/service/middleware/auth/api_only_test.go
echo
echo "== endpointor error encoding relevant lines =="
sed -n '90,215p' server/platform/endpointer/transport_error.go
echo
echo "== apiOnlyEndpointRestrictionDenied identifier sources =="
rg -n -C 8 --type go 'apiOnlyEndpointRestrictionDenied|EndpointRestrictionDeniedMessage|requireEndpointRestrictionDenied|NewUserMessageError\(.*EndpointRestrictionDenied' server || true
echo
echo "== request-level tests mentioning 403 and restrictive messages =="
rg -n --type go 'EndpointRestrictionDeniedMessage|requireEndpointRestrictionDenied|StatusCode\(\) == http\.StatusForbidden|require\.Equal\(t, http\.StatusForbidden' server/service server/platform || trueRepository: fleetdm/fleet
Length of output: 30407
Add a request-level assertion for endpoint restriction denials.
requireEndpointRestrictionDenied only checks the error returned by the middleware. Add a real HTTP response assertion that this error encodes http.StatusForbidden with EndpointRestrictionDeniedMessage in the request flow.
🤖 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/middleware/auth/api_only_test.go` around lines 51 - 62, Extend
the endpoint restriction denial tests around requireEndpointRestrictionDenied
with a request-level assertion that executes the middleware flow and verifies
the HTTP response status is http.StatusForbidden and the response body contains
EndpointRestrictionDeniedMessage. Keep the existing error-level assertions
unchanged and use the established request/response test helpers.
There was a problem hiding this comment.
Added in 7d60f3e: a unit test (TestEndpointRestrictionDeniedEncoding) pins the encoded 403 response (status + message in body) through the real endpointer.EncodeError path, and TestAPIOnlyUserEndpointMiddleware now asserts the response body contains EndpointRestrictionDeniedMessage on both denial paths in the full request flow.
…ody test coverage - Fail closed with a distinct denial_reason when the request method or mux route template is missing from context, instead of reporting a misleading catalog miss with an empty route. - Pin the encoded 403 response (status + distinct message) at the unit level and assert the response body in the endpoint middleware integration test.
Related issue: Resolves #50813
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
Details
apiOnlyEndpointCheckreturned the same genericfleet.NewPermissionError("forbidden")for both endpoint-restriction failure modes, making these 403s indistinguishable from role-based permission denials in both the response body and server logs.This PR makes endpoint-restriction denials identifiable in two places:
403 response body. The denial now returns a
UserMessageErrorwrapping the permission error, so the message survives encoding (the error encoder discardsPermissionErrormessages and renders a generic "Permission Denied" body):{ "message": "Forbidden", "errors": [ { "name": "base", "reason": "endpoint not permitted for this API-only user" } ], "uuid": "..." }Role-based denials are unchanged (
"forbidden"). The restriction list is not secret from the caller (they already hold a valid token), so naming the gate discloses nothing.Server logs. The denial is surfaced on the request log line at info level (role-based 403s log at debug, which is how these went unnoticed) with a denial marker, the reason, and the matched route template. User email and request method were already on the line:
The two failure modes (route not in the API endpoint catalog vs. route not on the user's allowlist) get distinct
denial_reasonvalues in the log; the response body message is the same for both.Verified the encoded response by running the error through the real
endpointer.EncodeErrorpath. Existing integration tests assert only the 403 status, which is unchanged.Summary by CodeRabbit