fix: polishing auto signup via oidc functionality#1569
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds user enable/disable support across API, service, and UI; restructures OIDC config to use a nested role-mapping and an IsConfigured() check; changes OIDC callback redirect to return token via URL; adds provider init retry and refines email allowlist logic; propagates remoteNode query param across frontend user endpoints. Changes
Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
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/users.go (1)
119-184: Consider forbidding self-disable (mirrors “cannot delete yourself”).
Right now an admin can PATCH themselves toisDisabled=trueand lock themselves out (especially problematic if they’re the only admin). You already do a self-protection check for Delete; worth doing the same for disable.Proposed fix (block self-disable in UpdateUser)
func (a *API) UpdateUser(ctx context.Context, request api.UpdateUserRequestObject) (api.UpdateUserResponseObject, error) { if err := a.requireUserManagement(); err != nil { return nil, err } if err := a.requireAdmin(ctx); err != nil { return nil, err } + // Prevent self-disable (consistent with DeleteUser self-protection). + if currentUser, ok := auth.UserFromContext(ctx); ok && request.Body != nil && request.Body.IsDisabled != nil { + if currentUser.ID == request.UserId && *request.Body.IsDisabled { + return nil, &Error{ + Code: api.ErrorCodeForbidden, + Message: "Cannot disable your own account", + HTTPStatus: http.StatusForbidden, + } + } + } + if request.Body == nil { return nil, &Error{ Code: api.ErrorCodeBadRequest, Message: "Invalid request body", HTTPStatus: http.StatusBadRequest, } }
🤖 Fix all issues with AI agents
In @internal/service/frontend/auth/oidc.go:
- Around line 504-514: The code currently appends the JWT to redirectURL and
sends it as a query param; instead set the JWT as a secure, HttpOnly cookie and
redirect without the token in the URL. In the handler that builds redirectURL
(variable redirectURL and the http.Redirect call), create and set an http.Cookie
(e.g. Name "auth_token") with Value tokenResult.Token, Secure: true, HttpOnly:
true, SameSite: http.SameSiteLaxMode (or Strict if appropriate), Path "/", and a
sensible Expires/MaxAge, then construct redirectURL without "?token=..." (only
include "&welcome=true" when isNewUser) and call http.Redirect(w, r,
redirectURL, http.StatusFound). Ensure you remove the token-related query
construction and replace it with the cookie set via http.SetCookie before
redirecting.
In @ui/src/pages/users/ResetPasswordModal.tsx:
- Around line 1-3: The fetch URL builder in ResetPasswordModal.tsx currently
interpolates remoteNode (and uses token) directly; update calls that construct
the fetch endpoint (the places around lines where remoteNode is used, e.g., the
initial GET/POST fetches and the reset submission block) to URL-encode
remoteNode with encodeURIComponent(remoteNode) and add a guard that token exists
before building the URL (return early or show an error if token is missing).
Ensure all occurrences (the spots near lines 38-39 and 76-85) use the encoded
value and perform a null/empty check on token before calling fetch.
In @ui/src/pages/users/UserFormModal.tsx:
- Around line 1-3: The fetch URL construction in UserFormModal.tsx uses raw
remoteNode (and token) values directly; URL-encode remoteNode (e.g., via
encodeURIComponent(remoteNode)) before interpolating into any fetch URL and
guard for a missing token (throw or early-return if token is falsy) so you never
build requests with undefined credentials; update all occurrences mentioned
(around lines 50-51, 85-96, 104-112) where fetch is called to use the
encodedRemoteNode and to check token presence before creating the URL or
headers.
🧹 Nitpick comments (4)
internal/service/frontend/server.go (1)
73-130: Good move toIsConfigured(), but validateRoleMapping.DefaultRolebefore casting.
If config parsing doesn’t already enforce valid roles,authmodel.Role(oidcCfg.RoleMapping.DefaultRole)can silently accept garbage and fail later in provisioning. Consider validating once (e.g., parse/valid check) and returning a clear startup error.internal/service/frontend/auth/oidc.go (1)
517-523: Build the error redirect URL viaurl.URL/url.Values(avoid subtle path/query edge cases).
Current concat is mostly fine, but this keeps the code consistent with token/welcome handling above.Proposed refactor
- redirectURL := strings.TrimSuffix(basePath, "/") + "/login?error=" + url.QueryEscape(errMsg) - http.Redirect(w, r, redirectURL, http.StatusFound) + loginPath := strings.TrimSuffix(basePath, "/") + "/login" + u := &url.URL{Path: loginPath} + q := u.Query() + q.Set("error", errMsg) + u.RawQuery = q.Encode() + http.Redirect(w, r, u.String(), http.StatusFound)internal/common/config/loader.go (1)
1250-1277: Consider trimming whitespace for[]interface{}case.The
stringcase trims whitespace from each element, but the[]interface{}case does not. This could lead to inconsistent behavior when the same config is provided via different formats.♻️ Proposed fix for consistency
case []interface{}: for _, item := range v { - if s, ok := item.(string); ok && s != "" { - result = append(result, s) + if s, ok := item.(string); ok { + if trimmed := strings.TrimSpace(s); trimmed != "" { + result = append(result, trimmed) + } } }ui/src/pages/users/index.tsx (1)
231-245: Add ARIA label for accessibility.The enable/disable button correctly prevents self-disabling and shows appropriate icons/labels. Consider adding an ARIA label to improve screen reader support.
♻️ Suggested accessibility improvement
{isAdmin && user.id !== currentUser?.id && ( - <DropdownMenuItem onClick={() => handleToggleDisabled(user)}> + <DropdownMenuItem + onClick={() => handleToggleDisabled(user)} + aria-label={user.isDisabled ? `Enable user ${user.username}` : `Disable user ${user.username}`} + > {user.isDisabled ? (
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
api/v2/api.gen.goapi/v2/api.yamlinternal/common/config/config.gointernal/common/config/config_test.gointernal/common/config/definition.gointernal/common/config/loader.gointernal/common/config/loader_test.gointernal/service/auth/service.gointernal/service/frontend/api/v2/users.gointernal/service/frontend/auth/oidc.gointernal/service/frontend/server.gointernal/service/oidcprovision/service.gointernal/service/oidcprovision/service_test.goui/src/api/v2/schema.tsui/src/pages/login.tsxui/src/pages/users/ResetPasswordModal.tsxui/src/pages/users/UserFormModal.tsxui/src/pages/users/index.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.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/common/config/definition.gointernal/service/oidcprovision/service_test.gointernal/service/frontend/auth/oidc.goapi/v2/api.gen.gointernal/service/frontend/server.gointernal/common/config/loader.gointernal/service/oidcprovision/service.gointernal/common/config/loader_test.gointernal/service/frontend/api/v2/users.gointernal/common/config/config_test.gointernal/service/auth/service.gointernal/common/config/config.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Co-locate Go tests as*_test.go; favour table-driven cases and cover failure paths
Usestretchr/testify/requireand shared fixtures frominternal/testinstead of duplicating mocks
Files:
internal/service/oidcprovision/service_test.gointernal/common/config/loader_test.gointernal/common/config/config_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/api/v2/schema.tsui/src/pages/users/ResetPasswordModal.tsxui/src/pages/users/UserFormModal.tsxui/src/pages/users/index.tsxui/src/pages/login.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/api/v2/schema.tsui/src/pages/users/ResetPasswordModal.tsxui/src/pages/users/UserFormModal.tsxui/src/pages/users/index.tsxui/src/pages/login.tsx
{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
🧠 Learnings (3)
📚 Learning: 2025-12-04T10:34:17.062Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.062Z
Learning: Applies to 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/UserFormModal.tsx
📚 Learning: 2025-12-04T10:34:01.045Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: ui/CLAUDE.md:0-0
Timestamp: 2025-12-04T10:34:01.045Z
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/index.tsx
📚 Learning: 2025-12-04T10:34:17.062Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.062Z
Learning: Applies to **/*_test.go : Use `stretchr/testify/require` and shared fixtures from `internal/test` instead of duplicating mocks
Applied to files:
internal/common/config/config_test.go
🧬 Code graph analysis (11)
ui/src/api/v2/schema.ts (1)
api/v2/api.gen.go (1)
UserAuthProvider(1210-1210)
ui/src/pages/users/ResetPasswordModal.tsx (1)
ui/src/contexts/AppBarContext.ts (1)
AppBarContext(11-21)
ui/src/pages/users/UserFormModal.tsx (1)
ui/src/contexts/AppBarContext.ts (1)
AppBarContext(11-21)
ui/src/pages/users/index.tsx (3)
ui/src/contexts/AuthContext.tsx (1)
TOKEN_KEY(24-24)ui/src/components/ui/table.tsx (5)
TableHead(131-131)TableRow(133-133)TableHeader(132-132)TableBody(127-127)TableCell(129-129)ui/src/components/ui/dropdown-menu.tsx (1)
DropdownMenuItem(185-185)
internal/service/frontend/server.go (2)
internal/service/oidcprovision/service.go (1)
Config(29-44)internal/auth/role.go (1)
Role(13-13)
internal/common/config/loader.go (1)
internal/common/config/config.go (2)
Server(87-128)Auth(161-167)
internal/service/oidcprovision/service.go (2)
internal/service/auth/service.go (1)
Service(78-83)internal/common/stringutil/stringutil.go (1)
ExtractEmailDomain(390-396)
internal/common/config/loader_test.go (1)
internal/common/config/config.go (1)
OIDCRoleMapping(176-189)
ui/src/pages/login.tsx (1)
ui/src/contexts/AuthContext.tsx (1)
TOKEN_KEY(24-24)
internal/common/config/config_test.go (1)
internal/common/config/config.go (2)
OIDCRoleMapping(176-189)Auth(161-167)
internal/service/auth/service.go (1)
internal/auth/role.go (1)
Role(13-13)
⏰ 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 (29)
internal/service/oidcprovision/service.go (1)
243-276: LGTM! Clear refactor that improves readability.The introduction of
hasWhitelistandhasAllowedDomainsguard variables makes the access control logic more explicit and easier to follow. The logic correctly implements an OR relationship where either the whitelist or allowed domains can independently permit access, with explicit denial when restrictions exist but the email matches neither.internal/service/oidcprovision/service_test.go (1)
483-494: LGTM! Good test coverage for whitelist-only scenarios.These test cases properly validate the behavior when only a whitelist is configured (no allowed domains), covering both the allow and deny paths. The tests align with the refactored logic in the service code.
ui/src/pages/login.tsx (2)
3-3: Nice reuse ofTOKEN_KEYinstead of a duplicate constant.
84-84: Background tweak looks fine and keeps dark-mode compatibility.api/v2/api.yaml (1)
4355-4369:UpdateUserRequest.isDisabledaddition is consistent with PATCH semantics.
Minor: consider adjusting the description to explicitly say “If omitted, the disabled status is not changed.”internal/service/auth/service.go (1)
286-291:IsDisabledplumbing in service is clean; consider a policy guard upstream.
This correctly applies the flag and existing auth checks already block disabled users. The remaining concern is policy (self-disable / last active admin) which likely belongs in the API layer (or a dedicated policy check).Also applies to: 322-325
ui/src/api/v2/schema.ts (2)
1910-1916: Schema additions forisDisabledandauthProviderare correctly sourced from the OpenAPI definition.Verified that
api/v2/api.yamlcontains bothUpdateUserRequest.isDisabledandUser.authProvider(enum: builtin/oidc) andUser.isDisableddefinitions, which are properly reflected in the auto-generated schema.ts file. The file header confirms it is auto-generated by openapi-typescript and should not be manually edited.
5583-5586:UserAuthProviderenum is correctly configured with consistent string values across all layers.The enum values (
builtin,oidc) match in the OpenAPI spec (api/v2/api.yaml), backend constants (api/v2/api.gen.go), and the auto-generated frontend schema (ui/src/api/v2/schema.ts). No action required.api/v2/api.gen.go (2)
1175-1183:IsDisabled *boolis the right PATCH shape; ensure semantics are documented/handled consistently.Using
*boolallows “no change” vs “explicitly enable/disable”. Just ensure callers understand: omit = unchanged,true= disable,false= enable.
11484-11622: GeneratedswaggerSpecblob regenerated correctly with pinned version v2.5.0.Verification confirms api/v2/api.gen.go was regenerated using the pinned oapi-codegen v2.5.0. Both generated files (api/v1/api.gen.go and api/v2/api.gen.go) are in sync with the same version marker and show no staleness.
internal/common/config/loader.go (4)
437-447: LGTM! Clean parsing for flexible config formats.The use of
parseStringListenables both YAML lists and comma-separated environment variable strings, which improves configuration flexibility. The AutoSignup defaulting totruealigns with the expected OIDC user experience.
450-462: LGTM! Role mapping configuration properly loaded.The RoleMapping configuration is cleanly loaded from the nested config structure, maintaining consistency with the new model where DefaultRole is part of RoleMapping.
511-513: LGTM! Sensible default for OIDC role.Defaulting
RoleMapping.DefaultRoleto"viewer"follows the principle of least privilege for auto-provisioned OIDC users.
1125-1125: LGTM! Environment binding properly nested.The environment binding
AUTH_OIDC_DEFAULT_ROLEcorrectly maps to the nestedauth.oidc.roleMapping.defaultRoleconfig key.internal/common/config/definition.go (2)
184-185: LGTM! DefaultRole field properly added to role mapping definition.The field is correctly placed within
OIDCRoleMappingDefwith appropriate mapstructure tag, aligning with the PR's goal of moving default role handling into the RoleMapping structure.
228-230: LGTM! Documentation updated to reflect new behavior.The docstrings correctly document that OIDC is automatically enabled when required fields are configured, and that AutoSignup defaults to true.
Also applies to: 240-240
ui/src/pages/users/index.tsx (4)
56-77: LGTM! Remote node support properly integrated.The
fetchUsersfunction correctly includes theremoteNodequery parameter with a sensible default of'local', and the dependency array properly includesappBarContext.selectedRemoteNodeto trigger re-fetches when the selection changes.
109-132: LGTM! Toggle disabled implementation is clean.The
handleToggleDisabledfunction properly:
- Uses PATCH method for partial updates
- Toggles
isDisabledbased on current state- Handles errors gracefully
- Refreshes the user list on success
159-166: LGTM! Table header properly expanded.New columns for Auth, Status, Created, and Updated are appropriately sized and maintain consistency with the existing table design.
183-206: LGTM! User status and auth provider display is well implemented.The implementation follows the coding guidelines:
- Uses appropriate opacity (
opacity-60) for disabled users- Applies proper dark mode support with
text-red-600 dark:text-red-400- Maintains compact table design with
text-smsizinginternal/common/config/loader_test.go (2)
177-180: LGTM! Test expectations correctly updated.The test expectations properly reflect the new OIDC configuration structure:
AutoSignup: trueas the defaultRoleMappingwithDefaultRole: "viewer"This aligns with the implementation changes in loader.go.
397-400: LGTM! YAML loading test updated consistently.The YAML configuration test expectations mirror the environment variable test, ensuring consistent behavior across configuration sources.
internal/common/config/config_test.go (3)
384-417: LGTM! Test correctly validates incomplete OIDC handling.The test properly verifies that:
- Incomplete OIDC configuration (missing
clientId) does not cause validation errorsIsConfigured()returnsfalsefor incomplete configurations- The error message clarifies the expected behavior
This aligns with the design where OIDC is opt-in based on field presence rather than an explicit flag.
419-449: LGTM! Role validation test uses new structure.The test correctly uses
RoleMapping.DefaultRoleand validates that invalid roles are rejected with an appropriate error message.
517-550: LGTM! Comprehensive role validation coverage.The test validates all valid roles (
admin,manager,operator,viewer) using the newRoleMapping.DefaultRolestructure.internal/common/config/config.go (4)
177-178: LGTM! DefaultRole properly moved into OIDCRoleMapping.Moving
DefaultRoleinto theOIDCRoleMappingstruct provides a cleaner separation of concerns - role-related configuration is now consolidated in one place.
214-239: LGTM! Clean IsConfigured() implementation.The
IsConfigured()method provides a clear, single source of truth for determining if OIDC is properly configured. The check for all four required fields (ClientId,ClientSecret,ClientUrl,Issuer) ensures OIDC won't be enabled with partial configuration.This is a good replacement for an explicit
Enabledboolean flag as it derives the enabled state from the presence of required configuration.
416-421: LGTM! Validation gate uses IsConfigured().Using
IsConfigured()to gate OIDC validation ensures that incomplete configurations are silently ignored (OIDC just won't be available) rather than causing validation errors.
451-453: LGTM! Error message references correct config path.The error message correctly references
roleMapping.defaultRolewhich matches the YAML configuration path, making it easy for users to locate and fix the issue.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @internal/common/config/loader.go:
- Around line 1250-1279: The []string branch in parseStringList currently
assigns the slice directly, bypassing trimming/filtering; update the case for
[]string to iterate over each element, apply strings.TrimSpace, and append only
non-empty results (same behavior as the string and []interface{} cases) so empty
or whitespace-only entries are filtered consistently; keep the function
signature and other branches unchanged and reuse the same trimming/append logic.
In @internal/service/frontend/auth/oidc.go:
- Around line 521-530: Update the misleading backend comment above the
redirectURL construction: explain that the frontend (login flow) extracts the
tokenResult.Token from the URL, stores it in localStorage, and then navigates
away using React Router's navigate(..., { replace: true }) which prevents the
token URL from entering browser history; reference redirectURL and
tokenResult.Token so maintainers know this is a one-time redirect and the
frontend (not history.replaceState) handles clearing the token from history.
🧹 Nitpick comments (3)
internal/service/frontend/auth/oidc.go (1)
86-101: Consider using contextual logger for consistency.The retry loop uses
slog.Warndirectly, but the codebase pattern (shown in relevant snippets) useslogger.Warn(ctx, ...)for structured logging with context. Consider updating for consistency:♻️ Proposed refactor
if attempt < oidcProviderMaxRetries { - slog.Warn("OIDC provider init failed, retrying", + logger.Warn(ctx, "OIDC provider init failed, retrying", tag.Error(err), slog.Int("attempt", attempt), slog.Int("maxRetries", oidcProviderMaxRetries)) time.Sleep(oidcProviderRetryDelay) }Based on coding guidelines and relevant code snippets showing logger.Warn usage patterns.
ui/src/pages/users/index.tsx (2)
109-132: Consider optimistic updates for better UX.The current implementation waits for the API response before updating the UI state. While this is safer, it could provide a snappier user experience with optimistic updates:
💡 Potential UX enhancement
const handleToggleDisabled = async (user: User) => { // Optimistically update UI setUsers(prevUsers => prevUsers.map(u => u.id === user.id ? { ...u, isDisabled: !u.isDisabled } : u ) ); try { const token = localStorage.getItem(TOKEN_KEY); const remoteNode = encodeURIComponent(appBarContext.selectedRemoteNode || 'local'); const response = await fetch(`${config.apiURL}/users/${user.id}?remoteNode=${remoteNode}`, { method: 'PATCH', headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${token}`, }, body: JSON.stringify({ isDisabled: !user.isDisabled }), }); if (!response.ok) { // Revert on error fetchUsers(); const data = await response.json().catch(() => ({})); throw new Error(data.message || 'Failed to update user'); } setError(null); } catch (err) { setError(err instanceof Error ? err.message : 'Failed to update user'); } };Based on coding guidelines emphasizing developer-centric UI with high information density.
231-245: Consider adding confirmation for disable action.The disable/enable toggle executes immediately without confirmation. While less destructive than deletion, disabling a user (especially by accident) could disrupt their workflow. Consider adding a confirmation modal similar to the delete action.
🛡️ Suggested confirmation flow
Add state for a confirmation modal:
const [togglingUser, setTogglingUser] = useState<User | null>(null);Update the dropdown item:
<DropdownMenuItem onClick={() => setTogglingUser(user)}> {user.isDisabled ? ( <> <UserCheck className="h-4 w-4 mr-2" /> Enable </> ) : ( <> <Ban className="h-4 w-4 mr-2" /> Disable </> )} </DropdownMenuItem>Add a confirmation modal similar to the delete modal pattern.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
internal/common/config/config.gointernal/common/config/config_test.gointernal/common/config/loader.gointernal/service/frontend/api/v2/users.gointernal/service/frontend/auth/oidc.goui/src/pages/login.tsxui/src/pages/users/ResetPasswordModal.tsxui/src/pages/users/UserFormModal.tsxui/src/pages/users/index.tsx
🧰 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/service/frontend/auth/oidc.gointernal/common/config/config.gointernal/service/frontend/api/v2/users.gointernal/common/config/loader.gointernal/common/config/config_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/pages/login.tsxui/src/pages/users/index.tsxui/src/pages/users/UserFormModal.tsxui/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/login.tsxui/src/pages/users/index.tsxui/src/pages/users/UserFormModal.tsxui/src/pages/users/ResetPasswordModal.tsx
**/*_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/common/config/config_test.go
🧠 Learnings (2)
📚 Learning: 2025-12-04T10:34:01.045Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: ui/CLAUDE.md:0-0
Timestamp: 2025-12-04T10:34:01.045Z
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/index.tsxui/src/pages/users/UserFormModal.tsxui/src/pages/users/ResetPasswordModal.tsx
📚 Learning: 2025-12-04T10:34:17.062Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.062Z
Learning: Applies to 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/UserFormModal.tsx
🧬 Code graph analysis (8)
internal/service/frontend/auth/oidc.go (2)
internal/common/logger/context.go (2)
Warn(45-47)Error(50-52)internal/common/logger/tag/tag.go (2)
Error(20-22)Token(386-388)
ui/src/pages/login.tsx (1)
ui/src/contexts/AuthContext.tsx (1)
TOKEN_KEY(24-24)
ui/src/pages/users/index.tsx (5)
internal/auth/user.go (1)
User(10-35)api/v2/api.gen.go (1)
User(1186-1207)ui/src/contexts/AuthContext.tsx (1)
TOKEN_KEY(24-24)ui/src/components/ui/table.tsx (5)
TableHead(131-131)TableRow(133-133)TableHeader(132-132)TableBody(127-127)TableCell(129-129)ui/src/components/ui/dropdown-menu.tsx (1)
DropdownMenuItem(185-185)
ui/src/pages/users/UserFormModal.tsx (1)
ui/src/contexts/AppBarContext.ts (1)
AppBarContext(11-21)
internal/service/frontend/api/v2/users.go (2)
internal/auth/context.go (1)
UserFromContext(20-23)api/v2/api.gen.go (3)
UserId(1366-1366)Error(580-589)ErrorCodeForbidden(46-46)
internal/common/config/loader.go (1)
internal/common/config/config.go (1)
Auth(162-168)
ui/src/pages/users/ResetPasswordModal.tsx (2)
ui/src/contexts/AppBarContext.ts (1)
AppBarContext(11-21)api/v2/api.gen.go (1)
Error(580-589)
internal/common/config/config_test.go (1)
internal/common/config/config.go (2)
OIDCRoleMapping(177-190)Auth(162-168)
⏰ 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 (32)
internal/common/config/loader.go (5)
437-439: LGTM: Flexible configuration parsing.The use of
parseStringListfor Scopes and Whitelist enables both YAML list and comma-separated string formats, improving configuration flexibility.
447-447: LGTM: Consistent domain parsing.Using
parseStringListfor AllowedDomains maintains consistency with the other OIDC configuration fields.
450-462: LGTM: Role mapping configuration.The role mapping configuration is correctly loaded from the definition, including the DefaultRole field. The nil checks ensure safe access.
511-513: LGTM: Secure default role.Defaulting to "viewer" is a secure choice, providing read-only access by default for auto-signed-up users.
443-446: The AutoSignup default totrueis intentional and already documented in the code comments (config.go line 230: "Auto-create users on first login (default: true)"). This is not a breaking change—it is the designed behavior for OIDC-based authentication in builtin mode. No additional documentation action is required.internal/service/frontend/api/v2/users.go (1)
151-164: LGTM: Self-disable protection implemented correctly.The self-disable protection is well-implemented:
- Checks only when attempting to disable (
*request.Body.IsDisabled == true)- Compares current user ID with target user ID
- Returns appropriate
ForbiddenerrorThe check gracefully handles missing user context (when
okis false), though this scenario is unlikely sincerequireAdminalready validates authentication.ui/src/pages/users/ResetPasswordModal.tsx (3)
1-3: LGTM: Standard context usage.The AppBarContext is correctly imported and consumed to access the selected remote node.
Also applies to: 38-38
77-79: LGTM: Defensive authentication check.The early token validation prevents unnecessary API calls and provides clear error messaging when not authenticated.
80-81: LGTM: Proper remoteNode handling.The remoteNode parameter is correctly:
- Retrieved from context with a sensible 'local' fallback
- URL-encoded to handle special characters
- Appended as a query parameter
ui/src/pages/users/UserFormModal.tsx (4)
1-3: LGTM: Consistent context usage.The AppBarContext usage is consistent with other user management components.
Also applies to: 50-50
85-88: LGTM: Consistent authentication and remoteNode handling.The token validation and remoteNode extraction match the pattern used in other user management modals.
92-93: LGTM: Correct HTTP method and remoteNode integration.Using
PATCHfor partial updates is semantically correct, and the remoteNode query parameter is properly integrated.
107-107: LGTM: remoteNode added to create endpoint.The remoteNode parameter is consistently added to the create user endpoint.
ui/src/pages/login.tsx (5)
3-3: LGTM: Centralized token key constant.Using
TOKEN_KEYfrom AuthContext ensures consistent token storage key usage across the application.
34-46: LGTM: Clean OIDC callback flow.The token callback handling is well-implemented:
- Stores token in localStorage using the centralized key
- Navigates immediately with
replace: trueto prevent back-button issues- Early return prevents interference with error/welcome message logic
- Clear comment explains the flow
49-50: LGTM: Fixed double-decoding bug.Removing
decodeURIComponentis correct sincesearchParams.get()already decodes URL parameters. The previous code would have double-decoded, potentially causing issues with certain error messages.
55-55: LGTM: Complete effect dependencies.Adding
navigateandfromto the dependency array follows React best practices, even though these values are typically stable.
85-85: LGTM: Refined background styling.The change to
bg-muted/50provides a softer, more visually refined login page appearance while maintaining light/dark mode compatibility.internal/service/frontend/auth/oidc.go (2)
35-39: LGTM! Sensible retry configuration.The retry constants (3 attempts with 500ms delays) provide reasonable resilience against transient network issues during OIDC provider initialization without excessive delays.
538-539: LGTM! Consistent URL path normalization.The trailing slash trimming ensures clean URL construction and matches the pattern used in the callback handler (line 526).
ui/src/pages/users/index.tsx (4)
59-77: LGTM! Proper remoteNode parameter handling.The implementation correctly:
- Defaults to 'local' when no remote node is selected
- URL-encodes the parameter value
- Updates the useCallback dependency array to include
selectedRemoteNode
160-164: LGTM! Compact table column layout.The updated header widths accommodate the new Auth and Status columns while maintaining the compact, high-information-density design specified in the coding guidelines.
171-183: LGTM! Correct colspan updates and disabled user styling.The colspan values correctly reflect the new column count (7), and the
opacity-60class provides a subtle visual indicator for disabled users without cluttering the interface.
197-206: LGTM! Clear status indicators with dark mode support.The Auth and Status columns provide clear visual indicators:
- Authentication provider (SSO vs Local) is immediately visible
- Disabled status uses red color coding with proper dark mode variants
- Text hierarchy maintains readability
Based on coding guidelines requiring dark mode support for all UI components.
internal/common/config/config.go (4)
178-190: LGTM! Improved configuration structure.Moving
DefaultRoleinto theOIDCRoleMappingstruct improves logical grouping of role-related configuration. The documentation clearly indicates the default value.
418-454: LGTM! Validation correctly updated for new structure.The validation logic properly uses
IsConfigured()for gating and accesses the nestedRoleMapping.DefaultRolefield. The error message accurately reflects the configuration path.
456-467: LGTM! Improved email scope validation.The refactored validation:
- Uses idiomatic
slices.Containsfor cleaner code- Clearly errors when email scope is required but missing (whitelist/allowedDomains configured)
- Provides helpful warning for potential future issues
This improves the developer experience by catching configuration issues early.
217-240: LGTM! Clean auto-detection pattern.The
IsConfigured()method provides a clean API for determining OIDC availability based on required fields, eliminating the need for an explicitEnabledflag. This is more intuitive and reduces configuration errors.The
AutoSignupdefault is properly handled: the loader uses a nullable*boolinput, applies atruedefault when not specified, then assigns it to the non-nullableboolfield in the final config struct.internal/common/config/config_test.go (4)
384-417: LGTM! Test correctly validates auto-detection behavior.The renamed test clearly documents the new behavior: incomplete OIDC configuration doesn't error (OIDC simply isn't enabled). The assertion using
IsConfigured()properly validates this.
419-449: LGTM! Test validates nested role mapping correctly.The test properly uses the nested
RoleMapping.DefaultRolestructure and verifies the validation error message matches the implementation.
451-481: LGTM! Valid configuration test updated correctly.The test uses the new nested
RoleMappingstructure and includes the email scope to ensure clean validation without warnings.
483-549: LGTM! Comprehensive email scope validation tests.The tests correctly cover both scenarios:
- Missing email scope without access control → warning
- Missing email scope with whitelist configured → error
This validates the improved validation logic from
config.go.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1569 +/- ##
==========================================
- Coverage 66.61% 66.58% -0.04%
==========================================
Files 247 247
Lines 27504 27527 +23
==========================================
+ Hits 18321 18328 +7
- Misses 7568 7580 +12
- Partials 1615 1619 +4
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
1 participant
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.