From 2249c80e8c4b50793a8261c4f9eb3f879ddf4aff Mon Sep 17 00:00:00 2001 From: lgarron Date: Fri, 7 Aug 2015 15:18:35 -0700 Subject: [PATCH] Switch media stream permissions to use IsOriginSecure() instead of SchemeIsSecure(). In particular, note that this means that media permissions are persisted on http://localhost. The change also updates the old media stream permission tests (which were no longer testing anything meaningful). BUG=295723, 362214, 509844 Review URL: https://codereview.chromium.org/1132203002 Cr-Commit-Position: refs/heads/master@{#342458} --- .../chrome_media_stream_infobar_browsertest.cc | 91 +++++++++++++++------ .../media/desktop_capture_access_handler.cc | 8 +- .../media/media_stream_device_permissions.cc | 7 +- .../media/media_stream_devices_controller.cc | 5 +- .../browser/media/media_stream_infobar_delegate.cc | 3 +- chrome/browser/media/webrtc_browsertest_base.cc | 94 +++++++++++++++++++++- chrome/browser/media/webrtc_browsertest_base.h | 7 ++ 7 files changed, 176 insertions(+), 39 deletions(-) diff --git a/chrome/browser/media/chrome_media_stream_infobar_browsertest.cc b/chrome/browser/media/chrome_media_stream_infobar_browsertest.cc index 395641f798ea..8f8dbed457af 100644 --- a/chrome/browser/media/chrome_media_stream_infobar_browsertest.cc +++ b/chrome/browser/media/chrome_media_stream_infobar_browsertest.cc @@ -22,15 +22,19 @@ #include "components/content_settings/core/common/content_settings_types.h" #include "content/public/browser/notification_service.h" #include "content/public/common/media_stream_request.h" +#include "content/public/common/origin_util.h" #include "content/public/test/browser_test_utils.h" #include "media/base/media_switches.h" +#include "net/dns/mock_host_resolver.h" #include "net/test/spawned_test_server/spawned_test_server.h" // MediaStreamPermissionTest --------------------------------------------------- class MediaStreamPermissionTest : public WebRtcTestBase { public: - MediaStreamPermissionTest() {} + // The default test server is localhost, which is considered secure: + // http://www.w3.org/TR/powerful-features/#is-origin-trustworthy + MediaStreamPermissionTest() : use_secure_origin_for_test_page_(true) {} ~MediaStreamPermissionTest() override {} // InProcessBrowserTest: @@ -67,11 +71,33 @@ class MediaStreamPermissionTest : public WebRtcTestBase { return result.compare("ok-stopped") == 0; } + void useNonSecureOriginForTestPage() { + use_secure_origin_for_test_page_ = false; + host_resolver()->AddRule("*", "127.0.0.1"); + } + private: + bool use_secure_origin_for_test_page_; + content::WebContents* LoadTestPageInBrowser(Browser* browser) { EXPECT_TRUE(test_server()->Start()); - ui_test_utils::NavigateToURL(browser, test_page_url()); + GURL url; + + if (use_secure_origin_for_test_page_) { + // Uses the default server. + url = test_page_url(); + } else { + static const char kFoo[] = "not-secure.example.com"; + GURL::Replacements replacements; + replacements.SetSchemeStr(url::kHttpScheme); + replacements.SetHostStr(kFoo); + url = test_page_url().ReplaceComponents(replacements); + } + + EXPECT_EQ(use_secure_origin_for_test_page_, content::IsOriginSecure(url)); + + ui_test_utils::NavigateToURL(browser, url); return browser->tab_strip_model()->GetActiveWebContents(); } @@ -107,7 +133,7 @@ IN_PROC_BROWSER_TEST_F(MediaStreamPermissionTest, } IN_PROC_BROWSER_TEST_F(MediaStreamPermissionTest, - TestAcceptThenDenyWhichShouldBeSticky) { + TestNonSecureOriginAcceptThenDenyIsSticky) { #if defined(OS_WIN) && defined(USE_ASH) // Disable this test in Metro+Ash for now (http://crbug.com/262796). if (base::CommandLine::ForCurrentProcess()->HasSwitch( @@ -115,32 +141,57 @@ IN_PROC_BROWSER_TEST_F(MediaStreamPermissionTest, return; #endif + useNonSecureOriginForTestPage(); content::WebContents* tab_contents = LoadTestPageInTab(); + EXPECT_FALSE(content::IsOriginSecure(tab_contents->GetLastCommittedURL())); EXPECT_TRUE(GetUserMediaAndAccept(tab_contents)); GetUserMediaAndDeny(tab_contents); - // Should fail with permission denied, instead of hanging. - GetUserMedia(tab_contents, kAudioVideoCallConstraints); - EXPECT_TRUE(test::PollingWaitUntil("obtainGetUserMediaResult()", - kFailedWithPermissionDeniedError, - tab_contents)); + GetUserMediaAndExpectAutoDenyWithoutPrompt(tab_contents); +} + +IN_PROC_BROWSER_TEST_F(MediaStreamPermissionTest, + TestNonSecureOriginDenyIsSticky) { + useNonSecureOriginForTestPage(); + content::WebContents* tab_contents = LoadTestPageInTab(); + EXPECT_FALSE(content::IsOriginSecure(tab_contents->GetLastCommittedURL())); + + GetUserMediaAndDeny(tab_contents); + GetUserMediaAndExpectAutoDenyWithoutPrompt(tab_contents); +} + +IN_PROC_BROWSER_TEST_F(MediaStreamPermissionTest, + TestSecureOriginDenyIsSticky) { + content::WebContents* tab_contents = LoadTestPageInTab(); + EXPECT_TRUE(content::IsOriginSecure(tab_contents->GetLastCommittedURL())); + + GetUserMediaAndDeny(tab_contents); + GetUserMediaAndExpectAutoDenyWithoutPrompt(tab_contents); } -IN_PROC_BROWSER_TEST_F(MediaStreamPermissionTest, TestAcceptIsNotSticky) { +IN_PROC_BROWSER_TEST_F(MediaStreamPermissionTest, + TestNonSecureOriginAcceptIsNotSticky) { + useNonSecureOriginForTestPage(); content::WebContents* tab_contents = LoadTestPageInTab(); + EXPECT_FALSE(content::IsOriginSecure(tab_contents->GetLastCommittedURL())); - // If accept were sticky the second call would hang because it hangs if a - // bubble does not pop up. EXPECT_TRUE(GetUserMediaAndAccept(tab_contents)); EXPECT_TRUE(GetUserMediaAndAccept(tab_contents)); } +IN_PROC_BROWSER_TEST_F(MediaStreamPermissionTest, + TestSecureOriginAcceptIsSticky) { + content::WebContents* tab_contents = LoadTestPageInTab(); + EXPECT_TRUE(content::IsOriginSecure(tab_contents->GetLastCommittedURL())); + + EXPECT_TRUE(GetUserMediaAndAccept(tab_contents)); + GetUserMediaAndExpectAutoAcceptWithoutPrompt(tab_contents); +} + IN_PROC_BROWSER_TEST_F(MediaStreamPermissionTest, TestDismissIsNotSticky) { content::WebContents* tab_contents = LoadTestPageInTab(); - // If dismiss were sticky the second call would hang because it hangs if a - // bubble does not pop up. GetUserMediaAndDismiss(tab_contents); GetUserMediaAndDismiss(tab_contents); } @@ -150,6 +201,7 @@ IN_PROC_BROWSER_TEST_F(MediaStreamPermissionTest, content::WebContents* tab_contents = LoadTestPageInTab(); GetUserMediaAndDeny(tab_contents); + GetUserMediaAndExpectAutoDenyWithoutPrompt(tab_contents); HostContentSettingsMap* settings_map = browser()->profile()->GetHostContentSettingsMap(); @@ -158,23 +210,13 @@ IN_PROC_BROWSER_TEST_F(MediaStreamPermissionTest, settings_map->ClearSettingsForOneType( CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA); - // If a bubble is not launched now, this will hang. GetUserMediaAndDeny(tab_contents); } -// Times out on win debug builds; http://crbug.com/295723 . -#if defined(OS_WIN) && !defined(NDEBUG) -#define MAYBE_DenyingMicDoesNotCauseStickyDenyForCameras \ - DISABLED_DenyingMicDoesNotCauseStickyDenyForCameras -#else -#define MAYBE_DenyingMicDoesNotCauseStickyDenyForCameras \ - DenyingMicDoesNotCauseStickyDenyForCameras -#endif IN_PROC_BROWSER_TEST_F(MediaStreamPermissionTest, - MAYBE_DenyingMicDoesNotCauseStickyDenyForCameras) { + DenyingMicDoesNotCauseStickyDenyForCameras) { content::WebContents* tab_contents = LoadTestPageInTab(); - // If mic blocking also blocked cameras, the second call here would hang. GetUserMediaWithSpecificConstraintsAndDeny(tab_contents, kAudioOnlyCallConstraints); EXPECT_TRUE(GetUserMediaWithSpecificConstraintsAndAccept( @@ -185,7 +227,6 @@ IN_PROC_BROWSER_TEST_F(MediaStreamPermissionTest, DenyingCameraDoesNotCauseStickyDenyForMics) { content::WebContents* tab_contents = LoadTestPageInTab(); - // If camera blocking also blocked mics, the second call here would hang. GetUserMediaWithSpecificConstraintsAndDeny(tab_contents, kVideoOnlyCallConstraints); EXPECT_TRUE(GetUserMediaWithSpecificConstraintsAndAccept( diff --git a/chrome/browser/media/desktop_capture_access_handler.cc b/chrome/browser/media/desktop_capture_access_handler.cc index 86785ae5c840..25bfa5f51137 100644 --- a/chrome/browser/media/desktop_capture_access_handler.cc +++ b/chrome/browser/media/desktop_capture_access_handler.cc @@ -22,6 +22,7 @@ #include "content/public/browser/render_frame_host.h" #include "content/public/browser/render_process_host.h" #include "content/public/common/media_stream_request.h" +#include "content/public/common/origin_util.h" #include "extensions/browser/app_window/app_window.h" #include "extensions/browser/app_window/app_window_registry.h" #include "extensions/common/constants.h" @@ -77,8 +78,8 @@ base::string16 GetApplicationTitle(content::WebContents* web_contents, return base::UTF8ToUTF16(title); } GURL url = web_contents->GetURL(); - title = url.SchemeIsSecure() ? net::GetHostAndOptionalPort(url) - : url.GetOrigin().spec(); + title = content::IsOriginSecure(url) ? net::GetHostAndOptionalPort(url) + : url.GetOrigin().spec(); return base::UTF8ToUTF16(title); } @@ -179,8 +180,7 @@ void DesktopCaptureAccessHandler::ProcessScreenCaptureAccessRequest( IsBuiltInExtension(request.security_origin); const bool origin_is_secure = - request.security_origin.SchemeIsSecure() || - request.security_origin.SchemeIs(extensions::kExtensionScheme) || + content::IsOriginSecure(request.security_origin) || base::CommandLine::ForCurrentProcess()->HasSwitch( switches::kAllowHttpScreenCapture); diff --git a/chrome/browser/media/media_stream_device_permissions.cc b/chrome/browser/media/media_stream_device_permissions.cc index a2c7c83ce0f7..8414bde8c604 100644 --- a/chrome/browser/media/media_stream_device_permissions.cc +++ b/chrome/browser/media/media_stream_device_permissions.cc @@ -12,6 +12,7 @@ #include "components/content_settings/core/browser/host_content_settings_map.h" #include "components/content_settings/core/common/content_settings_pattern.h" #include "content/public/browser/browser_thread.h" +#include "content/public/common/origin_util.h" #include "extensions/common/constants.h" #include "url/gurl.h" @@ -53,11 +54,7 @@ bool ShouldPersistContentSetting(ContentSetting setting, return true; // We persist requests from secure origins. - if (origin.SchemeIsSecure()) - return true; - - // We persist requests from extensions. - if (origin.SchemeIs(extensions::kExtensionScheme)) + if (content::IsOriginSecure(origin)) return true; return false; diff --git a/chrome/browser/media/media_stream_devices_controller.cc b/chrome/browser/media/media_stream_devices_controller.cc index 6da7f54a920f..f305de8eb564 100644 --- a/chrome/browser/media/media_stream_devices_controller.cc +++ b/chrome/browser/media/media_stream_devices_controller.cc @@ -24,6 +24,8 @@ #include "content/public/browser/browser_thread.h" #include "content/public/browser/render_widget_host_view.h" #include "content/public/common/media_stream_request.h" +#include "content/public/common/origin_util.h" +#include "extensions/common/constants.h" #include "grit/theme_resources.h" #include "ui/base/l10n/l10n_util.h" @@ -154,7 +156,7 @@ GURL MediaStreamDevicesController::GetRequestingHostname() const { void MediaStreamDevicesController::PermissionGranted() { GURL origin(GetSecurityOriginSpec()); - if (origin.SchemeIsSecure()) { + if (content::IsOriginSecure(origin)) { UMA_HISTOGRAM_ENUMERATION("Media.DevicePermissionActions", kAllowHttps, kPermissionActionsMax); } else { @@ -334,7 +336,6 @@ void MediaStreamDevicesController::StorePermission( ContentSetting new_audio_setting, ContentSetting new_video_setting) const { DCHECK_CURRENTLY_ON(BrowserThread::UI); - ContentSettingsPattern primary_pattern = ContentSettingsPattern::FromURLNoWildcard(request_.security_origin); diff --git a/chrome/browser/media/media_stream_infobar_delegate.cc b/chrome/browser/media/media_stream_infobar_delegate.cc index 46b95641d749..9f22e0726fce 100644 --- a/chrome/browser/media/media_stream_infobar_delegate.cc +++ b/chrome/browser/media/media_stream_infobar_delegate.cc @@ -13,6 +13,7 @@ #include "components/google/core/browser/google_util.h" #include "components/infobars/core/infobar.h" #include "content/public/browser/web_contents.h" +#include "content/public/common/origin_util.h" #include "grit/components_strings.h" #include "grit/theme_resources.h" #include "ui/base/l10n/l10n_util.h" @@ -123,7 +124,7 @@ base::string16 MediaStreamInfoBarDelegate::GetButtonLabel( bool MediaStreamInfoBarDelegate::Accept() { GURL origin(controller_->GetSecurityOriginSpec()); - if (origin.SchemeIsSecure()) { + if (content::IsOriginSecure(origin)) { UMA_HISTOGRAM_ENUMERATION("Media.DevicePermissionActions", kAllowHttps, kPermissionActionsMax); } else { diff --git a/chrome/browser/media/webrtc_browsertest_base.cc b/chrome/browser/media/webrtc_browsertest_base.cc index 52707ebe04de..52b55dcd502d 100644 --- a/chrome/browser/media/webrtc_browsertest_base.cc +++ b/chrome/browser/media/webrtc_browsertest_base.cc @@ -78,6 +78,41 @@ bool JavascriptErrorDetectingLogHandler(int severity, return false; } +// PermissionRequestObserver --------------------------------------------------- + +// Used to observe the creation of permission prompt without responding. +class PermissionRequestObserver : public PermissionBubbleManager::Observer { + public: + explicit PermissionRequestObserver(content::WebContents* web_contents) + : bubble_manager_(PermissionBubbleManager::FromWebContents(web_contents)), + request_shown_(false), + message_loop_runner_(new content::MessageLoopRunner) { + bubble_manager_->AddObserver(this); + } + ~PermissionRequestObserver() override { + // Safe to remove twice if it happens. + bubble_manager_->RemoveObserver(this); + } + + void Wait() { message_loop_runner_->Run(); } + + bool request_shown() { return request_shown_; } + + private: + // PermissionBubbleManager::Observer + void OnBubbleAdded() override { + request_shown_ = true; + bubble_manager_->RemoveObserver(this); + message_loop_runner_->Quit(); + } + + PermissionBubbleManager* bubble_manager_; + bool request_shown_; + scoped_refptr message_loop_runner_; + + DISALLOW_COPY_AND_ASSIGN(PermissionRequestObserver); +}; + } // namespace WebRtcTestBase::WebRtcTestBase(): detect_errors_in_javascript_(false) { @@ -109,7 +144,9 @@ bool WebRtcTestBase::GetUserMediaWithSpecificConstraintsAndAccept( std::string result; PermissionBubbleManager::FromWebContents(tab_contents) ->set_auto_response_for_test(PermissionBubbleManager::ACCEPT_ALL); + PermissionRequestObserver permissionRequestObserver(tab_contents); GetUserMedia(tab_contents, constraints); + EXPECT_TRUE(permissionRequestObserver.request_shown()); EXPECT_TRUE(content::ExecuteScriptAndExtractString( tab_contents->GetMainFrame(), "obtainGetUserMediaResult();", &result)); return kOkGotStream == result; @@ -126,7 +163,9 @@ void WebRtcTestBase::GetUserMediaWithSpecificConstraintsAndDeny( std::string result; PermissionBubbleManager::FromWebContents(tab_contents) ->set_auto_response_for_test(PermissionBubbleManager::DENY_ALL); + PermissionRequestObserver permissionRequestObserver(tab_contents); GetUserMedia(tab_contents, constraints); + EXPECT_TRUE(permissionRequestObserver.request_shown()); EXPECT_TRUE(content::ExecuteScriptAndExtractString( tab_contents->GetMainFrame(), "obtainGetUserMediaResult();", &result)); EXPECT_EQ(kFailedWithPermissionDeniedError, result); @@ -137,13 +176,57 @@ void WebRtcTestBase::GetUserMediaAndDismiss( std::string result; PermissionBubbleManager::FromWebContents(tab_contents) ->set_auto_response_for_test(PermissionBubbleManager::DISMISS); + PermissionRequestObserver permissionRequestObserver(tab_contents); GetUserMedia(tab_contents, kAudioVideoCallConstraints); + EXPECT_TRUE(permissionRequestObserver.request_shown()); // A dismiss should be treated like a deny. EXPECT_TRUE(content::ExecuteScriptAndExtractString( tab_contents->GetMainFrame(), "obtainGetUserMediaResult();", &result)); EXPECT_EQ(kFailedWithPermissionDismissedError, result); } +void WebRtcTestBase::GetUserMediaAndExpectAutoAcceptWithoutPrompt( + content::WebContents* tab_contents) const { + std::string result; + // We issue a GetUserMedia() request. We expect that the origin already has a + // sticky "accept" permission (e.g. because the caller previously called + // GetUserMediaAndAccept()), and therefore the GetUserMedia() request + // automatically succeeds without a prompt. + // If the caller made a mistake, a prompt may show up instead. For this case, + // we set an auto-response to avoid leaving the prompt hanging. The choice of + // DENY_ALL makes sure that the response to the prompt doesn't accidentally + // result in a newly granted media stream permission. + PermissionBubbleManager::FromWebContents(tab_contents) + ->set_auto_response_for_test(PermissionBubbleManager::DENY_ALL); + PermissionRequestObserver permissionRequestObserver(tab_contents); + GetUserMedia(tab_contents, kAudioVideoCallConstraints); + EXPECT_FALSE(permissionRequestObserver.request_shown()); + EXPECT_TRUE(content::ExecuteScriptAndExtractString( + tab_contents->GetMainFrame(), "obtainGetUserMediaResult();", &result)); + EXPECT_EQ(kOkGotStream, result); +} + +void WebRtcTestBase::GetUserMediaAndExpectAutoDenyWithoutPrompt( + content::WebContents* tab_contents) const { + std::string result; + // We issue a GetUserMedia() request. We expect that the origin already has a + // sticky "deny" permission (e.g. because the caller previously called + // GetUserMediaAndDeny()), and therefore the GetUserMedia() request + // automatically succeeds without a prompt. + // If the caller made a mistake, a prompt may show up instead. For this case, + // we set an auto-response to avoid leaving the prompt hanging. The choice of + // ACCEPT_ALL makes sure that the response to the prompt doesn't accidentally + // result in a newly granted media stream permission. + PermissionBubbleManager::FromWebContents(tab_contents) + ->set_auto_response_for_test(PermissionBubbleManager::ACCEPT_ALL); + PermissionRequestObserver permissionRequestObserver(tab_contents); + GetUserMedia(tab_contents, kAudioVideoCallConstraints); + EXPECT_FALSE(permissionRequestObserver.request_shown()); + EXPECT_TRUE(content::ExecuteScriptAndExtractString( + tab_contents->GetMainFrame(), "obtainGetUserMediaResult();", &result)); + EXPECT_EQ(kFailedWithPermissionDeniedError, result); +} + void WebRtcTestBase::GetUserMedia(content::WebContents* tab_contents, const std::string& constraints) const { // Request user media: this will launch the media stream info bar or bubble. @@ -168,8 +251,15 @@ WebRtcTestBase::OpenPageAndGetUserMediaInNewTabWithConstraints( ui_test_utils::NavigateToURL(browser(), url); content::WebContents* new_tab = browser()->tab_strip_model()->GetActiveWebContents(); - EXPECT_TRUE(GetUserMediaWithSpecificConstraintsAndAccept( - new_tab, constraints)); + // Accept if necessary, but don't expect a prompt (because auto-accept is also + // okay). + PermissionBubbleManager::FromWebContents(new_tab) + ->set_auto_response_for_test(PermissionBubbleManager::ACCEPT_ALL); + GetUserMedia(new_tab, constraints); + std::string result; + EXPECT_TRUE(content::ExecuteScriptAndExtractString( + new_tab->GetMainFrame(), "obtainGetUserMediaResult();", &result)); + EXPECT_EQ(kOkGotStream, result); return new_tab; } diff --git a/chrome/browser/media/webrtc_browsertest_base.h b/chrome/browser/media/webrtc_browsertest_base.h index 9cc092923d70..600a75d659c1 100644 --- a/chrome/browser/media/webrtc_browsertest_base.h +++ b/chrome/browser/media/webrtc_browsertest_base.h @@ -46,6 +46,9 @@ class WebRtcTestBase : public InProcessBrowserTest { // chrome/test/data/webrtc/getusermedia.js. // If an error is reported back from the getUserMedia call, these functions // will return false. + // The ...AndAccept()/...AndDeny()/...AndDismiss() functions expect that a + // prompt will be shown (i.e. the current origin in the tab_contents doesn't + // have a saved permission). bool GetUserMediaAndAccept(content::WebContents* tab_contents) const; bool GetUserMediaWithSpecificConstraintsAndAccept( content::WebContents* tab_contents, @@ -55,6 +58,10 @@ class WebRtcTestBase : public InProcessBrowserTest { content::WebContents* tab_contents, const std::string& constraints) const; void GetUserMediaAndDismiss(content::WebContents* tab_contents) const; + void GetUserMediaAndExpectAutoAcceptWithoutPrompt( + content::WebContents* tab_contents) const; + void GetUserMediaAndExpectAutoDenyWithoutPrompt( + content::WebContents* tab_contents) const; void GetUserMedia(content::WebContents* tab_contents, const std::string& constraints) const; -- 2.11.4.GIT