Port Personal Two-Factor Setup to Angular - #80
Open
nllong wants to merge 1 commit into
Open
Conversation
- Add TwoFactorService (@seed/api/two-factor) preserving the four legacy /api/v3/two_factor/ endpoints: set_method, resend_token_email, generate_qr_code, verify_code - Add ProfileTwoFactorComponent under profile/two-factor: disabled/email/ token-generator method selection, organization-enforced 2FA banner (correctly falls back to email rather than disabled when any org the user belongs to requires 2FA, fixing a latent bug in the legacy fallback logic that used an always-undefined scope variable), resend test email token, save/reload flow - Add QrCodeModalComponent dialog: QR code display, 6-digit verification form, re-generate token, and safe fallback to email/disabled on cancel or 'select a different method' - Add MatRadioModule to the shared MaterialImports barrel (not previously needed by any other page) - Register the profile/two-factor route (before no other dynamic routes are affected) and add a Two Factor Profile nav/tab entry - Add new Transloco keys to public/i18n/en_US.json (Lokalise sync still needed for fr_CA/es) - Update MIGRATION.md: move personal two-factor setup from 'Not yet migrated' to 'Already migrated' Verified against a live throwaway backend + seeded data (an org requiring 2FA and one that doesn't) via Playwright: email method save + resend, token method save -> QR modal -> decoded QR -> valid TOTP verification -> device confirmed in DB, invalid code handling, standalone re-generate, select-a-different-method fallback, dark mode, and mobile viewport. No console errors observed. pnpm lint and pnpm build both pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Not ready to approve
The implementation has user-visible behavior that doesn’t match the PR description (missing resend confirmation and no inline error on invalid verification codes), plus a minor copy issue in a newly added translation string.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Ports the legacy AngularJS personal two-factor enrollment flow into the Angular app under profile/two-factor, adding a dedicated API wrapper and UI (including a QR verification modal) while keeping the backend on the existing /api/v3/two_factor/* endpoints.
Changes:
- Added
TwoFactorService+ types undersrc/@seed/api/two-factorand exported it via the API barrel. - Introduced
ProfileTwoFactorComponent+QrCodeModalComponentUI, and wired the new route/tab into the profile section. - Updated shared Material imports (MatRadio), added new i18n keys, and updated
MIGRATION.md.
File summaries
| File | Description |
|---|---|
| src/app/modules/profile/two-factor/two-factor.component.ts | New personal 2FA settings page logic (load org enforcement, save method, resend/regenerate actions, QR modal flow). |
| src/app/modules/profile/two-factor/two-factor.component.html | New personal 2FA settings page template (method selection UI, enforcement banner, actions). |
| src/app/modules/profile/two-factor/modal/qr-code-modal.component.ts | New QR verification modal logic (verify + regenerate QR code). |
| src/app/modules/profile/two-factor/modal/qr-code-modal.component.html | New QR verification modal template (QR display, 6-digit code input, actions). |
| src/app/modules/profile/profile.routes.ts | Adds the profile/two-factor route. |
| src/app/modules/profile/profile.component.ts | Adds a new “Two Factor Profile” tab/navigation item. |
| src/@seed/materials/material.module.ts | Adds MatRadioModule to the shared Material barrel. |
| src/@seed/api/two-factor/two-factor.types.ts | Defines types for personal 2FA endpoints and responses. |
| src/@seed/api/two-factor/two-factor.service.ts | Implements calls to /api/v3/two_factor/* endpoints with shared error handling. |
| src/@seed/api/two-factor/index.ts | Exports the new two-factor service/types from the feature barrel. |
| src/@seed/api/index.ts | Re-exports the two-factor API barrel from the main API entrypoint. |
| public/i18n/en_US.json | Adds/extends translation keys used by the new page/modal. |
| MIGRATION.md | Marks the personal 2FA page as migrated and updates the cross-check references. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 3
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Comment on lines
+44
to
+51
| next: (response) => { | ||
| this.verifying = false | ||
| if (response.success) { | ||
| this._snackBar.success(this._transloco.translate('Authenticator App Verified!')) | ||
| this.close(true) | ||
| } else { | ||
| this._snackBar.alert(response.error ?? this._transloco.translate('Unable to verify code. Please try again.')) | ||
| } |
| "Save Settings": "Save Settings", | ||
| "Saving access levels": "Saving access levels", | ||
| "Saving…": "Saving…", | ||
| "Scan the QR code below using your authenticator app of choice (ex: Google Authenticator, Microsoft Authenticator, Authy, etc...)": "Scan the QR code below using your authenticator app of choice (ex: Google Authenticator, Microsoft Authenticator, Authy, etc...)", |
Comment on lines
+109
to
+116
| resendTokenEmail(): void { | ||
| this._twoFactorService | ||
| .resendTokenEmail(this.user.org_id, this.user.email) | ||
| .pipe(takeUntil(this._unsubscribeAll$)) | ||
| .subscribe(() => { | ||
| this.emailSent = true | ||
| }) | ||
| } |
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
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.
Port Personal Two-Factor Setup to Angular
Ports the legacy AngularJS "Personal Two-Factor Setup" page
(
/profile/two_factor_profile,two_factor_profile_controller) to this app, underprofile/two-factor. Org-level 2FA policy was already migrated(
organizations/settings/two-factor) — this is the separate personal device-enrollment flow.Changes
@seed/api/two-factor— newTwoFactorService+ types, preserving the four legacy/api/v3/two_factor/endpoints unchanged:set_method,resend_token_email,generate_qr_code,verify_code.profile/two-factor—ProfileTwoFactorComponent:mat-radio-group)the active one), matching the legacy
orgs_require_2facheckconfirmation message
SnackBarServiceprofile/two-factor/modal—QrCodeModalComponent(MatDialog): QR code display, 6-digitverification form (
Validators.pattern(/^\d{6}$/), preserving leading zeros — the legacy<input type="number">silently truncated those), re-generate-in-place, and a safe fallback toemail/disabled on cancel or "select a different method".
qr_code_scan_modal_controller'scancel/fallback path referenced
$scope.require_2fa, a variable that was never actuallyassigned anywhere in
two_factor_profile_controller(only the differently-named$scope.orgs_require_2fawas set). That meant canceling out of the QR flow always fell back todisabled, silently bypassing organization-enforced 2FA. This port computes the fallback fromthe real cross-org check instead, so canceling correctly falls back to
email(neverdisabled) when any org the user belongs to requires 2FA.MatRadioModuleto the sharedMaterialImportsbarrel (not previously needed elsewhere).public/i18n/en_US.json(English is the key = value convention usedthroughout this app).
fr_CA.json/es.jsonare Lokalise-managed and unchanged here — afollow-up
pnpm update-translationspush is needed for those locales.MIGRATION.mdupdated: moved personal two-factor setup from "Not yet migrated" to "Alreadymigrated".
Validation
pnpm lintandpnpm buildboth pass.seedplatform/seed:develop) with seeded data — twoorgs, one with
require_2fa=True— via Playwright:correct email interpolation.
→ success snackbar → modal closes → user refreshed →
TOTPDeviceconfirmed in the DB.email(notdisabled) because the user belongsto an org requiring 2FA — confirms the bug-fix above.
banner and keeps "Disabled" locked, because the check is user-wide across all org
memberships, not just the active org — matching the legacy controller's behavior.
Screenshots
Not done here
seed/static/seed/) is untouched — it keeps serving/app/traffic.fr_CA/esLokalise sync for the new keys.