Revert SSO Epic - #5032
Merged
Merged
Conversation
Reverts the Single Sign-On epic so it isn't part of the incoming release and can get a proper security review first. Reopens #4751 / #4621. The SSO commit (8038271) can't simply be reverted: the v2.17.0 security release was merged on top of it (48874fb), reshaping the same auth files again and porting v2.17.0's OIDC hardening onto SSO's architecture. So the revert target for the auth cluster is the v2.17.0 tag, which holds a complete, SSO-free, already-hardened version of it. 43 of the 51 SSO files had no other commits since that tag, so restoring them from v2.17.0 *is* the revert and brings the security release back verbatim. v2.17.0's security release is untouched. To confirm: git diff v2.17.0 HEAD -- lib/lightning/auth_providers \ lib/lightning_web/controllers/oidc_controller.ex \ lib/lightning/accounts.ex lib/lightning/accounts/user.ex \ ':!lib/lightning/auth_providers/common.ex' \ ':!lib/lightning/auth_providers/google.ex' \ ':!lib/lightning/auth_providers/salesforce.ex' \ ':!lib/lightning/auth_providers/oauth_behaviour.ex' is empty. The generic-OIDC (AuthConfig) id_token verification, nonce binding and email_verified checks are all back to exactly what v2.17.0 shipped. Kept deliberately, rather than reverted: - The dead-code removal of auth_providers/{common,google,salesforce, oauth_behaviour}.ex and Config.oauth_provider/1. That was an independent cleanup (#4825) bundled into the SSO PR, and nothing references any of it. - The credential_live_test.exs hunk that dropped Common.TokenBody, since Common stays deleted. All three SSO migrations are deleted outright with no down-migration; they only ever ran on local and CI databases. A fresh setup now yields the pre-SSO schema (no user_identities, users.hashed_password back to NOT NULL). Anyone whose local database already ran them needs the cleanup notes on the PR.
Follows the SSO revert. - CHANGELOG: remove the SSO entry from [Unreleased]. [2.17.0] is untouched. - .env.example / DEPLOYMENT.md: remove the SSO_GITHUB_*/SSO_GOOGLE_* block and the "Single Sign-On (SSO)" section. The SSO PR had *replaced* the old "Google Oauth2" / "Salesforce Oauth2" sections with a new "OAuth credential connections" section explaining that GOOGLE_CLIENT_ID and SALESFORCE_CLIENT_ID are no longer read and that OAuth clients are now registered in the UI. That replacement is accurate and unrelated to SSO, so it stays — only its cross-reference to the SSO section is removed. Restoring the originals would have re-published setup instructions for environment variables nothing reads.
|
Based on my review, this PR is a revert of the SSO epic. It removes SSO-related routes, controllers, LiveComponents, migrations, user identities, and helper functions. There are no additions of new project-scoped queries, new authorization-gated actions, or new config-resource writes. All three checks are N/A. Security Review ✅
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5032 +/- ##
=======================================
+ Coverage 90.5% 90.7% +0.2%
=======================================
Files 425 420 -5
Lines 20233 19953 -280
=======================================
- Hits 18308 18095 -213
+ Misses 1925 1858 -67 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR reverts the Single Sign-On epic (#4751) so it isn't part of the incoming release, giving it time for a proper security review before it ships.
Why this isn't a plain
git revertSSO landed as one squashed commit (
8038271d03), and then the v2.17.0 release was merged on top of it (48874fb308). Both branches had independently rewritten the same auth code, so that merge picked SSO's architecture for the auth cluster and then ported v2.17.0's OIDC hardening onto it. Reverting the SSO commit directly would conflict in exactly the files the merge reshaped, and resolving those by hand risks silently reinstating pre-v2.17.0 (unhardened) auth code.Instead, the revert target for every auth file is the
v2.17.0tag, which holds a complete, SSO-free, already-hardened version of the whole auth cluster. 43 of the 51 SSO files have no commits other than the SSO commit and the merge since that tag, so restoring them fromv2.17.0is the revert, and it brings the security release back verbatim rather than re-deriving it. Only 8 files needed hand edits.Kept deliberately, rather than reverted
auth_providers/{common,google,salesforce,oauth_behaviour}.exandConfig.oauth_provider/1. That was an independent cleanup (refactor: remove unused modules #4825) that happened to be bundled into the SSO PR; nothing inlib/ortest/references any of it, so restoring it would re-add dead code.credential_live_test.exshunk that replacedCommon.TokenBody.new/1with a plain map match, sinceCommonstays deleted.DEPLOYMENT.md's "OAuth credential connections" section. The SSO PR didn't just add SSO docs — it replaced the old "Google Oauth2" / "Salesforce Oauth2" sections with a new, accurate section explaining thatGOOGLE_CLIENT_IDandSALESFORCE_CLIENT_IDare no longer read and that OAuth clients are now registered in the UI. That replacement is correct and unrelated to SSO, so it stays; only its cross-reference to the SSO section is removed. Restoring the originals would have re-published setup instructions for environment variables nothing reads.Migrations
All three SSO migrations are deleted outright, with no down-migration. They only ever ran on local and CI databases — there is no deployed schema to roll back. A fresh setup now produces the pre-SSO schema: no
user_identitiestable, andusers.hashed_passwordback toNOT NULL.Deleting the files (rather than reversing them) also means they'll re-run cleanly at their original timestamps when SSO re-lands.
Your local database still has the SSO schema, and any account you created via SSO has no password — after this revert there is no password-less login path, so that account is locked out.
Simplest fix:
To keep your existing data instead, in
psql lightning_dev:To keep a password-less account rather than delete it, give it a password before step 1: use "Forgot your password?" on
/users/log_inand open the reset email athttp://localhost:4000/dev/mailbox. Steps 2–3 will then succeed with no deletions.Validation steps
git diff v2.17.0 HEAD -- lib/lightning/auth_providers lib/lightning_web/controllers/oidc_controller.ex lib/lightning/accounts.ex lib/lightning/accounts/user.exshould show nothing but the four deliberately-deleted dead modules listed above.git grep -In "user_identities\|hashed_password: nil\|SSO_GITHUB\|SSO_GOOGLE\|UserIdentity\|link_user_identity" -- lib test priv configreturns nothing.mix ecto.resetthenmix compile --force --warnings-as-errors— should be clean, and the resulting schema should have nouser_identitiestable.mix test test/lightning/accounts_test.exs test/lightning/auth_providers/ test/lightning_web/controllers/oidc_controller_test.exs test/lightning_web/controllers/user_session_controller_test.exs test/lightning_web/live/profile_live_test.exs test/lightning_web/live/reauthenticate_live_test.exs test/lightning_web/live/project_live_test.exs/users/log_inshows no SSO buttons and password login works (including the "disabled" and "scheduled for deletion" messages);/authenticate/githuband/authenticate/signup/confirmboth 404 without crashing;/profilerenders with no "Linked accounts" section and password change works; revealing a webhook auth method secret still prompts for password/2FA.Additional notes for the reviewer
Two SSO dependencies that greps didn't catch — both were found by the test suite, so they're worth knowing about if you're reviewing the completeness of the revert:
test/lightning/auth_providers/auth_providers_test.exsassertedHandler.authorize_url/2(SSO's params-based arity) against v2.17.0'sauthorize_url/3. It is not part of the SSO commit — the merge adapted it. Enumerating the reverted commit's file list isn't sufficient; the correct scan looks repo-wide for files whose only commits since thev2.17.0tag are those two SHAs.test/lightning_web/live/project_live_test.exs(describe "webhook-security") exercise the password-less webhook-secret-reveal path. They contain no SSO identifier at all — they set upinsert(:user, hashed_password: nil, ...).hashed_password: nilturned out to be the real fingerprint of SSO-dependent test setup. Both were removed. (These tests originally lived intrigger_test.exs; the legacy-editor sunset feat: sunsetting legacy editor #4908 moved them intoproject_live_test.exs.)Test results: the auth surface is 262 tests / 0 failures, adjacent suites (credential, user, version_control, registration, auth_providers_live) 164 / 0. The full suite is 5188 tests, 1 failure, and that one failure is an environment artefact, not a regression:
test/integration/web_and_worker_test.exs:119asserts the reported Node version starts with24.18, and I'd been running with a staleASDF_NODEJS_VERSION=24.15.0override. It passes with the repo default from.tool-versions(nodejs 24.18.0).mix format,mix compile --warnings-as-errors,mix credo --strict --allandmix sobeloware all clean.Bringing it back.
git revertof this PR's revert commit on a branch offmainrestores everything — the code is never lost, so nothing needs parking. Three things make it more than a revert, though: the three migrations come back at their original timestamps, which by then will be older than migrations already applied in production (Ecto runs them anyway but warns, and a latermix ecto.rollbackwould then revert the wrong migration — so re-stamp them); the security review has to actually happen; and the revert restores the merged hybrid (SSO's env-provider userinfo path running alongside v2.17.0's id_token-verifyingAuthConfigpath), which is the architectural split worth collapsing deliberately rather than restoring as-is. Full re-land procedure is written up alongside the plan in the shared context folder.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) — n/a for a revert; thisrestores v2.17.0's authorization behaviour verbatim, covered by its own tests