Auth/PM-3813 - 2FA Management Endpoints - User Verification Refactor#7841
Open
JaredSnider-Bitwarden wants to merge 48 commits into
Open
Auth/PM-3813 - 2FA Management Endpoints - User Verification Refactor#7841JaredSnider-Bitwarden wants to merge 48 commits into
JaredSnider-Bitwarden wants to merge 48 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.31%
==========================================
Files 2193 2241 +48
Lines 97296 98182 +886
Branches 8767 8849 +82
==========================================
+ Hits 59601 60450 +849
- Misses 35582 35596 +14
- Partials 2113 2136 +23 ☔ 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/chain flows onto the Code Review DetailsNo blocking findings. Notes considered and cleared during review:
|
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.
JaredSnider-Bitwarden
removed
t:bugfix
… Details Per ADR 0024, new C# files should opt in to nullable reference types. Drops the legacy #nullable disable from the new TwoFactor response and Details files, plus the per-action wrappers that were modified alongside. Properties that can be null when MetaData lacks the corresponding entry are now declared string? (Duo Host/ClientId/ClientSecret, Email Email, YubiKey Key1-Key5, WebAuthn Keys, WebAuthn KeyModel.Name).
…ared static class
…anizationDuoTests
…rHelperTests; delete monolithic file
…rAuthenticatorTests
…orControllerYubiKeyTests
…llerOrganizationDuoTests
…sts; delete monolithic file
…ebauthn Switches the WebAuthn challenge endpoint from re-collecting the user's secret to replaying the UV token minted by get-webauthn. The challenge step is now correctly modeled as a chain step: it validates an existing token, returns FIDO2 registration options, and leaves the original token valid for the subsequent PUT. The redundant fresh-mint in the challenge response is dropped — clients continue to use the original token through the full enrollment flow. The 2FA feature README's endpoint table grows a Chain column to surface this interior step (also covers Email's send-email).
…oc summaries Endpoint URLs and HTTP verbs in C# DTO class summaries rot when routes get renamed and rarely get updated alongside route changes. The 13 response models in this folder all repeated the route they're returned from; this strips those references and describes each model by domain role instead.
|
JaredSnider-Bitwarden
requested review from
JaredScar,
ike-kottlowski and
rr-bw
and removed request for
rr-bw
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
Clients PR: bitwarden/clients#21385
📔 Objective
Refactor the 2FA management endpoints to use our tokenable framework for ongoing user verification — a pattern already established by the Authenticator provider. Each
GET /two-factor/<provider>mints a short-lived user-verification token bound to(UserId, ProviderType); subsequent management calls (PUT,DELETE, and chained interior steps) replay that same token instead of re-collecting a secret. One secret prompt per management session, consistent shape across providers, single tokenable type to maintain.🎨 Rename — Disable → Delete. "Disable" implied a reversible soft-disable, but the underlying behavior hard-deletes the provider's
MetaData(TOTP shared secret, Duo client secret, YubiKey IDs, registered WebAuthn credentials, etc.). Renamed at the controller layer; service-layer names left alone to keep scope tight.🎨 Rename — Model naming. Every 2FA request model now follows
TwoFactor<Provider><Verb>RequestModel(e.g.,TwoFactorWebAuthnUpdateRequestModel,TwoFactorDuoDeleteRequestModel). Provider first to match theTwoFactorProviderTypeenum.API surface changes
DELETE /two-factor/{yubikey,duo,email,authenticator,webauthn,webauthn/all}andDELETE /organizations/{id}/two-factor/duo. All return 204 No Content except per-credentialDELETE /two-factor/webauthn(which shrinks the credential list rather than destroying the provider, so it still carries the updated parent state).PUT /two-factor/disableandPUT /organizations/{id}/two-factor/disable, plus their[Obsolete]POST companions.DELETE /two-factor/webauthn/allbulk endpoint. 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.POST /two-factor/get-webauthn-challengeswitched from re-collecting the secret to replaying the UV token minted byget-webauthn. The challenge step doesn't mint a new token — the original stays valid through the subsequentPUT /two-factor/webauthn.TwoFactorEmailLoginRequestModel(login challenge),TwoFactorEmailSetupRequestModel(setup-send),TwoFactorEmailUpdateRequestModel(PUT). Email is the only provider with three distinct flows.Architecture
TwoFactor<Provider><Action>ResponseModel), with the field surface delegated to sharedTwoFactor<Provider>DetailsDTOs that read provider state once. Replaces the prior pattern of one response model serving every action.POST /two-factor/get-webauthn-challenge— replays the GET's token, returns FIDO2 registration options the PUT needs.POST /two-factor/send-email— replays the GET's token, ships an OTP out-of-band that the PUT carries back alongside the same token.Behavior changes
GET /two-factor/yubikeyandGET /two-factor/duo. The matching PUTs still require premium. Lapsed-premium users can now read their own enrollment configuration and use the standardGET → DELETEflow to remove a provider they previously configured.Test coverage
TwoFactorControllerTests.cs(unit) andTwoFactorControllerTest.cs(integration) files were split into per-provider files undertest/Api.Test/Auth/Controllers/TwoFactor/andtest/Api.IntegrationTest/Controllers/TwoFactor/. Each file is 100–300 lines instead of 1600+; shared setup lives in static*TestHelpersclasses.TwoFactor*DetailsDTOs (null-handling, masking, key-by-key extraction).Docs
src/Core/Auth/UserFeatures/TwoFactorAuth/readme.mdcovering the UV-token flow, validation rules, the chain-endpoint table, and why Authenticator uses a different tokenable shape.IUserService.DisableTwoFactorProviderAsync/IOrganizationService.DisableTwoFactorProviderAsyncclarifying that they hard-delete the provider entry (the method names are historical).📸 Screenshots
See clients PR: bitwarden/clients#21385