buffet: Merge registration start and finalization. As currently user-initiated registration flow is recommended, it allows us to simplify registration protocol to a single method. BUG=None TEST=cros_workon_make buffet --test && manual testing. Change-Id: I1937b91f4f7b34585ce5b8cb1a4c26ec3a849865 Reviewed-on: https://chromium-review.googlesource.com/223700 Commit-Queue: Anton Muhin <antonm@chromium.org> Tested-by: Anton Muhin <antonm@chromium.org> Reviewed-by: Anton Muhin <antonm@chromium.org>
diff --git a/buffet/buffet_client.cc b/buffet/buffet_client.cc index f8ef56e..e32082d 100644 --- a/buffet/buffet_client.cc +++ b/buffet/buffet_client.cc
@@ -41,9 +41,8 @@ std::cerr << " " << kManagerStartDevice << std::endl; std::cerr << " " << kManagerCheckDeviceRegistered << std::endl; std::cerr << " " << kManagerGetDeviceInfo << std::endl; - std::cerr << " " << kManagerStartRegisterDevice + std::cerr << " " << kManagerRegisterDevice << " param1 = val1¶m2 = val2..." << std::endl; - std::cerr << " " << kManagerFinishRegisterDevice << std::endl; std::cerr << " " << kManagerAddCommand << " '{\"name\":\"command_name\",\"parameters\":{}}'" << std::endl; @@ -160,10 +159,10 @@ return EX_OK; } - int CallManagerStartRegisterDevice(const CommandLine::StringVector& args) { + int CallManagerRegisterDevice(const CommandLine::StringVector& args) { if (args.size() > 1) { std::cerr << "Invalid number of arguments for " - << "Manager." << kManagerStartRegisterDevice << std::endl; + << "Manager." << kManagerRegisterDevice << std::endl; usage(); return EX_USAGE; } @@ -181,35 +180,8 @@ auto response = CallMethodAndBlockWithTimeout( timeout_ms, manager_proxy_, - kManagerInterface, kManagerStartRegisterDevice, &error, + kManagerInterface, kManagerRegisterDevice, &error, params); - std::string info; - if (!response || - !ExtractMethodCallResults(response.get(), &error, &info)) { - std::cout << "Failed to receive a response:" - << error->GetMessage() << std::endl; - return EX_UNAVAILABLE; - } - - std::cout << "Registration started: " << info << std::endl; - return EX_OK; - } - - int CallManagerFinishRegisterDevice(const CommandLine::StringVector& args) { - if (args.size() > 0) { - std::cerr << "Invalid number of arguments for " - << "Manager." << kManagerFinishRegisterDevice << std::endl; - usage(); - return EX_USAGE; - } - - ErrorPtr error; - std::string user_auth_code; - static const int timeout_ms = 10000; - auto response = CallMethodAndBlockWithTimeout( - timeout_ms, - manager_proxy_, - kManagerInterface, kManagerFinishRegisterDevice, &error); std::string device_id; if (!response || !ExtractMethodCallResults(response.get(), &error, &device_id)) { @@ -218,9 +190,7 @@ return EX_UNAVAILABLE; } - std::cout << "Device ID is " - << (device_id.empty() ? std::string("<unregistered>") : device_id) - << std::endl; + std::cout << "Device registered: " << device_id << std::endl; return EX_OK; } @@ -328,12 +298,9 @@ } else if (command.compare(kManagerGetDeviceInfo) == 0 || command.compare("di") == 0) { err = helper.CallManagerGetDeviceInfo(args); - } else if (command.compare(kManagerStartRegisterDevice) == 0 || - command.compare("sr") == 0) { - err = helper.CallManagerStartRegisterDevice(args); - } else if (command.compare(kManagerFinishRegisterDevice) == 0 || - command.compare("fr") == 0) { - err = helper.CallManagerFinishRegisterDevice(args); + } else if (command.compare(kManagerRegisterDevice) == 0 || + command.compare("rd") == 0) { + err = helper.CallManagerRegisterDevice(args); } else if (command.compare(kManagerUpdateStateMethod) == 0 || command.compare("us") == 0) { err = helper.CallManagerUpdateState(args);
diff --git a/buffet/device_registration_info.cc b/buffet/device_registration_info.cc index 2c04dcf..c67dce0 100644 --- a/buffet/device_registration_info.cc +++ b/buffet/device_registration_info.cc
@@ -369,7 +369,7 @@ return false; } -std::string DeviceRegistrationInfo::StartRegistration( +std::string DeviceRegistrationInfo::RegisterDevice( const std::map<std::string, std::string>& params, chromeos::ErrorPtr* error) { GetParamValue(params, "ticket_id", &ticket_id_); @@ -382,7 +382,6 @@ GetParamValue(params, storage_keys::kOAuthURL, &oauth_url_); GetParamValue(params, storage_keys::kServiceURL, &service_url_); - std::unique_ptr<base::DictionaryValue> device_draft = BuildDeviceResource(error); if (!device_draft) @@ -411,37 +410,17 @@ {"client_id", client_id_} }); - base::DictionaryValue json; - json.SetString("ticket_id", ticket_id_); - json.SetString("auth_url", auth_url); - - std::string ret; - base::JSONWriter::Write(&json, &ret); - return ret; -} - -bool DeviceRegistrationInfo::FinishRegistration(chromeos::ErrorPtr* error) { - if (ticket_id_.empty()) { - LOG(ERROR) << "Finish registration without ticket ID"; - chromeos::Error::AddTo(error, kErrorDomainBuffet, - "registration_not_started", - "Device registration not started"); - return false; - } - - std::string url = GetServiceURL("registrationTickets/" + ticket_id_ + - "/finalize?key=" + api_key_); - std::unique_ptr<chromeos::http::Response> response = - chromeos::http::PostBinary(url, nullptr, 0, transport_, error); + url = GetServiceURL("registrationTickets/" + ticket_id_ + + "/finalize?key=" + api_key_); + response = chromeos::http::PostBinary(url, nullptr, 0, transport_, error); if (!response) - return false; - auto json_resp = chromeos::http::ParseJsonResponse(response.get(), nullptr, - error); + return std::string(); + json_resp = chromeos::http::ParseJsonResponse(response.get(), nullptr, error); if (!json_resp) - return false; + return std::string(); if (!response->IsSuccessful()) { ParseGCDError(json_resp.get(), error); - return false; + return std::string(); } std::string auth_code; @@ -450,7 +429,7 @@ !json_resp->GetString("deviceDraft.id", &device_id_)) { chromeos::Error::AddTo(error, kErrorDomainGCD, "unexpected_response", "Device account missing in response"); - return false; + return std::string(); } // Now get access_token and refresh_token @@ -463,7 +442,7 @@ {"grant_type", "authorization_code"} }, transport_, error); if (!response) - return false; + return std::string(); json_resp = ParseOAuthResponse(response.get(), error); int expires_in = 0; @@ -476,14 +455,14 @@ expires_in <= 0) { chromeos::Error::AddTo(error, kErrorDomainGCD, "unexpected_response", "Device access_token missing in response"); - return false; + return std::string(); } access_token_expiration_ = base::Time::Now() + base::TimeDelta::FromSeconds(expires_in); Save(); - return true; + return device_id_; } void DeviceRegistrationInfo::DoCloudRequest(
diff --git a/buffet/device_registration_info.h b/buffet/device_registration_info.h index 8292ea6..071df46 100644 --- a/buffet/device_registration_info.h +++ b/buffet/device_registration_info.h
@@ -95,19 +95,17 @@ // the device is not registered or communication failure. std::unique_ptr<base::Value> GetDeviceInfo(chromeos::ErrorPtr* error); - // Starts device registration procedure. |params| are a list of - // key-value pairs of device information, such as client_id, client_secret, - // and so on. If a particular key-value pair is omitted, a default value - // is used when possible. Returns a device registration ticket ID on success. + // Registers the device. + // + // |params| are a list of key-value pairs of device information, + // such as client_id, client_secret, and so on. If a particular key-value pair + // is omitted, a default value is used when possible. + // Returns a device ID on success. // The values are all strings for now. - std::string StartRegistration( + std::string RegisterDevice( const std::map<std::string, std::string>& params, chromeos::ErrorPtr* error); - // Finalizes the device registration. - // StartRegistration must have been invoked before. - bool FinishRegistration(chromeos::ErrorPtr* error); - // Start device execution. // Device will do required start up chores and then start to listen // to new commands.
diff --git a/buffet/device_registration_info_unittest.cc b/buffet/device_registration_info_unittest.cc index e33630f..097039a 100644 --- a/buffet/device_registration_info_unittest.cc +++ b/buffet/device_registration_info_unittest.cc
@@ -270,7 +270,7 @@ EXPECT_EQ(test_data::kDeviceId, id); } -TEST_F(DeviceRegistrationInfoTest, StartRegistration) { +TEST_F(DeviceRegistrationInfoTest, RegisterDevice) { EXPECT_TRUE(dev_reg_->Load()); auto update_ticket = [](const ServerRequest& request, @@ -339,24 +339,6 @@ std::string("registrationTickets/") + test_data::kClaimTicketId), chromeos::http::request_type::kPatch, base::Bind(update_ticket)); - std::map<std::string, std::string> params; - params["ticket_id"] = test_data::kClaimTicketId; - std::string json_resp = dev_reg_->StartRegistration(params, nullptr); - auto json = std::unique_ptr<base::Value>(base::JSONReader::Read(json_resp)); - EXPECT_NE(nullptr, json.get()); - base::DictionaryValue* dict = nullptr; - EXPECT_TRUE(json->GetAsDictionary(&dict)); - std::string value; - EXPECT_TRUE(dict->GetString("ticket_id", &value)); - EXPECT_EQ(test_data::kClaimTicketId, value); -} - -TEST_F(DeviceRegistrationInfoTest, FinishRegistration_NoAuth) { - // Test finalizing ticket with no user authorization token. - // This assumes that a client would patch in their email separately. - EXPECT_TRUE(dev_reg_->Load()); - - // General ticket finalization handler. std::string ticket_url = dev_reg_->GetServiceURL("registrationTickets/" + std::string(test_data::kClaimTicketId)); @@ -367,13 +349,17 @@ transport_->AddHandler(dev_reg_->GetOAuthURL("token"), chromeos::http::request_type::kPost, base::Bind(OAuth2Handler)); - storage_->reset_save_count(); DeviceRegistrationInfo::TestHelper::SetTestTicketId(dev_reg_.get()); - EXPECT_TRUE(dev_reg_->FinishRegistration(nullptr)); + + std::map<std::string, std::string> params; + params["ticket_id"] = test_data::kClaimTicketId; + std::string device_id = dev_reg_->RegisterDevice(params, nullptr); + + EXPECT_EQ(test_data::kDeviceId, device_id); EXPECT_EQ(1, storage_->save_count()); // The device info must have been saved. - EXPECT_EQ(2, transport_->GetRequestCount()); + EXPECT_EQ(3, transport_->GetRequestCount()); // Validate the device info saved to storage... auto storage_data = storage_->Load();
diff --git a/buffet/libbuffet/dbus_constants.cc b/buffet/libbuffet/dbus_constants.cc index fba4c79..16cdb36 100644 --- a/buffet/libbuffet/dbus_constants.cc +++ b/buffet/libbuffet/dbus_constants.cc
@@ -18,8 +18,7 @@ const char kManagerStartDevice[] = "StartDevice"; const char kManagerCheckDeviceRegistered[] = "CheckDeviceRegistered"; const char kManagerGetDeviceInfo[] = "GetDeviceInfo"; -const char kManagerStartRegisterDevice[] = "StartRegisterDevice"; -const char kManagerFinishRegisterDevice[] = "FinishRegisterDevice"; +const char kManagerRegisterDevice[] = "RegisterDevice"; const char kManagerUpdateStateMethod[] = "UpdateState"; const char kManagerAddCommand[] = "AddCommand"; const char kManagerTestMethod[] = "TestMethod";
diff --git a/buffet/libbuffet/dbus_constants.h b/buffet/libbuffet/dbus_constants.h index 0d0c63b..e0dd22a 100644 --- a/buffet/libbuffet/dbus_constants.h +++ b/buffet/libbuffet/dbus_constants.h
@@ -25,8 +25,7 @@ LIBBUFFET_EXPORT extern const char kManagerStartDevice[]; LIBBUFFET_EXPORT extern const char kManagerCheckDeviceRegistered[]; LIBBUFFET_EXPORT extern const char kManagerGetDeviceInfo[]; -LIBBUFFET_EXPORT extern const char kManagerStartRegisterDevice[]; -LIBBUFFET_EXPORT extern const char kManagerFinishRegisterDevice[]; +LIBBUFFET_EXPORT extern const char kManagerRegisterDevice[]; LIBBUFFET_EXPORT extern const char kManagerUpdateStateMethod[]; LIBBUFFET_EXPORT extern const char kManagerAddCommand[]; LIBBUFFET_EXPORT extern const char kManagerTestMethod[];
diff --git a/buffet/manager.cc b/buffet/manager.cc index af1632b..4648c83 100644 --- a/buffet/manager.cc +++ b/buffet/manager.cc
@@ -45,12 +45,9 @@ itf->AddMethodHandler(dbus_constants::kManagerGetDeviceInfo, base::Unretained(this), &Manager::HandleGetDeviceInfo); - itf->AddMethodHandler(dbus_constants::kManagerStartRegisterDevice, + itf->AddMethodHandler(dbus_constants::kManagerRegisterDevice, base::Unretained(this), - &Manager::HandleStartRegisterDevice); - itf->AddMethodHandler(dbus_constants::kManagerFinishRegisterDevice, - base::Unretained(this), - &Manager::HandleFinishRegisterDevice); + &Manager::HandleRegisterDevice); itf->AddMethodHandler(dbus_constants::kManagerUpdateStateMethod, base::Unretained(this), &Manager::HandleUpdateState); @@ -108,20 +105,12 @@ return device_info_str; } -std::string Manager::HandleStartRegisterDevice( +std::string Manager::HandleRegisterDevice( chromeos::ErrorPtr* error, const std::map<std::string, std::string>& params) { - LOG(INFO) << "Received call to Manager.StartRegisterDevice()"; + LOG(INFO) << "Received call to Manager.RegisterDevice()"; - return device_info_->StartRegistration(params, error); -} - -std::string Manager::HandleFinishRegisterDevice(chromeos::ErrorPtr* error) { - LOG(INFO) << "Received call to Manager.FinishRegisterDevice()"; - if (!device_info_->FinishRegistration(error)) - return std::string(); - - return device_info_->GetDeviceId(error); + return device_info_->RegisterDevice(params, error); } void Manager::HandleUpdateState(
diff --git a/buffet/manager.h b/buffet/manager.h index 8d7ab13..4fcf445 100644 --- a/buffet/manager.h +++ b/buffet/manager.h
@@ -49,12 +49,10 @@ std::string HandleCheckDeviceRegistered(chromeos::ErrorPtr* error); // Handles calls to org.chromium.Buffet.Manager.GetDeviceInfo(). std::string HandleGetDeviceInfo(chromeos::ErrorPtr* error); - // Handles calls to org.chromium.Buffet.Manager.StartRegisterDevice(). - std::string HandleStartRegisterDevice(chromeos::ErrorPtr* error, - const std::map<std::string, - std::string>& params); - // Handles calls to org.chromium.Buffet.Manager.FinishRegisterDevice(). - std::string HandleFinishRegisterDevice(chromeos::ErrorPtr* error); + // Handles calls to org.chromium.Buffet.Manager.RegisterDevice(). + std::string HandleRegisterDevice( + chromeos::ErrorPtr* error, + const std::map<std::string, std::string>& params); // Handles calls to org.chromium.Buffet.Manager.UpdateState(). void HandleUpdateState(chromeos::ErrorPtr* error, const chromeos::VariantDictionary& property_set);