Public Sessions - prompt the user for pageCapture requests

chrome/browser/chromeos/extensions/public_session_permission_helper_unittest.cc

Index: chrome/browser/chromeos/extensions/public_session_permission_helper_unittest.cc
diff --git a/chrome/browser/chromeos/extensions/public_session_permission_helper_unittest.cc b/chrome/browser/chromeos/extensions/public_session_permission_helper_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..ce06278368e872dae3e22b5623d1535044fff484
--- /dev/null
+++ b/chrome/browser/chromeos/extensions/public_session_permission_helper_unittest.cc
@@ -0,0 +1,211 @@
+// Copyright 2017 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 "chrome/browser/chromeos/extensions/public_session_permission_helper.h"
+
+#include <memory>
+#include <string>
+#include <utility>
+#include <vector>
+
+#include "base/bind.h"
+#include "base/memory/ptr_util.h"
+#include "base/run_loop.h"
+#include "chrome/browser/extensions/extension_install_prompt.h"
+#include "chrome/common/extensions/extension_test_util.h"
+#include "chrome/test/base/chrome_render_view_host_test_harness.h"
+#include "content/public/browser/web_contents.h"
+#include "extensions/browser/extension_dialog_auto_confirm.h"
+#include "extensions/common/extension.h"
+#include "extensions/common/manifest.h"
+#include "extensions/common/permissions/api_permission.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using extension_test_util::LoadManifestUnchecked;
+using content::WebContents;
+using extensions::APIPermission;
+using extensions::Extension;
+using extensions::Manifest;
+using Result = ExtensionInstallPrompt::Result;
+
+namespace {
+
+auto permission_a = APIPermission::kAudio;
+auto permission_b = APIPermission::kBookmark;
+
+scoped_refptr<Extension> LoadManifestHelper(const std::string& id) {
+  std::string error;
+  scoped_refptr<Extension> extension = LoadManifestUnchecked(
+      "common/background_page", "manifest.json", Manifest::INVALID_LOCATION,
+      Extension::NO_FLAGS, id, &error);
+  EXPECT_TRUE(extension.get()) << error;
+  return extension;
+}
+
+class ProgrammableInstallPrompt : public ExtensionInstallPrompt {

[Devlin, 2017/02/10 19:54:46]

Can we use a ScopedTestDialogAutoConfirm
(extensions/browser/extension_dialog_auto_confirm.h) here instead?  It can help
catch any errors that might not otherwise surface.

[Ivan Šandrk, 2017/02/13 13:18:20]

I used it in the first iteration of the unittest, but then realised that I
cannot do more complicated tests where I show two prompts and then resolve them
in random order, or ALLOW one of them and DENY the other. I could technically
use ScopedTestDialogAutoConfirm for some tests, and ProgrammableInstallPrompt
for others, but that just seems like complicating things too much.

If you know of any errors that can surface and which aren't caught with this
approach, I may consider using the mixed approach :)

+ public:
+  explicit ProgrammableInstallPrompt(WebContents* contents)
+      : ExtensionInstallPrompt(contents) {}
+
+  ~ProgrammableInstallPrompt() override {}
+
+  void ShowDialog(
+      const DoneCallback& done_callback,
+      const extensions::Extension* extension,
+      const SkBitmap* icon,
+      std::unique_ptr<Prompt> prompt,
+      std::unique_ptr<const extensions::PermissionSet> custom_permissions,
+      const ShowDialogCallback& show_dialog_callback) override {
+    done_callback_ = done_callback;
+  }
+
+  void Resolve(ExtensionInstallPrompt::Result result) {
+    done_callback_.Run(result);
+  }
+
+ private:
+  ExtensionInstallPrompt::DoneCallback done_callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(ProgrammableInstallPrompt);
+};
+
+std::unique_ptr<ExtensionInstallPrompt> ReturnPrompt(
+    std::unique_ptr<ExtensionInstallPrompt> prompt,
+    WebContents* web_contents) {
+  return prompt;
+}
+
+}  // namespace
+
+namespace extensions {
+namespace permission_helper {
+
+class PublicSessionPermissionHelperTest
+    : public ChromeRenderViewHostTestHarness {
+ public:
+  PublicSessionPermissionHelperTest() {}
+  ~PublicSessionPermissionHelperTest() override {}
+
+  // testing::Test
+  void SetUp() override;
+  void TearDown() override;
+
+  // Class helpers.
+  void RequestResolved(const PermissionIDSet& allowed_permissions);
+  ProgrammableInstallPrompt* CallHandlePermissionRequest(
+      const scoped_refptr<Extension>& extension,
+      const PermissionIDSet& permissions);
+
+ protected:
+  scoped_refptr<Extension> extension_a_;
+  scoped_refptr<Extension> extension_b_;
+
+  std::vector<PermissionIDSet> allowed_permissions_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(PublicSessionPermissionHelperTest);
+};
+
+void PublicSessionPermissionHelperTest::SetUp() {
+  ChromeRenderViewHostTestHarness::SetUp();
+  extension_a_ = LoadManifestHelper("extension_a");
+  extension_b_ = LoadManifestHelper("extension_b");
+}
+
+void PublicSessionPermissionHelperTest::TearDown() {
+  ResetPermissionsForTesting();
+  ChromeRenderViewHostTestHarness::TearDown();
+}
+
+void PublicSessionPermissionHelperTest::RequestResolved(
+    const PermissionIDSet& allowed_permissions) {
+  allowed_permissions_.push_back(allowed_permissions);
+}
+
+ProgrammableInstallPrompt*
+PublicSessionPermissionHelperTest::CallHandlePermissionRequest(
+    const scoped_refptr<Extension>& extension,
+    const PermissionIDSet& permissions) {
+  auto prompt = new ProgrammableInstallPrompt(web_contents());
+  auto factory_callback = base::Bind(
+      &ReturnPrompt,
+      base::Passed(base::WrapUnique<ExtensionInstallPrompt>(prompt)));
+  HandlePermissionRequest(
+      *extension.get(), permissions, web_contents(),
+      base::Bind(&PublicSessionPermissionHelperTest::RequestResolved,
+                 base::Unretained(this)),
+      factory_callback);
+  return prompt;

[Andrew T Wilson (Slow), 2017/02/11 09:23:24]

This looks great - one thing to keep in mind is that prompt *can* get freed
before you return (so you'll return a freed pointer) in the case that you call
HandlePermissionRequest() for a set of permissions that are already granted.

Not a problem for your existing tests, but if there's ever a bug introduced, you
might end up getting a crashed test instead of a nice failed expectation when
the caller calls Resolve() on a freed pointer, which is mildly more difficult to
diagnose/flaky.

I think it's fine as-is (I personally wouldn't do anything further). But an
alternative would be to use weak pointers to ensure you never return a pointer
into freed memory (only a nullptr if the callback was already invoked and the
PermissionInstallPrompt was already freed). But I probably wouldn't bother since
you'll be getting a crashed test either way.

[Ivan Šandrk, 2017/02/13 13:18:20]

It did cross my mind, but I didn't worry too much about it (because tests -
maybe I should worry a bit more tho?).

I've implemented the WeakPtr approach since it also gives me the ability to tell
whether the prompt was called or not.

+}
+
+TEST_F(PublicSessionPermissionHelperTest, TestPermissionAllowed) {
+  // Allow permission_a for extension_a.
+  CallHandlePermissionRequest(extension_a_, {permission_a})
+      ->Resolve(Result::ACCEPTED);
+  EXPECT_TRUE(allowed_permissions_.back().Equals({permission_a}));
+
+  // permission_a was already allowed for extension_a.
+  CallHandlePermissionRequest(extension_a_, {permission_a});
+  EXPECT_TRUE(allowed_permissions_.back().Equals({permission_a}));
+
+  // permission_a was allowed only for extension_a.
+  CallHandlePermissionRequest(extension_b_, {permission_a})
+      ->Resolve(Result::USER_CANCELED);
+  EXPECT_TRUE(allowed_permissions_.back().Equals({}));
+}
+
+TEST_F(PublicSessionPermissionHelperTest, TestPermissionDenied) {
+  // Deny permission_a for extension_a.
+  CallHandlePermissionRequest(extension_a_, {permission_a})
+      ->Resolve(Result::USER_CANCELED);
+  EXPECT_TRUE(allowed_permissions_.back().Equals({}));
+
+  // Still denied (previous choice is remembered).

[Devlin, 2017/02/10 19:54:46]

It would be nice to make sure we're actually remembering the choice, and that
the dialog isn't shown.  Right now, for instance, if the dialog is shown, we
never resolve it, and thus this test would pass (though the behavior is
incorrect).  If we can use the scoped autoconfirm, this is pretty easy:
{
  ScopedTestDialogAutoConfirm auto_deny(ScopedTestDialogAutoConfirm::CANCEL);
  CallHandlePermissionRequest();
  EXPECT_TRUE(allowed_permissions_.back().Equals({}));
}
{
  // Even though we would accept the prompt, the choice is persisted and the
prompt is never shown.
  ScopedTestDialogAutoConfirm auto_deny(ScopedTestDialogAutoConfirm::ACCEPT);
  CallHandlePermissionRequest();
  EXPECT_TRUE(allowed_permissions_.back().Equals({}));
}

[Ivan Šandrk, 2017/02/13 13:18:20]

I made some changes and now I can catch the dialog being shown when it
shouldn't. Looks better now?

+  CallHandlePermissionRequest(extension_a_, {permission_a});
+  EXPECT_TRUE(allowed_permissions_.back().Equals({}));
+
+  // permission_a was denied only for extension_a.
+  CallHandlePermissionRequest(extension_b_, {permission_a})
+      ->Resolve(Result::ACCEPTED);
+  EXPECT_TRUE(allowed_permissions_.back().Equals({permission_a}));
+}
+
+TEST_F(PublicSessionPermissionHelperTest, TestTwoPromptsA) {
+  // Open two permission prompts.
+  auto prompt1 =
+      CallHandlePermissionRequest(extension_a_, {permission_a, permission_b});
+  CallHandlePermissionRequest(extension_a_, {permission_b});

[Devlin, 2017/02/10 19:54:46]

Here, too, it would be nice to assert that no prompt is actually shown.

[Ivan Šandrk, 2017/02/13 13:18:20]

Done.

+  // prompt1 resolves both permission requests (second permission request
+  // doesn't show a prompt as permission_b is already prompted by first
+  // permission request).
+  prompt1->Resolve(Result::ACCEPTED);
+  EXPECT_TRUE(allowed_permissions_[0].Equals({permission_a, permission_b}));
+  EXPECT_TRUE(allowed_permissions_[1].Equals({permission_b}));
+}
+
+TEST_F(PublicSessionPermissionHelperTest, TestTwoPromptsB) {
+  auto prompt1 = CallHandlePermissionRequest(extension_a_, {permission_a});
+  auto prompt2 =
+      CallHandlePermissionRequest(extension_a_, {permission_a, permission_b});
+  // prompt2 resolves only permission_b because prompt1 already prompted for
+  // permission_a.
+  prompt2->Resolve(Result::ACCEPTED);
+  EXPECT_EQ(allowed_permissions_.size(), 0u);
+  prompt1->Resolve(Result::ACCEPTED);
+  EXPECT_TRUE(allowed_permissions_[0].Equals({permission_a}));
+  EXPECT_TRUE(allowed_permissions_[1].Equals({permission_a, permission_b}));
+}
+
+TEST_F(PublicSessionPermissionHelperTest, TestTwoPromptsDeny) {
+  auto prompt1 = CallHandlePermissionRequest(extension_a_, {permission_a});
+  auto prompt2 =
+      CallHandlePermissionRequest(extension_a_, {permission_a, permission_b});
+  prompt1->Resolve(Result::USER_CANCELED);
+  EXPECT_TRUE(allowed_permissions_[0].Equals({}));
+  prompt2->Resolve(Result::ACCEPTED);
+  EXPECT_TRUE(allowed_permissions_[1].Equals({permission_b}));
+}
+
+}  // namespace permission_helper
+}  // namespace extensions