platformKeys: Hook up the certificate selection dialog to selectClientCertificates.

chrome/browser/chromeos/platform_keys/platform_keys_service_factory.cc

Index: chrome/browser/chromeos/platform_keys/platform_keys_service_factory.cc
diff --git a/chrome/browser/chromeos/platform_keys/platform_keys_service_factory.cc b/chrome/browser/chromeos/platform_keys/platform_keys_service_factory.cc
index 8394c2f38f3cb6b15587ac8798775389760d9eed..e8dd230614000257d30d7aadd738be494f90c6f0 100644
--- a/chrome/browser/chromeos/platform_keys/platform_keys_service_factory.cc
+++ b/chrome/browser/chromeos/platform_keys/platform_keys_service_factory.cc
@@ -4,14 +4,65 @@
 
 #include "chrome/browser/chromeos/platform_keys/platform_keys_service_factory.h"
 
+#include "base/bind.h"
 #include "base/logging.h"
 #include "base/memory/singleton.h"
+#include "base/memory/weak_ptr.h"
 #include "chrome/browser/chromeos/platform_keys/platform_keys_service.h"
 #include "chrome/browser/extensions/extension_system_factory.h"
+#include "chrome/browser/policy/profile_policy_connector.h"
+#include "chrome/browser/policy/profile_policy_connector_factory.h"
 #include "chrome/browser/profiles/incognito_helpers.h"
+#include "chrome/browser/ui/platform_keys_certificate_selector_chromeos.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "extensions/browser/extension_registry.h"
 
 namespace chromeos {
+namespace {
+
+// This delegate selects a certificate by showing the certificate selection
+// dialog to the user.
+class DefaultSelectDelegate
+    : public chromeos::PlatformKeysService::SelectDelegate {
+ public:
+  // If |enabled| is false, this delegate always selects no certificate.
+  explicit DefaultSelectDelegate(bool enabled)
+      : enabled_(enabled), weak_factory_(this) {}
+  ~DefaultSelectDelegate() override {}
+
+  void Select(const std::string& extension_id,
+              const net::CertificateList& certs,
+              const CertificateSelectedCallback& callback,
+              content::WebContents* web_contents,
+              content::BrowserContext* context) override {
+    if (!enabled_) {

[msw, 2015/02/19 20:21:54]

Consider defining a separate no-op SelectDelegate subclass instead of using an
|enabled_| flag to cripple this class.

[pneubeck (no reviews), 2015/02/19 21:51:41]

Done.

+      callback.Run(nullptr);
+      return;
+    }
+    CHECK(web_contents);
+    const extensions::Extension* extension =

[bartfab (slow), 2015/02/19 18:55:42]

Nit 1: s/const extensions::Extension*/const extensions::Extension* const/
Nit 2: #include "extensions/common/extension.h"

[pneubeck (no reviews), 2015/02/19 21:51:41]

Done.

+        extensions::ExtensionRegistry::Get(context)->GetExtensionById(
+            extension_id, extensions::ExtensionRegistry::ENABLED);

[bartfab (slow), 2015/02/19 18:55:42]

What if the extension is not found? At least a DCHECK() would make sense.

[pneubeck (no reviews), 2015/02/19 21:51:41]

Done.

+    ShowPlatformKeysCertificateSelector(
+        web_contents, extension->short_name(), certs,
+        // Don't call |callback| once this delegate is destructed, thus use a
+        // WeakPtr.
+        base::Bind(&DefaultSelectDelegate::SelectedCertificate,
+                   weak_factory_.GetWeakPtr(), callback));
+  }
+
+  void SelectedCertificate(
+      const CertificateSelectedCallback& callback,
+      const scoped_refptr<net::X509Certificate>& selected_cert) {

[bartfab (slow), 2015/02/19 18:55:42]

Nit 1: #include "base/memory/ref_counted.h"
Nit 2: #include "net/cert/x509_certificate.h"

[pneubeck (no reviews), 2015/02/19 21:51:41]

Done.

+    callback.Run(selected_cert);

[msw, 2015/02/19 20:21:54]

Shouldn't this DCHECK(enabled_) or ensure that |selected_cert| is null/invalid
if |enabled_| is false? (or maybe ignore |selected_cert| altogether in that case
and run the callback with nullptr?)

[pneubeck (no reviews), 2015/02/19 21:51:41]

not required anymore.

+  }
+
+ private:
+  const bool enabled_;
+  base::WeakPtrFactory<DefaultSelectDelegate> weak_factory_;
+};

[bartfab (slow), 2015/02/19 18:55:42]

Nit: DISALLOW_COPY_AND_ASSIGN.

[pneubeck (no reviews), 2015/02/19 21:51:41]

Done.

+
+}  // namespace
 
 // static
 PlatformKeysService* PlatformKeysServiceFactory::GetForBrowserContext(
@@ -45,7 +96,17 @@ KeyedService* PlatformKeysServiceFactory::BuildServiceInstanceFor(
   extensions::StateStore* store =
       extensions::ExtensionSystem::Get(context)->state_store();
   DCHECK(store);
-  return new PlatformKeysService(context, store);
+  PlatformKeysService* service = new PlatformKeysService(context, store);
+
+  policy::ProfilePolicyConnector* connector =

[bartfab (slow), 2015/02/19 18:55:42]

Nit: const.

[pneubeck (no reviews), 2015/02/19 21:51:41]

Done.

+      policy::ProfilePolicyConnectorFactory::GetForBrowserContext(context);
+  // Only allow the user to grant certificate permissions to extensions, if user

[bartfab (slow), 2015/02/19 18:55:42]

Nit: s/, if/if the/

[pneubeck (no reviews), 2015/02/19 21:51:40]

Done.

+  // is not managed by policy. Otherwise the user might leak access to (private
+  // keys of) certificates against the intentions of the administrator.
+  // TODO(pneubeck): Remove this once the respective policy is implemented.

[bartfab (slow), 2015/02/19 18:55:42]

Nit: Reference a crbug number.

[pneubeck (no reviews), 2015/02/19 21:51:40]

Done.

+  service->SetSelectDelegate(make_scoped_ptr(

[bartfab (slow), 2015/02/19 18:55:42]

Nit: #include "base/memory/scoped_ptr.h"

[pneubeck (no reviews), 2015/02/19 21:51:40]

Done.

+      new DefaultSelectDelegate(!connector->IsManaged() /* enabled */)));
+  return service;
 }
 
 }  // namespace chromeos