Use enum for Settings::local_anonymous_access_role BUG:24267885 Change-Id: I406ed545f11be20177b9d38b3ea6fef9ded9a566 Reviewed-on: https://weave-review.googlesource.com/1200 Reviewed-by: Vitaly Buka <vitalybuka@google.com>
diff --git a/libweave/include/weave/cloud.h b/libweave/include/weave/cloud.h index ade4247..d3c9500 100644 --- a/libweave/include/weave/cloud.h +++ b/libweave/include/weave/cloud.h
@@ -10,6 +10,7 @@ #include <base/callback.h> #include <base/values.h> #include <weave/error.h> +#include <weave/settings.h> namespace weave { @@ -46,16 +47,14 @@ ErrorPtr* error) = 0; // Updates basic device information. - virtual bool UpdateDeviceInfo(const std::string& name, + virtual void UpdateDeviceInfo(const std::string& name, const std::string& description, - const std::string& location, - ErrorPtr* error) = 0; + const std::string& location) = 0; // Updates base device config. - virtual bool UpdateBaseConfig(const std::string& anonymous_access_role, + virtual void UpdateBaseConfig(AuthScope anonymous_access_role, bool local_discovery_enabled, - bool local_pairing_enabled, - ErrorPtr* error) = 0; + bool local_pairing_enabled) = 0; // Updates GCD service configuration. Usually for testing. virtual bool UpdateServiceConfig(const std::string& client_id,
diff --git a/libweave/include/weave/settings.h b/libweave/include/weave/settings.h index 2b38c91..c4352f0 100644 --- a/libweave/include/weave/settings.h +++ b/libweave/include/weave/settings.h
@@ -13,6 +13,14 @@ namespace weave { +// Scopes in order of increasing privileges. +enum class AuthScope { + kNone, + kViewer, + kUser, + kOwner, +}; + struct Settings { // Model specific information. Must be set by ConfigStore::LoadDefaults. std::string firmware_version; @@ -32,7 +40,7 @@ // Options mirrored into "base" state. // Maximum role for local anonymous user. - std::string local_anonymous_access_role; + AuthScope local_anonymous_access_role{AuthScope::kViewer}; // If true, allows local discovery using DNS-SD. bool local_discovery_enabled{true}; // If true, allows local pairing using Privet API.
diff --git a/libweave/src/base_api_handler.cc b/libweave/src/base_api_handler.cc index 6db8628..eb76919 100644 --- a/libweave/src/base_api_handler.cc +++ b/libweave/src/base_api_handler.cc
@@ -53,7 +53,8 @@ command->SetProgress(base::DictionaryValue{}, nullptr); const auto& settings = device_info_->GetSettings(); - std::string anonymous_access_role{settings.local_anonymous_access_role}; + std::string anonymous_access_role{ + EnumToString(settings.local_anonymous_access_role)}; bool discovery_enabled{settings.local_discovery_enabled}; bool pairing_enabled{settings.local_pairing_enabled}; @@ -62,18 +63,22 @@ parameters->GetBoolean("localDiscoveryEnabled", &discovery_enabled); parameters->GetBoolean("localPairingEnabled", &pairing_enabled); - if (!device_info_->UpdateBaseConfig(anonymous_access_role, discovery_enabled, - pairing_enabled, nullptr)) { + AuthScope auth_scope{AuthScope::kNone}; + if (!StringToEnum(anonymous_access_role, &auth_scope)) { return command->Abort(); } + device_info_->UpdateBaseConfig(auth_scope, discovery_enabled, + pairing_enabled); + command->Done(); } void BaseApiHandler::OnConfigChanged(const Settings& settings) { base::DictionaryValue state; - state.SetStringWithoutPathExpansion(kBaseStateAnonymousAccessRole, - settings.local_anonymous_access_role); + state.SetStringWithoutPathExpansion( + kBaseStateAnonymousAccessRole, + EnumToString(settings.local_anonymous_access_role)); state.SetBooleanWithoutPathExpansion(kBaseStateDiscoveryEnabled, settings.local_discovery_enabled); state.SetBooleanWithoutPathExpansion(kBaseStatePairingEnabled, @@ -94,10 +99,7 @@ parameters->GetString("description", &description); parameters->GetString("location", &location); - if (!device_info_->UpdateDeviceInfo(name, description, location, nullptr)) { - return command->Abort(); - } - + device_info_->UpdateDeviceInfo(name, description, location); command->Done(); }
diff --git a/libweave/src/base_api_handler_unittest.cc b/libweave/src/base_api_handler_unittest.cc index 5e2f68e..1dc843d 100644 --- a/libweave/src/base_api_handler_unittest.cc +++ b/libweave/src/base_api_handler_unittest.cc
@@ -119,7 +119,7 @@ 'localPairingEnabled': false } })"); - EXPECT_EQ("none", settings.local_anonymous_access_role); + EXPECT_EQ(AuthScope::kNone, settings.local_anonymous_access_role); EXPECT_FALSE(settings.local_discovery_enabled); EXPECT_FALSE(settings.local_pairing_enabled); @@ -142,7 +142,7 @@ 'localPairingEnabled': true } })"); - EXPECT_EQ("user", settings.local_anonymous_access_role); + EXPECT_EQ(AuthScope::kUser, settings.local_anonymous_access_role); EXPECT_TRUE(settings.local_discovery_enabled); EXPECT_TRUE(settings.local_pairing_enabled); expected = R"({ @@ -158,7 +158,7 @@ { Config::Transaction change{dev_reg_->GetMutableConfig()}; - change.set_local_anonymous_access_role("viewer"); + change.set_local_anonymous_access_role(AuthScope::kViewer); } expected = R"({ 'base': {
diff --git a/libweave/src/config.cc b/libweave/src/config.cc index e01e486..b0ec45d 100644 --- a/libweave/src/config.cc +++ b/libweave/src/config.cc
@@ -42,16 +42,11 @@ namespace { -bool IsValidAccessRole(const std::string& role) { - privet::AuthScope scope; - return StringToEnum(role, &scope); -} - Config::Settings CreateDefaultSettings() { Config::Settings result; result.oauth_url = "https://accounts.google.com/o/oauth2/"; result.service_url = "https://www.googleapis.com/clouddevices/v1/"; - result.local_anonymous_access_role = "viewer"; + result.local_anonymous_access_role = AuthScope::kViewer; result.pairing_modes.emplace(PairingType::kPinCode); return result; } @@ -98,8 +93,6 @@ CHECK(!settings_.model_id.empty()); CHECK(!settings_.name.empty()); - CHECK(IsValidAccessRole(settings_.local_anonymous_access_role)) - << "Invalid role: " << settings_.local_anonymous_access_role; CHECK_EQ( settings_.embedded_code.empty(), std::find(settings_.pairing_modes.begin(), settings_.pairing_modes.end(), @@ -156,8 +149,11 @@ if (dict->GetString(config_keys::kLocation, &tmp)) set_location(tmp); - if (dict->GetString(config_keys::kLocalAnonymousAccessRole, &tmp)) - set_local_anonymous_access_role(tmp); + AuthScope scope{AuthScope::kNone}; + if (dict->GetString(config_keys::kLocalAnonymousAccessRole, &tmp) && + StringToEnum(tmp, &scope)) { + set_local_anonymous_access_role(scope); + } if (dict->GetBoolean(config_keys::kLocalDiscoveryEnabled, &tmp_bool)) set_local_discovery_enabled(tmp_bool); @@ -201,7 +197,7 @@ dict.SetString(config_keys::kDescription, settings_.description); dict.SetString(config_keys::kLocation, settings_.location); dict.SetString(config_keys::kLocalAnonymousAccessRole, - settings_.local_anonymous_access_role); + EnumToString(settings_.local_anonymous_access_role)); dict.SetBoolean(config_keys::kLocalDiscoveryEnabled, settings_.local_discovery_enabled); dict.SetBoolean(config_keys::kLocalPairingEnabled, @@ -218,16 +214,6 @@ Commit(); } -bool Config::Transaction::set_local_anonymous_access_role( - const std::string& role) { - if (!IsValidAccessRole(role)) { - LOG(ERROR) << "Invalid role: " << role; - return false; - } - settings_->local_anonymous_access_role = role; - return true; -} - void Config::Transaction::Commit() { if (!config_) return;
diff --git a/libweave/src/config.h b/libweave/src/config.h index 5baff87..13e50da 100644 --- a/libweave/src/config.h +++ b/libweave/src/config.h
@@ -68,7 +68,9 @@ void set_location(const std::string& location) { settings_->location = location; } - bool set_local_anonymous_access_role(const std::string& role); + void set_local_anonymous_access_role(AuthScope role) { + settings_->local_anonymous_access_role = role; + } void set_local_discovery_enabled(bool enabled) { settings_->local_discovery_enabled = enabled; }
diff --git a/libweave/src/config_unittest.cc b/libweave/src/config_unittest.cc index 2639c22..f6f4fa0 100644 --- a/libweave/src/config_unittest.cc +++ b/libweave/src/config_unittest.cc
@@ -68,7 +68,7 @@ EXPECT_EQ("", GetSettings().name); EXPECT_EQ("", GetSettings().description); EXPECT_EQ("", GetSettings().location); - EXPECT_EQ("viewer", GetSettings().local_anonymous_access_role); + EXPECT_EQ(AuthScope::kViewer, GetSettings().local_anonymous_access_role); EXPECT_TRUE(GetSettings().local_pairing_enabled); EXPECT_TRUE(GetSettings().local_discovery_enabled); EXPECT_EQ("", GetSettings().cloud_id); @@ -121,7 +121,7 @@ EXPECT_EQ("state_name", GetSettings().name); EXPECT_EQ("state_description", GetSettings().description); EXPECT_EQ("state_location", GetSettings().location); - EXPECT_EQ("user", GetSettings().local_anonymous_access_role); + EXPECT_EQ(AuthScope::kUser, GetSettings().local_anonymous_access_role); EXPECT_FALSE(GetSettings().local_pairing_enabled); EXPECT_FALSE(GetSettings().local_discovery_enabled); EXPECT_EQ("state_cloud_id", GetSettings().cloud_id); @@ -158,14 +158,14 @@ change.set_location("set_location"); EXPECT_EQ("set_location", GetSettings().location); - change.set_local_anonymous_access_role("viewer"); - EXPECT_EQ("viewer", GetSettings().local_anonymous_access_role); + change.set_local_anonymous_access_role(AuthScope::kViewer); + EXPECT_EQ(AuthScope::kViewer, GetSettings().local_anonymous_access_role); - change.set_local_anonymous_access_role("none"); - EXPECT_EQ("none", GetSettings().local_anonymous_access_role); + change.set_local_anonymous_access_role(AuthScope::kNone); + EXPECT_EQ(AuthScope::kNone, GetSettings().local_anonymous_access_role); - change.set_local_anonymous_access_role("user"); - EXPECT_EQ("user", GetSettings().local_anonymous_access_role); + change.set_local_anonymous_access_role(AuthScope::kUser); + EXPECT_EQ(AuthScope::kUser, GetSettings().local_anonymous_access_role); change.set_local_discovery_enabled(false); EXPECT_FALSE(GetSettings().local_discovery_enabled);
diff --git a/libweave/src/device_registration_info.cc b/libweave/src/device_registration_info.cc index f1f94d0..a5799d4 100644 --- a/libweave/src/device_registration_info.cc +++ b/libweave/src/device_registration_info.cc
@@ -807,10 +807,9 @@ PublishStateUpdates(); } -bool DeviceRegistrationInfo::UpdateDeviceInfo(const std::string& name, +void DeviceRegistrationInfo::UpdateDeviceInfo(const std::string& name, const std::string& description, - const std::string& location, - ErrorPtr* error) { + const std::string& location) { Config::Transaction change{config_.get()}; change.set_name(name); change.set_description(description); @@ -821,27 +820,15 @@ UpdateDeviceResource(base::Bind(&base::DoNothing), base::Bind(&IgnoreCloudError)); } - - return true; } -bool DeviceRegistrationInfo::UpdateBaseConfig( - const std::string& anonymous_access_role, - bool local_discovery_enabled, - bool local_pairing_enabled, - ErrorPtr* error) { +void DeviceRegistrationInfo::UpdateBaseConfig(AuthScope anonymous_access_role, + bool local_discovery_enabled, + bool local_pairing_enabled) { Config::Transaction change(config_.get()); - if (!change.set_local_anonymous_access_role(anonymous_access_role)) { - Error::AddToPrintf(error, FROM_HERE, errors::kErrorDomain, - "invalid_parameter", "Invalid role: %s", - anonymous_access_role.c_str()); - return false; - } - + change.set_local_anonymous_access_role(anonymous_access_role); change.set_local_discovery_enabled(local_discovery_enabled); change.set_local_pairing_enabled(local_pairing_enabled); - - return true; } bool DeviceRegistrationInfo::UpdateServiceConfig(
diff --git a/libweave/src/device_registration_info.h b/libweave/src/device_registration_info.h index bd08ecd..77820e0 100644 --- a/libweave/src/device_registration_info.h +++ b/libweave/src/device_registration_info.h
@@ -72,14 +72,12 @@ const OnCloudRequestErrorCallback& error_callback) override; std::string RegisterDevice(const std::string& ticket_id, ErrorPtr* error) override; - bool UpdateDeviceInfo(const std::string& name, + void UpdateDeviceInfo(const std::string& name, const std::string& description, - const std::string& location, - ErrorPtr* error) override; - bool UpdateBaseConfig(const std::string& anonymous_access_role, + const std::string& location) override; + void UpdateBaseConfig(AuthScope anonymous_access_role, bool local_discovery_enabled, - bool local_pairing_enabled, - ErrorPtr* error) override; + bool local_pairing_enabled) override; bool UpdateServiceConfig(const std::string& client_id, const std::string& client_secret, const std::string& api_key,
diff --git a/libweave/src/device_registration_info_unittest.cc b/libweave/src/device_registration_info_unittest.cc index 1a84558..2a059fb 100644 --- a/libweave/src/device_registration_info_unittest.cc +++ b/libweave/src/device_registration_info_unittest.cc
@@ -140,7 +140,7 @@ settings->name = "Coffee Pot"; settings->description = "Easy to clean"; settings->location = "Kitchen"; - settings->local_anonymous_access_role = "viewer"; + settings->local_anonymous_access_role = AuthScope::kViewer; settings->model_id = "AAAAA"; settings->oauth_url = test_data::kOAuthURL; settings->service_url = test_data::kServiceURL;
diff --git a/libweave/src/privet/cloud_delegate.cc b/libweave/src/privet/cloud_delegate.cc index 1f69581..cccb6ce 100644 --- a/libweave/src/privet/cloud_delegate.cc +++ b/libweave/src/privet/cloud_delegate.cc
@@ -82,13 +82,8 @@ void UpdateDeviceInfo(const std::string& name, const std::string& description, - const std::string& location, - const SuccessCallback& success_callback, - const ErrorCallback& error_callback) override { - ErrorPtr error; - if (!device_->UpdateDeviceInfo(name, description, location, &error)) - return error_callback.Run(error.get()); - success_callback.Run(); + const std::string& location) override { + device_->UpdateDeviceInfo(name, description, location); } std::string GetOemName() const override { @@ -109,12 +104,7 @@ } AuthScope GetAnonymousMaxScope() const override { - AuthScope scope; - if (StringToEnum(device_->GetSettings().local_anonymous_access_role, - &scope)) { - return scope; - } - return AuthScope::kNone; + return device_->GetSettings().local_anonymous_access_role; } const ConnectionState& GetConnectionState() const override {
diff --git a/libweave/src/privet/cloud_delegate.h b/libweave/src/privet/cloud_delegate.h index 9069fb3..24c98fb 100644 --- a/libweave/src/privet/cloud_delegate.h +++ b/libweave/src/privet/cloud_delegate.h
@@ -66,9 +66,7 @@ // Update basic device information. virtual void UpdateDeviceInfo(const std::string& name, const std::string& description, - const std::string& location, - const SuccessCallback& success_callback, - const ErrorCallback& error_callback) = 0; + const std::string& location) = 0; // Returns the name of the maker. virtual std::string GetOemName() const = 0;
diff --git a/libweave/src/privet/mock_delegates.h b/libweave/src/privet/mock_delegates.h index eb4bd6e..1944fbb 100644 --- a/libweave/src/privet/mock_delegates.h +++ b/libweave/src/privet/mock_delegates.h
@@ -141,12 +141,10 @@ MOCK_CONST_METHOD0(GetName, std::string()); MOCK_CONST_METHOD0(GetDescription, std::string()); MOCK_CONST_METHOD0(GetLocation, std::string()); - MOCK_METHOD5(UpdateDeviceInfo, + MOCK_METHOD3(UpdateDeviceInfo, void(const std::string&, const std::string&, - const std::string&, - const SuccessCallback&, - const ErrorCallback&)); + const std::string&)); MOCK_CONST_METHOD0(GetOemName, std::string()); MOCK_CONST_METHOD0(GetModelName, std::string()); MOCK_CONST_METHOD0(GetServices, std::set<std::string>()); @@ -182,8 +180,7 @@ EXPECT_CALL(*this, GetName()).WillRepeatedly(Return("TestDevice")); EXPECT_CALL(*this, GetDescription()).WillRepeatedly(Return("")); EXPECT_CALL(*this, GetLocation()).WillRepeatedly(Return("")); - EXPECT_CALL(*this, UpdateDeviceInfo(_, _, _, _, _)) - .WillRepeatedly(RunCallback<3>()); + EXPECT_CALL(*this, UpdateDeviceInfo(_, _, _)).WillRepeatedly(Return()); EXPECT_CALL(*this, GetOemName()).WillRepeatedly(Return("Chromium")); EXPECT_CALL(*this, GetModelName()).WillRepeatedly(Return("Brillo")); EXPECT_CALL(*this, GetServices())
diff --git a/libweave/src/privet/privet_handler.cc b/libweave/src/privet/privet_handler.cc index 9790c76..4bc4822 100644 --- a/libweave/src/privet/privet_handler.cc +++ b/libweave/src/privet/privet_handler.cc
@@ -698,21 +698,9 @@ registration->GetString(kSetupStartUserKey, &user); } - cloud_->UpdateDeviceInfo(name, description, location, - base::Bind(&PrivetHandler::OnUpdateDeviceInfoDone, - weak_ptr_factory_.GetWeakPtr(), ssid, - passphrase, ticket, user, callback), - base::Bind(&OnCommandRequestFailed, callback)); -} + cloud_->UpdateDeviceInfo(name, description, location); -void PrivetHandler::OnUpdateDeviceInfoDone( - const std::string& ssid, - const std::string& passphrase, - const std::string& ticket, - const std::string& user, - const RequestCallback& callback) const { ErrorPtr error; - if (!ssid.empty() && !wifi_->ConfigureCredentials(ssid, passphrase, &error)) return ReturnError(*error, callback);
diff --git a/libweave/src/privet/privet_handler.h b/libweave/src/privet/privet_handler.h index bda4127..125c798 100644 --- a/libweave/src/privet/privet_handler.h +++ b/libweave/src/privet/privet_handler.h
@@ -28,8 +28,6 @@ class SecurityDelegate; class WifiDelegate; -enum class AuthScope; - // Privet V3 HTTP/HTTPS requests handler. // API details at https://developers.google.com/cloud-devices/ class PrivetHandler : public CloudDelegate::Observer { @@ -110,11 +108,6 @@ const UserInfo& user_info, const RequestCallback& callback); - void OnUpdateDeviceInfoDone(const std::string& ssid, - const std::string& passphrase, - const std::string& ticket, - const std::string& user, - const RequestCallback& callback) const; void ReplyWithSetupStatus(const RequestCallback& callback) const; CloudDelegate* cloud_ = nullptr;
diff --git a/libweave/src/privet/privet_handler_unittest.cc b/libweave/src/privet/privet_handler_unittest.cc index 853648c..7f79934 100644 --- a/libweave/src/privet/privet_handler_unittest.cc +++ b/libweave/src/privet/privet_handler_unittest.cc
@@ -525,8 +525,8 @@ } TEST_F(PrivetHandlerSetupTest, SetupNameDescriptionLocation) { - EXPECT_CALL(cloud_, UpdateDeviceInfo("testName", "testDescription", - "testLocation", _, _)) + EXPECT_CALL(cloud_, + UpdateDeviceInfo("testName", "testDescription", "testLocation")) .Times(1); const char kInput[] = R"({ 'name': 'testName',
diff --git a/libweave/src/privet/privet_types.cc b/libweave/src/privet/privet_types.cc index b5b7d78..3cf0ace 100644 --- a/libweave/src/privet/privet_types.cc +++ b/libweave/src/privet/privet_types.cc
@@ -15,7 +15,6 @@ namespace { -using privet::AuthScope; using privet::ConnectionState; using privet::CryptoType; using privet::SetupState;
diff --git a/libweave/src/privet/privet_types.h b/libweave/src/privet/privet_types.h index d013500..f9ea8b7 100644 --- a/libweave/src/privet/privet_types.h +++ b/libweave/src/privet/privet_types.h
@@ -9,6 +9,7 @@ #include <base/logging.h> #include <weave/error.h> +#include <weave/settings.h> namespace weave { namespace privet { @@ -24,14 +25,6 @@ kWifi50, }; -// Scopes in order of increasing privileges. -enum class AuthScope { - kNone, - kViewer, - kUser, - kOwner, -}; - class UserInfo { public: explicit UserInfo(AuthScope scope = AuthScope::kNone, uint64_t user_id = 0)