PlatformKeysService: Process state accessing operations sequentially.

chrome/browser/chromeos/platform_keys/platform_keys_service.cc

Index: chrome/browser/chromeos/platform_keys/platform_keys_service.cc
diff --git a/chrome/browser/chromeos/platform_keys/platform_keys_service.cc b/chrome/browser/chromeos/platform_keys/platform_keys_service.cc
index 4318043187267268698dff8eec92c9c01696d0d8..caf5de2ac750f496ba4deef12be1636bd33ec03a 100644
--- a/chrome/browser/chromeos/platform_keys/platform_keys_service.cc
+++ b/chrome/browser/chromeos/platform_keys/platform_keys_service.cc
@@ -6,6 +6,7 @@
 
 #include "base/base64.h"
 #include "base/callback.h"
+#include "base/callback_helpers.h"
 #include "base/values.h"
 #include "chrome/browser/chromeos/platform_keys/platform_keys.h"
 #include "content/public/browser/browser_thread.h"
@@ -30,32 +31,230 @@ scoped_ptr<base::StringValue> GetPublicKeyValue(
   return make_scoped_ptr(new base::StringValue(public_key_spki_der_b64));
 }
 
-void RunGenerateKeyCallback(
-    const PlatformKeysService::GenerateKeyCallback& callback,
-    const std::string& public_key_spki_der) {
-  callback.Run(public_key_spki_der, std::string() /* no error */);
-}
+}  // namespace
 
-// Callback used by |PlatformKeysService::Sign|.
-// Is called with the old validity of |public_key_spki_der| (or false if an
-// error occurred during reading the StateStore). If allowed, starts the actual
-// signing operation which will call back |callback|. If not allowed, calls
-// |callback| with an error.
-void CheckValidityAndSign(const std::string& token_id,
-                          scoped_ptr<platform_keys::SignRSAParams> params,
-                          const PlatformKeysService::SignCallback& callback,
-                          content::BrowserContext* browser_context,
-                          bool key_is_valid) {
-  if (!key_is_valid) {
-    callback.Run(std::string() /* no signature */,
-                 kErrorKeyNotAllowedForSigning);
-    return;
+class PlatformKeysService::Task {
+ public:
+  Task() {}
+  virtual ~Task() {}
+  virtual void Start() = 0;
+  virtual bool IsDone() = 0;
+
+ private:
+  DISALLOW_ASSIGN(Task);
+};
+
+class PlatformKeysService::PermissionUpdateTask : public Task {
+ public:
+  enum class State {
+    NOT_STARTED,
+    READ_PLATFORM_KEYS,
+    WRITE_UPDATE,

[kaliamoorthi, 2015/02/09 10:44:01]

Not sure if you need so many states. There is just a single asynchronous call
that is handled by this class. When the asynchronous call is complete, the
callback makes the state machine transition through WRITE_UPDATE and DONE.
NOT_STARTED is not that useful either. Makes one wonder if the state machine is
even needed here. It bloats the code for sure. A succinct implementation can
reduce the LOC in this class by a factor of 2.

[pneubeck (no reviews), 2015/02/09 14:02:38]

dropped the NOT_STARTED state (which was meant to more clearly document what the
_current_ state is).
But yeah, the additional bloat might not be worth it.

+    DONE,
+  };
+
+  PermissionUpdateTask(const bool new_permission_value,
+                       const std::string& public_key_spki_der,
+                       const std::string& extension_id,
+                       base::Callback<void(Task*)> callback,
+                       PlatformKeysService* service)
+      : new_permission_value_(new_permission_value),
+        public_key_spki_der_(public_key_spki_der),
+        extension_id_(extension_id),
+        callback_(callback),
+        service_(service),
+        weak_factory_(this) {}
+
+  ~PermissionUpdateTask() override {}
+
+  void Start() override {
+    CHECK(state_ == State::NOT_STARTED);
+    Step();
+  }
+  bool IsDone() override { return state_ == State::DONE; }
+  bool old_permission_value() { return old_permission_value_; }
+
+ private:
+  void Step() {
+    CHECK(state_ != State::DONE);
+    state_ = next_state_;
+    switch (state_) {
+      case State::NOT_STARTED:
+        NOTREACHED();
+        return;
+      case State::READ_PLATFORM_KEYS:
+        next_state_ = State::WRITE_UPDATE;
+        ReadPlatformKeys();
+        return;
+      case State::WRITE_UPDATE:
+        next_state_ = State::DONE;
+        WriteUpdate();
+        return;
+      case State::DONE:
+        if (!callback_.is_null()) {
+          // Make a local copy of the callback to run as it might be deleted
+          // during the Run().
+          base::ResetAndReturn(&callback_).Run(this);
+          // |this| might be invalid now.
+        }
+        return;
+    }
   }
-  platform_keys::subtle::SignRSA(token_id, params.Pass(), callback,
-                                 browser_context);
-}
 
-}  // namespace
+  // Reads the PlatformKeys value from the extension's state store and calls
+  // back to GotPlatformKeys().
+  void ReadPlatformKeys() {
+    service_->GetPlatformKeysOfExtension(
+        extension_id_, base::Bind(&PermissionUpdateTask::GotPlatformKeys,
+                                  weak_factory_.GetWeakPtr()));
+  }
+
+  void GotPlatformKeys(scoped_ptr<base::ListValue> platform_keys) {
+    platform_keys_ = platform_keys.Pass();
+    Step();
+  }
+
+  // Returns whether the extension has permission to use the key for signing
+  // according to the PlatformKeys value read from the extensions state store.
+  // Invalidates the key if it was found to be valid.
+  void WriteUpdate() {
+    scoped_ptr<base::StringValue> key_value(
+        GetPublicKeyValue(public_key_spki_der_));
+
+    base::ListValue::const_iterator it = platform_keys_->Find(*key_value);
+    old_permission_value_ = it != platform_keys_->end();
+    if (old_permission_value_ == new_permission_value_)
+      return;
+
+    if (new_permission_value_)
+      platform_keys_->Append(key_value.release());
+    else
+      platform_keys_->Remove(*key_value, nullptr);
+
+    service_->SetPlatformKeysOfExtension(extension_id_, platform_keys_.Pass());
+    Step();
+  }
+
+  State state_ = State::NOT_STARTED;
+  State next_state_ = State::READ_PLATFORM_KEYS;
+  scoped_ptr<base::ListValue> platform_keys_;
+  bool old_permission_value_ = false;
+
+  const bool new_permission_value_;
+  const std::string public_key_spki_der_;
+  const std::string extension_id_;
+  base::Callback<void(Task*)> callback_;
+  PlatformKeysService* const service_;
+  base::WeakPtrFactory<PermissionUpdateTask> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(PermissionUpdateTask);
+};
+
+class PlatformKeysService::SignTask : public Task {
+ public:
+  enum class State {
+    NOT_STARTED,
+    UPDATE_PERMISSION,
+    SIGN_OR_ABORT,
+    DONE,
+  };
+
+  // This Task will check the permissions of the extension with |extension_id|
+  // for the key identified by |public_key_spki_der|, then updates the
+  // permission to prevent any future signing operation of that extension using
+  // that same key. If the permission check was positive, it will actually sign
+  // |data| with the key and pass the signature to |callback|.
+  // If an error occurs, an error message is passed to |callback| instead.
+  SignTask(const std::string& token_id,
+           scoped_ptr<platform_keys::SignRSAParams> params,
+           const std::string& extension_id,
+           const SignCallback& callback,
+           PlatformKeysService* service)
+      : token_id_(token_id),
+        params_(params.Pass()),
+        extension_id_(extension_id),
+        callback_(callback),
+        service_(service),
+        weak_factory_(this) {}
+  ~SignTask() override {}
+
+  void Start() override {
+    CHECK(state_ == State::NOT_STARTED);
+    Step();
+  }
+  bool IsDone() override { return state_ == State::DONE; }
+
+ private:
+  void Step() {
+    CHECK(state_ != State::DONE);
+    state_ = next_state_;
+    switch (state_) {
+      case State::NOT_STARTED:
+        NOTREACHED();
+        return;
+      case State::UPDATE_PERMISSION:
+        next_state_ = State::SIGN_OR_ABORT;
+        UpdatePermission();
+        return;
+      case State::SIGN_OR_ABORT:
+        next_state_ = State::DONE;
+        if (!service_->permission_check_enabled_ ||
+            permission_update_->old_permission_value()) {
+          Sign();
+        } else {
+          callback_.Run(std::string() /* no signature */,
+                        kErrorKeyNotAllowedForSigning);
+          Step();
+        }
+        return;
+      case State::DONE:
+        service_->TaskFinished(this);
+        // |this| might be invalid now.
+        return;
+    }
+  }
+
+  // Reads the current permission of the extension with |extension_id_| for key
+  // |params_->public_key| and updates the permission to disable further
+  // signing operations with that key.
+  void UpdatePermission() {
+    permission_update_.reset(new PermissionUpdateTask(

[kaliamoorthi, 2015/02/09 10:44:01]

Combining SignTask with PermissionUpdateTask can result in a class that
justifies the use of a state machine. A clever implementation can make
PermissionUpdateTask be trigger-able by mentioning the end state alone since
both tasks execute PermissionUpdateTask as the first step. It would possibly
also help in constructor argument sharing.

[pneubeck (no reviews), 2015/02/09 14:02:38]

PermissionUpdate will also be used independent of Signing. I'd find it confusing
if that would be too tightly coupled with Signing.

+        false /* new permission value */, params_->public_key(), extension_id_,
+        base::Bind(&SignTask::DidUpdatePermission, weak_factory_.GetWeakPtr()),
+        service_));
+    permission_update_->Start();
+  }
+
+  void DidUpdatePermission(Task* /* task */) { Step(); }
+
+  // Starts the actual signing operation and afterwards passes the signature (or
+  // error) to |callback_|.
+  void Sign() {
+    platform_keys::subtle::SignRSA(
+        token_id_, params_.Pass(),
+        base::Bind(&SignTask::DidSign, weak_factory_.GetWeakPtr()),
+        service_->browser_context_);
+  }
+
+  void DidSign(const std::string& signature, const std::string& error_message) {
+    callback_.Run(signature, error_message);
+    Step();
+  }
+
+  State state_ = State::NOT_STARTED;
+  State next_state_ = State::UPDATE_PERMISSION;
+  scoped_ptr<base::ListValue> platform_keys_;
+  scoped_ptr<PermissionUpdateTask> permission_update_;
+
+  const std::string token_id_;
+  scoped_ptr<platform_keys::SignRSAParams> params_;
+  const std::string extension_id_;
+  const SignCallback callback_;
+  PlatformKeysService* const service_;
+  base::WeakPtrFactory<SignTask> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(SignTask);
+};
 
 PlatformKeysService::PlatformKeysService(
     content::BrowserContext* browser_context,
@@ -80,12 +279,9 @@ void PlatformKeysService::GenerateRSAKey(const std::string& token_id,
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   platform_keys::subtle::GenerateRSAKey(
-      token_id,
-      modulus_length,
-      base::Bind(&PlatformKeysService::GenerateRSAKeyCallback,
-                 weak_factory_.GetWeakPtr(),
-                 extension_id,
-                 callback),
+      token_id, modulus_length,
+      base::Bind(&PlatformKeysService::GeneratedKey, weak_factory_.GetWeakPtr(),
+                 extension_id, callback),
       browser_context_);
 }
 
@@ -95,10 +291,8 @@ void PlatformKeysService::SignRSA(
     const std::string& extension_id,
     const SignCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  ReadValidityAndInvalidateKey(
-      extension_id, params->public_key(),
-      base::Bind(&CheckValidityAndSign, token_id, base::Passed(&params),
-                 callback, browser_context_));
+  StartOrQueueTask(make_scoped_ptr(
+      new SignTask(token_id, params.Pass(), extension_id, callback, this)));
 }
 
 void PlatformKeysService::SelectClientCertificates(
@@ -114,29 +308,20 @@ void PlatformKeysService::SelectClientCertificates(
       browser_context_);
 }
 
-void PlatformKeysService::RegisterPublicKey(
-    const std::string& extension_id,
-    const std::string& public_key_spki_der,
-    const base::Closure& callback) {
-  GetPlatformKeysOfExtension(
-      extension_id,
-      base::Bind(&PlatformKeysService::RegisterPublicKeyGotPlatformKeys,
-                 weak_factory_.GetWeakPtr(),
-                 extension_id,
-                 public_key_spki_der,
-                 callback));
+void PlatformKeysService::StartOrQueueTask(scoped_ptr<Task> task) {
+  tasks_.push(make_linked_ptr(task.release()));
+  if (tasks_.size() == 1)
+    tasks_.front()->Start();
 }
 
-void PlatformKeysService::ReadValidityAndInvalidateKey(
-    const std::string& extension_id,
-    const std::string& public_key_spki_der,
-    const base::Callback<void(bool)>& callback) {
-  GetPlatformKeysOfExtension(extension_id,
-                             base::Bind(&PlatformKeysService::InvalidateKey,
-                                        weak_factory_.GetWeakPtr(),
-                                        extension_id,
-                                        public_key_spki_der,
-                                        callback));
+void PlatformKeysService::TaskFinished(Task* task) {
+  DCHECK(!tasks_.empty());
+  DCHECK(task == tasks_.front().get());
+  while (!tasks_.empty() && tasks_.front()->IsDone())
+    tasks_.pop();
+
+  if (!tasks_.empty())
+    tasks_.front()->Start();
 }
 
 void PlatformKeysService::GetPlatformKeysOfExtension(
@@ -155,18 +340,28 @@ void PlatformKeysService::SetPlatformKeysOfExtension(
                                   platform_keys.Pass());
 }
 
-void PlatformKeysService::GenerateRSAKeyCallback(
-    const std::string& extension_id,
-    const GenerateKeyCallback& callback,
-    const std::string& public_key_spki_der,
-    const std::string& error_message) {
+void PlatformKeysService::GeneratedKey(const std::string& extension_id,
+                                       const GenerateKeyCallback& callback,
+                                       const std::string& public_key_spki_der,
+                                       const std::string& error_message) {
   if (!error_message.empty()) {
     callback.Run(std::string() /* no public key */, error_message);
     return;
   }
-  base::Closure wrapped_callback(
-      base::Bind(&RunGenerateKeyCallback, callback, public_key_spki_der));
-  RegisterPublicKey(extension_id, public_key_spki_der, wrapped_callback);
+
+  StartOrQueueTask(make_scoped_ptr(new PermissionUpdateTask(
+      true /* new permission value */, public_key_spki_der, extension_id,
+      base::Bind(&PlatformKeysService::DidRegisterGeneratedKey,
+                 base::Unretained(this), callback, public_key_spki_der),
+      this)));
+}
+
+void PlatformKeysService::DidRegisterGeneratedKey(
+    const GenerateKeyCallback& callback,
+    const std::string& public_key_spki_der,
+    Task* task) {
+  callback.Run(public_key_spki_der, std::string() /* no error */);
+  TaskFinished(task);
 }
 
 void PlatformKeysService::SelectClientCertificatesCallback(
@@ -181,51 +376,6 @@ void PlatformKeysService::SelectClientCertificatesCallback(
   callback.Run(matches.Pass(), error_message);
 }
 
-void PlatformKeysService::RegisterPublicKeyGotPlatformKeys(
-    const std::string& extension_id,
-    const std::string& public_key_spki_der,
-    const base::Closure& callback,
-    scoped_ptr<base::ListValue> platform_keys) {
-  scoped_ptr<base::StringValue> key_value(
-      GetPublicKeyValue(public_key_spki_der));
-
-  DCHECK(platform_keys->end() == platform_keys->Find(*key_value))
-      << "Keys are assumed to be generated and not to be registered multiple "
-         "times.";
-  platform_keys->Append(key_value.release());
-  SetPlatformKeysOfExtension(extension_id, platform_keys.Pass());
-  callback.Run();
-}
-
-void PlatformKeysService::InvalidateKey(
-    const std::string& extension_id,
-    const std::string& public_key_spki_der,
-    const base::Callback<void(bool)>& callback,
-    scoped_ptr<base::ListValue> platform_keys) {
-  scoped_ptr<base::StringValue> key_value(
-      GetPublicKeyValue(public_key_spki_der));
-
-  size_t index = 0;
-  // If the key is found in |platform_keys|, it's valid for the extension to use
-  // it for signing.
-  bool key_was_valid = platform_keys->Remove(*key_value, &index);
-
-  if (key_was_valid) {
-    // Persist that the key is now invalid.
-    SetPlatformKeysOfExtension(extension_id, platform_keys.Pass());
-  }
-
-  if (permission_check_enabled_) {
-    // If permission checks are enabled, pass back the key permission (before
-    // it was removed above).
-    callback.Run(key_was_valid);
-  } else {
-    // Otherwise just allow signing with the key (which is enabled for testing
-    // only).
-    callback.Run(true);
-  }
-}
-
 void PlatformKeysService::GotPlatformKeysOfExtension(
     const std::string& extension_id,
     const GetPlatformKeysCallback& callback,