Show confirmation dialog for unsecure signin

chrome/browser/ui/webui/signin/inline_login_handler_impl.cc

Index: chrome/browser/ui/webui/signin/inline_login_handler_impl.cc
diff --git a/chrome/browser/ui/webui/signin/inline_login_handler_impl.cc b/chrome/browser/ui/webui/signin/inline_login_handler_impl.cc
index 0cef1e5ed58ebec2061f2204c06c45fe0acca655..67f8160156c62165422771726ab60c8762740210 100644
--- a/chrome/browser/ui/webui/signin/inline_login_handler_impl.cc
+++ b/chrome/browser/ui/webui/signin/inline_login_handler_impl.cc
@@ -23,6 +23,7 @@
 #include "chrome/browser/ui/sync/one_click_signin_helper.h"
 #include "chrome/browser/ui/sync/one_click_signin_histogram.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/webui/signin/inline_login_ui.h"
 #include "chrome/common/url_constants.h"
 #include "components/signin/core/browser/about_signin_internals.h"
 #include "components/signin/core/browser/profile_oauth2_token_service.h"
@@ -50,7 +51,8 @@ class InlineSigninHelper : public SigninOAuthHelper,
       const std::string& password,
       const std::string& session_index,
       const std::string& signin_scoped_device_id,
-      bool choose_what_to_sync);
+      bool choose_what_to_sync,
+      bool confirm_untrusted_signin);
 
  private:
   // Overriden from SigninOAuthHelper::Consumer.
@@ -68,6 +70,7 @@ class InlineSigninHelper : public SigninOAuthHelper,
   std::string password_;
   std::string session_index_;
   bool choose_what_to_sync_;
+  bool confirm_untrusted_signin_;
 
   DISALLOW_COPY_AND_ASSIGN(InlineSigninHelper);
 };
@@ -81,14 +84,17 @@ InlineSigninHelper::InlineSigninHelper(
     const std::string& password,
     const std::string& session_index,
     const std::string& signin_scoped_device_id,
-    bool choose_what_to_sync)
+    bool choose_what_to_sync,
+    bool confirm_untrusted_signin)
     : SigninOAuthHelper(getter, session_index, signin_scoped_device_id, this),
       handler_(handler),
       profile_(profile),
       current_url_(current_url),
       email_(email),
       password_(password),
-      choose_what_to_sync_(choose_what_to_sync) {
+      session_index_(session_index),
+      choose_what_to_sync_(choose_what_to_sync),
+      confirm_untrusted_signin_(confirm_untrusted_signin) {
   DCHECK(profile_);
   DCHECK(!email_.empty());
 }
@@ -136,12 +142,19 @@ void InlineSigninHelper::OnSigninOAuthInformationAvailable(
                 OneClickSigninSyncStarter::SHOW_SETTINGS_WITHOUT_CONFIGURE :
                 OneClickSigninSyncStarter::CONFIGURE_SYNC_FIRST :
                 OneClickSigninSyncStarter::SYNC_WITH_DEFAULT_SETTINGS;
-    OneClickSigninSyncStarter::ConfirmationRequired confirmation_required =
-        source == signin::SOURCE_SETTINGS ||
-        source == signin::SOURCE_WEBSTORE_INSTALL ||
-        choose_what_to_sync_ ?
-            OneClickSigninSyncStarter::NO_CONFIRMATION :
-            OneClickSigninSyncStarter::CONFIRM_AFTER_SIGNIN;
+
+    OneClickSigninSyncStarter::ConfirmationRequired confirmation_required;
+    if (confirm_untrusted_signin_) {
+      confirmation_required =
+          OneClickSigninSyncStarter::CONFIRM_UNTRUSTED_SIGNIN;
+    } else {
+      confirmation_required =
+          source == signin::SOURCE_SETTINGS ||
+          source == signin::SOURCE_WEBSTORE_INSTALL ||
+          choose_what_to_sync_ ?
+              OneClickSigninSyncStarter::NO_CONFIRMATION :
+              OneClickSigninSyncStarter::CONFIRM_AFTER_SIGNIN;
+    }
 
     bool start_signin =
         !OneClickSigninHelper::HandleCrossAccountError(
@@ -184,7 +197,7 @@ void InlineSigninHelper::OnSigninOAuthInformationFailure(
 
 InlineLoginHandlerImpl::InlineLoginHandlerImpl()
       : weak_factory_(this),
-        choose_what_to_sync_(false) {
+        confirm_untrusted_signin_(false) {
 }
 
 InlineLoginHandlerImpl::~InlineLoginHandlerImpl() {}
@@ -198,6 +211,28 @@ bool InlineLoginHandlerImpl::HandleContextMenu(
 #endif
 }
 
+void InlineLoginHandlerImpl::DidCommitProvisionalLoadForFrame(
+    content::RenderFrameHost* render_frame_host,
+    const GURL& url,
+    content::PageTransition transition_type) {
+  if (!web_contents())
+    return;
+
+  // Return early if this is not a gaia iframe navigation.
+  const GURL kGaiaExtOrigin(
+      "chrome-extension://mfffpogegjflfpflabcdkioaeobkgjik/");
+  content::RenderFrameHost* gaia_iframe = InlineLoginUI::GetAuthIframe(
+      web_contents(), kGaiaExtOrigin, "signin-frame");
+  if (render_frame_host != gaia_iframe)
+    return;
+
+  if (url.spec() != url::kAboutBlankURL &&

[Charlie Reis, 2014/07/23 21:42:00]

Let's put a comment here saying that loading any untrusted (e.g., HTTP) URLs in
the privileged sign-in process should require confirmation before the sign in
takes effect.

[guohui, 2014/07/23 21:57:51]

Done.

[guohui, 2014/07/23 21:57:51]

Done.

+      !gaia::IsGaiaSignonRealm(url.GetOrigin()) &&
+      !signin::IsContinueUrlForWebBasedSigninFlow(url)) {

[Charlie Reis, 2014/07/23 21:42:00]

Does the continue URL get loaded in the sign-in process?  Can it be HTTP?  That
could be a loophole.

[guohui, 2014/07/23 21:57:51]

The continue URL does get loaded in the signin process at the end of signin
flow. It is a fixed url as defined at,
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/sig...

Basically it is a blank page created solely for the chrome signin process that
is used to signal the completion of signin flow.

And in my test cl, i actually change the continue url to a local page in the
gaia auth extension. Either way this should not expose any security risk.

[Charlie Reis, 2014/07/23 22:18:06]

But it can be specified manually as a URL parameter, right?  That means we don't
have control over it, so I'm not sure why it's considered trusted.  Perhaps I'm
misunderstanding, but if it gets loaded in the process then it seems like it
poses a risk.

[guohui, 2014/07/23 23:19:46]

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/sig...

IsContinueUrlForWebBasedSigninFlow checks for a hardcoded URL thus we do have
control over it.

In some cases (webstore signin) we allow a customized continue URL, but the
customized continue URL param does not override the hardcoded url in the method
IsContinueUrlForWebBasedSigninFlow, thus the check should be good in all cases.

[Charlie Reis, 2014/07/23 23:36:12]

Acknowledged.

+    confirm_untrusted_signin_ = true;
+  }
+}
+
 void InlineLoginHandlerImpl::SetExtraInitParams(base::DictionaryValue& params) {
   params.SetString("service", "chromiumsync");
 
@@ -208,6 +243,8 @@ void InlineLoginHandlerImpl::SetExtraInitParams(base::DictionaryValue& params) {
   if (is_constrained == "1")
     contents->SetDelegate(this);
 
+  content::WebContentsObserver::Observe(contents);

[Charlie Reis, 2014/07/23 21:42:00]

I think this means we only observe when the WebContents is used for sign-in? 
That's good.

Is there a point at which we can stop observing (e.g., Observe(NULL))?

[guohui, 2014/07/23 21:57:51]

We don't explicitly stop observing, so the observation will be stopped upon
destruction, which happens when the signin tab is closed.

[Charlie Reis, 2014/07/23 22:18:06]

Is it necessary to keep it around that long?  It seems like unnecessary overhead
once the sign in flow is done.

[guohui, 2014/07/23 23:19:46]

sorry i made a mistake in my earlier comment, this class is destroyed when the
inline signin is closed or when the tab navigates to some other URL. And once
signin completes, chrome will immediately navigate to either ntp or settings
page or close the tab, thus there should be not much overhead.

[Charlie Reis, 2014/07/23 23:36:12]

Ah, good.  That sounds good to me.

+
   signin::Source source = signin::GetSourceForPromoURL(current_url);
   OneClickSigninHelper::LogHistogramValue(
       source, one_click_signin::HISTOGRAM_SHOWN);
@@ -228,13 +265,14 @@ void InlineLoginHandlerImpl::CompleteLogin(const base::ListValue* args) {
     return;
   }
 
-  base::string16 email;
-  dict->GetString("email", &email);
-  DCHECK(!email.empty());
-  email_ = base::UTF16ToASCII(email);
-  base::string16 password;
-  dict->GetString("password", &password);
-  password_ = base::UTF16ToASCII(password);
+  base::string16 email_string16;
+  dict->GetString("email", &email_string16);
+  DCHECK(!email_string16.empty());
+  std::string email(base::UTF16ToASCII(email_string16));
+
+  base::string16 password_string16;
+  dict->GetString("password", &password_string16);
+  std::string password(base::UTF16ToASCII(password_string16));
 
   // When doing a SAML sign in, this email check may result in a false
   // positive.  This happens when the user types one email address in the
@@ -246,23 +284,25 @@ void InlineLoginHandlerImpl::CompleteLogin(const base::ListValue* args) {
       net::GetValueForKeyInQuery(current_url, "validateEmail",
                                  &validate_email) &&
       validate_email == "1") {
-    if (!gaia::AreEmailsSame(email_, default_email)) {
+    if (!gaia::AreEmailsSame(email, default_email)) {
       SyncStarterCallback(OneClickSigninSyncStarter::SYNC_SETUP_FAILURE);
       return;
     }
   }
 
-  base::string16 session_index;
-  dict->GetString("sessionIndex", &session_index);
-  session_index_ = base::UTF16ToASCII(session_index);
-  DCHECK(!session_index_.empty());
-  dict->GetBoolean("chooseWhatToSync", &choose_what_to_sync_);
+  base::string16 session_index_string16;
+  dict->GetString("sessionIndex", &session_index_string16);
+  std::string session_index = base::UTF16ToASCII(session_index_string16);
+  DCHECK(!session_index.empty());
+
+  bool choose_what_to_sync = false;
+  dict->GetBoolean("chooseWhatToSync", &choose_what_to_sync);
 
   signin::Source source = signin::GetSourceForPromoURL(current_url);
   OneClickSigninHelper::LogHistogramValue(
       source, one_click_signin::HISTOGRAM_ACCEPTED);
   bool switch_to_advanced =
-      choose_what_to_sync_ && (source != signin::SOURCE_SETTINGS);
+      choose_what_to_sync && (source != signin::SOURCE_SETTINGS);
   OneClickSigninHelper::LogHistogramValue(
       source,
       switch_to_advanced ? one_click_signin::HISTOGRAM_WITH_ADVANCED :
@@ -289,7 +329,7 @@ void InlineLoginHandlerImpl::CompleteLogin(const base::ListValue* args) {
 
   std::string error_msg;
   bool can_offer = OneClickSigninHelper::CanOffer(
-      contents, can_offer_for, email_, &error_msg);
+      contents, can_offer_for, email, &error_msg);
   if (!can_offer) {
     HandleLoginError(error_msg);
     return;
@@ -312,12 +352,10 @@ void InlineLoginHandlerImpl::CompleteLogin(const base::ListValue* args) {
   // InlineSigninHelper will delete itself.
   new InlineSigninHelper(GetWeakPtr(), partition->GetURLRequestContext(),
                          Profile::FromWebUI(web_ui()), current_url,
-                         email_, password_, session_index_,
-                         signin_scoped_device_id, choose_what_to_sync_);
+                         email, password, session_index,
+                         signin_scoped_device_id, choose_what_to_sync,
+                         confirm_untrusted_signin_);
 
-  email_.clear();
-  password_.clear();
-  session_index_.clear();
   web_ui()->CallJavascriptFunction("inline.login.closeDialog");
 }
 
@@ -330,10 +368,6 @@ void InlineLoginHandlerImpl::HandleLoginError(const std::string& error_msg) {
             << error_msg;
     OneClickSigninHelper::ShowSigninErrorBubble(browser, error_msg);
   }
-
-  email_.clear();
-  password_.clear();
-  session_index_.clear();
 }
 
 Browser* InlineLoginHandlerImpl::GetDesktopBrowser() {