Move post-signin confirmation bubble to OneClickSigninSyncStarter.

chrome/browser/ui/sync/one_click_signin_helper.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/sync/one_click_signin_helper.h"
 
 #include <algorithm>
 #include <functional>
 #include <utility>
 #include <vector>
@@ skipping 64 matching lines @@
 #include "ui/base/resource/resource_bundle.h"
 
 
 namespace {
 
 // StartSyncArgs --------------------------------------------------------------
 
 // Arguments used with StartSync function.  base::Bind() cannot support too
 // many args for performance reasons, so they are packaged up into a struct.
 struct StartSyncArgs {
-  StartSyncArgs(Profile* profile,
-                Browser* browser,
-                OneClickSigninHelper::AutoAccept auto_accept,
-                const std::string& session_index,
-                const std::string& email,
-                const std::string& password,
-                bool force_same_tab_navigation,
-                bool confirmation_required);
+  StartSyncArgs(
+      Profile* profile,
+      Browser* browser,
+      OneClickSigninHelper::AutoAccept auto_accept,
+      const std::string& session_index,
+      const std::string& email,
+      const std::string& password,
+      bool force_same_tab_navigation,
+      bool saml_confirmation_required,

[Roger Tawa OOO till Jul 10th, 2013/05/03 15:16:47]

Is this really saml related?  I think my suggestion below it is no longer saml
specific.

[Andrew T Wilson (Slow), 2013/05/03 19:16:11]

I only set this if confirmation_required_ is set, which is what is_trusted used
to be called. So I presume this is SAML - I guess I don't understand what the
"untrusted" path would be if not SAML - can you clarify? I've renamed this to
"untrusted", but I'd still like to understand that case.

[Roger Tawa OOO till Jul 10th, 2013/05/06 15:46:39]

Thanks Drew.  The non-saml "untrusted" flow is the interstitial flow when
signing in to some other google service like gmail.

+      SyncPromoUI::Source source);
 
   Profile* profile;
   Browser* browser;
   OneClickSigninHelper::AutoAccept auto_accept;
   std::string session_index;
   std::string email;
   std::string password;
   bool force_same_tab_navigation;
-  bool confirmation_required;
+  OneClickSigninSyncStarter::ConfirmationRequired confirmation_required;
 };
 
-StartSyncArgs::StartSyncArgs(Profile* profile,
-                             Browser* browser,
-                             OneClickSigninHelper::AutoAccept auto_accept,
-                             const std::string& session_index,
-                             const std::string& email,
-                             const std::string& password,
-                             bool force_same_tab_navigation,
-                             bool confirmation_required)
+StartSyncArgs::StartSyncArgs(
+    Profile* profile,
+    Browser* browser,
+    OneClickSigninHelper::AutoAccept auto_accept,
+    const std::string& session_index,
+    const std::string& email,
+    const std::string& password,
+    bool force_same_tab_navigation,
+    bool saml_confirmation_required,
+    SyncPromoUI::Source source)
     : profile(profile),
       browser(browser),
       auto_accept(auto_accept),
       session_index(session_index),
       email(email),
       password(password),
-      force_same_tab_navigation(force_same_tab_navigation),
-      confirmation_required(confirmation_required) {
+      force_same_tab_navigation(force_same_tab_navigation) {
+  if (saml_confirmation_required) {
+    confirmation_required = OneClickSigninSyncStarter::CONFIRM_SAML_SIGNIN;
+  } else if (source == SyncPromoUI::SOURCE_SETTINGS ||
+             source == SyncPromoUI::SOURCE_WEBSTORE_INSTALL) {
+    // Do not display a status confirmation for webstore installs or re-auth.
+    confirmation_required = OneClickSigninSyncStarter::NO_CONFIRMATION;
+  } else {
+    confirmation_required = OneClickSigninSyncStarter::CONFIRM_AFTER_SIGNIN;
+  }
 }
 
 
 // Helpers --------------------------------------------------------------------
 
 // Add a specific email to the list of emails rejected for one-click
 // sign-in, for this profile.
 void AddEmailToOneClickRejectedList(Profile* profile,
                                     const std::string& email) {
   PrefService* pref_service = profile->GetPrefs();
@@ skipping 334 matching lines @@
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(OneClickSigninHelper);
 
 OneClickSigninHelper::OneClickSigninHelper(content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents),
       showing_signin_(false),
       auto_accept_(AUTO_ACCEPT_NONE),
       source_(SyncPromoUI::SOURCE_UNKNOWN),
       switched_to_advanced_(false),
       original_source_(SyncPromoUI::SOURCE_UNKNOWN),
       untrusted_navigations_since_signin_visit_(0),
-      confirmation_required_(false) {
+      saml_confirmation_required_(false) {
 }
 
 OneClickSigninHelper::~OneClickSigninHelper() {
   content::WebContents* contents = web_contents();
   if (contents) {
     Profile* profile =
         Profile::FromBrowserContext(contents->GetBrowserContext());
     ProfileSyncService* sync_service =
         ProfileSyncServiceFactory::GetForProfile(profile);
     if (sync_service && sync_service->HasObserver(this))
@@ skipping 332 matching lines @@
   CanOfferFor can_offer_for =
       (auto_accept != AUTO_ACCEPT_EXPLICIT &&
           helper->auto_accept_ != AUTO_ACCEPT_EXPLICIT) ?
           CAN_OFFER_FOR_INTERSTITAL_ONLY : CAN_OFFER_FOR_ALL;
 
   std::string error_message;
 
   if (!web_contents || !CanOffer(web_contents, can_offer_for, email,
                                  &error_message)) {
     VLOG(1) << "OneClickSigninHelper::ShowInfoBarUIThread: not offering";
+    // TODO(rogerta): Can we just display our error now instead of keeping it
+    // around and doing it later?
     if (helper && helper->error_message_.empty() && !error_message.empty())
       helper->error_message_ = error_message;
 
     return;
   }
 
   // Only allow the dedicated signin process to sign the user into
   // Chrome without intervention, because it doesn't load any untrusted
   // pages.  If at any point an untrusted page is detected, chrome will
   // show a modal dialog asking the user to confirm.
   Profile* profile =
       Profile::FromBrowserContext(web_contents->GetBrowserContext());
   SigninManager* manager = profile ?
       SigninManagerFactory::GetForProfile(profile) : NULL;
-  helper->confirmation_required_ |= (manager &&
-                                     !manager->IsSigninProcess(child_id));
+  helper->saml_confirmation_required_ |= (manager &&
+                                          !manager->IsSigninProcess(child_id));
 
   // Save the email in the one-click signin manager.  The manager may
   // not exist if the contents is incognito or if the profile is already
   // connected to a Google account.
   if (!session_index.empty())
     helper->session_index_ = session_index;
 
   if (!email.empty())
     helper->email_ = email;
 
   if (continue_url.is_valid())
     helper->continue_url_ = continue_url;
 }
 
 // static
 void OneClickSigninHelper::RemoveCurrentHistoryItem(
     content::WebContents* web_contents) {
   new CurrentHistoryCleaner(web_contents);  // will self-destruct when finished
 }
 
-void OneClickSigninHelper::ShowSyncConfirmationBubble(bool show_bubble) {
-  if (show_bubble) {
-    content::WebContents* contents = web_contents();
-    Profile* profile =
-        Profile::FromBrowserContext(contents->GetBrowserContext());
-    Browser* browser = chrome::FindBrowserWithWebContents(contents);
+void OneClickSigninHelper::ShowSigninErrorBubble(const std::string& error) {
+  DCHECK(!error.empty());
+  content::WebContents* contents = web_contents();
+  Profile* profile =
+      Profile::FromBrowserContext(contents->GetBrowserContext());
+  Browser* browser = chrome::FindBrowserWithWebContents(contents);
 
-    browser->window()->ShowOneClickSigninBubble(
-        BrowserWindow::ONE_CLICK_SIGNIN_BUBBLE_TYPE_BUBBLE,
-        string16(), /* no SAML email */
-        UTF8ToUTF16(error_message_),
-        base::Bind(&StartSync,
-                   StartSyncArgs(profile, browser, AUTO_ACCEPT_ACCEPTED,
-                                 session_index_, email_, password_,
-                                 false, confirmation_required_)));
-  }
-  error_message_.clear();
+  browser->window()->ShowOneClickSigninBubble(
+      BrowserWindow::ONE_CLICK_SIGNIN_BUBBLE_TYPE_BUBBLE,
+      string16(), /* no SAML email */
+      UTF8ToUTF16(error),
+      // This callback is never invoked.
+      // TODO(rogerta): Separate out the bubble API so we don't have to pass
+      // ignored |email| and |callback| params.
+      base::Bind(&StartSync,
+                 StartSyncArgs(profile, browser, AUTO_ACCEPT_ACCEPTED,
+                               session_index_, email_, password_,
+                               false, saml_confirmation_required_, source_)));
 }
 
-void OneClickSigninHelper::RedirectToNtpOrAppsPage(bool show_bubble) {
+void OneClickSigninHelper::RedirectToNtpOrAppsPage() {
   VLOG(1) << "OneClickSigninHelper::RedirectToNtpOrAppsPage";
-
   // Redirect to NTP/Apps page and display a confirmation bubble
   content::WebContents* contents = web_contents();
   GURL url(chrome::IsInstantExtendedAPIEnabled() ?
            chrome::kChromeUIAppsURL : chrome::kChromeUINewTabURL);
   content::OpenURLParams params(url,
                                 content::Referrer(),
                                 CURRENT_TAB,
                                 content::PAGE_TRANSITION_AUTO_TOPLEVEL,
                                 false);
   contents->OpenURL(params);
-
-  ShowSyncConfirmationBubble(show_bubble);
 }
 
 void OneClickSigninHelper::RedirectToSignin() {
   VLOG(1) << "OneClickSigninHelper::RedirectToSignin";
 
   // Extract the existing sounce=X value.  Default to "2" if missing.
   SyncPromoUI::Source source =
       SyncPromoUI::GetSourceForSyncPromoURL(continue_url_);
   if (source == SyncPromoUI::SOURCE_UNKNOWN)
     source = SyncPromoUI::SOURCE_MENU;
@@ skipping 10 matching lines @@
   VLOG(1) << "OneClickSigninHelper::CleanTransientState";
   showing_signin_ = false;
   email_.clear();
   password_.clear();
   auto_accept_ = AUTO_ACCEPT_NONE;
   source_ = SyncPromoUI::SOURCE_UNKNOWN;
   switched_to_advanced_ = false;
   original_source_ = SyncPromoUI::SOURCE_UNKNOWN;
   continue_url_ = GURL();
   untrusted_navigations_since_signin_visit_ = 0;
-  confirmation_required_ = false;
+  saml_confirmation_required_ = false;
+  error_message_.clear();
 
   // Post to IO thread to clear pending email.
   Profile* profile =
       Profile::FromBrowserContext(web_contents()->GetBrowserContext());
   content::BrowserThread::PostTask(
       content::BrowserThread::IO, FROM_HERE,
       base::Bind(&ClearPendingEmailOnIOThread,
                  base::Unretained(profile->GetResourceContext())));
 }
 
@@ skipping 54 matching lines @@
   // TODO(rogerta): might need to allow some youtube URLs.
   content::WebContents* contents = web_contents();
   const GURL url = contents->GetURL();
   Profile* profile =
       Profile::FromBrowserContext(contents->GetBrowserContext());
   VLOG(1) << "OneClickSigninHelper::DidStopLoading: url=" << url.spec();
 
   // If an error has already occured during the sign in flow, make sure to
   // display it to the user and abort the process.  Do this only for
   // explicit sign ins.
+  // TODO(rogerta): Could we move this code back up to ShowInfoBarUIThread()?
   if (!error_message_.empty() && auto_accept_ == AUTO_ACCEPT_EXPLICIT) {
     VLOG(1) << "OneClickSigninHelper::DidStopLoading: error=" << error_message_;
     RemoveCurrentHistoryItem(contents);
-    RedirectToNtpOrAppsPage(true);
+    // Redirect to the landing page and display an error popup.
+    RedirectToNtpOrAppsPage();
+    ShowSigninErrorBubble(error_message_);
+    CleanTransientState();
     return;
   }
 
   if (AreWeShowingSignin(url, source_, email_)) {
     if (!showing_signin_) {
       if (source_ == SyncPromoUI::SOURCE_UNKNOWN)
         LogOneClickHistogramValue(one_click_signin::HISTOGRAM_SHOWN);
       else
         LogHistogramValue(source_, one_click_signin::HISTOGRAM_SHOWN);
     }
@@ skipping 14 matching lines @@
   // password is allowed to be empty, since its no longer required to setup
   // sync.
   if (email_.empty()) {
     VLOG(1) << "OneClickSigninHelper::DidStopLoading: nothing to do";
     if (continue_url_match && auto_accept_ == AUTO_ACCEPT_EXPLICIT)
       RedirectToSignin();
     std::string unused_value;
     if (net::GetValueForKeyInQuery(url, "ntp", &unused_value)) {
       SyncPromoUI::SetUserSkippedSyncPromo(profile);
       RemoveCurrentHistoryItem(contents);
-      RedirectToNtpOrAppsPage(false);
+      RedirectToNtpOrAppsPage();
     }
 
     if (!continue_url_match && !IsValidGaiaSigninRedirectOrResponseURL(url) &&
         ++untrusted_navigations_since_signin_visit_ > kMaxNavigationsSince) {
       CleanTransientState();
     }
 
     return;
   }
 
@@ skipping 76 matching lines @@
       LogOneClickHistogramValue(one_click_signin::HISTOGRAM_WITH_DEFAULTS);
       SigninManager::DisableOneClickSignIn(profile);
       browser->window()->ShowOneClickSigninBubble(
           bubble_type,
           UTF8ToUTF16(email_),
           string16(), /* no error message to display */
           base::Bind(&StartSync,
                      StartSyncArgs(profile, browser, auto_accept_,
                                    session_index_, email_, password_,
                                    false /* force_same_tab_navigation */,
-                                   confirmation_required_)));
+                                   saml_confirmation_required_, source_)));

[Roger Tawa OOO till Jul 10th, 2013/05/03 15:16:47]

Hmm, this is actually broken.  Not your CL Drew, but I think of the recent
refactorings.  In this case, we will show two dialogs!  The one here, and since
|saml_confirmation_required_| will be true, the saml dialog is shown.

In discussions with the security team, it was decided that the non-saml
untrusted path should show the same dialog as the saml untrusted path.  That is,
the one with the email address.  So I think the right fix would be to replace
lines 1146-1154 with something like:

      StartSync(StartSyncArgs(profile, browser, auto_accept_,
                              session_index_, email_, password_,
                              false, true, source_),
                OneClickSigninSyncStarter::SYNC_WITH_DEFAULT_SETTINGS);

[Andrew T Wilson (Slow), 2013/05/03 19:16:11]

Do you mean that I should pass |false| as the confirmation_required flag instead
of |true| (I've made that change assuming that's what you meant).

[Roger Tawa OOO till Jul 10th, 2013/05/06 15:46:39]

I was actually suggesting passing |true| instead of |false|, and not calling
ShowOneClickSigninBubble() at all here.  Two reasons:

1/ We consolidate confirmations in the starter
2/ The security folks said we should display the email address in the dialog
even for the non-saml "untrusted" case

This allows us to get rid of ONE_CLICK_SIGNIN_BUBBLE_TYPE_MODAL_DIALOG and only
have one dialog ONE_CLICK_SIGNIN_BUBBLE_TYPE_SAML_MODAL_DIALOG.  (I can remove
ONE_CLICK_SIGNIN_BUBBLE_TYPE_MODAL_DIALOG in a follow up CL).

Does that sounds reasonable?

[Andrew T Wilson (Slow), 2013/05/06 17:00:13]

Done.

       break;
     case AUTO_ACCEPT_CONFIGURE:
       LogOneClickHistogramValue(one_click_signin::HISTOGRAM_ACCEPTED);
       LogOneClickHistogramValue(one_click_signin::HISTOGRAM_WITH_ADVANCED);
       SigninManager::DisableOneClickSignIn(profile);
       StartSync(
           StartSyncArgs(profile, browser, auto_accept_, session_index_, email_,
                         password_, false /* force_same_tab_navigation */,
-                        confirmation_required_),
+                        saml_confirmation_required_, source_),
           OneClickSigninSyncStarter::CONFIGURE_SYNC_FIRST);
       break;
     case AUTO_ACCEPT_EXPLICIT: {
       if (switched_to_advanced_) {
         LogHistogramValue(original_source_,
                           one_click_signin::HISTOGRAM_WITH_ADVANCED);
         LogHistogramValue(original_source_,
                           one_click_signin::HISTOGRAM_ACCEPTED);
       } else {
         LogHistogramValue(source_, one_click_signin::HISTOGRAM_ACCEPTED);
         LogHistogramValue(source_, one_click_signin::HISTOGRAM_WITH_DEFAULTS);
       }
       OneClickSigninSyncStarter::StartSyncMode start_mode =
           source_ == SyncPromoUI::SOURCE_SETTINGS ?
               OneClickSigninSyncStarter::CONFIGURE_SYNC_FIRST :
               OneClickSigninSyncStarter::SYNC_WITH_DEFAULT_SETTINGS;
 
       std::string last_email =
           profile->GetPrefs()->GetString(prefs::kGoogleServicesLastUsername);
 
       if (!last_email.empty() && last_email != email_) {
         // If the new email address is different from the email address that
         // just signed in, show a confirmation dialog.
 
         // No need to display a second confirmation.
-        confirmation_required_ = false;
+        saml_confirmation_required_ = false;
         ConfirmEmailDialogDelegate::AskForConfirmation(
             contents,
             last_email,
             email_,
             base::Bind(
                 &StartExplicitSync,
                 StartSyncArgs(profile, browser, auto_accept_,
                               session_index_, email_, password_,
                               force_same_tab_navigation,
-                              confirmation_required_),
+                              saml_confirmation_required_, source_),
                 contents,
                 start_mode));

[Roger Tawa OOO till Jul 10th, 2013/05/03 15:16:47]

Can we move this confirmation dialog into the starter as well?

[Andrew T Wilson (Slow), 2013/05/03 19:16:11]

Maybe as a separate CL? I wasn't really doing this CL just for cleanliness but
rather to fix specific bugs.

[Roger Tawa OOO till Jul 10th, 2013/05/06 15:46:39]

Sure.

[Andrew T Wilson (Slow), 2013/05/06 17:00:13]

Done.

       } else {
         StartExplicitSync(
             StartSyncArgs(profile, browser, auto_accept_, session_index_,
                           email_, password_, force_same_tab_navigation,
-                          confirmation_required_),
+                          saml_confirmation_required_, source_),
             contents,
             start_mode,
             IDS_ONE_CLICK_SIGNIN_CONFIRM_EMAIL_DIALOG_CANCEL_BUTTON);
       }
 
       if (source_ == SyncPromoUI::SOURCE_SETTINGS &&
           SyncPromoUI::GetSourceForSyncPromoURL(continue_url_) ==
           SyncPromoUI::SOURCE_WEBSTORE_INSTALL) {
         redirect_url_ = continue_url_;
         ProfileSyncService* sync_service =
           ProfileSyncServiceFactory::GetForProfile(profile);
         if (sync_service)
           sync_service->AddObserver(this);
       }
 
       // If this explicit sign in is not from settings page/webstore, show the
       // NTP/Apps page after sign in completes. In the case of the settings
       // page, it will get closed by SyncSetupHandler. In the case of webstore,
       // it will redirect back to webstore.
       if (source_ != SyncPromoUI::SOURCE_SETTINGS &&
           source_ != SyncPromoUI::SOURCE_WEBSTORE_INSTALL) {
-        signin_tracker_.reset(new SigninTracker(profile, this));
         RemoveCurrentHistoryItem(contents);
-        RedirectToNtpOrAppsPage(false);
+        RedirectToNtpOrAppsPage();
       }
       break;
     }
     case AUTO_ACCEPT_REJECTED_FOR_PROFILE:
       AddEmailToOneClickRejectedList(profile, email_);
       LogOneClickHistogramValue(one_click_signin::HISTOGRAM_REJECTED);
       break;
     default:
       NOTREACHED() << "Invalid auto_accept=" << auto_accept_;
       break;
   }
 
   CleanTransientState();
 }
 
-void OneClickSigninHelper::GaiaCredentialsValid() {
-}
-
 void OneClickSigninHelper::OnStateChanged() {
   // No redirect url after sync setup is set, thus no need to watch for sync
   // state changes.
   if (redirect_url_.is_empty())
     return;
 
   content::WebContents* contents = web_contents();
   Profile* profile =
       Profile::FromBrowserContext(contents->GetBrowserContext());
   ProfileSyncService* sync_service =
       ProfileSyncServiceFactory::GetForProfile(profile);
 
   // Sync setup not completed yet.
   if (sync_service->FirstSetupInProgress())
     return;
 
   if (sync_service->sync_initialized()) {
     contents->GetController().LoadURL(redirect_url_,
                                       content::Referrer(),
                                       content::PAGE_TRANSITION_AUTO_TOPLEVEL,
                                       std::string());
   }
 
   // Clear the redirect URL.
   redirect_url_ = GURL();
   sync_service->RemoveObserver(this);
 }
-
-void OneClickSigninHelper::SigninFailed(const GoogleServiceAuthError& error) {
-  if (error_message_.empty() && !error.error_message().empty())
-      error_message_ = error.error_message();
-
-  bool display_bubble = true;
-  if (error_message_.empty()) {
-    switch (error.state()) {
-      case GoogleServiceAuthError::NONE:
-        error_message_.clear();
-        break;
-      case GoogleServiceAuthError::SERVICE_UNAVAILABLE:
-        error_message_ = l10n_util::GetStringUTF8(IDS_SYNC_UNRECOVERABLE_ERROR);
-        break;
-      case GoogleServiceAuthError::REQUEST_CANCELED:
-        // If the user cancelled signin, then no need to display any error
-        // messages or anything - just go back to the NTP.
-        error_message_.clear();
-        display_bubble = false;
-        break;
-      default:
-        error_message_ = l10n_util::GetStringUTF8(IDS_SYNC_ERROR_SIGNING_IN);
-        break;
-    }
-  }
-  ShowSyncConfirmationBubble(display_bubble);
-  signin_tracker_.reset();
-}
-
-void OneClickSigninHelper::SigninSuccess() {
-  ShowSyncConfirmationBubble(true);
-  signin_tracker_.reset();
-}