Prevent ONC-pushed certificates from being used with multiprofiles.

Prevent ONC-pushed certificates from being used with multiprofiles.

If a profile uses ONC-pushed certificates then it can't be part of a
multiprofile session for security reasons. This change enforces that:

- if ONC-pushed certificates are in use then multiprofiles is disabled;

- if a multiprofile session is ongoing then updates to the certificates
  pushed by ONC are ignored until the next restart;

- a profile that already used an ONC-pushed certificate before can not
  join a multiprofile session.

The last point required moving the "tainted" signal from the profile's
prefs into local_state, so that it can be checked before the profile
is loaded and added to the session.

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

[Joao da Silva, 2013-12-17 14:21:10]

Please have a look:

@Philipp: policy/

@Nikita: login/ and chromeos

@James: ui/ash/

Thanks!

[pneubeck (no reviews), 2013-12-17 15:25:31]

the only non-nit comment is:

if
 user B has tainted his profile prior M33,
 device is update to M33,
 user A logs,
 and tries to add user B to the session,
then
 this will not fail, although it should, with the current code.

There seem to be two ways to solve this:
- decide if user B is allowed to join, after loading his profile
- don't allow users to join if their profile is not migrated yet to the current
release. I.e. such users have to login without Multiprofile at first.

The second point might in general be useful.

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/multi_profile_user_controller.cc (right):

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/multi_profile_user_controller.cc:86: if
(policy::PolicyCertServiceFactory::UsedPolicyCertificates(user_email))
this check fails, if the other user never logged-in after update to M33.

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/policy_cert_service.cc (right):

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/policy_cert_service.cc:33:
DCHECK(net_conf_updater_);
nit:
DCHECK(user_manager)

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/policy_cert_service.h (right):

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/policy_cert_service.h:42: const std::string&
user_id,
nit: const parameters should be first.

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/policy_cert_service.h:50: // Returns true if the
profile with |user_prefs| has used certificates
not up-to-date.

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/policy_cert_service.h:58: bool IsTainted() const;
the function name doesn't fit / hides details behind a vague name.
if there's not better name, maybe instead make a HasTrustAnchors() and move the
|| to the call-side.

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/policy_cert_service_factory.cc (right):

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/policy_cert_service_factory.cc:49: if
(!UsedPolicyCertificates(user_id)) {
if (Used...)
  return;

to remove indentation, if you like

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/policy_cert_service_factory.cc:51:
prefs::kUsedPolicyCertificates);
is it safe to use local_state implicitly in static functions, or should we
better pass it as an argument?

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/policy_cert_service_factory.cc:58: const
std::string& user_id) {
not sure whether we consistently use "user_id" as a synonym for "user_email".
If not, better use user_email consistently.

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/policy_cert_service_factory.cc:70: return list &&
list->Find(value) != list->end();
!list is an error so logging or DCHECK should be more correct.

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/policy_cert_service_factory.cc:103: if
(prefs->GetBoolean(prefs::kUsedPolicyCertificatesOnce)) {
is a UMA metric necessary, to decide when to remove this?

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/policy_cert_service_factory.cc:123:
registry->RegisterBooleanPref(
add a comment that it's still here for backwards compatibility.

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/policy_cert_service_factory.h (right):

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/policy_cert_service_factory.h:43: // Used to mark
or clear |user_id| as having used certificates pushed by
instead of having this here, it could be moved to something like
used_policy_certs_preference.h

however, I don't feel strong about this.

https://codereview.chromium.org/117263002/diff/1/chrome/browser/ui/ash/chrome...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://codereview.chromium.org/117263002/diff/1/chrome/browser/ui/ash/chrome...
chrome/browser/ui/ash/chrome_shell_delegate.cc:85: if (active)
are !active
and !profile
errors? DCHECK/LOG?

[Joao da Silva, 2013-12-17 16:36:40]

Philipp, PTAL. The case you found isn't easy to prevent, but isn't common
either; see the comments inline about the fix.

+Bernhard: please check chrome/browser/prefs

Thanks!

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/multi_profile_user_controller.cc (right):

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/multi_profile_user_controller.cc:86: if
(policy::PolicyCertServiceFactory::UsedPolicyCertificates(user_email))
On 2013/12/17 15:25:31, pneubeck wrote:
> this check fails, if the other user never logged-in after update to M33.

Great catch.

This isn't easy to prevent; after the profile has been loaded then there's no
way to go back.

So this is now doing the same that MultiProfileUserController::CheckSessionUsers
does if a profile gets added by mistake: a reboot is forced.

However, in our case we clear the pref and move it to local_state, so the reboot
happens at most once.

In practice I don't expect this to happen to anyone, but it's a good idea to
have the safeguard in place.

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/policy_cert_service.cc (right):

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/policy_cert_service.cc:33:
DCHECK(net_conf_updater_);
On 2013/12/17 15:25:31, pneubeck wrote:
> nit:
> DCHECK(user_manager)

Done.

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/policy_cert_service.h (right):

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/policy_cert_service.h:42: const std::string&
user_id,
On 2013/12/17 15:25:31, pneubeck wrote:
> nit: const parameters should be first.

Done.

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/policy_cert_service.h:50: // Returns true if the
profile with |user_prefs| has used certificates
On 2013/12/17 15:25:31, pneubeck wrote:
> not up-to-date.

Done.

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/policy_cert_service.h:58: bool IsTainted() const;
On 2013/12/17 15:25:31, pneubeck wrote:
> the function name doesn't fit / hides details behind a vague name.
> if there's not better name, maybe instead make a HasTrustAnchors() and move
the
> || to the call-side.

Done.

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/policy_cert_service_factory.cc (right):

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/policy_cert_service_factory.cc:49: if
(!UsedPolicyCertificates(user_id)) {
On 2013/12/17 15:25:31, pneubeck wrote:
> if (Used...)
>   return;
> 
> to remove indentation, if you like

Done.

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/policy_cert_service_factory.cc:51:
prefs::kUsedPolicyCertificates);
On 2013/12/17 15:25:31, pneubeck wrote:
> is it safe to use local_state implicitly in static functions, or should we
> better pass it as an argument?

That's a good point. This call is made from the service which only lives as long
as the Profile does, so this is safe.

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/policy_cert_service_factory.cc:58: const
std::string& user_id) {
On 2013/12/17 15:25:31, pneubeck wrote:
> not sure whether we consistently use "user_id" as a synonym for "user_email".
> If not, better use user_email consistently.

This comes from UserManager, which uses user_id.

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/policy_cert_service_factory.cc:70: return list &&
list->Find(value) != list->end();
On 2013/12/17 15:25:31, pneubeck wrote:
> !list is an error so logging or DCHECK should be more correct.

Done.

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/policy_cert_service_factory.cc:103: if
(prefs->GetBoolean(prefs::kUsedPolicyCertificatesOnce)) {
On 2013/12/17 15:25:31, pneubeck wrote:
> is a UMA metric necessary, to decide when to remove this?

This is mostly used by schools, and IIRC they disable metrics reporting. We'll
have to sync with Vidya about this.

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/policy_cert_service_factory.cc:123:
registry->RegisterBooleanPref(
On 2013/12/17 15:25:31, pneubeck wrote:
> add a comment that it's still here for backwards compatibility.

Done.

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/policy_cert_service_factory.h (right):

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/policy_cert_service_factory.h:43: // Used to mark
or clear |user_id| as having used certificates pushed by
On 2013/12/17 15:25:31, pneubeck wrote:
> instead of having this here, it could be moved to something like
> used_policy_certs_preference.h
> 
> however, I don't feel strong about this.

I'd keep it here, because this is backed by a pref shared by all profiles and
that pref gets set by the services created by this factory.

https://codereview.chromium.org/117263002/diff/1/chrome/browser/ui/ash/chrome...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://codereview.chromium.org/117263002/diff/1/chrome/browser/ui/ash/chrome...
chrome/browser/ui/ash/chrome_shell_delegate.cc:85: if (active)
On 2013/12/17 15:25:31, pneubeck wrote:
> are !active
> and !profile
> errors? DCHECK/LOG?

!active means that no-one is signed in yet, so it's fine.

active && !profile should not happen; GetProfileByUser triggers a DCHECK (at
ProfileManager) if that happens.

https://codereview.chromium.org/117263002/diff/30012/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/policy_cert_service_factory.cc (right):

https://codereview.chromium.org/117263002/diff/30012/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/policy_cert_service_factory.cc:121: }
This block handles the case of a pre-M33 tainted profile getting added as a
secondary profile. Note that the reboot won't happen again next time, because
the prefs are updated by then.

[Bernhard Bauer, 2013-12-17 16:54:41]

chrome/browser/prefs LGTM

[James Cook, 2013-12-17 17:19:45]

+skuhne, can you take a look at c/b/ui/ash?

https://codereview.chromium.org/117263002/diff/40001/chrome/browser/ui/ash/ch...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://codereview.chromium.org/117263002/diff/40001/chrome/browser/ui/ash/ch...
chrome/browser/ui/ash/chrome_shell_delegate.cc:82: chromeos::User* active =
chromeos::UserManager::Get()->GetActiveUser();
A few questions:
* Is there someplace else this blob of code could live to avoid adding extra
dependencies to this file (especially Profile)?  Maybe in the policy code
somewhere?
* Is it ever the case that there is no active user?
* Is it ever the case that the active user has no profile?

[James Cook, 2013-12-17 17:20:04]

really +skuhne this time

[Mr4D (OOO till 08-26), 2013-12-17 17:39:34]

Please have a look!

https://codereview.chromium.org/117263002/diff/40001/chrome/browser/ui/ash/ch...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://codereview.chromium.org/117263002/diff/40001/chrome/browser/ui/ash/ch...
chrome/browser/ui/ash/chrome_shell_delegate.cc:82: chromeos::User* active =
chromeos::UserManager::Get()->GetActiveUser();
Would it be possible to move this into the UserManager? There are already
several policy checks performed - and even more so
"GetUsersAdmittedForMultiProfile" should do the same test...

As such adding there a function like "IsUserAllowedForMultiProfile" should be
easy.

[pneubeck (no reviews), 2013-12-17 17:57:00]

lgtm

you could add a bug for removing the migration code.
when you do manual tests, you could trigger the reboot by modifying the user's
prefs manually.

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/policy_cert_service_factory.cc (right):

https://codereview.chromium.org/117263002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/policy_cert_service_factory.cc:51:
prefs::kUsedPolicyCertificates);
On 2013/12/17 16:36:41, Joao da Silva wrote:
> On 2013/12/17 15:25:31, pneubeck wrote:
> > is it safe to use local_state implicitly in static functions, or should we
> > better pass it as an argument?
> 
> That's a good point. This call is made from the service which only lives as
long
> as the Profile does, so this is safe.
Yeah, in case of this function. The other to functions have the same "issue".

https://codereview.chromium.org/117263002/diff/30012/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/multi_profile_user_controller_unittest.cc
(right):

https://codereview.chromium.org/117263002/diff/30012/chrome/browser/chromeos/...
chrome/browser/chromeos/login/multi_profile_user_controller_unittest.cc:272:
policy::PolicyCertServiceFactory::ClearUsedPolicyCertificates(kUsers[0]);
is local state reset anyways after the test / recreated on each test setup?

[Nikita (slow), 2013-12-17 18:42:04]

https://codereview.chromium.org/117263002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/multi_profile_user_controller.cc (right):

https://codereview.chromium.org/117263002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/multi_profile_user_controller.cc:86: if
(policy::PolicyCertServiceFactory::UsedPolicyCertificates(user_email))
What if you have primary user in session that has such certificates.
Are you allowed to add other users into that session?

https://codereview.chromium.org/117263002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/multi_profile_user_controller_unittest.cc
(right):

https://codereview.chromium.org/117263002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/multi_profile_user_controller_unittest.cc:271:
EXPECT_TRUE(controller()->IsUserAllowedInSession(kUsers[1]));
nit: It seems that you're actually checking both users here and they are both
allowed?

[Joao da Silva, 2013-12-17 20:44:20]

Moved all the logic to MultiProfileUserController, so it's all available in one
place and easier to follow.

Also added a couple more tests.

@Nikita: PTAL

@Philipp: mocking a PolicyCertService for a single test turned out to be quite
painful. Please have another look at the changes there.

Thanks!

https://codereview.chromium.org/117263002/diff/30012/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/multi_profile_user_controller_unittest.cc
(right):

https://codereview.chromium.org/117263002/diff/30012/chrome/browser/chromeos/...
chrome/browser/chromeos/login/multi_profile_user_controller_unittest.cc:272:
policy::PolicyCertServiceFactory::ClearUsedPolicyCertificates(kUsers[0]);
On 2013/12/17 17:57:01, pneubeck wrote:
> is local state reset anyways after the test / recreated on each test setup?

Right, removed this.

https://codereview.chromium.org/117263002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/multi_profile_user_controller.cc (right):

https://codereview.chromium.org/117263002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/multi_profile_user_controller.cc:86: if
(policy::PolicyCertServiceFactory::UsedPolicyCertificates(user_email))
On 2013/12/17 18:42:05, Nikita Kostylev wrote:
> What if you have primary user in session that has such certificates.
> Are you allowed to add other users into that session?

No, that is not allowed. That used to be handled in the chrome_shell_delegate.cc
and has been moved here.

Turns out that if this is always false (which is the case when the primary user
has certificates) then the list of allowed users is empty, which achieves the
same on the UI.

https://codereview.chromium.org/117263002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/multi_profile_user_controller_unittest.cc
(right):

https://codereview.chromium.org/117263002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/multi_profile_user_controller_unittest.cc:271:
EXPECT_TRUE(controller()->IsUserAllowedInSession(kUsers[1]));
On 2013/12/17 18:42:05, Nikita Kostylev wrote:
> nit: It seems that you're actually checking both users here and they are both
> allowed?

Yes; this test verifies that both "normal" and "tainted" users can become the
primary user. Note that none of them have logged in yet.

https://codereview.chromium.org/117263002/diff/40001/chrome/browser/ui/ash/ch...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://codereview.chromium.org/117263002/diff/40001/chrome/browser/ui/ash/ch...
chrome/browser/ui/ash/chrome_shell_delegate.cc:82: chromeos::User* active =
chromeos::UserManager::Get()->GetActiveUser();
On 2013/12/17 17:19:46, James Cook wrote:
> A few questions:
> * Is there someplace else this blob of code could live to avoid adding extra
> dependencies to this file (especially Profile)?  Maybe in the policy code
> somewhere?

IsUserAllowedInSession can also have this logic, and indeed seems like a better
fix. This file doesn't have changes anymore.

> * Is it ever the case that there is no active user?

Before the first user signs in.

> * Is it ever the case that the active user has no profile?

Not really, just being defensive :-)

https://codereview.chromium.org/117263002/diff/40001/chrome/browser/ui/ash/ch...
chrome/browser/ui/ash/chrome_shell_delegate.cc:82: chromeos::User* active =
chromeos::UserManager::Get()->GetActiveUser();
On 2013/12/17 17:39:35, Mr4D wrote:
> Would it be possible to move this into the UserManager? There are already
> several policy checks performed - and even more so
> "GetUsersAdmittedForMultiProfile" should do the same test...
> 
> As such adding there a function like "IsUserAllowedForMultiProfile" should be
> easy.

Moved to MultiProfileUserController::IsUserAllowedInSession. That class handles
logic related to multiprofiles, and that method is already performing similar
checks.

[James Cook, 2013-12-17 20:47:11]

-me since I don't own any code in this CL anymore. Re-add me if you want another
opinion. :-)

[Mr4D (OOO till 08-26), 2013-12-17 22:16:39]

lgtm - looks like that c/b/ui/ash went away

[pneubeck (no reviews), 2013-12-18 09:11:07]

lgtm (your choice to go with the current usage of the service in the test or to
mock it)

https://codereview.chromium.org/117263002/diff/50001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/multi_profile_user_controller.cc (right):

https://codereview.chromium.org/117263002/diff/50001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/multi_profile_user_controller.cc:135:
policy::PolicyCertServiceFactory::ClearUsedPolicyCertificates(user_email);
i think the old place was better for this as the tainting information is used by
multiprofile, but doesn't depend on it.

https://codereview.chromium.org/117263002/diff/50001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/multi_profile_user_controller.h (right):

https://codereview.chromium.org/117263002/diff/50001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/multi_profile_user_controller.h:46: // Removes the
cached value for the given user.
nit: s/value/values/

and the other comments in this file referring to "value" seem not up-to-date.

https://codereview.chromium.org/117263002/diff/50001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/multi_profile_user_controller_unittest.cc
(right):

https://codereview.chromium.org/117263002/diff/50001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/multi_profile_user_controller_unittest.cc:306:
UsedPolicyCertificatesDisallowsSecondaries) {
since this is about testing the UserController, the alternative would be to mock
the PolicyCertService (which in turn requires adding a pure interface).

[Joao da Silva, 2013-12-18 09:40:12]

Ready to land if Nikita acks the login/ changes.

https://codereview.chromium.org/117263002/diff/50001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/multi_profile_user_controller.cc (right):

https://codereview.chromium.org/117263002/diff/50001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/multi_profile_user_controller.cc:135:
policy::PolicyCertServiceFactory::ClearUsedPolicyCertificates(user_email);
On 2013/12/18 09:11:07, pneubeck wrote:
> i think the old place was better for this as the tainting information is used
by
> multiprofile, but doesn't depend on it.

I'd prefer to have this next to the code that calls the Set() (which is
PolicyCertService), but its profile does not exist when the user is deleted. So
it has to be in UserManagerImpl or in one of the methods that it calls; since
the test logic is here and there's also cleanup logic in this class then I think
this is the least bad place.

https://codereview.chromium.org/117263002/diff/50001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/multi_profile_user_controller.h (right):

https://codereview.chromium.org/117263002/diff/50001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/multi_profile_user_controller.h:46: // Removes the
cached value for the given user.
On 2013/12/18 09:11:07, pneubeck wrote:
> nit: s/value/values/
> 
> and the other comments in this file referring to "value" seem not up-to-date.

Done.

https://codereview.chromium.org/117263002/diff/50001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/multi_profile_user_controller_unittest.cc
(right):

https://codereview.chromium.org/117263002/diff/50001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/multi_profile_user_controller_unittest.cc:306:
UsedPolicyCertificatesDisallowsSecondaries) {
On 2013/12/18 09:11:07, pneubeck wrote:
> since this is about testing the UserController, the alternative would be to
mock
> the PolicyCertService (which in turn requires adding a pure interface).

Right. We may do that cleanup if more tests require a mocked PolicyCertService,
but the CreateForTesting() call works for now.

[pneubeck (no reviews), 2013-12-18 13:21:29]

https://codereview.chromium.org/117263002/diff/70001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/multi_profile_user_controller.cc (right):

https://codereview.chromium.org/117263002/diff/70001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/multi_profile_user_controller.cc:99:
service->has_policy_certificates())) {
another nit:

if the service does not exist, for whatever reasons, then we could still check
the tainted state in local_state (i.e. UsedPolicyCertificates).
Is it worth it, to directly call the static function of
PolicyCertServiceFactory?

your test wouldn't depend on the PolicyCertService anymore also.

https://codereview.chromium.org/117263002/diff/70001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/multi_profile_user_controller_unittest.cc
(right):

https://codereview.chromium.org/117263002/diff/70001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/multi_profile_user_controller_unittest.cc:306:
UsedPolicyCertificatesDisallowsSecondaries) {
if we would add another test which ensures that the has_trust_anchors condition
works, you have your excuse to implement a FakePolicyCertVerifier.

[Joao da Silva, 2013-12-19 09:13:23]

https://codereview.chromium.org/117263002/diff/70001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/multi_profile_user_controller.cc (right):

https://codereview.chromium.org/117263002/diff/70001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/multi_profile_user_controller.cc:99:
service->has_policy_certificates())) {
On 2013/12/18 13:21:30, pneubeck wrote:
> another nit:
> 
> if the service does not exist, for whatever reasons, then we could still check
> the tainted state in local_state (i.e. UsedPolicyCertificates).
> Is it worth it, to directly call the static function of
> PolicyCertServiceFactory?
> 
> your test wouldn't depend on the PolicyCertService anymore also.

local_state doesn't have the has_trust_anchors_ signal: it's possible that a
profile isn't tainted yet, but has the certs in memory; in that case we don't
want to add any other profiles either.

[pneubeck (no reviews), 2013-12-19 09:29:31]

https://codereview.chromium.org/117263002/diff/70001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/multi_profile_user_controller.cc (right):

https://codereview.chromium.org/117263002/diff/70001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/multi_profile_user_controller.cc:99:
service->has_policy_certificates())) {
On 2013/12/19 09:13:24, Joao da Silva wrote:
> On 2013/12/18 13:21:30, pneubeck wrote:
> > another nit:
> > 
> > if the service does not exist, for whatever reasons, then we could still
check
> > the tainted state in local_state (i.e. UsedPolicyCertificates).
> > Is it worth it, to directly call the static function of
> > PolicyCertServiceFactory?
> > 
> > your test wouldn't depend on the PolicyCertService anymore also.
> 
> local_state doesn't have the has_trust_anchors_ signal: it's possible that a
> profile isn't tainted yet, but has the certs in memory; in that case we don't
> want to add any other profiles either.

yes, that's clear.
I meant, instead of lines 91 - 99:

const User* primary_user = user_manager->GetPrimaryUser();

// Don't allow any secondary profiles if the primary profile is
// tainted.
if
(policy::PolicyCertServiceFactory::UsedPolicyCertificates(primary_user->email()))
  return false;

// Don't allow any secondary profiles also if the primary profile is not tainted
yet, but might be potentially tainted.
Profile* profile =
  primary_user ? user_manager->GetProfileByUser(primary_user) : NULL;
policy::PolicyCertService* service =
  profile ? policy::PolicyCertServiceFactory::GetForProfile(profile) : NULL;
if (service && service->has_policy_certificates()))
  return false

[Joao da Silva, 2013-12-19 10:37:25]

https://codereview.chromium.org/117263002/diff/70001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/multi_profile_user_controller.cc (right):

https://codereview.chromium.org/117263002/diff/70001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/multi_profile_user_controller.cc:99:
service->has_policy_certificates())) {
I see what you meant now, thanks. Done, and added another test to cover that
code path too.

[Nikita (slow), 2013-12-19 10:42:05]

lgtm

[commit-bot: I haz the power, 2013-12-19 10:42:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/117263002/90001

[pneubeck (no reviews), 2013-12-19 11:35:08]

https://codereview.chromium.org/117263002/diff/90001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/multi_profile_user_controller_unittest.cc
(right):

https://codereview.chromium.org/117263002/diff/90001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/multi_profile_user_controller_unittest.cc:315:
policy::PolicyCertVerifier verifier((base::Closure()));
nit: PolicyCertVerifier not required in this test anymore.

[commit-bot: I haz the power, 2013-12-19 14:29:04]

Change committed as 241857