Updated SessionManagerClient to use the multi-profile user policy calls.

Updated SessionManagerClient to use the multi-profile user policy calls.

The SessionManagerClient is used to store and retrieve the user policy blobs
to and from the session_manager over DBus. New variants of these calls have been
introduced that have an additional 'username' argument, so that user policy can
be used with multiple profiles.

This CL updates the SessionManagerClient to use the new calls, and it also
makes the SessionManagerClientStub mimic the behavior of session_manager
regarding the user policy key, so that user policy can be validated and loaded
on desktop builds.

BUG=187482, 230349
TEST=User policy works as before, all tests green; user policy works with --multi-profiles, and on desktop builds with --login-manager

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

[Joao da Silva, 2013-05-08 14:15:46]

PTAL. I've manually verified that user policy works on desktop builds, on a
chromeos image and on the image with --multi-profiles with this change and with
the old dbus calls removed from session_manager.

@mnissler: please review the whole thing :-)

@satorux: please review chromeos/

@nkostylev, @cmasone: FYI

Thanks!

[satorux1, 2013-05-10 05:58:40]

chromeos/... LGTM

I'd suggest to split the patch as it's big and 'chromeos' portion seems to be
easily split, but it's up to you.

[Mattias Nissler (ping if slow), 2013-05-10 12:40:14]

https://codereview.chromium.org/14761012/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/login_utils_browsertest.cc (right):

https://codereview.chromium.org/14761012/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/login_utils_browsertest.cc:241:
MockSessionManagerClient* session_managed_client =
while at it, can you fix the typo here?

https://codereview.chromium.org/14761012/diff/1/chrome/browser/policy/cloud/c...
File chrome/browser/policy/cloud/cloud_policy_browsertest.cc (right):

https://codereview.chromium.org/14761012/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_browsertest.cc:208:
.AppendASCII(std::string(GetTestUser()) + "-profile")
another call the the common helper here :)

https://codereview.chromium.org/14761012/diff/1/chromeos/dbus/cryptohome_clie...
File chromeos/dbus/cryptohome_client.cc (right):

https://codereview.chromium.org/14761012/diff/1/chromeos/dbus/cryptohome_clie...
chromeos/dbus/cryptohome_client.cc:857: // Keep this in sync with
SessionManagerClientStubImpl::StorePolicyForUser.
Then please make a static function somewhere that all code can call.

https://codereview.chromium.org/14761012/diff/1/chromeos/dbus/session_manager...
File chromeos/dbus/session_manager_client.cc (right):

https://codereview.chromium.org/14761012/diff/1/chromeos/dbus/session_manager...
chromeos/dbus/session_manager_client.cc:271: void
CallRetrievePolicyForUsername(const std::string& method_name,
Can we rename to CallRetrievePolicyByUsername in order to use a name that is
maximum different from RetrievePolicyForUser?

https://codereview.chromium.org/14761012/diff/1/chromeos/dbus/session_manager...
chromeos/dbus/session_manager_client.cc:288: void
CallStorePolicyForUsername(const std::string& method_name,
Ditto re nameing, i.e. CallStorePolicyByUsername

https://codereview.chromium.org/14761012/diff/1/chromeos/dbus/session_manager...
chromeos/dbus/session_manager_client.cc:516: // can be tested on desktop builds.
If you need this, why not extract the policy key from the policy blob here?

https://codereview.chromium.org/14761012/diff/1/chromeos/dbus/session_manager...
chromeos/dbus/session_manager_client.cc:524: key_path =
key_path.AppendASCII(sanitized).AppendASCII("policy.pub");
This is misleading, please name it policy_key.pub or something.

https://codereview.chromium.org/14761012/diff/1/chromeos/dbus/session_manager...
File chromeos/dbus/session_manager_client.h (right):

https://codereview.chromium.org/14761012/diff/1/chromeos/dbus/session_manager...
chromeos/dbus/session_manager_client.h:128: // by the stub implementation to
enable policy validation on desktop builds.
Can't the just assume successful validation?

[Joao da Silva, 2013-05-13 09:39:23]

@mnissler: PTAL

https://codereview.chromium.org/14761012/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/login_utils_browsertest.cc (right):

https://codereview.chromium.org/14761012/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/login_utils_browsertest.cc:241:
MockSessionManagerClient* session_managed_client =
On 2013/05/10 12:40:15, Mattias Nissler wrote:
> while at it, can you fix the typo here?

This has been removed.

https://codereview.chromium.org/14761012/diff/1/chrome/browser/policy/cloud/c...
File chrome/browser/policy/cloud/cloud_policy_browsertest.cc (right):

https://codereview.chromium.org/14761012/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_policy_browsertest.cc:208:
.AppendASCII(std::string(GetTestUser()) + "-profile")
On 2013/05/10 12:40:15, Mattias Nissler wrote:
> another call the the common helper here :)

Done.

https://codereview.chromium.org/14761012/diff/1/chromeos/dbus/cryptohome_clie...
File chromeos/dbus/cryptohome_client.cc (right):

https://codereview.chromium.org/14761012/diff/1/chromeos/dbus/cryptohome_clie...
chromeos/dbus/cryptohome_client.cc:857: // Keep this in sync with
SessionManagerClientStubImpl::StorePolicyForUser.
On 2013/05/10 12:40:15, Mattias Nissler wrote:
> Then please make a static function somewhere that all code can call.

Done.

https://codereview.chromium.org/14761012/diff/1/chromeos/dbus/session_manager...
File chromeos/dbus/session_manager_client.cc (right):

https://codereview.chromium.org/14761012/diff/1/chromeos/dbus/session_manager...
chromeos/dbus/session_manager_client.cc:271: void
CallRetrievePolicyForUsername(const std::string& method_name,
On 2013/05/10 12:40:15, Mattias Nissler wrote:
> Can we rename to CallRetrievePolicyByUsername in order to use a name that is
> maximum different from RetrievePolicyForUser?

Done.

https://codereview.chromium.org/14761012/diff/1/chromeos/dbus/session_manager...
chromeos/dbus/session_manager_client.cc:288: void
CallStorePolicyForUsername(const std::string& method_name,
On 2013/05/10 12:40:15, Mattias Nissler wrote:
> Ditto re nameing, i.e. CallStorePolicyByUsername

Done.

https://codereview.chromium.org/14761012/diff/1/chromeos/dbus/session_manager...
chromeos/dbus/session_manager_client.cc:516: // can be tested on desktop builds.
On 2013/05/10 12:40:15, Mattias Nissler wrote:
> If you need this, why not extract the policy key from the policy blob here?

That'd be my preferred solution, but chromeos/ can't #include the policy
protobufs. There's no good place outside chrome/ to move them to for now.

https://codereview.chromium.org/14761012/diff/1/chromeos/dbus/session_manager...
chromeos/dbus/session_manager_client.cc:524: key_path =
key_path.AppendASCII(sanitized).AppendASCII("policy.pub");
On 2013/05/10 12:40:15, Mattias Nissler wrote:
> This is misleading, please name it policy_key.pub or something.

This has to be the same name that the session_manager writes to.

https://codereview.chromium.org/14761012/diff/1/chromeos/dbus/session_manager...
File chromeos/dbus/session_manager_client.h (right):

https://codereview.chromium.org/14761012/diff/1/chromeos/dbus/session_manager...
chromeos/dbus/session_manager_client.h:128: // by the stub implementation to
enable policy validation on desktop builds.
On 2013/05/10 12:40:15, Mattias Nissler wrote:
> Can't the just assume successful validation?

The only way to achieve that that I can think of is to wrap the signature
validation check in

if (chromeos::IsRunningOnChromeOS()) { validator->CheckSignature(); }

I'd rather have the whole thing working on desktop builds as well, since that
gives us two benefits: we can test user policy on chromeos on desktop builds,
and browser_tests also run with the whole validation turned on.

[Mattias Nissler (ping if slow), 2013-05-13 10:27:14]

https://codereview.chromium.org/14761012/diff/1/chromeos/dbus/session_manager...
File chromeos/dbus/session_manager_client.cc (right):

https://codereview.chromium.org/14761012/diff/1/chromeos/dbus/session_manager...
chromeos/dbus/session_manager_client.cc:516: // can be tested on desktop builds.
On 2013/05/13 09:39:23, Joao da Silva wrote:
> On 2013/05/10 12:40:15, Mattias Nissler wrote:
> > If you need this, why not extract the policy key from the policy blob here?
> 
> That'd be my preferred solution, but chromeos/ can't #include the policy
> protobufs. There's no good place outside chrome/ to move them to for now.

Bummer. Any chance to get an exception into chromeos/DEPS for the
device_management_backend.pb.h? That's not nice either, but more honest than
just  screwing up the interface here in order to be able to implement stubs.

https://codereview.chromium.org/14761012/diff/1/chromeos/dbus/session_manager...
chromeos/dbus/session_manager_client.cc:524: key_path =
key_path.AppendASCII(sanitized).AppendASCII("policy.pub");
On 2013/05/13 09:39:23, Joao da Silva wrote:
> On 2013/05/10 12:40:15, Mattias Nissler wrote:
> > This is misleading, please name it policy_key.pub or something.
> 
> This has to be the same name that the session_manager writes to.

Oh right, I guess I should have payed closer attention in the review that added
this then :)

https://codereview.chromium.org/14761012/diff/1/chromeos/dbus/session_manager...
File chromeos/dbus/session_manager_client.h (right):

https://codereview.chromium.org/14761012/diff/1/chromeos/dbus/session_manager...
chromeos/dbus/session_manager_client.h:128: // by the stub implementation to
enable policy validation on desktop builds.
On 2013/05/13 09:39:23, Joao da Silva wrote:
> On 2013/05/10 12:40:15, Mattias Nissler wrote:
> > Can't the just assume successful validation?
> 
> The only way to achieve that that I can think of is to wrap the signature
> validation check in
> 
> if (chromeos::IsRunningOnChromeOS()) { validator->CheckSignature(); }
> 
> I'd rather have the whole thing working on desktop builds as well, since that
> gives us two benefits: we can test user policy on chromeos on desktop builds,
> and browser_tests also run with the whole validation turned on.

You are right, I didn't realize we're talking about policy validation in
dependent Chrome code.

[Joao da Silva, 2013-05-13 10:54:19]

Mattias, I'd like to land this as-is, and try to move the protobufs or introduce
a DEPS exception in another CL; wdyt?

https://codereview.chromium.org/14761012/diff/1/chromeos/dbus/session_manager...
File chromeos/dbus/session_manager_client.cc (right):

https://codereview.chromium.org/14761012/diff/1/chromeos/dbus/session_manager...
chromeos/dbus/session_manager_client.cc:516: // can be tested on desktop builds.
On 2013/05/13 10:27:14, Mattias Nissler wrote:
> On 2013/05/13 09:39:23, Joao da Silva wrote:
> > On 2013/05/10 12:40:15, Mattias Nissler wrote:
> > > If you need this, why not extract the policy key from the policy blob
here?
> > 
> > That'd be my preferred solution, but chromeos/ can't #include the policy
> > protobufs. There's no good place outside chrome/ to move them to for now.
> 
> Bummer. Any chance to get an exception into chromeos/DEPS for the
> device_management_backend.pb.h? That's not nice either, but more honest than
> just  screwing up the interface here in order to be able to implement stubs.

Can we propose that change in another CL? I'd rather try to find a better
top-level dir for the protobufs.

[Mattias Nissler (ping if slow), 2013-05-13 10:57:53]

On 2013/05/13 10:54:19, Joao da Silva wrote:
> Mattias, I'd like to land this as-is, and try to move the protobufs or
introduce
> a DEPS exception in another CL; wdyt?

Fine, as soon as we have a plan :) So please figure out what should be done here
and then you can land this and do whatever is needed in separate changes.

> 
>
https://codereview.chromium.org/14761012/diff/1/chromeos/dbus/session_manager...
> File chromeos/dbus/session_manager_client.cc (right):
> 
>
https://codereview.chromium.org/14761012/diff/1/chromeos/dbus/session_manager...
> chromeos/dbus/session_manager_client.cc:516: // can be tested on desktop
builds.
> On 2013/05/13 10:27:14, Mattias Nissler wrote:
> > On 2013/05/13 09:39:23, Joao da Silva wrote:
> > > On 2013/05/10 12:40:15, Mattias Nissler wrote:
> > > > If you need this, why not extract the policy key from the policy blob
> here?
> > > 
> > > That'd be my preferred solution, but chromeos/ can't #include the policy
> > > protobufs. There's no good place outside chrome/ to move them to for now.
> > 
> > Bummer. Any chance to get an exception into chromeos/DEPS for the
> > device_management_backend.pb.h? That's not nice either, but more honest than
> > just  screwing up the interface here in order to be able to implement stubs.
> 
> Can we propose that change in another CL? I'd rather try to find a better
> top-level dir for the protobufs.

[Joao da Silva, 2013-05-13 16:44:24]

We discussed offline how to make the protobufs visible, and agreed to move them
to a new top-level //policy directory or a //component/policy component so that
code outside chrome/ can use them.

This CL could introduce a DEPS exception to #include those protobufs from
//chromeos/ for now, but that would require their visibility to be set with a
"POLICY_EXPORT" macro, essentially making symbols from the browser component
visible to others; that didn't feel right, so we'll change the current CL after
moving the protobufs.

@mnissler: are you OK with the current CL as-is?

[Mattias Nissler (ping if slow), 2013-05-13 16:57:09]

On 2013/05/13 16:44:24, Joao da Silva wrote:
> We discussed offline how to make the protobufs visible, and agreed to move
them
> to a new top-level //policy directory or a //component/policy component so
that
> code outside chrome/ can use them.
> 
> This CL could introduce a DEPS exception to #include those protobufs from
> //chromeos/ for now, but that would require their visibility to be set with a
> "POLICY_EXPORT" macro, essentially making symbols from the browser component
> visible to others; that didn't feel right, so we'll change the current CL
after
> moving the protobufs.
> 
> @mnissler: are you OK with the current CL as-is?

Yes, I am. LGTM after you update the code with a TODO pointing at the bug you
filed for the follow-up change.

[commit-bot: I haz the power, 2013-05-13 17:01:52]

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

[commit-bot: I haz the power, 2013-05-13 19:32:31]

Retried try job too often on win_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-05-13 21:25:01]

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

[commit-bot: I haz the power, 2013-05-14 00:12:04]

Change committed as 199861