buffet: Update RegistrationStatus enum and how it's set
Motivation is to simplify states and make them more consistent with Privet
state.
Also update buffet code that switches states to better match cloud connection
state.
CQ-DEPEND=CL:260680
BUG=brillo:572,brillo:571,brillo:592
TEST=manual, register device, restart buffet and privetd, reboot router
checking if gcd.status of privet/info is correct.
Change-Id: I74c36d57a2396660417bf6db8166f90263d6636d
Reviewed-on: https://chromium-review.googlesource.com/260302
Tested-by: Vitaly Buka <vitalybuka@chromium.org>
Reviewed-by: Christopher Wiley <wiley@chromium.org>
Commit-Queue: Vitaly Buka <vitalybuka@chromium.org>
diff --git a/buffet/dbus_bindings/org.chromium.Buffet.Manager.xml b/buffet/dbus_bindings/org.chromium.Buffet.Manager.xml
index ff73108..2e23802 100644
--- a/buffet/dbus_bindings/org.chromium.Buffet.Manager.xml
+++ b/buffet/dbus_bindings/org.chromium.Buffet.Manager.xml
@@ -43,20 +43,17 @@
<tp:docstring>
State of Buffet's cloud registration.
Possible values include:
+ "unconfigured": Buffet has no credentials, either from an out of box
+ state, or because device was unregistered.
- "offline": Buffet has credentials, but no connectivity to the Internet.
- "cloud_error": Buffet has credentials, but cannot contact cloud services
- to verify their validity. This could indicate that cloud
- services are down, or that DNS is not resolving.
- "invalid_credentials": Buffet has credentials, but they are no longer
- valid.
- "unregistered": Buffet has no credentials, either from an out of
- box state, or because those credentials have been
- rejected by the cloud service. Note that we
- can unregistered with or without Internet connectivity.
- "registering": Buffet has been provided with credentials, and is
- registering with the cloud.
- "registered": Buffet is online and registered with cloud services.
+ "connecting": Buffet is registered and attempting to connect to the
+ cloud.
+
+ "connected": Buffet is online and connected to the cloud. Note that
+ only this state requires internet connectivity.
+
+ "invalid_credentials": Buffet has credentials, but they are no longer
+ valid.
</tp:docstring>
</property>
</interface>
diff --git a/buffet/device_registration_info.cc b/buffet/device_registration_info.cc
index 78b72a5..3b9fdc9 100644
--- a/buffet/device_registration_info.cc
+++ b/buffet/device_registration_info.cc
@@ -176,6 +176,7 @@
}
bool DeviceRegistrationInfo::Load() {
+ SetRegistrationStatus(RegistrationStatus::kUnconfigured);
auto value = storage_->Load();
const base::DictionaryValue* dict = nullptr;
if (!value || !value->GetAsDictionary(&dict))
@@ -243,7 +244,6 @@
location_ = location;
if (HaveRegistrationCredentials(nullptr)) {
- SetRegistrationStatus(RegistrationStatus::kOffline);
// Wait a significant amount of time for local daemons to publish their
// state to Buffet before publishing it to the cloud.
// TODO(wiley) We could do a lot of things here to either expose this
@@ -277,6 +277,7 @@
}
void DeviceRegistrationInfo::ScheduleStartDevice(const base::TimeDelta& later) {
+ SetRegistrationStatus(RegistrationStatus::kConnecting);
base::MessageLoop* current = base::MessageLoop::current();
if (!current)
return; // Assume we're in unittests
@@ -414,17 +415,18 @@
LOG(WARNING) << "Failed to watch XMPP file descriptor";
return;
}
+
xmpp_client_->StartStream();
}
void DeviceRegistrationInfo::OnFileCanReadWithoutBlocking(int fd) {
- if (xmpp_client_ && xmpp_client_->GetFileDescriptor() == fd) {
- if (!xmpp_client_->Read()) {
- // Authentication failed or the socket was closed.
- if (!fd_watcher_.StopWatchingFileDescriptor()) {
- LOG(WARNING) << "Failed to stop the watcher";
- }
- }
+ if (!xmpp_client_ || xmpp_client_->GetFileDescriptor() != fd)
+ return;
+ if (!xmpp_client_->Read()) {
+ // Authentication failed or the socket was closed.
+ if (!fd_watcher_.StopWatchingFileDescriptor())
+ LOG(WARNING) << "Failed to stop the watcher";
+ return;
}
}
@@ -587,7 +589,6 @@
// We're going to respond with our success immediately and we'll StartDevice
// shortly after.
ScheduleStartDevice(base::TimeDelta::FromSeconds(0));
- SetRegistrationStatus(RegistrationStatus::kRegistered);
return device_id_;
}
@@ -693,8 +694,12 @@
chromeos::mime::parameters::kCharset,
"utf-8")};
- auto request_cb =
- [success_callback, error_callback](int request_id, ResponsePtr response) {
+ auto status_cb = base::Bind(&DeviceRegistrationInfo::SetRegistrationStatus,
+ weak_factory_.GetWeakPtr());
+
+ auto request_cb = [success_callback, error_callback, status_cb](
+ int request_id, ResponsePtr response) {
+ status_cb.Run(RegistrationStatus::kConnected);
chromeos::ErrorPtr error;
std::unique_ptr<base::DictionaryValue> json_resp{
@@ -713,30 +718,29 @@
};
auto transport = transport_;
- auto error_callackback_with_reauthorization =
- base::Bind([method, url, data, mime_type, transport, request_cb, error_cb]
- (DeviceRegistrationInfo* self,
- int request_id,
- const chromeos::Error* error) {
- if (error->HasError(chromeos::errors::http::kDomain,
- std::to_string(chromeos::http::status_code::Denied))) {
- chromeos::ErrorPtr reauthorization_error;
- // Forcibly refresh the access token.
- if (!self->RefreshAccessToken(&reauthorization_error)) {
- // TODO(antonm): Check if the device has been actually removed.
- error_cb(request_id, reauthorization_error.get());
- return;
- }
- SendRequestWithRetries(method, url,
- data, mime_type,
- {self->GetAuthorizationHeader()},
- transport,
- 7,
- base::Bind(request_cb), base::Bind(error_cb));
- } else {
- error_cb(request_id, error);
- }
- }, base::Unretained(this));
+ auto error_callackback_with_reauthorization = base::Bind(
+ [method, url, data, mime_type, transport, request_cb, error_cb,
+ status_cb](DeviceRegistrationInfo* self, int request_id,
+ const chromeos::Error* error) {
+ status_cb.Run(RegistrationStatus::kConnecting);
+ if (error->HasError(
+ chromeos::errors::http::kDomain,
+ std::to_string(chromeos::http::status_code::Denied))) {
+ chromeos::ErrorPtr reauthorization_error;
+ // Forcibly refresh the access token.
+ if (!self->RefreshAccessToken(&reauthorization_error)) {
+ // TODO(antonm): Check if the device has been actually removed.
+ error_cb(request_id, reauthorization_error.get());
+ return;
+ }
+ SendRequestWithRetries(method, url, data, mime_type,
+ {self->GetAuthorizationHeader()}, transport, 7,
+ base::Bind(request_cb), base::Bind(error_cb));
+ } else {
+ error_cb(request_id, error);
+ }
+ },
+ base::Unretained(this));
SendRequestWithRetries(method, url,
data, mime_type,
@@ -888,10 +892,6 @@
base::Bind(&DeviceRegistrationInfo::PublishStateUpdates,
base::Unretained(this)),
base::TimeDelta::FromSeconds(7));
- // TODO(wiley): This is the very bare minimum of state to report to local
- // services interested in our GCD state. Build a more
- // robust model of our state with respect to the server.
- SetRegistrationStatus(RegistrationStatus::kRegistered);
}
void DeviceRegistrationInfo::PublishCommands(const base::ListValue& commands) {
diff --git a/buffet/device_registration_info.h b/buffet/device_registration_info.h
index e8f02eb..648bcd6 100644
--- a/buffet/device_registration_info.h
+++ b/buffet/device_registration_info.h
@@ -254,7 +254,7 @@
std::unique_ptr<chromeos::KeyValueStore> config_store_;
// Tracks our current registration status.
- RegistrationStatus registration_status_{RegistrationStatus::kUnregistered};
+ RegistrationStatus registration_status_{RegistrationStatus::kUnconfigured};
StatusHandler registration_status_handler_;
friend class TestHelper;
diff --git a/buffet/device_registration_info_unittest.cc b/buffet/device_registration_info_unittest.cc
index 2912fb6..7909713 100644
--- a/buffet/device_registration_info_unittest.cc
+++ b/buffet/device_registration_info_unittest.cc
@@ -319,8 +319,7 @@
SetDefaultDeviceRegistration(&data_);
storage_->Save(&data_);
EXPECT_TRUE(dev_reg_->Load());
- EXPECT_EQ(RegistrationStatus::kOffline,
- dev_reg_->GetRegistrationStatus());
+ EXPECT_EQ(RegistrationStatus::kConnecting, dev_reg_->GetRegistrationStatus());
transport_->AddHandler(dev_reg_->GetOAuthURL("token"),
chromeos::http::request_type::kPost,
@@ -331,16 +330,14 @@
EXPECT_EQ(1, transport_->GetRequestCount());
EXPECT_TRUE(error->HasError(buffet::kErrorDomainOAuth2,
"unable_to_authenticate"));
- EXPECT_EQ(RegistrationStatus::kOffline,
- dev_reg_->GetRegistrationStatus());
+ EXPECT_EQ(RegistrationStatus::kConnecting, dev_reg_->GetRegistrationStatus());
}
TEST_F(DeviceRegistrationInfoTest, CheckDeregistration) {
SetDefaultDeviceRegistration(&data_);
storage_->Save(&data_);
EXPECT_TRUE(dev_reg_->Load());
- EXPECT_EQ(RegistrationStatus::kOffline,
- dev_reg_->GetRegistrationStatus());
+ EXPECT_EQ(RegistrationStatus::kConnecting, dev_reg_->GetRegistrationStatus());
transport_->AddHandler(dev_reg_->GetOAuthURL("token"),
chromeos::http::request_type::kPost,
@@ -493,7 +490,7 @@
EXPECT_EQ(1,
storage_->save_count()); // The device info must have been saved.
EXPECT_EQ(3, transport_->GetRequestCount());
- EXPECT_EQ(RegistrationStatus::kRegistered, dev_reg_->GetRegistrationStatus());
+ EXPECT_EQ(RegistrationStatus::kConnecting, dev_reg_->GetRegistrationStatus());
// Validate the device info saved to storage...
auto storage_data = storage_->Load();
@@ -522,13 +519,13 @@
// After we've been initialized, we should be either offline or unregistered,
// depending on whether or not we've found credentials.
EXPECT_TRUE(dev_reg_->Load());
- EXPECT_EQ(RegistrationStatus::kUnregistered,
+ EXPECT_EQ(RegistrationStatus::kUnconfigured,
dev_reg_->GetRegistrationStatus());
// Put some credentials into our state, make sure we call that offline.
SetDefaultDeviceRegistration(&data_);
storage_->Save(&data_);
EXPECT_TRUE(dev_reg_->Load());
- EXPECT_EQ(RegistrationStatus::kOffline, dev_reg_->GetRegistrationStatus());
+ EXPECT_EQ(RegistrationStatus::kConnecting, dev_reg_->GetRegistrationStatus());
}
} // namespace buffet
diff --git a/buffet/manager.cc b/buffet/manager.cc
index d02c96b..7a4a9ca 100644
--- a/buffet/manager.cc
+++ b/buffet/manager.cc
@@ -71,8 +71,9 @@
std::move(state_store),
base::Bind(&Manager::OnRegistrationStatusChange,
base::Unretained(this))));
- device_info_->Load();
+ // Reset D-Bus properties.
OnRegistrationStatusChange(device_info_->GetRegistrationStatus());
+ device_info_->Load();
dbus_adaptor_.RegisterWithDBusObject(&dbus_object_);
dbus_object_.RegisterAsync(cb);
}
diff --git a/buffet/registration_status.cc b/buffet/registration_status.cc
index 3b44ae3..653715c 100644
--- a/buffet/registration_status.cc
+++ b/buffet/registration_status.cc
@@ -4,23 +4,22 @@
#include "buffet/registration_status.h"
+#include <base/logging.h>
+
namespace buffet {
std::string StatusToString(RegistrationStatus status) {
switch (status) {
- case RegistrationStatus::kOffline:
- return "offline";
- case RegistrationStatus::kCloudError:
- return "cloud_error";
- case RegistrationStatus::kUnregistered:
- return "unregistered";
- case RegistrationStatus::kRegistering:
- return "registering";
- case RegistrationStatus::kRegistered:
- return "registered";
+ case RegistrationStatus::kUnconfigured:
+ return "unconfigured";
+ case RegistrationStatus::kConnecting:
+ return "connecting";
+ case RegistrationStatus::kConnected:
+ return "connected";
case RegistrationStatus::kInvalidCredentials:
return "invalid_credentials";
}
+ CHECK(0) << "Unknown status";
return "unknown";
}
diff --git a/buffet/registration_status.h b/buffet/registration_status.h
index eee0c81..6b75f4d 100644
--- a/buffet/registration_status.h
+++ b/buffet/registration_status.h
@@ -11,11 +11,9 @@
// See the DBus interface XML file for complete descriptions of these states.
enum class RegistrationStatus {
- kOffline, // We have credentials but are offline.
- kCloudError, // We're online, but can't talk to cloud services.
- kUnregistered, // We have no credentials.
- kRegistering, // We've just been given credentials.
- kRegistered, // We're registered and online.
+ kUnconfigured, // We have no credentials.
+ kConnecting, // We have credentials but not yet connected.
+ kConnected, // We're registered and connected to the cloud.
kInvalidCredentials, // Our registration has been revoked.
};
