diff options
| -rw-r--r-- | chrome/browser/ui/login/login_prompt.cc | 21 | ||||
| -rw-r--r-- | chrome/browser/ui/login/login_prompt_browsertest.cc | 106 |
2 files changed, 121 insertions, 6 deletions
diff --git a/chrome/browser/ui/login/login_prompt.cc b/chrome/browser/ui/login/login_prompt.cc index edc52f7..3fce558 100644 --- a/chrome/browser/ui/login/login_prompt.cc +++ b/chrome/browser/ui/login/login_prompt.cc @@ -530,24 +530,33 @@ void LoginDialogCallback(const GURL& request_url, return; } - // Check if the request is cross origin. There are two different ways the - // navigation can occur: + // Check if this is a main frame navigation and + // (a) if the request is cross origin or + // (b) if an interstitial is already being shown. + // + // For (a), there are two different ways the navigation can occur: // 1- The user enters the resource URL in the omnibox. // 2- The page redirects to the resource. // In both cases, the last committed URL is different than the resource URL, // so checking it is sufficient. // Note that (1) will not be true once site isolation is enabled, as any // navigation could cause a cross-process swap, including link clicks. - if (is_main_frame && - parent_contents->GetLastCommittedURL().GetOrigin() != - request_url.GetOrigin()) { + // + // For (b), the login interstitial should always replace an existing + // interstitial. This is because |LoginHandler::CloseContentsDeferred| tries + // to proceed whatever interstitial is being shown when the login dialog is + // closed, so that interstitial should only be a login interstitial. + if (is_main_frame && (parent_contents->ShowingInterstitialPage() || + parent_contents->GetLastCommittedURL().GetOrigin() != + request_url.GetOrigin())) { // Show a blank interstitial for main-frame, cross origin requests // so that the correct URL is shown in the omnibox. base::Closure callback = base::Bind(&ShowLoginPrompt, request_url, make_scoped_refptr(auth_info), make_scoped_refptr(handler)); - // This is owned by the interstitial it creates. + // This is owned by the interstitial it creates. It cancels any existing + // interstitial. new LoginInterstitialDelegate(parent_contents, request_url, callback); diff --git a/chrome/browser/ui/login/login_prompt_browsertest.cc b/chrome/browser/ui/login/login_prompt_browsertest.cc index 47791cc..a284438 100644 --- a/chrome/browser/ui/login/login_prompt_browsertest.cc +++ b/chrome/browser/ui/login/login_prompt_browsertest.cc @@ -10,6 +10,7 @@ #include "base/strings/utf_string_conversions.h" #include "chrome/browser/chrome_notification_types.h" #include "chrome/browser/prerender/prerender_manager.h" +#include "chrome/browser/ssl/ssl_blocking_page.h" #include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/browser_commands.h" #include "chrome/browser/ui/login/login_interstitial_delegate.h" @@ -1212,6 +1213,10 @@ IN_PROC_BROWSER_TEST_F(LoginPromptBrowserTest, TestCrossOriginPrompt(test_page, auth_host); } +// Test the scenario where proceeding through a different type of interstitial +// that ends up with an auth URL works fine. This can happen if a URL that +// triggers the auth dialog can also trigger an SSL interstitial (or any other +// type of interstitial). IN_PROC_BROWSER_TEST_F( LoginPromptBrowserTest, DISABLED_LoginInterstitialShouldReplaceExistingInterstitial) { @@ -1242,6 +1247,9 @@ IN_PROC_BROWSER_TEST_F( ASSERT_EQ("127.0.0.1", contents->GetURL().host()); content::WaitForInterstitialAttach(contents); + EXPECT_EQ(SSLBlockingPage::kTypeForTesting, contents->GetInterstitialPage() + ->GetDelegateForTesting() + ->GetTypeForTesting()); // An overrideable SSL interstitial is now being displayed. Proceed through // the interstitial to see the login prompt. contents->GetInterstitialPage()->Proceed(); @@ -1269,4 +1277,102 @@ IN_PROC_BROWSER_TEST_F( } } +// Test the scenario where an auth interstitial should replace a different type +// of interstitial (e.g. SSL) even though the navigation isn't cross origin. +// This is different than the above scenario in that the last +// committed url is the same as the auth url. This can happen when: +// +// 1. Tab is navigated to the auth URL and the auth prompt is cancelled. +// 2. Tab is then navigated to an SSL interstitial. +// 3. Tab is again navigated to the same auth URL in (1). +// +// In this case, the last committed url is the same as the auth URL since the +// navigation at (1) is committed (user clicked cancel and the page loaded), but +// the navigation at (2) isn't (navigations ending up in interstitials don't +// immediately commit). So just checking for cross origin navigation before +// prompting the auth interstitial is not sufficient, must also check if there +// is any other interstitial being displayed. +IN_PROC_BROWSER_TEST_F(LoginPromptBrowserTest, + ShouldReplaceExistingInterstitialWhenNavigated) { + ASSERT_TRUE(test_server()->Start()); + net::SpawnedTestServer https_server( + net::SpawnedTestServer::TYPE_HTTPS, + net::SpawnedTestServer::SSLOptions( + net::SpawnedTestServer::SSLOptions::CERT_EXPIRED), + base::FilePath()); + ASSERT_TRUE(https_server.Start()); + + content::WebContents* contents = + browser()->tab_strip_model()->GetActiveWebContents(); + NavigationController* controller = &contents->GetController(); + LoginPromptBrowserTestObserver observer; + + observer.Register(content::Source<NavigationController>(controller)); + + GURL auth_url = test_server()->GetURL(kAuthBasicPage); + GURL broken_ssl_page = https_server.GetURL("/"); + + // Navigate to an auth url and wait for the login prompt. + { + WindowedAuthNeededObserver auth_needed_waiter(controller); + browser()->OpenURL(OpenURLParams(auth_url, Referrer(), CURRENT_TAB, + ui::PAGE_TRANSITION_TYPED, false)); + ASSERT_EQ("127.0.0.1", contents->GetURL().host()); + ASSERT_TRUE(contents->GetURL().SchemeIs("http")); + auth_needed_waiter.Wait(); + ASSERT_EQ(1u, observer.handlers().size()); + content::WaitForInterstitialAttach(contents); + ASSERT_TRUE(contents->ShowingInterstitialPage()); + EXPECT_EQ(LoginInterstitialDelegate::kTypeForTesting, + contents->GetInterstitialPage() + ->GetDelegateForTesting() + ->GetTypeForTesting()); + // Cancel the auth prompt. This commits the navigation. + LoginHandler* handler = *observer.handlers().begin(); + content::RunTaskAndWaitForInterstitialDetach( + contents, base::Bind(&LoginHandler::CancelAuth, handler)); + EXPECT_EQ("127.0.0.1", contents->GetVisibleURL().host()); + EXPECT_FALSE(contents->ShowingInterstitialPage()); + EXPECT_EQ(auth_url, contents->GetLastCommittedURL()); + } + + // Navigate to a broken SSL page. This is a cross origin navigation since + // schemes don't match (http vs https). + { + ASSERT_EQ("127.0.0.1", broken_ssl_page.host()); + browser()->OpenURL(OpenURLParams(broken_ssl_page, Referrer(), CURRENT_TAB, + ui::PAGE_TRANSITION_TYPED, false)); + ASSERT_EQ("127.0.0.1", contents->GetURL().host()); + ASSERT_TRUE(contents->GetURL().SchemeIs("https")); + content::WaitForInterstitialAttach(contents); + EXPECT_TRUE(contents->ShowingInterstitialPage()); + EXPECT_EQ(SSLBlockingPage::kTypeForTesting, contents->GetInterstitialPage() + ->GetDelegateForTesting() + ->GetTypeForTesting()); + EXPECT_EQ(auth_url, contents->GetLastCommittedURL()); + } + + // An overrideable SSL interstitial is now being displayed. Navigate to the + // auth URL again. This is again a cross origin navigation, but last committed + // URL is the same as the auth URL (since SSL navigation never committed). + // Should still replace SSL interstitial with an auth interstitial even though + // last committed URL and the new URL is the same. + { + WindowedAuthNeededObserver auth_needed_waiter(controller); + browser()->OpenURL(OpenURLParams(auth_url, Referrer(), CURRENT_TAB, + ui::PAGE_TRANSITION_TYPED, false)); + ASSERT_EQ("127.0.0.1", contents->GetURL().host()); + ASSERT_TRUE(contents->GetURL().SchemeIs("http")); + ASSERT_TRUE(contents->ShowingInterstitialPage()); + + auth_needed_waiter.Wait(); + ASSERT_EQ(1u, observer.handlers().size()); + content::WaitForInterstitialAttach(contents); + EXPECT_EQ(LoginInterstitialDelegate::kTypeForTesting, + contents->GetInterstitialPage() + ->GetDelegateForTesting() + ->GetTypeForTesting()); + } +} + } // namespace |