Fixed misuse of GetProfileByUser in MultiProfileUserController.

chrome/browser/chromeos/login/users/multi_profile_user_controller.cc

 // Copyright 2014 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/chromeos/login/users/multi_profile_user_controller.h"
 
 #include "base/bind.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/prefs/pref_change_registrar.h"
 #include "base/prefs/pref_registry_simple.h"
@@ skipping 76 matching lines @@
       user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
 }
 
 bool MultiProfileUserController::IsUserAllowedInSession(
     const std::string& user_email,
     MultiProfileUserController::UserAllowedInSessionReason* reason) const {
   UserManager* user_manager = UserManager::Get();
   CHECK(user_manager);
 
   const user_manager::User* primary_user = user_manager->GetPrimaryUser();
-  std::string primary_user_email;
-  if (primary_user)
-    primary_user_email = primary_user->email();
 
   // Always allow if there is no primary user or user being checked is the
   // primary user.
-  if (primary_user_email.empty() || primary_user_email == user_email)
+  if (!primary_user || primary_user->email() == user_email)
     return SetUserAllowedReason(reason, ALLOWED);
 
   // Owner is not allowed to be secondary user.
   if (user_manager->GetOwnerEmail() == user_email)
     return SetUserAllowedReason(reason, NOT_ALLOWED_OWNER_AS_SECONDARY);
 
   // Don't allow profiles potentially tainted by data fetched with policy-pushed
   // certificates to join a multiprofile session.
   if (policy::PolicyCertServiceFactory::UsedPolicyCertificates(user_email))
     return SetUserAllowedReason(reason, NOT_ALLOWED_POLICY_CERT_TAINTED);
 
   // Don't allow any secondary profiles if the primary profile is tainted.
   if (policy::PolicyCertServiceFactory::UsedPolicyCertificates(
-          primary_user_email)) {
+          primary_user->email())) {
     // Check directly in local_state before checking if the primary user has
     // a PolicyCertService. His profile may have been tainted previously though
     // he didn't get a PolicyCertService created for this session.
     return SetUserAllowedReason(reason,
                                 NOT_ALLOWED_PRIMARY_POLICY_CERT_TAINTED);
   }
 
-  // If the primary profile already has policy certificates installed but hasn't
-  // used them yet then it can become tainted at any time during this session;
-  // disable secondary profiles in this case too.
-  Profile* primary_user_profile =
-      primary_user ? ProfileHelper::Get()->GetProfileByUserUnsafe(primary_user)
-                   : NULL;
-  policy::PolicyCertService* service =
-      primary_user_profile ? policy::PolicyCertServiceFactory::GetForProfile(
-                                 primary_user_profile)
-                           : NULL;
-  if (service && service->has_policy_certificates())
-    return SetUserAllowedReason(reason,
-                                NOT_ALLOWED_PRIMARY_POLICY_CERT_TAINTED);
+  if (Profile* primary_user_profile =

[Nikita (slow), 2014/08/13 09:16:46]

I worry that if someone cals IsUserAllowedInSession() early when profile is not
available yet he will get cached value i.e. ignoring for instance
NOT_ALLOWED_PRIMARY_POLICY_CERT_TAINTED /
NOT_ALLOWED_PRIMARY_USER_POLICY_FORBIDS.

Can you make sure that this value is not cached by caller and once profile is
available it will still be checked?

[dzhioev (left Google), 2014/08/13 16:41:13]

Checked, nobody caches this value. 

+          ProfileHelper::Get()->GetProfileByUser(primary_user)) {
+    // If the primary profile already has policy certificates installed but
+    // hasn't  used them yet then it can become tainted at any time during this
+    // session disable secondary profiles in this case too.
+    if (policy::PolicyCertService* service =
+            policy::PolicyCertServiceFactory::GetForProfile(
+                 primary_user_profile)) {
+      if (service->has_policy_certificates()) {
+        return SetUserAllowedReason(reason,
+                                    NOT_ALLOWED_PRIMARY_POLICY_CERT_TAINTED);
+      }
+    }
 
-  // No user is allowed if the primary user policy forbids it.
-  const std::string primary_user_behavior =
-      primary_user_profile->GetPrefs()->GetString(
-          prefs::kMultiProfileUserBehavior);
-  if (primary_user_behavior == kBehaviorNotAllowed)
-    return SetUserAllowedReason(reason,
-                                NOT_ALLOWED_PRIMARY_USER_POLICY_FORBIDS);
+    // No user is allowed if the primary user policy forbids it.
+    const std::string primary_user_behavior =
+        primary_user_profile->GetPrefs()->GetString(
+            prefs::kMultiProfileUserBehavior);
+    if (primary_user_behavior == kBehaviorNotAllowed) {
+      return SetUserAllowedReason(reason,
+                                  NOT_ALLOWED_PRIMARY_USER_POLICY_FORBIDS);
+    }
+  }
 
   // The user must have 'unrestricted' policy to be a secondary user.
   const std::string behavior = GetCachedValue(user_email);
   return SetUserAllowedReason(
       reason,
       behavior == kBehaviorUnrestricted ? ALLOWED : NOT_ALLOWED_POLICY_FORBIDS);
 }
 
 void MultiProfileUserController::StartObserving(Profile* user_profile) {
   // Profile name could be empty during tests.
@@ skipping 69 matching lines @@
   } else {
     const std::string behavior =
         prefs->GetString(prefs::kMultiProfileUserBehavior);
     SetCachedValue(user_email, behavior);
   }
 
   CheckSessionUsers();
 }
 
 }  // namespace chromeos