feat: add proxy header authentication#2397
Merged
Merged
Conversation
This reverts commit 2d6672f.
📝 WalkthroughWalkthroughThis PR adds trusted-proxy authentication with validated configuration, proxy identity parsing, user provisioning, authorization synchronization, frontend login routing, API contracts, Helm support, and UI handling for externally managed users. ChangesTrusted proxy authentication
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Proxy
participant TrustedProxyLoginHandler
participant TrustedProxyProvisionService
participant UserStore
participant AuthService
Proxy->>TrustedProxyLoginHandler: GET /proxy-login with identity headers
TrustedProxyLoginHandler->>TrustedProxyProvisionService: ProcessLogin(identity, groups)
TrustedProxyProvisionService->>UserStore: Lookup or create and synchronize user
TrustedProxyProvisionService-->>TrustedProxyLoginHandler: Provisioned user
TrustedProxyLoginHandler->>AuthService: GenerateToken(user)
AuthService-->>TrustedProxyLoginHandler: Session token
TrustedProxyLoginHandler-->>Proxy: Redirect to login with token
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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)
internal/service/frontend/server.go (1)
585-590: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueNil-guard asymmetry on
srv.trustedProxyCfg.Line 580 guards
srv.trustedProxyCfg != nilbefore mutating it, but Line 588 dereferencessrv.trustedProxyCfg.Enabledunguarded. It's non-nil in all current construction paths, so not reachable today, but the inconsistency is a latent panic risk if a futureServerOptionsets it nil. Consider guarding Line 588 the same way.🛡️ Defensive guard
- if srv.licenseManager != nil && srv.trustedProxyCfg.Enabled && !srv.licenseManager.Checker().IsFeatureEnabled(license.FeatureSSO) { + if srv.licenseManager != nil && srv.trustedProxyCfg != nil && srv.trustedProxyCfg.Enabled && !srv.licenseManager.Checker().IsFeatureEnabled(license.FeatureSSO) {🤖 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 `@internal/service/frontend/server.go` around lines 585 - 590, Update the trusted-proxy license warning condition to verify srv.trustedProxyCfg is non-nil before accessing Enabled, matching the existing guard used when mutating trustedProxyCfg. Preserve the current warning behavior when the configuration exists, is enabled, and SSO is not licensed.internal/service/authmapping/mapper.go (1)
34-40: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate
DefaultRoleinNew()like every other role in this constructor.
New()validates everyGroupMappingsrole and everyWorkspaceMappingsrole (including rejectingadminas workspace-scoped), but never checksconfig.DefaultRole. BothMap()(line 183) andMapRole()(line 201) returnm.defaultRoledirectly on the non-strict fallback path, so an unset or malformedDefaultRolewould silently propagate an invalidauth.Roleinto a provisioned user's record instead of failing fast at construction, the way every other bad role value in this file does.🛠️ Proposed fix
func New(config Config) (*Mapper, error) { + if !config.DefaultRole.Valid() { + return nil, fmt.Errorf("invalid default role %q", config.DefaultRole) + } defaultWorkspaceAccess := config.DefaultWorkspaceAccessAlso applies to: 59-88, 183-183, 201-201
🤖 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 `@internal/service/authmapping/mapper.go` around lines 34 - 40, Update New() to validate config.DefaultRole using the same role-validation rules and error behavior applied to GroupMappings and WorkspaceMappings, while preserving the workspace-scoped admin restriction where applicable. Ensure invalid or unset defaults fail during construction before Map() or MapRole() can return m.defaultRole.
🤖 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 `@ui/src/pages/users/UserFormModal.tsx`:
- Around line 262-274: Update the muted explanatory text in the
authorizationManaged branch of UserFormModal, specifically the paragraph
following WorkspaceAccessSummary, to use text-slate-500 dark:text-slate-500
instead of text-muted-foreground. Preserve the existing text and layout classes.
---
Nitpick comments:
In `@internal/service/authmapping/mapper.go`:
- Around line 34-40: Update New() to validate config.DefaultRole using the same
role-validation rules and error behavior applied to GroupMappings and
WorkspaceMappings, while preserving the workspace-scoped admin restriction where
applicable. Ensure invalid or unset defaults fail during construction before
Map() or MapRole() can return m.defaultRole.
In `@internal/service/frontend/server.go`:
- Around line 585-590: Update the trusted-proxy license warning condition to
verify srv.trustedProxyCfg is non-nil before accessing Enabled, matching the
existing guard used when mutating trustedProxyCfg. Preserve the current warning
behavior when the configuration exists, is enabled, and SSO is not licensed.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fd13aad7-3a3f-4519-8fc8-3e1f58219448
📒 Files selected for processing (73)
api/v1/api.gen.goapi/v1/api.yamlcharts/dagu/Chart.yamlcharts/dagu/PROXY_AUTH.mdcharts/dagu/README.mdcharts/dagu/examples/proxy-network-policy.yamlcharts/dagu/templates/NOTES.txtcharts/dagu/templates/configmap.yamlcharts/dagu/templates/ui-deployment.yamlcharts/dagu/values.schema.jsoncharts/dagu/values.yamlinternal/auth/store.gointernal/auth/user.gointernal/auth/user_test.gointernal/cmn/config/config.gointernal/cmn/config/definition.gointernal/cmn/config/key_hints.gointernal/cmn/config/loader.gointernal/cmn/config/loader_test.gointernal/cmn/config/trusted_proxy_test.gointernal/cmn/schema/config.schema.jsoninternal/cmn/schema/config_schema_test.gointernal/persis/store/user.gointernal/persis/store/user_test.gointernal/service/auth/service.gointernal/service/auth/service_test.gointernal/service/authmapping/mapper.gointernal/service/authmapping/mapper_test.gointernal/service/frontend/api/v1/auth.gointernal/service/frontend/api/v1/auth_internal_test.gointernal/service/frontend/api/v1/auth_test.gointernal/service/frontend/api/v1/users.gointernal/service/frontend/api/v1/users_internal_test.gointernal/service/frontend/auth/middleware_test.gointernal/service/frontend/auth/trustedproxy.gointernal/service/frontend/auth/trustedproxy_headers.gointernal/service/frontend/auth/trustedproxy_headers_test.gointernal/service/frontend/auth/trustedproxy_test.gointernal/service/frontend/request_logging_test.gointernal/service/frontend/server.gointernal/service/frontend/server_test.gointernal/service/frontend/templates.gointernal/service/frontend/templates/base.gohtmlinternal/service/frontend/templates_test.gointernal/service/oidcprovision/rolemapper.gointernal/service/oidcprovision/service.gointernal/service/oidcprovision/service_test.gointernal/service/trustedproxyprovision/service.gointernal/service/trustedproxyprovision/service_test.goui/index.htmlui/src/__tests__/App.test.tsxui/src/__tests__/menu.test.tsxui/src/api/v1/schema.tsui/src/components/UserMenu.tsxui/src/components/__tests__/LicenseBanner.test.tsxui/src/components/__tests__/UserMenu.test.tsxui/src/contexts/ConfigContext.tsxui/src/features/cockpit/hooks/__tests__/useInfiniteKanban.test.tsxui/src/features/dag-runs/components/dag-run-list/__tests__/DAGRunTable.test.tsxui/src/features/dags/components/dag-details/__tests__/NotificationsTab.test.tsxui/src/lib/authSession.tsui/src/pages/__tests__/index.test.tsxui/src/pages/__tests__/login.test.tsxui/src/pages/__tests__/setup.test.tsxui/src/pages/api-docs/__tests__/index.test.tsxui/src/pages/api-keys/__tests__/index.test.tsxui/src/pages/dags/__tests__/index.test.tsxui/src/pages/event-logs/__tests__/index.test.tsxui/src/pages/license/__tests__/index.test.tsxui/src/pages/login.tsxui/src/pages/users/UserFormModal.tsxui/src/pages/users/__tests__/index.test.tsxui/src/pages/users/index.tsx
Collaborator
Author
|
@coderabbitai review Addressed all valid review feedback in f3dc134:
Also fixed an authorization synchronization race found during the follow-up audit. Proxy and OIDC now make the no-op/update decision atomically and log the actual previous authorization state. Deterministic regression tests cover both providers. |
|
✅ Action performedReview finished.
|
Collaborator
Author
|
@coderabbitai review Latest commit 4961b1d only simplifies the deterministic authorization-race test fixtures to satisfy staticcheck QF1008. Please review the current head. |
|
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds reverse-proxy header authentication as an optional “builtin” auth mode integration, with strict /proxy-login scoping, user provisioning, and managed-authorization synchronization. It also updates the UI/API/schema/config/Helm surfaces to expose proxy login and reflect externally managed users.
Changes:
- Introduced a dedicated trusted-proxy login handler (
GET /proxy-login) plus proxy user provisioning and identity persistence. - Unified/strengthened external authorization mapping and atomic role/workspace sync semantics (OIDC + Proxy).
- Updated UI, OpenAPI/schema, config validation, Helm chart, and documentation; added focused backend/frontend tests.
Reviewed changes
Copilot reviewed 74 out of 75 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| ui/src/pages/users/UserFormModal.tsx | Make role/workspace fields read-only when managed by external provider |
| ui/src/pages/users/index.tsx | Display managed-provider badges; hide password reset for external users |
| ui/src/pages/users/tests/index.test.tsx | Users page coverage for managed-provider combinations + proxy behavior |
| ui/src/pages/login.tsx | Add proxy login button/link; generalize callback handling to “external” |
| ui/src/pages/license/tests/index.test.tsx | Extend Config test fixtures with proxy fields |
| ui/src/pages/event-logs/tests/index.test.tsx | Extend Config test fixtures with proxy fields |
| ui/src/pages/dags/tests/index.test.tsx | Extend Config test fixtures with proxy fields |
| ui/src/pages/api-keys/tests/index.test.tsx | Extend Config test fixtures with proxy fields |
| ui/src/pages/api-docs/tests/index.test.tsx | Extend Config test fixtures with proxy fields |
| ui/src/pages/tests/setup.test.tsx | Extend Config test fixtures with proxy fields |
| ui/src/pages/tests/login.test.tsx | New tests covering proxy login link + separator behavior |
| ui/src/pages/tests/index.test.tsx | Extend Config test fixtures with proxy fields |
| ui/src/lib/authSession.ts | Replace oidc reason with generalized external |
| ui/src/features/dags/components/dag-details/tests/NotificationsTab.test.tsx | Extend Config test fixtures with proxy fields |
| ui/src/features/dag-runs/components/dag-run-list/tests/DAGRunTable.test.tsx | Extend Config test fixtures with proxy fields |
| ui/src/features/cockpit/hooks/tests/useInfiniteKanban.test.tsx | Extend Config test fixtures with proxy fields |
| ui/src/contexts/ConfigContext.tsx | Add proxyEnabled/proxyButtonLabel to UI config contract |
| ui/src/components/UserMenu.tsx | Hide “Change Password” for externally authenticated users |
| ui/src/components/tests/UserMenu.test.tsx | Coverage for proxy users not seeing password actions |
| ui/src/components/tests/LicenseBanner.test.tsx | Extend Config test fixtures with proxy fields |
| ui/src/api/v1/schema.ts | OpenAPI TS types updated (proxy provider + managed-provider metadata) |
| ui/src/tests/menu.test.tsx | Extend Config test fixtures with proxy fields |
| ui/src/tests/App.test.tsx | Extend Config test fixtures with proxy fields |
| ui/index.html | Add proxyEnabled/proxyButtonLabel in embedded UI config scaffold |
| internal/service/trustedproxyprovision/service.go | New proxy provisioning + authorization sync service |
| internal/service/oidcprovision/service.go | Switch to atomic SyncAuthorization and align skip-sync strict semantics |
| internal/service/oidcprovision/service_test.go | Extend OIDC provisioning tests for new sync/strict behavior |
| internal/service/oidcprovision/rolemapper.go | Refactor to use shared authmapping.Mapper for group/workspace mapping |
| internal/service/frontend/templates/base.gohtml | Plumb proxy config to UI; JS-escape auth button labels |
| internal/service/frontend/templates.go | Add proxyEnabled/proxyButtonLabel template funcs + license gating |
| internal/service/frontend/templates_test.go | Tests for licensed proxy enablement + JS escaping |
| internal/service/frontend/server.go | Register /proxy-login; enforce scoped header redaction; basePath hardening |
| internal/service/frontend/server_test.go | Route isolation tests for proxy headers + basePath validation tests |
| internal/service/frontend/request_logging_test.go | Ensure trusted-proxy headers are redacted in request logs |
| internal/service/frontend/auth/trustedproxy.go | New /proxy-login handler implementing strict parsing + redirects |
| internal/service/frontend/auth/trustedproxy_test.go | Handler success/failure coverage, including setup/license gates |
| internal/service/frontend/auth/trustedproxy_headers.go | Bounded/safe parsing for identity + CSV groups headers |
| internal/service/frontend/auth/trustedproxy_headers_test.go | Unit + fuzz tests for header parsing |
| internal/service/frontend/auth/middleware_test.go | Ensure proxy identity headers do not authenticate API middleware |
| internal/service/frontend/api/v1/users.go | Expose managedRoleProviders/managedWorkspaceAccessProviders in users list |
| internal/service/frontend/api/v1/users_internal_test.go | Coverage for managed-provider reporting + external-password reset denial |
| internal/service/frontend/api/v1/auth.go | Update “password managed externally” error mapping/message |
| internal/service/frontend/api/v1/auth_test.go | Route-isolation integration test: proxy headers ignored outside proxy-login |
| internal/service/frontend/api/v1/auth_internal_test.go | Change-password rejects external users message |
| internal/service/authmapping/mapper.go | New shared group→role/workspace authorization mapper |
| internal/service/authmapping/mapper_test.go | Mapper test coverage (merge semantics, strict mode, validation) |
| internal/service/auth/service.go | Treat any external provider as non-password; use Patch for updates |
| internal/service/auth/service_test.go | Coverage for external providers + patch/update concurrency behavior |
| internal/persis/store/user.go | Add trusted-proxy identity index + Patch + SyncAuthorization |
| internal/persis/store/user_test.go | Coverage for proxy identity indexing/immutability + Patch/SyncAuthorization |
| internal/cmn/schema/config.schema.json | Add auth.proxy schema and constraints |
| internal/cmn/schema/config_schema_test.go | Schema validation tests for proxy config cases |
| internal/cmn/config/loader_test.go | Ensure defaults include proxy auth subtree |
| internal/cmn/config/key_hints.go | Add legacy key hints for proxy auth config |
| internal/cmn/config/definition.go | Add proxy auth structs to config definitions |
| internal/cmn/config/config.go | Add validation for proxy auth (headers, mode, tunnel/headless constraints) |
| internal/auth/workspace_access.go | Add WorkspaceAccessEqual helper for canonical equality |
| internal/auth/workspace_access_test.go | Tests for WorkspaceAccessEqual |
| internal/auth/user.go | Add provider constants + trusted proxy identity fields + CanUsePassword |
| internal/auth/user_test.go | Tests for new user fields + CanUsePassword |
| internal/auth/store.go | Extend UserStore interfaces + new proxy identity errors/types |
| charts/dagu/values.yaml | Helm values for proxy auth config |
| charts/dagu/values.schema.json | Helm schema: proxy auth validation + UI replica constraint |
| charts/dagu/templates/ui-deployment.yaml | Enforce Recreate strategy + config checksum when proxy auth enabled |
| charts/dagu/templates/NOTES.txt | Warn about network trust boundary when proxy auth enabled |
| charts/dagu/templates/configmap.yaml | Render auth.proxy config + enforce guardrails/validation |
| charts/dagu/README.md | Document proxy auth availability and behavior |
| charts/dagu/PROXY_AUTH.md | New detailed proxy auth setup/security/rollback guide |
| charts/dagu/examples/proxy-network-policy.yaml | Example NetworkPolicy for UI isolation behind proxy |
| charts/dagu/Chart.yaml | Bump chart version |
| api/v1/api.yaml | OpenAPI updates: proxy provider enum + managed-provider fields + wording |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
auth.mode: builtin/proxy-loginbrowser endpoint, with strict parsing, redacted request logs, setup/license checks, and route-boundary security coverageChanges
Configuration
sourceis optional. Unmatched users are allowed by default with no named-workspace grants; setrequire_mapping: trueto reject them. Existing proxy users are synchronized on login unlessskip_org_role_sync: trueis configured.Security
Validation
make apiRelated Issues
N/A
Checklist
Summary by cubic
Adds reverse-proxy header authentication as an optional built-in login with strict route scoping. Also aligns OIDC authorization sync semantics with the proxy flow using atomic updates; updates UI, API wording, Helm, and docs; adds managed-provider reporting; and redacts proxy headers in logs.
New Features
GET /proxy-loginonly; all other routes ignore proxy headers.auth.mode: builtinviaauth.proxy.*(headers, button label, optionalsource, auto-signup, group→role/workspace mapping, strict matching).PROXY_AUTH.md).Migration
X-Auth-Request-User,X-Auth-Request-Groups) and enableauth.proxy.enabled: true.auth.proxy.roleMapping.requireMapping: trueto enforce mappings.auth.proxy.roleMapping.skipOrgRoleSync: trueis set.Written for commit ed43d44. Summary will update on new commits.
Summary by CodeRabbit
New Features
/proxy-loginflow, including identity header parsing, optional auto-signup, and group-to-role/workspace mapping.proxyas a supported auth provider and introduced “managed by Proxy/SSO” user handling viamanagedAuthorizationProviders(read-only role/workspace for managed users).Bug Fixes
Documentation
Chores
1.0.11.