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 163 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(),
+  std::string pending_email =
+      io_data->reverse_autologin_pending_email()->GetValue();
+  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(
@@ skipping 45 matching lines @@
     const std::pair<std::string, std::string>& pair = pairs[i];
     const std::string& key = pair.first;
     const std::string& value = pair.second;
     if (key == "email") {
       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);
-
   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;
   GURL continue_url;
@@ skipping 57 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;
 
+  // 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;
+    Profile* profile =
+        Profile::FromBrowserContext(web_contents->GetBrowserContext());

[erikwright (departed), 2013/01/18 03:06:54]

This is a bit of an abuse (perhaps harmless) of Preferences.

And in any case, it's not really necessary - presumably you can store this data
in ShowInfoBarIfPossible, in which case you save yourself all the trouble of
moving this data back and forth across threads (and cleaning it up afterwards).

[Roger Tawa OOO till Jul 10th, 2013/01/18 15:21:51]

Are you suggesting I use an std::string member directly in ProfileIOData, which
can be set in ShowInfoBarIfPossible() and read in CanOfferOnIOThreadImpl(),
since both occur on the IO thread?

However, I still need to be able to clear the value when the process is done,
see line 744 below, and this happens on the UI thread.  Maybe I can post to the
IO thread at line 744 to clear the value?

[erikwright (departed), 2013/01/18 15:36:28]

That's basically what I'm suggesting. Here are a few reasons:

1) There are other side effects of using preferences (i.e., this value will be
persisted if the browser quits/crashes during the process, no?) that it would be
best to avoid.
2) The cross-thread marshalling will be explicit, clear, and correlated with the
movement of the logic across threads.
3) I'm concerned about possible surprises - what if the tasks that are updating
the pref member and the tasks that are reading it don't get interleaved exactly
as you expect? You might end up reading a stale value.

Mostly, it just feels like it is unnecessary and a little abusive. #3 is
probably not a problem in practice.

So I'd suggest giving it a try and seeing what the impact on the code is.

[Roger Tawa OOO till Jul 10th, 2013/01/21 15:26:45]

Done.

+    profile->GetPrefs()->SetString(prefs::kReverseAutologinPendingEmail, 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;
   }
 
-  // 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;
-  }
-
   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();
+
+  Profile* profile =
+      Profile::FromBrowserContext(web_contents()->GetBrowserContext());
+  profile->GetPrefs()->SetString(prefs::kReverseAutologinPendingEmail, "");
 }
 
 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();
 }