Use correct origin when prompting for proxy authentication.

chrome/browser/ui/login/login_handler.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/login/login_handler.h"
 
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
@@ skipping 14 matching lines @@
 #include "components/url_formatter/elide_url.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/resource_dispatcher_host.h"
 #include "content/public/browser/resource_request_info.h"
 #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"
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_context.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/text_elider.h"
 
 #if defined(ENABLE_EXTENSIONS)
 #include "components/guest_view/browser/guest_view_base.h"
@@ skipping 13 matching lines @@
 
 namespace {
 
 // Helper to remove the ref from an net::URLRequest to the LoginHandler.
 // Should only be called from the IO thread, since it accesses an
 // net::URLRequest.
 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
 
 // ----------------------------------------------------------------------------
 // LoginHandler
 
 LoginHandler::LoginModelData::LoginModelData(
     password_manager::LoginModel* login_model,
     const autofill::PasswordForm& observed_form)
     : model(login_model), form(observed_form) {
   DCHECK(model);
@@ skipping 338 matching lines @@
 }
 
 // Closes the view_contents from the UI loop.
 void LoginHandler::CloseContentsDeferred() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   CloseDialog();
   if (interstitial_delegate_)
     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()) {
     // The request may have been canceled, or it may be for a renderer not
     // hosted by a tab (e.g. an extension). Cancel just in case (canceling twice
     // is a no-op).
     handler->CancelAuth();
     return;
   }
 
   // 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.
   //
   // 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())) {
+  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);
   }
 }
 
 // ----------------------------------------------------------------------------
 // Public API
 
 LoginHandler* CreateLoginPrompt(net::AuthChallengeInfo* auth_info,
                                 net::URLRequest* request) {
   bool is_main_frame = (request->load_flags() & net::LOAD_MAIN_FRAME) != 0;
   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;
-}