Recovery-factor verification: "verified" means the recovery factor, email is the special case#8
Merged
Merged
Conversation
…eaking: public AuthHooks surface)
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.
Recovery-factor verification: "verified" means the recovery factor, email is the special case
PR 1 made identity and recovery shape model-driven (
recovery="email" | "phone" | None), butverifiedstayed email-specific:current_user(verified=True)checkedemail_verified, and configuring that gate without an email column raised. This PR generalizes "verified" to mean "the contract's recovery factor is proven controlled," so a phone-recovery app verifies by SMS, withemail_verifiedbecoming the special case where the factor is email. The proof flow is the same signed-one-time-token machinery, re-pointed at the factor and its channel; the security core is untouched; and the public surface is renamed so its names tell the truth. This is PR 2 of 2, closing the identity-and-recovery arc; the existing email suite passes unchanged.Verification is proof of control of the recovery factor
Before, "verified" was hardcoded to email:
repo.email_verified(user), theemail_verifiedcolumn, and a gate that raised at construction unless the model had an email column. That left a placeholder seam: a phone-recovery app declaredrecovery="phone"(PR 1) but had no way to verify the phone.Now the repository resolves the factor:
recovery_verified(user)readsemail_verifiedwhen the recovery factor is email and{factor}_verifiedotherwise (recovery=Noneis never verified);mark_recovery_verified(db, user)writes the right column.Principalgainsrecovery_verified(populated bybuild_principal) alongside the keptemail_verified.current_user(verified=True)now gates onrecovery_verifiedand raises at construction only whenrecovery is None(an account shape with nothing to prove control of), not when email is absent. For an email-recovery app this is identical to before; for a phone app it finally works.Why this shape: the contract already knows the recovery factor (
identity.recovery), so "verified" should mean that factor is proven, not "an email is proven."email_verifiedstays the column for email apps (back-compat), and the concept generalizes above it.The proof is the same machinery, re-pointed (security core untouched)
email_verified=Truewas always backed by a real proof: the user returned a signed, one-time-use, TTL'd token delivered to the address being verified. Generalizing had to preserve that exactly, or it would be a silent downgrade to "some channel said OK."It does. The redemption core is byte-for-byte unchanged:
verify_signed_token,_consume(one-time-use), and the TTL are not touched. The only diffs inconfirm_*are the verified read/write switching torecovery_verified/mark_recovery_verified. On the request side, the token is now looked up by, and delivered to, the recovery factor:DeliveryIntent.recipient = repo.get(user, identity.recovery). This is where PR 1's deferred "recipient is still email" gap closes, for both the verify and the password-reset flows (reset also delivers to the factor).change_emailstays email-shaped (it intrinsically proves a new email address).The verified flag is as unsettable as
email_verifiedalways wasemail_verifiedwas protected on every write path because it is aLOGICAL_FIELDSmember. A non-email{factor}_verifiedcolumn is not, so it had to be gated explicitly or it would be a settable-without-proof downgrade.repo._recovery_verified_col()is unioned into both_gated_names(the register allowlist) and_contract_names(the provisioning filter), so{factor}_verifiedis dropped on all three write paths: register (even if opted intoregister_extra_fields),new_user_defaults(gated at construction), andnew_user_fields(gated viafilter_provisioning_data). The only way to set it remains returning the delivered token.The mixin emits
{factor}_verified; names made factor-neutralmake_auth_identity(recovery="phone")now emits aphone_verifiedbookkeeping column (NOT NULL, defaultFalse);recovery="email"does not double-emit (the always-presentemail_verifiedis reused);recovery=Noneemits no extra flag. The app still declares the factor column itself (phone, with its own constraints), the same as any app column.With the behavior generalized, the names were renamed so the surface stops lying for non-email apps. The
verify_emaildelivery kind is kept for email recovery (so existingEmailSenderimplementations that switch on it are unaffected) and a factor-neutralverify_recoveryis emitted for any other factor, so a channel never gets an "email"-named verify for a phone. The service methods becamerequest_recovery_verification/confirm_recovery_verification, and the public hookon_after_email_verifiedbecameon_after_recovery_verified. Names that are genuinely email-specific kept their names:on_after_email_changedand OAuth'semail_verified(both prove an actual email, per the convention's test), and the/verify-emaildefault path (user-facing config).Documentation Updates
docs/guides/infra/hooks.md:on_after_email_verifiedbecomeson_after_recovery_verified("recovery-factor verification confirm").docs/guides/accounts/email.md:on_after_recovery_verifiedinstead ofon_after_email_verified.(The API reference page renders
AuthHooksvia mkdocstrings, so the renamed field updates automatically.)Test Plan
Automated
uv run ruff check crudauth tests— cleanuv run mypy crudauth tests --config-file pyproject.toml— clean (100 files)uv run pytest— 294 passed (the pre-existing email suite unchanged, the back-compat anchor)Verification is proof of the recovery factor
test_phone_verify_requires_the_delivered_token)recovery_verifiedreads the right column per factor: email →email_verified, phone →phone_verified,recovery=None→ always False (test_recovery_verified_reads_right_column_per_factor)recovery_verifiedequalsemail_verified(test_email_recovery_equals_email_verified)current_user(verified=True): raises at construction forrecovery=None; 403 unverified then 200 after verifying (test_verified_gate_requires_recovery_factor,test_phone_verify_delivers_to_phone_and_gates_on_it)Delivery re-pointed to the factor
verify_recoverykind (test_phone_verify_delivers_to_phone_and_gates_on_it)test_phone_reset_delivers_to_the_phone)The flag is unsettable on every write path
{factor}_verifieddropped from register (even opted in) and provisioning (test_factor_verified_unsettable_via_register_and_provisioning){factor}_verifieddropped fromnew_user_defaultsat construction (test_factor_verified_unsettable_via_new_user_defaults)Mixin factory
recovery="phone"emitsphone_verified;recovery="email"does not double-emit;recovery=Noneemits no extra flag (test_factory_*)Dependencies
None new.
Breaking Changes
AuthHooks.on_after_email_verifiedrenamed toon_after_recovery_verified(and the internalrun_after_email_verifiedtorun_after_recovery_verified). Public surface: an app registeringAuthHooks(on_after_email_verified=...)must rename the kwarg. Done as a real rename, not a deprecated alias, deliberately, while the breaking-change cost is zero (pre-prod, no overriders).on_after_email_changedis unchanged (it proves an actual new email).EmailFlowService.request_email_verification/confirm_email_verificationrenamed torequest_recovery_verification/confirm_recovery_verification, andrequest_email_verification/request_password_resetnow take avalueparameter (the recovery-factor value) instead ofemail.EmailFlowServiceis constructed byCRUDAuthinternally, so most apps are unaffected; direct callers must update.current_user(verified=True)semantics generalized. It now gates onrecovery_verifiedand raises at construction only forrecovery=None(previously: required an email column). Email-recovery apps behave identically; a non-email app that previously hit the placeholder raise now works.UserRepository(recovery=...)parameter andrecovery_verified/mark_recovery_verifiedmethods (defaults preserve email behavior);Principal.recovery_verifiedfield (email_verifiedkept); theverify_recoveryvalue added toEmailKind(email recovery still emitsverify_email);make_auth_identityemits{factor}_verifiedonly for non-email factors (the default shape is unchanged).