Wire up component cloud policy to device local accounts.

Wire up component cloud policy to device local accounts.

This enables policy-for-extensions running in Public Sessions, and for
extensions running as Kiosk Apps too.

TBR=bartfab@chromium.org
BUG=224596
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278518

[Joao da Silva, 2014-06-19 16:48:03]

PTAL, thanks!

[Bernhard Bauer, 2014-06-19 16:59:27]

LGTM

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

https://codereview.chromium.org/341043005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:455:
CHECK(PathService::Get(chromeos::DIR_DEVICE_LOCAL_ACCOUNT_EXTENSIONS,
Why a CHECK instead of a DCHECK?

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

https://codereview.chromium.org/341043005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:126: // The
given |broken| is about to be destroyed.
Nit: "broker"

[Joao da Silva, 2014-06-19 17:04:56]

Thanks for the quick review!

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

https://codereview.chromium.org/341043005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:455:
CHECK(PathService::Get(chromeos::DIR_DEVICE_LOCAL_ACCOUNT_EXTENSIONS,
On 2014/06/19 16:59:27, Bernhard Bauer wrote:
> Why a CHECK instead of a DCHECK?

Because this can't possibly fail, but if it does then we can't proceed. Also, a
DCHECK will skip the Get() call in release builds.

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

https://codereview.chromium.org/341043005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:126: // The
given |broken| is about to be destroyed.
On 2014/06/19 16:59:27, Bernhard Bauer wrote:
> Nit: "broker"

Done.

[Joao da Silva, 2014-06-19 17:05:24]

The CQ bit was checked by joaodasilva@chromium.org

[commit-bot: I haz the power, 2014-06-19 17:07:20]

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

[Joao da Silva, 2014-06-19 17:07:49]

TBRing this because QA needs a build ASAP, and today is a holiday in MUC. I'll
cleanup any further comments in a subsequent CL but landing this now unblocks QA
testing.

Please have a look when you can:

@Bartosz: the whole thing :-)

@Zel: please review chromeos/ and chrome/browser/chromeos/extension/

Tests coming in a subsequent CL too.

Thanks!

[Bernhard Bauer, 2014-06-19 17:22:44]

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

https://codereview.chromium.org/341043005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:455:
CHECK(PathService::Get(chromeos::DIR_DEVICE_LOCAL_ACCOUNT_EXTENSIONS,
On 2014/06/19 17:04:56, Joao da Silva wrote:
> On 2014/06/19 16:59:27, Bernhard Bauer wrote:
> > Why a CHECK instead of a DCHECK?
> 
> Because this can't possibly fail, but if it does then we can't proceed. Also,
a
> DCHECK will skip the Get() call in release builds.

Eh, "Can't possibly fail" is pretty much the definition of DCHECK :) (see
http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-)

The usual way to make sure the code is compiled in in Release builds as well is
to extract the result to a variable and then DCHECK that.

[Joao da Silva, 2014-06-19 19:33:45]

The CQ bit was checked by joaodasilva@chromium.org

[commit-bot: I haz the power, 2014-06-19 19:37:14]

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

[commit-bot: I haz the power, 2014-06-19 22:49:17]

Change committed as 278518

[bartfab (slow), 2014-06-20 09:17:26]

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc
(right):

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:70:
"ongnjlefhnoajpbodoldndkbkdgfomlp",  // Show Managed Storage
You skipped process here :). We always ask Will or Sumit to sign off on
apps/extensions before adding them to the whitelist. Since this is an extension
we wrote ourselves, I am not concerned that it may be requesting excessive
permissions or doing something nasty but it would still be good to get their
blessing.

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_local_account_browsertest.cc (right):

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:424:
base::FilePath GetCacheDirectoryForAccountID(const std::string& account_id) {
Nit: This should be renamed to indicate that it refers to the extensions cache
only.

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:426:
PathService::Get(chromeos::DIR_DEVICE_LOCAL_ACCOUNT_EXTENSIONS,
Nit: ADD_FAILURE() if PathService::Get() fails.

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_local_account_policy_provider.cc
(right):

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_provider.cc:112:
return component_policy_service_->is_initialized();
Is it correct for this method to return |true| if the
|component_policy_service_| has simply not been initialized yet?

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_provider.cc:150: //
The |component_policy_service_| relies on the broker's CloudPolicyCore,
Nit: s/broker/|broker|/

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_provider.cc:151: //
so destroy it if the broker is going away.
Nit: s/broker/|broker|/

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_local_account_policy_provider.h
(right):

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_provider.h:18:
#include "components/policy/core/common/cloud/resource_cache.h"
Nit: Move this to the implementation file. It is not used by the header.

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_provider.h:29: // and
RefreshPolicies becomes a no-op.
Document what happens to component policy in that case.

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_local_account_policy_service.cc
(right):

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:76: //
extensions are cached for |account_id|. This is also used for the
Nit: Why not reword the comment to talk about both forice-installed extensions
and component policy then, e.g.:

Get the subdirectory of the force-installed extension cache and the component
policy cache used for |account_id|.

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:78:
std::string EncodeAccountId(const std::string& account_id) {
Why did you rename this? "Encode" could mean a million things.
"GetCacheSubdirectory" is much more precise.

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_local_account_policy_service.h
(right):

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:91: //
Returns a directory where component policy for this account can be cached.
Nit: s/ a / the /

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:92: // The
DeviceLocalAccountPolicyService takes care of cleaning up caches of
Nit: s /caches of/caches belonging to/

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:127:
virtual void OnBrokerShutdown(DeviceLocalAccountPolicyBroker* broker) {}
Nit: Instead of exposing the internal detail that policy for device-local
accounts is handled by Brokers, how about calling this something like
OnPolicyServiceShutdown(const std::string& user_id)?

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:228:
OrphanCacheDeletionState orphan_cache_deletion_state_;
Nit: The type, member name and comment need to be updated to clarify that they
refer to the extensions cache only.

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:245: //
Path to the directory that contains the cached policies for components
Nit: s/policies/policy/

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:246: // for
device local accounts.
Nit: s/device local/device-local/

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/policy/te...
File chrome/browser/policy/test/policy_testserver.py (right):

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/policy/te...
chrome/browser/policy/test/policy_testserver.py:446: # If this is a
publicaccount request then get the username now and use it
Nit 1: s/publicaccount/|publicaccount|/
Nit 2: s/username/|username|/

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/policy/te...
chrome/browser/policy/test/policy_testserver.py:448: # policy for extensions in
public accounts.
1: Nit: s/accounts/sessions/
2: Do we use the |publicaccount| type for single-app kiosk mode as well? If so,
the comment should say "device-local accounts" instead of "public sessions."

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/policy/te...
chrome/browser/policy/test/policy_testserver.py:449: username =
self.server.GetPolicies().get('policy_user', None)
1: Why is this needed? The code is prepared to handle |username == None|.
2: In ProcessCloudPolicy(), there is a comment that explains why the |username|
is read from global configuration. Here, it is done without any further comment.

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/policy/te...
chrome/browser/policy/test/policy_testserver.py:452: username =
request.settings_entity_id
What happens if we get requests for multiple public sessions together?

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/policy/te...
chrome/browser/policy/test/policy_testserver.py:652: username: The username for
the response.
Nit: Add "May be None."

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/policy/te...
chrome/browser/policy/test/policy_testserver.py:762: policy_data.username =
username
Why can we not extract the username from |msg| here as we normally do for public
sessions?

https://codereview.chromium.org/341043005/diff/40001/chromeos/chromeos_paths.h
File chromeos/chromeos_paths.h (right):

https://codereview.chromium.org/341043005/diff/40001/chromeos/chromeos_paths....
chromeos/chromeos_paths.h:41: DIR_DEVICE_LOCAL_ACCOUNT_COMPONENT_POLICY,  //
Directory where policy for the
Nit: This sentence is hard to parse. How about something like:

"Directory where policy for components is stored for device-local accounts."

https://codereview.chromium.org/341043005/diff/40001/chromeos/chromeos_paths....
chromeos/chromeos_paths.h:43: // account is stored. Currently
Nit: s/account/accounts/

[Joao da Silva, 2014-06-20 11:48:46]

Publishes comment to Bartosz's review, to be addressed in a subsequent CL.

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc
(right):

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:70:
"ongnjlefhnoajpbodoldndkbkdgfomlp",  // Show Managed Storage
On 2014/06/20 09:17:24, bartfab wrote:
> You skipped process here :). We always ask Will or Sumit to sign off on
> apps/extensions before adding them to the whitelist. Since this is an
extension
> we wrote ourselves, I am not concerned that it may be requesting excessive
> permissions or doing something nasty but it would still be good to get their
> blessing.

Thanks, I've notified Will and Sumit. As discussed offline, this file should at
least have a comment about that process; having an OWNERS file with set
no-parent would be even better.

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_local_account_browsertest.cc (right):

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:424:
base::FilePath GetCacheDirectoryForAccountID(const std::string& account_id) {
On 2014/06/20 09:17:24, bartfab wrote:
> Nit: This should be renamed to indicate that it refers to the extensions cache
> only.

Done.

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:426:
PathService::Get(chromeos::DIR_DEVICE_LOCAL_ACCOUNT_EXTENSIONS,
On 2014/06/20 09:17:24, bartfab wrote:
> Nit: ADD_FAILURE() if PathService::Get() fails.

Done.

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_local_account_policy_provider.cc
(right):

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_provider.cc:112:
return component_policy_service_->is_initialized();
On 2014/06/20 09:17:25, bartfab wrote:
> Is it correct for this method to return |true| if the
> |component_policy_service_| has simply not been initialized yet?

Good observation. In the next CL the component_policy_service_ has already been
created by the time this is called; if it's NULL then it has been disabled via
the command line flag, and in that case this should return true.

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_provider.cc:150: //
The |component_policy_service_| relies on the broker's CloudPolicyCore,
On 2014/06/20 09:17:24, bartfab wrote:
> Nit: s/broker/|broker|/

Obsolete in the next CL.

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_provider.cc:151: //
so destroy it if the broker is going away.
On 2014/06/20 09:17:25, bartfab wrote:
> Nit: s/broker/|broker|/

Same

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_local_account_policy_provider.h
(right):

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_provider.h:18:
#include "components/policy/core/common/cloud/resource_cache.h"
On 2014/06/20 09:17:25, bartfab wrote:
> Nit: Move this to the implementation file. It is not used by the header.

Obsolete in the next CL.

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_provider.h:29: // and
RefreshPolicies becomes a no-op.
On 2014/06/20 09:17:25, bartfab wrote:
> Document what happens to component policy in that case.

Done.

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_local_account_policy_service.cc
(right):

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:76: //
extensions are cached for |account_id|. This is also used for the
On 2014/06/20 09:17:25, bartfab wrote:
> Nit: Why not reword the comment to talk about both forice-installed extensions
> and component policy then, e.g.:
> 
> Get the subdirectory of the force-installed extension cache and the component
> policy cache used for |account_id|.

Done.

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:78:
std::string EncodeAccountId(const std::string& account_id) {
On 2014/06/20 09:17:25, bartfab wrote:
> Why did you rename this? "Encode" could mean a million things.
> "GetCacheSubdirectory" is much more precise.

Done.

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_local_account_policy_service.h
(right):

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:91: //
Returns a directory where component policy for this account can be cached.
On 2014/06/20 09:17:25, bartfab wrote:
> Nit: s/ a / the /

Obsolete in the next CL

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:92: // The
DeviceLocalAccountPolicyService takes care of cleaning up caches of
On 2014/06/20 09:17:25, bartfab wrote:
> Nit: s /caches of/caches belonging to/

Obsolete in the next CL

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:127:
virtual void OnBrokerShutdown(DeviceLocalAccountPolicyBroker* broker) {}
On 2014/06/20 09:17:25, bartfab wrote:
> Nit: Instead of exposing the internal detail that policy for device-local
> accounts is handled by Brokers, how about calling this something like
> OnPolicyServiceShutdown(const std::string& user_id)?

This method has been removed.

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:228:
OrphanCacheDeletionState orphan_cache_deletion_state_;
On 2014/06/20 09:17:25, bartfab wrote:
> Nit: The type, member name and comment need to be updated to clarify that they
> refer to the extensions cache only.

Done.

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:245: //
Path to the directory that contains the cached policies for components
On 2014/06/20 09:17:25, bartfab wrote:
> Nit: s/policies/policy/

Done.

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:246: // for
device local accounts.
On 2014/06/20 09:17:25, bartfab wrote:
> Nit: s/device local/device-local/

Done.

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/policy/te...
File chrome/browser/policy/test/policy_testserver.py (right):

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/policy/te...
chrome/browser/policy/test/policy_testserver.py:446: # If this is a
publicaccount request then get the username now and use it
On 2014/06/20 09:17:25, bartfab wrote:
> Nit 1: s/publicaccount/|publicaccount|/
> Nit 2: s/username/|username|/

Done.

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/policy/te...
chrome/browser/policy/test/policy_testserver.py:448: # policy for extensions in
public accounts.
On 2014/06/20 09:17:25, bartfab wrote:
> 1: Nit: s/accounts/sessions/
> 2: Do we use the |publicaccount| type for single-app kiosk mode as well? If
so,
> the comment should say "device-local accounts" instead of "public sessions."

Done.

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/policy/te...
chrome/browser/policy/test/policy_testserver.py:449: username =
self.server.GetPolicies().get('policy_user', None)
On 2014/06/20 09:17:25, bartfab wrote:
> 1: Why is this needed? The code is prepared to handle |username == None|.
> 2: In ProcessCloudPolicy(), there is a comment that explains why the
|username|
> is read from global configuration. Here, it is done without any further
comment.

Done.

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/policy/te...
chrome/browser/policy/test/policy_testserver.py:452: username =
request.settings_entity_id
On 2014/06/20 09:17:25, bartfab wrote:
> What happens if we get requests for multiple public sessions together?

Then this doesn't work :-) The current client implementation has one separate
broker for each account, so there is one request to the server per account and
this works.

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/policy/te...
chrome/browser/policy/test/policy_testserver.py:652: username: The username for
the response.
On 2014/06/20 09:17:25, bartfab wrote:
> Nit: Add "May be None."

Done.

https://codereview.chromium.org/341043005/diff/40001/chrome/browser/policy/te...
chrome/browser/policy/test/policy_testserver.py:762: policy_data.username =
username
On 2014/06/20 09:17:25, bartfab wrote:
> Why can we not extract the username from |msg| here as we normally do for
public
> sessions?

Because the |msg| here is one of the repeated PolicyFetchRequests and doesn't
necessarily have the username. If this is a request for policy-for-extensions
then it doesn't have the username, for example. That's why it's extracted from
the sibling PolicyFetchRequest (for publicaccounts policy) and then passed in
here.

https://codereview.chromium.org/341043005/diff/40001/chromeos/chromeos_paths.h
File chromeos/chromeos_paths.h (right):

https://codereview.chromium.org/341043005/diff/40001/chromeos/chromeos_paths....
chromeos/chromeos_paths.h:41: DIR_DEVICE_LOCAL_ACCOUNT_COMPONENT_POLICY,  //
Directory where policy for the
On 2014/06/20 09:17:25, bartfab wrote:
> Nit: This sentence is hard to parse. How about something like:
> 
> "Directory where policy for components is stored for device-local accounts."

Done.

https://codereview.chromium.org/341043005/diff/40001/chromeos/chromeos_paths....
chromeos/chromeos_paths.h:43: // account is stored. Currently
On 2014/06/20 09:17:25, bartfab wrote:
> Nit: s/account/accounts/

Done.