[unified-consent] Don't record user events when datatype is disabled.

USER_EVENTS is a special datatype, because it is commit-only. Therefore,
it is handled in a special way in a processor. As a result, when the
datatype is disabled (e.g. in Sync settings), it does not stop recording.
This was not a problem before, because there was no user visible way to
disable user events recording (except through history, but that works
well), however after unified consent there will be a separate toggle
in settings. This CL adds a temporary workaround (explicit check in the
user event service for the datatype) for M69 and only when USER_CONSENTS
type is enabled.

Once we improve handling of commit only types (https://crbug.com/830535),
this workaround won't be needed anymore.

Bug: 860616
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: Idac71d1d8d8d5a504940ffb1e09d6a7173989c57
Reviewed-on: https://chromium-review.googlesource.com/1143475
Commit-Queue: vitaliii <vitaliii@chromium.org>
Reviewed-by: Markus Heintz <markusheintz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577839}

chrome/browser/sync/test/integration/single_client_user_events_sync_test.cc

@@ -5,6 +5,7 @@
 #include <stdint.h>
 
 #include "base/macros.h"
+#include "base/test/scoped_feature_list.h"
 #include "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
 #include "chrome/browser/sync/test/integration/sessions_helper.h"
 #include "chrome/browser/sync/test/integration/single_client_status_change_checker.h"
@@ -12,9 +13,11 @@
 #include "chrome/browser/sync/test/integration/sync_integration_test_util.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
 #include "chrome/browser/sync/user_event_service_factory.h"
+#include "components/sync/driver/sync_driver_switches.h"
 #include "components/sync/model/model_type_sync_bridge.h"
 #include "components/sync/protocol/user_event_specifics.pb.h"
 #include "components/sync/user_events/user_event_service.h"
+#include "components/unified_consent/scoped_unified_consent.h"
 #include "components/variations/variations_associated_data.h"
 
 using fake_server::FakeServer;
@@ -287,9 +290,11 @@ IN_PROC_BROWSER_TEST_F(SingleClientUserEventsSyncTest, NoHistory) {
   EXPECT_TRUE(ExpectUserEvents({testEvent1, consent1, consent2, testEvent3}));
 }
 
-// TODO(http://crbug.com/860616) User events continue to be synced even when
-// USER_EVENTS is disabled.
-IN_PROC_BROWSER_TEST_F(SingleClientUserEventsSyncTest, DISABLED_NoUserEvents) {
+IN_PROC_BROWSER_TEST_F(SingleClientUserEventsSyncTest, NoUserEvents) {
+  // Enable unified consent feature and the ones it depends on.
+  unified_consent::ScopedUnifiedConsent scoped_unified_consent(
+      unified_consent::UnifiedConsentFeatureState::kEnabledNoBump);
+
   const UserEventSpecifics testEvent1 = CreateTestEvent(1);
   const UserEventSpecifics testEvent2 = CreateTestEvent(2);
   const UserEventSpecifics testEvent3 = CreateTestEvent(3);
@@ -314,6 +319,9 @@ IN_PROC_BROWSER_TEST_F(SingleClientUserEventsSyncTest, DISABLED_NoUserEvents) {
 
 // Test that events that are logged before sync is enabled don't get lost.
 IN_PROC_BROWSER_TEST_F(SingleClientUserEventsSyncTest, LoggedBeforeSyncSetup) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndDisableFeature(switches::kSyncUserConsentSeparateType);
+
   const UserEventSpecifics consent1 = CreateUserConsent(1);
   const UserEventSpecifics consent2 = CreateUserConsent(2);
   ASSERT_TRUE(SetupClients());
@@ -341,6 +349,9 @@ IN_PROC_BROWSER_TEST_F(SingleClientUserEventsSyncTest, NoSessions) {
 }
 
 IN_PROC_BROWSER_TEST_F(SingleClientUserEventsSyncTest, Encryption) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndDisableFeature(switches::kSyncUserConsentSeparateType);
+
   const UserEventSpecifics testEvent1 = CreateTestEvent(1);
   const UserEventSpecifics testEvent2 = CreateTestEvent(2);
   const UserEventSpecifics consent1 = CreateUserConsent(3);

components/sync/user_events/user_event_service_impl.cc

@@ -100,11 +100,21 @@ bool UserEventServiceImpl::MightRecordEvents(bool off_the_record,
 
 bool UserEventServiceImpl::CanRecordHistory() {
   // Before the engine is initialized, we cannot trust the other fields.
+  // TODO(vitaliii): Consider using GetState() instead of IsEngineInitialized(),
+  // because even when IsEngineInitialized() the user still may be configuring
+  // the datatypes.
   return sync_service_->IsEngineInitialized() &&
          !sync_service_->IsUsingSecondaryPassphrase() &&
          sync_service_->GetPreferredDataTypes().Has(HISTORY_DELETE_DIRECTIVES);
 }
 
+bool UserEventServiceImpl::IsUserEventsDatatypeEnabled() {
+  // Before the engine is initialized, we cannot trust the other fields.
+  return sync_service_->IsEngineInitialized() &&
+         !sync_service_->IsUsingSecondaryPassphrase() &&
+         sync_service_->GetPreferredDataTypes().Has(USER_EVENTS);
+}
+
 bool UserEventServiceImpl::ShouldRecordEvent(
     const UserEventSpecifics& specifics) {
   if (specifics.event_case() == UserEventSpecifics::EVENT_NOT_SET) {
@@ -116,10 +126,22 @@ bool UserEventServiceImpl::ShouldRecordEvent(
     return false;
   }
 
+  // TODO(vitaliii): Checking HISTORY_DELETE_DIRECTIVES directly should not be
+  // needed once USER_CONSENTS are fully launched. Then USER_EVENTS datatype
+  // should depend on History in Sync layers instead of here.
   if (specifics.has_navigation_id() && !CanRecordHistory()) {
     return false;
   }
 
+  // TODO(vitaliii): Checking USER_EVENTS directly should not be needed once
+  // https://crbug.com/830535 is fixed. Then disabling USER_EVENTS should be
+  // honored by the processor and it should drop all events.
+  if (base::FeatureList::IsEnabled(switches::kSyncUserConsentSeparateType) &&
+      !IsUserEventsDatatypeEnabled()) {
+    DCHECK(!specifics.has_user_consent());
+    return false;
+  }
+
   return true;
 }
 

components/sync/user_events/user_event_service_impl.h

@@ -43,6 +43,8 @@ class UserEventServiceImpl : public UserEventService {
   // Whether allowed to record events that link to navigation data.
   bool CanRecordHistory();
 
+  bool IsUserEventsDatatypeEnabled();
+
   // Checks dynamic or event specific conditions.
   bool ShouldRecordEvent(const sync_pb::UserEventSpecifics& specifics);
 

components/sync/user_events/user_event_service_impl_unittest.cc

@@ -118,6 +118,11 @@ class UserEventServiceImplTest : public testing::Test {
         mock_processor_.CreateForwardingProcessor(), &mapper_);
   }
 
+  void SetIsSeparateConsentTypeEnabledFeature(bool new_value) {
+    feature_list_.InitWithFeatureState(switches::kSyncUserConsentSeparateType,
+                                       new_value);
+  }
+
   TestSyncService* sync_service() { return &sync_service_; }
   MockModelTypeChangeProcessor* mock_processor() { return &mock_processor_; }
 
@@ -127,6 +132,8 @@ class UserEventServiceImplTest : public testing::Test {
   TestSyncService sync_service_;
   testing::NiceMock<MockModelTypeChangeProcessor> mock_processor_;
   TestGlobalIdMapper mapper_;
+
+  base::test::ScopedFeatureList feature_list_;
 };
 
 TEST_F(UserEventServiceImplTest, MightRecordEventsFeatureEnabled) {
@@ -146,12 +153,16 @@ TEST_F(UserEventServiceImplTest, MightRecordEventsFeatureDisabled) {
 }
 
 TEST_F(UserEventServiceImplTest, ShouldRecord) {
+  SetIsSeparateConsentTypeEnabledFeature(false);
+
   UserEventServiceImpl service(sync_service(), MakeBridge());
   EXPECT_CALL(*mock_processor(), Put(_, _, _));
   service.RecordUserEvent(AsTest(Event()));
 }
 
 TEST_F(UserEventServiceImplTest, ShouldRecordNoHistory) {
+  SetIsSeparateConsentTypeEnabledFeature(false);
+
   TestSyncService no_history_sync_service(true, false, ModelTypeSet());
   UserEventServiceImpl service(&no_history_sync_service, MakeBridge());
 
@@ -163,6 +174,8 @@ TEST_F(UserEventServiceImplTest, ShouldRecordNoHistory) {
 }
 
 TEST_F(UserEventServiceImplTest, ShouldRecordUserConsentNoHistory) {
+  SetIsSeparateConsentTypeEnabledFeature(false);
+
   TestSyncService no_history_sync_service(true, false, ModelTypeSet());
   UserEventServiceImpl service(&no_history_sync_service, MakeBridge());
 
@@ -172,6 +185,8 @@ TEST_F(UserEventServiceImplTest, ShouldRecordUserConsentNoHistory) {
 }
 
 TEST_F(UserEventServiceImplTest, ShouldRecordPassphrase) {
+  SetIsSeparateConsentTypeEnabledFeature(false);
+
   TestSyncService passphrase_sync_service(true, true,
                                           {HISTORY_DELETE_DIRECTIVES});
   UserEventServiceImpl service(&passphrase_sync_service, MakeBridge());
@@ -185,6 +200,8 @@ TEST_F(UserEventServiceImplTest, ShouldRecordPassphrase) {
 }
 
 TEST_F(UserEventServiceImplTest, ShouldRecordEngineOff) {
+  SetIsSeparateConsentTypeEnabledFeature(false);
+
   TestSyncService engine_not_initialized_sync_service(
       false, false, {HISTORY_DELETE_DIRECTIVES});
   UserEventServiceImpl service(&engine_not_initialized_sync_service,
@@ -208,6 +225,8 @@ TEST_F(UserEventServiceImplTest, ShouldRecordEmpty) {
 }
 
 TEST_F(UserEventServiceImplTest, ShouldRecordHasNavigationId) {
+  SetIsSeparateConsentTypeEnabledFeature(false);
+
   UserEventServiceImpl service(sync_service(), MakeBridge());
 
   // Verify logic for types that might or might not have a navigation id.
@@ -230,6 +249,8 @@ TEST_F(UserEventServiceImplTest, ShouldRecordHasNavigationId) {
 }
 
 TEST_F(UserEventServiceImplTest, SessionIdIsDifferent) {
+  SetIsSeparateConsentTypeEnabledFeature(false);
+
   std::vector<int64_t> put_session_ids;
   ON_CALL(*mock_processor(), Put(_, _, _))
       .WillByDefault([&](const std::string& storage_key,
@@ -250,6 +271,8 @@ TEST_F(UserEventServiceImplTest, SessionIdIsDifferent) {
 }
 
 TEST_F(UserEventServiceImplTest, FieldTrial) {
+  SetIsSeparateConsentTypeEnabledFeature(false);
+
   variations::AssociateGoogleVariationID(variations::CHROME_SYNC_EVENT_LOGGER,
                                          "trial", "group", 123);
   base::FieldTrialList::CreateFieldTrial("trial", "group");
@@ -259,6 +282,85 @@ TEST_F(UserEventServiceImplTest, FieldTrial) {
   UserEventServiceImpl service(sync_service(), MakeBridge());
 }
 
+TEST_F(
+    UserEventServiceImplTest,
+    WithConsentsTypeShouldRecordWhenBothHistoryAndEventsDatatypesAreEnabled) {
+  SetIsSeparateConsentTypeEnabledFeature(true);
+
+  TestSyncService sync_service(
+      /*is_engine_initialized=*/true,
+      /*is_using_secondary_passphrase=*/false,
+      /*preferred_data_types=*/{HISTORY_DELETE_DIRECTIVES, USER_EVENTS});
+
+  UserEventServiceImpl service(&sync_service, MakeBridge());
+  EXPECT_CALL(*mock_processor(), Put(_, _, _));
+  service.RecordUserEvent(AsTest(Event()));
+}
+
+TEST_F(UserEventServiceImplTest,
+       WithConsentsTypeShouldNotRecordWhenEventsDatatypeIsDisabled) {
+  SetIsSeparateConsentTypeEnabledFeature(true);
+
+  TestSyncService sync_service(
+      /*is_engine_initialized=*/true,
+      /*is_using_secondary_passphrase=*/false,
+      /*preferred_data_types=*/{HISTORY_DELETE_DIRECTIVES});
+
+  UserEventServiceImpl service(&sync_service, MakeBridge());
+  // USER_EVENTS type is disabled, thus, they should not be recorded.
+  EXPECT_CALL(*mock_processor(), Put(_, _, _)).Times(0);
+  service.RecordUserEvent(AsTest(Event()));
+}
+
+TEST_F(UserEventServiceImplTest,
+       WithConsentsTypeShouldNotRecordWhenHistoryDatatypeIsDisabled) {
+  SetIsSeparateConsentTypeEnabledFeature(true);
+
+  TestSyncService sync_service(
+      /*is_engine_initialized=*/true,
+      /*is_using_secondary_passphrase=*/false,
+      /*preferred_data_types=*/{USER_EVENTS});
+
+  UserEventServiceImpl service(&sync_service, MakeBridge());
+  // Even though USER_EVENTS type is enabled, events cannot be recorded when
+  // history sync is disabled.
+  EXPECT_CALL(*mock_processor(), Put(_, _, _)).Times(0);
+  // Use |WithNav| because only events with navigation id depend on history.
+  service.RecordUserEvent(WithNav(Event()));
+}
+
+TEST_F(UserEventServiceImplTest,
+       WithConsentsTypeShouldNotRecordWhenEngineIsNotInitialized) {
+  SetIsSeparateConsentTypeEnabledFeature(true);
+
+  TestSyncService sync_service(
+      /*is_engine_initialized=*/false,
+      /*is_using_secondary_passphrase=*/false,
+      /*preferred_data_types=*/{HISTORY_DELETE_DIRECTIVES, USER_EVENTS});
+
+  UserEventServiceImpl service(&sync_service, MakeBridge());
+  // Even though USER_EVENTS type is enabled, events cannot be recorded because
+  // we can't trust uninitialized engine.
+  EXPECT_CALL(*mock_processor(), Put(_, _, _)).Times(0);
+  service.RecordUserEvent(AsTest(Event()));
+}
+
+TEST_F(UserEventServiceImplTest,
+       WithConsentsTypeShouldNotRecordWhenPassphraseIsUsed) {
+  SetIsSeparateConsentTypeEnabledFeature(true);
+
+  TestSyncService sync_service(
+      /*is_engine_initialized=*/true,
+      /*is_using_secondary_passphrase=*/true,
+      /*preferred_data_types=*/{HISTORY_DELETE_DIRECTIVES, USER_EVENTS});
+
+  UserEventServiceImpl service(&sync_service, MakeBridge());
+  // Even though USER_EVENTS type is enabled, events cannot be recorded
+  // because custom passphrase is used.
+  EXPECT_CALL(*mock_processor(), Put(_, _, _)).Times(0);
+  service.RecordUserEvent(AsTest(Event()));
+}
+
 }  // namespace
 
 }  // namespace syncer