Reland "[iOS] Fix crash in password manager settings"

This is a reland of commit ea4d499 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 ad57f28) > > Fixed: 1322464 > Bug: 1321057 > Change-Id: I78a97358e06921848c9d9b6a076368a7c0ec24a4 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3627553 > Commit-Queue: Arthur Milchior <arthurmilchior@google.com> > Auto-Submit: Arthur Milchior <arthurmilchior@google.com> > Reviewed-by: Jérôme Lebel <jlebel@chromium.org> > 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: 5b4d945-refs/heads/main@{#992738} Bug: 1321057 Change-Id: I9972e3a616fb17925dfb34372dc14f4948d1f8c1 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3669049 Reviewed-by: Viktor Semeniuk <vsemeniuk@google.com> Auto-Submit: Mikel Astiz <mastiz@chromium.org> Commit-Queue: Viktor Semeniuk <vsemeniuk@google.com> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Commit-Queue: Mikel Astiz <mastiz@chromium.org> Cr-Commit-Position: refs/branch-heads/5005@{#1037} Cr-Branched-From: 5b4d945-refs/heads/main@{#992738}
chromium · May 27, 2022 · 0fd5794 · 0fd5794
1 parent 40fc3ac
commit 0fd5794
Show file tree

Hide file tree

Showing 6 changed files with 134 additions and 57 deletions.
diff --git 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
@@ -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
@@ -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 () <IdentityManagerObserverBridgeDelegate,
 // favicon images.
 @property(nonatomic, assign) FaviconLoader* faviconLoader;
 
+// Service to know whether passwords are synced.
+@property(nonatomic, assign) syncer::SyncService* syncService;
+
 @end
 
 @implementation PasswordsMediator
@@ -96,6 +98,7 @@ @implementation PasswordsMediator
                      syncService:(syncer::SyncService*)syncService {
   self = [super init];
   if (self) {
+    _syncService = syncService;
     _faviconLoader = faviconLoader;
     _identityManagerObserver =
         std::make_unique<signin::IdentityManagerObserverBridge>(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
@@ -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<std::unique_ptr<password_manager::PasswordForm>> CopyOf(
     const std::vector<password_manager::PasswordForm>& password_list) {
   std::vector<std::unique_ptr<password_manager::PasswordForm>>
@@ -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
@@ -7,6 +7,27 @@
 
 #import <Foundation/Foundation.h>
 
+// 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
@@ -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) <PasswordsConsumer,
+                                                UISearchBarDelegate,
+                                                UISearchControllerDelegate>
 - (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<TestChromeBrowserState> browser_state_;
   std::unique_ptr<TestBrowser> 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<PasswordsTableViewController*>(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 =