[PM-21204] Users without premium cannot disable premium 2FA #17134
Conversation
…rService (domain).
…all sites to use TwoFactorService.
…mmon/auth/two-factor feature namespace.
|
New Issues (7)Checkmarx found the following issues in this Pull Request
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #17134 +/- ##
==========================================
+ Coverage 41.20% 41.23% +0.03%
==========================================
Files 3543 3545 +2
Lines 101912 101986 +74
Branches 15282 15294 +12
==========================================
+ Hits 41988 42053 +65
- Misses 58159 58170 +11
+ Partials 1765 1763 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
enmande
changed the title
…isable-premium-mfa
|
For reviewers: 
Just so it's clear there was no feedback given and removed from this review. |
enmande
requested review from
Patrick-Pimentel-Bitwarden,
dereknance and
jrmccannon
|
Claude finished @enmande's task —— View job Code Review CompleteI've reviewed this PR addressing PM-21204 (users without premium cannot disable premium 2FA) and the accompanying two-factor service refactor. The implementation is solid and the refactor aligns well with ADRs. Here are my findings: SummaryFinding 1: The core bugfix (disable-only flow for non-premium users) is well-implemented with clear architectural reasoning. Finding 2: The service refactor successfully extracts API concerns and co-locates models, improving code organization. Finding 3: QA-requested error message improvement in latest commit enhances user experience. Finding 4: Two areas of technical debt remain acknowledged but acceptable for scope management. Detailed Analysis💭 Finding 1: Architectural Decision for Disable FlowThe choice to use
Documentation: The inline comment clearly explains this decision, which will help future maintainers. 💭 Finding 2: Template Logic CorrectnessThe conditional rendering in the template (two-factor-setup.component.html:74-93) correctly implements the requirement: @if (p.premium && p.enabled && !(canAccessPremium$ | async))This shows the "Disable" button when:
All other cases show the "Manage" button (disabled if user lacks premium for non-enabled premium providers). ♻️ Finding 3: TypeScript Strict Compliance IncompleteThe PR objective mentions "TypeScript strict compliance (ADR-0014)", but the new service file still has Status: Acknowledged by enmande in review comments as intentional scope management. This is acceptable given the PR's already large scope. Consider tracking in a follow-up issue. ♻️ Finding 4: Test Coverage GapCodecov reports 6.25% patch coverage (165 lines missing). The new Status: Acknowledged by enmande as "wontfix" - component-level tests not required, service layer is tested, human verification completed. While this is a pragmatic decision, note that security-critical 2FA functionality traditionally benefits from comprehensive testing. 💭 Finding 5: Service Abstraction QualityThe refactor cleanly separates concerns:
The abstraction is well-documented with comprehensive JSDoc comments explaining parameters, return values, and security requirements. ✅ Finding 6: Error Message ImprovementLatest commit (ca3bcfb) improves user experience:
This addresses QA feedback from PM-28511 appropriately. PR Title and Description QualityTitle: ✅ Clear and specific - describes both the bug fix and refactor scope Description: ✅ Comprehensive
Recommendations
ConclusionThis PR successfully addresses PM-21204 with a well-architected solution that respects existing patterns while improving code organization. The refactor brings meaningful ADR compliance improvements without overreaching in scope. Previous reviewers (jrmccannon, Patrick-Pimentel-Bitwarden, dereknance) have approved, and my review concurs. The acknowledged technical debt (strict mode, test coverage) represents reasonable scope management decisions rather than oversights. The core functionality is sound, security implications are properly handled, and the user experience improvement from the latest commit demonstrates responsiveness to QA feedback. Overall assessment: ✅ Ready for merge pending any final reviewer concerns about acknowledged technical debt. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
apps/web/src/app/auth/settings/two-factor/two-factor-setup.component.ts
Outdated
Show resolved
Hide resolved
apps/web/src/app/auth/settings/two-factor/two-factor-setup.component.html
Show resolved
Hide resolved
jrmccannon
left a comment
jrmccannon
left a comment
There was a problem hiding this comment.
👍 Looks fine from a AC standpoint. Our files were only affected by the rename.
Next time, I think it would be easier to digest if the rename/refactor was done separately so its easier to separate the logic changes from the refactor.
Patrick-Pimentel-Bitwarden
left a comment
Patrick-Pimentel-Bitwarden
left a comment
There was a problem hiding this comment.
👍 The refactor looks as good as good as the bug fix! You toed the line well of just handling the partitioning without addressing more technical debt (I would have mistakenly tried to fix a couple of the @ts-strict-ignore's and I'm glad you didn't). One comment I would make though is when doing these refactors consider where you could do file moves instead of whole new file creations/deletions to maintain some of the git commit history.
@jrmccannon Thank you for the feedback; you're not alone in having considered this through the review process. In hindsight, it was good learning and good progress toward aligning more of the codebase, but bit ambitious to combine them. At the very least this could have had a refactor PR fast-followed by the bugfix. Appreciate the extra effort on this review, and will definitely consider this more next time! 👍 |
enmande
dismissed stale reviews from dereknance, Patrick-Pimentel-Bitwarden, and jrmccannon
via
2acc4c9
…isable-premium-mfa
apps/web/src/app/auth/settings/two-factor/two-factor-setup.component.ts
Outdated
Show resolved
Hide resolved
apps/web/src/app/auth/settings/two-factor/two-factor-setup.component.html
Show resolved
Hide resolved
…isplayed to the user.
|
QA has been completed on this issue. Re-requesting reviews. |
enmande
requested review from
Patrick-Pimentel-Bitwarden,
dereknance and
jrmccannon
enmande
deleted the
auth/pm-21204/users-without-premium-cannot-disable-premium-mfa
branch
🎟️ Tracking
PM-21204
📔 Objective
Users without a premium subscription cannot disable premium-only 2FA factors (Yubikey, Duo) that may have been configured while a premium subscription was active, either for their account or inherited through a premium organization membership.
This provides users a disable-only path to removing premium-only 2FA without an active premium subscription. The Manage action will be replaced with a Turn Off action which verifies the user and disables the specified 2FA factor. Additional configuration or management is not supported for a non-premium account.
🥇 Bonus
This includes a move/refactor of the Two-Factor Service abstraction and default implementation, including naming to align with ADRs including:
a. Extraction of
ApiServicecalls into a feature-centricTwoFactorApiServiceandTwoFactorService.b. Co-located models and services.
a. Groups TwoFactor concerns in a central feature folder structure.
📸 Screenshots
User With Premium Lapsed
This user enabled a premium 2FA factor while subscribed to premium, lost premium, and is able to disable the factor.
PM-21204__mp-user-one-premium-2fa.mov
Enterprise Org Member Removed, Non-Premium User Account
Jimothy is an existing non-premium user. After being invited and confirmed to an enterprise organization (Examplecorp, a TDE organization), Jimothy enables a premium 2FA factor (Yubikey) which was made available through organization membership. Later, Jimothy is removed from the organization and consequently loses access to premium 2FA factors. Jimothy is still able to remove the 2FA factor.
PM-21204__free-user-added-removed-enterprise-org.mov
⏰ Reminders before review
🦮 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