[PM-8216] Add warning to people who don't have two-factor authentication turned on #1208

KatherineInCode · 2024-12-13T19:55:44Z

🎟️ Tracking

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

📔 Objective

This is still a work in progress; I have it set up as a draft to help me track things as I tie up the remaining problems and do some additional refactoring. Some of these changes will be pulled into other PRs for simplicity.

📸 Screenshots

⏰ 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 · 2024-12-13T20:14:10Z

Checkmarx One – Scan Summary & Details – b69c64c9-f56e-4955-b251-7a21b15976dc

No New Or Fixed Issues Found

codecov · 2024-12-16T19:07:34Z

Codecov Report

Attention: Patch coverage is 98.97698% with 4 lines in your changes missing coverage. Please review.

Project coverage is 89.59%. Comparing base (05dd571) to head (cdf4477).

Files with missing lines	Patch %	Lines
...ctorNotice/SetUpTwoFactor/SetUpTwoFactorView.swift	93.65%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1208      +/-   ##
==========================================
+ Coverage   89.51%   89.59%   +0.08%     
==========================================
  Files         699      709      +10     
  Lines       44556    44947     +391     
==========================================
+ Hits        39884    40271     +387     
- Misses       4672     4676       +4

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

GlobalTestHelpers/MockAppModule.swift

matt-livefront · 2024-12-20T18:07:12Z

BitwardenShared/UI/Auth/TwoFactorNotice/SetUpTwoFactor/SetUpTwoFactorView.swift

+                .buttonStyle(.secondary())
+            }
+
+            Spacer()


🤔 Is this Spacer() necessary? It seems like this could be for pushing up the content, but the content in the scroll view should already be at the top? Or if it's just padding, I think the scroll view modifier also adds bottom padding.

At the time I had it in there, I needed it to get things pinned to the top, but I also changed around how this view was constructed since then. I'll double-check.

matt-livefront · 2024-12-20T18:08:14Z

BitwardenShared/UI/Auth/TwoFactorNotice/EmailAccess/EmailAccessView.swift

+                .buttonStyle(.primary())
+            }
+            .dynamicTypeSize(...DynamicTypeSize.xxxLarge)
+            .padding(.horizontal, 16)


🤔 I think this might be adding double horizontal padding, since the scrollView() modifier also adds horizontal padding. scrollView() also adds the background color.

matt-livefront · 2024-12-20T18:09:49Z

BitwardenShared/UI/Auth/TwoFactorNotice/TwoFactorNoticeCoordinator.swift

+    /// The delegate for this coordinator, used to notify when the user logs out.
+    private weak var delegate: VaultCoordinatorDelegate?


🤔 Is this used?

Nope, copy-paste artifact :(

matt-livefront · 2024-12-20T18:10:36Z

BitwardenShared/UI/Auth/TwoFactorNotice/TwoFactorNoticeHelper.swift

+/// the two-factor notice, and displaying it if so.
+///
+protocol TwoFactorNoticeHelper {
+    ///


❓ Missing docs?

BitwardenShared/UI/Auth/TwoFactorNotice/TwoFactorNoticeHelperTests.swift

matt-livefront · 2024-12-20T18:24:29Z

BitwardenShared/UI/Auth/TwoFactorNotice/EmailAccess/EmailAccessView.swift

+            Text(LocalizedStringKey(Localizations.doYouHaveReliableAccessToYourEmail("person\u{2060}@example.com")))
+                .styleGuide(.body)
+                .foregroundColor(Asset.Colors.textPrimary.swiftUIColor)
+                .accessibilityHidden(true)


❓ Why hide this from accessibility?

🤔 I'm not sure where that came from. Probably a copy-paste thing? But yeah, I don't think it should be hidden

matt-livefront · 2024-12-20T18:25:17Z

BitwardenShared/UI/Auth/TwoFactorNotice/EmailAccess/EmailAccessView.swift

+                send: EmailAccessAction.canAccessEmailChanged
+            ))
+            .toggleStyle(.bitwarden)
+            .accessibilityIdentifier("ItemFavoriteToggle")


❓ Is this the right accessibility identifier?

Hah, probably not. Thanks copy-paste!

matt-livefront · 2024-12-20T18:26:56Z

BitwardenShared/UI/Auth/TwoFactorNotice/SetUpTwoFactor/SetUpTwoFactorViewTests.swift

+    func test_remindMeLater_tap() throws {
+        let button = try subject.inspect().find(button: Localizations.remindMeLater)
+        try button.tap()
+
+        waitFor(!processor.effects.isEmpty)


🎨 This could use find(asyncButton:) to avoid waiting.

matt-livefront · 2024-12-20T18:29:57Z

BitwardenShared/UI/Platform/Application/Support/Localizations/en.lproj/Localizable.strings

 "NothingAvailableToAutofill" = "Nothing available to autofill";
 "FailedToGenerateVerificationCode" = "Failed to generate verification code";
 "FailedToAutofillItem" = "Failed to autofill item %1$@";
 "ExportingFailed" = "Exporting failed";
 "YouMayNeedToEnableDevicePasscodeOrBiometrics" = "You may need to enable device passcode or biometrics.";
+"TurnOnTwoStepLoginConfirmation" = "You can turn on two-step login on the bitwarden.com web vault. Do you want to visit the website now?";


❓ Should bitwarden.com here be replaced with the user's web vault URL?

🤔 I was going off of the fact that the change email confirmation just says bitwarden.com. That's a good question, though.

matt-livefront · 2024-12-20T18:35:23Z

BitwardenShared/UI/Auth/TwoFactorNotice/SetUpTwoFactor/SetUpTwoFactorProcessor.swift

+        case .changeAccountEmailTapped:
+            coordinator.showAlert(.changeEmailAlert {
+                self.state.url = self.services.environmentService.changeEmailURL


❓ If I navigate to the web to turn on two-factor and then come back to the app, will I still be on this screen? How do you dismiss the view in that case? Or does enabling two-factor log you out?

There's a discussion about this on the Android ticket, but the end result is right now the user will need to kill and re-run the app to pick up on the change.

KatherineInCode added 24 commits December 5, 2024 15:02

WIP

9c124a8

WIP

70090b7

WIP

5610511

Two-step view

d75ee94

Update image

e412f51

WIP

f341416

WIP

5c073a2

WIP

8c55746

WIP

c747432

Rename

520d56a

Flesh out email acces screen

44cd639

Clean up set up view

fdf53bf

Add to app settings

bc1d05c

State service

67d0020

Add feature flags

3508862

Add logic

3e8fb10

Save email attestation

2fc6c40

Implement remind me later

a8622e3

Add URLs

9322b9b

Fix urls on set up two factor screen

cee664c

WIP

4ebc843

Move things into helper

d9cbd87

Update tests

c95ae08

Merge branch 'main' into pm-8216/tfa-warning

402665f

KatherineInCode changed the title ~~Pm 8216/tfa warning~~ [PM-8216] Add warning to people who don't have two-factor authentication turned on Dec 13, 2024

KatherineInCode added 2 commits December 13, 2024 15:43

Environment things that were done in another PR

2800125

Merge branch 'main' into pm-8216/tfa-warning

8cfe3c3

Undo line break

a77ec02

KatherineInCode added 11 commits December 17, 2024 11:35

More tests

ca2a4dc

Rename

f4ca68f

Backfill tests

e30ebe7

Update tests

8b9bd5b

Merge branch 'main' into pm-8216/tfa-warning

ce64c87

Comments and organization

ff24e48

Test

bed21b9

Tests

a7d10ae

Additional test

94af4cb

Remove dead code

2bf34a2

Don't show remind button when in permanent mode

db0409b

andrebispo5 reviewed Dec 18, 2024

View reviewed changes

GlobalTestHelpers/MockAppModule.swift Outdated Show resolved Hide resolved

KatherineInCode added 11 commits December 18, 2024 10:34

More tests

fa61a1a

Test

06a11c5

Remove parentheses

0f7908e

Refactor view

c6783b2

Merge branch 'main' into pm-8216/tfa-warning

11e0d54

Track if account has two-factor enabled

97be596

Include SSO check

df2bb4c

Remove dead code

e8c91d0

Remove defaults

cb34426

Reorganize

26e36cf

Merge branch 'main' into pm-8216/tfa-warning

cdf4477

KatherineInCode marked this pull request as ready for review December 19, 2024 20:38

KatherineInCode requested review from matt-livefront, phil-livefront, brant-livefront and a team as code owners December 19, 2024 20:38

matt-livefront reviewed Dec 20, 2024

View reviewed changes

Remove extraneous spacer

672d159

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM-8216] Add warning to people who don't have two-factor authentication turned on #1208

[PM-8216] Add warning to people who don't have two-factor authentication turned on #1208

KatherineInCode commented Dec 13, 2024 •

edited

Loading

github-actions bot commented Dec 13, 2024 •

edited

Loading

codecov bot commented Dec 16, 2024 •

edited

Loading

matt-livefront Dec 20, 2024

KatherineInCode Dec 20, 2024

matt-livefront Dec 20, 2024

matt-livefront Dec 20, 2024

KatherineInCode Dec 20, 2024

matt-livefront Dec 20, 2024

matt-livefront Dec 20, 2024

KatherineInCode Dec 20, 2024

matt-livefront Dec 20, 2024

KatherineInCode Dec 20, 2024

matt-livefront Dec 20, 2024

matt-livefront Dec 20, 2024

KatherineInCode Dec 20, 2024

matt-livefront Dec 20, 2024

KatherineInCode Dec 20, 2024

		/// The delegate for this coordinator, used to notify when the user logs out.
		private weak var delegate: VaultCoordinatorDelegate?

[PM-8216] Add warning to people who don't have two-factor authentication turned on #1208

Are you sure you want to change the base?

[PM-8216] Add warning to people who don't have two-factor authentication turned on #1208

Conversation

KatherineInCode commented Dec 13, 2024 • edited Loading

🎟️ Tracking

📔 Objective

📸 Screenshots

⏰ Reminders before review

🦮 Reviewer guidelines

github-actions bot commented Dec 13, 2024 • edited Loading

No New Or Fixed Issues Found

codecov bot commented Dec 16, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KatherineInCode commented Dec 13, 2024 •

edited

Loading

github-actions bot commented Dec 13, 2024 •

edited

Loading

codecov bot commented Dec 16, 2024 •

edited

Loading