The User Manager needs to use profile paths not display names for profile switching

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

 // Copyright 2013 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/webui/signin/user_manager_screen_handler.h"
 
 #include "base/bind.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/value_conversions.h"
@@ skipping 93 matching lines @@
   if (avatar_image.Width() <= profiles::kAvatarIconWidth ||
       avatar_image.Height() <= profiles::kAvatarIconHeight ) {
     avatar_image = ui::ResourceBundle::GetSharedInstance().GetImageNamed(
         profiles::GetPlaceholderAvatarIconResourceID());
   }
   gfx::Image resized_image = profiles::GetSizedAvatarIcon(
       avatar_image, is_gaia_picture, kAvatarIconSize, kAvatarIconSize);
   return webui::GetBitmapDataUrl(resized_image.AsBitmap());
 }
 
-size_t GetIndexOfProfileWithEmailAndName(const ProfileInfoCache& info_cache,
-                                         const base::string16& email,
-                                         const base::string16& name) {
+size_t GetIndexOfProfileWithEmail(const ProfileInfoCache& info_cache,
+                                  const base::string16& email) {

[Mike Lerman, 2015/01/13 19:55:41]

Optional simpliciation: Change the email parameter to type std::string and call
base::UTF8ToUTF16() in this method rather than in multiple locations below.

[noms (inactive), 2015/01/13 21:36:42]

Done.

   for (size_t i = 0; i < info_cache.GetNumberOfProfiles(); ++i) {
-    if (info_cache.GetUserNameOfProfileAtIndex(i) == email &&
-        (name.empty() ||
-         profiles::GetAvatarNameForProfile(
-             info_cache.GetPathOfProfileAtIndex(i)) == name)) {
+    if (info_cache.GetUserNameOfProfileAtIndex(i) == email)
       return i;
-    }
   }
   return std::string::npos;
 }
 
 extensions::ScreenlockPrivateEventRouter* GetScreenlockRouter(
     const std::string& email) {
-  ProfileInfoCache& info_cache =
+  const ProfileInfoCache& info_cache =
       g_browser_process->profile_manager()->GetProfileInfoCache();
-  const size_t profile_index = GetIndexOfProfileWithEmailAndName(
-      info_cache, base::UTF8ToUTF16(email), base::string16());
+  const size_t profile_index = GetIndexOfProfileWithEmail(
+      info_cache, base::UTF8ToUTF16(email));
   Profile* profile = g_browser_process->profile_manager()
       ->GetProfileByPath(info_cache.GetPathOfProfileAtIndex(profile_index));
   return extensions::ScreenlockPrivateEventRouter::GetFactoryInstance()->Get(
       profile);
 }
 
 bool IsGuestModeEnabled() {
   PrefService* service = g_browser_process->local_state();
   DCHECK(service);
   return service->GetBoolean(prefs::kBrowserGuestModeEnabled);
@@ skipping 124 matching lines @@
 
 ScreenlockBridge::LockHandler::AuthType UserManagerScreenHandler::GetAuthType(
       const std::string& user_email) const {
   UserAuthTypeMap::const_iterator it = user_auth_type_map_.find(user_email);
   if (it == user_auth_type_map_.end())
     return ScreenlockBridge::LockHandler::OFFLINE_PASSWORD;
   return it->second;
 }
 
 void UserManagerScreenHandler::Unlock(const std::string& user_email) {
-  ProfileInfoCache& info_cache =
+  const ProfileInfoCache& info_cache =
       g_browser_process->profile_manager()->GetProfileInfoCache();
-  const size_t profile_index = GetIndexOfProfileWithEmailAndName(
-      info_cache, base::UTF8ToUTF16(user_email), base::string16());
+  const size_t profile_index = GetIndexOfProfileWithEmail(
+      info_cache, base::UTF8ToUTF16(user_email));
   DCHECK_LT(profile_index, info_cache.GetNumberOfProfiles());
 
   authenticating_profile_index_ = profile_index;
   ReportAuthenticationResult(true, ProfileMetrics::AUTH_LOCAL);
 }
 
 void UserManagerScreenHandler::AttemptEasySignin(
     const std::string& user_email,
     const std::string& secret,
     const std::string& key_label) {
@@ skipping 22 matching lines @@
   }
   profiles::CreateAndSwitchToNewProfile(
       desktop_type_,
       base::Bind(&UserManagerScreenHandler::OnSwitchToProfileComplete,
                  weak_ptr_factory_.GetWeakPtr()),
       ProfileMetrics::ADD_NEW_USER_MANAGER);
 }
 
 void UserManagerScreenHandler::HandleAuthenticatedLaunchUser(
     const base::ListValue* args) {
-  base::string16 email_address;
-  if (!args->GetString(0, &email_address))
+

[Mike Lerman, 2015/01/13 19:55:41]

nit: Remove empty line.

[noms (inactive), 2015/01/13 21:36:42]

Done.

+  const base::Value* profile_path_value;
+  if (!args->Get(0, &profile_path_value))
     return;
 
-  base::string16 display_name;
-  if (!args->GetString(1, &display_name))
+  base::FilePath profile_path;
+  if (!base::GetValueAsFilePath(*profile_path_value, &profile_path))
+    return;
+
+  base::string16 email_address;
+  if (!args->GetString(1, &email_address))
     return;
 
   std::string password;
   if (!args->GetString(2, &password))
     return;
 
-  ProfileInfoCache& info_cache =
+  const ProfileInfoCache& info_cache =
       g_browser_process->profile_manager()->GetProfileInfoCache();
-  size_t profile_index = GetIndexOfProfileWithEmailAndName(
-      info_cache, email_address, display_name);
-  if (profile_index >= info_cache.GetNumberOfProfiles()) {
+  size_t profile_index = info_cache.GetIndexOfProfileWithPath(profile_path);
+
+  if (profile_index == std::string::npos) {
     NOTREACHED();
     return;
   }
 
   authenticating_profile_index_ = profile_index;
   if (!chrome::ValidateLocalAuthCredentials(profile_index, password)) {
     // Make a second attempt via an on-line authentication call.  This handles
     // profiles that are missing sign-in credentials and also cases where the
     // password has been changed externally.
     client_login_.reset(new GaiaAuthFetcher(
         this,
         GaiaConstants::kChromeSource,
         web_ui()->GetWebContents()->GetBrowserContext()->GetRequestContext()));
-    std::string email_string;
-    args->GetString(0, &email_string);
+
     client_login_->StartClientLogin(
-        email_string,
+        base::UTF16ToUTF8(email_address),
         password,
         GaiaConstants::kSyncService,
         std::string(),
         std::string(),
         GaiaAuthFetcher::HostedAccountsAllowed);
     password_attempt_ = password;
     return;
   }
 
   ReportAuthenticationResult(true, ProfileMetrics::AUTH_LOCAL);
@@ skipping 33 matching lines @@
         base::Bind(&UserManagerScreenHandler::OnSwitchToProfileComplete,
                    weak_ptr_factory_.GetWeakPtr()));
   } else {
     // The UI should have prevented the user from allowing the selection of
     // guest mode.
     NOTREACHED();
   }
 }
 
 void UserManagerScreenHandler::HandleLaunchUser(const base::ListValue* args) {
-  base::string16 email_address;
-  base::string16 display_name;
+  const base::Value* profile_path_value;

[Mike Lerman, 2015/01/13 19:55:41]

nit: Initialize profile_path_value to NULL (or nullptr).

[noms (inactive), 2015/01/13 21:36:42]

Done.

+  if (!args->Get(0, &profile_path_value))
+    return;
 
-  if (!args->GetString(0, &email_address) ||
-      !args->GetString(1, &display_name)) {
+  base::FilePath profile_path;
+  if (!base::GetValueAsFilePath(*profile_path_value, &profile_path))
+    return;
+
+  const ProfileInfoCache& info_cache =
+      g_browser_process->profile_manager()->GetProfileInfoCache();
+  size_t profile_index = info_cache.GetIndexOfProfileWithPath(profile_path);
+
+  if (profile_index == std::string::npos) {
     NOTREACHED();
     return;
   }
-
-  ProfileInfoCache& info_cache =
-      g_browser_process->profile_manager()->GetProfileInfoCache();
-  size_t profile_index = GetIndexOfProfileWithEmailAndName(
-      info_cache, email_address, display_name);
-
-  if (profile_index >= info_cache.GetNumberOfProfiles()) {
-    NOTREACHED();
-    return;
-  }
 
   // It's possible that a user breaks into the user-manager page using the
   // JavaScript Inspector and causes a "locked" profile to call this
   // unauthenticated version of "launch" instead of the proper one.  Thus,
   // we have to validate in (secure) C++ code that it really is a profile
   // not needing authentication.  If it is, just ignore the "launch" request.
   if (info_cache.ProfileIsSigninRequiredAtIndex(profile_index))
     return;
   ProfileMetrics::LogProfileAuthResult(ProfileMetrics::AUTH_UNNECESSARY);
 
-  base::FilePath path = info_cache.GetPathOfProfileAtIndex(profile_index);
   profiles::SwitchToProfile(
-      path,
+      profile_path,
       desktop_type_,
       false,  /* reuse any existing windows */
       base::Bind(&UserManagerScreenHandler::OnSwitchToProfileComplete,
                  weak_ptr_factory_.GetWeakPtr()),
       ProfileMetrics::SWITCH_PROFILE_MANAGER);
 }
 
 void UserManagerScreenHandler::HandleAttemptUnlock(
     const base::ListValue* args) {
   std::string email;
@@ skipping 208 matching lines @@
     bool is_active_user = (profile_path == active_profile_path);
 
     profile_value->SetString(
         kKeyUsername, info_cache.GetUserNameOfProfileAtIndex(i));
     profile_value->SetString(
         kKeyEmailAddress, info_cache.GetUserNameOfProfileAtIndex(i));
     // The profiles displayed in the User Manager are never guest profiles.
     profile_value->SetString(
         kKeyDisplayName,
         profiles::GetAvatarNameForProfile(profile_path));
     profile_value->SetString(kKeyProfilePath, profile_path.MaybeAsASCII());

[Mike Lerman, 2015/01/13 19:55:41]

FilePath::MaybeAsASCII() returns std::string() whenever the file path contains
non-ascii characters. If the OS name, for example, contains Unicode characters,
then the Profile Path (usually within C:\Users\<username>) will contain unicode
characters, and this won't work.

You can perhaps use FilePath::BaseName to get only the last part of the file
path (being the profile directory), which should be unique, and then using
LossyDisplayName() instead of MaybeAsASCII()?

[noms (inactive), 2015/01/13 21:36:42]

I used base::CreateFilePathValue(), which is what chrome://settings uses. It's
also paired with a base::GetValueAsFilePath() so everything should be ok. 

[Mike Lerman, 2015/01/14 01:53:17]

Chrome://settings doesn't seem to do anything with the value it gets from
CreateFilePathValue(). I do think this solution should work, but it should be
tested at least manually.

     profile_value->SetBoolean(kKeyPublicAccount, false);
     profile_value->SetBoolean(
         kKeySupervisedUser, info_cache.ProfileIsSupervisedAtIndex(i));
     profile_value->SetBoolean(
         kKeyChildUser, info_cache.ProfileIsChildAtIndex(i));
     profile_value->SetBoolean(kKeySignedIn, is_active_user);
     profile_value->SetBoolean(
         kKeyNeedsSignin, info_cache.ProfileIsSigninRequiredAtIndex(i));
     profile_value->SetBoolean(kKeyIsOwner, false);
     profile_value->SetBoolean(kKeyCanRemove, !active_user_is_supervised);
@@ skipping 12 matching lines @@
       users_list, base::FundamentalValue(IsGuestModeEnabled()));
 }
 
 void UserManagerScreenHandler::ReportAuthenticationResult(
     bool success,
     ProfileMetrics::ProfileAuth auth) {
   ProfileMetrics::LogProfileAuthResult(auth);
   password_attempt_.clear();
 
   if (success) {
-    ProfileInfoCache& info_cache =
+    const ProfileInfoCache& info_cache =
         g_browser_process->profile_manager()->GetProfileInfoCache();
     base::FilePath path = info_cache.GetPathOfProfileAtIndex(
         authenticating_profile_index_);
     profiles::SwitchToProfile(
         path,
         desktop_type_,
         true,
         base::Bind(&UserManagerScreenHandler::OnSwitchToProfileComplete,
                    weak_ptr_factory_.GetWeakPtr()),
         ProfileMetrics::SWITCH_PROFILE_UNLOCK);
@@ skipping 68 matching lines @@
     Profile* profile, Profile::CreateStatus profile_create_status) {
   Browser* browser = chrome::FindAnyBrowser(profile, false, desktop_type_);
   if (browser && browser->window()) {
     OnBrowserWindowReady(browser);
   } else {
     registrar_.Add(this,
                    chrome::NOTIFICATION_BROWSER_WINDOW_READY,
                    content::NotificationService::AllSources());
   }
 }