SplitSettingsSync: Fix Active Directory and device local account tests

The sync consent screen is skipped for non-branded (e.g. developer)
builds. We need to do this check after the check for non-GAIA and
public accounts, otherwise we try to enable sync for account types
that don't have an IdentityManager primary account.

This fixes DCHECK failures on account_id in Active Directory and
device local account tests.

This CL also adds an explicit sync consent screen test for Active
Directory accounts, because it took me a while to figure out why
unrelated AD tests were failing in sync consent code.

Bug: 1060289
Change-Id: Ief0b78ae6869349993b4427226af625591d827f0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2211181
Auto-Submit: James Cook <jamescook@chromium.org>
Commit-Queue: Roman Sorokin [CET] <rsorokin@chromium.org>
Reviewed-by: Roman Sorokin [CET] <rsorokin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#773125}

chrome/browser/chromeos/login/screens/sync_consent_browsertest.cc

@@ -11,6 +11,8 @@
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/login/screens/sync_consent_screen.h"
+#include "chrome/browser/chromeos/login/test/active_directory_login_mixin.h"
+#include "chrome/browser/chromeos/login/test/device_state_mixin.h"
 #include "chrome/browser/chromeos/login/test/js_checker.h"
 #include "chrome/browser/chromeos/login/test/login_manager_mixin.h"
 #include "chrome/browser/chromeos/login/test/oobe_base_test.h"
@@ -615,6 +617,50 @@ IN_PROC_BROWSER_TEST_F(SyncConsentSplitSettingsSyncTest,
   EXPECT_TRUE(prefs->GetBoolean(chromeos::prefs::kSyncOobeCompleted));
 }
 
+// Tests for Active Directory accounts, which skip the dialog because they do
+// not use sync.
+class SyncConsentActiveDirectoryTest : public OobeBaseTest {
+ public:
+  SyncConsentActiveDirectoryTest() {
+    sync_feature_list_.InitAndEnableFeature(
+        chromeos::features::kSplitSettingsSync);
+  }
+  ~SyncConsentActiveDirectoryTest() override = default;
+
+ protected:
+  base::test::ScopedFeatureList sync_feature_list_;
+  DeviceStateMixin device_state_{
+      &mixin_host_,
+      DeviceStateMixin::State::OOBE_COMPLETED_ACTIVE_DIRECTORY_ENROLLED};
+  ActiveDirectoryLoginMixin ad_login_{&mixin_host_};
+  base::HistogramTester histogram_tester_;
+};
+
+IN_PROC_BROWSER_TEST_F(SyncConsentActiveDirectoryTest, LoginDoesNotStartSync) {
+  // Sign in Active Directory user.
+  OobeBaseTest::WaitForSigninScreen();
+  ad_login_.SubmitActiveDirectoryCredentials(
+      "test-user@locally-managed.localhost", "password");
+  test::WaitForLastScreenAndTapGetStarted();
+  test::WaitForPrimaryUserSessionStart();
+
+  // OS sync is off.
+  syncer::SyncUserSettings* settings = GetSyncUserSettings();
+  EXPECT_FALSE(settings->IsOsSyncFeatureEnabled());
+
+  // Browser sync is off.
+  EXPECT_FALSE(settings->IsSyncRequested());
+  EXPECT_FALSE(settings->IsFirstSetupComplete());
+
+  // Dialog is marked completed (because it was skipped).
+  PrefService* prefs = ProfileManager::GetPrimaryUserProfile()->GetPrefs();
+  EXPECT_TRUE(prefs->GetBoolean(chromeos::prefs::kSyncOobeCompleted));
+
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTimeByExitReason.Sync-consent.Next", 0);
+  histogram_tester_.ExpectTotalCount("OOBE.StepCompletionTime.Sync-consent", 0);
+}
+
 // Tests that the SyncConsent screen performs a timezone request so that
 // subsequent screens can have a timezone to work with, and that the timezone
 // is properly stored in a preference.

chrome/browser/chromeos/login/screens/sync_consent_screen.cc

@@ -266,11 +266,7 @@ SyncConsentScreen::GetDelegateForTesting() const {
 
 SyncConsentScreen::SyncScreenBehavior SyncConsentScreen::GetSyncScreenBehavior()
     const {
-  // Skip for non-branded (e.g. developer) builds.
-  if (!g_is_branded_build)
-    return SyncScreenBehavior::kSkipAndEnableSync;
-
-  // Skip for users without Gaia account.
+  // Skip for users without Gaia account (e.g. Active Directory).
   if (!user_->HasGaiaAccount())
     return SyncScreenBehavior::kSkip;
 
@@ -278,6 +274,12 @@ SyncConsentScreen::SyncScreenBehavior SyncConsentScreen::GetSyncScreenBehavior()
   if (user_->GetType() == user_manager::USER_TYPE_PUBLIC_ACCOUNT)
     return SyncScreenBehavior::kSkip;
 
+  // Skip for non-branded (e.g. developer) builds. Check this after the account
+  // type checks so we don't try to enable sync in browser_tests for those
+  // account types.
+  if (!g_is_branded_build)
+    return SyncScreenBehavior::kSkipAndEnableSync;
+
   const user_manager::UserManager* user_manager =
       user_manager::UserManager::Get();
   // Skip for non-regular ephemeral users.