Skip to content

Conversation

@enmande
Copy link
Contributor

@enmande enmande commented Jan 29, 2026

🎟️ Tracking

PM-29890

📔 Objective

Refactor two-factor WebAuthn-supporting methods out of the UserService and into their own commands.
Includes the credential registration start, credential registration complete (persist), and delete credential actions, with test suites for each.

Moves Two-factor WebAuthn into the Auth UserFeatures namespace.

Moved the related Query, as this was in a root/Interfaces directory pattern; other similar areas use a root/Implementations pattern, with interfaces at the top-level namespace. No functional change, consistency only.

📸 Screenshots

Two-factor WebAuthn credential management

Lifecycle of credential management.

pm-29890__weauthn-management.mov

Key Deletion

pm-29890__delete-key.mov

Two-factor WebAuthn credential limits

Altered to allow a single WebAuthn credential for illustrative purposes.

pm-29890__over-limit.mov

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

@github-actions
Copy link
Contributor

github-actions bot commented Jan 29, 2026

Logo
Checkmarx One – Scan Summary & Detailsee1de646-b518-4617-9069-31175ac525b1

New Issues (1)

Checkmarx found the following issues in this Pull Request

# Severity Issue Source File / Package Checkmarx Insight
1 MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 291
detailsMethod at line 291 of /src/Api/Auth/Controllers/AccountsController.cs gets a parameter from a user request from model. This parameter value flow...
Attack Vector
Fixed Issues (2)

Great job! The following issues were fixed in this Pull Request

Severity Issue Source File / Package
MEDIUM CSRF /src/Api/Tools/Controllers/SendsController.cs: 68
MEDIUM Use_Of_Hardcoded_Password /util/Seeder/Factories/UserSeeder.cs: 50

@codecov
Copy link

codecov bot commented Jan 29, 2026

Codecov Report

❌ Patch coverage is 84.17266% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.11%. Comparing base (d9e849a) to head (5c2d502).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
src/Api/Auth/Controllers/TwoFactorController.cs 43.75% 9 Missing ⚠️
...ns/CompleteTwoFactorWebAuthnRegistrationCommand.cs 82.00% 6 Missing and 3 partials ⚠️
...tions/StartTwoFactorWebAuthnRegistrationCommand.cs 93.02% 0 Missing and 3 partials ⚠️
...ations/DeleteTwoFactorWebAuthnCredentialCommand.cs 95.65% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6920      +/-   ##
==========================================
+ Coverage   56.06%   56.11%   +0.05%     
==========================================
  Files        1969     1974       +5     
  Lines       87065    87128      +63     
  Branches     7755     7763       +8     
==========================================
+ Hits        48811    48893      +82     
+ Misses      36446    36427      -19     
  Partials     1808     1808              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@enmande enmande added the ai-review Request a Claude code review label Jan 29, 2026
@claude
Copy link
Contributor

claude bot commented Jan 29, 2026

Claude finished @enmande's task in 1m 54s —— View job


Code Review: Auth/pm 29890/refactor webauthn 2fa methods out of userservice

Reviewed commit: 5c2d502
Author: @enmande

Summary

This PR refactors WebAuthn 2FA credential management methods from UserService into dedicated command classes following the CQRS pattern. The changes include:

  • StartTwoFactorWebAuthnRegistrationCommand - Initiates credential registration with challenge generation
  • CompleteTwoFactorWebAuthnRegistrationCommand - Persists credentials after successful attestation
  • DeleteTwoFactorWebAuthnCredentialCommand - Removes individual credentials

The refactoring also moves TwoFactorIsEnabledQuery into the Auth/UserFeatures/TwoFactorAuth namespace for consistency with other feature areas.

Re-review (5c2d502)

The latest commit adds a documentation comment to DeleteTwoFactorWebAuthnCredentialCommand.cs explaining the minimum credential count check. This addresses the feedback from @JaredSnider-Bitwarden requesting the explanation be added to the implementation file alongside the interface documentation.

No new issues identified in this incremental change.

Findings

No critical or important issues identified.


Verdict: Approved - Clean refactoring with good test coverage. The code follows established patterns and maintains the same security guarantees.


Metric Value
Risk Low
Complexity Low
Automated Review Claude Code

@enmande enmande changed the title Auth/pm 29890/refactor webauthn 2fa methods out of userservice [PM-29890] Refactor Two-factor WebAuthn Methods Out of UserService Jan 29, 2026
@enmande enmande marked this pull request as ready for review January 29, 2026 04:33
@enmande enmande requested a review from a team as a code owner January 29, 2026 04:33
Copy link
Contributor

@JaredSnider-Bitwarden JaredSnider-Bitwarden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work! Appreciate the high test coverage w/ the clean extraction and well documented interfaces! One question below.

Copy link
Contributor

@JaredSnider-Bitwarden JaredSnider-Bitwarden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@enmande enmande added needs-qa and removed ai-review Request a Claude code review labels Jan 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants