From 0fd5794db46a45c900c30bdbc44253d34132e11a Mon Sep 17 00:00:00 2001 From: Arthur Milchior Date: Fri, 27 May 2022 09:41:44 +0000 Subject: [PATCH] Reland "[iOS] Fix crash in password manager settings" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a reland of commit ea4d49929e416bc03b32f7ba9c5b1b259b79961e The reland is identical to the original patch, nothing changed. It only undoes a revert that landed on the release branch because it was considered it didn't get enough Beta coverage in M103 at the time. Original change's description: > [iOS] Fix crash in password manager settings > > The solution consists simply in stating that, if the search mode is > on, “on device encryption” section should not be shown. > > Note that it won’t appear when search end, which is not the expected > behavior. > > This must also correct bug 1322464, as otherwise the DCHECK fail during the unit test. > > Tested: > I added a unit test. I tested without the correction, and it crashes at the same place. I added the correction, and it succeed. > > Manually Tested: > Open ipad with two windows, be logged-in, with password sync disabled and promotion of on-device experiment activated. > On one window, open password-manager and click on search. > On the second window, activate password sync and click done. > > It should not crash anymore. > > (cherry picked from commit ad57f289a804e215bfd4aae5ff52647f0af69bec) > > Fixed: 1322464 > Bug: 1321057 > Change-Id: I78a97358e06921848c9d9b6a076368a7c0ec24a4 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3627553 > Commit-Queue: Arthur Milchior > Auto-Submit: Arthur Milchior > Reviewed-by: Jérôme Lebel > Cr-Original-Commit-Position: refs/heads/main@{#1002118} > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3651989 > Cr-Commit-Position: refs/branch-heads/5005@{#866} > Cr-Branched-From: 5b4d9450fee01f821b6400e947b3839727643a71-refs/heads/main@{#992738} Bug: 1321057 Change-Id: I9972e3a616fb17925dfb34372dc14f4948d1f8c1 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3669049 Reviewed-by: Viktor Semeniuk Auto-Submit: Mikel Astiz Commit-Queue: Viktor Semeniuk Bot-Commit: Rubber Stamper Commit-Queue: Mikel Astiz Cr-Commit-Position: refs/branch-heads/5005@{#1037} Cr-Branched-From: 5b4d9450fee01f821b6400e947b3839727643a71-refs/heads/main@{#992738} --- .../browser/ui/settings/password/BUILD.gn | 1 + .../password/passwords_coordinator.mm | 1 + .../settings/password/passwords_mediator.mm | 21 +++++- .../passwords_table_view_controller.mm | 69 +++++------------ ...passwords_table_view_controller_delegate.h | 24 ++++++ ...asswords_table_view_controller_unittest.mm | 75 +++++++++++++++++-- 6 files changed, 134 insertions(+), 57 deletions(-) diff --git a/ios/chrome/browser/ui/settings/password/BUILD.gn b/ios/chrome/browser/ui/settings/password/BUILD.gn index de8185638e2669..f6c637dbd8e3dd 100644 --- a/ios/chrome/browser/ui/settings/password/BUILD.gn +++ b/ios/chrome/browser/ui/settings/password/BUILD.gn @@ -176,6 +176,7 @@ source_set("unit_tests") { "//ios/chrome/common/ui/colors", "//ios/chrome/common/ui/reauthentication", "//ios/chrome/common/ui/table_view:cells_constants", + "//ios/chrome/test:test_support", "//ios/chrome/test/app:test_support", "//ios/web/public/test", "//testing/gmock", diff --git a/ios/chrome/browser/ui/settings/password/passwords_coordinator.mm b/ios/chrome/browser/ui/settings/password/passwords_coordinator.mm index 75f995cb1ce078..8e42ed4476e82f 100644 --- a/ios/chrome/browser/ui/settings/password/passwords_coordinator.mm +++ b/ios/chrome/browser/ui/settings/password/passwords_coordinator.mm @@ -140,6 +140,7 @@ - (void)start { } - (void)stop { + self.passwordsViewController.delegate = nil; self.passwordsViewController = nil; [self.passwordIssuesCoordinator stop]; diff --git a/ios/chrome/browser/ui/settings/password/passwords_mediator.mm b/ios/chrome/browser/ui/settings/password/passwords_mediator.mm index 456d3ea207d455..373ec36a4e6843 100644 --- a/ios/chrome/browser/ui/settings/password/passwords_mediator.mm +++ b/ios/chrome/browser/ui/settings/password/passwords_mediator.mm @@ -10,14 +10,13 @@ #include "components/password_manager/core/browser/leak_detection_dialog_utils.h" #include "components/password_manager/core/common/password_manager_features.h" #import "components/signin/public/identity_manager/objc/identity_manager_observer_bridge.h" +#include "components/sync/driver/sync_service_utils.h" #import "ios/chrome/browser/favicon/favicon_loader.h" #import "ios/chrome/browser/net/crurl.h" #include "ios/chrome/browser/passwords/password_check_observer_bridge.h" #import "ios/chrome/browser/passwords/save_passwords_consumer.h" #import "ios/chrome/browser/signin/authentication_service.h" -#include "ios/chrome/browser/signin/identity_manager_factory.h" #include "ios/chrome/browser/sync/sync_observer_bridge.h" -#include "ios/chrome/browser/sync/sync_service_factory.h" #include "ios/chrome/browser/sync/sync_setup_service.h" #import "ios/chrome/browser/ui/settings/password/passwords_consumer.h" #import "ios/chrome/browser/ui/settings/password/saved_passwords_presenter_observer.h" @@ -83,6 +82,9 @@ @interface PasswordsMediator () (identityManager, @@ -152,6 +155,7 @@ - (void)deletePasswordForm:(const password_manager::PasswordForm&)form { - (void)disconnect { _identityManagerObserver.reset(); _syncObserver.reset(); + _syncService = nullptr; } #pragma mark - PasswordsTableViewControllerDelegate @@ -240,6 +244,19 @@ - (NSAttributedString*)passwordCheckErrorInfo { attributes:textAttributes]; } +// Returns the on-device encryption state according to the sync service. +- (OnDeviceEncryptionState)onDeviceEncryptionState { + if (ShouldOfferTrustedVaultOptIn(_syncService)) { + return OnDeviceEncryptionStateOfferOptIn; + } + syncer::SyncUserSettings* syncUserSettings = _syncService->GetUserSettings(); + if (syncUserSettings->GetPassphraseType() == + syncer::PassphraseType::kTrustedVaultPassphrase) { + return OnDeviceEncryptionStateOptedIn; + } + return OnDeviceEncryptionStateNotShown; +} + #pragma mark - PasswordCheckObserver - (void)passwordCheckStateDidChange:(PasswordCheckState)state { diff --git a/ios/chrome/browser/ui/settings/password/passwords_table_view_controller.mm b/ios/chrome/browser/ui/settings/password/passwords_table_view_controller.mm index 1e07fae52d5dc1..995b927c17e8e5 100644 --- a/ios/chrome/browser/ui/settings/password/passwords_table_view_controller.mm +++ b/ios/chrome/browser/ui/settings/password/passwords_table_view_controller.mm @@ -33,7 +33,6 @@ #import "ios/chrome/browser/net/crurl.h" #import "ios/chrome/browser/signin/chrome_account_manager_service_factory.h" #import "ios/chrome/browser/signin/chrome_account_manager_service_observer_bridge.h" -#include "ios/chrome/browser/sync/sync_service_factory.h" #include "ios/chrome/browser/system_flags.h" #import "ios/chrome/browser/ui/elements/home_waiting_view.h" #import "ios/chrome/browser/ui/settings/cells/settings_check_cell.h" @@ -121,27 +120,6 @@ typedef NS_ENUM(NSInteger, ItemType) { ItemTypeOnDeviceEncryptionOptedInLearnMore, }; -// State of on-device encryption used for -// ItemTypeOnDeviceEncryptionOptInDescription, ItemTypeOnDeviceEncryptionSetUp -// and ItemTypeOnDeviceEncryptionSetUp. -typedef NS_ENUM(NSInteger, OnDeviceEncryptionState) { - // On device encryption is on. - // ItemTypeOnDeviceEncryptionOptInDescription is shown. - OnDeviceEncryptionStateOptedIn, - // User can opt-in on device encryption. - // ItemTypeOnDeviceEncryptionOptInDescription and - // ItemTypeOnDeviceEncryptionSetUp are shown. - OnDeviceEncryptionStateOfferOptIn, - // User can not opt-in in their current state. - // Currently it is either because: - // * User is not signed-in, - // * User hasn’t opted in to or disabled Sync for passwords (or equivalent - // enterprise policies), - // * User has a custom passphrase. - // SectionIdentifierOnDeviceEncryption is hidden. - OnDeviceEncryptionStateNotShown, -}; - std::vector> CopyOf( const std::vector& password_list) { std::vector> @@ -657,36 +635,42 @@ - (void)reloadData { - (void)updateOnDeviceEncryptionSessionWithUpdateTableView: (BOOL)updateTableView { OnDeviceEncryptionState oldState = self.onDeviceEncryptionStateInModel; - OnDeviceEncryptionState newState = [self onDeviceEncryptionState]; + OnDeviceEncryptionState newState = + self.navigationItem.searchController.active + ? OnDeviceEncryptionStateNotShown + : [self.delegate onDeviceEncryptionState]; if (newState == oldState) { return; } self.onDeviceEncryptionStateInModel = newState; TableViewModel* model = self.tableViewModel; - // Index of the OnDeviceEncryption section if it exists. - // Index where it should be added if it does not exists. - NSInteger sectionIdentifierOnDeviceEncryptionIndex = - [model sectionForSectionIdentifier:SectionIdentifierPasswordCheck] + 1; - NSIndexSet* sectionIdentifierOnDeviceEncryptionIndexSet = - [NSIndexSet indexSetWithIndex:sectionIdentifierOnDeviceEncryptionIndex]; - if (newState == OnDeviceEncryptionStateNotShown) { - // Previous state was not `OnDeviceEncryptionStateNotShown`, wich mean the + // Previous state was not `OnDeviceEncryptionStateNotShown`, which means the // section `SectionIdentifierOnDeviceEncryption` exists and must be removed. // It also mean the table view is not yet shown and thus should not be // updated. - DCHECK(!updateTableView); + DCHECK(updateTableView); [self clearSectionWithIdentifier:SectionIdentifierOnDeviceEncryption withRowAnimation:UITableViewRowAnimationAutomatic]; return; } + NSInteger onDeviceEncryptionSectionIndex = NSNotFound; if (oldState == OnDeviceEncryptionStateNotShown) { - [model - insertSectionWithIdentifier:SectionIdentifierOnDeviceEncryption - atIndex:sectionIdentifierOnDeviceEncryptionIndex]; + NSInteger passwordCheckSectionIndex = + [model sectionForSectionIdentifier:SectionIdentifierPasswordCheck]; + DCHECK_NE(NSNotFound, passwordCheckSectionIndex); + onDeviceEncryptionSectionIndex = passwordCheckSectionIndex + 1; + [model insertSectionWithIdentifier:SectionIdentifierOnDeviceEncryption + atIndex:onDeviceEncryptionSectionIndex]; + } else { + onDeviceEncryptionSectionIndex = + [model sectionForSectionIdentifier:SectionIdentifierOnDeviceEncryption]; } + DCHECK_NE(NSNotFound, onDeviceEncryptionSectionIndex); + NSIndexSet* sectionIdentifierOnDeviceEncryptionIndexSet = + [NSIndexSet indexSetWithIndex:onDeviceEncryptionSectionIndex]; [model deleteAllItemsFromSectionWithIdentifier: SectionIdentifierOnDeviceEncryption]; @@ -1863,21 +1847,6 @@ - (void)scrollToLastUpdatedItem { } } -// Returns the on-device encryption state according to the sync service. -- (OnDeviceEncryptionState)onDeviceEncryptionState { - syncer::SyncService* syncService = - SyncServiceFactory::GetForBrowserState(_browserState); - if (ShouldOfferTrustedVaultOptIn(syncService)) { - return OnDeviceEncryptionStateOfferOptIn; - } - syncer::SyncUserSettings* syncUserSettings = syncService->GetUserSettings(); - if (syncUserSettings->GetPassphraseType() == - syncer::PassphraseType::kTrustedVaultPassphrase) { - return OnDeviceEncryptionStateOptedIn; - } - return OnDeviceEncryptionStateNotShown; -} - // Notifies accessibility to focus on the Password Check Status cell when its // layout changed. - (void)focusAccessibilityOnPasswordCheckStatus { diff --git a/ios/chrome/browser/ui/settings/password/passwords_table_view_controller_delegate.h b/ios/chrome/browser/ui/settings/password/passwords_table_view_controller_delegate.h index f2cf44c3e602b0..940373afe6648d 100644 --- a/ios/chrome/browser/ui/settings/password/passwords_table_view_controller_delegate.h +++ b/ios/chrome/browser/ui/settings/password/passwords_table_view_controller_delegate.h @@ -7,6 +7,27 @@ #import +// State of on-device encryption used for +// ItemTypeOnDeviceEncryptionOptInDescription, ItemTypeOnDeviceEncryptionSetUp +// and ItemTypeOnDeviceEncryptionSetUp. +typedef NS_ENUM(NSInteger, OnDeviceEncryptionState) { + // On device encryption is on. + // ItemTypeOnDeviceEncryptionOptInDescription is shown. + OnDeviceEncryptionStateOptedIn, + // User can opt-in on device encryption. + // ItemTypeOnDeviceEncryptionOptInDescription and + // ItemTypeOnDeviceEncryptionSetUp are shown. + OnDeviceEncryptionStateOfferOptIn, + // User can not opt-in in their current state. + // Currently it is either because: + // * User is not signed-in, + // * User hasn’t opted in to or disabled Sync for passwords (or equivalent + // enterprise policies), + // * User has a custom passphrase. + // SectionIdentifierOnDeviceEncryption is hidden. + OnDeviceEncryptionStateNotShown, +}; + namespace password_manager { struct PasswordForm; } @@ -30,6 +51,9 @@ struct PasswordForm; // Returns detailed information about Password Check error if applicable. - (NSAttributedString*)passwordCheckErrorInfo; +// Returns the on-device encryption state according to the sync service. +- (OnDeviceEncryptionState)onDeviceEncryptionState; + @end #endif // IOS_CHROME_BROWSER_UI_SETTINGS_PASSWORD_PASSWORDS_TABLE_VIEW_CONTROLLER_DELEGATE_H_ diff --git a/ios/chrome/browser/ui/settings/password/passwords_table_view_controller_unittest.mm b/ios/chrome/browser/ui/settings/password/passwords_table_view_controller_unittest.mm index c44c07e14fb91c..f7865c4f0dfda3 100644 --- a/ios/chrome/browser/ui/settings/password/passwords_table_view_controller_unittest.mm +++ b/ios/chrome/browser/ui/settings/password/passwords_table_view_controller_unittest.mm @@ -38,6 +38,8 @@ #import "ios/chrome/common/ui/table_view/table_view_cells_constants.h" #include "ios/chrome/grit/ios_chromium_strings.h" #include "ios/chrome/grit/ios_strings.h" +#import "ios/chrome/test/app/sync_test_util.h" +#include "ios/chrome/test/scoped_key_window.h" #include "ios/web/public/test/web_task_environment.h" #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" @@ -58,12 +60,28 @@ // Declaration to conformance to SavePasswordsConsumerDelegate and keep tests in // this file working. -@interface PasswordsTableViewController (Test) < - UISearchBarDelegate, - PasswordsConsumer> +@interface PasswordsTableViewController (Test) - (void)updateExportPasswordsButton; @end +// TODO(crbug.com/1324555): Remove this double and uses TestSyncUserSettings +@interface TestPasswordsMediator : PasswordsMediator + +@property(nonatomic) OnDeviceEncryptionState encryptionState; + +@end + +@implementation TestPasswordsMediator + +- (OnDeviceEncryptionState)onDeviceEncryptionState: + (ChromeBrowserState*)browserState { + return self.encryptionState; +} + +@end + namespace { typedef struct { @@ -104,7 +122,7 @@ void SetUp() override { CreateController(); ChromeBrowserState* browserState = browser_->GetBrowserState(); - mediator_ = [[PasswordsMediator alloc] + mediator_ = [[TestPasswordsMediator alloc] initWithPasswordCheckManager:IOSChromePasswordCheckManagerFactory:: GetForBrowserState(browserState) syncSetupService:nil @@ -287,7 +305,9 @@ void SetEditing(bool editing) { web::WebTaskEnvironment task_environment_; std::unique_ptr browser_state_; std::unique_ptr browser_; - PasswordsMediator* mediator_; + TestPasswordsMediator* mediator_; + ScopedKeyWindow scoped_window_; + UIViewController* root_view_controller_ = nil; }; // Tests default case has no saved sites and no blocked sites. @@ -501,6 +521,51 @@ void SetEditing(bool editing) { UIAccessibilityTraitNotEnabled); } +// Tests that adding "on device encryption" don’t break during search. +TEST_F(PasswordsTableViewControllerTest, TestOnDeviceEncryptionWhileSearching) { + root_view_controller_ = [[UIViewController alloc] init]; + scoped_window_.Get().rootViewController = root_view_controller_; + + PasswordsTableViewController* passwords_controller = + static_cast(controller()); + + // Present the view controller. + __block bool presentation_finished = NO; + UINavigationController* navigation_controller = + [[UINavigationController alloc] + initWithRootViewController:passwords_controller]; + [root_view_controller_ presentViewController:navigation_controller + animated:NO + completion:^{ + presentation_finished = YES; + }]; + EXPECT_TRUE(base::test::ios::WaitUntilConditionOrTimeout( + base::test::ios::kWaitForUIElementTimeout, ^bool { + return presentation_finished; + })); + + // Disable on device encryption to prepare the state. + mediator_.encryptionState = OnDeviceEncryptionStateNotShown; + [passwords_controller updateOnDeviceEncryptionSessionAndUpdateTableView]; + + // start of the actual test. + passwords_controller.navigationItem.searchController.active = YES; + mediator_.encryptionState = OnDeviceEncryptionStateOptedIn; + [passwords_controller updateOnDeviceEncryptionSessionAndUpdateTableView]; + + passwords_controller.navigationItem.searchController.active = NO; + // Dismiss |view_controller_| and waits for the dismissal to finish. + __block bool dismissal_finished = NO; + [root_view_controller_ dismissViewControllerAnimated:NO + completion:^{ + dismissal_finished = YES; + }]; + EXPECT_TRUE(base::test::ios::WaitUntilConditionOrTimeout( + base::test::ios::kWaitForUIElementTimeout, ^bool { + return dismissal_finished; + })); +} + // Tests that the "Export Passwords..." button is greyed out in edit mode. TEST_F(PasswordsTableViewControllerTest, TestExportButtonDisabledEditMode) { PasswordsTableViewController* passwords_controller =