Auth/PM-3813 - 2FA Management Endpoints - User Verification Refactor#7841
Draft
JaredSnider-Bitwarden wants to merge 9 commits into
Draft
Auth/PM-3813 - 2FA Management Endpoints - User Verification Refactor#7841JaredSnider-Bitwarden wants to merge 9 commits into
JaredSnider-Bitwarden wants to merge 9 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 2217 +24
Lines 97296 97823 +527
Branches 8767 8819 +52
==========================================
+ Hits 59601 60220 +619
+ Misses 35582 35454 -128
- 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 Code Review DetailsNo new findings. One pre-existing unresolved thread remains:
Notes considered and not flagged:
|
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.
|
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.
📸 Screenshots
See clients PR: TODO