platformKeys: Add per-extension sign permissions.

chrome/browser/chromeos/platform_keys/platform_keys_service.h

Index: chrome/browser/chromeos/platform_keys/platform_keys_service.h
diff --git a/chrome/browser/chromeos/platform_keys/platform_keys_service.h b/chrome/browser/chromeos/platform_keys/platform_keys_service.h
index 71adf3b7a32088fc33e1725ac506de880566f9ba..64bc12e14392be0543fd001bbea8d9a0b2520019 100644
--- a/chrome/browser/chromeos/platform_keys/platform_keys_service.h
+++ b/chrome/browser/chromeos/platform_keys/platform_keys_service.h
@@ -39,6 +39,33 @@ namespace chromeos {
 
 class PlatformKeysService : public KeyedService {
  public:
+  struct KeyEntry;
+  using KeyEntries = std::vector<KeyEntry>;
+
+  class SelectDelegate {

[Andrew T Wilson (Slow), 2015/02/18 19:53:35]

Would be nice if we had some class-level documentation here - this is an
interesting class that is called into, and which itself calls another callback
back when it is done with Select(). This is an unusual enough pattern that it
should be documented.

[pneubeck (no reviews), 2015/02/19 11:08:40]

Done.

+   public:
+    // TODO(pneubeck): Handle if the selection was aborted, e.g. by the user.
+    using Callback =

[Andrew T Wilson (Slow), 2015/02/18 19:53:35]

nit: I'm not a huge fan of overloading names (SelectDelegate::Callback vs
Callback - IMO they make the code harder to read as the user has to be aware of
what the current namespace is). Perhaps something more descriptive like
CertificateSelectedCallback?

[pneubeck (no reviews), 2015/02/19 11:08:40]

Done.

+        base::Callback<void(scoped_refptr<net::X509Certificate> selection)>;
+
+    SelectDelegate();
+    virtual ~SelectDelegate();
+
+    // Called on an interactive SelectClientCertificates call with the list of
+    // matching certificates, |certs|. Must eventually call |callback| or be
+    // destructed. |callback| will not be called after this delegate is

[Andrew T Wilson (Slow), 2015/02/18 19:53:35]

What do you mean |callback| will not be called after this delegate is
destructed? What are you trying to say here? Because typically one does not
describe behavior in class documentation around things that may or may not
happen after the object itself is destroyed?

Are you trying to say that if the object is destroyed mid-Select(), it should
not invoke the callback from its destructor? That's OK, but there are better
ways to deal with this type of dependency (like using weak pointers that are
reset before the delegate is freed).

[pneubeck (no reviews), 2015/02/19 11:08:40]

Yes, that's the point. This is part of the contract and fits explicit ownership
rather well: After SelectDelegate is destructed there shouldn't be anything left
that can call back.
Why would a weak pointer be preferred here? The Delegate can do that himself to
fulfill the contract.

[Andrew T Wilson (Slow), 2015/02/19 11:16:44]

OK, then you should word this prescriptively, not descriptively (i.e.
"|callback| must not be invoked after this delegate is destroyed").

+    // destructed.
+    // The certificate passed to |callback| will be forwarded to the
+    // calling extension and the extension will get unlimited sign permission
+    // for this cert. By passing null to |callback|, no cert will be selected.
+    virtual void Select(const std::string& extension_id,
+                        const net::CertificateList& certs,
+                        const Callback& callback) = 0;
+
+   private:

[Andrew T Wilson (Slow), 2015/02/18 19:53:35]

This class has no data members - why are you restricting assignment
constructors?

[pneubeck (no reviews), 2015/02/19 11:08:40]

the assignment operator is not virtual. calling it on a SelectDelegate type will
be valid but a no-op. So nobody will notice the error.

Not a huge deal, just prevents a small class of mistakes.

+    DISALLOW_ASSIGN(SelectDelegate);
+  };
+
   // Stores registration information in |state_store|, i.e. for each extension
   // the list of public keys that are valid to be used for signing. Each key can
   // be used for signing at most once.
@@ -50,18 +77,22 @@ class PlatformKeysService : public KeyedService {
                                extensions::StateStore* state_store);
   ~PlatformKeysService() override;
 
-  // Disables the checks whether an extension is allowed to read client
-  // certificates or allowed to use the signing function of a key.
-  // TODO(pneubeck): Remove this once a permissions are implemented.
-  void DisablePermissionCheckForTesting();
+  // Sets the delegate which will be used for interactive
+  // SelectClientCertificates calls.
+  void SetSelectDelegate(scoped_ptr<SelectDelegate> delegate);
+
+  // Grants unlimited sign permission for |cert| to the extension with the ID
+  // |extension_id|.
+  void GrantUnlimitedSignPermission(const std::string& extension_id,
+                                    scoped_refptr<net::X509Certificate> cert);
 
   // If the generation was successful, |public_key_spki_der| will contain the
   // DER encoding of the SubjectPublicKeyInfo of the generated key and
   // |error_message| will be empty. If it failed, |public_key_spki_der| will be
   // empty and |error_message| contain an error message.
-  typedef base::Callback<void(const std::string& public_key_spki_der,
-                              const std::string& error_message)>
-      GenerateKeyCallback;
+  using GenerateKeyCallback =
+      base::Callback<void(const std::string& public_key_spki_der,
+                          const std::string& error_message)>;
 
   // Generates an RSA key pair with |modulus_length_bits| and registers the key
   // to allow a single sign operation by the given extension. |token_id| is
@@ -77,8 +108,8 @@ class PlatformKeysService : public KeyedService {
   // If signing was successful, |signature| will be contain the signature and
   // |error_message| will be empty. If it failed, |signature| will be empty and
   // |error_message| contain an error message.
-  typedef base::Callback<void(const std::string& signature,
-                              const std::string& error_message)> SignCallback;
+  using SignCallback = base::Callback<void(const std::string& signature,
+                                           const std::string& error_message)>;
 
   // Digests |data|, applies PKCS1 padding and afterwards signs the data with
   // the private key matching |params.public_key|. If a non empty token id is
@@ -118,25 +149,30 @@ class PlatformKeysService : public KeyedService {
   // contain the list of matching certificates (maybe empty) and |error_message|
   // will be empty. If an error occurred, |matches| will be null and
   // |error_message| contain an error message.
-  typedef base::Callback<void(scoped_ptr<net::CertificateList> matches,
-                              const std::string& error_message)>
-      SelectCertificatesCallback;
-
-  // Returns the list of all certificates that match |request|. |callback| will
-  // be invoked with these matches or an error message.
+  using SelectCertificatesCallback =
+      base::Callback<void(scoped_ptr<net::CertificateList> matches,
+                          const std::string& error_message)>;
+
+  // Returns the list of all certificates that match |request|. If |interactive|
+  // is true will select from these matches using the currently set
+  // SelectDelegate. Afterwards filters only the certificates that the extension

[Andrew T Wilson (Slow), 2015/02/18 19:53:35]

I couldn't quite understand what "Afterwards filters only the certificates that
the extension has unlimited sign permission for". Shouldn't certificates be
filtered *before* the user is prompted to select one, not "afterwards"?

[pneubeck (no reviews), 2015/02/19 11:08:40]

I made a bit more verbose.

+  // has unlimited sign permission for. |callback| will be invoked with these
+  // certificates or an error message.
   // Will only call back during the lifetime of this object.
-  // TODO(pneubeck): Add the interactive option and integrate the select
-  // certificate dialog.
   void SelectClientCertificates(
       const platform_keys::ClientCertificateRequest& request,
+      bool interactive,
       const std::string& extension_id,
       const SelectCertificatesCallback& callback);
 
  private:
   using GetPlatformKeysCallback =
-      base::Callback<void(scoped_ptr<base::ListValue> platform_keys)>;
+      base::Callback<void(scoped_ptr<KeyEntries> platform_keys)>;
+
+  enum SignPermission { ONCE, UNLIMITED };
 
   class Task;
+  class SelectTask;

[Andrew T Wilson (Slow), 2015/02/18 19:53:35]

Should these classes be ordered somehow?

[pneubeck (no reviews), 2015/02/19 11:08:40]

Done.

   class SignTask;
   class PermissionUpdateTask;
 
@@ -159,7 +195,7 @@ class PlatformKeysService : public KeyedService {
   // Writes |platform_keys| to the state store of the extension with id
   // |extension_id|.
   void SetPlatformKeysOfExtension(const std::string& extension_id,
-                                  scoped_ptr<base::ListValue> platform_keys);
+                                  const KeyEntries& platform_keys);
 
   // Callback used by |GenerateRSAKey|.
   // If the key generation was successful, registers the generated public key
@@ -179,17 +215,6 @@ class PlatformKeysService : public KeyedService {
                               const std::string& public_key_spki_der,
                               Task* task);
 
-  // Calback used by |SelectClientCertificates|.
-  // If the certificate request could be processed successfully, |matches| will
-  // contain the list of matching certificates (maybe empty) and |error_message|
-  // will be empty. If an error occurred, |matches| will be null and
-  // |error_message| contain an error message.
-  void SelectClientCertificatesCallback(
-      const std::string& extension_id,
-      const SelectCertificatesCallback& callback,
-      scoped_ptr<net::CertificateList> matches,
-      const std::string& error_message);
-
   // Callback used by |GetPlatformKeysOfExtension|.
   // Is called with |value| set to the PlatformKeys value read from the
   // StateStore, which it forwards to |callback|. On error, calls |callback|
@@ -200,7 +225,7 @@ class PlatformKeysService : public KeyedService {
 
   content::BrowserContext* browser_context_;
   extensions::StateStore* state_store_;
-  bool permission_check_enabled_ = true;
+  scoped_ptr<SelectDelegate> select_delegate_;
   std::queue<linked_ptr<Task>> tasks_;
   base::WeakPtrFactory<PlatformKeysService> weak_factory_;