platformKeys: Add per-extension sign permissions.

chrome/browser/chromeos/platform_keys/platform_keys_service.h

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_CHROMEOS_PLATFORM_KEYS_PLATFORM_KEYS_SERVICE_H_
 #define CHROME_BROWSER_CHROMEOS_PLATFORM_KEYS_PLATFORM_KEYS_SERVICE_H_
 
 #include <queue>
 #include <string>
 #include <vector>
@@ skipping 21 matching lines @@
 
 namespace net {
 class X509Certificate;
 typedef std::vector<scoped_refptr<X509Certificate>> CertificateList;
 }
 
 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.
   // The format written to |state_store| is:
   //   kStateStorePlatformKeys maps to a list of strings.
   //   Each string is the base64 encoding of the DER representation of a public
   //   key's SPKI.
   explicit PlatformKeysService(content::BrowserContext* browser_context,
                                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
   // currently ignored, instead the user token associated with |browser_context|
   // is always used. |callback| will be invoked with the resulting public key or
   // an error.
   // Will only call back during the lifetime of this object.
   void GenerateRSAKey(const std::string& token_id,
                       unsigned int modulus_length_bits,
                       const std::string& extension_id,
                       const GenerateKeyCallback& callback);
 
   // 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
   // provided and the key is not found in that token, the operation aborts.
   // If the extension does not have permissions for signing with this key, the
   // operation aborts. In case of a one time permission (granted after
   // generating the key), this function also removes the permission to prevent
   // future signing attempts.
   // |callback| will be invoked with the signature or an error message.
   // Will only call back during the lifetime of this object.
@@ skipping 19 matching lines @@
   void SignRSAPKCS1Raw(const std::string& token_id,
                        const std::string& data,
                        const std::string& public_key,
                        const std::string& extension_id,
                        const SignCallback& callback);
 
   // 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.
-  typedef base::Callback<void(scoped_ptr<net::CertificateList> matches,
-                              const std::string& error_message)>
-      SelectCertificatesCallback;
+  using SelectCertificatesCallback =
+      base::Callback<void(scoped_ptr<net::CertificateList> matches,
+                          const std::string& error_message)>;
 
-  // Returns the list of all certificates that match |request|. |callback| will
-  // be invoked with these matches or an 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;
 
   // Starts |task| eventually. To ensure that at most one |Task| is running at a
   // time, it queues |task| for later execution if necessary.
   void StartOrQueueTask(scoped_ptr<Task> task);
 
   // Must be called after |task| is done. |task| will be invalid after this
   // call. This must not be called for any but the task that ran last. If any
   // other tasks are queued (see StartOrQueueTask()), it will start the next
   // one.
   void TaskFinished(Task* task);
 
   // Reads the list of public keys currently registered for |extension_id| from
   // StateStore. Calls |callback| with the read list, or a new empty list if
   // none existed. If an error occurred, calls |callback| with NULL.
   void GetPlatformKeysOfExtension(const std::string& extension_id,
                                   const GetPlatformKeysCallback& callback);
 
   // 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
   // for the given extension. If any error occurs during key generation or
   // registration, calls |callback| with an error. Otherwise, on success, calls
   // |callback| with the public key.
   void GeneratedKey(const std::string& extension_id,
                     const GenerateKeyCallback& callback,
                     const std::string& public_key_spki_der,
                     const std::string& error_message);
 
   // Callback used by |GeneratedKey|.
   // |public_key_spki_der| will contain the X.509 Subject Public Key Info  of
   // the generated key in DER encoding. |task| points to the finished |Task|
   // object.
   void RegisteredGeneratedKey(const GenerateKeyCallback& callback,
                               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|
   // with NULL; if no value existed, with an empty list.
   void GotPlatformKeysOfExtension(const std::string& extension_id,
                                   const GetPlatformKeysCallback& callback,
                                   scoped_ptr<base::Value> value);
 
   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_;
 
   DISALLOW_COPY_AND_ASSIGN(PlatformKeysService);
 };
 
 }  // namespace chromeos
 
 #endif  // CHROME_BROWSER_CHROMEOS_PLATFORM_KEYS_PLATFORM_KEYS_SERVICE_H_