Enable Enterprise enrollment on desktop builds.

Enable Enterprise enrollment on desktop builds.

This change implements some of the DBus stub methods so that enterprise enrollment works on desktop builds. That will make development of features that depend on enrollment faster for developers that use this workflow (e.g. for kiosk enterprise apps, public accounts, testing some device policies, etc).

- Override some of the directories and files involved with the enrollment state
- Simple stub implementation of the DBus calls involved
- Write a persistent cache of the install attributes
- Cleaned up the stub for user cloud policy and made them persistent too
- Updated some tests

This change doesn't affect production code.

TBR=jochen@chromium.org
BUG=240269, 367674
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267640

[Joao da Silva, 2014-04-28 10:22:13]

Please have a look:

@Brett: please review base/path_service.cc. I believe you introduced the code
that makes paths absolute, which requires realpath() and thus Override() doesn't
work if the file doesn't exist yet. If there is a better way to override
non-existent files then I'm happy to update this change.

@Nikita: please review chrome/browser/chromeos [/login]

@Julian: please review components/policy.gypi, chrome/browser/chromeos{/policy,
/settings}, and maybe have a look at the session_manager_client.cc changes

@Satoru: please review chromeos/

Thanks!

[Nikita (slow), 2014-04-28 10:34:43]

On 2014/04/28 10:22:13, Joao da Silva wrote:
> @Nikita: please review chrome/browser/chromeos [/login]

lgtm

[Joao da Silva, 2014-04-28 11:24:56]

The CQ bit was checked by joaodasilva@chromium.org

[Joao da Silva, 2014-04-28 11:24:57]

The CQ bit was unchecked by joaodasilva@chromium.org

[pastarmovj, 2014-04-28 13:41:53]

lgtm

[Joao da Silva, 2014-04-29 16:09:04]

This is now ready to go, linking on windows works too.

brettw, satorux: friendly ping :-)

[brettw, 2014-04-29 22:41:50]

https://codereview.chromium.org/258743005/diff/60001/base/path_service.h
File base/path_service.h (right):

https://codereview.chromium.org/258743005/diff/60001/base/path_service.h#newc...
base/path_service.h:61: static void OverrideWithAbsolutePath(int key, const
base::FilePath& path);
I'd rather see this as an additional flag to OverrideAndCreateIfNeeded than a
new function. I think there are only a handful of callers to update.

[satorux1, 2014-04-29 23:50:12]

https://codereview.chromium.org/258743005/diff/60001/chromeos/dbus/fake_crypt...
File chromeos/dbus/fake_cryptohome_client.cc (right):

https://codereview.chromium.org/258743005/diff/60001/chromeos/dbus/fake_crypt...
chromeos/dbus/fake_cryptohome_client.cc:274: // low-level protobuf API instead
to just write the name-value pairs.
This looks tricky and fragile. Would it be difficult to move the .proto file to
somewhere that can be accessed from here? What about policy/proto?

[Joao da Silva, 2014-04-30 13:08:06]

@brettw: please have another look at path_service; I've added a new test for the
new argument too.

@satorux: please see inline re moving the protobufs.

@pastarmovj: there's a new unit test in
enterprise_install_attributes_unittest.cc, can you have another look there?

Thanks!

https://codereview.chromium.org/258743005/diff/60001/base/path_service.h
File base/path_service.h (right):

https://codereview.chromium.org/258743005/diff/60001/base/path_service.h#newc...
base/path_service.h:61: static void OverrideWithAbsolutePath(int key, const
base::FilePath& path);
On 2014/04/29 22:41:51, brettw wrote:
> I'd rather see this as an additional flag to OverrideAndCreateIfNeeded than a
> new function. I think there are only a handful of callers to update.

Done.

https://codereview.chromium.org/258743005/diff/60001/chromeos/dbus/fake_crypt...
File chromeos/dbus/fake_cryptohome_client.cc (right):

https://codereview.chromium.org/258743005/diff/60001/chromeos/dbus/fake_crypt...
chromeos/dbus/fake_cryptohome_client.cc:274: // low-level protobuf API instead
to just write the name-value pairs.
On 2014/04/29 23:50:12, satorux1 wrote:
> This looks tricky and fragile. Would it be difficult to move the .proto file
to
> somewhere that can be accessed from here? What about policy/proto?

Unfortunately it's a bit tricky to move those protobufs, because they are also
pulled by some ChromeOS packages from a git mirror and all of that would have to
be updated too.

Note that the install attributes format is quite stable, given that devices must
keep their enrollment state across updates.

I don't think it's worth the hassle given that this is just a convenience
feature; if it breaks then only developers will notice. For the time being I've
added a new unittest that verifies that EnterpriseInstallAttributes parses the
stub cache produced by this function. If you feel strongly then I'll file a bug
to move those protobufs eventually.

[brettw, 2014-04-30 16:24:34]

base lgtm

[satorux1, 2014-05-01 10:23:11]

chromeos/... LGTM

[Joao da Silva, 2014-05-01 10:48:12]

The CQ bit was checked by joaodasilva@chromium.org

[commit-bot: I haz the power, 2014-05-01 10:48:29]

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

[commit-bot: I haz the power, 2014-05-01 11:02:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-01 11:02:33]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium

[Joao da Silva, 2014-05-01 11:16:38]

Jochen to TBR for the mechanical changes to

  chrome/app/chrome_main_delegate.cc
  chrome/browser/extensions/api/file_system/file_system_apitest.cc

[Joao da Silva, 2014-05-01 11:16:48]

The CQ bit was checked by joaodasilva@chromium.org

[commit-bot: I haz the power, 2014-05-01 11:16:52]

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

[commit-bot: I haz the power, 2014-05-01 11:48:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-01 11:48:37]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium

[Joao da Silva, 2014-05-01 12:55:43]

The CQ bit was checked by joaodasilva@chromium.org

[commit-bot: I haz the power, 2014-05-01 12:56:06]

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

[commit-bot: I haz the power, 2014-05-01 13:49:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-01 13:49:43]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium

[Joao da Silva, 2014-05-01 15:39:59]

The CQ bit was checked by joaodasilva@chromium.org

[commit-bot: I haz the power, 2014-05-01 15:40:33]

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

[commit-bot: I haz the power, 2014-05-01 17:02:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-01 17:03:00]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium

[Joao da Silva, 2014-05-01 17:08:47]

The CQ bit was checked by joaodasilva@chromium.org

[commit-bot: I haz the power, 2014-05-01 17:11:28]

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

[commit-bot: I haz the power, 2014-05-01 18:16:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-01 18:16:21]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium

[Joao da Silva, 2014-05-01 20:25:50]

The CQ bit was checked by joaodasilva@chromium.org

[commit-bot: I haz the power, 2014-05-01 20:27:45]

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

[commit-bot: I haz the power, 2014-05-01 21:54:01]

Change committed as 267640

[falken, 2014-05-02 05:21:15]

On 2014/05/01 21:54:01, I haz the power (commit-bot) wrote:
> Change committed as 267640

Sorry to revert this change. It seems to have broken the following test from
base_unittests and unit_tests on chromium.webkit Android Tests (dbg) builder:
PathServiceTest.Override

Log:
http://build.chromium.org/p/chromium.webkit/builders/Android%20Tests%20%28dbg...
Snippet from log:
C  237s Main  [FAIL] PathServiceTest.Override:
C  237s Main  ../../base/path_service_unittest.cc:184: Failure
C  237s Main  Value of: PathService::OverrideAndCreateIfNeeded(my_special_key,
non_existent, false, false)
C  237s Main    Actual: true
C  237s Main  Expected: false

I plan to revert shortly.

[falken, 2014-05-02 05:22:20]

A revert of this CL has been created in
https://codereview.chromium.org/265013002/ by falken@chromium.org.

The reason for reverting is: It seems to have broken the following test from
base_unittests and unit_tests on chromium.webkit Android Tests (dbg) builder:
PathServiceTest.Override

Log:
http://build.chromium.org/p/chromium.webkit/builders/Android%20Tests%20%28dbg...

Snippet from log:
C  237s Main  [FAIL] PathServiceTest.Override:
C  237s Main  ../../base/path_service_unittest.cc:184: Failure
C  237s Main  Value of: PathService::OverrideAndCreateIfNeeded(my_special_key,
non_existent, false, false)
C  237s Main    Actual: true
C  237s Main  Expected: false.

[Mattias Nissler (ping if slow), 2014-05-02 08:09:28]

https://codereview.chromium.org/258743005/diff/80001/chromeos/dbus/fake_crypt...
File chromeos/dbus/fake_cryptohome_client.cc (right):

https://codereview.chromium.org/258743005/diff/80001/chromeos/dbus/fake_crypt...
chromeos/dbus/fake_cryptohome_client.cc:274: // low-level protobuf API instead
to just write the name-value pairs.
Are you serious? Why not just place a copy of the protobuf file or find a way to
make it accessible? This is a big hack.

[Joao da Silva, 2014-05-02 08:22:09]

https://codereview.chromium.org/258743005/diff/80001/chromeos/dbus/fake_crypt...
File chromeos/dbus/fake_cryptohome_client.cc (right):

https://codereview.chromium.org/258743005/diff/80001/chromeos/dbus/fake_crypt...
chromeos/dbus/fake_cryptohome_client.cc:274: // low-level protobuf API instead
to just write the name-value pairs.
On 2014/05/02 08:09:29, Mattias Nissler wrote:
> Are you serious? Why not just place a copy of the protobuf file or find a way
to
> make it accessible? This is a big hack.

I don't think we want to have a copy that may get out of sync and that may be
mistaken for the real file.

Code at //chromeos can't include files from //chrome, so the only way to make it
accessible is to move the protobuf. However that requires creating yet another
git mirror and updating the chromeos packages again, and IMO it's not worth the
hassle.

What is it that you find so surprising about this implementation?

[Mattias Nissler (ping if slow), 2014-05-02 08:34:00]

https://codereview.chromium.org/258743005/diff/80001/chromeos/dbus/fake_crypt...
File chromeos/dbus/fake_cryptohome_client.cc (right):

https://codereview.chromium.org/258743005/diff/80001/chromeos/dbus/fake_crypt...
chromeos/dbus/fake_cryptohome_client.cc:274: // low-level protobuf API instead
to just write the name-value pairs.
On 2014/05/02 08:22:10, Joao da Silva wrote:
> On 2014/05/02 08:09:29, Mattias Nissler wrote:
> > Are you serious? Why not just place a copy of the protobuf file or find a
way
> to
> > make it accessible? This is a big hack.
> 
> I don't think we want to have a copy that may get out of sync and that may be
> mistaken for the real file.

Protobufs need to be backward-compatible, so I don't a copy is too bad. Also,
your code here gets out of sync just as well, and is much harder to fix.

> 
> Code at //chromeos can't include files from //chrome, so the only way to make
it
> accessible is to move the protobuf. However that requires creating yet another
> git mirror and updating the chromeos packages again, and IMO it's not worth
the
> hassle.
> 
> What is it that you find so surprising about this implementation?

Maintenance sucks - you have to understand the details of the protobuf wire
format to be able to understand and fix this.

[Joao da Silva, 2014-05-02 08:43:28]

https://codereview.chromium.org/258743005/diff/80001/chromeos/dbus/fake_crypt...
File chromeos/dbus/fake_cryptohome_client.cc (right):

https://codereview.chromium.org/258743005/diff/80001/chromeos/dbus/fake_crypt...
chromeos/dbus/fake_cryptohome_client.cc:274: // low-level protobuf API instead
to just write the name-value pairs.
On 2014/05/02 08:34:01, Mattias Nissler wrote:
> On 2014/05/02 08:22:10, Joao da Silva wrote:
> > On 2014/05/02 08:09:29, Mattias Nissler wrote:
> > > Are you serious? Why not just place a copy of the protobuf file or find a
> way
> > to
> > > make it accessible? This is a big hack.
> > 
> > I don't think we want to have a copy that may get out of sync and that may
be
> > mistaken for the real file.
> 
> Protobufs need to be backward-compatible, so I don't a copy is too bad. Also,
> your code here gets out of sync just as well, and is much harder to fix.
> 

That's fair.

> > 
> > Code at //chromeos can't include files from //chrome, so the only way to
make
> it
> > accessible is to move the protobuf. However that requires creating yet
another
> > git mirror and updating the chromeos packages again, and IMO it's not worth
> the
> > hassle.
> > 
> > What is it that you find so surprising about this implementation?
> 
> Maintenance sucks - you have to understand the details of the protobuf wire
> format to be able to understand and fix this.

Indeed, but as you said above it needs to be backwards compatible so I don't
expect this code to require updates.

I'll just bite the bullet and file a bug to move the protobuf.

[Mattias Nissler (ping if slow), 2014-05-02 09:32:58]

https://codereview.chromium.org/258743005/diff/80001/chromeos/dbus/fake_crypt...
File chromeos/dbus/fake_cryptohome_client.cc (right):

https://codereview.chromium.org/258743005/diff/80001/chromeos/dbus/fake_crypt...
chromeos/dbus/fake_cryptohome_client.cc:274: // low-level protobuf API instead
to just write the name-value pairs.
On 2014/05/02 08:43:29, Joao da Silva wrote:
> On 2014/05/02 08:34:01, Mattias Nissler wrote:
> > On 2014/05/02 08:22:10, Joao da Silva wrote:
> > > On 2014/05/02 08:09:29, Mattias Nissler wrote:
> > > > Are you serious? Why not just place a copy of the protobuf file or find
a
> > way
> > > to
> > > > make it accessible? This is a big hack.
> > > 
> > > I don't think we want to have a copy that may get out of sync and that may
> be
> > > mistaken for the real file.
> > 
> > Protobufs need to be backward-compatible, so I don't a copy is too bad.
Also,
> > your code here gets out of sync just as well, and is much harder to fix.
> > 
> 
> That's fair.
> 
> > > 
> > > Code at //chromeos can't include files from //chrome, so the only way to
> make
> > it
> > > accessible is to move the protobuf. However that requires creating yet
> another
> > > git mirror and updating the chromeos packages again, and IMO it's not
worth
> > the
> > > hassle.
> > > 
> > > What is it that you find so surprising about this implementation?
> > 
> > Maintenance sucks - you have to understand the details of the protobuf wire
> > format to be able to understand and fix this.
> 
> Indeed, but as you said above it needs to be backwards compatible so I don't
> expect this code to require updates.

That assumes (1) the code is entirely correct in its current form and (2) the
low-level protobuf API is not going to change. Even in the absence of these
points, writing code that is hard to read and maintain is something we try to
avoid :)

> 
> I'll just bite the bullet and file a bug to move the protobuf.