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)
