fix(auth): allow API token to perform write/execute operations #1486
Conversation
WalkthroughMiddleware now treats successful API-token authentication as an admin identity by injecting a synthetic admin user into the request context. A new test verifies that in builtin auth mode an API token permits write/execute/delete operations while unauthenticated requests remain rejected. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware
participant AuthStore as API Token Verifier
participant Handler
participant PermissionCheck
Client->>Middleware: HTTP POST /api/v2/dags (with API token)
Middleware->>AuthStore: validate API token
AuthStore-->>Middleware: token valid (metadata)
Middleware->>Middleware: inject synthetic admin user into context
Middleware->>Handler: forward request (with admin context)
Handler->>PermissionCheck: check permissions (reads context)
PermissionCheck-->>Handler: allow (admin)
Handler-->>Client: 201 Created
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/service/frontend/api/v2/auth_test.go (1)
187-222: Good regression test coverage for issue #1478.The test comprehensively validates that API tokens can perform write (create), execute (start), and delete operations in builtin auth mode, which was the core issue being fixed.
Minor suggestion: Consider adding an assertion that write operations fail without authentication, similar to the pattern used in other tests (e.g., line 165 in
TestAuth_BuiltinModeWithAPIToken). This would make the test self-contained as a regression test:+ // Without auth - write should fail + server.Client().Post("/api/v2/dags", api.CreateNewDAGJSONRequestBody{ + Name: "api_token_test_dag", + Spec: &spec, + }).ExpectStatus(http.StatusUnauthorized).Send(t) + // Test write operation (create DAG) with API token - this was failing before the fix server.Client().Post("/api/v2/dags", api.CreateNewDAGJSONRequestBody{
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/service/frontend/api/v2/auth_test.go(1 hunks)internal/service/frontend/auth/middleware.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Backend entrypoint incmd/orchestrates the scheduler and CLI; runtime, persistence, and service layers sit underinternal/*(for exampleinternal/runtime,internal/persistence)
Keep Go filesgofmt/goimportsclean; use tabs, PascalCase for exported symbols (SchedulerClient), lowerCamelCase for locals, andErr...names for package-level errors
Repository linting relies ongolangci-lint; prefer idiomatic Go patterns, minimal global state, and structured logging helpers ininternal/common
Files:
internal/service/frontend/api/v2/auth_test.gointernal/service/frontend/auth/middleware.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Co-locate Go tests as*_test.go; favour table-driven cases and cover failure paths
Usestretchr/testify/requireand shared fixtures frominternal/testinstead of duplicating mocks
Files:
internal/service/frontend/api/v2/auth_test.go
🧬 Code graph analysis (2)
internal/service/frontend/api/v2/auth_test.go (4)
internal/test/server.go (2)
SetupServer(26-63)Server(21-23)internal/test/helper.go (1)
WithConfigMutator(72-76)internal/common/config/config.go (2)
Auth(144-150)AuthModeBuiltin(138-138)api/v2/api.gen.go (2)
CreateNewDAGJSONRequestBody(1612-1612)ExecuteDAGJSONRequestBody(1627-1627)
internal/service/frontend/auth/middleware.go (4)
api/v2/api.gen.go (1)
User(991-1006)internal/auth/role.go (2)
Role(16-16)RoleAdmin(20-20)internal/auth/context.go (1)
WithUser(17-19)internal/core/execution/context.go (1)
WithContext(241-243)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test on ubuntu-latest
🔇 Additional comments (1)
internal/service/frontend/auth/middleware.go (1)
89-98: LGTM - Synthetic admin user injection for API token auth.The approach correctly injects an admin user context so that downstream permission checks (
requireWrite,requireExecute,requireAdmin) function properly. The hardcoded"api-token"identifier provides clear audit trail for API token-authenticated requests.One consideration: API tokens now unconditionally grant full admin privileges. If finer-grained API token permissions are needed in the future (e.g., read-only tokens), this design would need revisiting. For now, this aligns with the PR objective.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/service/frontend/api/v2/auth_test.go (1)
187-228: Excellent regression test for issue #1478.The test properly validates that API tokens can perform write (create), execute (start), and delete operations when builtin auth mode is enabled. The setup is correct, the test flow is logical, and it aligns with existing test patterns in the file.
Optional: Consider testing unauthorized access for start/delete operations.
For consistency and completeness, you could add unauthorized checks before the start and delete operations (similar to lines 207-211 for create). While the middleware applies uniformly to all endpoints and this may seem redundant, it would make the test coverage more explicit and self-documenting.
Example:
}).WithBearerToken("my-api-token").ExpectStatus(http.StatusCreated).Send(t) + // Without auth - execute should fail + server.Client().Post("/api/v2/dags/api_token_test_dag/start", api.ExecuteDAGJSONRequestBody{}). + ExpectStatus(http.StatusUnauthorized).Send(t) + // Test execute operation (start DAG) with API token - this was also failing server.Client().Post("/api/v2/dags/api_token_test_dag/start", api.ExecuteDAGJSONRequestBody{}). WithBearerToken("my-api-token"). ExpectStatus(http.StatusOK).Send(t) + // Without auth - delete should fail + server.Client().Delete("/api/v2/dags/api_token_test_dag"). + ExpectStatus(http.StatusUnauthorized).Send(t) + // Test delete operation with API token server.Client().Delete("/api/v2/dags/api_token_test_dag"). WithBearerToken("my-api-token").
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/service/frontend/api/v2/auth_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Backend entrypoint incmd/orchestrates the scheduler and CLI; runtime, persistence, and service layers sit underinternal/*(for exampleinternal/runtime,internal/persistence)
Keep Go filesgofmt/goimportsclean; use tabs, PascalCase for exported symbols (SchedulerClient), lowerCamelCase for locals, andErr...names for package-level errors
Repository linting relies ongolangci-lint; prefer idiomatic Go patterns, minimal global state, and structured logging helpers ininternal/common
Files:
internal/service/frontend/api/v2/auth_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Co-locate Go tests as*_test.go; favour table-driven cases and cover failure paths
Usestretchr/testify/requireand shared fixtures frominternal/testinstead of duplicating mocks
Files:
internal/service/frontend/api/v2/auth_test.go
🧬 Code graph analysis (1)
internal/service/frontend/api/v2/auth_test.go (4)
internal/test/server.go (2)
SetupServer(26-63)Server(21-23)internal/service/frontend/server.go (1)
Server(40-46)internal/common/config/config.go (2)
Auth(144-150)AuthModeBuiltin(138-138)api/v2/api.gen.go (2)
CreateNewDAGJSONRequestBody(1612-1612)ExecuteDAGJSONRequestBody(1627-1627)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test on ubuntu-latest
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1486 +/- ##
==========================================
- Coverage 59.89% 59.87% -0.02%
==========================================
Files 195 195
Lines 21918 21918
==========================================
- Hits 13127 13124 -3
- Misses 7385 7388 +3
Partials 1406 1406 see 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Issue #1478
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.