Add a browser test for access control for file: scheme

chrome/browser/net/chrome_network_delegate_browsertest.cc

Index: chrome/browser/net/chrome_network_delegate_browsertest.cc
diff --git a/chrome/browser/net/chrome_network_delegate_browsertest.cc b/chrome/browser/net/chrome_network_delegate_browsertest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..ce9f461bc91362ddc42a4dbd1befffb380b39200
--- /dev/null
+++ b/chrome/browser/net/chrome_network_delegate_browsertest.cc
@@ -0,0 +1,49 @@
+// Copyright (c) 2017 The Chromium Authors. All rights reserved.

[mmenke, 2017/06/05 18:31:33]

nit:  new files shouldn't use the "(c)"

[satorux1, 2017/06/06 08:52:37]

Good catch! Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/bind.h"
+#include "base/command_line.h"

[mmenke, 2017/06/05 18:31:33]

Neither of these are used.

[satorux1, 2017/06/06 08:52:37]

Good catch again. These are leftover from a previous attempt. Removed!

+#include "base/path_service.h"
+#include "base/strings/utf_string_conversions.h"

[mmenke, 2017/06/05 18:31:33]

Not used.

[satorux1, 2017/06/06 08:52:37]

Done.

+#include "chrome/browser/net/chrome_network_delegate.h"
+#include "chrome/browser/net/errorpage_test_util.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/common/chrome_paths.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/test/browser_test_utils.h"
+#include "net/base/filename_util.h"
+#include "net/base/network_delegate.h"
+#include "url/gurl.h"
+
+#if defined(OS_CHROMEOS)
+
+class ChromeNetworkDelegateBrowserTest : public InProcessBrowserTest {
+ protected:
+  ChromeNetworkDelegateBrowserTest() {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ChromeNetworkDelegateBrowserTest);

[mmenke, 2017/06/05 18:31:33]

include base/macros.h

[satorux1, 2017/06/06 08:52:37]

Done.

+};
+
+// Ensure that access to a test file via file: scheme is rejected with
+// ERR_ACCESS_DENIED.

[mmenke, 2017/06/05 18:31:33]

Also add a test with a symbolic like that is accessible to a location that
isn't?  That was the entire motivation for asking for this test, and what I'm
most concerned about regressing, when we switch to using a network service (When
we'll lose existing unit tests, and file requests may not even go through
ChromeNetworkDelegate, or URLRequest, if we decide on using a sandbox that
restricts the network process's file I/O capabilities).

[satorux1, 2017/06/06 08:52:37]

Good point. Added a test for that.


Also created a separate CL that is to replace "/tmp" with a call to
PathService::Get() for writing the test with a symlink:

https://codereview.chromium.org/2926543002/

+IN_PROC_BROWSER_TEST_F(ChromeNetworkDelegateBrowserTest, AccessToFile) {
+  // Access to all files via file: scheme is allowed on browser tests. Bring
+  // back the production behaviors.
+  ChromeNetworkDelegate::EnableAccessToAllFilesForTesting(false);

[mmenke, 2017/06/05 18:31:33]

It's possible some other module has already started a URLRequest at this point. 
So to avoid any races between setting the bool and other URLRequests made by
chrome (Which would be benign in terms of the test passing, but may still be
caught by TSAN), should move this into the test fixture:

void SetUp() override {
  InProcessBrowserTest::SetUp();
  // Must be done after the above call, which will set it to true, but also
before the browser is set up, to avoid any races)
  ChromeNetworkDelegate::EnableAccessToAllFilesForTesting(false);
}

[satorux1, 2017/06/06 08:52:37]

Good idea! However, I was surprised that your code didn't work because the test
function is called from InProcessBrowserTest::SetUp(). :) 

Hopefully this is documented in browser_test_base.h and the suggested solution
is to use SetUpInProcessBrowserTestFixture(). :)

https://cs.chromium.org/chromium/src/content/public/test/browser_test_base.h?...

+
+  base::FilePath test_directory;
+  ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &test_directory));
+  const base::FilePath test_file = test_directory.AppendASCII("empty.html");
+  const GURL url = net::FilePathToFileURL(test_file);

[mmenke, 2017/06/05 18:31:33]

nit:  convention is not to use const, except for build time constants and class
members (No idea why, it's just prevailing style).

[satorux1, 2017/06/06 08:52:37]

Done.

+
+  ui_test_utils::NavigateToURL(browser(), url);
+  EXPECT_TRUE(errorpage_test_util::IsDisplayingText(
+      browser(), net::ErrorToShortString(net::ERR_ACCESS_DENIED)));

[mmenke, 2017/06/05 18:31:33]

Rather than sharing errorpage_test_util::IsDisplayingText, could we just use a
NavigationObserver instead?  Could even been up TestNavigationObserver to grab
the net error code from the NavigationHandle (GetNetErrorCode()) in
OnDidFinishNavigation, so you could grab it there.

[satorux1, 2017/06/06 08:52:37]

This was a brilliant idea. Done!

+}
+
+#endif  // defined(OS_CHROMEOS)