Implementation for chrome.certificateProvider.requestPin/stopPinRequest

chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc

 // Copyright 2015 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.
 
 #include <openssl/evp.h>
 #include <openssl/rsa.h>
 #include <stddef.h>
 #include <stdint.h>
 #include <stdlib.h>
 
 #include <memory>
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/run_loop.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
+#include "chrome/browser/chromeos/certificate_provider/certificate_provider_service.h"
+#include "chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.h"
+#include "chrome/browser/extensions/api/certificate_provider/certificate_provider_api.h"
 #include "chrome/browser/extensions/extension_apitest.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/policy/core/browser/browser_policy_connector.h"
 #include "components/policy/core/common/mock_configuration_policy_provider.h"
 #include "components/policy/core/common/policy_map.h"
 #include "components/policy/core/common/policy_types.h"
 #include "components/policy/policy_constants.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/test_navigation_observer.h"
 #include "content/public/test/test_utils.h"
 #include "crypto/rsa_private_key.h"
 #include "crypto/scoped_openssl_types.h"
 #include "extensions/common/extension.h"
 #include "extensions/test/result_catcher.h"
 #include "net/test/spawned_test_server/spawned_test_server.h"
 #include "testing/gmock/include/gmock/gmock.h"
+#include "ui/views/controls/label.h"
+#include "ui/views/controls/textfield/textfield.h"
+#include "ui/views/widget/widget.h"
 
 using testing::Return;
 using testing::_;
 
 namespace {
 
 void IgnoreResult(const base::Closure& callback, const base::Value* value) {
   callback.Run();
 }
 
@@ skipping 64 matching lines @@
 std::string JsUint8Array(const std::vector<uint8_t>& bytes) {
   std::string res = "new Uint8Array([";
   for (const uint8_t byte : bytes) {
     res += base::UintToString(byte);
     res += ", ";
   }
   res += "])";
   return res;
 }
 
+// Enters the code in the ShowPinDialog window and pushes the OK event.
+void EnterCode(chromeos::CertificateProviderService* service,
+               const base::string16& code) {
+  chromeos::RequestPinView* view =
+      service->pin_dialog_manager()->active_view_for_testing();
+  view->textfield_for_testing()->SetText(code);
+  view->Accept();
+  base::RunLoop().RunUntilIdle();
+}
+
+// Enters the valid code for extensions from local example folders, in the
+// ShowPinDialog window and waits for the window to close.
+void EnterCorrectPin(chromeos::CertificateProviderService* service) {
+  views::Widget* active_window =
+      service->pin_dialog_manager()->active_window_for_testing();
+  EnterCode(service, base::ASCIIToUTF16("1234"));
+
+  // Wait for extension to send request to close the window.
+  while (!active_window->IsClosed()) {
+    base::RunLoop().RunUntilIdle();

[Devlin, 2016/10/20 21:20:44]

Busy loops are generally bad, and timeouts over failures are harder to debug.
Can we make this more deterministic?

[igorcov, 2016/10/25 16:38:35]

I could probably make an WidgetObserver and involve a couple of callbacks in the
code to wait for closing event from window. But I feel like it will complicate
the code too much. And there are other places where I need to wait for some
other type of events (line 164 too).

[Devlin, 2016/10/26 16:59:15]

What about waiting for a message from the extension instead?  Then at least we
know the extension tried to close the window and can verify whether it happened.
 Alternatively, writing a "WaitUntilWidgetClosed" is pretty simple:

class WidgetClosedWaiter : public WidgetObserver {
 public:
  WidgetClosedWaiter(Widget* widget) : WidgetObserver(widget);
  void OnWidgetDestroying(Widget* widget) override {
    closed_ = true;
    run_loop_.Quit();
  }

  void Wait() {
    if (closed_)
      return;
    run_loop_.Run();
  }

 private:
  bool closed_ = false;
  base::RunLoop run_loop_;
};

WidgetClosedWaiter closed_waiter(widget);
// Do something
closed_waiter.Wait();

[igorcov, 2016/11/04 15:51:41]

Implemented, thanks!

+  }
+}
+
+// Enters an invalid code for extensions from local example folders, in the
+// ShowPinDialog window and waits for the window to update with the error.
+void EnterWrongPin(chromeos::CertificateProviderService* service) {
+  EnterCode(service, base::ASCIIToUTF16("567"));
+
+  chromeos::RequestPinView* view =
+      service->pin_dialog_manager()->active_view_for_testing();
+  // Waiting for extension to open the dialog again with an error in red
+  // displayed.
+  while (view->error_label_for_testing()->enabled_color() != SK_ColorRED) {
+    base::RunLoop().RunUntilIdle();
+  }
+}
+
 class CertificateProviderApiTest : public ExtensionApiTest {
  public:
   CertificateProviderApiTest() {}
 
   void SetUpInProcessBrowserTestFixture() override {
     EXPECT_CALL(provider_, IsInitializationComplete(_))
         .WillRepeatedly(Return(true));
     policy::BrowserPolicyConnector::SetPolicyProviderForTesting(&provider_);
 
     ExtensionApiTest::SetUpInProcessBrowserTestFixture();
@@ skipping 15 matching lines @@
                nullptr);
     provider_.UpdateChromePolicy(policy);
 
     content::RunAllPendingInMessageLoop();
   }
 
  protected:
   policy::MockConfigurationPolicyProvider provider_;
 };
 
+class CertificateProviderRequestPinTest : public CertificateProviderApiTest {
+ public:
+  // Loads certificate_provider extension from |folder| and |file_name|.
+  // Returns the CertificateProviderService object from browser context.
+  chromeos::CertificateProviderService* LoadRequestPinExtension(
+      const std::string& folder,
+      const std::string& file_name) {
+    content::WebContents* extension_contents =

[Devlin, 2016/10/20 21:20:44]

How is that the extension_contents when the extension isn't loaded?  Also, if we
just use this for the browser context, let's just use the test class accessor.

[igorcov, 2016/10/25 16:38:35]

You're right, we need this just for browser context. Don't understand though
what do you mean by class accessor?

[Devlin, 2016/10/26 16:59:15]

Browser tests have a profile() accessor.

[igorcov, 2016/11/04 15:51:41]

Fixed, Thanks.

+        browser()->tab_strip_model()->GetActiveWebContents();
+    chromeos::CertificateProviderService* service =
+        chromeos::CertificateProviderServiceFactory::GetForBrowserContext(
+            extension_contents->GetBrowserContext());
+    const base::FilePath extension_path =
+        test_data_dir_.AppendASCII("certificate_provider/" + folder);
+    const extensions::Extension* const extension =
+        LoadExtension(extension_path);
+    service->pin_dialog_manager()->AddSignRequestId(extension->id(), 123);
+    ui_test_utils::NavigateToURL(browser(),
+                                 extension->GetResourceURL(file_name));
+    base::RunLoop().RunUntilIdle();

[Devlin, 2016/10/20 21:20:44]

What's this for?

[igorcov, 2016/10/25 16:38:35]

I expected the browser would need some time to navigate to URL. Turns out it
doesn't:)

[Devlin, 2016/10/26 16:59:15]

Yep, NavigateToURL() blocks until navigation is complete, by design.  (And if it
didn't, RunUntilIdle() would be insufficient, since run loops only apply to one
thread.)

+    return service;
+  }
+};
+
 }  // namespace
 
 IN_PROC_BROWSER_TEST_F(CertificateProviderApiTest, Basic) {
   // Start an HTTPS test server that requests a client certificate.
   net::SpawnedTestServer::SSLOptions ssl_options;
   ssl_options.request_client_certificate = true;
   net::SpawnedTestServer https_server(net::SpawnedTestServer::TYPE_HTTPS,
                                       ssl_options, base::FilePath());
   ASSERT_TRUE(https_server.Start());
 
@@ skipping 91 matching lines @@
     base::RunLoop run_loop;
     const std::string code = "replyWithSignatureSecondTime();";
     bool result = false;
     extension_contents->GetMainFrame()->ExecuteJavaScriptForTests(
         base::ASCIIToUTF16(code),
         base::Bind(&StoreBool, &result, run_loop.QuitClosure()));
     run_loop.Run();
     EXPECT_TRUE(result);
   }
 }
+
+// User enters the correct PIN.
+IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest, ShowPinDialogAccept) {
+  chromeos::CertificateProviderService* service =
+      LoadRequestPinExtension("request_pin", "basic.html");
+
+  // Enter the valid PIN.
+  EnterCorrectPin(service);
+
+  // The view should be set to nullptr when the window is closed.
+  EXPECT_EQ(service->pin_dialog_manager()->active_view_for_testing(), nullptr);
+}
+
+// User closes the dialog kMaxClosedDialogsPer10Mins times, and the extension
+// should be blocked from showing it again.
+IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest, ShowPinDialogClose) {
+  chromeos::CertificateProviderService* service =
+      LoadRequestPinExtension("request_pin", "basic.html");
+
+  views::Widget* window =
+      service->pin_dialog_manager()->active_window_for_testing();
+  for (int i = 0;
+       i < extensions::api::certificate_provider::kMaxClosedDialogsPer10Mins;
+       i++) {
+    window->Close();
+    // Waiting for the new window to pop up.
+    while (service->pin_dialog_manager()->active_view_for_testing() == nullptr)
+      base::RunLoop().RunUntilIdle();

[Devlin, 2016/10/20 21:20:44]

ditto re busy loops

+
+    window = service->pin_dialog_manager()->active_window_for_testing();
+  }
+
+  // This time when closed, the window should not be able to pop up again.
+  window->Close();
+  base::RunLoop().RunUntilIdle();

[Devlin, 2016/10/20 21:20:44]

If the "while" on line 364 was needed, then this wouldn't be sufficient.

[igorcov, 2016/10/25 16:38:35]

It's different, because we don't have to wait for another window to pop-up here
(like in the case above). I agree that it's not a safe bet here, as other window
could pop-up after the EXPECT check was done, but don't really see a better way.

[Devlin, 2016/11/01 16:24:30]

It's not really different, though.

If the while () is necessary on line 364, then that means that it takes more
than one spin to open a new window.  If it takes more than one spin to open a
new window, then checking that a window isn't open after one spin doesn't
actually test anything.  Right?

+  EXPECT_EQ(service->pin_dialog_manager()->active_view_for_testing(), nullptr);
+}
+
+// User enters a wrong PIN first and a correct PIN on the second try.
+IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest,
+                       ShowPinDialogWrongPin) {
+  chromeos::CertificateProviderService* service =
+      LoadRequestPinExtension("request_pin", "basic.html");
+  EnterWrongPin(service);
+
+  // The window should be active.
+  EXPECT_EQ(
+      service->pin_dialog_manager()->active_window_for_testing()->IsVisible(),
+      true);
+  EXPECT_NE(service->pin_dialog_manager()->active_view_for_testing(), nullptr);

[Devlin, 2016/10/20 21:20:44]

If this were null, your expectation above would seg fault. :)

[igorcov, 2016/10/25 16:38:35]

I think you misread, one is active_window_for_testing and the other is
active_view_for_testing.

+
+  // Enter the valid PIN.
+  EnterCorrectPin(service);
+
+  // The view should be set to nullptr when the window is closed.
+  EXPECT_EQ(service->pin_dialog_manager()->active_view_for_testing(), nullptr);
+}
+
+// User enters wrong PIN three times.
+IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest,
+                       ShowPinDialogWrongPinThreeTimes) {
+  chromeos::CertificateProviderService* service =
+      LoadRequestPinExtension("request_pin", "basic.html");
+  for (int i = 0; i < 3; i++) {
+    EnterWrongPin(service);
+  }
+
+  chromeos::RequestPinView* view =
+      service->pin_dialog_manager()->active_view_for_testing();
+
+  // The textfield has to be disabled, as extension does not allow input now.
+  EXPECT_EQ(view->textfield_for_testing()->enabled(), false);
+
+  // Close the dialog.
+  service->pin_dialog_manager()->active_window_for_testing()->Close();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(service->pin_dialog_manager()->active_view_for_testing(), nullptr);
+}
+
+// User closes the dialog while the extension is processing the request.
+IN_PROC_BROWSER_TEST_F(CertificateProviderRequestPinTest,
+                       ShowPinDialogCloseWhileProcessing) {
+  chromeos::CertificateProviderService* service =
+      LoadRequestPinExtension("request_pin", "basic_lock.html");
+
+  EnterCode(service, base::ASCIIToUTF16("123"));
+  service->pin_dialog_manager()->active_window_for_testing()->Close();
+  base::RunLoop().RunUntilIdle();
+  // The view should be set to nullptr when the window is closed.
+  EXPECT_EQ(service->pin_dialog_manager()->active_view_for_testing(), nullptr);
+}