fix: resolve OIDC account linking and show SSO error messages#28
fix: resolve OIDC account linking and show SSO error messages#28TerrifiedBug merged 2 commits intomainfrom
Conversation
Bug 1: OAuthAccountNotLinked after deleting a local user and re-logging via SSO. Root cause: the signIn callback creates the User record before Auth.js's handleLoginOrRegister runs. Auth.js then finds the user by email, sees no linked Account, and throws OAuthAccountNotLinked because allowDangerousEmailAccountLinking was false. Fix: set it to true — this is safe because the signIn callback already guards against local account hijacking (returns redirect for LOCAL authMethod users). Bug 2: When a local user attempts SSO login, the generic AccessDenied error gave no indication of what went wrong. Fix: return a redirect URL with a specific error code instead of returning false, and display a clear message on the login page explaining the user needs admin linking. Also adds debug logging for OIDC group claims to help diagnose team mapping issues.
Greptile SummaryThis PR fixes two OIDC SSO issues: a
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Browser
participant NextJS as Next.js / Auth.js
participant Callback as signIn Callback
participant DB as Prisma / DB
Browser->>NextJS: OIDC redirect (code exchange complete)
NextJS->>Callback: signIn({ user, account, profile })
Callback->>DB: findUnique(email)
alt No user found
DB-->>Callback: null
Callback->>DB: user.create({ authMethod: "OIDC" })
DB-->>Callback: new dbUser
Callback-->>NextJS: return true
NextJS->>DB: create Account record (allowDangerousEmailAccountLinking=true)
else authMethod === "LOCAL"
DB-->>Callback: LOCAL user
Callback->>DB: writeAuditLog(oidc_link_blocked)
Callback-->>NextJS: return "/login?error=local_account"
NextJS-->>Browser: redirect → /login?error=local_account
Browser->>Browser: useEffect reads ?error param, shows message, replaces URL
else authMethod === "OIDC" (existing, no Account)
DB-->>Callback: OIDC user
Callback-->>NextJS: return true
NextJS->>DB: link Account by email (allowDangerousEmailAccountLinking=true)
NextJS-->>Browser: session created → redirect /
end
Last reviewed commit: 05e9050 |
| const errorParam = searchParams.get("error"); | ||
| if (errorParam) { | ||
| setError(SSO_ERROR_MESSAGES[errorParam] ?? "An error occurred during sign-in. Please try again."); | ||
| window.history.replaceState({}, "", "/login"); |
There was a problem hiding this comment.
replaceState doesn't sync Next.js router state
window.history.replaceState updates the browser URL directly but does NOT update Next.js's internal router state. As a result, useSearchParams() may still return the old params (containing ?error=...) on subsequent renders, causing this useEffect to re-run and redundantly re-set the same error message.
The recommended pattern in Next.js App Router is to use router.replace('/login') instead, which flushes both the browser URL and Next.js's searchParams state:
| window.history.replaceState({}, "", "/login"); | |
| router.replace("/login"); |
This ensures searchParams is cleanly emptied after the error is consumed, preventing any stale re-invocation of the effect.
Rule Used: ## Code Style & Conventions
TypeScript Conven... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/(auth)/login/page.tsx
Line: 41
Comment:
**`replaceState` doesn't sync Next.js router state**
`window.history.replaceState` updates the browser URL directly but does NOT update Next.js's internal router state. As a result, `useSearchParams()` may still return the old params (containing `?error=...`) on subsequent renders, causing this `useEffect` to re-run and redundantly re-set the same error message.
The recommended pattern in Next.js App Router is to use `router.replace('/login')` instead, which flushes both the browser URL and Next.js's `searchParams` state:
```suggestion
router.replace("/login");
```
This ensures `searchParams` is cleanly emptied after the error is consumed, preventing any stale re-invocation of the effect.
**Rule Used:** ## Code Style & Conventions
### TypeScript Conven... ([source](https://app.greptile.com/review/custom-context?memory=6ae51394-d0b6-4686-bc4c-1ad840c2e310))
How can I resolve this? If you propose a fix, please make it concise.| const groupsClaim = settings?.oidcGroupsClaim ?? "groups"; | ||
| const profileData = profile as Record<string, unknown> | undefined; | ||
| const userGroups = (profileData?.[groupsClaim] as string[] | undefined) ?? []; | ||
| console.log(`[oidc] User ${user.email} groups (claim "${groupsClaim}"):`, userGroups); |
There was a problem hiding this comment.
Permanent PII logging on every OIDC login
This line logs the user's email address and their full group membership list on every SSO login. These are server logs that may be collected by log aggregation pipelines, and group membership can reveal organizational structure and access levels.
The PR description calls this "debug logging," but it's unconditional and permanent — there's no debug flag guarding it. Consider either removing it after diagnosis or gating it on an environment variable:
| console.log(`[oidc] User ${user.email} groups (claim "${groupsClaim}"):`, userGroups); | |
| if (process.env.DEBUG_OIDC_GROUPS === "true") { | |
| console.log(`[oidc] User ${user.email} groups (claim "${groupsClaim}"):`, userGroups); | |
| } |
Rule Used: ## Security & Cryptography Review Rules
When revi... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/auth.ts
Line: 204
Comment:
**Permanent PII logging on every OIDC login**
This line logs the user's email address and their full group membership list on every SSO login. These are server logs that may be collected by log aggregation pipelines, and group membership can reveal organizational structure and access levels.
The PR description calls this "debug logging," but it's unconditional and permanent — there's no debug flag guarding it. Consider either removing it after diagnosis or gating it on an environment variable:
```suggestion
if (process.env.DEBUG_OIDC_GROUPS === "true") {
console.log(`[oidc] User ${user.email} groups (claim "${groupsClaim}"):`, userGroups);
}
```
**Rule Used:** ## Security & Cryptography Review Rules
When revi... ([source](https://app.greptile.com/review/custom-context?memory=7cb20c56-ca6a-40aa-8660-7fa75e6e3db2))
How can I resolve this? If you propose a fix, please make it concise.PR #28 added useSearchParams() to the login page for SSO error messages, but Next.js requires a Suspense boundary around components using this hook or static prerendering fails during build.
PR #28 added useSearchParams() to the login page for SSO error messages, but Next.js requires a Suspense boundary around components using this hook or static prerendering fails during build.
Summary
OAuthAccountNotLinkedafter deleting a local user: ThesignIncallback creates the User record before Auth.js'shandleLoginOrRegisterruns. Auth.js then finds the user by email, sees no linked Account, and throwsOAuthAccountNotLinkedbecauseallowDangerousEmailAccountLinkingwasfalse. Changed totrue— safe because the callback already guards against local account hijacking (redirectsLOCALauthMethod users).return false(genericAccessDenied), return a redirect to/login?error=local_accountwith a user-friendly message explaining the admin must link accounts first.Test plan