Use correct origin when prompting for proxy authentication.

chrome/browser/ui/login/login_handler.cc

Index: chrome/browser/ui/login/login_handler.cc
diff --git a/chrome/browser/ui/login/login_handler.cc b/chrome/browser/ui/login/login_handler.cc
index 7c23cc3c91a5ecd9a8e665ba3ab743b4229e00df..01fde2b7667b7ca77c5c7ae5c5b0a2160cfb2fe4 100644
--- a/chrome/browser/ui/login/login_handler.cc
+++ b/chrome/browser/ui/login/login_handler.cc
@@ -32,6 +32,7 @@
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/origin_util.h"
 #include "net/base/auth.h"
+#include "net/base/host_port_pair.h"
 #include "net/base/load_flags.h"
 #include "net/http/http_auth_scheme.h"
 #include "net/http/http_transaction_factory.h"
@@ -65,95 +66,6 @@ void ResetLoginHandlerForRequest(net::URLRequest* request) {
   ResourceDispatcherHost::Get()->ClearLoginDelegateForRequest(request);
 }
 
-// Helper to create a PasswordForm for PasswordManager to start looking for
-// saved credentials.
-PasswordForm MakeInputForPasswordManager(const GURL& request_url,
-                                         net::AuthChallengeInfo* auth_info) {
-  PasswordForm dialog_form;
-  if (base::LowerCaseEqualsASCII(auth_info->scheme, net::kBasicAuthScheme)) {
-    dialog_form.scheme = PasswordForm::SCHEME_BASIC;
-  } else if (base::LowerCaseEqualsASCII(auth_info->scheme,
-                                        net::kDigestAuthScheme)) {
-    dialog_form.scheme = PasswordForm::SCHEME_DIGEST;
-  } else {
-    dialog_form.scheme = PasswordForm::SCHEME_OTHER;
-  }
-  std::string host_and_port(auth_info->challenger.ToString());
-  if (auth_info->is_proxy) {
-    std::string origin = host_and_port;
-    // We don't expect this to already start with http:// or https://.
-    DCHECK(origin.find("http://") != 0 && origin.find("https://") != 0);
-    origin = std::string("http://") + origin;

[asanka, 2016/06/14 18:50:48]

vabr: This logic doesn't distinguish between http and https proxies. I.e. a
password stored for an https proxy will be automatically provided to a http
proxy. In the new logic, we are passing up the origin of the proxy server up to
this layer. So we are using the correct origin here when populating
dialog_form.origin.

This will cause existing stored password entries for https proxies to become
orphaned and users will be prompted again after this change. I think this is OK
given that we really don't want to share passwords between secure and insecure
proxies. WDYT?

[vabr (Chromium), 2016/06/15 08:50:06]

Thanks for the heads-up. I agree with your arguments. If we see a surge of bug
reports we will be able to direct users to chrome://passwords/settings to look
up the old passwords.

-    dialog_form.origin = GURL(origin);
-  } else if (!auth_info->challenger.Equals(
-                 net::HostPortPair::FromURL(request_url))) {
-    dialog_form.origin = GURL();
-    NOTREACHED();  // crbug.com/32718
-  } else {
-    dialog_form.origin = GURL(request_url.scheme() + "://" + host_and_port);

[asanka, 2016/06/14 18:50:48]

vabr: Note that host_and_port will always contain a port even if the port is
standard. Hence dialog_form.origin = GURL("http://example.com:80") when
authenticating against "http://example.com".

After this CL, dialog_form.origin would be GURL("http://example.com"). Since
GURL canonicalizes the URL there should be no observable change in behavior
before and after this change.

[vabr (Chromium), 2016/06/15 08:50:06]

Acknowledged.

-  }
-  dialog_form.signon_realm = GetSignonRealm(dialog_form.origin, *auth_info);
-  return dialog_form;
-}
-
-void ShowLoginPrompt(const GURL& request_url,
-                     net::AuthChallengeInfo* auth_info,
-                     LoginHandler* handler) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  WebContents* parent_contents = handler->GetWebContentsForLogin();
-  if (!parent_contents)
-    return;
-  prerender::PrerenderContents* prerender_contents =
-      prerender::PrerenderContents::FromWebContents(parent_contents);
-  if (prerender_contents) {
-    prerender_contents->Destroy(prerender::FINAL_STATUS_AUTH_NEEDED);
-    return;
-  }
-
-  base::string16 authority = l10n_util::GetStringFUTF16(
-      auth_info->is_proxy ? IDS_LOGIN_DIALOG_PROXY_AUTHORITY
-                          : IDS_LOGIN_DIALOG_AUTHORITY,
-      url_formatter::FormatUrlForSecurityDisplay(request_url));
-  base::string16 explanation;
-  if (!content::IsOriginSecure(request_url)) {
-    explanation =
-        l10n_util::GetStringUTF16(IDS_WEBSITE_SETTINGS_NON_SECURE_TRANSPORT);
-  }
-
-  password_manager::PasswordManager* password_manager =
-      handler->GetPasswordManagerForLogin();
-
-  if (!password_manager) {
-#if defined(ENABLE_EXTENSIONS)
-    // A WebContents in a <webview> (a GuestView type) does not have a password
-    // manager, but still needs to be able to show login prompts.
-    const auto* guest =
-        guest_view::GuestViewBase::FromWebContents(parent_contents);
-    if (guest &&
-        extensions::GetViewType(guest->owner_web_contents()) !=
-            extensions::VIEW_TYPE_EXTENSION_BACKGROUND_PAGE) {
-      handler->BuildViewWithoutPasswordManager(authority, explanation);
-      return;
-    }
-#endif
-    handler->CancelAuth();
-    return;
-  }
-
-  if (password_manager &&
-      password_manager->client()->GetLogManager()->IsLoggingActive()) {
-    password_manager::BrowserSavePasswordProgressLogger logger(
-        password_manager->client()->GetLogManager());
-    logger.LogMessage(
-        autofill::SavePasswordProgressLogger::STRING_SHOW_LOGIN_PROMPT_METHOD);
-  }
-
-  PasswordForm observed_form(
-      MakeInputForPasswordManager(request_url, auth_info));
-  handler->BuildViewWithPasswordManager(authority, explanation,
-                                        password_manager, observed_form);
-}
-
 }  // namespace
 
 // ----------------------------------------------------------------------------
@@ -512,16 +424,139 @@ void LoginHandler::CloseContentsDeferred() {
     interstitial_delegate_->Proceed();
 }
 
-// This callback is run on the UI thread and creates a constrained window with
-// a LoginView to prompt the user. If the prompt is triggered because of
-// a cross origin navigation in the main frame, a blank interstitial is first
-// created which in turn creates the LoginView. Otherwise, a LoginView is
-// directly in this callback. In both cases, the response will be sent to
-// LoginHandler, which then routes it to the net::URLRequest on the I/O thread.
-void LoginDialogCallback(const GURL& request_url,
-                         net::AuthChallengeInfo* auth_info,
-                         LoginHandler* handler,
-                         bool is_main_frame) {
+// static
+std::string LoginHandler::GetSignonRealm(
+    const GURL& url,
+    const net::AuthChallengeInfo& auth_info) {
+  std::string signon_realm;
+  if (auth_info.is_proxy) {
+    // Historically we've been storing the signon realm for proxies using
+    // net::HostPortPair::ToString().
+    net::HostPortPair host_port_pair =
+        net::HostPortPair::FromURL(GURL(auth_info.challenger.Serialize()));
+    signon_realm = host_port_pair.ToString();
+    signon_realm.append("/");
+  } else {
+    // Take scheme, host, and port from the url.
+    signon_realm = url.GetOrigin().spec();
+    // This ends with a "/".
+  }
+  signon_realm.append(auth_info.realm);
+  return signon_realm;
+}
+
+// static
+PasswordForm LoginHandler::MakeInputForPasswordManager(
+    const GURL& request_url,
+    const net::AuthChallengeInfo& auth_info) {
+  PasswordForm dialog_form;
+  if (base::LowerCaseEqualsASCII(auth_info.scheme, net::kBasicAuthScheme)) {
+    dialog_form.scheme = PasswordForm::SCHEME_BASIC;
+  } else if (base::LowerCaseEqualsASCII(auth_info.scheme,
+                                        net::kDigestAuthScheme)) {
+    dialog_form.scheme = PasswordForm::SCHEME_DIGEST;
+  } else {
+    dialog_form.scheme = PasswordForm::SCHEME_OTHER;
+  }
+  if (auth_info.is_proxy) {
+    dialog_form.origin = GURL(auth_info.challenger.Serialize());
+  } else if (auth_info.challenger != url::Origin(request_url)) {

[meacer, 2016/06/16 01:12:14]

Why not use !IsSameOrigin here? It sounds a bit heavy handed to add an overload
just for a single call.

[asanka, 2016/06/16 16:25:47]

The addition of the operator follows the style guidelines to overload related
operators, which is why I didn't think much about it. However I'll remove it
from the CL since IsSameOriginWith() is clearer in its intent.

+    dialog_form.origin = GURL();
+    NOTREACHED();  // crbug.com/32718
+  } else {
+    dialog_form.origin = GURL(auth_info.challenger.Serialize());
+  }
+  dialog_form.signon_realm = GetSignonRealm(dialog_form.origin, auth_info);
+  return dialog_form;
+}
+
+// static
+void LoginHandler::GetDialogStrings(const GURL& request_url,
+                                    const net::AuthChallengeInfo& auth_info,
+                                    base::string16* authority,
+                                    base::string16* explanation) {
+  GURL authority_url;
+
+  if (auth_info.is_proxy) {
+    *authority = l10n_util::GetStringFUTF16(
+        IDS_LOGIN_DIALOG_PROXY_AUTHORITY,
+        url_formatter::FormatOriginForSecurityDisplay(
+            auth_info.challenger, url_formatter::SchemeDisplay::SHOW));
+    authority_url = GURL(auth_info.challenger.Serialize());
+  } else {
+    *authority = l10n_util::GetStringFUTF16(
+        IDS_LOGIN_DIALOG_AUTHORITY,
+        url_formatter::FormatUrlForSecurityDisplay(request_url));
+    authority_url = request_url;
+  }
+
+  if (!content::IsOriginSecure(authority_url)) {
+    // TODO(asanka): The string should be different for proxies and servers.

[vabr (Chromium), 2016/06/15 08:50:06]

nit: Could you please make this a TODO(crbug.com/XXX) where XXX is the number of
the bug having the details for what is the issue and plans to fix it?

[asanka, 2016/06/16 16:25:47]

Added one. The plan is to pretty much fix it immediately. The string change is
not part of this CL because we may need to merge to the release branches and
such changes can't contain string changes.

I also removed the no-op change I was making to the .grdp file since that'll
probably be disconcerting during merge consideration.

[vabr (Chromium), 2016/06/16 16:43:21]

Acknowledged.

+    *explanation =
+        l10n_util::GetStringUTF16(IDS_WEBSITE_SETTINGS_NON_SECURE_TRANSPORT);
+  } else {
+    explanation->clear();
+  }
+}
+
+// static
+void LoginHandler::ShowLoginPrompt(const GURL& request_url,
+                                   net::AuthChallengeInfo* auth_info,
+                                   LoginHandler* handler) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  WebContents* parent_contents = handler->GetWebContentsForLogin();
+  if (!parent_contents)
+    return;
+  prerender::PrerenderContents* prerender_contents =
+      prerender::PrerenderContents::FromWebContents(parent_contents);
+  if (prerender_contents) {
+    prerender_contents->Destroy(prerender::FINAL_STATUS_AUTH_NEEDED);
+    return;
+  }
+
+  base::string16 authority;
+  base::string16 explanation;
+  GetDialogStrings(request_url, *auth_info, &authority, &explanation);
+
+  password_manager::PasswordManager* password_manager =
+      handler->GetPasswordManagerForLogin();
+
+  if (!password_manager) {
+#if defined(ENABLE_EXTENSIONS)
+    // A WebContents in a <webview> (a GuestView type) does not have a password
+    // manager, but still needs to be able to show login prompts.
+    const auto* guest =
+        guest_view::GuestViewBase::FromWebContents(parent_contents);
+    if (guest &&
+        extensions::GetViewType(guest->owner_web_contents()) !=
+            extensions::VIEW_TYPE_EXTENSION_BACKGROUND_PAGE) {
+      handler->BuildViewWithoutPasswordManager(authority, explanation);
+      return;
+    }
+#endif
+    handler->CancelAuth();
+    return;
+  }
+
+  if (password_manager &&
+      password_manager->client()->GetLogManager()->IsLoggingActive()) {
+    password_manager::BrowserSavePasswordProgressLogger logger(
+        password_manager->client()->GetLogManager());
+    logger.LogMessage(
+        autofill::SavePasswordProgressLogger::STRING_SHOW_LOGIN_PROMPT_METHOD);
+  }
+
+  PasswordForm observed_form(
+      LoginHandler::MakeInputForPasswordManager(request_url, *auth_info));
+  handler->BuildViewWithPasswordManager(authority, explanation,
+                                        password_manager, observed_form);
+}
+
+// static
+void LoginHandler::LoginDialogCallback(const GURL& request_url,
+                                       net::AuthChallengeInfo* auth_info,
+                                       LoginHandler* handler,
+                                       bool is_main_frame) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   WebContents* parent_contents = handler->GetWebContentsForLogin();
   if (!parent_contents || handler->WasAuthHandled()) {
@@ -548,18 +583,21 @@ void LoginDialogCallback(const GURL& request_url,
   // 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())) {
+  if (is_main_frame &&
+      (parent_contents->ShowingInterstitialPage() || auth_info->is_proxy ||

[meacer, 2016/06/16 01:12:14]

nit: Can you update the comment above and add the proxy case?

[asanka, 2016/06/16 16:25:47]

Done. Thanks, missed the comment in the first pass.

+       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, base::RetainedRef(auth_info),
-                   base::RetainedRef(handler));
+        base::Bind(&LoginHandler::ShowLoginPrompt, request_url,
+                   base::RetainedRef(auth_info), base::RetainedRef(handler));
     // The interstitial delegate is owned by the interstitial that it creates.
     // This cancels any existing interstitial.
     handler->SetInterstitialDelegate(
-        (new LoginInterstitialDelegate(parent_contents, request_url, callback))
+        (new LoginInterstitialDelegate(
+             parent_contents, auth_info->is_proxy ? GURL() : request_url,
+             callback))
             ->GetWeakPtr());
   } else {
     ShowLoginPrompt(request_url, auth_info, handler);
@@ -575,32 +613,9 @@ LoginHandler* CreateLoginPrompt(net::AuthChallengeInfo* auth_info,
   LoginHandler* handler = LoginHandler::Create(auth_info, request);
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
-      base::Bind(&LoginDialogCallback, request->url(),
+      base::Bind(&LoginHandler::LoginDialogCallback, request->url(),
                  base::RetainedRef(auth_info), base::RetainedRef(handler),
                  is_main_frame));
   return handler;
 }
 
-// Get the signon_realm under which this auth info should be stored.
-//
-// The format of the signon_realm for proxy auth is:
-//     proxy-host/auth-realm
-// The format of the signon_realm for server auth is:
-//     url-scheme://url-host[:url-port]/auth-realm
-//
-// Be careful when changing this function, since you could make existing
-// saved logins un-retrievable.
-std::string GetSignonRealm(const GURL& url,
-                           const net::AuthChallengeInfo& auth_info) {
-  std::string signon_realm;
-  if (auth_info.is_proxy) {
-    signon_realm = auth_info.challenger.ToString();
-    signon_realm.append("/");
-  } else {
-    // Take scheme, host, and port from the url.
-    signon_realm = url.GetOrigin().spec();
-    // This ends with a "/".
-  }
-  signon_realm.append(auth_info.realm);
-  return signon_realm;
-}