Always allow one download from an origin even for blocked setting

The content setting is confusing as it is "allow multiple downloads".
So in case an origin is blocked, we should still allow one download.
This CL also fixes an issue that on user gesture, download status
from other origins are reset although it should only reset the
status on the current page.

BUG=981966

Change-Id: I0519aa091446f2a2d81f4dcf074abf34849b9fe3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1693552Reviewed-by: Xing Liu <xingliu@chromium.org>
Commit-Queue: Min Qin <qinmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#676121}

chrome/browser/download/download_request_limiter.cc

@@ -310,17 +310,30 @@ bool DownloadRequestLimiter::TabDownloadState::is_showing_prompt() const {
 }
 
 void DownloadRequestLimiter::TabDownloadState::OnUserInteraction() {
-  bool promptable =
-      PermissionRequestManager::FromWebContents(web_contents()) != nullptr;
-
   // See PromptUserForDownload(): if there's no PermissionRequestManager, then
   // DOWNLOADS_NOT_ALLOWED is functionally equivalent to PROMPT_BEFORE_DOWNLOAD.
-  if ((status_ != DownloadRequestLimiter::ALLOW_ALL_DOWNLOADS) &&
-      (!promptable ||
-       (status_ != DownloadRequestLimiter::DOWNLOADS_NOT_ALLOWED))) {
-    // Revert to default status.
-    host_->Remove(this, web_contents());
-    // WARNING: We've been deleted.
+  bool need_prompt =
+      (PermissionRequestManager::FromWebContents(web_contents()) == nullptr &&
+       status_ == DOWNLOADS_NOT_ALLOWED) ||
+      status_ == PROMPT_BEFORE_DOWNLOAD;
+
+  // If content setting blocks automatic downloads, don't reset the
+  // PROMPT_BEFORE_DOWNLOAD status for the current page because doing
+  // that will default the download status to ALLOW_ONE_DOWNLOAD. That
+  // will allow an extra download when CanDownloadImpl() is called.
+  ContentSetting setting = GetAutoDownloadContentSetting(
+      web_contents(), web_contents()->GetVisibleURL());
+  if (status_ == ALLOW_ONE_DOWNLOAD ||
+      (need_prompt && setting != CONTENT_SETTING_BLOCK)) {
+    GURL origin = web_contents()->GetVisibleURL().GetOrigin();
+
+    // Revert to default status and notify if needed.
+    if (!origin.is_empty())
+      download_status_map_.erase(origin);
+    if (download_status_map_.empty()) {
+      host_->Remove(this, web_contents());
+      // WARNING: We've been deleted.
+    }
   }
 }
 
@@ -553,6 +566,19 @@ HostContentSettingsMap* DownloadRequestLimiter::GetContentSettings(
       Profile::FromBrowserContext(contents->GetBrowserContext()));
 }
 
+ContentSetting DownloadRequestLimiter::GetAutoDownloadContentSetting(
+    content::WebContents* contents,
+    const GURL& request_initiator) {
+  HostContentSettingsMap* content_settings = GetContentSettings(contents);
+  ContentSetting setting = CONTENT_SETTING_ASK;
+  if (content_settings) {
+    setting = content_settings->GetContentSetting(
+        request_initiator, request_initiator,
+        CONTENT_SETTINGS_TYPE_AUTOMATIC_DOWNLOADS, std::string());
+  }
+  return setting;
+}
+
 void DownloadRequestLimiter::CanDownloadImpl(
     content::WebContents* originating_contents,
     const std::string& request_method,
@@ -574,22 +600,18 @@ void DownloadRequestLimiter::CanDownloadImpl(
   // Always check for the content setting first. Having an content setting
   // observer won't work as |request_initiator| might be different from the tab
   // URL.
-  HostContentSettingsMap* content_settings =
-      GetContentSettings(originating_contents);
-  ContentSetting setting = CONTENT_SETTING_ASK;
-  if (content_settings) {
-    setting = content_settings->GetContentSetting(
-        initiator, initiator, CONTENT_SETTINGS_TYPE_AUTOMATIC_DOWNLOADS,
-        std::string());
-    // Override the status if content setting is block. If the content setting
-    // is always allow, only reset the status if it is |DOWNLOADS_NOT_ALLOWED|
-    // so unnecessary notifications will not be triggered.
-    if (setting == CONTENT_SETTING_BLOCK) {
-      status = DOWNLOADS_NOT_ALLOWED;
-    } else if (setting == CONTENT_SETTING_ALLOW &&
-               status == DOWNLOADS_NOT_ALLOWED) {
-      status = ALLOW_ALL_DOWNLOADS;
-    }
+  ContentSetting setting =
+      GetAutoDownloadContentSetting(originating_contents, initiator);
+  // Override the status if content setting is block or allow. If the content
+  // setting is always allow, only reset the status if it is
+  // DOWNLOADS_NOT_ALLOWED so unnecessary notifications will not be triggered.
+  // If the content setting is block, allow only one download to proceed if the
+  // current status is ALLOW_ALL_DOWNLOADS.
+  if (setting == CONTENT_SETTING_BLOCK && status == ALLOW_ALL_DOWNLOADS) {
+    status = ALLOW_ONE_DOWNLOAD;
+  } else if (setting == CONTENT_SETTING_ALLOW &&
+             status == DOWNLOADS_NOT_ALLOWED) {
+    status = ALLOW_ALL_DOWNLOADS;
   }
 
   // Always call SetDownloadStatusAndNotify since we may need to change the

chrome/browser/download/download_request_limiter.h

@@ -298,6 +298,11 @@ class DownloadRequestLimiter
   static HostContentSettingsMap* GetContentSettings(
       content::WebContents* contents);
 
+  // Gets the content setting for a particular request initiator.
+  static ContentSetting GetAutoDownloadContentSetting(
+      content::WebContents* contents,
+      const GURL& request_initiator);
+
   // Sets the callback for tests to know the result of OnCanDownloadDecided().
   using CanDownloadDecidedCallback =
       base::RepeatingCallback<void(bool /*allow*/)>;

chrome/browser/download/download_request_limiter_unittest.cc

@@ -911,10 +911,10 @@ TEST_F(DownloadRequestLimiterTest, RendererInitiatedDownloadFromAnotherOrigin) {
   // Trigger a renderer initiated download from the other origin.
   CanDownloadFor(web_contents(),
                  url::Origin::Create(GURL("http://foobar.com")));
-  ExpectAndResetCounts(0, 1, 0, __LINE__);
-  EXPECT_EQ(DownloadRequestLimiter::DOWNLOADS_NOT_ALLOWED,
+  ExpectAndResetCounts(1, 0, 0, __LINE__);
+  EXPECT_EQ(DownloadRequestLimiter::PROMPT_BEFORE_DOWNLOAD,
             download_request_limiter_->GetDownloadStatus(web_contents()));
-  EXPECT_EQ(DownloadRequestLimiter::DOWNLOAD_UI_BLOCKED,
+  EXPECT_EQ(DownloadRequestLimiter::DOWNLOAD_UI_DEFAULT,
             download_request_limiter_->GetDownloadUiStatus(web_contents()));
 
   // The current tab is not affected, still allowing one download.
@@ -945,7 +945,7 @@ TEST_F(DownloadRequestLimiterTest, RendererInitiatedDownloadFromAnotherOrigin) {
   EXPECT_EQ(DownloadRequestLimiter::DOWNLOAD_UI_BLOCKED,
             download_request_limiter_->GetDownloadUiStatus(web_contents()));
 
-  // Download should proceed work for the other origin.
+  // Download should proceed for the other origin.
   CanDownloadFor(web_contents(),
                  url::Origin::Create(GURL("http://foobar.com")));
   ExpectAndResetCounts(1, 0, 0, __LINE__);
@@ -954,3 +954,47 @@ TEST_F(DownloadRequestLimiterTest, RendererInitiatedDownloadFromAnotherOrigin) {
   EXPECT_EQ(DownloadRequestLimiter::DOWNLOAD_UI_ALLOWED,
             download_request_limiter_->GetDownloadUiStatus(web_contents()));
 }
+
+// Test that user interaction on the current page won't reset download status
+// for another origin.
+TEST_F(DownloadRequestLimiterTest,
+       DownloadStatusForOtherOriginsNotResetOnUserInteraction) {
+  NavigateAndCommit(GURL("http://foo.com/bar"));
+  LoadCompleted();
+
+  // Trigger a renderer initiated download from the other origin.
+  CanDownloadFor(web_contents(),
+                 url::Origin::Create(GURL("http://foobar.com")));
+  ExpectAndResetCounts(1, 0, 0, __LINE__);
+  EXPECT_EQ(DownloadRequestLimiter::PROMPT_BEFORE_DOWNLOAD,
+            download_request_limiter_->GetDownloadStatus(web_contents()));
+  EXPECT_EQ(DownloadRequestLimiter::DOWNLOAD_UI_DEFAULT,
+            download_request_limiter_->GetDownloadUiStatus(web_contents()));
+
+  // The current tab is not affected, still allowing one download.
+  CanDownloadFor(web_contents());
+  ExpectAndResetCounts(1, 0, 0, __LINE__);
+  EXPECT_EQ(DownloadRequestLimiter::PROMPT_BEFORE_DOWNLOAD,
+            download_request_limiter_->GetDownloadStatus(web_contents()));
+  EXPECT_EQ(DownloadRequestLimiter::DOWNLOAD_UI_DEFAULT,
+            download_request_limiter_->GetDownloadUiStatus(web_contents()));
+
+  // On user interaction, the current page should reset its download status.
+  OnUserInteraction(blink::WebInputEvent::kTouchStart);
+  CanDownloadFor(web_contents());
+  ExpectAndResetCounts(1, 0, 0, __LINE__);
+  EXPECT_EQ(DownloadRequestLimiter::PROMPT_BEFORE_DOWNLOAD,
+            download_request_limiter_->GetDownloadStatus(web_contents()));
+  EXPECT_EQ(DownloadRequestLimiter::DOWNLOAD_UI_DEFAULT,
+            download_request_limiter_->GetDownloadUiStatus(web_contents()));
+
+  // Download status from the other origin does not reset and should prompt.
+  UpdateExpectations(CANCEL);
+  CanDownloadFor(web_contents(),
+                 url::Origin::Create(GURL("http://foobar.com")));
+  ExpectAndResetCounts(0, 1, 1, __LINE__);
+  EXPECT_EQ(DownloadRequestLimiter::DOWNLOADS_NOT_ALLOWED,
+            download_request_limiter_->GetDownloadStatus(web_contents()));
+  EXPECT_EQ(DownloadRequestLimiter::DOWNLOAD_UI_BLOCKED,
+            download_request_limiter_->GetDownloadUiStatus(web_contents()));
+}