Refactoring IsUserAllowedInSession and GetCachedValue

BUG=388279

Review URL: https://codereview.chromium.org/420243002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@287522 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/chromeos/login/screens/user_selection_screen.cc

@@ -88,11 +88,19 @@ void UserSelectionScreen::FillUserDictionary(
   if (is_signin_to_add) {
     MultiProfileUserController* multi_profile_user_controller =
         UserManager::Get()->GetMultiProfileUserController();
-    std::string behavior =
-        multi_profile_user_controller->GetCachedValue(user_id);
-    user_dict->SetBoolean(kKeyMultiProfilesAllowed,
-                          multi_profile_user_controller->IsUserAllowedInSession(
-                              user_id) == MultiProfileUserController::ALLOWED);
+    MultiProfileUserController::UserAllowedInSessionReason isUserAllowedReason;
+    bool isUserAllowed = multi_profile_user_controller->IsUserAllowedInSession(
+        user_id, &isUserAllowedReason);
+    user_dict->SetBoolean(kKeyMultiProfilesAllowed, isUserAllowed);
+
+    std::string behavior;
+    switch (isUserAllowedReason) {
+      case MultiProfileUserController::NOT_ALLOWED_OWNER_AS_SECONDARY:
+        behavior = MultiProfileUserController::kBehaviorOwnerPrimaryOnly;
+        break;
+      default:
+        behavior = multi_profile_user_controller->GetCachedValue(user_id);
+    }
     user_dict->SetString(kKeyMultiProfilesPolicy, behavior);
   } else {
     user_dict->SetBoolean(kKeyMultiProfilesAllowed, true);

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

@@ -198,9 +198,9 @@ user_manager::UserList ChromeUserManager::GetUsersAdmittedForMultiProfile()
        ++it) {
     if ((*it)->GetType() == user_manager::USER_TYPE_REGULAR &&
         !(*it)->is_logged_in()) {
-      MultiProfileUserController::UserAllowedInSessionResult check =
-          multi_profile_user_controller_->IsUserAllowedInSession(
-              (*it)->email());
+      MultiProfileUserController::UserAllowedInSessionReason check;
+      multi_profile_user_controller_->IsUserAllowedInSession((*it)->email(),
+                                                             &check);
       if (check ==
           MultiProfileUserController::NOT_ALLOWED_PRIMARY_USER_POLICY_FORBIDS) {
         return user_manager::UserList();

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

@@ -28,14 +28,21 @@ namespace {
 std::string SanitizeBehaviorValue(const std::string& value) {
   if (value == MultiProfileUserController::kBehaviorUnrestricted ||
       value == MultiProfileUserController::kBehaviorPrimaryOnly ||
-      value == MultiProfileUserController::kBehaviorNotAllowed ||
-      value == MultiProfileUserController::kBehaviorOwnerPrimaryOnly) {
+      value == MultiProfileUserController::kBehaviorNotAllowed) {
     return value;
   }
 
   return std::string(MultiProfileUserController::kBehaviorUnrestricted);
 }
 
+bool SetUserAllowedReason(
+    MultiProfileUserController::UserAllowedInSessionReason* reason,
+    MultiProfileUserController::UserAllowedInSessionReason value) {
+  if (reason)
+    *reason = value;
+  return value == MultiProfileUserController::ALLOWED;
+}
+
 }  // namespace
 
 // static
@@ -80,9 +87,9 @@ void MultiProfileUserController::RegisterProfilePrefs(
       user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
 }
 
-MultiProfileUserController::UserAllowedInSessionResult
-MultiProfileUserController::IsUserAllowedInSession(
-    const std::string& user_email) const {
+bool MultiProfileUserController::IsUserAllowedInSession(
+    const std::string& user_email,
+    MultiProfileUserController::UserAllowedInSessionReason* reason) const {
   UserManager* user_manager = UserManager::Get();
   CHECK(user_manager);
 
@@ -94,16 +101,16 @@ MultiProfileUserController::IsUserAllowedInSession(
   // 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)
-    return ALLOWED;
+    return SetUserAllowedReason(reason, ALLOWED);
 
   // Owner is not allowed to be secondary user.
   if (user_manager->GetOwnerEmail() == user_email)
-    return NOT_ALLOWED_OWNER_AS_SECONDARY;
+    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 NOT_ALLOWED_POLICY_CERT_TAINTED;
+    return SetUserAllowedReason(reason, NOT_ALLOWED_POLICY_CERT_TAINTED);
 
   // Don't allow any secondary profiles if the primary profile is tainted.
   if (policy::PolicyCertServiceFactory::UsedPolicyCertificates(
@@ -111,7 +118,8 @@ MultiProfileUserController::IsUserAllowedInSession(
     // 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 NOT_ALLOWED_PRIMARY_POLICY_CERT_TAINTED;
+    return SetUserAllowedReason(reason,
+                                NOT_ALLOWED_PRIMARY_POLICY_CERT_TAINTED);
   }
 
   // If the primary profile already has policy certificates installed but hasn't
@@ -125,19 +133,22 @@ MultiProfileUserController::IsUserAllowedInSession(
                                  primary_user_profile)
                            : NULL;
   if (service && service->has_policy_certificates())
-    return NOT_ALLOWED_PRIMARY_POLICY_CERT_TAINTED;
+    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 NOT_ALLOWED_PRIMARY_USER_POLICY_FORBIDS;
+    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 behavior == kBehaviorUnrestricted ? ALLOWED :
-                                             NOT_ALLOWED_POLICY_FORBIDS;
+  return SetUserAllowedReason(
+      reason,
+      behavior == kBehaviorUnrestricted ? ALLOWED : NOT_ALLOWED_POLICY_FORBIDS);
 }
 
 void MultiProfileUserController::StartObserving(Profile* user_profile) {
@@ -173,10 +184,6 @@ std::string MultiProfileUserController::GetCachedValue(
   if (dict && dict->GetStringWithoutPathExpansion(user_email, &value))
     return SanitizeBehaviorValue(value);
 
-  // Owner is not allowed to be secondary user (see http://crbug.com/385034).
-  if (UserManager::Get()->GetOwnerEmail() == user_email)
-    return std::string(kBehaviorOwnerPrimaryOnly);
-
   return std::string(kBehaviorUnrestricted);
 }
 
@@ -194,7 +201,7 @@ void MultiProfileUserController::CheckSessionUsers() {
   for (user_manager::UserList::const_iterator it = users.begin();
        it != users.end();
        ++it) {
-    if (IsUserAllowedInSession((*it)->email()) != ALLOWED) {
+    if (!IsUserAllowedInSession((*it)->email(), NULL)) {
       delegate_->OnUserNotAllowed((*it)->email());
       return;
     }

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

@@ -30,8 +30,8 @@ class UserManager;
 // user login and checks if the meaning of the value is respected.
 class MultiProfileUserController {
  public:
-  // Return value of IsUserAllowedInSession().
-  enum UserAllowedInSessionResult {
+  // Second return value of IsUserAllowedInSession().
+  enum UserAllowedInSessionReason {
     // User is allowed in multi-profile session.
     ALLOWED,
 
@@ -65,10 +65,10 @@ class MultiProfileUserController {
   // Returns the cached policy value for |user_email|.
   std::string GetCachedValue(const std::string& user_email) const;
 
-  // Returns UserAllowedInSessionResult enum that describe whether the user is
-  // allowed to be in the current session.
-  UserAllowedInSessionResult IsUserAllowedInSession(
-      const std::string& user_email) const;
+  // Returns true if user allowed to be in the current session. If |reason| not
+  // null stores UserAllowedInSessionReason enum that describes actual reason.
+  bool IsUserAllowedInSession(const std::string& user_email,
+                              UserAllowedInSessionReason* reason) const;
 
   // Starts to observe the multiprofile user behavior pref of the given profile.
   void StartObserving(Profile* user_profile);

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

@@ -34,7 +34,7 @@ const char* kUsers[] = {"a@gmail.com", "b@gmail.com" };
 struct BehaviorTestCase {
   const char* primary;
   const char* secondary;
-  MultiProfileUserController::UserAllowedInSessionResult expected_allowed;
+  MultiProfileUserController::UserAllowedInSessionReason expected_allowed;
 };
 
 const BehaviorTestCase kBehaviorTestCases[] = {
@@ -212,9 +212,10 @@ TEST_F(MultiProfileUserControllerTest, AllAllowedBeforeLogin) {
   };
   for (size_t i = 0; i < arraysize(kTestCases); ++i) {
     SetCachedBehavior(0, kTestCases[i]);
-    EXPECT_EQ(MultiProfileUserController::ALLOWED,
-              controller()->IsUserAllowedInSession(kUsers[0]))
+    MultiProfileUserController::UserAllowedInSessionReason reason;
+    EXPECT_TRUE(controller()->IsUserAllowedInSession(kUsers[0], &reason))
         << "Case " << i;
+    EXPECT_EQ(MultiProfileUserController::ALLOWED, reason) << "Case " << i;
   }
 }
 
@@ -271,8 +272,9 @@ TEST_F(MultiProfileUserControllerTest, IsSecondaryAllowed) {
   for (size_t i = 0; i < arraysize(kBehaviorTestCases); ++i) {
     SetPrefBehavior(0, kBehaviorTestCases[i].primary);
     SetCachedBehavior(1, kBehaviorTestCases[i].secondary);
-    EXPECT_EQ(kBehaviorTestCases[i].expected_allowed,
-              controller()->IsUserAllowedInSession(kUsers[1])) << "Case " << i;
+    MultiProfileUserController::UserAllowedInSessionReason reason;
+    controller()->IsUserAllowedInSession(kUsers[1], &reason);
+    EXPECT_EQ(kBehaviorTestCases[i].expected_allowed, reason) << "Case " << i;
   }
 }
 
@@ -303,10 +305,9 @@ TEST_F(MultiProfileUserControllerTest, NoSecondaryOwner) {
   LoginUser(0);
   SetOwner(1);
 
-  EXPECT_EQ(MultiProfileUserController::NOT_ALLOWED_OWNER_AS_SECONDARY,
-            controller()->IsUserAllowedInSession(kUsers[1]));
-  EXPECT_EQ(MultiProfileUserController::kBehaviorOwnerPrimaryOnly,
-            GetCachedBehavior(1));
+  MultiProfileUserController::UserAllowedInSessionReason reason;
+  EXPECT_FALSE(controller()->IsUserAllowedInSession(kUsers[1], &reason));
+  EXPECT_EQ(MultiProfileUserController::NOT_ALLOWED_OWNER_AS_SECONDARY, reason);
 
   EXPECT_EQ(0, user_not_allowed_count());
   LoginUser(1);
@@ -318,10 +319,11 @@ TEST_F(MultiProfileUserControllerTest,
   // Verifies that any user can sign-in as the primary user, regardless of the
   // tainted state.
   policy::PolicyCertServiceFactory::SetUsedPolicyCertificates(kUsers[0]);
-  EXPECT_EQ(MultiProfileUserController::ALLOWED,
-            controller()->IsUserAllowedInSession(kUsers[0]));
-  EXPECT_EQ(MultiProfileUserController::ALLOWED,
-            controller()->IsUserAllowedInSession(kUsers[1]));
+  MultiProfileUserController::UserAllowedInSessionReason reason;
+  EXPECT_TRUE(controller()->IsUserAllowedInSession(kUsers[0], &reason));
+  EXPECT_EQ(MultiProfileUserController::ALLOWED, reason);
+  EXPECT_TRUE(controller()->IsUserAllowedInSession(kUsers[1], &reason));
+  EXPECT_EQ(MultiProfileUserController::ALLOWED, reason);
 }
 
 TEST_F(MultiProfileUserControllerTest,
@@ -334,11 +336,14 @@ TEST_F(MultiProfileUserControllerTest,
   // changed back to enabled.
   SetPrefBehavior(1, MultiProfileUserController::kBehaviorUnrestricted);
 
-  EXPECT_EQ(MultiProfileUserController::ALLOWED,
-            controller()->IsUserAllowedInSession(kUsers[0]));
+  MultiProfileUserController::UserAllowedInSessionReason reason;
+  EXPECT_TRUE(controller()->IsUserAllowedInSession(kUsers[0], &reason));
+  EXPECT_EQ(MultiProfileUserController::ALLOWED, reason);
+
   policy::PolicyCertServiceFactory::SetUsedPolicyCertificates(kUsers[0]);
+  EXPECT_FALSE(controller()->IsUserAllowedInSession(kUsers[0], &reason));
   EXPECT_EQ(MultiProfileUserController::NOT_ALLOWED_POLICY_CERT_TAINTED,
-            controller()->IsUserAllowedInSession(kUsers[0]));
+            reason);
 }
 
 TEST_F(MultiProfileUserControllerTest,
@@ -354,11 +359,14 @@ TEST_F(MultiProfileUserControllerTest,
       policy::PolicyCertServiceFactory::GetInstance()->SetTestingFactoryAndUse(
           profile(0), TestPolicyCertServiceFactory));
 
+  MultiProfileUserController::UserAllowedInSessionReason reason;
+  EXPECT_FALSE(controller()->IsUserAllowedInSession(kUsers[1], &reason));
   EXPECT_EQ(MultiProfileUserController::NOT_ALLOWED_PRIMARY_POLICY_CERT_TAINTED,
-            controller()->IsUserAllowedInSession(kUsers[1]));
+            reason);
   policy::PolicyCertServiceFactory::SetUsedPolicyCertificates(kUsers[1]);
+  EXPECT_FALSE(controller()->IsUserAllowedInSession(kUsers[1], &reason));
   EXPECT_EQ(MultiProfileUserController::NOT_ALLOWED_POLICY_CERT_TAINTED,
-            controller()->IsUserAllowedInSession(kUsers[1]));
+            reason);
 
   // Flush tasks posted to IO.
   base::RunLoop().RunUntilIdle();
@@ -384,16 +392,18 @@ TEST_F(MultiProfileUserControllerTest,
   ASSERT_TRUE(service);
 
   EXPECT_FALSE(service->has_policy_certificates());
-  EXPECT_EQ(MultiProfileUserController::ALLOWED,
-            controller()->IsUserAllowedInSession(kUsers[1]));
+  MultiProfileUserController::UserAllowedInSessionReason reason;
+  EXPECT_TRUE(controller()->IsUserAllowedInSession(kUsers[1], &reason));
+  EXPECT_EQ(MultiProfileUserController::ALLOWED, reason);
 
   net::CertificateList certificates;
   certificates.push_back(new net::X509Certificate(
       "subject", "issuer", base::Time(), base::Time()));
   service->OnTrustAnchorsChanged(certificates);
   EXPECT_TRUE(service->has_policy_certificates());
+  EXPECT_FALSE(controller()->IsUserAllowedInSession(kUsers[1], &reason));
   EXPECT_EQ(MultiProfileUserController::NOT_ALLOWED_PRIMARY_POLICY_CERT_TAINTED,
-            controller()->IsUserAllowedInSession(kUsers[1]));
+            reason);
 
   // Flush tasks posted to IO.
   base::RunLoop().RunUntilIdle();