feat(all): implement builtin user management feature #1463
Conversation
WalkthroughAdds builtin JWT-based authentication and RBAC (admin/manager/operator/viewer): file-backed user store, auth service (bcrypt + JWT), v2 API endpoints (/auth, /users), middleware and template/UI wiring, config validation and loader changes, and frontend login/protected routes and components. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Browser
participant FrontendUI
participant APIv2
participant AuthService
participant UserStore
Browser->>FrontendUI: POST /login (username,password)
FrontendUI->>APIv2: POST /auth/login
APIv2->>AuthService: Authenticate(username,password)
AuthService->>UserStore: GetByUsername(username)
UserStore-->>AuthService: user + password_hash
AuthService->>AuthService: compare bcrypt hash
AuthService-->>APIv2: authentication success + user
APIv2->>AuthService: GenerateToken(user)
AuthService-->>APIv2: JWT token + expiry
APIv2-->>FrontendUI: 200 { token, user, expiresAt }
FrontendUI->>Browser: store token, update UI
sequenceDiagram
autonumber
participant AdminBrowser
participant FrontendUI
participant APIv2
participant AuthService
participant UserStore
AdminBrowser->>FrontendUI: POST /users (Bearer token)
FrontendUI->>APIv2: POST /users
APIv2->>APIv2: requireAdmin(ctx) check
APIv2->>AuthService: CreateUser(input)
AuthService->>AuthService: validate & bcrypt-hash password
AuthService->>UserStore: Create(user)
UserStore-->>AuthService: persisted user
AuthService-->>APIv2: created user
APIv2-->>FrontendUI: 201 { user }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (16)
ui/src/features/system-status/components/ResourceChart.tsx (1)
76-76: Consider improving gradient ID uniqueness and character handling.The current approach
gradient-${title.replace(/\s+/g, '-')}has two potential issues:
Uniqueness: SVG gradient IDs are document-scoped. If multiple
ResourceChartinstances with the same title are rendered (e.g., multiple CPU charts for different servers), they'll share the same gradient ID, causing the last-defined gradient to overwrite previous ones and potentially display incorrect colors.Special characters: Titles containing characters like
(,),/will produce IDs with invalid CSS identifier characters. While browsers are lenient, this isn't spec-compliant.Consider these improvements:
- const gradientId = `gradient-${title.replace(/\s+/g, '-')}`; + const gradientId = `gradient-${title.replace(/[^a-zA-Z0-9-]/g, '-')}-${color.replace('#', '')}`;This sanitizes all special characters and includes the color to ensure uniqueness when the same metric is displayed with different colors. Alternatively, use React 18's
useId()hook for guaranteed uniqueness:import { useId } from 'react'; // Inside component: const uniqueId = useId(); const gradientId = `gradient-${uniqueId}`;ui/src/components/ChangePasswordModal.tsx (2)
50-53: Consider stronger password requirements.The minimum password length of 8 characters meets basic requirements but is on the lower end by modern security standards. Consider increasing to 12+ characters or adding complexity requirements (uppercase, lowercase, numbers, special characters).
58-69: Prefer token from AuthContext for consistency.The component directly accesses
localStorage.getItem('dagu_auth_token')instead of using theuseAuthcontext'stokenproperty. For consistency and maintainability, consider using the centralized auth state.Apply this diff:
export function ChangePasswordModal({ open, onClose }: ChangePasswordModalProps) { const config = useConfig(); + const { token } = useAuth(); const [currentPassword, setCurrentPassword] = useState(''); // ... const handleSubmit = async (e: React.FormEvent) => { // ... try { - const token = localStorage.getItem('dagu_auth_token'); const response = await fetch(`${config.apiURL}/auth/change-password`, {ui/src/lib/fetchJson.ts (1)
5-18: Consolidate auth header logic across three separate implementations.The codebase has auth header management duplicated in three distinct places:
fetchJsonwrapper (lines 5-13),authMiddlewareinapi.ts(lines 11-19), and manual token retrieval scattered in multiple components (AuthContext.tsx,pages/users/*,ChangePasswordModal.tsx). WhilefetchJsonserves as the SWR global fetcher andopenapi-fetchclient is the primary API layer, components still manually handle token retrieval and header setup. This creates three independent implementations of the same auth logic—if auth token storage or Bearer token format changes, all three must be updated. Consider whether raw fetch calls in components could migrate to either thefetchJsonwrapper oropenapi-fetchclient to reduce maintenance burden.internal/core/auth/role_test.go (1)
6-8: Consider using testify/require for assertions.The coding guidelines recommend using
stretchr/testify/requirefor test assertions instead of manual error checking witht.Errorf.Based on learnings, the project prefers testify/require for Go tests. Example refactor:
import ( "testing" + "github.com/stretchr/testify/require" )Then update assertions like:
-if got := tt.role.Valid(); got != tt.valid { - t.Errorf("Role(%q).Valid() = %v, want %v", tt.role, got, tt.valid) -} +require.Equal(t, tt.valid, tt.role.Valid(), "Role(%q).Valid()", tt.role)ui/src/components/UserMenu.tsx (1)
33-46: Consider extracting role colors as constants.The role strings are hardcoded in the switch statement. While they match the backend roles, extracting them as constants would improve maintainability and type safety.
Example:
const ROLE_COLORS: Record<string, string> = { admin: 'bg-red-500/20 text-red-600 dark:text-red-400', manager: 'bg-blue-500/20 text-blue-600 dark:text-blue-400', operator: 'bg-green-500/20 text-green-600 dark:text-green-400', viewer: 'bg-gray-500/20 text-gray-600 dark:text-gray-400', } as const; const getRoleBadgeColor = (role: string) => { return ROLE_COLORS[role] ?? ROLE_COLORS.viewer; };internal/persistence/fileuser/store_test.go (1)
1-14: Consider usingtestify/requirefor assertions.The test file uses manual error checking with
t.Fatalf/t.Errorf. Per coding guidelines, preferstretchr/testify/requirefor cleaner assertions and immediate test failure on first error.Example transformation:
import ( "context" "os" "path/filepath" "testing" "github.com/dagu-org/dagu/internal/core/auth" + "github.com/stretchr/testify/require" )Then assertions like:
-if err != nil { - t.Fatalf("failed to create temp dir: %v", err) -} +require.NoError(t, err, "failed to create temp dir")ui/src/pages/users/index.tsx (3)
44-48: Hardcoded token key should use a shared constant.The token key
'dagu_auth_token'is hardcoded here but defined asTOKEN_KEYinAuthContext.tsx. Consider exporting and reusing the constant to avoid duplication and potential drift.+import { TOKEN_KEY } from '@/contexts/AuthContext'; // ... - const token = localStorage.getItem('dagu_auth_token'); + const token = localStorage.getItem(TOKEN_KEY);
108-112: MovesetTitlecall touseEffectto avoid render-phase side effects.Calling
setTitleduring render can cause React warnings and is not idiomatic. UseuseEffectinstead.+import { useContext, useEffect } from 'react'; +import { AppBarContext } from '@/contexts/AppBarContext'; export default function UsersPage() { + const appBarContext = useContext(AppBarContext); // ... other state + useEffect(() => { + appBarContext.setTitle('User Management'); + }, [appBarContext]); + return ( - <AppBarContext.Consumer> - {(appBarContext) => { - if (appBarContext.title !== 'User Management') { - appBarContext.setTitle('User Management'); - } - - return ( - <div className="space-y-4"> + <div className="space-y-4"> // ... rest of content - </div> - ); - }} - </AppBarContext.Consumer> + </div> ); }
68-90: Clear error state on successful delete.The error state persists after successful operations. Clear it on success for better UX.
if (!response.ok) { const data = await response.json().catch(() => ({})); throw new Error(data.message || 'Failed to delete user'); } + setError(null); setDeletingUser(null); fetchUsers();internal/service/auth/service_test.go (1)
431-440: Consider usingfmt.Sprintffor clearer username generation.
string(rune('0'+i))works but is non-obvious. Usingfmt.Sprintf("user%d", i)would be clearer.for i := 0; i < 3; i++ { _, err := svc.CreateUser(ctx, CreateUserInput{ - Username: "user" + string(rune('0'+i)), + Username: fmt.Sprintf("user%d", i), Password: "password123", Role: auth.RoleViewer, })internal/service/frontend/auth/builtin.go (1)
52-63: Consider bidirectional trailing slash matching.The function only tries removing the trailing slash. If
publicPathscontains/path/but the request is/path, it won't match. Consider adding the reverse check for consistency:func isPublicPath(path string, publicSet map[string]struct{}) bool { // Exact match if _, ok := publicSet[path]; ok { return true } // Try with trailing slash removed if _, ok := publicSet[strings.TrimSuffix(path, "/")]; ok { return true } + // Try with trailing slash added + if _, ok := publicSet[path+"/"]; ok { + return true + } return false }internal/service/frontend/api/v2/auth.go (1)
100-106: Consider using 400 instead of 401 for password mismatch.The user is already authenticated at this point;
ErrPasswordMismatchindicates the current password verification failed, which is more of a validation error (400) than an authentication failure (401). However, if the intent is to treat current password verification as re-authentication, 401 is acceptable.internal/common/config/loader.go (1)
686-691: Missing environment variable binding for UsersDir.Other path configurations have environment variable bindings (e.g.,
DATA_DIR,LOG_DIR), butpaths.usersDiris missing. Consider adding for consistency:Add this binding in the "File paths" section (around line 709):
bindEnv("paths.usersDir", "USERS_DIR", asPath())internal/persistence/fileuser/store.go (1)
96-100: Missing logging for corrupted user files.The comment indicates a warning should be logged, but no actual logging is performed. This could make debugging difficult when user files are corrupted.
Consider adding structured logging:
+ // Log warning but continue - don't fail entire index for one bad file + // TODO: Add structured logging here when a user file fails to load continueOr if a logger is available in the codebase:
log.Warn("failed to load user file", "path", filePath, "error", err)internal/service/auth/service.go (1)
342-348: Consider adding password complexity requirements.The current validation only checks minimum length. For a user management system, consider adding complexity requirements (mixed case, numbers, special characters) to improve security posture.
Example enhancement:
func (s *Service) validatePassword(password string) error { if len(password) < minPasswordLength { return fmt.Errorf("%w: minimum length is %d characters", ErrWeakPassword, minPasswordLength) } // Optional: Add complexity checks var hasUpper, hasLower, hasDigit bool for _, c := range password { switch { case 'A' <= c && c <= 'Z': hasUpper = true case 'a' <= c && c <= 'z': hasLower = true case '0' <= c && c <= '9': hasDigit = true } } if !(hasUpper && hasLower && hasDigit) { return fmt.Errorf("%w: must contain uppercase, lowercase, and digit", ErrWeakPassword) } return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ui/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (43)
api/v2/api.yaml(4 hunks)internal/common/config/config.go(3 hunks)internal/common/config/config_test.go(1 hunks)internal/common/config/definition.go(2 hunks)internal/common/config/loader.go(5 hunks)internal/common/config/loader_test.go(6 hunks)internal/core/auth/context.go(1 hunks)internal/core/auth/role.go(1 hunks)internal/core/auth/role_test.go(1 hunks)internal/core/auth/store.go(1 hunks)internal/core/auth/user.go(1 hunks)internal/persistence/fileuser/store.go(1 hunks)internal/persistence/fileuser/store_test.go(1 hunks)internal/service/auth/service.go(1 hunks)internal/service/auth/service_test.go(1 hunks)internal/service/frontend/api/v2/api.go(8 hunks)internal/service/frontend/api/v2/auth.go(1 hunks)internal/service/frontend/api/v2/dagruns.go(8 hunks)internal/service/frontend/api/v2/dags.go(8 hunks)internal/service/frontend/api/v2/users.go(1 hunks)internal/service/frontend/auth/builtin.go(1 hunks)internal/service/frontend/server.go(4 hunks)internal/service/frontend/templates.go(2 hunks)internal/service/frontend/templates/base.gohtml(2 hunks)ui/index.html(1 hunks)ui/package.json(1 hunks)ui/src/App.tsx(3 hunks)ui/src/api/v2/schema.ts(3 hunks)ui/src/components/ChangePasswordModal.tsx(1 hunks)ui/src/components/ProtectedRoute.tsx(1 hunks)ui/src/components/UserMenu.tsx(1 hunks)ui/src/components/ui/dropdown-menu.tsx(1 hunks)ui/src/contexts/AuthContext.tsx(1 hunks)ui/src/contexts/ConfigContext.tsx(2 hunks)ui/src/features/system-status/components/ResourceChart.tsx(3 hunks)ui/src/hooks/api.ts(2 hunks)ui/src/layouts/Layout.tsx(2 hunks)ui/src/lib/fetchJson.ts(1 hunks)ui/src/menu.tsx(4 hunks)ui/src/pages/login.tsx(1 hunks)ui/src/pages/users/ResetPasswordModal.tsx(1 hunks)ui/src/pages/users/UserFormModal.tsx(1 hunks)ui/src/pages/users/index.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
ui/**/*.{ts,tsx,jsx,js}
📄 CodeRabbit inference engine (ui/CLAUDE.md)
ui/**/*.{ts,tsx,jsx,js}: Use developer-centric UI design with high information density, minimal whitespace, compact components, and no unnecessary decorations
Support both light and dark modes for all UI components using Tailwind CSS class pairs likedark:bg-slate-700
NEVER use full-page loading overlays or LoadingIndicator components that hide content - show stale data while fetching updates instead
Use compact modal design with small headers, minimal padding (p-2 or p-3), tight spacing, and support keyboard navigation (arrows, enter, escape)
Use small heights for form elements: select boxes h-7 or smaller, buttons h-7 or h-8, inputs with compact padding (py-0.5 or py-1)
Minimize row heights in tables and lists while maintaining readability, merge related columns, and always handle long text withwhitespace-normal break-words
Use consistent metadata styling withbg-slate-200 dark:bg-slate-700backgrounds and maintain text hierarchy with primary/secondary/muted text colors
Use flexbox-first layouts withmin-h-0andoverflow-hiddento prevent layout breaks, account for fixed elements when setting heights
Maintain keyboard navigation support in all interactive components with appropriate focus indicators and ARIA labels
Files:
ui/src/contexts/ConfigContext.tsxui/src/components/ProtectedRoute.tsxui/src/pages/login.tsxui/src/layouts/Layout.tsxui/src/components/ChangePasswordModal.tsxui/src/menu.tsxui/src/components/UserMenu.tsxui/src/App.tsxui/src/hooks/api.tsui/src/features/system-status/components/ResourceChart.tsxui/src/pages/users/UserFormModal.tsxui/src/pages/users/index.tsxui/src/contexts/AuthContext.tsxui/src/lib/fetchJson.tsui/src/pages/users/ResetPasswordModal.tsxui/src/components/ui/dropdown-menu.tsxui/src/api/v2/schema.ts
ui/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
ui/**/*.{ts,tsx}: The React + TypeScript frontend resides inui/, with production bundles copied tointernal/service/frontend/assetsbymake ui
UI code follows ESLint + Prettier (2-space indent) and Tailwind utilities; name React components in PascalCase (JobList.tsx) and hooks withuse*(useJobs.ts)
Files:
ui/src/contexts/ConfigContext.tsxui/src/components/ProtectedRoute.tsxui/src/pages/login.tsxui/src/layouts/Layout.tsxui/src/components/ChangePasswordModal.tsxui/src/menu.tsxui/src/components/UserMenu.tsxui/src/App.tsxui/src/hooks/api.tsui/src/features/system-status/components/ResourceChart.tsxui/src/pages/users/UserFormModal.tsxui/src/pages/users/index.tsxui/src/contexts/AuthContext.tsxui/src/lib/fetchJson.tsui/src/pages/users/ResetPasswordModal.tsxui/src/components/ui/dropdown-menu.tsxui/src/api/v2/schema.ts
**/*.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/core/auth/user.gointernal/service/frontend/auth/builtin.gointernal/service/frontend/api/v2/dagruns.gointernal/service/auth/service_test.gointernal/core/auth/role.gointernal/service/frontend/api/v2/dags.gointernal/core/auth/store.gointernal/service/frontend/api/v2/api.gointernal/common/config/config_test.gointernal/core/auth/context.gointernal/common/config/definition.gointernal/persistence/fileuser/store_test.gointernal/common/config/config.gointernal/service/frontend/api/v2/auth.gointernal/service/frontend/templates.gointernal/core/auth/role_test.gointernal/persistence/fileuser/store.gointernal/service/frontend/api/v2/users.gointernal/common/config/loader.gointernal/common/config/loader_test.gointernal/service/frontend/server.gointernal/service/auth/service.go
{api/v1,api/v2}/**/*.{proto,yaml,yml,json}
📄 CodeRabbit inference engine (AGENTS.md)
API definitions live in
api/v1andapi/v2; generated server stubs land ininternal/service, while matching TypeScript clients flow intoui/src/api
Files:
api/v2/api.yaml
**/*_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/auth/service_test.gointernal/common/config/config_test.gointernal/persistence/fileuser/store_test.gointernal/core/auth/role_test.gointernal/common/config/loader_test.go
🧠 Learnings (11)
📚 Learning: 2025-12-04T10:34:17.051Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.051Z
Learning: Applies to ui/**/*.{ts,tsx} : UI code follows ESLint + Prettier (2-space indent) and Tailwind utilities; name React components in PascalCase (`JobList.tsx`) and hooks with `use*` (`useJobs.ts`)
Applied to files:
ui/src/pages/login.tsxui/src/components/ui/dropdown-menu.tsx
📚 Learning: 2025-12-04T10:34:17.051Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.051Z
Learning: Applies to ui/**/*.{ts,tsx} : The React + TypeScript frontend resides in `ui/`, with production bundles copied to `internal/service/frontend/assets` by `make ui`
Applied to files:
ui/src/pages/login.tsxui/src/menu.tsxui/src/components/UserMenu.tsxui/src/pages/users/index.tsxui/src/components/ui/dropdown-menu.tsx
📚 Learning: 2025-12-04T10:34:01.023Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: ui/CLAUDE.md:0-0
Timestamp: 2025-12-04T10:34:01.023Z
Learning: Applies to ui/**/*.{ts,tsx,jsx,js} : Use developer-centric UI design with high information density, minimal whitespace, compact components, and no unnecessary decorations
Applied to files:
ui/src/layouts/Layout.tsxui/src/menu.tsxui/src/components/ui/dropdown-menu.tsx
📚 Learning: 2025-12-04T10:34:01.023Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: ui/CLAUDE.md:0-0
Timestamp: 2025-12-04T10:34:01.023Z
Learning: Applies to ui/**/*.{ts,tsx,jsx,js} : Use consistent metadata styling with `bg-slate-200 dark:bg-slate-700` backgrounds and maintain text hierarchy with primary/secondary/muted text colors
Applied to files:
ui/src/layouts/Layout.tsxui/src/features/system-status/components/ResourceChart.tsxui/src/components/ui/dropdown-menu.tsx
📚 Learning: 2025-12-04T10:34:01.023Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: ui/CLAUDE.md:0-0
Timestamp: 2025-12-04T10:34:01.023Z
Learning: Applies to ui/**/*.{ts,tsx,jsx,js} : Use compact modal design with small headers, minimal padding (p-2 or p-3), tight spacing, and support keyboard navigation (arrows, enter, escape)
Applied to files:
ui/src/pages/users/UserFormModal.tsxui/src/pages/users/ResetPasswordModal.tsx
📚 Learning: 2025-12-04T10:34:17.051Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.051Z
Learning: Applies to **/*_test.go : Use `stretchr/testify/require` and shared fixtures from `internal/test` instead of duplicating mocks
Applied to files:
internal/service/auth/service_test.gointernal/common/config/config_test.gointernal/persistence/fileuser/store_test.gointernal/core/auth/role_test.go
📚 Learning: 2025-12-04T10:34:17.051Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.051Z
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/auth/service_test.gointernal/persistence/fileuser/store_test.gointernal/core/auth/role_test.go
📚 Learning: 2025-12-04T10:34:17.051Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.051Z
Learning: `make test` (or `make test-coverage`) executes the Go suite via `gotestsum`; append `TEST_TARGET=./internal/...` to focus packages
Applied to files:
internal/service/auth/service_test.go
📚 Learning: 2025-12-04T10:34:17.051Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.051Z
Learning: Applies to **/*.go : Backend entrypoint in `cmd/` orchestrates the scheduler and CLI; runtime, persistence, and service layers sit under `internal/*` (for example `internal/runtime`, `internal/persistence`)
Applied to files:
internal/service/frontend/api/v2/api.gointernal/service/frontend/server.go
📚 Learning: 2025-12-04T10:34:01.023Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: ui/CLAUDE.md:0-0
Timestamp: 2025-12-04T10:34:01.023Z
Learning: Applies to ui/**/*.{ts,tsx,jsx,js} : Maintain keyboard navigation support in all interactive components with appropriate focus indicators and ARIA labels
Applied to files:
ui/src/components/ui/dropdown-menu.tsx
📚 Learning: 2025-12-04T10:34:01.023Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: ui/CLAUDE.md:0-0
Timestamp: 2025-12-04T10:34:01.023Z
Learning: Applies to ui/**/*.{ts,tsx,jsx,js} : Minimize row heights in tables and lists while maintaining readability, merge related columns, and always handle long text with `whitespace-normal break-words`
Applied to files:
ui/src/components/ui/dropdown-menu.tsx
🧬 Code graph analysis (26)
ui/src/contexts/ConfigContext.tsx (1)
internal/common/config/config.go (1)
AuthMode(131-131)
ui/src/components/ProtectedRoute.tsx (2)
ui/src/contexts/ConfigContext.tsx (1)
useConfig(38-40)ui/src/contexts/AuthContext.tsx (2)
useAuth(119-125)hasRole(151-153)
ui/src/pages/login.tsx (5)
ui/src/contexts/ConfigContext.tsx (1)
useConfig(38-40)ui/src/contexts/AuthContext.tsx (1)
useAuth(119-125)ui/src/components/ui/label.tsx (1)
Label(22-22)ui/src/components/ui/input.tsx (1)
Input(21-21)ui/src/components/ui/button.tsx (1)
Button(59-59)
ui/src/layouts/Layout.tsx (1)
ui/src/components/UserMenu.tsx (1)
UserMenu(17-94)
internal/core/auth/user.go (1)
internal/core/auth/role.go (1)
Role(16-16)
ui/src/components/ChangePasswordModal.tsx (5)
ui/src/contexts/ConfigContext.tsx (1)
useConfig(38-40)npm/dagu/index.js (1)
message(27-27)ui/src/components/ui/label.tsx (1)
Label(22-22)ui/src/components/ui/input.tsx (1)
Input(21-21)ui/src/components/ui/button.tsx (1)
Button(59-59)
ui/src/menu.tsx (1)
ui/src/contexts/AuthContext.tsx (1)
useIsAdmin(128-133)
ui/src/components/UserMenu.tsx (4)
ui/src/contexts/AuthContext.tsx (1)
useAuth(119-125)ui/src/contexts/ConfigContext.tsx (1)
useConfig(38-40)ui/src/components/ui/dropdown-menu.tsx (4)
DropdownMenu(182-182)DropdownMenuTrigger(183-183)DropdownMenuContent(184-184)DropdownMenuItem(185-185)ui/src/components/ChangePasswordModal.tsx (1)
ChangePasswordModal(19-177)
internal/service/frontend/api/v2/dagruns.go (1)
api/v2/api.gen.go (2)
Error(462-471)ErrorCodeForbidden(42-42)
ui/src/App.tsx (5)
ui/src/contexts/AuthContext.tsx (1)
AuthProvider(34-117)ui/src/pages/login.tsx (1)
LoginPage(10-98)ui/src/components/ProtectedRoute.tsx (1)
ProtectedRoute(11-37)ui/src/layouts/Layout.tsx (1)
Layout(307-309)ui/src/pages/users/index.tsx (1)
UsersPage(29-263)
ui/src/hooks/api.ts (2)
internal/service/frontend/auth/middleware.go (1)
Middleware(40-89)ui/src/api/v2/schema.ts (1)
paths(6-887)
internal/core/auth/store.go (1)
internal/core/auth/user.go (1)
User(13-27)
internal/service/frontend/api/v2/api.go (4)
internal/service/auth/service.go (4)
Claims(51-56)CreateUserInput(163-167)UpdateUserInput(203-207)Service(59-62)internal/service/frontend/auth/middleware.go (2)
Options(15-27)Middleware(40-89)internal/service/frontend/auth/builtin.go (1)
BuiltinAuthMiddleware(18-50)internal/core/auth/context.go (1)
UserFromContext(23-26)
internal/common/config/config_test.go (2)
internal/common/config/config.go (9)
Config(9-39)Server(83-120)Auth(143-149)AuthModeBuiltin(137-137)AuthBuiltin(152-155)AdminConfig(158-161)TokenConfig(164-167)PathsConfig(190-204)UI(206-212)internal/common/logger/tag/tag.go (2)
Port(185-187)Token(386-388)
internal/core/auth/context.go (1)
internal/core/auth/user.go (1)
User(13-27)
internal/persistence/fileuser/store_test.go (4)
internal/persistence/fileuser/store.go (1)
New(47-73)internal/core/auth/user.go (1)
NewUser(30-40)internal/core/auth/role.go (3)
RoleManager(22-22)Role(16-16)RoleAdmin(20-20)internal/core/auth/store.go (2)
ErrUserNotFound(14-14)ErrUserAlreadyExists(17-17)
ui/src/contexts/AuthContext.tsx (1)
ui/src/contexts/ConfigContext.tsx (1)
useConfig(38-40)
ui/src/pages/users/ResetPasswordModal.tsx (2)
ui/src/api/v2/schema.ts (1)
components(889-1557)ui/src/contexts/ConfigContext.tsx (1)
useConfig(38-40)
internal/common/config/config.go (3)
ui/src/contexts/ConfigContext.tsx (2)
AuthMode(16-16)Config(18-34)internal/service/auth/service.go (1)
Config(41-48)internal/common/config/path.go (1)
Paths(13-30)
internal/service/frontend/api/v2/auth.go (4)
api/v2/api.gen.go (6)
LoginRequestObject(4846-4848)LoginResponseObject(4850-4852)Login200JSONResponse(4854-4854)User(988-1003)GetCurrentUser401JSONResponse(4900-4900)UserRole(1012-1012)internal/service/auth/service.go (3)
ErrInvalidCredentials(22-22)ErrPasswordMismatch(26-26)ErrWeakPassword(27-27)internal/core/auth/context.go (1)
UserFromContext(23-26)internal/core/auth/role.go (1)
Role(16-16)
internal/service/frontend/templates.go (3)
internal/common/config/config.go (1)
AuthMode(131-131)ui/src/contexts/ConfigContext.tsx (1)
AuthMode(16-16)internal/common/config/path.go (1)
Paths(13-30)
internal/core/auth/role_test.go (1)
internal/core/auth/role.go (7)
Role(16-16)RoleAdmin(20-20)RoleManager(22-22)RoleOperator(24-24)RoleViewer(26-26)ParseRole(70-76)AllRoles(33-37)
internal/persistence/fileuser/store.go (2)
internal/core/auth/user.go (2)
User(13-27)UserForStorage(44-51)internal/core/auth/store.go (1)
ErrUserNotFound(14-14)
internal/common/config/loader.go (1)
internal/common/config/config.go (3)
Server(83-120)Auth(143-149)AuthMode(131-131)
internal/common/config/loader_test.go (2)
internal/common/config/config.go (8)
AuthBuiltin(152-155)AdminConfig(158-161)TokenConfig(164-167)AuthModeNone(135-135)Auth(143-149)AuthModeBuiltin(137-137)AuthModeOIDC(139-139)AuthMode(131-131)ui/src/contexts/ConfigContext.tsx (1)
AuthMode(16-16)
internal/service/auth/service.go (3)
internal/core/auth/role.go (2)
Role(16-16)RoleAdmin(20-20)internal/core/auth/store.go (2)
UserStore(26-52)ErrUserNotFound(14-14)internal/core/auth/user.go (2)
User(13-27)NewUser(30-40)
🪛 GitHub Actions: CI
internal/core/auth/role_test.go
[error] 1-1: Go fmt required. Detected formatting changes in internal/core/auth/role_test.go. Please run 'go fmt ./...'.
internal/common/config/loader_test.go
[error] 1-1: Go fmt required. Detected formatting changes in internal/common/config/loader_test.go. Please run 'go fmt ./...'.
🔇 Additional comments (93)
ui/src/features/system-status/components/ResourceChart.tsx (1)
100-102: Visual improvements look good.The gradient definition is correctly structured with standard offsets (0% to 100%), and the
strokeWidth={2}addition makes the chart line more visible. Thefillreference properly uses the dynamicgradientId.Also applies to: 125-126
ui/src/hooks/api.ts (1)
11-24: LGTM! Clean middleware implementation.The auth middleware correctly reads the token from localStorage and attaches it as a Bearer token to all API requests. This approach is standard for JWT-based authentication.
ui/src/contexts/AuthContext.tsx (2)
34-117: Well-structured authentication context.The AuthProvider implementation is clean and handles the authentication lifecycle properly. Token persistence, role-based permissions, and graceful error handling are all well implemented.
Note: Storing JWT tokens in localStorage is vulnerable to XSS attacks. This is acceptable for many use cases, but be aware that if an XSS vulnerability exists elsewhere in the application, the token could be stolen. Consider httpOnly cookies for enhanced security in future iterations.
128-149: Smart permission hook design.The permission hooks elegantly handle both builtin and external auth modes, falling back to config permissions when appropriate. The role hierarchy comparison ensures consistent permission checks.
ui/src/components/ui/dropdown-menu.tsx (1)
1-197: Excellent component implementation.The dropdown menu wrapper follows best practices for Radix UI integration:
- Proper forwardRef usage for all components
- Dark mode support with appropriate Tailwind classes
- Compact, developer-centric styling as per guidelines
- Keyboard navigation and accessibility supported by Radix primitives
- Clean composition with optional
insetprops where neededBased on coding guidelines: The component follows the UI design principles with compact sizing (py-1.5, px-2), dark mode support, and maintains keyboard navigation through Radix UI's built-in accessibility features.
ui/src/layouts/Layout.tsx (1)
14-14: LGTM! Clean integration of UserMenu.The UserMenu is properly imported and rendered in the header's right-side control cluster, following the existing pattern alongside ThemeToggle. The placement is logical and consistent with the layout structure.
Also applies to: 261-261
ui/index.html (1)
50-52: LGTM! Development configuration.The additions of
usersDirandauthModeto the config object are appropriate for development/testing. These values align with the new authentication feature and follow the existing path patterns.Note: This appears to be a development configuration file. Production values would be templated from the backend service configuration.
ui/package.json (1)
76-76: No action needed on @radix-ui/react-dropdown-menu version.Version 2.1.16 is the latest stable release, and no known security vulnerabilities are reported for this package. The caret constraint allows automatic minor and patch updates within the 2.x range.
internal/service/frontend/templates.go (1)
63-63: LGTM!The additions properly expose
AuthModeandUsersDirconfiguration to templates through well-defined template functions.Also applies to: 138-143
internal/service/frontend/templates/base.gohtml (1)
22-22: LGTM!Template changes correctly expose
authModeandusersDirto the frontend configuration object.Also applies to: 38-38
ui/src/menu.tsx (1)
3-3: LGTM!The admin-gated User Management navigation item is properly implemented using the
useIsAdmin()hook, consistent with existing patterns in the codebase.Also applies to: 14-14, 70-70, 222-230
internal/common/config/config_test.go (1)
197-312: LGTM! Comprehensive test coverage for builtin auth validation.The test cases thoroughly cover:
- Missing required fields (usersDir, token secret, admin username)
- Valid builtin auth configuration
- Validation skipping for non-builtin auth modes
All tests properly use
t.Parallel()and appropriate assertion methods.api/v2/api.yaml (4)
63-162: LGTM! Auth endpoints are well-defined.The authentication endpoints follow RESTful conventions:
- Login is properly marked as public with
security: []- Me and change-password endpoints require authentication (use default security)
- Response schemas are complete with appropriate field types
167-436: LGTM! User management endpoints follow best practices.The user management endpoints are well-designed:
- All require authentication and admin role (per descriptions)
- DELETE endpoint includes self-deletion protection
- PATCH for updates (partial modification) is appropriate
- Response codes are comprehensive (401, 403, 404, 409)
- Username conflict handling (409) is properly defined
3269-3426: LGTM! Auth schemas are well-defined with appropriate constraints.The authentication and user management schemas include:
- Sensible password minimum length (8 characters)
- Username length constraints (1-64 characters)
- Clear role hierarchy (admin → manager → operator → viewer)
- Proper timestamp fields for audit trail (createdAt, updatedAt)
3428-3432: Add explicit security requirements to sensitive endpoints that require authentication.The global security setting allows anonymous access via the empty object
{}. While/auth/logincorrectly overrides this withsecurity: [], sensitive endpoints like user deletion and password reset lack explicit security declarations and inherit the permissive global setting. This creates a spec/implementation mismatch: endpoints document "requires admin role" with 401/403 response codes, but the OpenAPI spec itself permits anonymous access.Add
security:declarations to endpoints requiring authentication (POST/PUT/DELETE operations on/users,/dags, etc.) to explicitly enforce authentication:security: - apiToken: [] - basicAuth: []Endpoints intentionally supporting anonymous access (like
/health) should explicitly declaresecurity: [].ui/src/contexts/ConfigContext.tsx (1)
16-16: Empty string in AuthMode is necessary as the default/uninitialized state.The backend Go code defines
AuthModewith only three named constants ("none","builtin","oidc"), but the config loader testTestLoad_AuthMode/AuthModeDefaultEmptyexplicitly confirms that when no auth mode is configured, the backend defaults to an empty string:AuthMode(""). The frontend's TypeScript union includes''to accurately represent this default state that the backend can send. While the UI treats all non-'builtin'values identically (empty string,'none', and'oidc'are functionally equivalent in permission checks), the empty string is semantically distinct as the uninitialized/unconfigured state.internal/core/auth/role_test.go (2)
124-136: LGTM: Defensive mutation test.The test correctly verifies that modifying the returned slice doesn't affect internal state, which is good defensive programming.
1-136: Fix: Rungo fmtto resolve pipeline failure.The CI pipeline is failing because this file needs formatting.
Run the following command to fix:
go fmt ./internal/core/auth/role_test.go⛔ Skipped due to learnings
Learnt from: CR Repo: dagu-org/dagu PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-12-04T10:34:17.051Z Learning: Applies to **/*.go : Keep Go files `gofmt`/`goimports` clean; use tabs, PascalCase for exported symbols (`SchedulerClient`), lowerCamelCase for locals, and `Err...` names for package-level errorsLearnt from: CR Repo: dagu-org/dagu PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-12-04T10:34:17.051Z Learning: Applies to **/*_test.go : Co-locate Go tests as `*_test.go`; favour table-driven cases and cover failure pathsLearnt from: CR Repo: dagu-org/dagu PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-12-04T10:34:17.051Z Learning: Applies to **/*.go : Repository linting relies on `golangci-lint`; prefer idiomatic Go patterns, minimal global state, and structured logging helpers in `internal/common`Learnt from: CR Repo: dagu-org/dagu PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-12-04T10:34:17.051Z Learning: Applies to **/*_test.go : Use `stretchr/testify/require` and shared fixtures from `internal/test` instead of duplicating mocksLearnt from: CR Repo: dagu-org/dagu PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-12-04T10:34:17.051Z Learning: `make test` (or `make test-coverage`) executes the Go suite via `gotestsum`; append `TEST_TARGET=./internal/...` to focus packagesui/src/components/UserMenu.tsx (1)
48-94: LGTM: Clean component implementation.The component correctly guards rendering based on auth mode, handles user interactions, and follows the compact UI design guidelines with proper dark mode support.
ui/src/components/ProtectedRoute.tsx (1)
11-37: LGTM: Solid route protection implementation.The component correctly handles all auth states (loading, unauthenticated, role-based) and follows React Router best practices by preserving the target location for post-login redirect.
ui/src/pages/login.tsx (1)
44-97: LGTM: Clean form implementation.The form handling, validation, error display, and loading states follow best practices and align with the UI guidelines for compact design.
internal/core/auth/user.go (1)
12-75: LGTM: Well-designed user model with proper security.The implementation correctly excludes the password hash from JSON serialization (Line 20), generates UUIDs for user IDs, and provides clean conversion methods for storage. The separation between
UserandUserForStorageis a good design choice.ui/src/pages/users/UserFormModal.tsx (3)
31-36: LGTM: Well-structured role definitions.The role definitions with descriptions provide good UX and align with the backend role system.
48-59: LGTM: Proper form state management.The effect correctly resets form state when the user prop or modal visibility changes, preventing stale data issues.
61-113: LGTM: Robust form submission with validation.The form correctly validates password length, handles both create and update modes, and provides appropriate error feedback. The API error handling extracts messages from the response structure.
internal/core/auth/context.go (2)
8-26: LGTM: Idiomatic Go context utilities.The implementation follows Go best practices with a private context key type to avoid collisions and provides both safe (
UserFromContext) and panic-on-error (MustUserFromContext) accessors.
28-36: Remove or defer this comment—MustUserFromContexthas no callers in the codebase.The function is currently unused. If it's intended as a public API for external use, keep it with the panic behavior and existing documentation. If it's internal, remove it or add usages; the middleware protection requirement cannot be verified without actual call sites.
ui/src/pages/users/ResetPasswordModal.tsx (3)
31-41: LGTM: Clean form lifecycle management.The reset and close handlers properly clear all form state, preventing stale data when reopening the modal.
43-86: LGTM: Comprehensive validation and error handling.The form correctly validates password requirements, handles API errors with proper message extraction, and provides success feedback with auto-close functionality.
88-161: LGTM: Accessible and user-friendly UI.The modal follows the compact design guidelines with proper labeling, placeholder text, autocomplete attributes, and keyboard navigation support.
internal/persistence/fileuser/store_test.go (5)
96-122: LGTM!The duplicate username test correctly verifies the unique constraint enforcement and expected error type.
124-162: LGTM!Comprehensive coverage of not-found scenarios for all relevant store operations.
164-197: LGTM!Solid test for index rebuild on store restart - validates persistence layer correctly reloads state from disk.
199-204: LGTM!Validates the empty baseDir error handling.
206-237: LGTM!Good verification of filesystem side effects - confirms files are properly created and cleaned up.
ui/src/App.tsx (3)
9-10: LGTM!Correct imports for authentication scaffolding with
AuthProviderandProtectedRoute.
129-137: NestedProtectedRoutefor admin role is correct but slightly redundant.The outer
ProtectedRoutealready ensures authentication. The inner one re-checks auth before verifying the admin role. This works correctly but adds minor overhead. Consider whether theProtectedRoutecomponent could acceptrequiredRoleat the outer level to simplify, though the current approach is explicit and clear.
97-147: LGTM!Provider nesting order is correct -
AuthProvideris placed insideConfigContext.Providersince it depends onuseConfig()for API URL.ui/src/pages/users/index.tsx (2)
135-218: LGTM!Table implementation follows coding guidelines with compact row heights, proper dark mode support, and appropriate loading/empty states.
220-257: LGTM!Modal integrations are correctly wired with proper state management and list refresh on success.
internal/service/auth/service_test.go (8)
16-42: LGTM!Well-structured test helper with proper cleanup. Using
BcryptCost: 4for faster test execution is a good practice.
44-87: LGTM!Good coverage of user creation including password hashing verification and weak password rejection.
89-125: LGTM!Good security practice - returning the same
ErrInvalidCredentialsfor both wrong password and non-existent user prevents user enumeration attacks.
127-209: LGTM!Comprehensive token lifecycle testing including generation, validation, and claim extraction.
211-267: LGTM!Good password change coverage including old password verification and post-change authentication tests.
269-329: LGTM!Good coverage of admin provisioning including idempotency and password generation.
360-381: LGTM!Critical self-deletion protection is properly tested, preventing users from deleting their own accounts.
452-508: LGTM!Good distinction between admin
ResetPassword(no old password) and userChangePassword(requires old password) with proper validation.internal/core/auth/store.go (2)
24-52: LGTM!Clean repository interface with proper context support, documented error contracts, and concurrent-safety requirement.
11-22: LGTM!Well-defined error sentinel values with clear documentation. These enable consistent error handling across the codebase. Both
ErrInvalidUsernameandErrInvalidUserIDare actively used in the fileuser implementation, confirming their necessity.internal/service/frontend/auth/builtin.go (4)
18-50: LGTM! Clean middleware implementation.The authentication middleware correctly validates JWT tokens, handles public paths efficiently with O(1) lookup, and properly injects the user into the request context. Error handling with structured JSON responses is consistent.
65-104: LGTM! Role-based authorization is well-structured.The middleware correctly distinguishes between authentication (401) and authorization (403) failures. The convenience wrappers align with the role hierarchy defined in
role.go.
106-119: LGTM!Bearer token extraction follows RFC 6750 conventions correctly.
132-142: LGTM!The error response structure is clean and consistent. Ignoring the encode error is acceptable here since we're already in an error path.
internal/service/frontend/api/v2/auth.go (3)
17-58: LGTM! Login endpoint is well-implemented.The endpoint properly handles disabled auth, validates request body, correctly maps authentication errors to 401, and returns appropriate token metadata. The immediate token validation after generation ensures the token is valid before returning it to the client.
60-73: LGTM!Clean implementation that correctly retrieves the user from context.
132-144:toAPIUsersis actually used; onlyformatTimemay be unused.
toAPIUsersis called ininternal/service/frontend/api/v2/users.go:37, so it is not unused. However,formatTimeinauth.godoes not appear to be called within that file or elsewhere in the API layer. A separateformatTimefunction exists ininternal/cmd/migrator.gowith its own usage. If the one inauth.gois not needed, consider removing it.internal/service/frontend/server.go (3)
55-72: LGTM! Clean API options pattern.The conditional auth service initialization and option passing pattern is well-structured. Graceful degradation (logging error but continuing) allows the server to start even if auth initialization fails.
118-129: Security consideration: auto-generated password logged in plaintext.Logging the auto-generated admin password could expose it if logs are stored, shipped to a log aggregation service, or accessible to unauthorized users. Consider alternatives:
- Write to a separate bootstrap file with restricted permissions
- Output only to stderr (not structured logs) on first run
- Require password to be set via environment variable
The current approach is acceptable for initial setup convenience, but ensure operators understand the security implications.
300-303: Document v1 API deprecation when auth is enabled.When any auth mode is enabled, the v1 API becomes unavailable. This is a breaking change for users migrating to builtin auth. Ensure this is documented in release notes or migration guides.
internal/core/auth/role.go (4)
8-27: LGTM! Well-documented role hierarchy.The role definitions are clear with good documentation explaining the permission hierarchy. Using
constwith typed string ensures type safety.
29-37: LGTM!Returning a copy prevents external mutation of the internal roles slice.
39-66: LGTM!Role capability methods correctly implement the documented hierarchy. The explicit switch/comparison approach is clear and maintainable.
68-76: LGTM!The error message helpfully lists valid role options, improving developer experience.
internal/common/config/loader.go (3)
208-241: LGTM! Auth configuration loading is well-structured.The configuration properly handles nested structs with nil checks, parses TTL duration with error handling, and records warnings for invalid values.
244-251: LGTM!Sensible defaults for token TTL (24h) and admin username ("admin") are applied when not explicitly configured.
432-434: LGTM!UsersDir default follows the established pattern for other data directories.
internal/common/config/definition.go (2)
160-184: LGTM! Clean struct definitions for builtin authentication.The new authentication configuration types are well-structured with appropriate mapstructure tags. The hierarchical organization (AuthDef → AuthBuiltinDef → AdminConfigDef/TokenConfigDef) provides clear separation of concerns.
219-219: UsersDir addition aligns with the builtin auth feature.The new
UsersDirfield properly extends PathsDef to support file-based user persistence.internal/common/config/loader_test.go (1)
831-956: Comprehensive test coverage for new auth modes.The tests appropriately cover:
- All three auth modes (none, builtin, OIDC)
- Environment variable loading
- Default TTL behavior (24h)
- Empty password handling
- YAML and environment-based configuration
internal/service/frontend/api/v2/api.go (1)
53-67: Well-designed AuthService interface for dependency injection.The interface provides a clean abstraction for authentication operations, enabling testability and separation of concerns.
internal/common/config/config.go (3)
130-140: Clean AuthMode type definition with well-documented constants.The enum-like pattern with typed constants is idiomatic Go.
151-167: AuthBuiltin and related config types are well-structured.The separation of
AdminConfig(initial admin credentials) andTokenConfig(JWT settings) is a clean design.
327-353: Thorough validation for builtin authentication prerequisites.The validation correctly ensures:
- UsersDir is set when builtin auth is enabled
- Admin username is configured
- Token secret is provided
- Token TTL is positive
The error messages are clear and actionable.
internal/service/frontend/api/v2/users.go (4)
16-39: ListUsers handler follows consistent patterns.Proper checks for auth service availability and admin role, with appropriate error responses.
42-96: CreateUser handler with comprehensive error handling.Good coverage of edge cases: nil body, invalid role, existing username, and weak password.
191-233: DeleteUser correctly prevents self-deletion.The handler properly extracts the current user from context and passes their ID to the service layer for the self-deletion check.
236-278: ResetUserPassword handler is well-implemented.Proper validation and error handling for user not found and weak password scenarios.
internal/persistence/fileuser/store.go (6)
1-26: LGTM! Well-structured constants and package setup.The permission constants (0750 for directory, 0600 for files) follow security best practices for sensitive user data.
31-73: LGTM! Clean initialization with proper validation.The constructor properly validates input, creates the directory with appropriate permissions, and builds the initial index.
129-165: LGTM! Create operation handles conflicts correctly.The implementation properly checks for both username and ID conflicts before persisting, and the write-then-index order ensures crash safety (rebuildIndex will recover from files).
167-187: Good use of atomic file writes.The temp-file + rename pattern ensures crash safety and prevents partial writes from corrupting user data.
232-255: LGTM! List operation handles concurrent modifications gracefully.The implementation correctly releases the lock between collecting IDs and loading users, and gracefully skips users that may have been deleted concurrently.
257-300: LGTM! Update operation with proper rollback on failure.The implementation correctly handles username changes with conflict detection and rolls back the index if the file write fails.
ui/src/api/v2/schema.ts (2)
1-4: Auto-generated file - minimal review required.This file is auto-generated from OpenAPI specifications. The authentication and user management schemas look correct and comprehensive with proper error handling (401, 403, 404, 409) for different scenarios.
4062-4067: UserRole enum values are consistent with backend.The role hierarchy (admin, manager, operator, viewer) aligns with the backend RBAC implementation.
internal/service/auth/service.go (6)
20-38: LGTM! Appropriate security constants and error definitions.
- BCrypt cost of 12 is appropriate for production use
- Minimum password length of 8 is reasonable
- 24-hour token TTL is a sensible default
97-117: LGTM! JWT token generation is properly implemented.The implementation correctly sets standard claims (Subject, IssuedAt, ExpiresAt) and validates the secret before signing.
119-145: LGTM! Token validation includes algorithm confusion protection.The signing method verification at line 126 prevents algorithm confusion attacks where an attacker might try to use a different signing algorithm.
309-340: LGTM! Safe admin bootstrap with secure password generation.The implementation correctly creates an admin user only when no users exist, and uses cryptographic randomness for password generation when no password is provided.
350-357: LGTM! Secure password generation using crypto/rand.The implementation correctly uses the cryptographically secure random source and base64 URL encoding for safe characters.
209-245: LGTM! UpdateUser properly validates optional fields.The implementation correctly handles partial updates, validating role and password only when provided.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
internal/persistence/fileuser/store_test.go (1)
81-81: Handle the error fromGetByIDafter update.Line 81 discards the error with
_, which could mask failures during the Update verification.Apply this diff to check the error:
- retrieved, _ = store.GetByID(ctx, user.ID) + retrieved, err = store.GetByID(ctx, user.ID) + if err != nil { + t.Fatalf("GetByID() after update error = %v", err) + }internal/persistence/fileuser/store.go (2)
203-210: Bug:os.IsNotExistcheck won't work due to error wrapping.
loadUserFromFilewraps errors withfmt.Errorf(..., %w)at line 113, soos.IsNotExist(err)at line 206 will always returnfalse. Useerrors.Isinstead.Apply this fix:
user, err := s.loadUserFromFile(filePath) if err != nil { // File might have been deleted externally - if os.IsNotExist(err) { + if errors.Is(err, os.ErrNotExist) { return nil, auth.ErrUserNotFound } return nil, fmt.Errorf("fileuser: failed to load user %s: %w", id, err) }
316-320: Bug: Sameos.IsNotExistissue as in GetByID.The
os.IsNotExist(err)check won't work becauseloadUserFromFilewraps the error. Useerrors.Isinstead.Apply this fix:
// Load user to get username for index cleanup user, err := s.loadUserFromFile(filePath) - if err != nil && !os.IsNotExist(err) { + if err != nil && !errors.Is(err, os.ErrNotExist) { return fmt.Errorf("fileuser: failed to load user for deletion: %w", err) }
🧹 Nitpick comments (4)
internal/service/frontend/api/v2/auth.go (1)
47-50: Consider removing redundant token validation.The token is generated on line 42 and immediately validated on lines 47-50. Since the token was just created by the same service, this validation appears redundant and adds unnecessary overhead.
Consider simplifying to:
token, err := a.authService.GenerateToken(user) if err != nil { return nil, err } - claims, err := a.authService.ValidateToken(token) - if err != nil { - return nil, err - } - return api.Login200JSONResponse{ Token: token, - ExpiresAt: claims.ExpiresAt.Time, + ExpiresAt: time.Now().Add(a.authService.TokenTTL()), // or retrieve from token config User: toAPIUser(user), }, nilIf the
ExpiresAtfield must be extracted from the token claims, document why validation is necessary here.internal/service/auth/service_test.go (1)
431-439: Simplify username generation for clarity.The expression
"user" + string(rune('0'+i))works but is non-idiomatic. Consider usingfmt.Sprintffor better readability.Apply this diff:
+ "fmt" // Create multiple users for i := 0; i < 3; i++ { _, err := svc.CreateUser(ctx, CreateUserInput{ - Username: "user" + string(rune('0'+i)), + Username: fmt.Sprintf("user%d", i), Password: "password123", Role: auth.RoleViewer, })internal/common/config/loader_test.go (2)
831-891: Auth mode tests are comprehensive; consider asserting env bindings too.The new
TestLoad_AuthModesubtests covernone,builtin,oidc, env-driven mode, and the default empty case cleanly and in a focused way. As a small enhancement, you could strengthen the"AuthModeFromEnv"subtest by also asserting that the env-provided token secret and users dir are bound correctly, e.g.:- t.Run("AuthModeFromEnv", func(t *testing.T) { - cfg := loadWithEnv(t, "# empty", map[string]string{ - "DAGU_AUTH_MODE": "builtin", - "DAGU_AUTH_TOKEN_SECRET": "test-secret", - "DAGU_PATHS_USERS_DIR": t.TempDir(), - }) - assert.Equal(t, AuthModeBuiltin, cfg.Server.Auth.Mode) - }) + t.Run("AuthModeFromEnv", func(t *testing.T) { + usersDir := t.TempDir() + cfg := loadWithEnv(t, "# empty", map[string]string{ + "DAGU_AUTH_MODE": "builtin", + "DAGU_AUTH_TOKEN_SECRET": "test-secret", + "DAGU_PATHS_USERS_DIR": usersDir, + }) + assert.Equal(t, AuthModeBuiltin, cfg.Server.Auth.Mode) + assert.Equal(t, "test-secret", cfg.Server.Auth.Builtin.Token.Secret) + assert.Equal(t, usersDir, cfg.Paths.UsersDir) + })That would catch regressions in env binding for builtin auth without changing behavior.
893-957: Builtin auth tests exercise key paths; optionally add validation/error cases.
TestLoad_AuthBuiltindoes a good job covering YAML vs env loading, allowing empty admin passwords, and the default 24h TTL whenttlis omitted. If you don’t already have them elsewhere, you might add one or two focused tests for invalid builtin auth configuration (e.g., non-positive TTL or missing token secret whenmode: "builtin"), to document and lock in the expected validation behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
internal/common/config/loader_test.go(6 hunks)internal/core/auth/role_test.go(1 hunks)internal/persistence/fileuser/store.go(1 hunks)internal/persistence/fileuser/store_test.go(1 hunks)internal/service/auth/service_test.go(1 hunks)internal/service/frontend/api/v2/auth.go(1 hunks)ui/src/layouts/Layout.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/core/auth/role_test.go
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/persistence/fileuser/store_test.gointernal/service/auth/service_test.gointernal/common/config/loader_test.gointernal/persistence/fileuser/store.gointernal/service/frontend/api/v2/auth.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/persistence/fileuser/store_test.gointernal/service/auth/service_test.gointernal/common/config/loader_test.go
ui/**/*.{ts,tsx,jsx,js}
📄 CodeRabbit inference engine (ui/CLAUDE.md)
ui/**/*.{ts,tsx,jsx,js}: Use developer-centric UI design with high information density, minimal whitespace, compact components, and no unnecessary decorations
Support both light and dark modes for all UI components using Tailwind CSS class pairs likedark:bg-slate-700
NEVER use full-page loading overlays or LoadingIndicator components that hide content - show stale data while fetching updates instead
Use compact modal design with small headers, minimal padding (p-2 or p-3), tight spacing, and support keyboard navigation (arrows, enter, escape)
Use small heights for form elements: select boxes h-7 or smaller, buttons h-7 or h-8, inputs with compact padding (py-0.5 or py-1)
Minimize row heights in tables and lists while maintaining readability, merge related columns, and always handle long text withwhitespace-normal break-words
Use consistent metadata styling withbg-slate-200 dark:bg-slate-700backgrounds and maintain text hierarchy with primary/secondary/muted text colors
Use flexbox-first layouts withmin-h-0andoverflow-hiddento prevent layout breaks, account for fixed elements when setting heights
Maintain keyboard navigation support in all interactive components with appropriate focus indicators and ARIA labels
Files:
ui/src/layouts/Layout.tsx
ui/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
ui/**/*.{ts,tsx}: The React + TypeScript frontend resides inui/, with production bundles copied tointernal/service/frontend/assetsbymake ui
UI code follows ESLint + Prettier (2-space indent) and Tailwind utilities; name React components in PascalCase (JobList.tsx) and hooks withuse*(useJobs.ts)
Files:
ui/src/layouts/Layout.tsx
🧠 Learnings (4)
📚 Learning: 2025-12-04T10:34:17.051Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.051Z
Learning: Applies to **/*_test.go : Co-locate Go tests as `*_test.go`; favour table-driven cases and cover failure paths
Applied to files:
internal/persistence/fileuser/store_test.gointernal/service/auth/service_test.go
📚 Learning: 2025-12-04T10:34:17.051Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.051Z
Learning: Applies to **/*_test.go : Use `stretchr/testify/require` and shared fixtures from `internal/test` instead of duplicating mocks
Applied to files:
internal/persistence/fileuser/store_test.gointernal/service/auth/service_test.go
📚 Learning: 2025-12-04T10:34:17.051Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.051Z
Learning: `make test` (or `make test-coverage`) executes the Go suite via `gotestsum`; append `TEST_TARGET=./internal/...` to focus packages
Applied to files:
internal/service/auth/service_test.go
📚 Learning: 2025-12-04T10:34:17.051Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.051Z
Learning: Applies to **/*.go : Keep Go files `gofmt`/`goimports` clean; use tabs, PascalCase for exported symbols (`SchedulerClient`), lowerCamelCase for locals, and `Err...` names for package-level errors
Applied to files:
internal/common/config/loader_test.go
🧬 Code graph analysis (5)
internal/persistence/fileuser/store_test.go (4)
internal/persistence/fileuser/store.go (1)
New(47-73)internal/core/auth/user.go (1)
NewUser(30-40)internal/core/auth/role.go (4)
RoleManager(22-22)Role(16-16)RoleAdmin(20-20)RoleViewer(26-26)internal/core/auth/store.go (2)
ErrUserNotFound(14-14)ErrUserAlreadyExists(17-17)
internal/common/config/loader_test.go (1)
internal/common/config/config.go (9)
AuthBuiltin(152-155)AdminConfig(158-161)TokenConfig(164-167)AuthModeNone(135-135)Server(83-120)Auth(143-149)AuthModeBuiltin(137-137)AuthModeOIDC(139-139)AuthMode(131-131)
ui/src/layouts/Layout.tsx (2)
ui/src/contexts/ConfigContext.tsx (1)
useConfig(38-40)ui/src/components/UserMenu.tsx (1)
UserMenu(17-94)
internal/persistence/fileuser/store.go (2)
internal/core/auth/user.go (2)
User(13-27)UserForStorage(44-51)internal/core/auth/store.go (4)
ErrInvalidUserID(21-21)ErrInvalidUsername(19-19)ErrUserAlreadyExists(17-17)ErrUserNotFound(14-14)
internal/service/frontend/api/v2/auth.go (5)
internal/service/frontend/api/v2/api.go (1)
API(35-51)api/v2/api.gen.go (13)
LoginRequestObject(4846-4848)LoginResponseObject(4850-4852)Login200JSONResponse(4854-4854)User(988-1003)GetCurrentUserRequestObject(4884-4885)GetCurrentUserResponseObject(4887-4889)GetCurrentUser401JSONResponse(4900-4900)GetCurrentUser200JSONResponse(4891-4891)ChangePasswordRequestObject(4799-4801)ChangePassword401JSONResponse(4825-4825)ChangePassword400JSONResponse(4816-4816)ChangePassword200JSONResponse(4807-4807)UserRole(1012-1012)internal/service/auth/service.go (3)
ErrInvalidCredentials(22-22)ErrPasswordMismatch(26-26)ErrWeakPassword(27-27)internal/core/auth/context.go (1)
UserFromContext(23-26)internal/core/auth/role.go (1)
Role(16-16)
⏰ 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 (31)
ui/src/layouts/Layout.tsx (4)
12-12: LGTM! Clean integration of authentication context and UI.The new imports integrate cleanly with the existing layout structure, adding authentication-aware behavior and user management UI.
Also applies to: 15-15
233-240: LGTM! Conditional logic correctly handles builtin auth and remote nodes.The OR condition appropriately hides the remote node selector when:
- Builtin auth is enabled (local-only authentication), OR
- No remote nodes are configured
This ensures the selector only appears when remote nodes are both available and supported by the authentication mode.
265-265: LGTM! UserMenu integrated cleanly into the header.The UserMenu component is positioned logically alongside the ThemeToggle and handles its own visibility logic internally, keeping the layout code clean.
82-82: No issues found. The ConfigContext is properly initialized with a Config value passed as a prop to the App component (line 29:function App({ config }: Props)) and provided to all descendants via ConfigContext.Provider (line 95). The config parameter is a typed prop (not optional), ensuringconfig.authModeis safely accessible in Layout.tsx without null/undefined checks.internal/persistence/fileuser/store_test.go (5)
96-122: LGTM!The duplicate username test correctly validates the unique constraint enforcement.
124-162: LGTM!The not-found test cases provide thorough coverage of error handling paths.
164-197: LGTM!The index rebuild test correctly simulates restart behavior and validates persistence.
199-204: LGTM!The empty baseDir test correctly validates input validation.
206-237: LGTM!The file existence test correctly validates filesystem side effects of Create and Delete operations.
internal/service/frontend/api/v2/auth.go (4)
59-72: LGTM!The GetCurrentUser implementation correctly retrieves the authenticated user from context with proper error handling.
74-118: LGTM!The ChangePassword implementation provides comprehensive error handling for authentication, password mismatch, and weak password scenarios.
120-129: LGTM!The toAPIUser conversion correctly maps all required fields from the core model to the API representation.
131-138: LGTM!The toAPIUsers helper correctly converts a slice with proper preallocation for efficiency.
internal/service/auth/service_test.go (2)
16-42: LGTM!The test setup helper follows best practices with proper cleanup, test-specific configuration (low bcrypt cost for speed), and t.Helper() marking.
44-508: LGTM!The test suite provides comprehensive coverage of the authentication service, including positive cases, error handling, password validation, admin user management, and CRUD operations. The tests follow best practices with proper error checking and verification.
internal/persistence/fileuser/store.go (11)
19-40: LGTM!The Store structure and constants follow security best practices with restrictive file permissions (0600 for files, 0750 for directories) and proper thread-safety design using RWMutex.
45-73: LGTM!The constructor properly validates inputs, initializes the store, creates the directory if needed, and builds the initial index with appropriate error handling.
75-107: LGTM!The index rebuild implementation is resilient to corrupted files and properly handles locking. The decision to continue on bad files (line 99) is reasonable for recovery scenarios.
109-122: LGTM!The file loading helper properly uses error wrapping with
%w, which is essential for the error checking in GetByID and Delete.
124-127: LGTM!Simple helper with correct path construction.
129-165: LGTM!The Create method provides thorough validation and properly enforces uniqueness constraints before updating indexes.
167-187: LGTM!The atomic write pattern using temp file + rename is correct and provides crash-safety. Cleanup on failure is properly handled.
215-230: LGTM!GetByUsername correctly delegates to GetByID after index lookup with proper validation.
232-255: LGTM!The List implementation correctly releases the lock before loading files and gracefully handles externally deleted files.
257-300: LGTM!The Update method handles username changes correctly with conflict detection and provides rollback on write failure for consistency.
336-341: LGTM!The Count method correctly returns the index size under read lock.
internal/common/config/loader_test.go (5)
162-165: Builtin auth defaults in env-based config look correct.Expecting
Builtin: AuthBuiltin{Admin: AdminConfig{Username: "admin"}, Token: TokenConfig{TTL: 24 * time.Hour}}here nicely encodes the default admin username and token TTL while leaving secret/password at zero values, which matches the intended builtin defaults and keeps the struct shape explicit in the assertion.
187-187: UsersDir derived from DataDir (env) is a good invariant to lock in.Asserting
UsersDir: filepath.Join(testPaths, "data", "users")here confirms the new UsersDir derivation fromDataDirunder env-driven config, aligning it with the other derived path conventions.
374-377: Builtin auth defaults populated even when YAML omits builtin block.Including the
Builtinblock with default admin username and 24h token TTL in the YAML-based expected config ensures the loader always produces a stable builtin auth shape, even when the YAML doesn’t specify it, which is helpful for future mode switches.
417-417: YAML-based UsersDir derivation matches DataDir semantics.
UsersDir: "/var/dagu/data/users"is consistent with the new rule that UsersDir is derived fromDataDir, and this expectation keeps the path behavior aligned with the other derived directories.
512-512: Edge-case coverage for derived UsersDir is solid.Extending the derived-paths edge-case test to assert
"/custom/data/users"forcfg.Paths.UsersDirnicely guards the new UsersDir derivation when onlydataDiris configured.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
ui/src/contexts/AuthContext.tsx (1)
4-10: ExportUserRoleandUsertypes for consumer type safety.The exported
hasRolefunction requiresUserRoleparameters, and components consuming user data will need theUsertype. Without exports, consumers lose type safety and IDE support.-type UserRole = 'admin' | 'manager' | 'operator' | 'viewer'; +export type UserRole = 'admin' | 'manager' | 'operator' | 'viewer'; -type User = { +export type User = { id: string; username: string; role: UserRole; };internal/service/frontend/auth/builtin.go (2)
52-71: Simplify the public path matching logic.The trailing slash normalization logic works correctly but can be simplified. Lines 65-69 check for adding a trailing slash, but line 60 already handles this case by removing the trailing slash from the input path.
Apply this diff to simplify:
func isPublicPath(path string, publicSet map[string]struct{}) bool { // Exact match if _, ok := publicSet[path]; ok { return true } - // Try with trailing slash removed - withoutSlash := strings.TrimSuffix(path, "/") - if _, ok := publicSet[withoutSlash]; ok { - return true - } - // Try with trailing slash added - if path != "" && !strings.HasSuffix(path, "/") { - if _, ok := publicSet[path+"/"]; ok { - return true - } - } - return false + // Normalize trailing slash and try again + normalized := strings.TrimSuffix(path, "/") + if normalized != path { + // path had trailing slash, check without it + _, ok := publicSet[normalized] + return ok + } + // path had no trailing slash, check with it + _, ok := publicSet[path+"/"] + return ok }This achieves the same result with clearer intent: normalize once and check the opposite form.
129-150: JSON encoding error is silently ignored.Line 144 ignores the error from
json.NewEncoder(w).Encode(). While this is unlikely to fail for simple error structures, it could mask issues if the response cannot be written.Consider one of the following:
- Log the error (recommended for debugging):
func writeAuthError(w http.ResponseWriter, status int, code, message string) { w.Header().Set("Content-Type", "application/json") w.WriteHeader(status) - _ = json.NewEncoder(w).Encode(ErrorResponse{ + if err := json.NewEncoder(w).Encode(ErrorResponse{ Error: ErrorDetail{ Code: code, Message: message, }, - }) + }); err != nil { + // Log encoding failure, but can't change response status at this point + // Could use a logger here if available in context + } }
- Pre-marshal the error to detect encoding issues before writing status:
func writeAuthError(w http.ResponseWriter, status int, code, message string) { + response := ErrorResponse{ + Error: ErrorDetail{ + Code: code, + Message: message, + }, + } + data, err := json.Marshal(response) + if err != nil { + // Fallback to plain text if JSON encoding fails + w.Header().Set("Content-Type", "text/plain") + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte("Internal server error")) + return + } w.Header().Set("Content-Type", "application/json") w.WriteHeader(status) - _ = json.NewEncoder(w).Encode(ErrorResponse{ - Error: ErrorDetail{ - Code: code, - Message: message, - }, - }) + _, _ = w.Write(data) }Based on coding guidelines, structured logging from
internal/common/loggercould be used if context is available.internal/common/config/loader.go (2)
206-251: Builtin auth loading, TTL defaults, and env bindingsWiring for
auth.mode, builtin admin/token, TTL parsing, and the new env bindings (DAGU_AUTH_MODE,DAGU_AUTH_ADMIN_USERNAME,DAGU_AUTH_ADMIN_PASSWORD,DAGU_AUTH_TOKEN_SECRET,DAGU_AUTH_TOKEN_TTL) looks consistent with the existing patterns (e.g. scheduler/monitoring durations and legacy auth).A couple of behavioral points worth double‑checking:
- TTL semantics: invalid or empty
auth.builtin.token.ttl(from config or env) leavesBuiltin.Token.TTLat 0, which is then forced to24 * time.Hour. That also means a configured0sor negative duration will ultimately be coerced to 24h. If you ever want to support “no expiry” or treat bad values as hard errors instead of “warning + default”, this will need explicit validation (likely inConfig.Validate).- Unconditional defaults: the default TTL and
Builtin.Admin.Username = "admin"are applied regardless ofauth.mode. That’s functionally harmless (consumers should branch onauth.mode) but it does mean you can’t distinguish “unset” vs “defaulted” just from the struct’s zero values.It would be good to confirm that
Config.Validate(or equivalent) enforces the invariants for builtin mode, e.g.:
- When
auth.mode == "builtin",Builtin.Admin.PasswordandBuiltin.Token.Secretare non‑empty.Builtin.Token.TTLis strictly positive (after parsing/fallback).Otherwise it’s easy to end up with a misconfigured or weak builtin auth setup that still loads.
Also applies to: 686-692
269-269: UsersDir path wiring and security considerationsThe UsersDir wiring looks coherent: you resolve
def.Paths.UsersDirviaResolvePathOrBlank, fall back tofilepath.Join(cfg.Paths.DataDir, "users")when unset, and expose an override viaDAGU_USERS_DIR(paths.usersDirbinding withasPath()), consistent with the rest of the path settings.Given this directory will hold builtin auth user data, it’s important that the code which actually creates it (likely in
ResolvePaths, config validation, or the auth/user store) does so with restrictive permissions (e.g.0700) and avoids placing it under world‑readable/shared locations.Please double‑check the creation/permission logic in that layer to ensure we’re not accidentally exposing credential material on disk.
Also applies to: 432-434, 710-710
internal/persistence/fileuser/store.go (3)
46-74: GuardbaseDirafter applying options and usestore.baseDirforMkdirAllIf any
Optionmutatesstore.baseDir(for example, a futureWithBaseDir), the current constructor still validates and creates the directory based on the originalbaseDirargument. This can lead to a mismatch betweenstore.baseDirand the directory actually created/read.Consider validating and using
store.baseDirafter applying options:func New(baseDir string, opts ...Option) (*Store, error) { - if baseDir == "" { - return nil, errors.New("fileuser: baseDir cannot be empty") - } - store := &Store{ baseDir: baseDir, byID: make(map[string]string), byUsername: make(map[string]string), } for _, opt := range opts { opt(store) } + if store.baseDir == "" { + return nil, errors.New("fileuser: baseDir cannot be empty") + } + // Create base directory if it doesn't exist - if err := os.MkdirAll(baseDir, userDirPermissions); err != nil { - return nil, fmt.Errorf("fileuser: failed to create directory %s: %w", baseDir, err) + if err := os.MkdirAll(store.baseDir, userDirPermissions); err != nil { + return nil, fmt.Errorf("fileuser: failed to create directory %s: %w", store.baseDir, err) }This keeps the constructor robust even as more options are added.
128-131: Consider validating user IDs against path traversal before using them in filenames
userFilePathjoinss.baseDirwithuserID+userFileExtension, andCreateaccepts any non-emptyuser.ID. Ifuser.IDever becomes influenced by untrusted input (now or in the future), IDs containing..or path separators could escapebaseDir.Since you already depend on
filepath, a low-cost hardening is to enforce that IDs are simple basenames (no separators) when creating/updating users:func (s *Store) Create(_ context.Context, user *auth.User) error { if user == nil { return errors.New("fileuser: user cannot be nil") } if user.ID == "" { return auth.ErrInvalidUserID } if user.Username == "" { return auth.ErrInvalidUsername } + // Guard against path traversal in IDs if they are ever user-controlled. + if filepath.Base(user.ID) != user.ID { + return auth.ErrInvalidUserID + }You could apply the same check in
Updateto keep invariants consistent there as well.Also applies to: 133-169
193-217: Optional: clean stale index entries when the backing file disappears
GetByIDcorrectly returnsauth.ErrUserNotFoundwhenloadUserFromFileultimately hitsos.ErrNotExist, which covers the “file deleted externally” case. However, thebyID/byUsernameentries for that ID remain in-memory, so:
Countwill still include this user.Listwill keep trying (and skipping) this ID.- A future
Createfor a user with the same username could be blocked by a stalebyUsernameentry.If you expect external tampering with the directory, you might want to also drop stale index entries in this branch, e.g., under a brief
mu.Lockthat deletes the ID and any matching username. If such tampering is not expected, the current behavior is acceptable and this can stay as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
internal/common/config/loader.go(6 hunks)internal/core/auth/context.go(1 hunks)internal/persistence/fileuser/store.go(1 hunks)internal/persistence/fileuser/store_test.go(1 hunks)internal/service/frontend/auth/builtin.go(1 hunks)internal/service/frontend/server.go(4 hunks)ui/src/contexts/AuthContext.tsx(1 hunks)ui/src/pages/users/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/persistence/fileuser/store_test.go
- ui/src/pages/users/index.tsx
- internal/core/auth/context.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/auth/builtin.gointernal/common/config/loader.gointernal/persistence/fileuser/store.gointernal/service/frontend/server.go
ui/**/*.{ts,tsx,jsx,js}
📄 CodeRabbit inference engine (ui/CLAUDE.md)
ui/**/*.{ts,tsx,jsx,js}: Use developer-centric UI design with high information density, minimal whitespace, compact components, and no unnecessary decorations
Support both light and dark modes for all UI components using Tailwind CSS class pairs likedark:bg-slate-700
NEVER use full-page loading overlays or LoadingIndicator components that hide content - show stale data while fetching updates instead
Use compact modal design with small headers, minimal padding (p-2 or p-3), tight spacing, and support keyboard navigation (arrows, enter, escape)
Use small heights for form elements: select boxes h-7 or smaller, buttons h-7 or h-8, inputs with compact padding (py-0.5 or py-1)
Minimize row heights in tables and lists while maintaining readability, merge related columns, and always handle long text withwhitespace-normal break-words
Use consistent metadata styling withbg-slate-200 dark:bg-slate-700backgrounds and maintain text hierarchy with primary/secondary/muted text colors
Use flexbox-first layouts withmin-h-0andoverflow-hiddento prevent layout breaks, account for fixed elements when setting heights
Maintain keyboard navigation support in all interactive components with appropriate focus indicators and ARIA labels
Files:
ui/src/contexts/AuthContext.tsx
ui/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
ui/**/*.{ts,tsx}: The React + TypeScript frontend resides inui/, with production bundles copied tointernal/service/frontend/assetsbymake ui
UI code follows ESLint + Prettier (2-space indent) and Tailwind utilities; name React components in PascalCase (JobList.tsx) and hooks withuse*(useJobs.ts)
Files:
ui/src/contexts/AuthContext.tsx
🧠 Learnings (1)
📚 Learning: 2025-12-04T10:34:17.051Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.051Z
Learning: Applies to **/*.go : Backend entrypoint in `cmd/` orchestrates the scheduler and CLI; runtime, persistence, and service layers sit under `internal/*` (for example `internal/runtime`, `internal/persistence`)
Applied to files:
internal/service/frontend/server.go
🧬 Code graph analysis (5)
internal/service/frontend/auth/builtin.go (3)
internal/service/auth/service.go (1)
Service(59-62)internal/core/auth/context.go (2)
WithUser(17-19)UserFromContext(23-26)internal/core/auth/role.go (4)
Role(16-16)RoleAdmin(20-20)RoleManager(22-22)RoleOperator(24-24)
ui/src/contexts/AuthContext.tsx (1)
ui/src/contexts/ConfigContext.tsx (1)
useConfig(38-40)
internal/common/config/loader.go (3)
internal/common/config/config.go (2)
Auth(143-149)AuthMode(131-131)ui/src/contexts/ConfigContext.tsx (1)
AuthMode(16-16)internal/common/fileutil/fileutil.go (1)
ResolvePathOrBlank(187-194)
internal/persistence/fileuser/store.go (4)
internal/common/logger/context.go (2)
Errorf(75-77)Warn(45-47)internal/common/fileutil/fileutil.go (1)
IsDir(34-40)internal/core/auth/user.go (2)
User(13-27)UserForStorage(44-51)internal/core/auth/store.go (4)
ErrInvalidUserID(21-21)ErrInvalidUsername(19-19)ErrUserAlreadyExists(17-17)ErrUserNotFound(14-14)
internal/service/frontend/server.go (4)
internal/service/frontend/api/v2/api.go (3)
APIOption(70-70)WithAuthService(73-77)New(83-123)internal/common/config/config.go (5)
Auth(143-149)AuthModeBuiltin(137-137)AuthMode(131-131)Config(9-39)AuthModeNone(135-135)internal/persistence/fileuser/store.go (1)
New(48-74)internal/service/auth/service.go (3)
New(65-76)Config(41-48)Service(59-62)
⏰ 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 (19)
ui/src/contexts/AuthContext.tsx (6)
1-3: LGTM!Imports are clean and minimal, using modern React patterns.
12-32: LGTM!Context type is well-defined with complete auth state and actions. The role hierarchy ordering is correct, and keeping
ROLE_HIERARCHYinternal while exposinghasRoleis a good encapsulation pattern.
40-72: LGTM!The
logoutcallback is properly memoized with empty dependencies. TherefreshUserfunction correctly reads fromlocalStoragedirectly (rather than stale state) and has appropriate error handling that fails closed by logging out on any error.
74-92: LGTM!Login error handling is robust with the
.catch(() => ({}))fallback for non-JSON error responses. State updates follow the correct sequence with localStorage persistence before React state.
94-117: LGTM!The conditional
refreshUsercall based onauthModecorrectly handles different authentication strategies. The provider value shape matches the context type.
119-153: LGTM!The
useAuthhook properly enforces usage within the provider. Permission hooks correctly differentiate between builtin and external auth modes, with appropriate fallbacks toconfig.permissionsfor external modes. The purehasRoleutility enables permission checks outside React components.internal/service/frontend/server.go (1)
23-25: LGTM!The new imports for
fileuserandauthserviceare correctly added to support the builtin authentication feature.internal/service/frontend/auth/builtin.go (5)
1-13: LGTM!Package declaration, imports, and copyright header are correctly structured.
15-50: LGTM!The middleware implementation correctly:
- Builds an O(1) lookup set for public paths
- Bypasses authentication for public paths
- Extracts and validates Bearer tokens
- Injects authenticated user into request context
- Returns appropriate 401 errors with structured JSON responses
73-97: LGTM!The role-based access control middleware correctly:
- Builds an O(1) role lookup set
- Retrieves the authenticated user from context
- Validates the user has one of the required roles
- Returns appropriate 401/403 errors
99-112: LGTM!The convenience middleware wrappers (RequireAdmin, RequireWrite, RequireExecute) correctly compose RequireRole with appropriate role combinations for common access patterns.
114-127: LGTM!The Bearer token extraction correctly validates the Authorization header format and extracts the token value.
internal/persistence/fileuser/store.go (7)
4-45: File-level structure, constants, and indexing fields are clear and idiomaticPackage docs, constants (extensions/permissions), and the
Storelayout withbyID/byUsernameguarded by anRWMutexare straightforward and align with Go conventions; no issues here.
76-111: Index rebuild logic and error handling look solid
rebuildIndexcorrectly clears the maps under a write lock, scansbaseDir, filters by the configured extension, and logs-and-skips malformed files instead of failing the whole index, which is a good resilience trade-off. No changes needed.
113-126:loadUserFromFileis straightforward and composes well with callersThe helper cleanly wraps file I/O and JSON parsing with
%wso callers can useerrors.Ison the underlying causes, and returns a properly converted*auth.User. The//nolint:goseccomment is appropriate given the path is internal.
171-191: Atomic write pattern and permissions are appropriateWriting to a
*.tmpfile with restrictive0600permissions and thenos.Rename-ing into place, with best-effort cleanup on failure, gives you an atomic and reasonably safe persistence path for single-host use. No changes needed here.
219-234:GetByUsernamecorrectly reusesGetByIDfor consistent semanticsThe method validates the input, safely reads
byUsernameunderRLock, and delegates toGetByIDso not-found and file-level error handling stay centralized. This is clean and idiomatic.
236-259:Listbehavior is reasonable; order is intentionally unspecifiedListing IDs under
RLock, then resolving them viaGetByIDand skippingErrUserNotFoundusers provides a robust view even if concurrent deletions happen. The trade-off of n file reads is acceptable for small-to-moderate user counts and the unspecified ordering is fine unless callers explicitly require sorting.
340-345:Countis simple and correctly uses a read lockReturning
int64(len(s.byID))underRLockis the right level of synchronization for this method; nothing to change here.
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @yottahmd. * #1463 (comment) The following files were modified: * `internal/common/config/loader.go` * `internal/core/auth/context.go` * `internal/core/auth/role.go` * `internal/core/auth/user.go` * `internal/persistence/fileuser/store.go` * `internal/service/auth/service.go` * `internal/service/frontend/api/v2/api.go` * `internal/service/frontend/api/v2/auth.go` * `internal/service/frontend/auth/builtin.go` * `internal/service/frontend/server.go` * `internal/service/frontend/templates.go` * `ui/src/App.tsx` * `ui/src/components/ChangePasswordModal.tsx` * `ui/src/components/ProtectedRoute.tsx` * `ui/src/components/UserMenu.tsx` * `ui/src/contexts/AuthContext.tsx` * `ui/src/layouts/Layout.tsx` * `ui/src/lib/fetchJson.ts` * `ui/src/pages/login.tsx` * `ui/src/pages/users/ResetPasswordModal.tsx` * `ui/src/pages/users/UserFormModal.tsx` * `ui/src/pages/users/index.tsx`
Docstrings generation was requested by @yottahmd. * #1463 (comment) The following files were modified: * `internal/common/config/loader.go` * `internal/core/auth/context.go` * `internal/core/auth/role.go` * `internal/core/auth/user.go` * `internal/persistence/fileuser/store.go` * `internal/service/auth/service.go` * `internal/service/frontend/api/v2/api.go` * `internal/service/frontend/api/v2/auth.go` * `internal/service/frontend/auth/builtin.go` * `internal/service/frontend/server.go` * `internal/service/frontend/templates.go` * `ui/src/App.tsx` * `ui/src/components/ChangePasswordModal.tsx` * `ui/src/components/ProtectedRoute.tsx` * `ui/src/components/UserMenu.tsx` * `ui/src/contexts/AuthContext.tsx` * `ui/src/layouts/Layout.tsx` * `ui/src/lib/fetchJson.ts` * `ui/src/pages/login.tsx` * `ui/src/pages/users/ResetPasswordModal.tsx` * `ui/src/pages/users/UserFormModal.tsx` * `ui/src/pages/users/index.tsx` <details> <summary>These files were ignored</summary> * `internal/common/config/config_test.go` * `internal/common/config/loader_test.go` * `internal/core/auth/role_test.go` * `internal/persistence/fileuser/store_test.go` * `internal/service/auth/service_test.go` </details> <details> <summary>These file types are not supported</summary> * `api/v2/api.yaml` * `internal/service/frontend/templates/base.gohtml` * `ui/index.html` * `ui/package.json` </details> <details> <summary>ℹ️ Note</summary><blockquote> CodeRabbit cannot perform edits on its own pull requests yet. </blockquote></details> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Yota Hamada <yohamta@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
255-276: Align users directory environment variable name with loader bindingsThe README documents
DAGU_USERS_DIR, but the config tests wirepaths.usersDirfromDAGU_PATHS_USERS_DIR(seeinternal/common/config/loader_test.go,TestLoad_AuthMode→"DAGU_PATHS_USERS_DIR"). It would be good to standardize on a single env-var name (or explicitly document both if both are supported) so users aren’t confused about which one actually works.Also, if you care about satisfying grammar checkers, you might consider “Built-in Authentication” in the heading, though keeping “Builtin” is reasonable since it mirrors the literal
builtinmode value.
♻️ Duplicate comments (1)
internal/service/frontend/server.go (1)
121-133: Revisit printing auto-generated admin passwords to stdoutEven with the comment warning, printing the generated admin password to stdout can leak credentials via container logs or external log aggregation. If feasible, consider gating this behind a config flag or providing an alternative discovery mechanism (e.g., writing to a restricted file or requiring an explicit password in config).
🧹 Nitpick comments (14)
internal/common/config/loader_test.go (1)
176-189: Derived usersDir behavior is well tested; consider also asserting the env overrideThe new
UsersDirassertions in the env/YAML tests plusTestLoad_EdgeCases_DerivedPathsclearly pin the “{dataDir}/users” default behavior, which is great.To fully cover the override path, you might also add an assertion (e.g. in
TestLoad_AuthMode’s"AuthModeFromEnv"subtest) that whenDAGU_PATHS_USERS_DIRis set,cfg.Paths.UsersDirequals that value, so any future binding changes are caught immediately.Also applies to: 409-421, 506-517
ui/src/contexts/AuthContext.tsx (1)
149-185: Be explicit when gating builtin‑only UI on both auth mode and role
useIsAdminintentionally returnstruewheneverconfig.authMode !== 'builtin', delegating authorization to whatever external system is in use, whileuseCanWrite/useCanExecuteonly enforce role thresholds in builtin mode.This is fine, but it means any UI that should exist only when builtin RBAC is enabled (e.g., builtin user-management screens) should explicitly check
config.authMode === 'builtin'in addition to using these helpers, otherwise those entry points may appear under OIDC/basic modes even though the backend will later reject the calls.ui/src/lib/fetchJson.ts (1)
1-30: Reuse TOKEN_KEY and normalize headers to handleHeadersinstances safelyTwo small improvements would make this helper more robust and DRY:
- The token key string is duplicated here and in
AuthContext; reusingTOKEN_KEYavoids drift if the storage key ever changes.- Spreading
init?.headersinto an object only works as expected when callers pass a plain{[key]: value}; if aHeadersinstance orstring[][]is ever passed, those header values won’t be preserved. Creating aHeadersfrominit?.headersavoids this.You could implement both like this:
+import { TOKEN_KEY } from '../contexts/AuthContext'; + export default async function fetchJson<JSON = unknown>( input: RequestInfo, init?: RequestInit ): Promise<JSON> { - const token = localStorage.getItem('dagu_auth_token'); - const headers: HeadersInit = { - ...(init?.headers || {}), - Accept: 'application/json', - }; - - if (token) { - (headers as Record<string, string>)['Authorization'] = `Bearer ${token}`; - } + const token = + typeof localStorage !== 'undefined' ? localStorage.getItem(TOKEN_KEY) : null; + const headers = new Headers(init?.headers); + headers.set('Accept', 'application/json'); + if (token) { + headers.set('Authorization', `Bearer ${token}`); + } const response = await fetch(`${getConfig().apiURL}${input}`, { ...init, - headers, + headers, });This keeps the key definition single-sourced and ensures all existing headers are preserved regardless of how callers populate
init.headers.internal/core/auth/context.go (1)
21-23: Remove duplicate docstring line.Lines 21-22 contain redundant documentation for
UserFromContext. The second line (22-23) is more complete and should be kept.// UserFromContext retrieves the authenticated user from the context. -// UserFromContext retrieves the authenticated *User stored in ctx. -// It returns the user and true if a *User value is present for the package's userContextKey, or nil and false otherwise. +// It returns the user and true if a *User value is present for the package's userContextKey, or nil and false otherwise. func UserFromContext(ctx context.Context) (*User, bool) {ui/src/components/UserMenu.tsx (1)
40-53: Consider extractinggetRoleBadgeColorto a shared utility.This function is duplicated in
ui/src/pages/users/index.tsx(lines 106-119). Extract to a shared utility (e.g.,ui/src/lib/roleUtils.ts) to maintain consistency and reduce duplication.// ui/src/lib/roleUtils.ts export function getRoleBadgeColor(role: string): string { switch (role) { case 'admin': return 'bg-red-500/20 text-red-600 dark:text-red-400'; case 'manager': return 'bg-blue-500/20 text-blue-600 dark:text-blue-400'; case 'operator': return 'bg-green-500/20 text-green-600 dark:text-green-400'; case 'viewer': default: return 'bg-gray-500/20 text-gray-600 dark:text-gray-400'; } }ui/src/pages/users/index.tsx (2)
51-53: Stabilize useEffect dependency to avoid potential re-renders.Using
appBarContextas a dependency may cause re-renders if the context object reference changes. ExtractsetTitlebefore the effect or use a ref.+ const { setTitle } = useContext(AppBarContext); + // Set page title useEffect(() => { - appBarContext.setTitle('User Management'); - }, [appBarContext]); + setTitle('User Management'); + }, [setTitle]);
106-119: DuplicategetRoleBadgeColorfunction.This helper is duplicated from
UserMenu.tsx. Consider extracting to a shared utility as suggested in the UserMenu review.ui/src/components/ChangePasswordModal.tsx (1)
29-31: Token access pattern is cleaner here but inconsistent with other modals.This component uses
useAuth().tokendirectly, which is preferable tolocalStorage.getItem(TOKEN_KEY). Consider updating other modals (ResetPasswordModal, UserFormModal, UsersPage) to use the same pattern for consistency.ui/src/pages/users/ResetPasswordModal.tsx (1)
74-74: UseTOKEN_KEYconstant instead of hardcoded string.For consistency with other components, import and use the
TOKEN_KEYconstant fromAuthContext.+import { TOKEN_KEY } from '@/contexts/AuthContext'; ... - const token = localStorage.getItem('dagu_auth_token'); + const token = localStorage.getItem(TOKEN_KEY);internal/service/frontend/api/v2/auth.go (2)
24-29: Consider distinguishing malformed requests from auth failuresBoth
Login(nilrequest.Body) andChangePassword(“Invalid request body”) currently map malformed/empty bodies to auth-style errors (401/400). If your OpenAPI surface allows a 400 response for these operations, consider using a dedicated “bad request” error code/message so clients can differentiate “malformed input” from “bad credentials.”Also applies to: 86-91
115-134: Tighten thetoAPIUserscomment for clarityThe comment at Line 127 is truncated. Consider making it self-contained, e.g.:
-// preserving the input order. -func toAPIUsers(users []*auth.User) []api.User { +// toAPIUsers converts a slice of core users into API users, preserving input order. +func toAPIUsers(users []*auth.User) []api.User {internal/service/auth/service_test.go (1)
17-512: Auth service test coverage looks strongThe temp store setup and the breadth of cases (happy paths + all key error paths) give good confidence in the new auth service behavior. If you ever want to reduce boilerplate/assert noise, you could migrate to
testify/requirein a follow-up, but it’s not necessary for this PR.internal/service/frontend/api/v2/api.go (1)
53-66: AuthService interface is tightly coupled toauthserviceDTOs
AuthServicecurrently depends onauthservice.TokenResult,CreateUserInput, andUpdateUserInput. That makes swapping in an alternative implementation harder without also importinginternal/service/auth. If you ever need pluggability beyond the builtin service, consider narrowing this interface to core-domain types (e.g.,auth.Userand simple primitives) or moving the DTOs to a more neutral package.internal/service/auth/service.go (1)
170-183: Treat tokens for deleted users as invalid tokens
GetUserFromTokencurrently wraps any store error as"failed to get user from token", which becomes a 500 even when the underlying error isauth.ErrUserNotFound(e.g., user deleted after token issuance). That scenario is better modeled as an invalid/expired token.Consider mapping it like this:
user, err := s.store.GetByID(ctx, claims.UserID) if err != nil { - return nil, fmt.Errorf("failed to get user from token: %w", err) + if errors.Is(err, auth.ErrUserNotFound) { + return nil, ErrInvalidToken + } + return nil, fmt.Errorf("failed to get user from token: %w", err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
README.md(2 hunks)internal/cmd/context.go(1 hunks)internal/common/config/loader.go(7 hunks)internal/common/config/loader_test.go(10 hunks)internal/core/auth/context.go(1 hunks)internal/core/auth/role.go(1 hunks)internal/core/auth/user.go(1 hunks)internal/integration/queue_shell_test.go(2 hunks)internal/persistence/fileuser/store.go(1 hunks)internal/service/auth/service.go(1 hunks)internal/service/auth/service_test.go(1 hunks)internal/service/frontend/api/v1/dags_test.go(0 hunks)internal/service/frontend/api/v1/health_test.go(0 hunks)internal/service/frontend/api/v2/api.go(8 hunks)internal/service/frontend/api/v2/auth.go(1 hunks)internal/service/frontend/api/v2/dagruns.go(8 hunks)internal/service/frontend/api/v2/dags.go(8 hunks)internal/service/frontend/auth/builtin.go(1 hunks)internal/service/frontend/server.go(4 hunks)internal/service/frontend/templates.go(2 hunks)internal/test/server.go(1 hunks)ui/src/App.tsx(3 hunks)ui/src/components/ChangePasswordModal.tsx(1 hunks)ui/src/components/ProtectedRoute.tsx(1 hunks)ui/src/components/UserMenu.tsx(1 hunks)ui/src/contexts/AuthContext.tsx(1 hunks)ui/src/layouts/Layout.tsx(5 hunks)ui/src/lib/fetchJson.ts(2 hunks)ui/src/pages/login.tsx(1 hunks)ui/src/pages/users/ResetPasswordModal.tsx(1 hunks)ui/src/pages/users/UserFormModal.tsx(1 hunks)ui/src/pages/users/index.tsx(1 hunks)
💤 Files with no reviewable changes (2)
- internal/service/frontend/api/v1/dags_test.go
- internal/service/frontend/api/v1/health_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- internal/core/auth/user.go
- internal/service/frontend/auth/builtin.go
- ui/src/components/ProtectedRoute.tsx
- internal/service/frontend/api/v2/dagruns.go
- ui/src/layouts/Layout.tsx
- internal/common/config/loader.go
- internal/service/frontend/api/v2/dags.go
- ui/src/pages/login.tsx
- ui/src/pages/users/UserFormModal.tsx
🧰 Additional context used
📓 Path-based instructions (4)
ui/**/*.{ts,tsx,jsx,js}
📄 CodeRabbit inference engine (ui/CLAUDE.md)
ui/**/*.{ts,tsx,jsx,js}: Use developer-centric UI design with high information density, minimal whitespace, compact components, and no unnecessary decorations
Support both light and dark modes for all UI components using Tailwind CSS class pairs likedark:bg-slate-700
NEVER use full-page loading overlays or LoadingIndicator components that hide content - show stale data while fetching updates instead
Use compact modal design with small headers, minimal padding (p-2 or p-3), tight spacing, and support keyboard navigation (arrows, enter, escape)
Use small heights for form elements: select boxes h-7 or smaller, buttons h-7 or h-8, inputs with compact padding (py-0.5 or py-1)
Minimize row heights in tables and lists while maintaining readability, merge related columns, and always handle long text withwhitespace-normal break-words
Use consistent metadata styling withbg-slate-200 dark:bg-slate-700backgrounds and maintain text hierarchy with primary/secondary/muted text colors
Use flexbox-first layouts withmin-h-0andoverflow-hiddento prevent layout breaks, account for fixed elements when setting heights
Maintain keyboard navigation support in all interactive components with appropriate focus indicators and ARIA labels
Files:
ui/src/pages/users/index.tsxui/src/components/UserMenu.tsxui/src/App.tsxui/src/pages/users/ResetPasswordModal.tsxui/src/contexts/AuthContext.tsxui/src/components/ChangePasswordModal.tsxui/src/lib/fetchJson.ts
ui/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
ui/**/*.{ts,tsx}: The React + TypeScript frontend resides inui/, with production bundles copied tointernal/service/frontend/assetsbymake ui
UI code follows ESLint + Prettier (2-space indent) and Tailwind utilities; name React components in PascalCase (JobList.tsx) and hooks withuse*(useJobs.ts)
Files:
ui/src/pages/users/index.tsxui/src/components/UserMenu.tsxui/src/App.tsxui/src/pages/users/ResetPasswordModal.tsxui/src/contexts/AuthContext.tsxui/src/components/ChangePasswordModal.tsxui/src/lib/fetchJson.ts
**/*.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/integration/queue_shell_test.gointernal/test/server.gointernal/service/frontend/api/v2/api.gointernal/service/frontend/api/v2/auth.gointernal/common/config/loader_test.gointernal/core/auth/role.gointernal/persistence/fileuser/store.gointernal/service/frontend/templates.gointernal/core/auth/context.gointernal/cmd/context.gointernal/service/auth/service_test.gointernal/service/auth/service.gointernal/service/frontend/server.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/integration/queue_shell_test.gointernal/common/config/loader_test.gointernal/service/auth/service_test.go
🧠 Learnings (11)
📚 Learning: 2025-12-04T10:34:17.051Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.051Z
Learning: Applies to ui/**/*.{ts,tsx} : UI code follows ESLint + Prettier (2-space indent) and Tailwind utilities; name React components in PascalCase (`JobList.tsx`) and hooks with `use*` (`useJobs.ts`)
Applied to files:
ui/src/pages/users/index.tsx
📚 Learning: 2025-12-04T10:34:17.051Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.051Z
Learning: Applies to ui/**/*.{ts,tsx} : The React + TypeScript frontend resides in `ui/`, with production bundles copied to `internal/service/frontend/assets` by `make ui`
Applied to files:
ui/src/pages/users/index.tsx
📚 Learning: 2025-12-04T10:34:01.023Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: ui/CLAUDE.md:0-0
Timestamp: 2025-12-04T10:34:01.023Z
Learning: Applies to ui/**/*.{ts,tsx,jsx,js} : Use compact modal design with small headers, minimal padding (p-2 or p-3), tight spacing, and support keyboard navigation (arrows, enter, escape)
Applied to files:
ui/src/pages/users/ResetPasswordModal.tsx
📚 Learning: 2025-12-04T10:34:17.051Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.051Z
Learning: `make run` starts the Go scheduler and serves the compiled UI (fails fast if `ui/dist` is missing)
Applied to files:
internal/test/server.go
📚 Learning: 2025-12-04T10:34:17.051Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.051Z
Learning: Applies to **/*.go : Backend entrypoint in `cmd/` orchestrates the scheduler and CLI; runtime, persistence, and service layers sit under `internal/*` (for example `internal/runtime`, `internal/persistence`)
Applied to files:
internal/service/frontend/api/v2/api.gointernal/service/frontend/server.go
📚 Learning: 2025-12-04T10:34:17.051Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.051Z
Learning: Applies to **/*.go : Keep Go files `gofmt`/`goimports` clean; use tabs, PascalCase for exported symbols (`SchedulerClient`), lowerCamelCase for locals, and `Err...` names for package-level errors
Applied to files:
internal/common/config/loader_test.go
📚 Learning: 2025-12-04T10:34:01.023Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: ui/CLAUDE.md:0-0
Timestamp: 2025-12-04T10:34:01.023Z
Learning: Applies to ui/~/.config/dagu/config.yaml : Dagu configuration file supports environment variables with DAGU_* prefix, per-DAG overrides, and command-line arguments
Applied to files:
README.md
📚 Learning: 2025-12-04T10:34:01.023Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: ui/CLAUDE.md:0-0
Timestamp: 2025-12-04T10:34:01.023Z
Learning: Applies to ui/**/*.{yaml,yml} : Dagu YAML configurations should support minimal setup with sensible defaults and support for environment variable expansion and command substitution
Applied to files:
README.md
📚 Learning: 2025-12-04T10:34:17.051Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.051Z
Learning: Applies to **/*_test.go : Use `stretchr/testify/require` and shared fixtures from `internal/test` instead of duplicating mocks
Applied to files:
internal/service/auth/service_test.go
📚 Learning: 2025-12-04T10:34:17.051Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.051Z
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/auth/service_test.go
📚 Learning: 2025-12-04T10:34:17.051Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.051Z
Learning: `make test` (or `make test-coverage`) executes the Go suite via `gotestsum`; append `TEST_TARGET=./internal/...` to focus packages
Applied to files:
internal/service/auth/service_test.go
🧬 Code graph analysis (13)
ui/src/pages/users/index.tsx (6)
ui/src/api/v2/schema.ts (1)
components(889-1557)ui/src/contexts/ConfigContext.tsx (1)
useConfig(38-40)ui/src/contexts/AuthContext.tsx (2)
useAuth(136-142)TOKEN_KEY(24-24)ui/src/contexts/AppBarContext.ts (1)
AppBarContext(11-21)ui/src/pages/users/UserFormModal.tsx (1)
UserFormModal(47-205)ui/src/pages/users/ResetPasswordModal.tsx (1)
ResetPasswordModal(35-174)
internal/integration/queue_shell_test.go (1)
internal/service/frontend/server.go (1)
NewServer(51-89)
ui/src/App.tsx (1)
ui/src/contexts/AuthContext.tsx (1)
AuthProvider(45-128)
ui/src/pages/users/ResetPasswordModal.tsx (4)
ui/src/api/v2/schema.ts (1)
components(889-1557)ui/src/contexts/ConfigContext.tsx (1)
useConfig(38-40)ui/src/pages/index.tsx (1)
mutate(382-382)ui/src/pages/dags/index.tsx (1)
mutate(213-213)
internal/service/frontend/api/v2/auth.go (4)
api/v2/api.gen.go (14)
LoginRequestObject(4846-4848)LoginResponseObject(4850-4852)Login401JSONResponse(4863-4863)Login200JSONResponse(4854-4854)User(988-1003)GetCurrentUserRequestObject(4884-4885)GetCurrentUserResponseObject(4887-4889)GetCurrentUser401JSONResponse(4900-4900)GetCurrentUser200JSONResponse(4891-4891)ChangePasswordRequestObject(4799-4801)ChangePassword401JSONResponse(4825-4825)ChangePassword400JSONResponse(4816-4816)ChangePassword200JSONResponse(4807-4807)UserRole(1012-1012)internal/service/auth/service.go (3)
ErrInvalidCredentials(22-22)ErrPasswordMismatch(26-26)ErrWeakPassword(27-27)internal/core/auth/user.go (1)
User(13-27)internal/core/auth/context.go (1)
UserFromContext(24-27)
ui/src/contexts/AuthContext.tsx (1)
ui/src/contexts/ConfigContext.tsx (1)
useConfig(38-40)
internal/persistence/fileuser/store.go (2)
internal/core/auth/user.go (2)
User(13-27)UserForStorage(45-52)internal/core/auth/store.go (4)
ErrInvalidUserID(21-21)ErrInvalidUsername(19-19)ErrUserAlreadyExists(17-17)ErrUserNotFound(14-14)
internal/service/frontend/templates.go (3)
internal/common/config/config.go (1)
AuthMode(131-131)ui/src/contexts/ConfigContext.tsx (1)
AuthMode(16-16)internal/common/config/path.go (1)
Paths(13-30)
ui/src/components/ChangePasswordModal.tsx (2)
ui/src/contexts/ConfigContext.tsx (1)
useConfig(38-40)ui/src/contexts/AuthContext.tsx (1)
useAuth(136-142)
internal/core/auth/context.go (1)
internal/core/auth/user.go (1)
User(13-27)
internal/cmd/context.go (2)
internal/service/frontend/server.go (1)
NewServer(51-89)internal/common/config/config.go (1)
Config(9-39)
internal/service/auth/service_test.go (2)
internal/core/auth/role.go (4)
Role(16-16)RoleManager(22-22)RoleViewer(26-26)RoleAdmin(20-20)internal/core/auth/store.go (1)
ErrUserNotFound(14-14)
internal/service/auth/service.go (3)
internal/core/auth/role.go (2)
Role(16-16)RoleAdmin(20-20)internal/core/auth/store.go (2)
UserStore(26-52)ErrUserNotFound(14-14)internal/core/auth/user.go (2)
User(13-27)NewUser(31-41)
🪛 LanguageTool
README.md
[grammar] ~265-~265: Ensure spelling is correct
Context: ...mail whitelist (comma-separated) | ### Builtin Authentication (RBAC) When `DAGU_AUTH_...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ 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 (23)
internal/service/frontend/templates.go (1)
52-64: AuthMode and usersDir exposure in templates is coherentPlumbing
AuthModeintofuncsConfigand exposing it via"authMode"plus"pathUsersDir"cleanly aligns templates with the new auth/users configuration; both helpers are simple accessors and safe to use.Also applies to: 139-144
internal/common/config/loader_test.go (2)
154-166: Good coverage for builtin auth defaults, usersDir, and warningsThe extended expectations for
Server.Auth(builtin admin default + 24h TTL),Paths.UsersDir(derived fromDataDir), and the explicit OIDC auto-detect warning string give solid regression coverage for the new auth wiring and loader behavior. Nothing to change here.Also applies to: 188-189, 220-221
365-380: AuthMode/AuthBuiltin tests nicely exercise the new matrix of modesThe
TestLoad_AuthModeandTestLoad_AuthBuiltinsubtests cover YAML and env-driven modes (none,builtin,oidc), builtin admin credentials, token secret/TTL parsing (including default TTL), and permissiveness around empty admin passwords. Together with the warnings tests, this should catch most loader regressions in the auth surface.Also applies to: 420-421, 835-961
ui/src/contexts/AuthContext.tsx (1)
45-83: AuthProvider state management and token persistence look solidThe provider’s use of
localStorage-backed token state,refreshUseron builtin auth, andisAuthenticated: !!user(not just presence of a token) gives a clear and robust client-side model for builtin auth. Error paths correctly fall back tologout()and clear both token and user.Also applies to: 105-128
internal/cmd/context.go (1)
175-200: Correctly propagate frontend.NewServer errors from Context.NewServerReturning
frontend.NewServer(...)directly matches the updated(*Server, error)constructor and ensures builtin-auth initialization errors surface to CLI callers instead of being dropped.internal/test/server.go (1)
67-70: Test helper correctly adapts to NewServer’s error returnCapturing
(server, err)and assertingrequire.NoError(t, err)before callingserver.Serveis the right adjustment for the new constructor and avoids silently proceeding with a nil server.internal/integration/queue_shell_test.go (1)
40-53: Integration test’s NewServer usage is updated appropriatelyThe test now handles
frontend.NewServer’s(server, err)return and fails fast on construction errors viarequire.NoError, which is the right adaptation and keeps the rest of the test logic unchanged.internal/core/auth/context.go (1)
1-19: LGTM!The context key pattern using a private type prevents key collisions with other packages.
WithUserandUserFromContextfollow idiomatic Go context value handling.ui/src/components/UserMenu.tsx (2)
24-38: LGTM!The component correctly guards rendering based on auth mode and authentication state. The logout handler properly clears state and navigates to login.
55-100: LGTM!Compact UI design with appropriate element heights (h-7, h-8), dark mode support, and proper keyboard accessibility via the dropdown menu primitives. The modal integration is clean.
ui/src/pages/users/index.tsx (3)
55-79: LGTM!The fetch implementation properly uses the shared
TOKEN_KEYconstant, handles errors gracefully, and manages loading state. The useCallback dependency onconfig.apiURLis correct.
81-104: LGTM!Delete handler correctly checks for
deletingUserpresence, clears errors on success, and refreshes the user list. Error extraction from API response is handled properly.
153-225: LGTM!Table implementation follows guidelines: inline loading state (not full-page overlay), compact row heights, proper handling of empty state, and self-deletion prevention via
disabled={user.id === currentUser?.id}. Dark mode is supported throughout.ui/src/components/ChangePasswordModal.tsx (2)
52-97: LGTM!Validation logic is correct (password match, minimum length). Error extraction handles multiple response formats (
data.message,data.error?.message). The auto-close after success provides good UX feedback.
99-186: LGTM!Compact modal design with appropriate heights (h-8, h-9), proper accessibility attributes (
autoComplete,required), and dark mode support for success/error states.ui/src/pages/users/ResetPasswordModal.tsx (1)
35-98: LGTM on validation and error handling.Validation logic (password match, min length, user presence check) and error handling are implemented correctly. The auto-close after success provides consistent UX with other modals.
ui/src/App.tsx (2)
106-156: LGTM!The routing structure correctly separates public (
/login) and protected routes.AuthProviderwrapping ensures auth context is available throughout. The nestedProtectedRoutefor/userswithrequiredRole="admin"provides proper RBAC enforcement—the outer wrapper handles authentication while the inner enforces admin role.
139-146: Nested ProtectedRoute is intentional but could be simplified.The outer
ProtectedRoute(line 118) already ensures authentication. The innerProtectedRoutefor/usersonly needs to check the admin role. The current implementation works correctly, but ifProtectedRouteshort-circuits when already authenticated, the double-check is harmless.internal/core/auth/role.go (1)
8-77: Role/permission helpers are clear and consistentThe
Roleconstants,Valid/CanWrite/CanExecute/IsAdmin, andParseRolebehavior are all coherent and match the documented hierarchy; this will keep authorization checks readable across the codebase.internal/service/frontend/api/v2/api.go (1)
195-231: Auth middleware split and permission helpers look correctWiring builtin JWT auth via
BuiltinAuthMiddlewareonly whenAuthModeBuiltin+ non-nilauthService, with a fallback to the legacy middleware otherwise, looks sound. TherequireAdmin/requireWrite/requireExecutehelpers correctly short-circuit when auth is disabled and return structured 401/403 errors when enabled, which should integrate cleanly withhandleError.Also applies to: 297-367
internal/service/frontend/server.go (1)
307-313: Disabling v1 API when auth is enabled is clearly signpostedShort-circuiting v1 route setup when
Auth.Mode != AuthModeNoneand emitting an explicit info log is a good way to make this breaking behavior visible to operators while steering them toward v2.internal/persistence/fileuser/store.go (1)
20-360: File-backed user store implementation looks robustThe store’s invariants (non-empty ID/username), atomic write pattern, index rebuild, and
errors.Ishandling for missing files andauthsentinels are all well thought out. The RWMutex usage around the in-memory indices also looks correct for concurrent access.internal/service/auth/service.go (1)
335-366: Admin bootstrap flow is sensible
EnsureAdminUser’s “no users → create admin (with optional generated password)” semantics, combined withvalidatePasswordandauth.RoleAdmin, look correct and align with the server’s builtin-auth initialization.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/service/frontend/server.go (1)
126-142: Security concern about printing admin password to stdout was previously raised.This concern about the auto-generated admin password being printed to stdout (which could be captured by container orchestrators, CI/CD systems, or monitoring tools) was already flagged in a previous review. Consider the recommendations from that review regarding secure alternatives.
🧹 Nitpick comments (4)
ui/src/pages/users/ResetPasswordModal.tsx (2)
122-150: Align input sizing with compact UI guidelinesBoth password
Inputfields useclassName="h-9", which is a bit larger than the compact form control sizes described in the UI guidelines (ideallyh-7or similar with tight vertical padding). Consider reducing the height and padding to keep the modal denser and more consistent with other forms.For example:
- <Input + <Input id="new-password" type="password" value={newPassword} onChange={(e) => setNewPassword(e.target.value)} required autoComplete="new-password" - className="h-9" + className="h-7 py-1" placeholder="Minimum 8 characters" />(and similarly for the confirm-password input).
As per coding guidelines and retrieved learnings, compact modal and control sizing is preferred.
89-102: Consider cleaning up the success close timeout on unmount/early close
setTimeout(() => handleClose(), 1500)will still fire even if the parent unmounts this modal or otherwise stops rendering it before the timeout elapses, which can lead to state updates on an unmounted component in some lifecycles.Consider storing the timeout ID in a ref and clearing it in a
useEffectcleanup (and/or when the dialog is closed manually) to make this more robust.internal/service/auth/service.go (2)
335-366: Consider graceful handling of concurrent admin bootstrap race.If multiple server instances start simultaneously with an empty user store, both might see
count == 0and attempt to create the admin user. The second instance will fail with a wrapped error fromstore.Create(likelyErrUserAlreadyExists).For a more graceful bootstrap experience, consider handling this race:
_, err = s.CreateUser(ctx, CreateUserInput{ Username: username, Password: generatedPassword, Role: auth.RoleAdmin, }) if err != nil { + // Handle race condition: another instance may have created the admin + if errors.Is(err, auth.ErrUserAlreadyExists) { + return "", false, nil + } return "", false, fmt.Errorf("failed to create admin user: %w", err) }
376-385: Clarify entropy ingenerateSecurePassworddocumentation.The function generates 16 random bytes (~128 bits entropy) but then base64-encodes and truncates to
lengthcharacters. The actual entropy of the output depends on the truncation. Forlength=16, the output has approximately 96 bits of entropy (16 chars × 6 bits per base64 char), which is still secure but differs from the raw byte count.Consider either:
- Updating the comment to clarify the effective entropy, or
- Calculating bytes needed to achieve target entropy after encoding
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/service/auth/service.go(1 hunks)internal/service/frontend/server.go(4 hunks)ui/src/pages/users/ResetPasswordModal.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/server.gointernal/service/auth/service.go
ui/**/*.{ts,tsx,jsx,js}
📄 CodeRabbit inference engine (ui/CLAUDE.md)
ui/**/*.{ts,tsx,jsx,js}: Use developer-centric UI design with high information density, minimal whitespace, compact components, and no unnecessary decorations
Support both light and dark modes for all UI components using Tailwind CSS class pairs likedark:bg-slate-700
NEVER use full-page loading overlays or LoadingIndicator components that hide content - show stale data while fetching updates instead
Use compact modal design with small headers, minimal padding (p-2 or p-3), tight spacing, and support keyboard navigation (arrows, enter, escape)
Use small heights for form elements: select boxes h-7 or smaller, buttons h-7 or h-8, inputs with compact padding (py-0.5 or py-1)
Minimize row heights in tables and lists while maintaining readability, merge related columns, and always handle long text withwhitespace-normal break-words
Use consistent metadata styling withbg-slate-200 dark:bg-slate-700backgrounds and maintain text hierarchy with primary/secondary/muted text colors
Use flexbox-first layouts withmin-h-0andoverflow-hiddento prevent layout breaks, account for fixed elements when setting heights
Maintain keyboard navigation support in all interactive components with appropriate focus indicators and ARIA labels
Files:
ui/src/pages/users/ResetPasswordModal.tsx
ui/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
ui/**/*.{ts,tsx}: The React + TypeScript frontend resides inui/, with production bundles copied tointernal/service/frontend/assetsbymake ui
UI code follows ESLint + Prettier (2-space indent) and Tailwind utilities; name React components in PascalCase (JobList.tsx) and hooks withuse*(useJobs.ts)
Files:
ui/src/pages/users/ResetPasswordModal.tsx
🧠 Learnings (2)
📚 Learning: 2025-12-04T10:34:17.051Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.051Z
Learning: Applies to **/*.go : Backend entrypoint in `cmd/` orchestrates the scheduler and CLI; runtime, persistence, and service layers sit under `internal/*` (for example `internal/runtime`, `internal/persistence`)
Applied to files:
internal/service/frontend/server.go
📚 Learning: 2025-12-04T10:34:01.023Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: ui/CLAUDE.md:0-0
Timestamp: 2025-12-04T10:34:01.023Z
Learning: Applies to ui/**/*.{ts,tsx,jsx,js} : Use compact modal design with small headers, minimal padding (p-2 or p-3), tight spacing, and support keyboard navigation (arrows, enter, escape)
Applied to files:
ui/src/pages/users/ResetPasswordModal.tsx
🧬 Code graph analysis (3)
internal/service/frontend/server.go (4)
internal/service/auth/service.go (3)
Config(41-48)Service(59-62)New(66-77)internal/common/config/config.go (5)
Config(9-39)Server(83-120)Auth(143-149)AuthModeBuiltin(137-137)AuthMode(131-131)internal/service/frontend/api/v2/api.go (3)
APIOption(69-69)WithAuthService(73-77)New(89-129)internal/persistence/fileuser/store.go (1)
New(52-78)
internal/service/auth/service.go (3)
internal/core/auth/role.go (2)
Role(16-16)RoleAdmin(20-20)internal/core/auth/store.go (3)
UserStore(26-52)ErrUserNotFound(14-14)ErrInvalidUsername(19-19)internal/core/auth/user.go (2)
User(13-27)NewUser(31-41)
ui/src/pages/users/ResetPasswordModal.tsx (4)
ui/src/contexts/ConfigContext.tsx (1)
useConfig(38-40)ui/src/components/ui/label.tsx (1)
Label(22-22)ui/src/components/ui/input.tsx (1)
Input(21-21)ui/src/components/ui/button.tsx (1)
Button(59-59)
⏰ 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 (9)
ui/src/pages/users/ResetPasswordModal.tsx (1)
23-173: Overall implementation ofResetPasswordModallooks solidThe component cleanly wires validation, API interaction, error/success feedback, and dialog lifecycle. State reset on close, guarded submit when
useris missing, and inline error parsing from the response are all sensible and consistent with the rest of the UI.internal/service/frontend/server.go (3)
48-89: LGTM! Fail-fast authentication initialization properly implemented.The updated signature returning
(*Server, error)and the fail-fast error propagation at lines 64-66 correctly addresses the concern about servers running without expected authentication when auth is configured but fails to initialize.
312-318: LGTM! V1 API deprecation under auth mode is properly communicated.The startup log message clearly informs operators that V1 API is disabled when authentication is enabled, with guidance to use V2 API instead.
103-107: No action needed.UsersDiris properly defined in thePathsConfigstruct (internal/common/config/config.goline 202) and initialized during config loading atinternal/common/config/loader.goline 279, with a fallback tofilepath.Join(cfg.Paths.DataDir, "users")if not explicitly set (lines 442-443). The field is also validated to ensure it's set when builtin auth is enabled.internal/service/auth/service.go (5)
31-38: LGTM! Security constants are well-chosen.The bcrypt cost of 12 aligns with OWASP recommendations (10+), and the minimum password length of 8 characters meets baseline security requirements.
84-102: LGTM! Secure authentication with timing attack mitigation.The dummy hash comparison for non-existent users (line 91) prevents timing-based user enumeration, and the consolidated handling of
ErrUserNotFoundandErrInvalidUsername(line 88) correctly maps both toErrInvalidCredentials.
148-153: Good security: Algorithm validation prevents JWT confusion attacks.The explicit check for HMAC signing method (line 149) prevents algorithm substitution attacks where an attacker might try to use "none" or asymmetric algorithms.
192-213: LGTM! User creation with proper validation chain.Password and role validation happen before any persistence, and the bcrypt cost is configurable through the service config.
273-280: Good guardrail: Preventing self-deletion.The
ErrCannotDeleteSelfcheck protects against accidental account lockout by preventing users from deleting their own accounts.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/service/frontend/api/v2/api.go (1)
209-220: OIDC enablement wrongly requiresClientSecret, breaking public-client support.Here OIDC is only initialized when
authOIDC.ClientId,authOIDC.ClientSecret, andauthOIDC.Issuerare all non-empty, butInitVerifierAndConfigexplicitly documents thatClientSecretmay be empty for public clients. This condition will silently disable OIDC for valid public-client configurations.Consider dropping the
ClientSecretcheck from the condition, e.g.:- if authOIDC.ClientId != "" && authOIDC.ClientSecret != "" && authOIDC.Issuer != "" { + if authOIDC.ClientId != "" && authOIDC.Issuer != "" {so that public clients with an empty secret can still negotiate OIDC correctly.
🧹 Nitpick comments (13)
internal/auth/role_test.go (1)
10-136: Role tests look solid; consider explicit subtest names andrequireThe table-driven coverage for
Valid,CanWrite,CanExecute,IsAdmin,ParseRole, andAllRoleslooks good. For ergonomics, you might:
- Add a
name stringfield in the test cases and use that int.Runinstead ofstring(tt.role)/ raw input, so even the empty-string cases get meaningful subtest names.- Switch to
stretchr/testify/requirefor assertions to cut down on repetitiveif ... { t.Errorf/Fatalf }and fail fast on setup errors, in line with the repo’s testing guidelines.As per coding guidelines, prefer
requirein*_test.gowhere practical.internal/auth/context.go (1)
16-27: Treatnilusers in context as “no user” for extra safetyIf
WithUseris ever called with aniluser,UserFromContextwill currently return(nil, true), which could lead to a nil dereference in callers that only check the boolean. Consider either:
- Having
WithUserno-op whenuser == nil, or- Having
UserFromContexttreat a storednilas(nil, false).This keeps the contract simple for callers:
okimplies a non-nil*User.internal/persistence/fileuser/store_test.go (1)
15-240: Store tests have good coverage; refine temp dirs, assertions, andos.StathandlingThese tests do a nice job exercising the file-backed store end-to-end (CRUD, duplicate usernames, not-found semantics, index rebuild, and file lifecycle on disk).
A few non-blocking refinements:
- In all tests using
os.MkdirTemp/RemoveAll, considert.TempDir()instead; it simplifies cleanup and avoids ignoringRemoveAllerrors.- Per repo guidance, you could switch repeated
if err != nil { t.Fatalf(...) }and equality checks tostretchr/testify/require/assertto make failures more concise and uniform.- In
TestStore_FileExists, treat unexpectedos.Staterrors as failures, e.g.if err != nil && os.IsNotExist(err)/if err != nil { t.Fatalf(...) }, so permission or IO errors don’t get silently ignored.Based on learnings, tests under
**/*_test.goare encouraged to userequireand shared fixtures where possible.internal/auth/role.go (1)
8-77: Role API is clear and consistent; consider deriving error message fromallRolesThe
Roleconstants, helper predicates, andAllRolesimplementation are clean and match the documented hierarchy; tests exercise them well.To reduce duplication, you might consider building the
ParseRoleerror’s “must be one of …” list fromallRoles(e.g. by joiningAllRoles()), so adding a new role can’t accidentally leave the error message out of sync. Purely a maintainability nicety.internal/service/auth/service_test.go (1)
17-512: Auth service tests are comprehensive; align helpers with project testing conventionsThis suite does a thorough job validating the auth service end-to-end (hashing, token claims, admin bootstrap, password changes, delete/update/list, and error cases like invalid credentials and weak passwords).
To make it easier to maintain and keep consistent with the rest of the repo:
setupTestServicecould uset.TempDir()instead of manualos.MkdirTemp/RemoveAll, simplifying cleanup.- Many tests repeat the same
if err != nil { t.Fatalf(...) }/if err != want { t.Errorf(...) }patterns; switching tostretchr/testify/require/assertwould compact these and make failures clearer.- If
internal/testalready provides common fixtures (e.g., for temp stores or auth config), consider reusing them instead of rolling your own helper here.Based on learnings,
**/*_test.gofiles are encouraged to userequireand shared fixtures frominternal/test.internal/service/frontend/auth/builtin.go (2)
42-56: Harden bearer parsing and nil-user handling in builtin auth middleware.A couple of small robustness tweaks would help here:
extractBearerTokenonly accepts anAuthorizationheader starting with"Bearer "in a case-sensitive way and without leading spaces. The scheme token is case-insensitive; consider normalizing case and trimming leading spaces before the prefix check to avoid surprising client failures.- After
svc.GetUserFromToken, it would be safer to also treat aniluser as unauthorized (even iferr == nil), to guard against futureTokenValidatorimplementations accidentally returningnil, niland causing a panic when roles are later accessed from context.These don’t change behavior for the current implementation but make the middleware more defensive against future changes.
Also applies to: 123-137
61-80: Consider reusing existing path normalization for public paths.
isPublicPathreimplements trailing-slash handling, while the legacy middleware usespathutil.NormalizePathfor the same goal. To avoid subtle differences between builtin and legacy modes, consider reusing the same normalization helper here (or centralizing the logic) so public path matching behaves identically across auth modes.internal/service/frontend/api/v2/api.go (2)
222-231: Middleware selection logic is sound but assumes consistent public-path handling.The switch between
BuiltinAuthMiddleware(JWT + user context) and the legacyMiddlewarebased onauthConfig.Mode == config.AuthModeBuiltin && a.authService != nilis a reasonable separation of concerns. Just ensure thatauthOptions.PublicPathssemantics match whatBuiltinAuthMiddlewareexpects; it currently uses a simple string set plusisPublicPath, while the legacy middleware normalizes withpathutil, so keeping those in sync avoids surprises when toggling auth modes.
297-367: requireAdmin/requireWrite/requireExecute now return API errors, but callers should respect HTTPStatus.These helpers correctly wrap authorization failures in the API
Errortype with appropriateHTTPStatusvalues so thathandleErrorcan emit 401 vs 403 as intended.Where they are used directly (e.g., in
users.go), callers currently ignoreHTTPStatusand always return a 403 typed response, even whenrequire*failed due to missing authentication. To keep behavior consistent and distinguish 401 from 403, consider inspecting the returned*Error’sHTTPStatus(orCode) in those handlers and mapping to the corresponding 401/403 response variants instead of hard-coding 403.internal/service/frontend/api/v2/users.go (2)
16-39: Differentiate unauthenticated vs forbidden when propagatingrequireAdminfailures.Across
ListUsers,CreateUser,GetUser,UpdateUser, andResetUserPassword, any error froma.requireAdmin(ctx)is always mapped to a403response withErrorCodeForbidden, even when the underlying error represents a missing authentication (401).Since
requireAdminalready distinguishes these cases via itsError.HTTPStatusandCode, consider:
- Detecting when the returned error is an
*ErrorwithHTTPStatus == http.StatusUnauthorized, and in that case returning the...401JSONResponsetype withErrorCodeUnauthorized.- Reserving the
...403JSONResponsefor genuine role/permission failures.This keeps the users API’s status codes aligned with the rest of the v2 API and makes it easier for clients to react appropriately to “not logged in” vs “not allowed”.
Also applies to: 98-188, 235-278
190-233: DeleteUser’s extra 401 path is likely unreachable and can be simplified.After a successful
requireAdmin(ctx)call,auth.UserFromContext(ctx)should always return a user when builtin auth is enabled; theif !ok { ... DeleteUser401JSONResponse ... }branch is effectively dead given current wiring.Not harmful, but you could simplify by assuming the user is present after
requireAdmin(or, if you want to keep the check, treat it as an internal inconsistency and log rather than returning a distinct 401 that clients will probably never see).internal/persistence/fileuser/store.go (1)
52-78: File-based user store is solid; consider validating user IDs against path traversal.The store correctly:
- Uses a mutex-protected in-memory index for fast lookups.
- Performs atomic writes via temp files and
os.Rename.- Keeps indexes consistent in
Create,Update, andDelete(including the “file already missing” case).Given that
userFilePathjoinsbaseDirwithuserID + ".json", it would be slightly safer to also enforce thatuser.IDis a simple filename (e.g.,filepath.Base(user.ID) == user.ID) inCreate(and possiblyUpdate) to guard against any future callers constructingauth.Userwith IDs containing path separators. Today IDs are generated byauth.NewUseras UUIDs, so this is more of a future-proofing hardening than a current bug.Also applies to: 132-173, 265-311, 313-353
internal/service/auth/service.go (1)
368-385: Secure password generator is reasonable; consider documenting length semantics.
generateSecurePasswordusesrand.Readonlengthbytes and then truncates a URL-safe base64 encoding tolengthcharacters. This yields high-entropy passwords with the requested character length.It might be worth adding a short comment clarifying that the
lengthparameter is in characters (not bytes) and that extra encoded characters are discarded, so future refactors don’t accidentally change that contract (e.g., by switching toRawURLEncoding). Functionally this is fine as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
internal/auth/context.go(1 hunks)internal/auth/role.go(1 hunks)internal/auth/role_test.go(1 hunks)internal/auth/store.go(1 hunks)internal/auth/user.go(1 hunks)internal/persistence/fileuser/store.go(1 hunks)internal/persistence/fileuser/store_test.go(1 hunks)internal/service/auth/service.go(1 hunks)internal/service/auth/service_test.go(1 hunks)internal/service/frontend/api/v2/api.go(8 hunks)internal/service/frontend/api/v2/auth.go(1 hunks)internal/service/frontend/api/v2/users.go(1 hunks)internal/service/frontend/auth/builtin.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/auth/context.gointernal/auth/user.gointernal/auth/role.gointernal/persistence/fileuser/store_test.gointernal/service/auth/service_test.gointernal/service/frontend/api/v2/auth.gointernal/auth/store.gointernal/service/frontend/api/v2/api.gointernal/persistence/fileuser/store.gointernal/service/frontend/api/v2/users.gointernal/service/frontend/auth/builtin.gointernal/service/auth/service.gointernal/auth/role_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/persistence/fileuser/store_test.gointernal/service/auth/service_test.gointernal/auth/role_test.go
🧠 Learnings (3)
📚 Learning: 2025-12-04T10:34:17.051Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.051Z
Learning: Applies to **/*_test.go : Co-locate Go tests as `*_test.go`; favour table-driven cases and cover failure paths
Applied to files:
internal/persistence/fileuser/store_test.gointernal/service/auth/service_test.gointernal/auth/role_test.go
📚 Learning: 2025-12-04T10:34:17.051Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.051Z
Learning: Applies to **/*_test.go : Use `stretchr/testify/require` and shared fixtures from `internal/test` instead of duplicating mocks
Applied to files:
internal/persistence/fileuser/store_test.gointernal/service/auth/service_test.gointernal/auth/role_test.go
📚 Learning: 2025-12-04T10:34:17.051Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.051Z
Learning: Applies to **/*.go : Backend entrypoint in `cmd/` orchestrates the scheduler and CLI; runtime, persistence, and service layers sit under `internal/*` (for example `internal/runtime`, `internal/persistence`)
Applied to files:
internal/service/frontend/api/v2/api.go
🧬 Code graph analysis (10)
internal/auth/context.go (1)
internal/auth/user.go (1)
User(13-27)
internal/auth/user.go (1)
internal/auth/role.go (1)
Role(16-16)
internal/persistence/fileuser/store_test.go (4)
internal/persistence/fileuser/store.go (1)
New(52-78)internal/auth/user.go (1)
NewUser(31-41)internal/auth/role.go (4)
RoleManager(22-22)Role(16-16)RoleAdmin(20-20)RoleViewer(26-26)internal/auth/store.go (2)
ErrUserNotFound(14-14)ErrUserAlreadyExists(17-17)
internal/service/frontend/api/v2/auth.go (5)
internal/service/frontend/api/v2/api.go (1)
API(35-51)api/v2/api.gen.go (6)
LoginRequestObject(4846-4848)Login200JSONResponse(4854-4854)User(988-1003)GetCurrentUser401JSONResponse(4900-4900)GetCurrentUser200JSONResponse(4891-4891)ChangePasswordRequestObject(4799-4801)internal/service/auth/service.go (3)
ErrInvalidCredentials(22-22)ErrPasswordMismatch(26-26)ErrWeakPassword(27-27)internal/auth/user.go (1)
User(13-27)internal/auth/context.go (1)
UserFromContext(24-27)
internal/auth/store.go (1)
internal/auth/user.go (1)
User(13-27)
internal/service/frontend/api/v2/api.go (7)
internal/auth/user.go (1)
User(13-27)internal/common/config/config.go (2)
RemoteNode(220-229)AuthModeBuiltin(137-137)internal/service/frontend/auth/middleware.go (2)
Options(15-27)Middleware(40-89)internal/service/frontend/api/pathutil/pathutil.go (1)
BuildPublicEndpointPath(11-32)internal/service/frontend/auth/oidc.go (1)
InitVerifierAndConfig(46-93)internal/service/frontend/auth/builtin.go (1)
BuiltinAuthMiddleware(27-59)internal/auth/context.go (1)
UserFromContext(24-27)
internal/persistence/fileuser/store.go (2)
internal/auth/user.go (2)
User(13-27)UserForStorage(45-52)internal/auth/store.go (4)
ErrInvalidUserID(21-21)ErrInvalidUsername(19-19)ErrUserAlreadyExists(17-17)ErrUserNotFound(14-14)
internal/service/frontend/api/v2/users.go (7)
internal/service/frontend/api/v2/api.go (1)
API(35-51)internal/auth/role.go (2)
ParseRole(71-77)Role(16-16)internal/service/auth/service.go (3)
CreateUserInput(186-190)UpdateUserInput(229-233)ErrCannotDeleteSelf(28-28)internal/auth/store.go (2)
ErrUserAlreadyExists(17-17)ErrUserNotFound(14-14)internal/auth/user.go (1)
User(13-27)internal/auth/context.go (1)
UserFromContext(24-27)internal/common/logger/tag/tag.go (1)
ID(276-278)
internal/service/frontend/auth/builtin.go (4)
internal/auth/user.go (1)
User(13-27)internal/auth/context.go (2)
WithUser(17-19)UserFromContext(24-27)internal/core/execution/context.go (1)
WithContext(241-243)internal/auth/role.go (4)
Role(16-16)RoleAdmin(20-20)RoleManager(22-22)RoleOperator(24-24)
internal/service/auth/service.go (3)
internal/auth/role.go (2)
Role(16-16)RoleAdmin(20-20)internal/auth/store.go (3)
UserStore(26-52)ErrUserNotFound(14-14)ErrInvalidUsername(19-19)internal/auth/user.go (2)
User(13-27)NewUser(31-41)
⏰ 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 (5)
internal/auth/user.go (1)
12-76: User model and storage mapping look correctThe
User/UserForStorageshapes,NewUserconstructor (UUID + UTC timestamps), and theToStorage/ToUserconversions are straightforward and consistent with the intended persistence model. No issues from a correctness or security (password hash exposure) standpoint.internal/auth/store.go (1)
11-52: UserStore interface and error contracts look well-definedThe sentinel errors and
UserStoremethod docs give clear expectations (especially aroundErrUserNotFoundandErrUserAlreadyExists), and the interface surface matches how the file-backed store and auth service tests are using it. No changes needed here.internal/service/frontend/api/v2/auth.go (1)
15-134: Auth v2 handlers correctly wire service errors to API responsesThe Login, GetCurrentUser, and ChangePassword endpoints are wired cleanly into the auth service:
a.authService == nilis handled explicitly, avoiding panics when auth is disabled.- Authentication and password-change errors are mapped to appropriate structured responses (
ErrorCodeUnauthorizedvsErrorCodeBadRequest) while propagating unexpected errors.auth.UserFromContextandtoAPIUserkeep the API model decoupled from the coreauth.User.This looks consistent with the surrounding auth service and context helpers; no functional issues found.
internal/service/frontend/api/v2/api.go (1)
53-76: AuthService abstraction and API options wiring look good.Replacing the concrete
*authservice.Servicedependency with theAuthServiceinterface andWithAuthServiceoption cleanly removes the unsafe type assertion and makes the API construction more flexible. Applyingopts ...APIOptionafter initializing the core fields is also a sensible pattern.Also applies to: 100-128
internal/service/auth/service.go (1)
84-102: Authenticate correctly normalizes “user not found/invalid username” to invalid credentials.Handling both
auth.ErrUserNotFoundandauth.ErrInvalidUsernameby performing a dummy bcrypt comparison and returningErrInvalidCredentialsis a good improvement: it avoids leaking which usernames exist and maintains roughly constant timing across failure cases.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/service/frontend/api/v2/api.go (3)
50-76: AuthService interface and APIOption wiring look good; consider narrowing the interface.The AuthService abstraction and WithAuthService option are idiomatic and make the API nicely pluggable. The only minor concern is that AuthService is quite broad and depends on several authservice‑specific types, which slightly increases coupling and cost of mocking. Over time you may want to split it into smaller interfaces (e.g., token/identity vs. user‑admin operations) so call sites can depend only on what they actually need.
188-231: Auth middleware selection is clear; consider surfacing misconfigured builtin auth.The unified authOptions setup and split between BuiltinAuthMiddleware (JWT) and the legacy Middleware for basic/token/OIDC are clear and easy to follow. One thing to consider: when
authConfig.Mode == config.AuthModeBuiltinbuta.authServiceis nil, the code silently falls back to the legacy middleware. If builtin mode is selected in config but the service wasn’t wired via WithAuthService, this could unintentionally leave the API protected only by whatever legacy options are configured (or potentially with effectively no auth, depending on Middleware behavior). You might want to at least log a warning or even return an error from ConfigureRoutes in that case to make the misconfiguration explicit.
297-367: Permission helpers correctly integrate with API errors; small duplication only.The requireAdmin/requireWrite/requireExecute helpers correctly:
- Treat
authService == nilas “auth disabled”.- Pull the user from context via
auth.UserFromContext.- Return structured *Error values with appropriate 401/403 codes, so handleError will emit the right HTTP status.
Logic looks sound. There’s some copy‑paste between the three helpers (context extraction and 401 path); if this grows further, consider a small internal helper to DRY that up, but it’s not urgent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/service/frontend/api/v2/api.go(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/api.go
🧠 Learnings (1)
📚 Learning: 2025-12-04T10:34:17.051Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.051Z
Learning: Applies to **/*.go : Backend entrypoint in `cmd/` orchestrates the scheduler and CLI; runtime, persistence, and service layers sit under `internal/*` (for example `internal/runtime`, `internal/persistence`)
Applied to files:
internal/service/frontend/api/v2/api.go
⏰ 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 (2)
internal/service/frontend/api/v2/api.go (2)
79-129: New(...) constructor uses the functional‑options pattern correctly.Remote nodes, base paths, and other core fields are initialized up front, and APIOption functions are applied afterwards, which keeps construction order clear and thread‑safe. No functional issues spotted here.
369-375: ptrOf doc comment matches behavior.The updated comment now accurately documents the zero‑value → nil behavior, which is helpful for callers relying on this helper.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
deploy/docker/compose.minimal.yaml (1)
53-58: User experience improvement: clarify that both auth methods cannot be active simultaneously.The comment structure is helpful, but it might be clearer to explicitly state that only one authentication method should be enabled at a time. Currently, it could be ambiguous whether having both DAGU_AUTH_MODE and DAGU_AUTH_BASIC_* variables set simultaneously would cause conflicts.
Consider enhancing the comment to be more explicit:
- # Alternative: basic auth (simpler, no user management) - # To use basic auth instead of builtin auth, comment out - # DAGU_AUTH_MODE and DAGU_AUTH_TOKEN_SECRET above, then uncomment: + # Alternative: basic auth (simpler, no user management) + # To use basic auth instead of builtin auth: + # 1. Comment out DAGU_AUTH_MODE and DAGU_AUTH_TOKEN_SECRET above + # 2. Uncomment the following (only one auth method can be active):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(5 hunks)deploy/docker/compose.minimal.yaml(2 hunks)deploy/docker/compose.prod.yaml(1 hunks)internal/auth/context.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/auth/context.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-04T10:34:01.023Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: ui/CLAUDE.md:0-0
Timestamp: 2025-12-04T10:34:01.023Z
Learning: Applies to ui/~/.config/dagu/config.yaml : Dagu configuration file supports environment variables with DAGU_* prefix, per-DAG overrides, and command-line arguments
Applied to files:
deploy/docker/compose.minimal.yamlREADME.mddeploy/docker/compose.prod.yaml
📚 Learning: 2025-12-04T10:34:01.023Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: ui/CLAUDE.md:0-0
Timestamp: 2025-12-04T10:34:01.023Z
Learning: Applies to ui/**/*.{yaml,yml} : Dagu YAML configurations should support minimal setup with sensible defaults and support for environment variable expansion and command substitution
Applied to files:
deploy/docker/compose.minimal.yamlREADME.md
🪛 LanguageTool
README.md
[grammar] ~265-~265: Ensure spelling is correct
Context: ...mail whitelist (comma-separated) | ### Builtin Authentication (RBAC) When `DAGU_AUTH_...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ 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)
deploy/docker/compose.prod.yaml (1)
85-91: Verify runtime validation ensures token secret is not left as placeholder in production.The token secret placeholder is intentionally generic to prompt users to change it, but there's no explicit indication of whether runtime validation prevents production deployments with this default value. This could pose a security risk if a user mistakenly deploys without changing the secret.
Does the application validate at startup that
DAGU_AUTH_TOKEN_SECREThas been changed from the placeholder value in production environments, or should a warning/error be added?README.md (3)
47-47: ✓ Good addition to production-ready statement.Mentioning "Built-in user management with role-based access control (RBAC)" in the production-ready section appropriately highlights a key governance feature for enterprise/team deployments.
255-275: ✓ Comprehensive authentication and RBAC documentation.The environment variables section is well-structured, documenting all three authentication modes (none, builtin, oidc) and the complete set of builtin RBAC configuration options. Defaults are clearly stated, and the roles (admin, manager, operator, viewer) are properly documented.
420-422: ✓ Features table properly updated for new authentication and RBAC capabilities.The three new rows (Authentication with JWT support, RBAC with role definitions, User management) appropriately document the new builtin RBAC functionality without overreaching. Distinguishing between builtin RBAC (current) and fine-grained permissions (future enterprise feature) on line 475 is good planning.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
internal/service/frontend/api/v2/users.go (2)
174-189: MissingErrInvalidUsernamemapping in UpdateUser error handling.When updating a username, the store may return
auth.ErrInvalidUsernameif the new username is invalid. Currently this would bubble up as a 500. Consider adding:if errors.Is(err, auth.ErrUserAlreadyExists) { return api.UpdateUser409JSONResponse{ Code: api.ErrorCodeAlreadyExists, Message: "Username already exists", }, nil } + if errors.Is(err, auth.ErrInvalidUsername) { + return api.UpdateUser400JSONResponse{ + Code: api.ErrorCodeBadRequest, + Message: "Invalid username", + }, nil + } return nil, err
212-219: Redundant context user retrieval afterrequireAdminsucceeds.If
requireAdminsucceeds (line 205), the user is already confirmed to be in the context with admin role. The secondUserFromContextcall at line 213 will always succeed at this point.This is defensive but could be simplified by having
requireAdminreturn the user, or by extracting the user once and reusing it. Low priority given it's a minor inefficiency.internal/service/auth/service.go (3)
372-378: Consider documenting password policy or enhancing validation.The current validation only enforces minimum length (8 characters). For a production user management system, consider:
- Documenting that this is the intentional policy
- Or adding complexity requirements (uppercase, digit, special character)
This is acceptable as-is if the policy is intentional.
380-388: Potential panic iflengthexceeds base64 output length.The function reads
lengthrandom bytes but base64 encoding produces approximately4*length/3characters. Slicing to[:length]works whenlength=16(produces ~22 base64 chars), but if called with a largerlengthrelative to the byte array, it could panic on slice bounds.Since this is only called internally with
length=16, this is low risk. Consider adding a guard or documenting the constraint:func generateSecurePassword(length int) (string, error) { - bytes := make([]byte, length) + // Generate enough random bytes to produce at least 'length' base64 characters + numBytes := (length*3 + 3) / 4 // ceiling division to ensure enough chars + bytes := make([]byte, numBytes) if _, err := rand.Read(bytes); err != nil { return "", err } return base64.URLEncoding.EncodeToString(bytes)[:length], nil }
339-370: Minor race condition during admin bootstrap.If multiple processes start simultaneously and call
EnsureAdminUser, there's a TOCTOU window betweenCount()andCreateUser(). The second process would fail with "failed to create admin user: user already exists".For a bootstrap operation that typically runs once at startup, this is acceptable. If needed, the error could be handled more gracefully:
if errors.Is(err, auth.ErrUserAlreadyExists) { // Another process already created the admin return "", false, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/service/auth/service.go(1 hunks)internal/service/frontend/api/v2/api.go(8 hunks)internal/service/frontend/api/v2/users.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/users.gointernal/service/frontend/api/v2/api.gointernal/service/auth/service.go
🧠 Learnings (1)
📚 Learning: 2025-12-04T10:34:17.051Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.051Z
Learning: Applies to **/*.go : Backend entrypoint in `cmd/` orchestrates the scheduler and CLI; runtime, persistence, and service layers sit under `internal/*` (for example `internal/runtime`, `internal/persistence`)
Applied to files:
internal/service/frontend/api/v2/api.go
🧬 Code graph analysis (2)
internal/service/frontend/api/v2/users.go (4)
internal/auth/role.go (2)
ParseRole(71-77)Role(16-16)internal/service/auth/service.go (3)
CreateUserInput(190-194)ErrWeakPassword(27-27)UpdateUserInput(233-237)internal/auth/store.go (2)
ErrUserAlreadyExists(17-17)ErrInvalidUsername(19-19)internal/auth/context.go (1)
UserFromContext(23-26)
internal/service/auth/service.go (3)
internal/auth/role.go (2)
Role(16-16)RoleAdmin(20-20)internal/auth/store.go (4)
UserStore(26-52)ErrUserNotFound(14-14)ErrInvalidUsername(19-19)ErrInvalidUserID(21-21)internal/auth/user.go (2)
User(13-27)NewUser(31-41)
⏰ 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: Build
- GitHub Check: Test on ubuntu-latest
🔇 Additional comments (7)
internal/service/frontend/api/v2/api.go (3)
50-77: Well-structured AuthService interface and functional options pattern.The
AuthServiceinterface cleanly abstracts authentication operations, enabling testability and decoupling from the concrete implementation. TheAPIOptionfunctional pattern is idiomatic Go for optional configuration.
222-245: Clean auth middleware selection based on configuration.The branching logic correctly applies JWT-based
BuiltinAuthMiddlewarefor builtin auth mode while falling back to legacy middleware for basic/token/OIDC modes. The explicit nil check with error return prevents runtime panics.
306-376: Permission helpers correctly return typed API errors.The previous review concern about plain
errors.New()returning 500s has been addressed. All three methods now return&Error{}with appropriate HTTP status codes (401 for unauthenticated, 403 for insufficient permissions).internal/service/frontend/api/v2/users.go (1)
72-96: Comprehensive error mapping for CreateUser.Previous review concern about
auth.ErrInvalidUsernamebubbling as 500 is addressed. The error handling now correctly maps:
ErrUserAlreadyExists→ 409ErrInvalidUsername→ 400ErrWeakPassword→ 400internal/service/auth/service.go (3)
84-102: Secure authentication with timing-attack mitigation.The previous review concern is addressed. Both
ErrUserNotFoundandErrInvalidUsernamenow flow through the timing-safe path withdummyHashcomparison before returningErrInvalidCredentials, preventing user enumeration attacks.
170-187: Properly handles deleted users with valid tokens.The previous review concern is addressed. When a user is deleted after a token was issued,
GetUserFromTokennow returnsErrInvalidTokeninstead of bubbling up as a 500. This provides correct client-facing behavior.
229-237: Good documentation clarifying Password field usage.The previous review concern about the unused
Passwordfield inUpdateUserInputis addressed. The comment clearly explains that the field is supported by the service layer but intentionally omitted by the API handler, with password changes routed through dedicated endpoints.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/service/auth/service.go (1)
376-382: Consider adding character-class checks for production password strength.The current validation enforces only a length minimum, which meets the documented "minimum requirements." For production deployments, consider augmenting with checks for uppercase, lowercase, digits, and special characters, or integrating a library that detects common/breached passwords.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/service/auth/service.go(1 hunks)internal/service/frontend/api/v2/api.go(8 hunks)internal/service/frontend/api/v2/users.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/users.gointernal/service/frontend/api/v2/api.gointernal/service/auth/service.go
🧠 Learnings (1)
📚 Learning: 2025-12-04T10:34:17.051Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.051Z
Learning: Applies to **/*.go : Backend entrypoint in `cmd/` orchestrates the scheduler and CLI; runtime, persistence, and service layers sit under `internal/*` (for example `internal/runtime`, `internal/persistence`)
Applied to files:
internal/service/frontend/api/v2/api.go
🧬 Code graph analysis (3)
internal/service/frontend/api/v2/users.go (6)
internal/service/frontend/api/v2/api.go (1)
API(35-51)api/v2/api.gen.go (8)
ListUsersRequestObject(6430-6431)Error(462-471)User(988-1003)GetUserRequestObject(6596-6598)GetUserResponseObject(6600-6602)UserId(1092-1092)ResetUserPasswordRequestObject(6727-6730)ResetUserPasswordResponseObject(6732-6734)internal/auth/role.go (2)
ParseRole(71-77)Role(16-16)internal/service/auth/service.go (3)
CreateUserInput(190-194)UpdateUserInput(233-237)ErrCannotDeleteSelf(28-28)internal/auth/store.go (3)
ErrUserAlreadyExists(17-17)ErrInvalidUsername(19-19)ErrUserNotFound(14-14)internal/auth/context.go (1)
UserFromContext(23-26)
internal/service/frontend/api/v2/api.go (8)
internal/auth/user.go (1)
User(13-27)internal/common/config/config.go (2)
RemoteNode(220-229)AuthModeBuiltin(137-137)internal/service/frontend/auth/middleware.go (2)
Options(15-27)Middleware(40-89)internal/service/frontend/api/pathutil/pathutil.go (1)
BuildPublicEndpointPath(11-32)internal/service/frontend/auth/oidc.go (1)
InitVerifierAndConfig(46-93)internal/service/frontend/auth/builtin.go (1)
BuiltinAuthMiddleware(27-59)internal/service/frontend/api/v2/remote.go (1)
WithRemoteNode(23-134)internal/auth/context.go (1)
UserFromContext(23-26)
internal/service/auth/service.go (3)
internal/auth/role.go (2)
Role(16-16)RoleAdmin(20-20)internal/auth/store.go (5)
UserStore(26-52)ErrUserNotFound(14-14)ErrInvalidUsername(19-19)ErrInvalidUserID(21-21)ErrUserAlreadyExists(17-17)internal/auth/user.go (2)
User(13-27)NewUser(31-41)
⏰ 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 (8)
internal/service/auth/service.go (3)
84-102: Past issue correctly resolved—invalid usernames now map to credential errors.The handling of
auth.ErrInvalidUsernamealongsideauth.ErrUserNotFound(lines 88-92) ensures both cases returnErrInvalidCredentialswhile preserving timing-attack mitigation via the dummy bcrypt comparison. This prevents malformed usernames from surfacing as 500s.
170-187: Past issue correctly resolved—deleted users treated as invalid tokens.Lines 180-181 now map both
auth.ErrUserNotFoundandauth.ErrInvalidUserIDtoErrInvalidToken, ensuring that tokens referencing deleted or invalid users flow through the same "cannot use this token" path rather than surfacing as 500s.
339-374: Solid admin bootstrap logic with race-condition handling.The implementation correctly checks the user count before creating the admin, and handles concurrent admin creation gracefully by catching
auth.ErrUserAlreadyExists(line 367). Password generation and user creation flow is clean and secure.internal/service/frontend/api/v2/users.go (2)
35-94: Comprehensive error handling with proper status codes.The
CreateUserhandler now correctly mapsauth.ErrInvalidUsernameto a 400 response (lines 74-80), addressing the past issue. All validation errors (ErrUserAlreadyExists,ErrWeakPassword,ErrInvalidUsername) are properly translated to appropriate HTTP status codes instead of 500s.
186-225: Self-deletion safeguard properly implemented.The handler correctly retrieves the current user from context (lines 196-203) and passes their ID to
DeleteUser(line 205) to enforce the self-deletion check. TheErrCannotDeleteSelfsentinel is appropriately mapped to a 403 Forbidden response (lines 214-220).internal/service/frontend/api/v2/api.go (3)
53-77: Clean abstraction and idiomatic options pattern.The
AuthServiceinterface (lines 55-66) provides a well-defined contract for authentication operations, enabling the API to work with or without auth configured. The functional options pattern (lines 68-77) is idiomatic and allows clean dependency injection.
222-245: Auth middleware routing correctly uses interface, no unsafe assertion.The builtin auth path (lines 223-235) now passes
a.authServicedirectly toBuiltinAuthMiddleware(line 230) without type assertion, addressing the past safety concern. The explicit nil-check with a clear error message (lines 224-226) catches misconfigurations early.
306-388: Permission helpers return properly structured errors.The
requireAdmin,requireWrite,requireExecute, andrequireUserManagementmethods now return*Errortypes with explicitHTTPStatusfields, ensuring authentication failures produce 401 and authorization failures produce 403 instead of 500s. This addresses the past issue and provides clear, consistent error handling.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1463 +/- ##
==========================================
- Coverage 59.95% 59.93% -0.03%
==========================================
Files 191 196 +5
Lines 21473 21896 +423
==========================================
+ Hits 12875 13123 +248
- Misses 7249 7366 +117
- Partials 1349 1407 +58
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.