Auth/PM-3813 - 2FA Management Endpoints - User Verification Refactor#7841
Draft
JaredSnider-Bitwarden wants to merge 23 commits into
Draft
Auth/PM-3813 - 2FA Management Endpoints - User Verification Refactor#7841JaredSnider-Bitwarden wants to merge 23 commits into
JaredSnider-Bitwarden wants to merge 23 commits into
Conversation
Introduces a new tokenable bound to UserId + ProviderType for the upcoming per-provider 2FA flow, alongside its factory, DI registration, and configurable lifetime. Pure addition with no behavior change to existing flows. - TwoFactorUserVerificationTokenable + ITwoFactorUserVerificationTokenableFactory + factory implementation - Unit tests for tokenable and factory - IGlobalSettings.TwoFactorUserVerificationTokenLifetimeInMinutes (default 30) - DI registration for the data protector + tokenable factory
Per-provider disable request models for the upcoming DELETE endpoints, a wrapper response model for the WebAuthn challenge, and a UserVerificationToken field on the existing response models and PUT request models that will carry the new replay token. Pure addition — no existing wire contracts narrow here.
…endpoints Replace CheckAsync with three single-purpose helpers (ValidateUserBySecretAsync, ValidateUserVerificationTokenAsync, ValidateUserHasPremiumAsync) plus a MintProtectedUserVerificationToken helper. Each call site composes the guards it needs explicitly. Other changes in this refactor: - New per-provider DELETE endpoints: DisableYubiKey, DisableDuo, DisableEmail, DisableOrganizationDuo - GetWebAuthnChallenge returns the new TwoFactorWebAuthnChallengeResponseModel wrapper so a UV token can travel with the FIDO2 options - DisableAuthenticator hardcodes TwoFactorProviderType.Authenticator - GetYubiKey and GetDuo no longer gate on premium; lapsed-premium users can read their own configuration and use the standard GET -> DELETE flow - Legacy PutDisable and PutOrganizationDisable endpoints removed; per-provider DELETEs replace them - Inline Task.Delay calls dropped from rewritten methods; rate limiting belongs at the edge - Unit test coverage extended: Goal-7 non-premium GET assertions, per-endpoint validator negative paths through PutDuo, organization NotFound branches for both PutOrganizationDuo and DisableOrganizationDuo, and the DisableOrganizationDuo happy path
The Authenticator PUT and DELETE request models inherited fields they no longer read (Secret, MasterPasswordHash, Type). Rewrite both as standalone classes carrying only the fields the controller actually uses. With those two models no longer inheriting it, TwoFactorProviderRequestModel has no remaining consumers and is removed. - UpdateTwoFactorAuthenticatorRequestModel: standalone with Token, Key, UserVerificationToken - TwoFactorAuthenticatorDisableRequestModel: standalone with UserVerificationToken, Key - TwoFactorProviderRequestModel: deleted - Integration tests updated to stop referencing the dropped fields
Drop the unused third parameter from the VerifySecretAsync signature and inline the remaining conditional returns. Existing callers all pass the two-argument form so no downstream changes are required; the existing VerifySecretAsync_Works theory continues to cover password and OTP paths.
Adds end-to-end coverage for the GET, PUT, and DELETE paths on every 2FA provider: - Per-provider GET round-trip tests that mint via the controller and replay the resulting token against the matching DELETE (or PUT for the WebAuthn challenge endpoint), verifying the token round-trip survives DI + wire serialization - Per-provider PUT happy paths exercising the token-replay chain end-to-end - SendEmail invokes the email service when given a valid token - DisableAuthenticator_BodyTypeMismatch_RespectsUrlRoute confirms the URL is the sole source of provider truth and any Type field in the body is dropped by deserialization - DisableYubiKey_CrossProviderToken_BadRequest confirms the validator's provider-type binding rejects cross-provider replay
JaredSnider-Bitwarden
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7841 +/- ##
==========================================
+ Coverage 61.25% 61.56% +0.30%
==========================================
Files 2193 2219 +26
Lines 97296 97856 +560
Branches 8767 8819 +52
==========================================
+ Hits 59601 60247 +646
+ Misses 35582 35460 -122
- Partials 2113 2149 +36 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the 2FA management endpoint refactor that moves per-provider GET → PUT/DELETE flows onto the tokenable user-verification framework. Focused on the new Code Review DetailsNo blocking findings. Notes from analysis (no action required):
|
Per-provider DELETE /two-factor/webauthn/all that mirrors the legacy generic disable behavior for WebAuthn. The existing per-credential DELETE /two-factor/webauthn refuses to remove the last registered credential by design, so a bulk path is required for users who want to disable WebAuthn entirely. Unit and integration coverage included.
…points Mirror the five-test pattern (valid token, expired token, TryUnprotect failure, token bound to different user, token bound to different provider) across DisableYubiKey, DisableDuo, and DisableEmail. Brings the non-Authenticator per-provider DELETE endpoints up to the same unit-test shape recently added for DisableWebAuthnAll.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Per-provider 2FA endpoints reached via HTTP DELETE were named DisableX on the controller. The underlying behavior is a hard removal of the provider configuration, not a reversible disable, so the method and request-model names now read as DeleteX to match what the code actually does. Scope is limited to the controller surface and its request models. The underlying service methods (IUserService / IOrganizationService) keep their historical DisableTwoFactorProviderAsync names but gain XML doc comments clarifying that they hard-delete the provider configuration.
TwoFactorWebAuthnDeleteRequestModel (also the parent of TwoFactorWebAuthnRequestModel for PUT) no longer inherits SecretVerificationRequestModel, dropping the inherited secret-required validation that was masking the token-only flow for PUT /two-factor/webauthn and DELETE /two-factor/webauthn. UserVerificationToken is now [Required] at the model layer, matching the other per-provider request models. Both WebAuthn integration tests now exercise the token-only path end-to-end.
UpdateTwoFactorDuoRequestModel and UpdateTwoFactorYubicoOtpRequestModel no longer inherit SecretVerificationRequestModel. Each model is now standalone with [Required] UserVerificationToken, matching the per-provider request model shape used elsewhere in this controller. Email integration tests for /send-email and PUT /email drop redundant MasterPasswordHash payloads so they exercise the token-only flow cleanly.
The class was orphaned in cb1db26 (2025-09-02, PM-18179) when the pm-17128-recovery-code-login feature-flag cleanup removed its sole consumer (the PostRecover controller action and the matching IUserService.RecoverTwoFactorAsync overload). No references remain in either the server or clients repos.
TwoFactorEmailRequestModel (previously the shared body for the anonymous
login endpoint, the authenticated setup-send endpoint, and the
authenticated PUT setup endpoint via inheritance) splits into three
purpose-specific models:
- TwoFactorEmailLoginRequestModel (login flow, secret-based)
- TwoFactorEmailSetupRequestModel (setup-send, token-only)
- UpdateTwoFactorEmailRequestModel (setup-update, inherits the setup-send
shape and adds the OTP)
The setup pair carry the token-based authentication shape and share the
ToUser mutation; the login model keeps only the credentials its endpoint
consumes. Setup models gain [Required] enforcement on Email and
UserVerificationToken at the model layer.
Unit and integration tests added for SendEmail, PutEmail, and GetEmail
(mirroring the DeleteEmail patterns), plus the [Required] regression
guards on the setup wire shapes, plus a master-password happy-path and
validator regression guard for the login endpoint.
The previous summary opened with "Single-use proof", which contradicts the immediately following clause about replay within the token's lifetime. Reworded as "Time-limited proof" so the two clauses align.
ReadJsonRootAsync now owns the JsonDocument lifetime via a using declaration, returning the cloned root element. Callers no longer need to track the document to dispose it.
… DeleteWebAuthn Adds the 5-test user-verification token validation matrix that already exists for the DeleteEmail / DeleteAuthenticator / etc. actions: expired token, TryUnprotect fail, cross-user binding, cross-provider binding, and a valid-token happy path. Each happy path also verifies the appropriate downstream command or service invocation.
…ections Reorders the existing tests into contiguous provider blocks mirroring the integration test file's layout, with banner comments delimiting each section: controller-helper tests, Authenticator, YubiKey, Duo, Organization Duo, WebAuthn, Email, and private helpers. No tests or helper bodies change.
Documents the per-provider GET → PUT/DELETE flow, the TwoFactorUserVerificationTokenable's bindings and lifetime, the validation rules ValidateUserVerificationTokenAsync enforces, and why Authenticator continues to use its own Key-bound tokenable. Structured so additional 2FA aspects can be added as sibling sections.
Aligns every 2FA request-model class to the file-wide TwoFactor<Provider> prefix already used by the Delete family: UpdateTwoFactorAuthenticatorRequestModel -> TwoFactorAuthenticatorUpdateRequestModel UpdateTwoFactorDuoRequestModel -> TwoFactorDuoUpdateRequestModel UpdateTwoFactorYubicoOtpRequestModel -> TwoFactorYubiKeyUpdateRequestModel UpdateTwoFactorEmailRequestModel -> TwoFactorEmailUpdateRequestModel TwoFactorWebAuthnRequestModel -> TwoFactorWebAuthnUpdateRequestModel The YubiKey rename also aligns the class name with the TwoFactorProviderType enum value (YubiKey, not YubicoOtp). No HTTP route or wire-shape changes.
…nces
Earlier work renamed the user-validation helper to ValidateUserBySecretAsync
and inlined the per-action organization access check, but the unit tests
still carried the old names:
CheckAsync_* -> ValidateUserBySecretAsync_*
CheckOrganizationAsync_* -> GetOrganizationDuo_* (and moved into the
Organization Duo section, alongside the
matching Put/Delete tests)
SetupCheckOrganizationAsyncToPass -> SetupOrganizationAccessToPass
The CheckOrganizationAsync helper no longer exists; the two tests that
referenced it are GetOrganizationDuo tests exercising the same
ManagePolicies / GetByIdAsync paths that the Put and Delete variants
already cover.
- Mark the 3 GetUserTwoFactorXProvidersJson / GetOrganizationTwoFactorDuoProvidersJson helpers and SetupOrganizationAccessToPass as `static` to match the rest of the helper methods in the file. - Generalize the comment in SetupGetUserByPrincipalAsync — every action that calls model.ToUser(user) needs TwoFactorProviders cleared, not just PutAuthenticator.
|
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.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-38137
📔 Objective
To refactor our 2FA management endpoints to use our tokenable framework for ongoing user verification (previously proven by its adoption for authenticator). The GET endpoints require normal user verification via secret which then mints a user verification tokenable which can be presented to the PUT or DELETE endpoints for changing the data.
🎨 : Rename -
Disablewas changed toDeleteeverywhere applicable as Disable implies a soft delete and we actually hard delete all 2FA providers when they are "turned off"🎨 : Rename - Aligned all models to use consistent naming convention.
API surface changes
DELETE /two-factor/{yubikey,duo,email,authenticator,webauthn/all}andDELETE /organizations/{id}/two-factor/duo.PUT /two-factor/disableandPUT /organizations/{id}/two-factor/disable, plus their[Obsolete]POST companions.DELETE /two-factor/webauthn/allbulk endpoint exists for a specific reason: the per-credentialDELETE /two-factor/webauthnrefuses to remove the last registered credential (lockout-prevention rule inDeleteTwoFactorWebAuthnCredentialCommand). Without/all, a user with exactly one WebAuthn credential would have no way to disable WebAuthn entirely.Behavior changes
GET /two-factor/yubikeyandGET /two-factor/duo. The matching PUT endpoints still require premium. This lets lapsed-premium users read their own enrollment configuration and use the standardGET → DELETEflow to remove a provider they previously configured.Test coverage
📸 Screenshots
See clients PR: bitwarden/clients#21385