fix(auth): allow API token auth to work alongside builtin mode #1480
Conversation
WalkthroughConsolidates v2 authentication into a unified middleware that supports JWT Bearer, API tokens, Basic, and OIDC (JWT checked first when configured); adds comprehensive v2 auth tests, adjusts test-server helpers and config paths, warns when basic auth is present with builtin mode, updates UI token handling and fetch header ordering, and extends API error codes and generated swagger. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant APIRouter
participant AuthMiddleware
participant JWTValidator
participant APITokenValidator
participant BasicAuthValidator
participant OIDCValidator
participant UserContext
Client->>APIRouter: HTTP request (may include Authorization header)
APIRouter->>AuthMiddleware: invoke unified middleware
alt JWTValidator configured
AuthMiddleware->>JWTValidator: validate Bearer token
alt JWT valid
JWTValidator-->>AuthMiddleware: user identity
AuthMiddleware->>UserContext: inject user
AuthMiddleware-->>APIRouter: proceed (authenticated)
else JWT invalid
AuthMiddleware->>APITokenValidator: validate API token
alt API token valid
APITokenValidator-->>AuthMiddleware: user identity
AuthMiddleware->>UserContext: inject user
AuthMiddleware-->>APIRouter: proceed (authenticated)
else
AuthMiddleware->>BasicAuthValidator: attempt Basic auth (if enabled)
alt Basic auth valid
BasicAuthValidator-->>AuthMiddleware: user identity
AuthMiddleware->>UserContext: inject user
AuthMiddleware-->>APIRouter: proceed (authenticated)
else
AuthMiddleware->>OIDCValidator: validate OIDC (if configured)
alt OIDC valid
OIDCValidator-->>AuthMiddleware: user identity
AuthMiddleware->>UserContext: inject user
AuthMiddleware-->>APIRouter: proceed (authenticated)
else All methods failed
AuthMiddleware-->>APIRouter: issue challenge (Basic or Bearer)
APIRouter-->>Client: 401 Unauthorized
end
end
end
end
else (no JWTValidator)
AuthMiddleware->>APITokenValidator: validate API token (then Basic/OIDC fallback)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 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: 1
🧹 Nitpick comments (1)
internal/service/frontend/auth/middleware.go (1)
73-91: Consider minor efficiency improvement: JWT validation precedes API token check.When both JWT and API token authentication are enabled, Bearer tokens are first validated as JWTs (line 76), and only if that fails, validated as static API tokens (line 88). This means valid API tokens will go through JWT validation (and fail) before succeeding. While functionally correct, this adds a small overhead for API token users in builtin mode.
If this becomes a performance concern, you could optimize by checking token format or using different header schemes, but this is likely acceptable for most use cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/service/frontend/api/v2/api.go(1 hunks)internal/service/frontend/api/v2/auth_test.go(1 hunks)internal/service/frontend/auth/middleware.go(5 hunks)internal/test/helper.go(2 hunks)internal/test/server.go(5 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/test/helper.gointernal/service/frontend/api/v2/api.gointernal/test/server.gointernal/service/frontend/auth/middleware.gointernal/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
🧠 Learnings (2)
📚 Learning: 2025-12-04T10:34:17.062Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.062Z
Learning: Applies to **/*_test.go : Co-locate Go tests as `*_test.go`; favour table-driven cases and cover failure paths
Applied to files:
internal/service/frontend/api/v2/auth_test.go
📚 Learning: 2025-12-04T10:34:17.062Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.062Z
Learning: Applies to **/*_test.go : Use `stretchr/testify/require` and shared fixtures from `internal/test` instead of duplicating mocks
Applied to files:
internal/service/frontend/api/v2/auth_test.go
🧬 Code graph analysis (3)
internal/test/helper.go (1)
internal/common/config/path.go (1)
Paths(13-30)
internal/service/frontend/api/v2/api.go (3)
internal/common/config/config.go (1)
AuthModeBuiltin(138-138)internal/service/frontend/auth/middleware.go (1)
Middleware(50-117)internal/service/frontend/api/v2/remote.go (1)
WithRemoteNode(23-134)
internal/service/frontend/auth/middleware.go (4)
internal/service/frontend/auth/builtin.go (1)
TokenValidator(17-19)internal/service/frontend/api/pathutil/pathutil.go (1)
NormalizePath(36-47)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 (4)
internal/service/frontend/api/v2/api.go (1)
222-243: LGTM! Unified authentication middleware consolidation.The consolidation of authentication into a single route group with unified middleware handling (JWT, API token, Basic Auth, OIDC) simplifies the routing logic and aligns with the PR objective to allow multiple authentication methods to work alongside builtin mode. The inline comments clearly document the flow.
internal/service/frontend/api/v2/auth_test.go (1)
1-241: LGTM! Comprehensive authentication test coverage.The test suite thoroughly covers all authentication scenarios including the key fix for issue #1478 (TestAuth_BuiltinModeWithAPIToken). The tests follow Go best practices with testify/require assertions and consistent structure covering both success and failure paths.
Based on learnings, the tests properly use shared fixtures from
internal/testand follow the table-driven pattern via config mutations.internal/test/server.go (2)
46-60: LGTM! Improved server startup with async error handling.The asynchronous startup pattern with error channel and timeout prevents tests from hanging indefinitely if the server fails to start. The 5-second timeout is reasonable for a test environment.
176-179: No changes needed—Basic Auth implementation is RFC 7617 compliant.The
basicAuthfunction correctly implements the RFC 7617 specification: it constructsusername:password, converts to bytes (US-ASCII compatible), and encodes using Go'sbase64.StdEncoding, which implements RFC 4648 Section 4. No modifications required.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/service/frontend/api/v2/api.go (1)
190-196: LGTM! Basic auth correctly disabled in builtin mode.The logic properly disables basic auth when builtin mode is enabled, preventing conflicts between the two authentication methods. This aligns with the PR objective to allow API token auth alongside builtin mode.
Consider adding a reference to the config loader warning (if one exists) in the comment to help maintainers understand the full picture.
</comment_end -->
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/service/frontend/api/v2/api.go(2 hunks)internal/service/frontend/api/v2/auth_test.go(1 hunks)internal/service/frontend/server.go(0 hunks)
💤 Files with no reviewable changes (1)
- internal/service/frontend/server.go
🧰 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/api/v2/api.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
🧠 Learnings (2)
📚 Learning: 2025-12-04T10:34:17.062Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.062Z
Learning: Applies to **/*_test.go : Co-locate Go tests as `*_test.go`; favour table-driven cases and cover failure paths
Applied to files:
internal/service/frontend/api/v2/auth_test.go
📚 Learning: 2025-12-04T10:34:17.062Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.062Z
Learning: Applies to **/*_test.go : Use `stretchr/testify/require` and shared fixtures from `internal/test` instead of duplicating mocks
Applied to 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)
LoginRequest(552-558)LoginResponse(561-570)
internal/service/frontend/api/v2/api.go (3)
internal/common/config/config.go (1)
AuthModeBuiltin(138-138)internal/service/frontend/auth/middleware.go (2)
Options(16-31)Middleware(50-117)internal/service/frontend/api/v2/remote.go (1)
WithRemoteNode(23-134)
⏰ 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). (2)
- GitHub Check: Test on ubuntu-latest
- GitHub Check: Build
🔇 Additional comments (12)
internal/service/frontend/api/v2/api.go (2)
219-227: Excellent implementation of JWT support for builtin mode.The JWT validator setup is clean and properly validates that authService is initialized. This enables the unified middleware to accept both JWT tokens (from login) and API tokens, which directly addresses issue #1478.
</comment_end -->
229-240: Clean consolidation into unified middleware.The refactor to a single router group with unified authentication middleware improves maintainability and ensures consistent auth handling across all routes. The middleware ordering (auth → remote node → handler) is correct.
</comment_end -->
internal/service/frontend/api/v2/auth_test.go (10)
14-20: LGTM! Baseline test validates no-auth behavior.The test correctly verifies that requests succeed when no authentication is configured.
</comment_end -->
22-52: LGTM! Basic auth tests cover key scenarios.The tests properly validate both authentication failure (401) and success (200) cases with correct/incorrect credentials.
</comment_end -->
54-70: Excellent coverage of special character edge case.Testing passwords with special characters ensures proper encoding/decoding in the Basic Auth flow.
</comment_end -->
72-90: LGTM! API token test validates key scenarios.The test covers unauthorized access, invalid token, and valid token cases.
</comment_end -->
92-112: LGTM! Multi-method auth test validates parallel authentication.The test confirms that basic auth and API token can coexist and either method grants access independently.
</comment_end -->
114-141: LGTM! Builtin mode test validates JWT authentication flow.The test comprehensively validates the JWT-based authentication in builtin mode, including login, token generation, and token usage for subsequent requests.
</comment_end -->
143-177: Excellent test! This is the key validation for issue #1478.This test directly addresses the PR objective by verifying that API tokens work alongside builtin authentication mode. The test:
- Configures both builtin mode and API token (lines 146-154)
- Validates API token grants access (lines 159-162) - this was broken in #1478
- Validates JWT login still works (lines 164-176)
This serves as a regression test for the reported bug.
</comment_end -->
179-213: LGTM! Test validates basic auth is disabled in builtin mode.This test confirms the design decision to disable basic auth when builtin mode is active, preventing confusion between authentication methods. The test validates that basic auth credentials are rejected (lines 195-198) and only JWT authentication works.
</comment_end -->
215-236: LGTM! Public paths test validates selective authentication bypass.The test confirms that health and login endpoints remain publicly accessible while other endpoints require authentication, which is the expected behavior for public paths.
</comment_end -->
1-236: Outstanding test coverage for authentication scenarios.This new test file provides comprehensive coverage of all authentication modes and their interactions. The tests are well-organized, follow Go testing best practices, and directly validate the fix for issue #1478. Key strengths:
- Covers all authentication methods: None, Basic, API Token, JWT (builtin), OIDC-ready
- Tests method combinations and edge cases (special characters, public paths)
- The critical test
TestAuth_BuiltinModeWithAPITokenvalidates the PR's main objective- Uses recommended patterns: testify/require, test helpers, config mutators
</comment_end -->
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/service/frontend/api/v2/auth_test.go (2)
150-185: Key test for issue #1478 looks solid.This test correctly verifies that API tokens work alongside builtin JWT authentication, directly addressing the reported bug. The test flow is clear and comprehensive.
For consistency with
TestAuth_BuiltinMode(line 142), consider adding a token verification after login:var loginResult api.LoginResponse loginResp.Unmarshal(t, &loginResult) +require.NotEmpty(t, loginResult.Token, "expected JWT token in response") // With JWT token - should also succeedThis makes the test more defensive and consistent with the pattern established in
TestAuth_BuiltinMode.
187-222: Good verification that builtin mode takes precedence.This test correctly verifies that basic auth credentials are ignored when builtin mode is active, ensuring clear authentication mode behavior.
For consistency with
TestAuth_BuiltinMode(line 142), consider verifying the JWT token after login:var loginResult api.LoginResponse loginResp.Unmarshal(t, &loginResult) +require.NotEmpty(t, loginResult.Token, "expected JWT token in response") // With JWT token - should succeed
📜 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
🧠 Learnings (2)
📚 Learning: 2025-12-04T10:34:17.062Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.062Z
Learning: Applies to **/*_test.go : Co-locate Go tests as `*_test.go`; favour table-driven cases and cover failure paths
Applied to files:
internal/service/frontend/api/v2/auth_test.go
📚 Learning: 2025-12-04T10:34:17.062Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.062Z
Learning: Applies to **/*_test.go : Use `stretchr/testify/require` and shared fixtures from `internal/test` instead of duplicating mocks
Applied to 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/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)
LoginRequest(552-558)LoginResponse(561-570)
⏰ 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). (2)
- GitHub Check: Test on ubuntu-latest
- GitHub Check: Build
🔇 Additional comments (4)
internal/service/frontend/api/v2/auth_test.go (4)
1-12: LGTM! Solid test file structure.The package declaration, imports, and overall test organization follow Go best practices and coding guidelines. Black-box testing via
api_testpackage is appropriate, and the use oftestify/requirealigns with project conventions.
14-74: Excellent coverage of basic authentication scenarios.The tests thoroughly cover no-auth, basic auth requirements, correct/incorrect credentials, and edge cases like special characters in passwords. The parallel execution and consistent test patterns enhance maintainability.
76-118: Strong coverage of API token authentication.These tests verify API token behavior in isolation and alongside basic auth, which directly supports the PR's objective of allowing multiple authentication methods to coexist.
224-246: Solid coverage of public endpoint behavior.The test correctly verifies that health and login endpoints bypass authentication while other endpoints remain protected, ensuring the authentication boundary is properly defined.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1480 +/- ##
==========================================
+ Coverage 59.89% 59.93% +0.03%
==========================================
Files 196 195 -1
Lines 21901 21889 -12
==========================================
+ Hits 13118 13119 +1
+ Misses 7376 7363 -13
Partials 1407 1407
... and 6 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Issue: #1478, #1475
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.