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

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

BUG=388279
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289304

[Roman Sorokin (ftl), 2014-07-08 10:31:30]

dzhioev@, please take a look.

[Roman Sorokin (ftl), 2014-08-05 14:17:40]

dzhioev@, can you please take another look

[dzhioev (left Google), 2014-08-06 12:37:39]

https://codereview.chromium.org/374853002/diff/60001/ash/session/session_stat...
File ash/session/session_state_delegate.h (right):

https://codereview.chromium.org/374853002/diff/60001/ash/session/session_stat...
ash/session/session_state_delegate.h:72: };
Why do we need PrimaryUserPolicy enum, if we don't make distinctions in handling
of PRIMARY_USER_POLICY_FORBIDS and PRIMARY_USER_POLICY_CERT_TAINTED? Can we
change method "GetPrimaryUserPolicy" to something like "bool
IsMultiProfileAllowedByPolicy()"?

https://codereview.chromium.org/374853002/diff/60001/chrome/browser/ui/ash/se...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):

https://codereview.chromium.org/374853002/diff/60001/chrome/browser/ui/ash/se...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:214:
SessionStateDelegateChromeos::GetPrimaryUserPolicy() const {
This method is identical to the part of
MultiProfileUserController::IsUserAllowedInSession. Please move common code to
the new method MultiProfileUserController::GetPrimaryUserPolicy, returning
ALLOWED/NOT_ALLOWED_PRIMARY_USER_POLICY_FORBIDS/NOT_ALLOWED_PRIMARY_POLICY_CERT_TAINTED
and reuse it here and in IsUserAllowedInSession.

[Roman Sorokin (ftl), 2014-08-08 09:22:00]

dzhioev@, please review

https://codereview.chromium.org/374853002/diff/60001/ash/session/session_stat...
File ash/session/session_state_delegate.h (right):

https://codereview.chromium.org/374853002/diff/60001/ash/session/session_stat...
ash/session/session_state_delegate.h:72: };
On 2014/08/06 12:37:39, dzhioev wrote:
> Why do we need PrimaryUserPolicy enum, if we don't make distinctions in
handling
> of PRIMARY_USER_POLICY_FORBIDS and PRIMARY_USER_POLICY_CERT_TAINTED? Can we
> change method "GetPrimaryUserPolicy" to something like "bool
> IsMultiProfileAllowedByPolicy()"?

Done.

https://codereview.chromium.org/374853002/diff/60001/chrome/browser/ui/ash/se...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):

https://codereview.chromium.org/374853002/diff/60001/chrome/browser/ui/ash/se...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:214:
SessionStateDelegateChromeos::GetPrimaryUserPolicy() const {
On 2014/08/06 12:37:39, dzhioev wrote:
> This method is identical to the part of
> MultiProfileUserController::IsUserAllowedInSession. Please move common code to
> the new method MultiProfileUserController::GetPrimaryUserPolicy, returning
>
ALLOWED/NOT_ALLOWED_PRIMARY_USER_POLICY_FORBIDS/NOT_ALLOWED_PRIMARY_POLICY_CERT_TAINTED
> and reuse it here and in IsUserAllowedInSession.

Done.

[dzhioev (left Google), 2014-08-08 11:01:51]

On 2014/08/08 09:22:00, Roman Sorokin wrote:
> dzhioev@, please review
> 
>
https://codereview.chromium.org/374853002/diff/60001/ash/session/session_stat...
> File ash/session/session_state_delegate.h (right):
> 
>
https://codereview.chromium.org/374853002/diff/60001/ash/session/session_stat...
> ash/session/session_state_delegate.h:72: };
> On 2014/08/06 12:37:39, dzhioev wrote:
> > Why do we need PrimaryUserPolicy enum, if we don't make distinctions in
> handling
> > of PRIMARY_USER_POLICY_FORBIDS and PRIMARY_USER_POLICY_CERT_TAINTED? Can we
> > change method "GetPrimaryUserPolicy" to something like "bool
> > IsMultiProfileAllowedByPolicy()"?
> 
> Done.
> 
>
https://codereview.chromium.org/374853002/diff/60001/chrome/browser/ui/ash/se...
> File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):
> 
>
https://codereview.chromium.org/374853002/diff/60001/chrome/browser/ui/ash/se...
> chrome/browser/ui/ash/session_state_delegate_chromeos.cc:214:
> SessionStateDelegateChromeos::GetPrimaryUserPolicy() const {
> On 2014/08/06 12:37:39, dzhioev wrote:
> > This method is identical to the part of
> > MultiProfileUserController::IsUserAllowedInSession. Please move common code
to
> > the new method MultiProfileUserController::GetPrimaryUserPolicy, returning
> >
>
ALLOWED/NOT_ALLOWED_PRIMARY_USER_POLICY_FORBIDS/NOT_ALLOWED_PRIMARY_POLICY_CERT_TAINTED
> > and reuse it here and in IsUserAllowedInSession.
> 
> Done.

LGTM
You also need a review from ash/system owners.

[Roman Sorokin (ftl), 2014-08-11 11:50:56]

+jamescook

jamescook@, please review

[James Cook, 2014-08-11 17:01:35]

https://codereview.chromium.org/374853002/diff/120001/ash/system/user/user_vi...
File ash/system/user/user_view.cc (right):

https://codereview.chromium.org/374853002/diff/120001/ash/system/user/user_vi...
ash/system/user/user_view.cc:497: bool IsMultiProfileAllowed =
is_multi_profile_allowed (or just multi_profile_allowed)

https://codereview.chromium.org/374853002/diff/120001/ash/system/user/user_vi...
ash/system/user/user_view.cc:516: int messageId;
message_id

https://codereview.chromium.org/374853002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/multi_profile_user_controller.cc
(right):

https://codereview.chromium.org/374853002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/multi_profile_user_controller.cc:90:
MultiProfileUserController::UserAllowedInSessionReason
can this function be static?  or moved into the anonymous namespace?

https://codereview.chromium.org/374853002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/multi_profile_user_controller.cc:91:
MultiProfileUserController::GetPrimaryUserPolicy() const {
Pass in |primary_user| since you've already looked it up in the calling
function?

https://codereview.chromium.org/374853002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/multi_profile_user_controller.cc:92:
chromeos::UserManager* user_manager = chromeos::UserManager::Get();
"chromeos::" not needed

https://codereview.chromium.org/374853002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/multi_profile_user_controller.cc:97: return
MultiProfileUserController::ALLOWED;
No need for MultiProfileUserController:: (unless you put this in anonymous
namespace)

https://codereview.chromium.org/374853002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/multi_profile_user_controller.cc:129: return
MultiProfileUserController::ALLOWED;
ditto

[Roman Sorokin (ftl), 2014-08-11 17:41:45]

please review

https://codereview.chromium.org/374853002/diff/120001/ash/system/user/user_vi...
File ash/system/user/user_view.cc (right):

https://codereview.chromium.org/374853002/diff/120001/ash/system/user/user_vi...
ash/system/user/user_view.cc:497: bool IsMultiProfileAllowed =
On 2014/08/11 17:01:34, James Cook wrote:
> is_multi_profile_allowed (or just multi_profile_allowed)

Done.

https://codereview.chromium.org/374853002/diff/120001/ash/system/user/user_vi...
ash/system/user/user_view.cc:516: int messageId;
On 2014/08/11 17:01:34, James Cook wrote:
> message_id

Done.

https://codereview.chromium.org/374853002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/multi_profile_user_controller.cc
(right):

https://codereview.chromium.org/374853002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/multi_profile_user_controller.cc:90:
MultiProfileUserController::UserAllowedInSessionReason
On 2014/08/11 17:01:34, James Cook wrote:
> can this function be static?  or moved into the anonymous namespace?

made static

https://codereview.chromium.org/374853002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/multi_profile_user_controller.cc:91:
MultiProfileUserController::GetPrimaryUserPolicy() const {
On 2014/08/11 17:01:34, James Cook wrote:
> Pass in |primary_user| since you've already looked it up in the calling
> function?

Actually I call this function from session_state_delegate_chromeos as well and
don't look up |primary_user| there.

https://codereview.chromium.org/374853002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/multi_profile_user_controller.cc:92:
chromeos::UserManager* user_manager = chromeos::UserManager::Get();
On 2014/08/11 17:01:34, James Cook wrote:
> "chromeos::" not needed

Done.

https://codereview.chromium.org/374853002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/multi_profile_user_controller.cc:97: return
MultiProfileUserController::ALLOWED;
On 2014/08/11 17:01:34, James Cook wrote:
> No need for MultiProfileUserController:: (unless you put this in anonymous
> namespace)

Done.

https://codereview.chromium.org/374853002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/multi_profile_user_controller.cc:129: return
MultiProfileUserController::ALLOWED;
On 2014/08/11 17:01:34, James Cook wrote:
> ditto

Done.

[James Cook, 2014-08-11 17:52:25]

This code also needs tests, in particular for the delegate/controller changes.

[James Cook, 2014-08-11 17:53:04]

(Apologies for not noting that in my previous review.)

[Roman Sorokin (ftl), 2014-08-12 18:27:01]

Added tests for
SessionStateDelegateChromeos::IsMultiProfileAllowedByPrimaryUserPolicy 
and added checks for MultiProfileUserController::GetPrimaryUserPolicy.

please review

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

LGTM with nits. Nice patch!

https://codereview.chromium.org/374853002/diff/180001/chrome/browser/chromeos...
File
chrome/browser/chromeos/login/users/multi_profile_user_controller_unittest.cc
(right):

https://codereview.chromium.org/374853002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/multi_profile_user_controller_unittest.cc:38:
MultiProfileUserController::UserAllowedInSessionReason
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".

https://codereview.chromium.org/374853002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/multi_profile_user_controller_unittest.cc:44:
MultiProfileUserController::kBehaviorUnrestricted,
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.

https://codereview.chromium.org/374853002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/multi_profile_user_controller_unittest.cc:228:
EXPECT_EQ(MultiProfileUserController::ALLOWED,
Likewise I would reorder all these, so you first test the primary policy, then
the final decision.

https://codereview.chromium.org/374853002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/profiles/profile_helper.h (right):

https://codereview.chromium.org/374853002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/profiles/profile_helper.h:123: friend class
::SessionStateDelegateChromeOSTest;
Put this in namespace chromeos like ProfileListChromeOSTest.

https://codereview.chromium.org/374853002/diff/180001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/session_state_delegate_chromeos_unittest.cc (right):

https://codereview.chromium.org/374853002/diff/180001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_state_delegate_chromeos_unittest.cc:30:
KeyedService* TestPolicyCertServiceFactory(content::BrowserContext* context) {
nit: I would name this CreatePolicyCertServiceForTest or
CreateTestPolicyCertService -- it needs a verb.

https://codereview.chromium.org/374853002/diff/180001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_state_delegate_chromeos_unittest.cc:210: }
Nice test suite. Good coverage, and helps me as a reader understand the
multi-profile/certificate interactions.

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

+nkostylev@

Nikita, please review chrome/browser/chromeos/profiles/profile_helper.h

https://codereview.chromium.org/374853002/diff/180001/chrome/browser/chromeos...
File
chrome/browser/chromeos/login/users/multi_profile_user_controller_unittest.cc
(right):

https://codereview.chromium.org/374853002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/multi_profile_user_controller_unittest.cc:38:
MultiProfileUserController::UserAllowedInSessionReason
On 2014/08/12 20:08:30, James Cook wrote:
> 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".

Done.

https://codereview.chromium.org/374853002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/multi_profile_user_controller_unittest.cc:44:
MultiProfileUserController::kBehaviorUnrestricted,
Yes, it is auto-formatter
On 2014/08/12 20:08:29, James Cook wrote:
> 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.

https://codereview.chromium.org/374853002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/multi_profile_user_controller_unittest.cc:228:
EXPECT_EQ(MultiProfileUserController::ALLOWED,
On 2014/08/12 20:08:29, James Cook wrote:
> Likewise I would reorder all these, so you first test the primary policy, then
> the final decision.

Done.

https://codereview.chromium.org/374853002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/profiles/profile_helper.h (right):

https://codereview.chromium.org/374853002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/profiles/profile_helper.h:123: friend class
::SessionStateDelegateChromeOSTest;
On 2014/08/12 20:08:30, James Cook wrote:
> Put this in namespace chromeos like ProfileListChromeOSTest.

Done.

https://codereview.chromium.org/374853002/diff/180001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/session_state_delegate_chromeos_unittest.cc (right):

https://codereview.chromium.org/374853002/diff/180001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_state_delegate_chromeos_unittest.cc:30:
KeyedService* TestPolicyCertServiceFactory(content::BrowserContext* context) {
On 2014/08/12 20:08:30, James Cook wrote:
> nit: I would name this CreatePolicyCertServiceForTest or
> CreateTestPolicyCertService -- it needs a verb.

Done.

[Nikita (slow), 2014-08-13 11:03:21]

On 2014/08/13 09:11:52, Roman Sorokin wrote:
> +nkostylev@
> 
> Nikita, please review chrome/browser/chromeos/profiles/profile_helper.h

lgtm

[Roman Sorokin (ftl), 2014-08-13 13:24:50]

The CQ bit was checked by rsorokin@chromium.org

[commit-bot: I haz the power, 2014-08-13 13:25:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rsorokin@chromium.org/374853002/220001

[Roman Sorokin (ftl), 2014-08-13 13:52:23]

The CQ bit was unchecked by rsorokin@chromium.org

[Roman Sorokin (ftl), 2014-08-13 13:55:47]

The CQ bit was checked by rsorokin@chromium.org

[commit-bot: I haz the power, 2014-08-13 13:56:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rsorokin@chromium.org/374853002/240001

[commit-bot: I haz the power, 2014-08-13 15:21:57]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[commit-bot: I haz the power, 2014-08-13 16:04:10]

Committed patchset #10 (240001) as 289304