bitwarden / clients

Bitwarden client apps (web, browser extension, desktop, and cli).
https://bitwarden.com
Other
8.66k stars 1.14k forks source link

[PM-7084] Refactor TwoFactorComponent #9766

Closed quexten closed 1 week ago

quexten commented 1 week ago

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-7084

📔 Objective

This PR refactors the two-factor component to: Use shared component logic and templates Break up components into per-authenthication-method components

The idea is to never have the orchestrator component deal with auth-method specific logic, but to delegate this to components specifically for the auth method. Further, we do not want to introduce platform-specific components that duplicate logic or templates if we can avoid it.

DO NOT REVIEW THIS PR

The review will be done in a set of smaller PR's since this is quite a complex changeset, untangling the (currently) overcomplex two-factor component. This PR will be merged, after review and QA is done on the individual PR's.

9767 - Shared two-fa options component

9768 - Shared two-fa orchestrator component & totp authenticator component

9769 - Shared two-fa yubikey component

9770 - Shared two-fa email component

9771 - Shared two-fa webauthn component

9772 - Shared two-fa duo component

📸 Screenshots

⏰ Reminders before review

🦮 Reviewer guidelines

codecov[bot] commented 1 week ago

Codecov Report

Attention: Patch coverage is 0% with 456 lines in your changes missing coverage. Please review.

Project coverage is 29.04%. Comparing base (9fc89aa) to head (98aa46e). Report is 41 commits behind head on main.

:exclamation: Current head 98aa46e differs from pull request most recent head dd7013c

Please upload reports for the commit dd7013c to get more accurate results.

Files Patch % Lines
...nents/two-factor-auth/two-factor-auth.component.ts 0.00% 66 Missing :warning:
...-factor-auth/two-factor-auth-webauthn.component.ts 0.00% 54 Missing :warning:
...two-factor-auth/two-factor-auth-email.component.ts 0.00% 51 Missing :warning:
.../src/auth/popup/two-factor-auth-email.component.ts 0.00% 37 Missing :warning:
.../desktop/src/auth/two-factor-auth-duo.component.ts 0.00% 36 Missing :warning:
...er/src/auth/popup/two-factor-auth-duo.component.ts 0.00% 35 Missing :warning:
...ts/two-factor-auth/two-factor-options.component.ts 0.00% 34 Missing :warning:
...s/two-factor-auth/two-factor-auth-duo.component.ts 0.00% 32 Missing :warning:
.../web/src/app/auth/two-factor-auth-duo.component.ts 0.00% 25 Missing :warning:
apps/web/src/app/auth/two-factor-auth.component.ts 0.00% 23 Missing :warning:
... and 7 more
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #9766 +/- ## ========================================== - Coverage 29.12% 29.04% -0.08% ========================================== Files 2531 2540 +9 Lines 73713 73865 +152 Branches 13753 13734 -19 ========================================== - Hits 21468 21456 -12 - Misses 50629 50831 +202 + Partials 1616 1578 -38 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

github-actions[bot] commented 1 week ago

Logo Checkmarx One – Scan Summary & Details5bba161f-aa04-4c42-9749-4a4f0805bdbd

New Issues

Severity Issue Source File / Package Checkmarx Insight
HIGH Client_DOM_XSS /apps/web/src/connectors/common.ts: 2 Attack Vector
LOW Client_DOM_Open_Redirect /apps/browser/src/platform/popup/layout/popup-header.component.ts: 29 Attack Vector
LOW Client_DOM_Open_Redirect /apps/web/src/connectors/common.ts: 2 Attack Vector
LOW Client_DOM_Open_Redirect /apps/web/src/connectors/common.ts: 2 Attack Vector
LOW Client_Use_Of_Iframe_Without_Sandbox /apps/web/src/connectors/duo.ts: 8 Attack Vector
LOW Unprotected_Cookie /apps/web/src/app/auth/two-factor-auth-duo.component.ts: 60 Attack Vector
LOW Unprotected_Cookie /apps/web/src/app/auth/two-factor.component.ts: 159 Attack Vector
LOW Unprotected_Cookie /apps/web/src/connectors/duo-redirect.ts: 57 Attack Vector
LOW Unprotected_Cookie /apps/web/src/connectors/duo-redirect.ts: 112 Attack Vector

Fixed Issues

Severity Issue Source File / Package
HIGH Client_DOM_XSS /apps/web/src/connectors/common.ts: 2
LOW Client_DOM_Open_Redirect /apps/web/src/connectors/common.ts: 2
LOW Client_DOM_Open_Redirect /apps/web/src/connectors/common.ts: 2
quexten commented 1 week ago

Not using this parent PR anymore.