commit 4547afa5c3a4cf638e4c0c777648113af433b731
author Vitaly Buka <vitalybuka@chromium.org> Tue Jun 09 09:46:53 2015 -0700
committer ChromeOS Commit Bot <chromeos-commit-bot@chromium.org> Wed Jun 10 05:03:55 2015 +0000
tree 2ce421e533410ee3d9e19db1011e9c4ea2db126c
parent df7dcda28887803d69be5ea6bd6c662a47cabec0

buffet: Move command minimal role checking into CommandManager

Code uses information available inside CommandManager.

BUG=brillo:1161
TEST=`FEATURES=test emerge-gizmo buffet`

Change-Id: I21bb7f6ca3923b5375c97eea4a05f942893f3ab1
Reviewed-on: https://chromium-review.googlesource.com/276361
Reviewed-by: Alex Vakulenko <avakulenko@chromium.org>
Reviewed-by: Vitaly Buka <vitalybuka@chromium.org>
Tested-by: Vitaly Buka <vitalybuka@chromium.org>
Commit-Queue: Vitaly Buka <vitalybuka@chromium.org>

buffet/commands/command_manager.cc

diff --git a/buffet/commands/command_manager.cc b/buffet/commands/command_manager.cc
index d0743de..71ea5a9 100644
--- a/buffet/commands/command_manager.cc
+++ b/buffet/commands/command_manager.cc
@@ -99,6 +99,32 @@
   command_queue_.Add(std::move(command_instance));
 }
 
+bool CommandManager::AddCommand(const base::DictionaryValue& command,
+                                UserRole role,
+                                std::string* id,
+                                chromeos::ErrorPtr* error) {
+  auto command_instance = buffet::CommandInstance::FromJson(
+      &command, commands::attributes::kCommand_Visibility_Local,
+      GetCommandDictionary(), nullptr, error);
+  if (!command_instance)
+    return false;
+
+  UserRole minimal_role =
+      command_instance->GetCommandDefinition()->GetMinimalRole();
+  if (role < minimal_role) {
+    chromeos::Error::AddToPrintf(
+        error, FROM_HERE, errors::commands::kDomain, "access_denied",
+        "User role '%s' less than minimal: '%s'", ToString(role).c_str(),
+        ToString(minimal_role).c_str());
+    return false;
+  }
+
+  *id = std::to_string(++next_command_id_);
+  command_instance->SetID(*id);
+  AddCommand(std::move(command_instance));
+  return true;
+}
+
 CommandInstance* CommandManager::FindCommand(const std::string& id) const {
   return command_queue_.Find(id);
 }

buffet/commands/command_manager.h

diff --git a/buffet/commands/command_manager.h b/buffet/commands/command_manager.h
index 9a891ba..5a1cc5e 100644
--- a/buffet/commands/command_manager.h
+++ b/buffet/commands/command_manager.h
@@ -87,6 +87,10 @@
 
   // Adds a new command to the command queue.
   void AddCommand(std::unique_ptr<CommandInstance> command_instance);
+  bool AddCommand(const base::DictionaryValue& command,
+                  UserRole role,
+                  std::string* id,
+                  chromeos::ErrorPtr* error);
 
   // Finds a command by the command |id|. Returns nullptr if the command with
   // the given |id| is not found. The returned pointer should not be persisted
@@ -110,6 +114,7 @@
   DBusCommandDispacher command_dispatcher_;
   CommandQueue command_queue_;
   std::vector<base::Callback<void()>> on_command_changed_;
+  uint32_t next_command_id_{0};
 
   DISALLOW_COPY_AND_ASSIGN(CommandManager);
 };

buffet/manager.cc

diff --git a/buffet/manager.cc b/buffet/manager.cc
index 40ef9c0..6ca276d 100644
--- a/buffet/manager.cc
+++ b/buffet/manager.cc
@@ -164,45 +164,25 @@
 void Manager::AddCommand(DBusMethodResponse<std::string> response,
                          const std::string& json_command,
                          const std::string& in_user_role) {
-  static int next_id = 0;
   std::string error_message;
   std::unique_ptr<base::Value> value(base::JSONReader::ReadAndReturnError(
       json_command, base::JSON_PARSE_RFC, nullptr, &error_message));
-  if (!value) {
-    response->ReplyWithError(FROM_HERE, chromeos::errors::json::kDomain,
-                             chromeos::errors::json::kParseError,
-                             error_message);
-    return;
+  const base::DictionaryValue* command{nullptr};
+  if (!value || !value->GetAsDictionary(&command)) {
+    return response->ReplyWithError(FROM_HERE, chromeos::errors::json::kDomain,
+                                    chromeos::errors::json::kParseError,
+                                    error_message);
   }
+
   chromeos::ErrorPtr error;
-  auto command_instance = buffet::CommandInstance::FromJson(
-      value.get(), commands::attributes::kCommand_Visibility_Local,
-      command_manager_->GetCommandDictionary(), nullptr, &error);
-  if (!command_instance) {
-    response->ReplyWithError(error.get());
-    return;
-  }
-
   UserRole role;
-  if (!FromString(in_user_role, &role, &error)) {
-    response->ReplyWithError(error.get());
-    return;
-  }
+  if (!FromString(in_user_role, &role, &error))
+    return response->ReplyWithError(error.get());
 
-  UserRole minimal_role =
-      command_instance->GetCommandDefinition()->GetMinimalRole();
-  if (role < minimal_role) {
-    chromeos::Error::AddToPrintf(
-        &error, FROM_HERE, kErrorDomainGCD, "access_denied",
-        "User role '%s' less than minimal: '%s'", ToString(role).c_str(),
-        ToString(minimal_role).c_str());
-    response->ReplyWithError(error.get());
-    return;
-  }
+  std::string id;
+  if (!command_manager_->AddCommand(*command, role, &id, &error))
+    return response->ReplyWithError(error.get());
 
-  std::string id = std::to_string(++next_id);
-  command_instance->SetID(id);
-  command_manager_->AddCommand(std::move(command_instance));
   response->Return(id);
 }