Harden UserProvisioner: retry, inactivity guard, unverified adoption#9
Merged
Conversation
5 tasks
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.
bd2234b to
b40e24b
Compare
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.
This was referenced Jun 2, 2026
Fivell
added a commit
that referenced
this pull request
Jun 2, 2026
Follow-ups to PRs #9 and #10 surfaced by the code review. - Apply the same OmniAuth.config.path_prefix substitution to both install-generator templates. New installs were still shipping the hardcoded path PR #10 fixed in the live view. - Default the InactiveError key to :inactive when the model's inactive_message returns nil/blank. The empty key used to render an empty flash; now the user always sees a reason. - Stop leaking custom inactive_message symbols. If the host returns a symbol like :locked_by_admin with no devise.failure translation, the raw symbol used to land in a public flash. Fall back to devise.failure.inactive instead. The disabled-user request spec is consolidated via let helpers (auth_hash, disabling_hook, post_callback) and now uses new_admin_user_session_path / OmniAuth.config.path_prefix instead of hardcoded URLs. Adds a regression spec for the retry leg + an inactive winner row, documenting the expected redirect-and-no- warden-session behaviour.
Fivell
added a commit
that referenced
this pull request
Jun 2, 2026
#11) * Code-review followups: generator templates, inactive_message hardening Follow-ups to PRs #9 and #10 surfaced by the code review. - Apply the same OmniAuth.config.path_prefix substitution to both install-generator templates. New installs were still shipping the hardcoded path PR #10 fixed in the live view. - Default the InactiveError key to :inactive when the model's inactive_message returns nil/blank. The empty key used to render an empty flash; now the user always sees a reason. - Stop leaking custom inactive_message symbols. If the host returns a symbol like :locked_by_admin with no devise.failure translation, the raw symbol used to land in a public flash. Fall back to devise.failure.inactive instead. The disabled-user request spec is consolidated via let helpers (auth_hash, disabling_hook, post_callback) and now uses new_admin_user_session_path / OmniAuth.config.path_prefix instead of hardcoded URLs. Adds a regression spec for the retry leg + an inactive winner row, documenting the expected redirect-and-no- warden-session behaviour. * Drop review-process references from disabled-user spec comments
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.
Three security fixes for
UserProvisioner, surfaced by code review. Each one has a failing spec first, then the fix.1. on_login fires twice on a race
When two first sign-ins for the same
(provider, uid)happen at the same time (two browser tabs, two workers), the loser hitsRecordNotUniqueand the provisioner retries. The old retry path re-ranon_loginon the now-found row. If the host hook writes to an audit log, sends a webhook, or queues a welcome email — that all happens twice.The
@retriedflag set bysave!'s rescue is now read in#callto short-circuit right after the row is found:The hook ran once on the (discarded) in-memory build during the first attempt — that's enough.
2. Disabled users were written to the DB before being rejected
If the host's
on_loginflipsenabled = falseand returns truthy:The provisioner used to save the row anyway. The controller would then reject the sign-in via
active_for_authentication?— but the row was already written. Repeated attempts grew the table.Moved the check inside the provisioner, between the hook and
save!:3. Account adoption via unverified email
A legacy admin row (
ceo@example.com,provider: nil, uid: nil— seeded by DBA before SSO) could be claimed by anyone the IdP said owned that email. If the IdP doesn't verify email ownership (guest tenants, lax Keycloak realms, Zitadel without a verification policy), the attacker registersceo@example.comthere and walks off with the account.Now refuses adoption when the IdP explicitly marks the email as unverified:
Note the
== false— many IdPs never emitemail_verifiedat all (Zitadel, plain Keycloak). A strict!= truewould break them. So: block only when the IdP says "do not trust this", trust the host's IdP choice otherwise. If you want a strict opt-in mode, aconfig.require_verified_email_for_adoptionflag is a natural follow-up.Commits
Test plan
bundle exec rspec— 103/0bundle exec rake spec:engine— 7/0bundle exec rake spec:isolated— 5/0