[DeviceSync v2] Simplify CryptAuthFeatureType functions

Provide to/from conversion functions between CryptAuthFeatureType and
 * the string used for protos
 * multidevice::SoftwareFeature

Add unit tests for conversions to and from CryptAuthFeatureType.

Bug: 951969
Change-Id: Ic2ffdcd76ee69b3ed36901848e61c842e3230d51
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1917980
Commit-Queue: Josh Nohle <nohle@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#716037}

chromeos/services/device_sync/BUILD.gn

@@ -259,6 +259,7 @@ source_set("unit_tests") {
     "cryptauth_enrollment_manager_impl_unittest.cc",
     "cryptauth_feature_status_getter_impl_unittest.cc",
     "cryptauth_feature_status_setter_impl_unittest.cc",
+    "cryptauth_feature_type_unittest.cc",
     "cryptauth_gcm_manager_impl_unittest.cc",
     "cryptauth_group_private_key_sharer_impl_unittest.cc",
     "cryptauth_key_bundle_unittest.cc",

chromeos/services/device_sync/cryptauth_feature_status_getter_impl.cc

@@ -78,29 +78,28 @@ ConvertFeatureStatusesToSoftwareFeatureMap(
   base::flat_set<multidevice::SoftwareFeature> marked_enabled;
   for (const cryptauthv2::DeviceFeatureStatus::FeatureStatus& status :
        feature_statuses) {
+    base::Optional<CryptAuthFeatureType> feature_type =
+        CryptAuthFeatureTypeFromString(status.feature_type());
+
     // TODO(https://crbug.com/936273): Add metrics to track unknown feature type
     // occurrences.
-    if (!base::Contains(GetCryptAuthFeatureTypeStrings(),
-                        status.feature_type())) {
+    if (!feature_type) {
       PA_LOG(ERROR) << "Unknown feature type: " << status.feature_type();
       *did_non_fatal_error_occur = true;
       continue;
     }
 
-    multidevice::SoftwareFeature feature =
-        CryptAuthFeatureTypeStringToSoftwareFeature(status.feature_type());
-
-    if (base::Contains(GetSupportedCryptAuthFeatureTypeStrings(),
-                       status.feature_type()) &&
+    if (base::Contains(GetSupportedCryptAuthFeatureTypes(), *feature_type) &&
         status.enabled()) {
-      marked_supported.insert(feature);
+      marked_supported.insert(
+          CryptAuthFeatureTypeToSoftwareFeature(*feature_type));
       continue;
     }
 
-    if (base::Contains(GetEnabledCryptAuthFeatureTypeStrings(),
-                       status.feature_type()) &&
+    if (base::Contains(GetEnabledCryptAuthFeatureTypes(), *feature_type) &&
         status.enabled()) {
-      marked_enabled.insert(feature);
+      marked_enabled.insert(
+          CryptAuthFeatureTypeToSoftwareFeature(*feature_type));
       continue;
     }
   }
@@ -197,8 +196,9 @@ void CryptAuthFeatureStatusGetterImpl::OnAttemptStarted(
   cryptauthv2::BatchGetFeatureStatusesRequest request;
   request.mutable_context()->CopyFrom(request_context);
   *request.mutable_device_ids() = {device_ids.begin(), device_ids.end()};
-  *request.mutable_feature_types() = {GetCryptAuthFeatureTypeStrings().begin(),
-                                      GetCryptAuthFeatureTypeStrings().end()};
+  *request.mutable_feature_types() = {
+      GetAllCryptAuthFeatureTypeStrings().begin(),
+      GetAllCryptAuthFeatureTypeStrings().end()};
 
   start_get_feature_statuses_timestamp_ = base::TimeTicks::Now();
 

chromeos/services/device_sync/cryptauth_feature_status_getter_impl_unittest.cc

@@ -59,17 +59,17 @@ const cryptauthv2::RequestContext& GetRequestContext() {
 
 cryptauthv2::DeviceFeatureStatus ConvertDeviceToDeviceFeatureStatus(
     const CryptAuthDevice& device,
-    const base::flat_set<std::string>& feature_types) {
+    const base::flat_set<CryptAuthFeatureType>& feature_types) {
   cryptauthv2::DeviceFeatureStatus device_feature_status;
   device_feature_status.set_device_id(device.instance_id());
 
   int64_t last_modified_time_offset_millis = 0;
-  for (const std::string& feature_type : feature_types) {
+  for (CryptAuthFeatureType feature_type : feature_types) {
     bool is_supported_feature_type =
-        base::Contains(GetSupportedCryptAuthFeatureTypeStrings(), feature_type);
+        base::Contains(GetSupportedCryptAuthFeatureTypes(), feature_type);
 
     const auto it = device.feature_states.find(
-        CryptAuthFeatureTypeStringToSoftwareFeature(feature_type));
+        CryptAuthFeatureTypeToSoftwareFeature(feature_type));
     bool is_supported =
         it != device.feature_states.end() &&
         it->second != multidevice::SoftwareFeatureState::kNotSupported;
@@ -87,12 +87,13 @@ cryptauthv2::DeviceFeatureStatus ConvertDeviceToDeviceFeatureStatus(
                          last_modified_time_offset_millis));
     ++last_modified_time_offset_millis;
 
-    feature_status->set_feature_type(feature_type);
+    feature_status->set_feature_type(
+        CryptAuthFeatureTypeToString(feature_type));
     if (is_supported_feature_type) {
       feature_status->set_enabled(is_supported);
     } else {
-      EXPECT_TRUE(base::Contains(GetEnabledCryptAuthFeatureTypeStrings(),
-                                 feature_type));
+      EXPECT_TRUE(
+          base::Contains(GetEnabledCryptAuthFeatureTypes(), feature_type));
       feature_status->set_enabled(is_enabled);
     }
   }
@@ -159,7 +160,7 @@ class DeviceSyncCryptAuthFeatureStatusGetterImplTest
               base::flat_set<std::string>(
                   batch_get_feature_statuses_request_->device_ids().begin(),
                   batch_get_feature_statuses_request_->device_ids().end()));
-    EXPECT_EQ(GetCryptAuthFeatureTypeStrings(),
+    EXPECT_EQ(GetAllCryptAuthFeatureTypeStrings(),
               base::flat_set<std::string>(
                   batch_get_feature_statuses_request_->feature_types().begin(),
                   batch_get_feature_statuses_request_->feature_types().end()));
@@ -167,7 +168,7 @@ class DeviceSyncCryptAuthFeatureStatusGetterImplTest
 
   void SendCorrectBatchGetFeatureStatusesResponse(
       const base::flat_set<std::string>& device_ids,
-      const base::flat_set<std::string>& feature_types) {
+      const base::flat_set<CryptAuthFeatureType>& feature_types) {
     cryptauthv2::BatchGetFeatureStatusesResponse response;
     for (const std::string& device_id : device_ids) {
       base::Optional<CryptAuthDevice> device = GetTestDeviceWithId(device_id);
@@ -262,7 +263,7 @@ TEST_F(DeviceSyncCryptAuthFeatureStatusGetterImplTest, Success) {
   VerifyBatchGetFeatureStatusesRequest(GetAllTestDeviceIds());
 
   SendCorrectBatchGetFeatureStatusesResponse(GetAllTestDeviceIds(),
-                                             GetCryptAuthFeatureTypeStrings());
+                                             GetAllCryptAuthFeatureTypes());
 
   VerifyGetFeatureStatuesResult(
       GetAllTestDeviceIds(), CryptAuthDeviceSyncResult::ResultCode::kSuccess);
@@ -279,7 +280,7 @@ TEST_F(DeviceSyncCryptAuthFeatureStatusGetterImplTest,
   // Include an unknown feature type string in the response. The unknown feature
   // type should be ignored.
   cryptauthv2::DeviceFeatureStatus status = ConvertDeviceToDeviceFeatureStatus(
-      GetLocalDeviceForTest(), GetCryptAuthFeatureTypeStrings());
+      GetLocalDeviceForTest(), GetAllCryptAuthFeatureTypes());
   status.add_feature_statuses()->set_feature_type("Unknown_feature_type");
 
   cryptauthv2::BatchGetFeatureStatusesResponse response;
@@ -300,7 +301,7 @@ TEST_F(DeviceSyncCryptAuthFeatureStatusGetterImplTest,
   VerifyBatchGetFeatureStatusesRequest(device_ids);
 
   cryptauthv2::DeviceFeatureStatus status = ConvertDeviceToDeviceFeatureStatus(
-      GetLocalDeviceForTest(), GetCryptAuthFeatureTypeStrings());
+      GetLocalDeviceForTest(), GetAllCryptAuthFeatureTypes());
 
   // The BetterTogether host feature is not supported for the local device.
   EXPECT_EQ(multidevice::SoftwareFeatureState::kNotSupported,
@@ -356,7 +357,7 @@ TEST_F(DeviceSyncCryptAuthFeatureStatusGetterImplTest,
   // Include features statuses for unrequested devices. These extra devices
   // should be ignored.
   SendCorrectBatchGetFeatureStatusesResponse(GetAllTestDeviceIds(),
-                                             GetCryptAuthFeatureTypeStrings());
+                                             GetAllCryptAuthFeatureTypes());
 
   VerifyGetFeatureStatuesResult(
       requested_device_ids,
@@ -374,7 +375,7 @@ TEST_F(DeviceSyncCryptAuthFeatureStatusGetterImplTest,
   // Send duplicate local device entries in the response. These duplicate
   // entries should be ignored.
   cryptauthv2::DeviceFeatureStatus status = ConvertDeviceToDeviceFeatureStatus(
-      GetLocalDeviceForTest(), GetCryptAuthFeatureTypeStrings());
+      GetLocalDeviceForTest(), GetAllCryptAuthFeatureTypes());
   cryptauthv2::BatchGetFeatureStatusesResponse response;
   response.add_device_feature_statuses()->CopyFrom(status);
   response.add_device_feature_statuses()->CopyFrom(status);
@@ -395,7 +396,7 @@ TEST_F(DeviceSyncCryptAuthFeatureStatusGetterImplTest,
   base::flat_set<std::string> returned_device_ids = {
       GetLocalDeviceMetadataPacketForTest().device_id()};
   SendCorrectBatchGetFeatureStatusesResponse(returned_device_ids,
-                                             GetCryptAuthFeatureTypeStrings());
+                                             GetAllCryptAuthFeatureTypes());
 
   VerifyGetFeatureStatuesResult(
       returned_device_ids,

chromeos/services/device_sync/cryptauth_feature_status_setter_impl.cc

@@ -213,8 +213,8 @@ void CryptAuthFeatureStatusSetterImpl::ProcessRequestQueue() {
   cryptauthv2::DeviceFeatureStatus::FeatureStatus* feature_status =
       device_feature_status->add_feature_statuses();
   feature_status->set_feature_type(
-      SoftwareFeatureToEnabledCryptAuthFeatureTypeString(
-          pending_requests_.front().feature));
+      CryptAuthFeatureTypeToString(CryptAuthFeatureTypeFromSoftwareFeature(
+          pending_requests_.front().feature)));
 
   switch (pending_requests_.front().status_change) {
     case FeatureStatusChange::kEnableExclusively:

chromeos/services/device_sync/cryptauth_feature_type.h

@@ -16,12 +16,14 @@ namespace chromeos {
 
 namespace device_sync {
 
-// The feature types used by CryptAuth v2 DeviceSync's BatchGetFeatureStatuses,
-// BatchSetFeatureStatuses, and BatchNotifyGroupDevices. Each enum value is
-// uniquely mapped to a string used in the protos and understood by CryptAuth.
+// The BetterTogether feature types used by the CryptAuth v2 backend. Each
+// feature has a separate type for the "supported" state and "enabled" state of
+// the feature. Currently, the "supported" types are only used for the CryptAuth
+// v2 DeviceSync BatchGetFeatureStatuses RPC. Each enum value is uniquely mapped
+// to a string used in the protos and understood by CryptAuth.
 //
 // Example: The following FeatureStatus messages received in a
-// BatchGetFeatureStatuses response would indicate that BetterTogetherHost is
+// BatchGetFeatureStatuses response would indicate that BetterTogether host is
 // supported but not enabled:
 //   [
 //     message FeatureStatus {
@@ -62,26 +64,34 @@ enum class CryptAuthFeatureType {
   kSmsConnectClientEnabled,
 };
 
-// Uniquely maps each CryptAuthFeatureType enum value to a string used in the
-// protos and understood by CryptAuth.
-const char* CryptAuthFeatureTypeToString(CryptAuthFeatureType feature_type);
-
-// Uniquely maps each SoftwareFeature enum value to a string used in the protos
-// and understood by CryptAuth.
-const char* SoftwareFeatureToEnabledCryptAuthFeatureTypeString(
-    multidevice::SoftwareFeature software_feature);
+const base::flat_set<CryptAuthFeatureType>& GetAllCryptAuthFeatureTypes();
+const base::flat_set<CryptAuthFeatureType>& GetSupportedCryptAuthFeatureTypes();
+const base::flat_set<CryptAuthFeatureType>& GetEnabledCryptAuthFeatureTypes();
+const base::flat_set<std::string>& GetAllCryptAuthFeatureTypeStrings();
 
-// Returns null if |feature_type_string| does not map to a known
-// CryptAuthFeatureType.
+// Provides a unique mapping between each CryptAuthFeatureType enum value and
+// the corresponding string used in the protos and understood by CryptAuth.
+// CryptAuthFeatureTypeFromString returns null if |feature_type_string| does not
+// map to a known CryptAuthFeatureType.
+const char* CryptAuthFeatureTypeToString(CryptAuthFeatureType feature_type);
 base::Optional<CryptAuthFeatureType> CryptAuthFeatureTypeFromString(
     const std::string& feature_type_string);
 
-multidevice::SoftwareFeature CryptAuthFeatureTypeStringToSoftwareFeature(
-    const std::string& feature_type_string);
-
-const base::flat_set<std::string>& GetCryptAuthFeatureTypeStrings();
-const base::flat_set<std::string>& GetSupportedCryptAuthFeatureTypeStrings();
-const base::flat_set<std::string>& GetEnabledCryptAuthFeatureTypeStrings();
+// Provides a mapping between CryptAuthFeatureTypes and SoftwareFeatures.
+//
+// The "enabled" and "supported" feature types are mapped to the same
+// SoftwareFeature. For example,
+// CryptAuthFeatureType::kBetterTogetherHostEnabled and
+// CryptAuthFeatureType::kBetterTogetherHostSupported are both mapped to
+// SoftwareFeature::kBetterTogetherHost.
+//
+// A SoftwareFeature is mapped to the "enabled" CryptAuthFeatureType. For
+// example, SoftwareFeature::kBetterTogetherHost maps to
+// CryptAuthFeatureType::kBetterTogetherHostEnabled.
+multidevice::SoftwareFeature CryptAuthFeatureTypeToSoftwareFeature(
+    CryptAuthFeatureType feature_type);
+CryptAuthFeatureType CryptAuthFeatureTypeFromSoftwareFeature(
+    multidevice::SoftwareFeature software_feature);
 
 std::ostream& operator<<(std::ostream& stream,
                          CryptAuthFeatureType feature_type);

chromeos/services/device_sync/cryptauth_feature_type_unittest.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chromeos/services/device_sync/cryptauth_feature_type.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace chromeos {
+
+namespace device_sync {
+
+TEST(DeviceSyncCryptAuthFeatureTypeTest, ToAndFromString) {
+  for (CryptAuthFeatureType feature_type : GetAllCryptAuthFeatureTypes()) {
+    EXPECT_EQ(feature_type, CryptAuthFeatureTypeFromString(
+                                CryptAuthFeatureTypeToString(feature_type)));
+  }
+}
+
+TEST(DeviceSyncCryptAuthFeatureTypeTest, ToAndFromSoftwareFeature) {
+  // SoftwareFeature map onto the "enabled" feature types
+  for (CryptAuthFeatureType feature_type : GetEnabledCryptAuthFeatureTypes()) {
+    EXPECT_EQ(feature_type,
+              CryptAuthFeatureTypeFromSoftwareFeature(
+                  CryptAuthFeatureTypeToSoftwareFeature(feature_type)));
+  }
+}
+
+}  // namespace device_sync
+
+}  // namespace chromeos