[Downloads] Correctly test page transition when calculating danger level.

chrome/browser/download/download_target_determiner_unittest.cc

Index: chrome/browser/download/download_target_determiner_unittest.cc
diff --git a/chrome/browser/download/download_target_determiner_unittest.cc b/chrome/browser/download/download_target_determiner_unittest.cc
index f274298d172ca720916d7841a9dc3884085d43bd..45f576b5af6e28388e9816c9dcb506dc0ca0fc9d 100644
--- a/chrome/browser/download/download_target_determiner_unittest.cc
+++ b/chrome/browser/download/download_target_determiner_unittest.cc
@@ -1030,6 +1030,137 @@ TEST_F(DownloadTargetDeterminerTest, TargetDeterminer_VisitedReferrer) {
                              arraysize(kVisitedReferrerCases));
 }
 
+TEST_F(DownloadTargetDeterminerTest, TransitionType) {
+  const struct {
+    ui::PageTransition page_transition;
+    DownloadTestCase test_case;
+  } kTestCases[] = {
+      {
+          // Benign file type. Results in a danger type of NOT_DANGEROUS. Page
+          // transition type is irrelevant.
+          ui::PAGE_TRANSITION_LINK,

[Peter Kasting, 2016/06/29 23:35:01]

So many of these enums probably should be converted to enum classes
eventually...

[asanka, 2016/06/30 15:48:52]

Acknowledged.

+          {AUTOMATIC, content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS,
+           DownloadFileType::NOT_DANGEROUS, "http://example.com/foo.txt",
+           "text/plain", FILE_PATH_LITERAL(""),
+
+           FILE_PATH_LITERAL("foo.txt"),
+           DownloadItem::TARGET_DISPOSITION_OVERWRITE,
+
+           EXPECT_CRDOWNLOAD},
+      },

[Peter Kasting, 2016/06/29 23:35:01]

Nit: Maybe you should write a helper that constructs the desired
DownloadTestCase given the info that you actually change in each case, since so
much of it is repeated boilerplate.

This would also make the disparity I ask about in the next comment more
apparent.

[asanka, 2016/06/30 15:48:52]

Yeah. I rewrote the test cases so that the definition eliminates as much of the
boilerplate as possible.

+
+      {
+          // File type is ALLOW_ON_USER_GESTURE. PAGE_TRANSITION_LINK doesn't
+          // cause file to be marked as safe.
+          ui::PAGE_TRANSITION_LINK,
+          {AUTOMATIC, content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE,

[Peter Kasting, 2016/06/29 23:35:01]

Shouldn't this case and the next one be using
DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS?  DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE should
mean the file is unsafe even if FROM_ADDRESS_BAR is missing, no?

[asanka, 2016/06/30 15:48:52]

Danger type of the download == F( |danger level of the target file type|, |page
transition type|, |whether or not there was a user gesture|, |whether or not
there was a history entry for the origin| ...)

The danger type of a download is the conclusion we reach after considering the
danger level of the file type, whether there was a user gesture, whether there
was a history entry for the origin etc... In this case and the next the danger
level for the file type is "ALLOW_ON_USER_GESTURE". That is the only danger
level for which we factor in the page transition.

In the absence of FROM_ADDRESS_BAR, both cases result in the download being
marked DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE.

It's confusing since there are a few very closely related concepts being thrown
around here with overlapping names. As you mentioned I refactored the tests so
that it's clearer. But I think we should rename these concepts so that they are
easier to tell apart.

+           DownloadFileType::ALLOW_ON_USER_GESTURE,
+           "http://example.com/foo.crx", "application/octet-stream",
+           FILE_PATH_LITERAL(""),
+
+           FILE_PATH_LITERAL("foo.crx"),
+           DownloadItem::TARGET_DISPOSITION_OVERWRITE,
+
+           EXPECT_UNCONFIRMED},
+      },
+
+      {
+          // File type is ALLOW_ON_USER_GESTURE. PAGE_TRANSITION_TYPED doesn't
+          // cause file to be marked as safe. TYPED can be used for certain
+          // types of explicit page transitions that aren't necessarily
+          // initiated by a user. Hence a resulting download may not be
+          // intentional.
+          ui::PAGE_TRANSITION_TYPED,
+          {AUTOMATIC, content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE,
+           DownloadFileType::ALLOW_ON_USER_GESTURE,
+           "http://example.com/foo.crx", "application/octet-stream",
+           FILE_PATH_LITERAL(""),
+
+           FILE_PATH_LITERAL("foo.crx"),
+           DownloadItem::TARGET_DISPOSITION_OVERWRITE,
+
+           EXPECT_UNCONFIRMED},
+      },
+
+      {
+          // File type is ALLOW_ON_USER_GESTURE.
+          // PAGE_TRANSITION_FROM_ADDRESS_BAR causes file to be marked as safe.
+          static_cast<ui::PageTransition>(ui::PAGE_TRANSITION_TYPED |
+                                          ui::PAGE_TRANSITION_FROM_ADDRESS_BAR),
+          {AUTOMATIC, content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS,
+           DownloadFileType::NOT_DANGEROUS, "http://example.com/foo.crx",
+           "application/octet-stream", FILE_PATH_LITERAL(""),
+
+           FILE_PATH_LITERAL("foo.crx"),
+           DownloadItem::TARGET_DISPOSITION_OVERWRITE,
+
+           EXPECT_CRDOWNLOAD},
+      },
+
+      {
+          // File type is ALLOW_ON_USER_GESTURE.
+          // PAGE_TRANSITION_FROM_ADDRESS_BAR causes file to be marked as safe.
+          static_cast<ui::PageTransition>(ui::PAGE_TRANSITION_GENERATED |
+                                          ui::PAGE_TRANSITION_FROM_ADDRESS_BAR),
+          {AUTOMATIC, content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS,
+           DownloadFileType::NOT_DANGEROUS, "http://example.com/foo.crx",
+           "application/octet-stream", FILE_PATH_LITERAL(""),
+
+           FILE_PATH_LITERAL("foo.crx"),
+           DownloadItem::TARGET_DISPOSITION_OVERWRITE,
+
+           EXPECT_CRDOWNLOAD},
+      },
+
+      {
+          // File type is ALLOW_ON_USER_GESTURE.
+          // PAGE_TRANSITION_FROM_ADDRESS_BAR causes file to be marked as safe.
+          ui::PAGE_TRANSITION_FROM_ADDRESS_BAR,
+          {AUTOMATIC, content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS,
+           DownloadFileType::NOT_DANGEROUS, "http://example.com/foo.crx",
+           "application/octet-stream", FILE_PATH_LITERAL(""),
+
+           FILE_PATH_LITERAL("foo.crx"),
+           DownloadItem::TARGET_DISPOSITION_OVERWRITE,
+
+           EXPECT_CRDOWNLOAD},
+      },
+
+      {
+          // File type is DANGEROUS. PageTransition is irrelevant.
+          static_cast<ui::PageTransition>(ui::PAGE_TRANSITION_TYPED |
+                                          ui::PAGE_TRANSITION_FROM_ADDRESS_BAR),
+          {AUTOMATIC, content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE,
+           DownloadFileType::DANGEROUS, "http://example.com/foo.swf",
+           "application/octet-stream", FILE_PATH_LITERAL(""),
+
+           FILE_PATH_LITERAL("foo.swf"),
+           DownloadItem::TARGET_DISPOSITION_OVERWRITE,
+
+           EXPECT_UNCONFIRMED},
+      },
+  };
+
+  // Test assumptions:
+  ASSERT_EQ(DownloadFileType::ALLOW_ON_USER_GESTURE,
+            Policies()->GetFileDangerLevel(
+                base::FilePath(FILE_PATH_LITERAL("foo.crx"))));
+  ASSERT_EQ(DownloadFileType::DANGEROUS,
+            Policies()->GetFileDangerLevel(
+                base::FilePath(FILE_PATH_LITERAL("foo.swf"))));
+  ASSERT_EQ(DownloadFileType::NOT_DANGEROUS,
+            Policies()->GetFileDangerLevel(
+                base::FilePath(FILE_PATH_LITERAL("foo.txt"))));
+
+  for (const auto& test_case : kTestCases) {
+    std::unique_ptr<content::MockDownloadItem> item(
+        CreateActiveDownloadItem(1, test_case.test_case));
+    EXPECT_CALL(*item, GetTransitionType())
+        .WillRepeatedly(Return(test_case.page_transition));
+    RunTestCase(test_case.test_case, base::FilePath(), item.get());
+  }
+}
+
 // These test cases are run with "Prompt for download" user preference set to
 // true.
 TEST_F(DownloadTargetDeterminerTest, TargetDeterminer_PromptAlways) {