Don't use request user data during one-click sign in

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 105 matching lines @@
       break;
     default:
       NOTREACHED() << "Invalid auto_accept: " << auto_accept;
       break;
   }
 
   UMA_HISTOGRAM_ENUMERATION("AutoLogin.Reverse", action,
                             one_click_signin::HISTOGRAM_MAX);
 }
 
+void ClearPendingEmailOnIOThread(content::ResourceContext* context) {
+  ProfileIOData* io_data = ProfileIOData::FromResourceContext(context);
+  DCHECK(io_data);
+  if (io_data)

[tim (not reviewing), 2013/01/23 02:07:52]

Is there a reason you're handling this this way?  Convention would say instead
of DCHECK(ptr) if (ptr), you would just do ptr->blah(), since the way it is
written now seems to imply that the DCHECK can in fact fail, which is
contradictory.  

See the 'Coding Style' section on dev.chromium.org, specifically the following
passage - "... A consequence of this is that you should not handle DCHECK()
failures, even if failure would result in a crash. Attempting to handle a
DCHECK() failure is a statement that the DCHECK() can fail, which contradicts
the point of writing the DCHECK()."

[Roger Tawa OOO till Jul 10th, 2013/01/25 20:14:10]

Done.

+    io_data->set_reverse_autologin_pending_email("");

[tim (not reviewing), 2013/01/23 02:07:52]

nit - prefer std::string() to "", as it is more explicit / clear, and more
efficient.

[Roger Tawa OOO till Jul 10th, 2013/01/25 20:14:10]

Done.

[Roger Tawa OOO till Jul 10th, 2013/01/25 20:14:10]

Done.

+}
+
 // Determines the source of the sign in and the continue URL.  Its either one
 // of the known sign in access point (first run, NTP, menu, settings) or its
 // an implicit sign in via another Google property.  In the former case,
 // "service" is also checked to make sure its "chromiumsync".
 SyncPromoUI::Source GetSigninSource(const GURL& url, GURL* continue_url) {
   std::string value;
   chrome_common_net::GetValueForKeyInQuery(url, "service", &value);
   bool possibly_an_explicit_signin = value == "chromiumsync";
 
   // Find the final continue URL for this sign in.  In some cases, Gaia can
@@ skipping 38 matching lines @@
   }
 
   GURL origin = url.GetOrigin();
   if (origin == GURL("https://accounts.youtube.com") ||
       origin == GURL("https://accounts.blogger.com"))
     return true;
 
   return false;
 }
 
-// This class is associated as user data with a given URLRequest object, in
-// order to pass information from one response to another during the process
-// of signing the user into their Gaia account.  This class is only meant
-// to be used from the IO thread.
-class OneClickSigninRequestUserData : public base::SupportsUserData::Data {
- public:
-  const std::string& email() const { return email_; }
-
-  // Associates signin information with the request.  Overwrites existing
-  // information if any.
-  static void AssociateWithRequest(base::SupportsUserData* request,
-                                   const std::string& email);
-
-  // Gets the one-click sign in information associated with the request.
-  static OneClickSigninRequestUserData* FromRequest(
-      base::SupportsUserData* request);
-
- private:
-  // Key used when setting this object on the request.
-  static const void* const kUserDataKey;
-
-  explicit OneClickSigninRequestUserData(const std::string& email)
-      : email_(email) {
-  }
-
-  std::string email_;
-
-  DISALLOW_COPY_AND_ASSIGN(OneClickSigninRequestUserData);
-};
-
-// static
-void OneClickSigninRequestUserData::AssociateWithRequest(
-    base::SupportsUserData* request,
-    const std::string& email) {
-  request->SetUserData(kUserDataKey, new OneClickSigninRequestUserData(email));
-}
-
-// static
-OneClickSigninRequestUserData* OneClickSigninRequestUserData::FromRequest(
-    base::SupportsUserData* request) {
-  return static_cast<OneClickSigninRequestUserData*>(
-      request->GetUserData(kUserDataKey));
-}
-
-const void* const OneClickSigninRequestUserData::kUserDataKey =
-    static_cast<const void* const>(
-        &OneClickSigninRequestUserData::kUserDataKey);
-
 }  // namespace
 
 // The infobar asking the user if they want to use one-click sign in.
 // TODO(rogerta): once we move to a web-based sign in flow, we can get rid
 // of this infobar.
 class OneClickInfoBarDelegateImpl : public OneClickSigninInfoBarDelegate {
  public:
   // Creates a one click signin delegate and adds it to |infobar_service|.
   static void Create(InfoBarService* infobar_service,
                      const std::string& session_index,
@@ skipping 160 matching lines @@
 OneClickSigninHelper::OneClickSigninHelper(content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents),
       auto_accept_(AUTO_ACCEPT_NONE),
       source_(SyncPromoUI::SOURCE_UNKNOWN) {
 }
 
 OneClickSigninHelper::~OneClickSigninHelper() {
 }
 
 // static
-void OneClickSigninHelper::AssociateWithRequestForTesting(
-    base::SupportsUserData* request,
-    const std::string& email) {
-  OneClickSigninRequestUserData::AssociateWithRequest(request, email);
-}
-
-// static
 bool OneClickSigninHelper::CanOffer(content::WebContents* web_contents,
                                     CanOfferFor can_offer_for,
                                     const std::string& email,
                                     int* error_message_id) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
     VLOG(1) << "OneClickSigninHelper::CanOffer";
 
   if (error_message_id)
     *error_message_id = 0;
 
@@ skipping 128 matching lines @@
 
   if (!SigninManager::AreSigninCookiesAllowed(io_data->GetCookieSettings()))
     return DONT_OFFER;
 
   // The checks below depend on chrome already knowing what account the user
   // signed in with.  This happens only after receiving the response containing
   // the Google-Accounts-SignIn header.  Until then, if there is even a chance
   // that we want to connect the profile, chrome needs to tell Gaia that
   // it should offer the interstitial.  Therefore missing one click data on
   // the request means can offer is true.
-  OneClickSigninRequestUserData* one_click_data =
-      OneClickSigninRequestUserData::FromRequest(request);
-  if (one_click_data) {
-    if (!SigninManager::IsAllowedUsername(one_click_data->email(),
+  const std::string& pending_email = io_data->reverse_autologin_pending_email();
+  if (!pending_email.empty()) {
+    if (!SigninManager::IsAllowedUsername(pending_email,
             io_data->google_services_username_pattern()->GetValue())) {
       return DONT_OFFER;
     }
 
     std::vector<std::string> rejected_emails =
         io_data->one_click_signin_rejected_email_list()->GetValue();
     if (std::count_if(rejected_emails.begin(), rejected_emails.end(),
                       std::bind2nd(std::equal_to<std::string>(),
-                                   one_click_data->email())) > 0) {
+                                   pending_email)) > 0) {
       return DONT_OFFER;
     }
 
     if (io_data->signin_names()->GetEmails().count(
-            UTF8ToUTF16(one_click_data->email())) > 0) {
+            UTF8ToUTF16(pending_email)) > 0) {
       return DONT_OFFER;
     }
   }
 
   return CAN_OFFER;
 }
 
 // static
 void OneClickSigninHelper::InitializeFieldTrial() {
   scoped_refptr<base::FieldTrial> trial(
       base::FieldTrialList::FactoryGetFieldTrial("OneClickSignIn", 100,
                                                  "Standard", 2013, 9, 1, NULL));
 
   // For dev and beta, we'll give half the people the new experience.  For
   // stable, only 1%.  These numbers are overridable on the server.
   const bool kIsStableChannel =
       chrome::VersionInfo::GetChannel() == chrome::VersionInfo::CHANNEL_STABLE;
   const int kBlueOnWhiteGroup = trial->AppendGroup("BlueOnWhite",
                                                    kIsStableChannel ? 1 : 50);
   use_blue_on_white = trial->group() == kBlueOnWhiteGroup;
 }
 
 // static
 void OneClickSigninHelper::ShowInfoBarIfPossible(net::URLRequest* request,
+                                                 ProfileIOData* io_data,
                                                  int child_id,
                                                  int route_id) {
   std::string google_chrome_signin_value;
   std::string google_accounts_signin_value;
   request->GetResponseHeaderByName("Google-Chrome-SignIn",
                                    &google_chrome_signin_value);
   request->GetResponseHeaderByName("Google-Accounts-SignIn",
                                    &google_accounts_signin_value);
 
   if (!google_accounts_signin_value.empty() ||
@@ skipping 25 matching lines @@
       TrimString(value, "\"", &email);
     } else if (key == "sessionindex") {
       session_index = value;
     }
   }
 
   // Later in the chain of this request, we'll need to check the email address
   // in the IO thread (see CanOfferOnIOThread).  So save the email address as
   // user data on the request (only for web-based flow).
   if (SyncPromoUI::UseWebBasedSigninFlow() && !email.empty())
-    OneClickSigninRequestUserData::AssociateWithRequest(request, email);
+    io_data->set_reverse_autologin_pending_email(email);
 
   if (!email.empty() || !session_index.empty()) {
     VLOG(1) << "OneClickSigninHelper::ShowInfoBarIfPossible:"
             << " email=" << email
             << " sessionindex=" << session_index;
   }
 
   // Parse Google-Chrome-SignIn.
   AutoAccept auto_accept = AUTO_ACCEPT_NONE;
   SyncPromoUI::Source source = SyncPromoUI::SOURCE_UNKNOWN;
@@ skipping 58 matching lines @@
 
   // TODO(mathp): The appearance of this infobar should be tested using a
   // browser_test.
   OneClickSigninHelper* helper =
       OneClickSigninHelper::FromWebContents(web_contents);
   if (!helper)
     return;
 
   int error_message_id = 0;
 
-  CanOfferFor can_offer_for =
-      (auto_accept != AUTO_ACCEPT_EXPLICIT &&
-          helper->auto_accept_ != AUTO_ACCEPT_EXPLICIT) ?
-          CAN_OFFER_FOR_INTERSTITAL_ONLY : CAN_OFFER_FOR_ALL;
-
-  if (!web_contents || !CanOffer(web_contents, can_offer_for, email,
-                                 &error_message_id)) {
-    VLOG(1) << "OneClickSigninHelper::ShowInfoBarUIThread: not offering";
-    if (helper && helper->error_message_.empty() && error_message_id != 0)
-      helper->error_message_ = l10n_util::GetStringUTF8(error_message_id);
-
-    return;
-  }
-
   // 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 (auto_accept != AUTO_ACCEPT_NONE) {
     helper->auto_accept_ = auto_accept;
     helper->source_ = source;
   }
 
+  CanOfferFor can_offer_for =
+      (auto_accept != AUTO_ACCEPT_EXPLICIT &&
+          helper->auto_accept_ != AUTO_ACCEPT_EXPLICIT) ?
+          CAN_OFFER_FOR_INTERSTITAL_ONLY : CAN_OFFER_FOR_ALL;
+
+  if (!web_contents || !CanOffer(web_contents, can_offer_for, email,
+                                 &error_message_id)) {
+    VLOG(1) << "OneClickSigninHelper::ShowInfoBarUIThread: not offering";
+    if (helper && helper->error_message_.empty() && error_message_id != 0)
+      helper->error_message_ = l10n_util::GetStringUTF8(error_message_id);
+
+    return;
+  }
+
   if (continue_url.is_valid()) {
     // When Gaia finally redirects to the continue URL, Gaia will add some
     // extra query parameters.  So ignore the parameters when checking to see
     // if the user has continued.
     GURL::Replacements replacements;
     replacements.ClearQuery();
     helper->continue_url_ = continue_url.ReplaceComponents(replacements);
   }
 }
 
@@ skipping 17 matching lines @@
   signin_tracker_.reset();
 }
 
 void OneClickSigninHelper::CleanTransientState() {
   VLOG(1) << "OneClickSigninHelper::CleanTransientState";
   email_.clear();
   password_.clear();
   auto_accept_ = AUTO_ACCEPT_NONE;
   source_ = SyncPromoUI::SOURCE_UNKNOWN;
   continue_url_ = GURL();
+
+  // 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())));
 }
 
 void OneClickSigninHelper::DidNavigateAnyFrame(
     const content::LoadCommittedDetails& details,
     const content::FrameNavigateParams& params) {
   // We only need to scrape the password for Gaia logins.
   const content::PasswordForm& form = params.password_form;
   if (form.origin.is_valid() &&
       gaia::IsGaiaSignonRealm(GURL(form.signon_realm))) {
     VLOG(1) << "OneClickSigninHelper::DidNavigateAnyFrame: got password";
@@ skipping 163 matching lines @@
         break;
     }
   }
 
   RedirectToNTP();
 }
 
 void OneClickSigninHelper::SigninSuccess() {
   RedirectToNTP();
 }