Unified authentication: a single sign-in surface for portal and admin#284
Unified authentication: a single sign-in surface for portal and admin#284mortondev wants to merge 68 commits into
Conversation
…hods
- New script set-portal-auth-methods.ts (disable/restore/enable-magic-link)
reads and patches settings.portal_config.oauth in the DB, mirroring the
set-workspace-anon.ts pattern
- Case (d) now disables all portal auth methods before navigating, asserts
the team form still renders, and restores in a finally block
- Case (c) wraps loginViaMagicLink in enable-magic-link/restore so the
portal magic-link hook does not silently drop the send on repeat runs
(existing role='user' principals are blocked by hooks.ts:277-279 when
portal magicLink is off by default)
- Case (c) now passes { role: 'user' } explicitly to loginViaMagicLink
- Added flushMagicLinkRateLimit helper (execFileSync, no shell) + beforeAll
- Added comment to case (e) confirming /auth/login loader does not resolve
the invitation id — only isTeamCallback/isSafeCallbackUrl are called
… the provider list
…config domain normalization, per-provider test/enforcement, onboarding registry read)
…rallelize test stamps
…, single-source button predicate, non-sso enforce UI dead-end)
…-code break-glass link
Move the recoveryLink element outside the emailEntryEnabled guard in Stage 1 so it renders whenever showRecoveryLink is true, regardless of whether password or magic-link is enabled. SSO-only workspaces (password=false, magicLink=false) previously hid the link entirely because the emailEntryEnabled block was skipped. Adds two tests: recovery link absent when callbackUrl is undefined, and recovery link visible in SSO-only Stage 1 with a team callbackUrl.
…nsition Team surfaces (e.g. /admin) are now reached via window.location.assign so the second postAuthSuccess listener (portal-header invalidate) cannot race with the client-side route change and blank the page mid-transition.
Generalize the "Test sign-in" flow from the legacy single-sso provider to any OIDC identity provider stored in identity_provider. - startSsoTestFn now accepts registrationId, resolves the provider via listIdentityProviders, fetches the secret from getIdentityProviderCredentials, and builds redirectUri as /api/auth/oauth2/callback/<registrationId> — the provider's own production callback. - TestSession carries registrationId so the callback knows which provider to stamp. - sso-test-callback stamps the session's provider row (markTestSucceeded) and, for the legacy 'sso' registrationId only, also stamps the settings blob (markSsoTestSucceeded) for backward compatibility. - The auth catch-all now intercepts all /api/auth/oauth2/callback/* paths (startsWith SSO_OAUTH_CALLBACK_PREFIX) before Better-Auth; a Redis miss still falls through cleanly. - provider-editor passes registrationId to TestSignInButton and computes enforceable via the isSsoTestValid predicate inline (no DB import). Removes the 'sso'-only guard, TODO comment, and stale copy.
…ixes Identity providers: - persist the selected IdP family (migration 0116, nullable `kind`) so the editor and list always render the chosen provider; legacy rows fall back to inferring it from the discovery URL - tile picker with logos; list as cards with an in-row enable toggle and per-domain chips; add a `configured` (secret-present) DTO flag Keep one working sign-in method: - server guard refuses to disable/remove the last working method across the IdP delete/disable, built-in/social config, and legacy SSO secret-delete paths; tier-gated to mirror registration - the count reads one unified config (auth is unified on this branch); reject whitespace-only client secrets Portal sign-in: - surface the Log in entry and the email input for a routed-only IdP - redirect a sole-IdP workspace to the provider server-side; drop the unused prompt=login / suppressInstantSso path UI: rename the Sign-in tab; show the entered email as a filled field on login; make the recovery-codes dialog always closable via X / Escape
The /sso route is now a redirect and the old portal/admin login forms were deleted, orphaning a large legacy surface. Removed across the branch: - the dead single-SSO UI subtree (sso-page + connection / verified-domains / attribute-mapping sections, sso-page-callout) and their tests - the legacy SSO server fns that subtree drove (test-connection, switch-provider, set-secret, get-status, add/verify/enforce verified-domain) plus the dead ssoStatus query and sso-secret helpers; sso.ts drops ~565 lines - unused exports + their tests: OAuthButtons, a duplicate getBaseUrl, parseGateError / isValidGateError, isSsoConfigured, isEmailAtVerifiedDomain, hasRole, getMagicLinkToken - the unreachable sso-unavailable login branch and the vestigial portalConfig prop on the sign-in tab Also fixes two audit-test suites the previous commit's last-method guard broke: they did not mock the new DB-backed sign-in-method check. ~3,200 lines net removed. Kept clearSsoClientSecretFn (break-glass) and the legacy ssoOidc config subtree (deferred migration). typecheck + lint clean; the full suite passes (the lone failure is a pre-existing node:dns mock issue).
- Nest recovery codes inside the Single sign-on (OIDC) card as a bordered subsection instead of a standalone card; they're the account break-glass for SSO and back up TOTP, so they stay shown regardless of the OIDC tier. - Rename the OIDC card to "Single sign-on (OIDC)" and unify the tier-on and tier-off states into one card. Only mount the SSO-test provider (and its global postMessage listener) when the tier is on. - Relabel the 2FA row "Require two-factor authentication" with a left-rule nested treatment under Password and a state-aware description. - Refresh the page header, Email, and Social sign-in copy. - Update the affected component tests.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 112014323a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const authConfig = { | ||
| found: true, | ||
| oauth: publicPortalConfig?.oauth ?? DEFAULT_PORTAL_CONFIG.oauth, | ||
| customProviderNames: publicPortalConfig?.customProviderNames, | ||
| oidcProviders: publicPortalConfig?.oidcProviders, | ||
| registeredAuthProviders, |
There was a problem hiding this comment.
When /admin/login now redirects into the portal shell with callbackUrl=/admin, this still builds the dialog from publicPortalConfig (and the form later calls lookupAuthMethodsFn with surface: 'portal'). In any existing/asymmetric config where a team method is enabled but the portal method is disabled, admins are shown the portal login policy instead of publicAuthConfig, so the configured admin password/magic-link/social path disappears or instant SSO can take over. Select the team auth config/surface whenever the safe callback is a team callback.
Useful? React with 👍 / 👎.
| // Stash the post-login destination before the sign-in call so the | ||
| // twoFactor client can forward to it from its /auth/two-factor page. | ||
| stashTwoFactorCallbackUrl(callbackUrl) | ||
| const result = await authClient.signIn.email({ |
There was a problem hiding this comment.
The header still opens this dialog without a callbackUrl, so passing undefined here clears the 2FA callback stash. For password users who trigger Better Auth's TOTP redirect from a portal page, /auth/two-factor then receives no callback and falls back to /, whereas the previous code preserved the current path. Use the explicit callback when present, but fall back to the current location before calling stashTwoFactorCallbackUrl.
Useful? React with 👍 / 👎.
…lper CodeQL js/clear-text-logging flags any value reached via an `oauth` property as sensitive. The helper only logged it for diagnostics and callers ignore the output, so log just the action.
Remove the portal-specific checkPortalAuthMethod branch in isAuthMethodAllowed. All roles now read authConfig.oauth via isSignInMethodEnabled/normalizeMethodKey (Task 1 helper). Delete checkPortalAuthMethod and the now-unused getPublicPortalConfig import. Rename the unused `role` param to `_role` to satisfy lint. Update the existing test suite to reflect unified semantics: magic-link is opt-in for all roles (was default-on for team); portal tests now mock getTenantSettings instead of getPublicPortalConfig.
Replace getPublicPortalConfig with getPublicAuthConfig and use isSignInMethodEnabled for the password/magic-link gate, so admin and portal sign-in evaluate the same single policy.
Remove oauth from PublicPortalConfig; both the portal sign-in dialog and portal header now read the unified publicAuthConfig.oauth, keeping oidcProviders sourced from publicPortalConfig.
Drop the dual-write to portalConfig.oauth from SignInProvidersTab. saveBuiltin and saveOauthProvider now call updateAuthConfigFn once; the portal rollback path is removed. oauthState seeds from teamAuthConfig.oauth; display booleans use isSignInMethodEnabled so password defaults ON when the key is absent. The initialPortalOauth prop is removed from the component, auth-settings, and the route. auth-provider-credentials also reads authConfig.oauth when disabling a provider on credential deletion. New test asserts updatePortalConfigFn is never called on a password toggle.
…istration Delete PortalAuthMethods + PortalConfig.oauth + DEFAULT_PORTAL_CONFIG.oauth and their re-exports. Provider registration (auth/index.ts and registered-providers.ts) now reads authConfig.oauth only via isSignInMethodEnabled; the two-surface union is gone. updatePortalConfig no longer accepts or bumps auth version for an oauth field. Stale managed-path strings (portalConfig.oauth.*) in the sign-in UI removed. Legacy OIDC cleanup preserved via existing Record<string,unknown> cast. Dead getPublicPortalConfig mocks removed from test helpers; backfill test adds absent-authConfig coverage.
Route hasAnyWorkingSignInMethod through isSignInMethodEnabled so the absent-key default is authoritative in signin-methods.ts for all surfaces. Previously an absent magicLink key was treated as enabled (old !== false default), letting the lockout backstop count a phantom working method the runtime gate would reject. Adds a regression test covering the absent-key case.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1329f1183
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await database | ||
| .update(settings) | ||
| .set({ authConfig: JSON.stringify(parsedAuth) }) | ||
| .where(eq(settings.id, rows[0].id)) |
There was a problem hiding this comment.
When an upgraded workspace has portal-only methods to merge, this startup backfill writes settings.authConfig but never bumps auth_config_version, invalidates TENANT_SETTINGS, or calls resetAuth(). Because logStartupBanner() starts the backfill asynchronously, a first request can cache tenant settings or build the Better Auth instance before this update; with no version change, the auth runtime keeps the pre-merge provider list until a restart or some later auth-config write. Mirror updateAuthConfig's version/cache reset when this update (or the custom-OIDC insert in the same startup backfill) actually changes auth inputs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff99993fff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| label: input.label, | ||
| kind: input.kind ?? null, | ||
| clientId: input.clientId, | ||
| discoveryUrl: input.discoveryUrl ?? null, |
There was a problem hiding this comment.
When an admin creates a provider with a discovery URL such as https://127.0.0.1/... or a hostname that resolves to a private address, this insert persists it after only the httpsUrl schema check. createAuth() later passes that value to the generic OAuth discovery flow, so sign-in/auth rebuilds can make server-side requests to internal services; the legacy updateAuthConfig path still runs checkUrlSafety before saving SSO discovery URLs. Apply the same SSRF guard before persisting identity-provider discovery URLs.
Useful? React with 👍 / 👎.
| const patch: Partial<typeof identityProvider.$inferInsert> = { | ||
| registrationId: input.registrationId, | ||
| label: input.label, | ||
| clientId: input.clientId, |
There was a problem hiding this comment.
When an already-tested provider has its Client ID changed here, the row keeps its old detailsChangedAt timestamp; the same stale-test path applies to the discoveryUrl assignment below. setDomainEnforcedFn compares lastSuccessfulTestAt against detailsChangedAt, so a test from before the connection change can still unlock SSO enforcement and make the UI show the provider as enforceable. Stamp detailsChangedAt whenever connection fields actually change, matching the legacy SSO config path.
Useful? React with 👍 / 👎.
| }, | ||
| headers: getRequestHeaders(), | ||
| }, | ||
| async () => upsertIdentityProvider(data) |
There was a problem hiding this comment.
When the config file declares auth.identityProviders.<label>.*, computeManagedPaths records those fields as locked, but this write path calls upsertIdentityProvider without checking the managed paths. As a result, an admin can edit a config-managed provider's Client ID/discovery URL from the UI/API, leaving production drift until the next reconcile flips it back. Check the relevant auth.identityProviders.<encoded label>.* locks before allowing the upsert.
Useful? React with 👍 / 👎.
…ithout a callbackUrl When callbackUrl is undefined (e.g. the portal-header open-dialog path), stashTwoFactorCallbackUrl(undefined) was actively clearing any prior stash entry via sessionStorage.removeItem. Fall back to window.location.pathname + window.location.search so the stash is always populated before signIn.email is called.
…a provider backfillCustomOidcProvider writes a new identity_provider row that changes the auth plugin graph (listIdentityProviders feeds createAuth). The post- transaction resetAuth + invalidateSettingsCache calls were only guarded on signInMethodsMerged, so a freshly-backfilled custom-OIDC provider stayed unregistered until the next restart or auth-config write. Hoist the created count outside the transaction and widen the guard to `signInMethodsMerged || customOidcCreated > 0`.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0cb4a42d8e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // SSRF guard: validate the discoveryUrl before any DB write — the auth | ||
| // runtime and SSO test callback fetch this URL server-side. | ||
| if (input.discoveryUrl) { |
There was a problem hiding this comment.
This guard only checks discoveryUrl, but the same upsert accepts manual endpoint fields such as tokenUrl; when an admin/API caller saves a provider with no discovery URL and tokenUrl=https://127.0.0.1/..., buildGenericOAuthConfigs later passes that URL to genericOAuth (apps/web/src/lib/server/auth/build-oauth-configs.ts:109-118), so the OAuth callback can make server-side token requests to internal hosts. Apply the same checkUrlSafety protection to server-fetched manual endpoints before persisting them.
Useful? React with 👍 / 👎.
| const oauthConfig = (authConfig.oauth ?? {}) as Record<string, boolean | undefined> | ||
| if (oauthConfig[provider.id]) { | ||
| await updatePortalConfig({ oauth: { [provider.id]: false } }) | ||
| await updateAuthConfig({ oauth: { [provider.id]: false } }) |
There was a problem hiding this comment.
When these credentials belong to the only working social sign-in method (for example password and magic-link are disabled and only Google is enabled/configured), this path deletes the credential row first and then calls the low-level updateAuthConfig, bypassing updateAuthConfigFn's wouldLeaveNoWorkingSignInMethod guard. That leaves the unified auth config with no usable sign-in method, so check the last-method invariant before deleting credentials and abort while the existing method is still intact.
Useful? React with 👍 / 👎.
Remove the declarative config-file path for OIDC identity providers so they are configured solely through the in-app UI/API (the identity_provider table). This collapses two parallel configuration mechanisms into one, shrinking the config-file schema and its lock surface. Removed: - ssoOidc + identityProviders (and their nested schemas) from the config-file schema - the managed-path computation, providerPathKey encoder, and the IdentityProviderSpec reconcile/upsert path - the in-app managed-path guard on upsertIdentityProviderFn and the ssoOidc assertNotManaged block in updateAuthConfig - the four AUTH_SSO_* client-side managed-path constants mergeAuthConfig still passes an existing ssoOidc DB blob through unchanged, so config written via the in-app path is never erased when the reconciler merges oauth/openSignup.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5492ba7fe8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const params = new URLSearchParams({ | ||
| response_type: 'code', | ||
| client_id: sso.clientId, | ||
| client_id: provider.clientId, | ||
| redirect_uri: redirectUri, | ||
| scope: 'openid email profile', |
There was a problem hiding this comment.
When a migrated or API-created provider has custom scopes, production sign-in now uses provider.scopes in buildGenericOAuthConfigs, but this test authorize URL still always requests openid email profile. In that case the admin test can pass and stamp lastSuccessfulTestAt even though the real provider will request a different scope set and may fail or omit required claims, so enforcement can be unlocked by a test that did not mirror production.
Useful? React with 👍 / 👎.
…+ features) Auth and feature-flag configuration is now in-app/DB only. The config file no longer accepts `auth` or `features` blocks, so the reconciler never writes `auth_config` or `feature_flags` — both columns remain freely editable via the admin UI. Removes `mergeAuthConfig`, `safeAuthExisting`, `resetAuth` from the reconciler surface and trims the matching managed-path guards from `updateAuthConfig` / `updateFeatureFlags`.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
2 participants
Closes #270 — one sign-in surface for admins and regular users (with an "Admin area" entry in the user dropdown), and a single SSO/sign-in config governing both.
What this does
Consolidates authentication onto a single sign-in surface for the portal and the admin team, and tidies up the Security settings that drive it.
/auth/loginand/auth/signuppages. Old login redirects point at it, and the private-portal gate auto-opens it while honouringcallbackUrl.callbackUrlvalidation against open-redirects.Security settings (latest commit)
Testing
bun run typecheck: clean.bun run test apps/web/src/components/admin/settings/security: 42 tests pass across 7 files.eslinton the changed files: clean.