[CrOS MultiDevice] Update "feature states" computations.

This CL adds functionality for two edge cases:
(1) If all sub-features are prohibited by policy, the Better Together
    suite should also be prohibited.
(2) If a feature would have been enabled except that the suite as a
    whole is disabled, the feature is now in the
    kUnavailableSuiteDisabled state.

Note: This CL also renames the kDisabledByPolicy enum value to be
kProhibitedByPolicy instead for clarity.

Bug: 870113, 824568
Change-Id: I11bdfcfbfd3313490599831606d31cb5ffa22983
Reviewed-on: https://chromium-review.googlesource.com/1187834
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586095}

chrome/browser/resources/settings/multidevice_page/multidevice_constants.js

@@ -38,13 +38,14 @@ cr.define('settings', function() {
    * @enum {number}
    */
   MultiDeviceFeatureState = {
-    DISABLED_BY_POLICY: 0,
+    PROHIBITED_BY_POLICY: 0,
     DISABLED_BY_USER: 1,
     ENABLED_BY_USER: 2,
     NOT_SUPPORTED_BY_CHROMEBOOK: 3,
     NOT_SUPPORTED_BY_PHONE: 4,
     UNAVAILABLE_NO_VERIFIED_HOST: 5,
     UNAVAILABLE_INSUFFICIENT_SECURITY: 6,
+    UNAVAILABLE_SUITE_DISABLED: 7,
   };
 
   return {

chrome/browser/resources/settings/multidevice_page/multidevice_feature_behavior.js

@@ -50,10 +50,11 @@ const MultiDeviceFeatureBehaviorImpl = {
     }
 
     if ([
-          settings.MultiDeviceFeatureState.DISABLED_BY_POLICY,
+          settings.MultiDeviceFeatureState.PROHIBITED_BY_POLICY,
           settings.MultiDeviceFeatureState.NOT_SUPPORTED_BY_CHROMEBOOK,
           settings.MultiDeviceFeatureState.NOT_SUPPORTED_BY_PHONE,
           settings.MultiDeviceFeatureState.UNAVAILABLE_INSUFFICIENT_SECURITY,
+          settings.MultiDeviceFeatureState.UNAVAILABLE_SUITE_DISABLED,
         ].includes(this.getFeatureState(feature))) {
       return false;
     }

chrome/test/data/webui/settings/multidevice_feature_toggle_tests.js

@@ -76,7 +76,7 @@ suite('Multidevice', () => {
   });
 
   test('disabled property can be set by feature state', () => {
-    setMessagesState(settings.MultiDeviceFeatureState.DISABLED_BY_POLICY);
+    setMessagesState(settings.MultiDeviceFeatureState.PROHIBITED_BY_POLICY);
     assertTrue(crToggle.disabled);
 
     setMessagesState(settings.MultiDeviceFeatureState.DISABLED_BY_USER);
@@ -89,7 +89,7 @@ suite('Multidevice', () => {
     assertTrue(crToggle.checked);
     assertFalse(crToggle.disabled);
 
-    setMessagesState(settings.MultiDeviceFeatureState.DISABLED_BY_POLICY);
+    setMessagesState(settings.MultiDeviceFeatureState.PROHIBITED_BY_POLICY);
     assertFalse(featureToggle.checked_);
     assertFalse(crToggle.checked);
     assertTrue(crToggle.disabled);

chromeos/services/multidevice_setup/feature_state_manager_impl.cc

@@ -54,6 +54,42 @@ GenerateInitialDefaultCachedStateMap() {
        mojom::FeatureState::kUnavailableNoVerifiedHost}};
 }
 
+void ProcessSuiteEdgeCases(
+    FeatureStateManager::FeatureStatesMap* feature_states_map) {
+  // First edge case: The Better Together suite does not have its own explicit
+  // device policy; instead, if all supported sub-features are prohibited by
+  // policy, the entire suite should be considered prohibited.
+  bool are_all_sub_features_prohibited = true;
+  for (const auto& map_entry : *feature_states_map) {
+    // Only check for sub-features.
+    if (map_entry.first == mojom::Feature::kBetterTogetherSuite)
+      continue;
+
+    if (map_entry.second != mojom::FeatureState::kProhibitedByPolicy) {
+      are_all_sub_features_prohibited = false;
+      break;
+    }
+  }
+
+  if (are_all_sub_features_prohibited) {
+    (*feature_states_map)[mojom::Feature::kBetterTogetherSuite] =
+        mojom::FeatureState::kProhibitedByPolicy;
+    return;
+  }
+
+  // Second edge case: If the Better Together suite is disabled by the user, any
+  // sub-features which have been enabled by the user should be unavailable. In
+  // this context, the suite serves as a gatekeeper to all sub-features.
+  if ((*feature_states_map)[mojom::Feature::kBetterTogetherSuite] !=
+      mojom::FeatureState::kDisabledByUser)
+    return;
+
+  for (auto& map_entry : *feature_states_map) {
+    if (map_entry.second == mojom::FeatureState::kEnabledByUser)
+      map_entry.second = mojom::FeatureState::kUnavailableSuiteDisabled;
+  }
+}
+
 }  // namespace
 
 // static
@@ -162,13 +198,16 @@ void FeatureStateManagerImpl::UpdateFeatureStateCache(
   FeatureStatesMap previous_cached_feature_state_map =
       cached_feature_state_map_;
 
-  // Update |cached_feature_state_map_|.
+  // Update |cached_feature_state_map_| with computed values.
   auto it = cached_feature_state_map_.begin();
   while (it != cached_feature_state_map_.end()) {
     it->second = ComputeFeatureState(it->first);
     ++it;
   }
 
+  // Some computed values must be updated to support various edge cases.
+  ProcessSuiteEdgeCases(&cached_feature_state_map_);
+
   if (previous_cached_feature_state_map == cached_feature_state_map_)
     return;
 
@@ -177,6 +216,9 @@ void FeatureStateManagerImpl::UpdateFeatureStateCache(
 
 mojom::FeatureState FeatureStateManagerImpl::ComputeFeatureState(
     mojom::Feature feature) {
+  if (!IsAllowedByPolicy(feature))
+    return mojom::FeatureState::kProhibitedByPolicy;
+
   HostStatusProvider::HostStatusWithDevice status_with_device =
       host_status_provider_->GetHostWithStatus();
 
@@ -195,6 +237,15 @@ mojom::FeatureState FeatureStateManagerImpl::ComputeFeatureState(
   return GetEnabledOrDisabledState(feature);
 }
 
+bool FeatureStateManagerImpl::IsAllowedByPolicy(mojom::Feature feature) {
+  // If no policy preference exists for this feature, the feature is implicitly
+  // allowed.
+  if (!base::ContainsKey(feature_to_allowed_pref_name_map_, feature))
+    return true;
+
+  return pref_service_->GetBoolean(feature_to_allowed_pref_name_map_[feature]);
+}
+
 bool FeatureStateManagerImpl::IsSupportedByChromebook(mojom::Feature feature) {
   static const std::pair<mojom::Feature, cryptauth::SoftwareFeature>
       kFeatureAndClientSoftwareFeaturePairs[] = {
@@ -262,13 +313,6 @@ bool FeatureStateManagerImpl::HasBeenActivatedByPhone(
 
 mojom::FeatureState FeatureStateManagerImpl::GetEnabledOrDisabledState(
     mojom::Feature feature) {
-  // If an "allowed pref" exists for this feature and that preference is set to
-  // false, the feature is disabled by policy.
-  if (base::ContainsKey(feature_to_allowed_pref_name_map_, feature) &&
-      !pref_service_->GetBoolean(feature_to_allowed_pref_name_map_[feature])) {
-    return mojom::FeatureState::kDisabledByPolicy;
-  }
-
   if (!base::ContainsKey(feature_to_enabled_pref_name_map_, feature)) {
     PA_LOG(ERROR) << "FeatureStateManagerImpl::GetEnabledOrDisabledState(): "
                   << "Feature not present in \"enabled pref\" map: " << feature;

chromeos/services/multidevice_setup/feature_state_manager_impl.h

@@ -64,6 +64,7 @@ class FeatureStateManagerImpl : public FeatureStateManager,
   void OnPrefValueChanged();
   void UpdateFeatureStateCache(bool notify_observers_of_changes);
   mojom::FeatureState ComputeFeatureState(mojom::Feature feature);
+  bool IsAllowedByPolicy(mojom::Feature feature);
   bool IsSupportedByChromebook(mojom::Feature feature);
   bool HasSufficientSecurity(mojom::Feature feature,
                              const cryptauth::RemoteDeviceRef& host_device);

chromeos/services/multidevice_setup/feature_state_manager_impl_unittest.cc

@@ -138,6 +138,16 @@ class MultiDeviceSetupFeatureStateManagerImplTest : public testing::Test {
               fake_observer_->feature_state_updates().size());
   }
 
+  void MakeBetterTogetherSuiteDisabledByUser() {
+    SetSoftwareFeatureState(true /* use_local_device */,
+                            cryptauth::SoftwareFeature::BETTER_TOGETHER_CLIENT,
+                            cryptauth::SoftwareFeatureState::kSupported);
+    test_pref_service_->SetBoolean(kBetterTogetherSuiteEnabledPrefName, false);
+    EXPECT_EQ(
+        mojom::FeatureState::kDisabledByUser,
+        manager_->GetFeatureStates()[mojom::Feature::kBetterTogetherSuite]);
+  }
+
   void VerifyFeatureStateChange(size_t expected_index,
                                 mojom::Feature expected_feature,
                                 mojom::FeatureState expected_feature_state) {
@@ -207,6 +217,18 @@ TEST_F(MultiDeviceSetupFeatureStateManagerImplTest, BetterTogetherSuite) {
   VerifyFeatureStateChange(2u /* expected_index */,
                            mojom::Feature::kBetterTogetherSuite,
                            mojom::FeatureState::kDisabledByUser);
+
+  // Set all features to prohibited. This should cause the Better Together suite
+  // to become prohibited as well.
+  test_pref_service()->SetBoolean(kInstantTetheringAllowedPrefName, false);
+  test_pref_service()->SetBoolean(kMessagesAllowedPrefName, false);
+  test_pref_service()->SetBoolean(kSmartLockAllowedPrefName, false);
+  EXPECT_EQ(
+      mojom::FeatureState::kProhibitedByPolicy,
+      manager()->GetFeatureStates()[mojom::Feature::kBetterTogetherSuite]);
+  VerifyFeatureStateChange(5u /* expected_index */,
+                           mojom::Feature::kBetterTogetherSuite,
+                           mojom::FeatureState::kProhibitedByPolicy);
 }
 
 TEST_F(MultiDeviceSetupFeatureStateManagerImplTest, InstantTethering) {
@@ -235,19 +257,26 @@ TEST_F(MultiDeviceSetupFeatureStateManagerImplTest, InstantTethering) {
                            mojom::Feature::kInstantTethering,
                            mojom::FeatureState::kEnabledByUser);
 
+  MakeBetterTogetherSuiteDisabledByUser();
+  EXPECT_EQ(mojom::FeatureState::kUnavailableSuiteDisabled,
+            manager()->GetFeatureStates()[mojom::Feature::kInstantTethering]);
+  VerifyFeatureStateChange(4u /* expected_index */,
+                           mojom::Feature::kInstantTethering,
+                           mojom::FeatureState::kUnavailableSuiteDisabled);
+
   test_pref_service()->SetBoolean(kInstantTetheringEnabledPrefName, false);
   EXPECT_EQ(mojom::FeatureState::kDisabledByUser,
             manager()->GetFeatureStates()[mojom::Feature::kInstantTethering]);
-  VerifyFeatureStateChange(3u /* expected_index */,
+  VerifyFeatureStateChange(5u /* expected_index */,
                            mojom::Feature::kInstantTethering,
                            mojom::FeatureState::kDisabledByUser);
 
   test_pref_service()->SetBoolean(kInstantTetheringAllowedPrefName, false);
-  EXPECT_EQ(mojom::FeatureState::kDisabledByPolicy,
+  EXPECT_EQ(mojom::FeatureState::kProhibitedByPolicy,
             manager()->GetFeatureStates()[mojom::Feature::kInstantTethering]);
-  VerifyFeatureStateChange(4u /* expected_index */,
+  VerifyFeatureStateChange(6u /* expected_index */,
                            mojom::Feature::kInstantTethering,
-                           mojom::FeatureState::kDisabledByPolicy);
+                           mojom::FeatureState::kProhibitedByPolicy);
 }
 
 TEST_F(MultiDeviceSetupFeatureStateManagerImplTest, Messages) {
@@ -273,17 +302,23 @@ TEST_F(MultiDeviceSetupFeatureStateManagerImplTest, Messages) {
   VerifyFeatureStateChange(2u /* expected_index */, mojom::Feature::kMessages,
                            mojom::FeatureState::kEnabledByUser);
 
+  MakeBetterTogetherSuiteDisabledByUser();
+  EXPECT_EQ(mojom::FeatureState::kUnavailableSuiteDisabled,
+            manager()->GetFeatureStates()[mojom::Feature::kMessages]);
+  VerifyFeatureStateChange(4u /* expected_index */, mojom::Feature::kMessages,
+                           mojom::FeatureState::kUnavailableSuiteDisabled);
+
   test_pref_service()->SetBoolean(kMessagesEnabledPrefName, false);
   EXPECT_EQ(mojom::FeatureState::kDisabledByUser,
             manager()->GetFeatureStates()[mojom::Feature::kMessages]);
-  VerifyFeatureStateChange(3u /* expected_index */, mojom::Feature::kMessages,
+  VerifyFeatureStateChange(5u /* expected_index */, mojom::Feature::kMessages,
                            mojom::FeatureState::kDisabledByUser);
 
   test_pref_service()->SetBoolean(kMessagesAllowedPrefName, false);
-  EXPECT_EQ(mojom::FeatureState::kDisabledByPolicy,
+  EXPECT_EQ(mojom::FeatureState::kProhibitedByPolicy,
             manager()->GetFeatureStates()[mojom::Feature::kMessages]);
-  VerifyFeatureStateChange(4u /* expected_index */, mojom::Feature::kMessages,
-                           mojom::FeatureState::kDisabledByPolicy);
+  VerifyFeatureStateChange(6u /* expected_index */, mojom::Feature::kMessages,
+                           mojom::FeatureState::kProhibitedByPolicy);
 }
 
 TEST_F(MultiDeviceSetupFeatureStateManagerImplTest, SmartLock) {
@@ -310,17 +345,23 @@ TEST_F(MultiDeviceSetupFeatureStateManagerImplTest, SmartLock) {
   VerifyFeatureStateChange(2u /* expected_index */, mojom::Feature::kSmartLock,
                            mojom::FeatureState::kEnabledByUser);
 
+  MakeBetterTogetherSuiteDisabledByUser();
+  EXPECT_EQ(mojom::FeatureState::kUnavailableSuiteDisabled,
+            manager()->GetFeatureStates()[mojom::Feature::kSmartLock]);
+  VerifyFeatureStateChange(4u /* expected_index */, mojom::Feature::kSmartLock,
+                           mojom::FeatureState::kUnavailableSuiteDisabled);
+
   test_pref_service()->SetBoolean(kSmartLockEnabledPrefName, false);
   EXPECT_EQ(mojom::FeatureState::kDisabledByUser,
             manager()->GetFeatureStates()[mojom::Feature::kSmartLock]);
-  VerifyFeatureStateChange(3u /* expected_index */, mojom::Feature::kSmartLock,
+  VerifyFeatureStateChange(5u /* expected_index */, mojom::Feature::kSmartLock,
                            mojom::FeatureState::kDisabledByUser);
 
   test_pref_service()->SetBoolean(kSmartLockAllowedPrefName, false);
-  EXPECT_EQ(mojom::FeatureState::kDisabledByPolicy,
+  EXPECT_EQ(mojom::FeatureState::kProhibitedByPolicy,
             manager()->GetFeatureStates()[mojom::Feature::kSmartLock]);
-  VerifyFeatureStateChange(4u /* expected_index */, mojom::Feature::kSmartLock,
-                           mojom::FeatureState::kDisabledByPolicy);
+  VerifyFeatureStateChange(6u /* expected_index */, mojom::Feature::kSmartLock,
+                           mojom::FeatureState::kProhibitedByPolicy);
 }
 
 }  // namespace multidevice_setup

chromeos/services/multidevice_setup/public/mojom/multidevice_setup.mojom

@@ -51,8 +51,8 @@ enum Feature {
 };
 
 enum FeatureState {
-  // Feature was disabled by a device policy (e.g., EDU or Enterprise).
-  kDisabledByPolicy,
+  // Feature was prohibited by a device policy (e.g., EDU or Enterprise).
+  kProhibitedByPolicy,
 
   // Feature was disabled by the user (i.e., via settings).
   kDisabledByUser,
@@ -74,7 +74,11 @@ enum FeatureState {
   // The feature is unavailable because there are insufficient security
   // mechanisms in place (e.g., Smart Lock returns this value when the host
   // phone device does not have a lock screen set).
-  kUnavailableInsufficientSecurity
+  kUnavailableInsufficientSecurity,
+
+  // The feature has been enabled by the user, but it is still unavailable
+  // because the entire Better Together suite has been disabled by the user.
+  kUnavailableSuiteDisabled
 };
 
 interface AccountStatusChangeDelegate {