[Safe Browsing] Reorder PolicyEngine method to enable refactoring

We will need to refactor
PolicyEngine::CanPerformFullUrlLookupWithToken() as part of enabling
Gaia-keyed URL lookups in WebLayer: the parts that integrate with
Chrome signin and sync will need to be pulled out into a separate
method. This CL prepares for that refactoring by reordering this method
so that the common checks are all first followed by the checks that are
specific to signin/sync integration.

There is a slight behavioral change in this CL:
- Currently, if the user has enabled enhanced protection and the primary
  account is available, the value of the
  kRealTimeUrlLookupEnabledWithToken feature is not taken into account.
  However, if the user has enabled enhanced protection but the primary
  account is *not* available,
  PolicyEngine::CanPerformFullUrlLookupWithToken() will short-circuit
  out if the kRealTimeUrlLookupEnabledWithToken feature is not enabled.
- In this CL, if the user has enabled enhanced protection, the value of
  the kRealTimeUrlLookupEnabledWithToken feature is not taken into
  account (full stop).

This behavioral change reflects the originally-intended behavior
(cf. https://chromium-review.googlesource.com/c/chromium/src/+/2103582).
The current behavior of considering the value of the feature if the user
has enabled enhanced protection but the primary account isn't available
came in unintentionally as part of
https://chromium-review.googlesource.com/c/chromium/src/+/2138164.

This behavioral change also entails a slight modification of the
PolicyEngine unittest: the unittest was previously testing a case where
enhanced protection was enabled, the user was not signed in, the
kRealTimeUrlLookupEnabledWithToken feature was disabled, and sync was
enabled. The result in this case differs before/after this CL, but this
case also doesn't reflect reality: it's not possible for the user to be
syncing browser history without being signed in. We update the test to
have the cases reflect reality.

Bug: 1080748
Change-Id: I075bdb7567b8b35a266b2c3cc85258db5a91d042
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2610095Reviewed-by: Xinghui Lu <xinghuilu@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#840968}

components/safe_browsing/core/realtime/policy_engine.cc

@@ -92,26 +92,30 @@ bool RealTimePolicyEngine::CanPerformFullURLLookupWithToken(
     return false;
   }
 
-  if (IsUserEpOptedIn(pref_service) &&
-      IsPrimaryAccountSignedIn(identity_manager)) {
-    return true;
-  }
-
-  if (!base::FeatureList::IsEnabled(kRealTimeUrlLookupEnabledWithToken)) {
+  // Safe browsing token fetches are usually disabled if the feature is not
+  // enabled via Finch. The only exception is for users who have explicitly
+  // enabled enhanced protection, for whom the Finch feature is not relevant.
+  if (!base::FeatureList::IsEnabled(kRealTimeUrlLookupEnabledWithToken) &&
+      !IsUserEpOptedIn(pref_service)) {
     return false;
   }
 
-  // |sync_service| can be null in Incognito, and also be set to null by a
-  // cmdline param.
-  if (!sync_service) {
-    return false;
+  // If the user has explicitly enabled enhanced protection and the primary
+  // account is available, no further conditions are needed.
+  if (IsUserEpOptedIn(pref_service) &&
+      IsPrimaryAccountSignedIn(identity_manager)) {
+    return true;
   }
 
-  // Full URL lookup with token is enabled when the user is syncing their
-  // browsing history without a custom passphrase.
-  return syncer::GetUploadToGoogleState(
-             sync_service, syncer::ModelType::HISTORY_DELETE_DIRECTIVES) ==
-             syncer::UploadState::ACTIVE &&
+  // Otherwise, check the status of sync: Safe browsing token fetches are
+  // enabled when the user is syncing their browsing history without a custom
+  // passphrase.
+  // NOTE: |sync_service| can be null in Incognito, and can also be set to null
+  // by a cmdline param.
+  return sync_service &&
+         (syncer::GetUploadToGoogleState(
+              sync_service, syncer::ModelType::HISTORY_DELETE_DIRECTIVES) ==
+          syncer::UploadState::ACTIVE) &&
          !sync_service->GetUserSettings()->IsUsingSecondaryPassphrase();
 }
 

components/safe_browsing/core/realtime/policy_engine_unittest.cc

@@ -179,24 +179,26 @@ TEST_F(RealTimePolicyEngineTest,
       identity_test_env->identity_manager();
   syncer::TestSyncService sync_service;
 
-  // Enhanced protection is on but user is not signed in.
+  // For the purposes of this test, disable sync.
+  sync_service.SetDisableReasons(
+      {syncer::SyncService::DISABLE_REASON_USER_CHOICE});
+  sync_service.SetTransportState(syncer::SyncService::TransportState::DISABLED);
+  sync_service.GetUserSettings()->SetSelectedTypes(
+      /* sync_everything */ false, {});
+
+  // Enhanced protection is on but the user is not signed in: it should not be
+  // possible to perform URL lookups with tokens.
   pref_service_.SetBoolean(prefs::kSafeBrowsingEnhanced, true);
   EXPECT_FALSE(CanPerformFullURLLookupWithToken(
       /* is_off_the_record */ false, &sync_service, identity_manager));
 
-  // User is signed in.
+  // Enhanced protection is on and the user is signed in: it should be possible
+  // to perform URL lookups with tokens (even though the
+  // kRealTimeLookupEnabledWithToken feature and sync/history sync are
+  // disabled).
   identity_test_env->MakeUnconsentedPrimaryAccountAvailable("test@example.com");
   EXPECT_TRUE(CanPerformFullURLLookupWithToken(
       /* is_off_the_record */ false, &sync_service, identity_manager));
-
-  // Sync and history sync is disabled but enhanced protection is enabled.
-  sync_service.SetDisableReasons(
-      {syncer::SyncService::DISABLE_REASON_USER_CHOICE});
-  sync_service.SetTransportState(syncer::SyncService::TransportState::DISABLED);
-  sync_service.GetUserSettings()->SetSelectedTypes(
-      /* sync_everything */ false, {});
-  EXPECT_TRUE(CanPerformFullURLLookupWithToken(
-      /*is_off_the_record=*/false, &sync_service, identity_manager));
 }
 
 TEST_F(RealTimePolicyEngineTest, TestCanPerformEnterpriseFullURLLookup) {