Epic: Single Sign On#4751
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4751 +/- ##
=======================================
+ Coverage 90.3% 90.5% +0.2%
=======================================
Files 442 444 +2
Lines 22540 22765 +225
=======================================
+ Hits 20353 20605 +252
+ Misses 2187 2160 -27 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
doc-han
requested review from
brandonjackson,
elias-ba,
josephjclark,
midigofrank and
theroinaochieng
and removed request for
josephjclark
|
Now I have enough information to perform the security review. This PR adds SSO functionality (GitHub/Google OAuth providers, identity linking/unlinking, signup confirmation). Let me assess each check: S0 (project scoping): The PR touches only user accounts, identities, sessions, and OAuth flows — none of which are project-scoped resources (no work with workflows, runs, dataclips, work orders, collections, project credentials, triggers, edges, or jobs). New queries ( S1 (authorization): New web entrypoints check authorization appropriately:
S2 (audit trail): Per the agent guidance, S2 applies to project/instance configuration changes. This PR modifies user-level account state (registration, identity linking) — analogous to existing account operations (signup, password change) which do not write to Security Review ✅
|
midigofrank
left a comment
midigofrank
left a comment
There was a problem hiding this comment.
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) vsGITHUB_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_SECRETare already documented (DEPLOYMENT.md L363-377, "Google Oauth2") for the existing DB-backed auth-provider path, whose callback is/authenticate/callback. The newGoogleHandlerreuses 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
endmodify/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/1is defined identically inOidcControllerandIdentitiesComponent;link_identityinAccountHookduplicatesAccounts.link_user_identity/3. Consider consolidating.CacheWarmer.execute/1'stry/rescue _ -> []is very broad and will swallow genuine config errors silently.GithubHandlerhas no test file (GoogleHandlerdoes) — given #1, a GitHub-specific userinfo test would be valuable.- Changelog not updated (PR checklist acknowledges this).
theroinaochieng
requested review from
lmac-1
and removed request for
brandonjackson and
theroinaochieng
Replace invalid button-wrapping-anchor pattern with button_link, and swap inline-flex/inline-block/align-middle mix for flex items-center justify-center to fix icon and text baseline alignment.
Add explicit display_name/1 clauses for github and google so all provider name display sites show GitHub/Google rather than relying on String.capitalize. Route confirm_signup template through display_name/1 to match the rest.
lmac-1
left a comment
lmac-1
left a comment
There was a problem hiding this comment.
Finished my UX review, looking good! I directly made a couple of changes to improve the following:
- Improved horizontal alignment of the "Sign in with X" buttons
- Handle capitalisation logic directly in
display_namefunction inlib/lightning/auth_providers.capitalizeis 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:
- User has already logged in with SSO with GitHub or Google (or email..)
- User tries to register with the same email
- 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.
There was a problem hiding this comment.
Hey @doc-han, morning! Went through this one properly and honestly it's really solid work, the SSO flow is well thought out. The way you handle the login state is genuinely better than what I usually see, and I like that you check the email is verified before creating any account, and that a matching email doesn't get silently linked. Nice.
I spent a couple of focused hours on it, mostly checking whether any of this touches our OAuth credentials, the Google Sheets and Salesforce connections that jobs use, since that was my main worry going in. Good news, it doesn't. Those connections use the OAuth clients we set up in the UI and a different callback URL, and none of that is touched here. I also checked the modules you removed and confirmed they were already dead code with no live references on main, so that's a clean cleanup.
Couple of things I'd like us to sort before this goes live though, mostly so we start on the strongest footing:
-
The Google section in
DEPLOYMENT.mdis a bit misleading. It saysGOOGLE_CLIENT_IDandGOOGLE_CLIENT_SECRETare also used by the older Google setup and tells people to register both callback URLs on the same client. But I couldn't find anywhere in the code on main that actually reads those two env vars, so that section looks like it's documenting something that isn't wired up anymore. On top of that the doc never really separates the credential connections from SSO, so someone who has a Google app for Sheets might read this and think they need to go change it, when they really don't. Could you either fix or drop that stale section, and make it clear that the credential connections are set up in the UI and aren't affected, and that the only new callback to add is the SSO one, and only if you turn SSO on? -
There's a behavior change for anyone already using SSO that needs a note in the changelog. Login used to match people by their email, now it needs a linked identity record, and there's no migration to backfill those. So anyone who used to sign in through their provider won't get logged in straight through anymore, they'll have to re-link from their profile once. If that's intended, and I think it's the right call for security, let's just call it out so self hosted folks aren't caught off guard.
Two smaller ones, not blockers:
-
When you unlink a provider, the check for "do they still have another way to log in" and the actual delete aren't done as one operation. In theory a password-less user clicking unlink twice at once could remove both and lock themselves out. Probably worth tightening that up.
-
In the email resolution code, when GitHub gives you back an email you mark it as verified without checking. It's fine because GitHub only exposes verified emails publicly, but a one line comment saying that would save the next person a double take.
Happy to jump on a call if any of it's unclear.
Thanks @elias-ba,
|
1 similar comment
Thanks @elias-ba,
|
|
This PR adds Single Sign-On sign-in to Lightning, letting users authenticate with GitHub or Google instead of (or alongside) an email and password. It also lets users link and unlink providers from their profile. SSO sign-ups flow through the same account-creation path as password sign-ups, so provisioning stays consistent across both methods. User-facing features
Design
DB Changes
Security
cc: @stuartc |
The SSO callback fetched the provider email before branching on login-vs-link, so a GitHub user with a private email was blocked from linking even though the link path never uses the email. Move email resolution into the login path and assert linking succeeds without one.
get_userinfo/2 used the bang OAuth2.Client.get!, so a provider-side HTTP error during the userinfo step raised out of the SSO callback and became a 500. Switch to the non-bang get/2 and return a tagged tuple, matching get_token/2, so the controller redirects with "Authentication failed".
maybe_resolve_email stamped email_verified: true whenever userinfo carried an email, asserting a verification status the userinfo endpoint never provides. Consult the authoritative /user/emails endpoint instead and set email_verified from the address's actual verified flag, failing closed when the endpoint can't be reached.
A Repo.delete failure during unlink rolled back as :not_linked, so the user was told the identity wasn't linked when it was — the deletion just failed. Roll back with :delete_failed so it surfaces the accurate "could not unlink" message instead.
can_remove_identity? counted all other identities just to test for at least one. Repo.exists? short-circuits with a LIMIT 1 and matches the idiom used elsewhere in the module.
6 participants
Description
This PR Implements the Full SSO Experience epic.
AccountHookas password sign-ups. ***Closes #4621
Validation steps
Setup
<host>/authenticate/<provider>/callbackwhere isgoogleorgithubfor now.GITHUB_CLIENT_IDandGITHUB_CLIENT_SECRET.Validation
Sign up via SSO
Sign in via SSO
Email collision
you@domain.com+ SSO sign-up with same emailuser_identitiesrow createdLink from
/profileUnlink from
/profileForgot password (SSO-only)
AI Usage
Please disclose whether you've used AI anywhere in this PR (it's cool, we just
want to know!):
You can read more details in our
Responsible AI Policy
Pre-submission checklist
/reviewwith Claude Code)
(e.g.,
:owner,:admin,:editor,:viewer)