Providing more information on why certain users can't be added to multi-profile

chrome/browser/chromeos/login/users/multi_profile_user_controller_unittest.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/memory/scoped_ptr.h"
 #include "base/run_loop.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/chromeos/login/users/fake_user_manager.h"
@@ skipping 17 matching lines @@
 namespace chromeos {
 
 namespace {
 
 const char* kUsers[] = {"a@gmail.com", "b@gmail.com" };
 
 struct BehaviorTestCase {
   const char* primary;
   const char* secondary;
   MultiProfileUserController::UserAllowedInSessionReason expected_allowed;
+  MultiProfileUserController::UserAllowedInSessionReason

[James Cook, 2014/08/12 20:08:30]

I would reorder these two member variables. Then the test cases read like this:

primary behavior,
secondary behavior,
was it allowed initially?
was it allowed in the end?

Otherwise you get weird test data like line 74, which reads like "it was blocked
but then it was re-allowed".

[Roman Sorokin (ftl), 2014/08/13 09:11:52]

Done.

+      expected_primary_policy;
 };
 
 const BehaviorTestCase kBehaviorTestCases[] = {
-  {
-    MultiProfileUserController::kBehaviorUnrestricted,
-    MultiProfileUserController::kBehaviorUnrestricted,
-    MultiProfileUserController::ALLOWED,
-  },
-  {
-    MultiProfileUserController::kBehaviorUnrestricted,
-    MultiProfileUserController::kBehaviorPrimaryOnly,
-    MultiProfileUserController::NOT_ALLOWED_POLICY_FORBIDS,
-  },
-  {
-    MultiProfileUserController::kBehaviorUnrestricted,
-    MultiProfileUserController::kBehaviorNotAllowed,
-    MultiProfileUserController::NOT_ALLOWED_POLICY_FORBIDS,
-  },
-  {
-    MultiProfileUserController::kBehaviorPrimaryOnly,
-    MultiProfileUserController::kBehaviorUnrestricted,
-    MultiProfileUserController::ALLOWED,
-  },
-  {
-    MultiProfileUserController::kBehaviorPrimaryOnly,
-    MultiProfileUserController::kBehaviorPrimaryOnly,
-    MultiProfileUserController::NOT_ALLOWED_POLICY_FORBIDS,
-  },
-  {
-    MultiProfileUserController::kBehaviorPrimaryOnly,
-    MultiProfileUserController::kBehaviorNotAllowed,
-    MultiProfileUserController::NOT_ALLOWED_POLICY_FORBIDS,
-  },
-  {
-    MultiProfileUserController::kBehaviorNotAllowed,
-    MultiProfileUserController::kBehaviorUnrestricted,
-    MultiProfileUserController::NOT_ALLOWED_PRIMARY_USER_POLICY_FORBIDS,
-  },
-  {
-    MultiProfileUserController::kBehaviorNotAllowed,
-    MultiProfileUserController::kBehaviorPrimaryOnly,
-    MultiProfileUserController::NOT_ALLOWED_PRIMARY_USER_POLICY_FORBIDS,
-  },
-  {
-    MultiProfileUserController::kBehaviorNotAllowed,
-    MultiProfileUserController::kBehaviorNotAllowed,
-    MultiProfileUserController::NOT_ALLOWED_PRIMARY_USER_POLICY_FORBIDS,
-  },
+    {
+     MultiProfileUserController::kBehaviorUnrestricted,

[James Cook, 2014/08/12 20:08:29]

Is this the indentation "git cl format" uses? It looks odd, but if that's what
the auto-formatter does then use it. If not, please run it through the
formatter.

[Roman Sorokin (ftl), 2014/08/13 09:11:52]

Yes, it is auto-formatter
On 2014/08/12 20:08:29, James Cook wrote:

+     MultiProfileUserController::kBehaviorUnrestricted,
+     MultiProfileUserController::ALLOWED, MultiProfileUserController::ALLOWED,
+    },
+    {
+     MultiProfileUserController::kBehaviorUnrestricted,
+     MultiProfileUserController::kBehaviorPrimaryOnly,
+     MultiProfileUserController::NOT_ALLOWED_POLICY_FORBIDS,
+     MultiProfileUserController::ALLOWED,
+    },
+    {
+     MultiProfileUserController::kBehaviorUnrestricted,
+     MultiProfileUserController::kBehaviorNotAllowed,
+     MultiProfileUserController::NOT_ALLOWED_POLICY_FORBIDS,
+     MultiProfileUserController::ALLOWED,
+    },
+    {
+     MultiProfileUserController::kBehaviorPrimaryOnly,
+     MultiProfileUserController::kBehaviorUnrestricted,
+     MultiProfileUserController::ALLOWED, MultiProfileUserController::ALLOWED,
+    },
+    {
+     MultiProfileUserController::kBehaviorPrimaryOnly,
+     MultiProfileUserController::kBehaviorPrimaryOnly,
+     MultiProfileUserController::NOT_ALLOWED_POLICY_FORBIDS,
+     MultiProfileUserController::ALLOWED,
+    },
+    {
+     MultiProfileUserController::kBehaviorPrimaryOnly,
+     MultiProfileUserController::kBehaviorNotAllowed,
+     MultiProfileUserController::NOT_ALLOWED_POLICY_FORBIDS,
+     MultiProfileUserController::ALLOWED,
+    },
+    {
+     MultiProfileUserController::kBehaviorNotAllowed,
+     MultiProfileUserController::kBehaviorUnrestricted,
+     MultiProfileUserController::NOT_ALLOWED_PRIMARY_USER_POLICY_FORBIDS,
+     MultiProfileUserController::NOT_ALLOWED_PRIMARY_USER_POLICY_FORBIDS,
+    },
+    {
+     MultiProfileUserController::kBehaviorNotAllowed,
+     MultiProfileUserController::kBehaviorPrimaryOnly,
+     MultiProfileUserController::NOT_ALLOWED_PRIMARY_USER_POLICY_FORBIDS,
+     MultiProfileUserController::NOT_ALLOWED_PRIMARY_USER_POLICY_FORBIDS,
+    },
+    {
+     MultiProfileUserController::kBehaviorNotAllowed,
+     MultiProfileUserController::kBehaviorNotAllowed,
+     MultiProfileUserController::NOT_ALLOWED_PRIMARY_USER_POLICY_FORBIDS,
+     MultiProfileUserController::NOT_ALLOWED_PRIMARY_USER_POLICY_FORBIDS,
+    },
 };
 
 // Weak ptr to PolicyCertVerifier - object is freed in test destructor once
 // we've ensured the profile has been shut down.
 policy::PolicyCertVerifier* g_policy_cert_verifier_for_factory = NULL;
 
 KeyedService* TestPolicyCertServiceFactory(content::BrowserContext* context) {
   return policy::PolicyCertService::CreateForTesting(
       kUsers[0], g_policy_cert_verifier_for_factory, UserManager::Get())
       .release();
@@ skipping 113 matching lines @@
     MultiProfileUserController::kBehaviorUnrestricted,
     MultiProfileUserController::kBehaviorPrimaryOnly,
     MultiProfileUserController::kBehaviorNotAllowed,
   };
   for (size_t i = 0; i < arraysize(kTestCases); ++i) {
     SetCachedBehavior(0, kTestCases[i]);
     MultiProfileUserController::UserAllowedInSessionReason reason;
     EXPECT_TRUE(controller()->IsUserAllowedInSession(kUsers[0], &reason))
         << "Case " << i;
     EXPECT_EQ(MultiProfileUserController::ALLOWED, reason) << "Case " << i;
+    EXPECT_EQ(MultiProfileUserController::ALLOWED,

[James Cook, 2014/08/12 20:08:29]

Likewise I would reorder all these, so you first test the primary policy, then
the final decision.

[Roman Sorokin (ftl), 2014/08/13 09:11:52]

Done.

+              MultiProfileUserController::GetPrimaryUserPolicy())
+        << "Case " << i;
   }
 }
 
 // Tests that invalid cache value would become the default "unrestricted".
 TEST_F(MultiProfileUserControllerTest, InvalidCacheBecomesDefault) {
   const char kBad[] = "some invalid value";
   SetCachedBehavior(0, kBad);
   EXPECT_EQ(MultiProfileUserController::kBehaviorUnrestricted,
             GetCachedBehavior(0));
 }
@@ skipping 39 matching lines @@
 // Tests cases before the second user login.
 TEST_F(MultiProfileUserControllerTest, IsSecondaryAllowed) {
   LoginUser(0);
 
   for (size_t i = 0; i < arraysize(kBehaviorTestCases); ++i) {
     SetPrefBehavior(0, kBehaviorTestCases[i].primary);
     SetCachedBehavior(1, kBehaviorTestCases[i].secondary);
     MultiProfileUserController::UserAllowedInSessionReason reason;
     controller()->IsUserAllowedInSession(kUsers[1], &reason);
     EXPECT_EQ(kBehaviorTestCases[i].expected_allowed, reason) << "Case " << i;
+    EXPECT_EQ(kBehaviorTestCases[i].expected_primary_policy,
+              MultiProfileUserController::GetPrimaryUserPolicy())
+        << "Case " << i;
   }
 }
 
 // Tests user behavior changes within a two-user session.
 TEST_F(MultiProfileUserControllerTest, PrimaryBehaviorChange) {
   LoginUser(0);
   LoginUser(1);
 
   for (size_t i = 0; i < arraysize(kBehaviorTestCases); ++i) {
     SetPrefBehavior(0, MultiProfileUserController::kBehaviorUnrestricted);
@@ skipping 29 matching lines @@
 TEST_F(MultiProfileUserControllerTest,
        UsedPolicyCertificatesAllowedForPrimary) {
   // Verifies that any user can sign-in as the primary user, regardless of the
   // tainted state.
   policy::PolicyCertServiceFactory::SetUsedPolicyCertificates(kUsers[0]);
   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);
+  EXPECT_EQ(MultiProfileUserController::ALLOWED,
+            MultiProfileUserController::GetPrimaryUserPolicy());
 }
 
 TEST_F(MultiProfileUserControllerTest,
        UsedPolicyCertificatesDisallowedForSecondary) {
   // Verifies that if a regular user is signed-in then other regular users can
   // be added but tainted users can't.
   LoginUser(1);
 
   // TODO(xiyuan): Remove the following SetPrefBehavor when default is
   // changed back to enabled.
@@ skipping 19 matching lines @@
   cert_verifier_.reset(new policy::PolicyCertVerifier(base::Closure()));
   g_policy_cert_verifier_for_factory = cert_verifier_.get();
   ASSERT_TRUE(
       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,
             reason);
+  EXPECT_EQ(MultiProfileUserController::NOT_ALLOWED_PRIMARY_POLICY_CERT_TAINTED,
+            MultiProfileUserController::GetPrimaryUserPolicy());
   policy::PolicyCertServiceFactory::SetUsedPolicyCertificates(kUsers[1]);
   EXPECT_FALSE(controller()->IsUserAllowedInSession(kUsers[1], &reason));
   EXPECT_EQ(MultiProfileUserController::NOT_ALLOWED_POLICY_CERT_TAINTED,
             reason);
+  EXPECT_EQ(MultiProfileUserController::NOT_ALLOWED_PRIMARY_POLICY_CERT_TAINTED,
+            MultiProfileUserController::GetPrimaryUserPolicy());
 
   // Flush tasks posted to IO.
   base::RunLoop().RunUntilIdle();
 }
 
 TEST_F(MultiProfileUserControllerTest,
        PolicyCertificatesInMemoryDisallowsSecondaries) {
   // Verifies that if a user is signed-in and has policy certificates installed
   // then no other users can be added.
   LoginUser(0);
 
   // TODO(xiyuan): Remove the following SetPrefBehavor when default is
   // changed back to enabled.
   SetPrefBehavior(0, MultiProfileUserController::kBehaviorUnrestricted);
 
   cert_verifier_.reset(new policy::PolicyCertVerifier(base::Closure()));
   g_policy_cert_verifier_for_factory = cert_verifier_.get();
   ASSERT_TRUE(
       policy::PolicyCertServiceFactory::GetInstance()->SetTestingFactoryAndUse(
           profile(0), TestPolicyCertServiceFactory));
   policy::PolicyCertService* service =
       policy::PolicyCertServiceFactory::GetForProfile(profile(0));
   ASSERT_TRUE(service);
 
   EXPECT_FALSE(service->has_policy_certificates());
   MultiProfileUserController::UserAllowedInSessionReason reason;
   EXPECT_TRUE(controller()->IsUserAllowedInSession(kUsers[1], &reason));
   EXPECT_EQ(MultiProfileUserController::ALLOWED, reason);
+  EXPECT_EQ(MultiProfileUserController::ALLOWED,
+            MultiProfileUserController::GetPrimaryUserPolicy());
 
   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,
             reason);
+  EXPECT_EQ(MultiProfileUserController::NOT_ALLOWED_PRIMARY_POLICY_CERT_TAINTED,
+            MultiProfileUserController::GetPrimaryUserPolicy());
 
   // Flush tasks posted to IO.
   base::RunLoop().RunUntilIdle();
 }
 
 }  // namespace chromeos