Conversation
- Added user_api_endpoints table to track per user API endpoint permissions. - Added service/api_endpoints, used to handle service/api_endpoints.yml artifact. - Added check on server start that makes sure that service/apin_endpoints.yml is a subset of router routes.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #43077 +/- ##
==========================================
- Coverage 66.86% 66.85% -0.02%
==========================================
Files 2578 2580 +2
Lines 206869 206933 +64
Branches 9168 9168
==========================================
+ Hits 138328 138345 +17
- Misses 55978 56012 +34
- Partials 12563 12576 +13
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:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis change introduces API endpoint management by adding a new 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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)
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.
Actionable comments posted: 2
🤖 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/datastore/mysql/schema.sql`:
- Line 2977: The column is_allowed currently permits NULL causing tri-state
permission ambiguity; create a new migration that alters the column is_allowed
to be NOT NULL with DEFAULT 1 (e.g., ALTER TABLE ... MODIFY COLUMN `is_allowed`
TINYINT(1) NOT NULL DEFAULT 1 or the equivalent in your migration DSL), run the
migration and then regenerate the auto-generated
server/datastore/mysql/schema.sql from migrations so tests use the updated
schema.
In `@server/service/api_endpoints.yml`:
- Around line 1-4: The endpoint definition for the POST
"/api/_version_/fleet/trigger" contains a placeholder `name` value ("Some wild
description goes here"); replace this with a concise, meaningful description
(e.g., "Trigger fleet operation" or similar) in the `name` field of the API
spec, or if this file is only used to test validation, add a clarifying comment
next to the `name` entry indicating it's intentionally a placeholder for tests.
🪄 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: 5f497943-3672-4193-88cc-c0598da52242
📒 Files selected for processing (7)
changes/42881-api-endpoints-initial-modelscmd/fleet/serve.goserver/datastore/mysql/migrations/tables/20260406114157_AddApiEndpointPermissionsTables.goserver/datastore/mysql/schema.sqlserver/service/api_endpoints.goserver/service/api_endpoints.ymlserver/service/api_endpoints_test.go
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.
| _, err := tx.Exec(` | ||
| CREATE TABLE user_api_endpoints ( | ||
| user_id INT UNSIGNED NOT NULL, | ||
| endpoint_hash BINARY(32) NOT NULL, |
There was a problem hiding this comment.
This is meant to store the encoded method + path: UNHEX(SHA2('|GET|/api/v1/reports|', 256) for example.
| user_id INT UNSIGNED NOT NULL, | ||
| endpoint_hash BINARY(32) NOT NULL, | ||
|
|
||
| is_allowed BOOLEAN DEFAULT TRUE NOT NULL, |
There was a problem hiding this comment.
This will allow us to express: "Allow all API endpoints, except these" access rules if needed.
| @@ -0,0 +1,4 @@ | |||
| - method: POST | |||
There was a problem hiding this comment.
This is a placeholder. This file will be replaced on a subsequent PR.
| []endpointer.HandlerRoutesFunc{android_service.GetRoutes(svc, androidSvc), activityRoutes, acmeRoutes}, extra...) | ||
|
|
||
| if ok, missing := service.ValidateAPIEndpoints(apiHandler); !ok { | ||
| panic(fmt.Sprintf("api_endpoints.yml contains routes not registered in the router: %v", missing)) |
There was a problem hiding this comment.
Since api_endpoints.yml is owned by us, I think a panic here is OK
Related issue: Resolves #42881
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
Database migrations
Summary by CodeRabbit
Release Notes