Review — Epic: Single Sign On

Really nice work on this. A few change requests below before merge:

1. GitHub /user endpoint often returns no email — many GitHub logins will fail (high)

GithubHandler points userinfo_endpoint at https://api.github.com/user. That endpoint's email field is the user's public profile email, which is null for the majority of GitHub users (those who haven't set a public email) — even though the user:email scope is granted. extract_email/1 then returns nil → {:error, :no_email} → "Could not retrieve your email."

To reliably obtain a (verified) email, GitHub requires a second call to https://api.github.com/user/emails and selecting the primary && verified entry. As written, GitHub SSO will silently fail for a large fraction of real users. (Confirmed reproducible against a test account with no public email set.)

2. Deployment documentation & env-var disambiguation (high)

DEPLOYMENT.md has no instructions for the new SSO env vars, and the variable names collide/overlap with existing ones — this will confuse operators:

• GITHUB_CLIENT_ID / GITHUB_CLIENT_SECRET (new, SSO login) vs GITHUB_APP_ID / GITHUB_APP_CLIENT_ID / GITHUB_APP_CLIENT_SECRET (existing, DEPLOYMENT.md L97-101). The latter is the GitHub App used for repo/version-control sync — a completely different feature with confusingly similar names. The docs must clearly separate "GitHub App (project version control)" from "GitHub SSO (sign-in)".
• GOOGLE_CLIENT_ID / GOOGLE_CLIENT_SECRET are already documented (DEPLOYMENT.md L363-377, "Google Oauth2") for the existing DB-backed auth-provider path, whose callback is /authenticate/callback. The new GoogleHandler reuses the same env var names but with a different callback (/authenticate/google/callback). Please clarify whether these are intentionally shared or a collision, and document the redirect URI difference.

Please add a dedicated "Single Sign-On (SSO)" section to DEPLOYMENT.md covering: which provider vars enable SSO, the exact redirect/callback URLs (/authenticate/<provider>/callback), and an explicit note distinguishing these from the GitHub App and the older Google OAuth2 settings. The .env.example comments are a good start but operators rely on DEPLOYMENT.md.

3. Migration is irreversible — mix ecto.rollback will raise (medium)

20260515133933_allow_null_hashed_password.exs:

def change do
 alter table(:users) do
 modify :hashed_password, :string, null: true
 end
end

modify/3 inside change/0 is only reversible when the :from option is given. Without it, a rollback raises Ecto.MigrationError. Use modify :hashed_password, :string, null: true, from: {:string, null: false} (or split into up/down).

4. Provider emails are trusted without a verification check (medium)

New-account provisioning trusts userinfo["email"] as a proven-owned address. Google's userinfo includes an email_verified claim; GitHub's /user/emails includes verified. Neither is checked. An attacker controlling a provider account with an unverified email could provision a Lightning account under an address they don't own. (The collision path protects existing accounts, so this is limited to new-account creation, but it's a cheap, standard hardening step.)

5. SSO button click target is too small — only the inner link is clickable (medium)

On both the login and register pages, the SSO button is a <.button> wrapping an inner <a href={provider.url}>. Only the anchor (which wraps the inline-flex icon+text) is clickable — clicking the button's padding does nothing, so the user has to aim at the link text in the middle. The full button should be the click target. Make the link itself the styled element (e.g. <.button_link href={provider.url}> / <.link> styled as a button) rather than nesting an <a> inside <.button>. Note IdentitiesComponent already uses <.button_link> for its "Link" action — the same pattern would fix this.

6. Email-existence oracle on the login form (low / product decision)

The {:error, :sso_account} flash ("This account uses single sign-on...") and the collision message ("An account already exists for {email}...") both confirm to an unauthenticated visitor whether a given email is registered, and how. This is a deliberate UX tradeoff and common, but flagging it as a behavior change from the previous generic "Invalid email or password".

7. on_conflict: :nothing masks cross-account identity collisions (low)

Accounts.link_user_identity/3 and the duplicate AccountHook.link_identity/3 insert with on_conflict: :nothing, conflict_target: [:provider, :uid]. If (provider, uid) already belongs to a different user, the insert silently no-ops and returns {:ok, identity} with a nil id, so the caller believes the link succeeded. The controller's pre-checks make this a narrow race, but :nothing is hiding a real conflict — consider distinguishing "already linked to me" from "claimed by someone else" at the data layer.

8. Minor

• defp display_name/1 is defined identically in OidcController and IdentitiesComponent; link_identity in AccountHook duplicates Accounts.link_user_identity/3. Consider consolidating.
• CacheWarmer.execute/1's try/rescue _ -> [] is very broad and will swallow genuine config errors silently.
• GithubHandler has no test file (GoogleHandler does) — given #1, a GitHub-specific userinfo test would be valuable.
• Changelog not updated (PR checklist acknowledges this).

@doc-han now this is ready for merge. Just a few things:

1. Can we prefix the SSO to these envs to avoid confusion? SSO_GITHUB_CLIENT_ID looks more explicit
2. Why can't we reuse /oauth/github/callback?
3. I don't see the need for listing all the envs for the version control here. I found the callback url in there quite useful though 👏
4. Your changelog entry looks incomplete
5. Please use button_link component in the signup/signin page. It already has all the styling you need
6. You haven't addressed the email-existence oracle

Great work on this. The overall shape is solid: routing SSO sign-ups through the
same AccountHook as password sign-ups is the right call, the hot paths that now
have to cope with a null password are handled correctly, sessions are rotated on
every login so there's no fixation, there's no open redirect through return_to,
and provider tokens and secrets are never persisted or logged.

The gaps I found sit at the security boundary and a few failure edges, not in the
core flow.

I've grouped the feedback into blocking, concerns worth addressing, and
notes and nits. Two items are blocking and I'd want them resolved before merge.

Blocking

1. The OAuth callbacks have no state parameter.

Handler.authorize_url/1 (handler.ex:89-92) passes only scope:, and the callback
OidcController.new/2 (oidc_controller.ex:54-81) never sends or verifies a state
value. Nothing ties a callback back to the browser that started the flow.

Because the callback is a GET (router.ex:72), an attacker can run their own provider
login, capture the resulting code, and force a victim's browser to hit
/authenticate/:provider/callback?code=.... The victim ends up logged into the
attacker's account. On the link flow (oidc_controller.ex:38-48) it's worse: a forged
callback grafts an attacker-controlled identity onto the victim's authenticated
session, which is a persistent backdoor.

This is a regression. The deleted Common.authorize_url/4 used to take and pass
state, and the OAuth2 library generates none of its own, so right now it's
unmitigated. Note that the framework CSRF protection on the POST signup confirmation
form is unaffected; this is specifically the OAuth authorize request and the GET
callback.

Fix: we already have most of the plumbing. The credential OAuth flow passes state
around as an encrypted, self-contained string via Lightning.SafetyString
(OauthCredentialHelper), so nothing needs persisting. The SSO flow wants the same
treatment plus a nonce: mint one when redirecting, carry it in state, and verify it
on callback (bound to the current user on the link flow), rejecting anything missing
or mismatched.

2. Nothing enforces one identity per provider, but the code assumes it.

The migration adds a unique index on (provider, uid) and a plain index on
user_id, but there's no constraint stopping a user having two identities for the
same provider - even though the moduledoc asserts there's only one
(user_identity.ex:5). link_user_identity (accounts.ex:211) only checks whether
this uid is claimed elsewhere; it never checks whether the current user already has
a different uid for the same provider.

So if someone links github/uid-A and then authorises a second GitHub account (uid-B),
they get a second row. get_user_identity/2 (accounts.ex:248, a Repo.get_by) then
raises MultipleResultsError, which 500s both /profile rendering and unlink
(identities_component.ex:60-61). It's directly reachable, because the link route
GET /authenticate/:provider/link can be hit directly.

There's a security angle too. can_remove_identity? / unlink_user_identity is the
lockout protection, and once there are two identities for a provider the lookup
crashes instead of evaluating the "would lock out" guard - so that guard is only as
sound as this missing constraint.

We've decided on one identity per provider per user, rejecting the second link. Fix:
add a (user_id, provider) unique index and unique_constraint, and/or an explicit
guard in link_user_identity that rejects the second link. As a belt-and-braces
measure, key the profile component's unlink by identity id rather than by provider.

Concerns worth addressing

3. Two concrete crashes, for SSO-only users and for duplicate providers.

• An SSO-only user (no password) crashes the change-email form. It routes through the
  unsafe Accounts.validate_current_password/2 (form_component.ex:28,113), which
  runs Bcrypt over a nil hash and raises ArgumentError. The password-change form
  handles nil safely; only change-email is exposed.
• The duplicate-identity case from item 2 also 500s the profile component on unlink
  (identities_component.ex:59-61). The duplicates collapse to the last one written
  in the UI, and the unlink button carries only phx-value-provider, so the lookup
  hits MultipleResultsError. Fixing item 2 covers this; keying the UI by identity id
  is the belt-and-braces part.

4. A GitHub public-profile email is trusted as verified without a re-check.

handler.ex:125-126 short-circuits on any binary userinfo["email"] and stamps
email_verified=true, never reaching the careful selection of a primary, verified
email in select_primary_verified_email. For GitHub that's the /user public email
your own test flags as unreliable (github_handler_test.exs:33-34).

Since email_verified gates the collision and auto-link safety property
(oidc_controller.ex:297-300), an unverified email can be treated as verified. It's
not blocking - the collision path doesn't link automatically, it shows a "sign in and
link" flash - but the fast path is worth tightening so it consults
select_primary_verified_email rather than trusting the raw field.

5. Disabling a provider locks out users who only have SSO.

Setting and unsetting the provider env vars is already the opt-in/opt-out, so there's
no need for a separate instance-level gate. But a user who only ever signed in through
that provider, and never set a password, is locked out if an operator disables it.
Worth a deliberate decision, and probably a note in DEPLOYMENT.md.

6. The email-collision check reads, then acts (a TOCTOU race).

The collision handling reads, then acts, without a unique_constraint(:email) on the
SSO registration changeset, so a race surfaces as a raw 500 rather than a clean
changeset error. The users.email citext unique index does exist, so this is about
the shape of the error rather than data integrity. Adding unique_constraint(:email)
to sso_registration_changeset turns the 500 into a proper changeset error.

7. Authorization for link/unlink/callback is inline only.

These rely on inline current_user / socket.assigns.user checks, and the OIDC
routes sit in the bare :browser pipeline with no formal policy. The inline checks do
work, so this isn't an open hole, but we'd like it moved into the Bodyguard policy
pattern for consistency with the rest of the app. (The real defect in the link flow is
the missing state binding from item 1, not this.)

8. Other confirmed concerns, lower priority, grouped for brevity:

• Handler discovery depends entirely on the cache warmer, so a cold start - or the
  warmer not having run - is a single point of failure.
• OAuth error and deny callbacks, and a raising get_userinfo, currently 500 rather
  than redirecting with a friendly message.
• The can_remove_identity? lockout check counts identities without a lock, so two
  concurrent unlinks of the last two identities can both pass. It's unlikely (it's
  self-inflicted and needs two sessions) and there's no concurrency test, but a row
  lock, or running the guard inside a transaction, would close it.
• The GitHub-specific email resolution lives in the generic Handler
  (handler.ex:119-164). Fine for two providers; worth factoring into a provider
  strategy before a third lands.

One product decision flagged (not a request to change code)

SSO-only login returns a distinct "this account uses single sign-on" message
(user_session_controller.ex:50-58), which lets someone enumerate which emails are
registered and SSO-only. This is intended UX per the forgot-password validation table,
so it's a conscious tradeoff rather than a bug. We're accepting it, and it shouldn't
block the PR.

I'm only flagging it because Lightning has no login rate-limiting, which is the usual
safety net here. We'll track a possible identifier-first login flow as separate work.
(Google, Microsoft, Auth0 and Okta all leak SSO routing by design, so we're in good
company, and OWASP has no enforceable control mandating generic responses.)

Notes and nits

• complete_signup clears the pending blob on error, forcing a full SSO restart
  instead of a retry (oidc_controller.ex).
• display_name uses String.capitalize, which mangles cased provider names
  ("GitHub" becomes "Github").
• UserIdentity.provider is free-form, with no inclusion validation or uid
  normalisation (user_identity.ex:19).
• The show/2 action looks effectively dead and isn't hardened (oidc_controller.ex).
• Partial provider config is silently disabled with no boot warning, which is easy to
  misconfigure quietly.
• A few test-coverage gaps worth filling: no SSO-only (nil-password) user factory; no
  test for the link callback when no link intent is set; no coverage for the SSO
  bootstrap config blocks or sso_redirect_uri; no would_lock_out concurrency test.
  (A test for a callback without state will fall out of fixing item 1.)

Finished my UX review, looking good! I directly made a couple of changes to improve the following:

1. Improved horizontal alignment of the "Sign in with X" buttons
2. Handle capitalisation logic directly in display_name function in lib/lightning/auth_providers. capitalize is used there and we explicitly define "GitHub" so that it isn't displayed as "Github"

A slight nitpick with the following UX, but shouldn't block the PR as it's an existing issue:

1. User has already logged in with SSO with GitHub or Google (or email..)
2. User tries to register with the same email
3. They see an error that says "This value should be unique"
   It would be better to have a more informational message here, such as "An account already exists with this email address". But this is a product decision and should be picked up in a separate issue.