feat(auth): redirect-only login mode with break-glass admin allowlist - #18
Conversation
- /dashboard/login auto-redirects to OIDC when it is the only transport; suppressed on ?oidcError bounce (no loop), WxWork-only environments, and ?direct=1 - break-glass: auth.breakGlassEmails (env BREAK_GLASS_LOGIN_EMAILS) keeps password login reachable for allowlisted admin emails while password login is disabled suite-wide; matched by email only, so the seeded default-password account can never qualify; PublicConfig exposes breakGlassLoginEnabled so the form renders only when armed - JIT role mapping follows the suite standard: default cs_user, team LEAD claims grant cs_team_leader, org ADMIN/OWNER claims grant admin; role absence no longer defaults to admin - the default-password bootstrap admin is not seeded in SSO-only mode (oidc enabled + password login disabled); allowlisted admins bootstrap the first admin on OIDC login instead - support portal is never auto-redirected (it serves customer accounts) - tests: break-glass config helpers, allowlist matching, bootstrap seed gating in SSO-only and mixed modes
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
![dos[bot]](https://avatars.githubusercontent.com/in/4711427?s=60&v=4){kind=small}
There was a problem hiding this comment.
⏱️ Code Review completed (18 files · 32,942 chars · 2 PR unit(s))
ℹ️ Full-Context Analysis: Analyzed all changed files in a unified context pass to preserve cross-file type definitions, imports, and caller contracts. Deducted 2 PR units.
⏱️ Adversarial Review completed (Model: qwen3.8-27b)
🔍 Verified Adversarial Review Findings
🟡 IMPORTANT
internal/services/auth_service.go:100-115: User Enumeration via Differential Error Responses in Break-Glass Mode-
Failure Trace:
- Configuration:
passwordLoginEnabled: false,breakGlassEmails: "admin@corp.com". - Attacker sends
POST /api/auth/loginwithusername: "victim@corp.com"(a valid, non-admin email) andpassword: "wrong". - Code path:
passwordLoginEnabledis false.UserService.GetByUsername("victim@corp.com")returnsnil. - Fallback:
UserService.GetByEmail("victim@corp.com")returns the user object. - Check:
user.StatusisStatusOk. - Check:
isBreakGlassUserreturnsfalse(victim is not on the allowlist). - Response:
errorsx.ForbiddenI18n("error.auth.passwordLoginDisabled")(HTTP 403). - Attacker sends
POST /api/auth/loginwithusername: "nonexistent@corp.com"andpassword: "wrong". - Code path:
GetByUsernamereturnsnil.GetByEmailreturnsnil. - Response:
errorsx.InvalidAccountI18n("error.e0260")(HTTP 401/400, distinct from 403). - Result: The attacker can distinguish between existing and non-existing email addresses by observing the difference in error codes/messages, enabling user enumeration.
- Configuration:
-
Actionable Fix:
WhenpasswordLoginEnabledis false, the API should return a uniform error response for all failed login attempts to prevent enumeration. The specific "break-glass" logic should only apply if the request explicitly includes the break-glass indicator (e.g., a query parameter or header), or the error messages should be standardized to "Invalid credentials" regardless of whether the user exists but is unauthorized.if user == nil || user.Status != enums.StatusOk { if !passwordLoginEnabled { _ = s.createLoginCredentialLog(principal, 0, false, clientIP, userAgent, "password login disabled") return nil, errorsx.InvalidAccountI18n("error.e0260") } _ = s.createLoginCredentialLog(principal, 0, false, clientIP, userAgent, "user not found") return nil, errorsx.InvalidAccountI18n("error.e0260") } if !passwordLoginEnabled && !isBreakGlassUser(authCfg, user) { _ = s.createLoginCredentialLog(principal, user.ID, false, clientIP, userAgent, "password login disabled") return nil, errorsx.InvalidAccountI18n("error.e0260") }
-
🛡️ Dismissed Claims
- None: The single candidate claim was verified and retained as an Important security issue.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a "break-glass" login mechanism allowing allowlisted admin emails to bypass SSO-only mode and log in via password, along with JIT role mapping during OIDC login. The review feedback identifies a user enumeration vulnerability in the break-glass login flow and notes that disabling password login for staff inadvertently blocks customer password logins. It recommends decoupling customer and staff authentication paths, removing redundant handler-level checks, and adding a nil check in the migration script to prevent potential nil pointer dereferences.
| passwordLoginEnabled := authCfg.IsPasswordLoginEnabled() | ||
|
|
||
| user := UserService.GetByUsername(username) | ||
| if user == nil && !passwordLoginEnabled { | ||
| // Break-glass door: in SSO-only mode an allowlisted admin signs in with | ||
| // the account email, which is not necessarily the username. | ||
| user = UserService.GetByEmail(strings.ToLower(username)) | ||
| } | ||
| if user == nil || user.Status != enums.StatusOk { | ||
| if !passwordLoginEnabled { | ||
| _ = s.createLoginCredentialLog(principal, 0, false, clientIP, userAgent, "password login disabled") | ||
| return nil, errorsx.ForbiddenI18n("error.auth.passwordLoginDisabled") | ||
| } | ||
| _ = s.createLoginCredentialLog(principal, 0, false, clientIP, userAgent, "user not found") | ||
| return nil, errorsx.InvalidAccountI18n("error.e0260") | ||
| } | ||
| if !passwordLoginEnabled && !isBreakGlassUser(authCfg, user) { | ||
| _ = s.createLoginCredentialLog(principal, user.ID, false, clientIP, userAgent, "password login disabled") | ||
| return nil, errorsx.ForbiddenI18n("error.auth.passwordLoginDisabled") | ||
| } | ||
| if strs.IsBlank(user.Password) || bcrypt.CompareHashAndPassword([]byte(user.Password), []byte(password)) != nil { | ||
| _ = s.createLoginCredentialLog(principal, user.ID, false, clientIP, userAgent, "password mismatch") | ||
| return nil, errorsx.InvalidAccountI18n("error.e0260") |
There was a problem hiding this comment.
The current implementation of the break-glass login mode introduces a user enumeration vulnerability. When passwordLoginEnabled is false, a login attempt for a non-existent user or a user not on the break-glass list returns ForbiddenI18n("error.auth.passwordLoginDisabled"). However, an attempt for a valid break-glass user with an incorrect password returns InvalidAccountI18n("error.e0260"). This discrepancy allows attackers to easily enumerate which admin emails are on the break-glass allowlist.
Additionally, as documented in docs/ARCHITECTURE.md, sharing the same password login endpoint and configuration between staff and customers means that disabling password login for staff completely disables it for customers as well.
We can solve both issues elegantly by:
- Returning a generic
InvalidAccountI18nerror for all failed attempts when the user is not found or not authorized, preventing enumeration. - Distinguishing between staff (
enums.UserTypeEmployee) and customers (enums.UserTypeUser) usinguser.UserType, so that customers can always log in with passwords even when staff password login is disabled.
passwordLoginEnabled := authCfg.IsPasswordLoginEnabled()
user := UserService.GetByUsername(username)
if user == nil {
user = UserService.GetByEmail(strings.ToLower(username))
}
if user == nil || user.Status != enums.StatusOk {
_ = s.createLoginCredentialLog(principal, 0, false, clientIP, userAgent, "user not found")
return nil, errorsx.InvalidAccountI18n("error.e0260")
}
if user.UserType == enums.UserTypeEmployee && !passwordLoginEnabled && !isBreakGlassUser(authCfg, user) {
_ = s.createLoginCredentialLog(principal, user.ID, false, clientIP, userAgent, "password login disabled")
return nil, errorsx.ForbiddenI18n("error.auth.passwordLoginDisabled")
}
if strs.IsBlank(user.Password) || bcrypt.CompareHashAndPassword([]byte(user.Password), []byte(password)) != nil {
_ = s.createLoginCredentialLog(principal, user.ID, false, clientIP, userAgent, "password mismatch")
return nil, errorsx.InvalidAccountI18n("error.e0260")
}| if !cfg.Auth.IsPasswordLoginEnabled() && !cfg.Auth.HasBreakGlassEmails() { | ||
| httpx.WriteJSON(ctx, errorsx.ForbiddenI18n("error.auth.passwordLoginDisabled")) | ||
| return | ||
| } |
There was a problem hiding this comment.
To allow customers (enums.UserTypeUser) to continue logging in via password when staff password login is disabled, we should remove this handler-level restriction. The AuthService.Login method will handle the password login restrictions specifically for employees (enums.UserTypeEmployee), making the handler-level check redundant and overly restrictive.
// Password login enablement is checked per-user in AuthService.Login to support customer login.| const passwordLoginEnabled = publicConfig?.passwordLoginEnabled !== false | ||
| // Break-glass door: with password login disabled suite-wide, an allowlisted | ||
| // admin can still reach the password form via ?direct=1. The portal is | ||
| // never auto-redirected: it primarily serves customer accounts. | ||
| const isBreakGlassRequested = | ||
| searchParams.get("direct") === "1" && publicConfig?.breakGlassLoginEnabled === true | ||
| const showPasswordForm = passwordLoginEnabled || isBreakGlassRequested | ||
| const providerCount = Number(publicConfig?.wxworkEnabled) + Number(publicConfig?.oidcEnabled) | ||
| const hasAnyLoginMethod = passwordLoginEnabled || providerCount > 0 | ||
| const hasAnyLoginMethod = showPasswordForm || providerCount > 0 |
There was a problem hiding this comment.
Since the customer support portal primarily serves customer accounts, customers should always be allowed to log in with their passwords. Restricting the password form visibility based on the staff-facing passwordLoginEnabled setting prevents customers from logging in when SSO-only mode is active for staff.
We should decouple the customer login form visibility from the staff password login configuration.
| const passwordLoginEnabled = publicConfig?.passwordLoginEnabled !== false | |
| // Break-glass door: with password login disabled suite-wide, an allowlisted | |
| // admin can still reach the password form via ?direct=1. The portal is | |
| // never auto-redirected: it primarily serves customer accounts. | |
| const isBreakGlassRequested = | |
| searchParams.get("direct") === "1" && publicConfig?.breakGlassLoginEnabled === true | |
| const showPasswordForm = passwordLoginEnabled || isBreakGlassRequested | |
| const providerCount = Number(publicConfig?.wxworkEnabled) + Number(publicConfig?.oidcEnabled) | |
| const hasAnyLoginMethod = passwordLoginEnabled || providerCount > 0 | |
| const hasAnyLoginMethod = showPasswordForm || providerCount > 0 | |
| // The portal primarily serves customer accounts, who always use password login. | |
| const showPasswordForm = true | |
| const providerCount = Number(publicConfig?.wxworkEnabled) + Number(publicConfig?.oidcEnabled) | |
| const hasAnyLoginMethod = showPasswordForm || providerCount > 0 |
| cfg := config.Current() | ||
| if cfg.OIDC.Enabled && !cfg.Auth.IsPasswordLoginEnabled() { | ||
| slog.Info("skipping bootstrap admin seed: SSO-only login mode is active", | ||
| "oidc_enabled", true, | ||
| "password_login_enabled", false) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Add a nil check for cfg before accessing its fields to prevent potential nil pointer dereference panics during migrations (especially in test environments where the global config might not be fully initialized).
| cfg := config.Current() | |
| if cfg.OIDC.Enabled && !cfg.Auth.IsPasswordLoginEnabled() { | |
| slog.Info("skipping bootstrap admin seed: SSO-only login mode is active", | |
| "oidc_enabled", true, | |
| "password_login_enabled", false) | |
| return nil | |
| } | |
| cfg := config.Current() | |
| if cfg != nil && cfg.OIDC.Enabled && !cfg.Auth.IsPasswordLoginEnabled() { | |
| slog.Info("skipping bootstrap admin seed: SSO-only login mode is active", | |
| "oidc_enabled", true, | |
| "password_login_enabled", false) | |
| return nil | |
| } |
- register the loaded config in cmd/migration and cmd/testdata so the SSO-only bootstrap-admin gate cannot panic on standalone runs - warn at startup when SSO-only mode is active without a break-glass allowlist (fresh deployment has no admin bootstrap or recovery path) - hide the support-portal register toggle while password login is disabled: the server rejects registrations in that mode, so the toggle would only dead-end a break-glass admin - doc precision: ensureOrganisationAdminRole is claim- or allowlist- triggered; the bootstrap-seed skip only covers fresh deployments
1 participant
Implements Wave 2 (Crove-Desk) of the Crove suite redirect-only DOS ID login plan (survey: DOS/DOS.Me#793, section 8 in #797 PR). Two security preconditions from the survey are included: the OIDC JIT default-admin grant and the default-password bootstrap seed.
Behaviour (all gated; default config unchanged):
Known interaction documented in docs/ARCHITECTURE.md: customer portal sign-in shares the staff password endpoint, so in SSO-only mode only break-glass principals can password-sign in there.
Verification: go build ./..., go test ./internal/... ./cmd/... all pass (incl. new tests for config helpers, allowlist matching, bootstrap seed gating), gofmt clean on changed files, pnpm typecheck passes, pnpm lint has no errors in changed files (6 pre-existing errors elsewhere), i18n keys added to zh-CN/en-US/vi-VN.