Restore orphaned PR #10 merge: source SSO login path from OmniAuth.config.path_prefix#12
Merged
Conversation
Each spec is RED today and documents one production bug. The next commit implements the fixes and turns them green. 1. on_login runs twice when two first sign-ins race. The loser hits RecordNotUnique, the provisioner retries, and the host's hook double-fires (audit log, webhook, welcome email all duplicated). 2. A disabled-by-hook user still gets a row in admin_users. The active_for_authentication? check runs in the controller AFTER the provisioner has already persisted. Hostile attempts grow the table. 3. A pre-seeded admin row (provider/uid still nil) gets adopted by anyone the IdP says owns that email - even when the IdP marks the email as unverified. Attacker registers ceo@example.com at a lax IdP, signs in, walks off with the CEO account.
Fix the three bugs covered by the failing specs in the previous commit. 1. Don't re-run on_login on the RecordNotUnique retry path. The flag set by save!'s rescue is now read in #call to short-circuit after the row is found: return the winner's record without re-invoking the host hook. Side effects fire exactly once. 2. Check active_for_authentication? inside the provisioner, between on_login and save!. If the hook flipped an inactivity flag, raise ProvisioningError BEFORE writing the row. 3. Refuse to adopt a legacy admin row when email_verified is explicitly false. IdPs that don't ship the claim keep the old behaviour - many never emit it, so a strict "must be true" check would break legitimate hosts.
The previous commit moved the active_for_authentication? guard into
the provisioner, but the controller's generic ProvisioningError
rescue replaced the model's I18n-translated reason ("Your account
is not activated yet.") with the catch-all access_denied_message.
Disabled users lost the specific feedback.
Add InactiveError (a ProvisioningError subclass that carries the
inactive_message symbol) and rescue it separately in the callbacks
controller, translating via I18n.t("devise.failure.<symbol>"). The
generic rescue stays in place for every other denial path.
Also drop the now-dead active_for_authentication? check in the
controller - the provisioner runs the same guard earlier and never
returns an inactive user.
The login view hardcoded /admin/auth/oidc. Hosts that change Devise.omniauth_path_prefix (mounting Devise at a different scope, say) ended up with a button POSTing to a dead URL. Source the path from OmniAuth.config.path_prefix instead - it's the single Rack-level value of where OmniAuth actually listens, and Devise keeps it in sync with its own omniauth_path_prefix. Both view branches (AA v3 form_tag and AA v4 button_to) updated. Why not omniauth_authorize_path? Tried that first - it breaks in the isolated engine case: Devise's helper resolves through engine routes and the engine mount path gets prepended, producing /admin/admin/auth/oidc. OmniAuth.config.path_prefix is plain Rack and sidesteps the issue. Regression spec stubs OmniAuth.config.path_prefix to a sentinel and asserts the form action follows it. The spec's before block force- loads routes first - on Rails 8 with lazy route loading, the stub would otherwise still be active when devise_for runs, tripping Devise's "path_prefix must match Devise.omniauth_path_prefix" guard.
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
1 participant
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.
Restores the orphaned merge of PR #10 (#10).
PR #10's base was set to
high-risk-failing-testsso it could stack on top of #9. PR #9 merged to main at 14:40:29 (and its branch was deleted shortly after), but PR #10 was merged into the still-existinghigh-risk-failing-testsbranch at 14:40:46. So PR #10's commit ended up on a branch that never made it back to main.This PR brings that content (the view + the regression spec) straight onto main.
What's in the diff
The login view now sources the form action from
OmniAuth.config.path_prefix(Rack-level source of truth) instead of hardcoding/admin/auth/oidc. The hardcoded literal broke hosts that customiseDevise.omniauth_path_prefix(different mount path, different sub-prefix for SSO) — their SSO button POSTed to a dead URL.The companion spec stubs
OmniAuth.config.path_prefixto a sentinel and asserts the rendered form action follows it.Test plan
bundle exec rake spec:all— full suite green on this branch