Allow setting of user and device policies in functional tests

Allow setting of user and device policies in functional tests

This CL extends the PolicyTestBase so that both user and device
policies can be set. Updates to user policies now lead to an immediate
refresh on all supported platforms (ChromeOS, Linux, Windows). Device
policies are supported only on ChromeOS and require a session manager
restart.

BUG=chromium-os:28027
TEST=All affected pyauto functional tests on desktop Linux and Chrome OS:
TEST=chromeos_onc.py, policy_prefs_ui.py, policy.py

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=131743

[frankf, 2012-04-04 01:44:01]

This is looking good! It'd be nice if you included the
DeviceEphemeralUsersEnabled test in this CL to serve as an example of how
SetDevicePolicy() is used.

http://codereview.chromium.org/9791023/diff/2001/chrome/test/pyautolib/policy...
File chrome/test/pyautolib/policy_base.py (right):

http://codereview.chromium.org/9791023/diff/2001/chrome/test/pyautolib/policy...
chrome/test/pyautolib/policy_base.py:21: # a new device policy blob and starting
Chrome and the session manager again.
Mention that this will log you out. The limitation of this approach as opposed
to going through the test server is that you can't test the scenario where a
device policy is changed while you're logged in.

http://codereview.chromium.org/9791023/diff/2001/chrome/test/pyautolib/policy...
chrome/test/pyautolib/policy_base.py:26: # the right files on Linux and Mac.
delete this line.

http://codereview.chromium.org/9791023/diff/2001/chrome/test/pyautolib/policy...
chrome/test/pyautolib/policy_base.py:134: flags.remove('--skip-oauth-login')
why do we need oauth-login?

http://codereview.chromium.org/9791023/diff/2001/chrome/test/pyautolib/policy...
chrome/test/pyautolib/policy_base.py:160: asn1der.Data(asn1der.NULL, '') ])
No spaces around brackets.

http://codereview.chromium.org/9791023/diff/2001/chrome/test/pyautolib/policy...
chrome/test/pyautolib/policy_base.py:176: if self.IsChromeOS():
Add comment on why you need this.

http://codereview.chromium.org/9791023/diff/2001/chrome/test/pyautolib/policy...
chrome/test/pyautolib/policy_base.py:324: def _GenerateDevicePolicyBlob(self,
device_policy=None, owner=None):
Move this before SetDevicePolicy. It's a good idea to define functions you
depend on first.

http://codereview.chromium.org/9791023/diff/2001/chrome/test/pyautolib/policy...
chrome/test/pyautolib/policy_base.py:350: policy_data.username = owner or ''
don't need 'or'.

[bartfab (slow), 2012-04-04 09:24:14]

Inline comments addressed. Regarding the DeviceEphemeralUsersEnabled test: I am
working on that as crosbug.com/27702 to keep this CL a bit leaner.

http://codereview.chromium.org/9791023/diff/2001/chrome/test/pyautolib/policy...
File chrome/test/pyautolib/policy_base.py (right):

http://codereview.chromium.org/9791023/diff/2001/chrome/test/pyautolib/policy...
chrome/test/pyautolib/policy_base.py:21: # a new device policy blob and starting
Chrome and the session manager again.
On 2012/04/04 01:44:01, frankf wrote:
> Mention that this will log you out. The limitation of this approach as opposed
> to going through the test server is that you can't test the scenario where a
> device policy is changed while you're logged in. 

Done.

http://codereview.chromium.org/9791023/diff/2001/chrome/test/pyautolib/policy...
chrome/test/pyautolib/policy_base.py:26: # the right files on Linux and Mac.
On 2012/04/04 01:44:01, frankf wrote:
> delete this line.

Done.

http://codereview.chromium.org/9791023/diff/2001/chrome/test/pyautolib/policy...
chrome/test/pyautolib/policy_base.py:134: flags.remove('--skip-oauth-login')
An OAuth token is required for Chrome to register with the DMServer/TestServer
for user policy fetching. Since a valid OAuth token kicks off Sync, that then
has to be explicitly disabled by appending --disable-sync below.

http://codereview.chromium.org/9791023/diff/2001/chrome/test/pyautolib/policy...
chrome/test/pyautolib/policy_base.py:160: asn1der.Data(asn1der.NULL, '') ])
On 2012/04/04 01:44:01, frankf wrote:
> No spaces around brackets.

Done.

http://codereview.chromium.org/9791023/diff/2001/chrome/test/pyautolib/policy...
chrome/test/pyautolib/policy_base.py:176: if self.IsChromeOS():
On 2012/04/04 01:44:01, frankf wrote:
> Add comment on why you need this.

Done.

http://codereview.chromium.org/9791023/diff/2001/chrome/test/pyautolib/policy...
chrome/test/pyautolib/policy_base.py:324: def _GenerateDevicePolicyBlob(self,
device_policy=None, owner=None):
Done. I reordered all functions so that they are defined before being used
wherever possible.

http://codereview.chromium.org/9791023/diff/2001/chrome/test/pyautolib/policy...
chrome/test/pyautolib/policy_base.py:350: policy_data.username = owner or ''
On 2012/04/04 01:44:01, frankf wrote:
> don't need 'or'.

Done.

[frankf, 2012-04-04 21:44:10]

http://codereview.chromium.org/9791023/diff/2001/chrome/test/pyautolib/policy...
File chrome/test/pyautolib/policy_base.py (right):

http://codereview.chromium.org/9791023/diff/2001/chrome/test/pyautolib/policy...
chrome/test/pyautolib/policy_base.py:134: flags.remove('--skip-oauth-login')
Previously, we explicitly registered with the DMServer and fetched DMTokens,
bypassing the need to have a OAuth token. In that case, it would have been
easier to make this work with a local auth server to be hermetic, right? I am
not going to press for this, but I want all design trade-offs to be thought
out...

On 2012/04/04 09:24:14, bartfab wrote:
> An OAuth token is required for Chrome to register with the DMServer/TestServer
> for user policy fetching. Since a valid OAuth token kicks off Sync, that then
> has to be explicitly disabled by appending --disable-sync below.

[bartfab (slow), 2012-04-05 08:24:45]

http://codereview.chromium.org/9791023/diff/2001/chrome/test/pyautolib/policy...
File chrome/test/pyautolib/policy_base.py (right):

http://codereview.chromium.org/9791023/diff/2001/chrome/test/pyautolib/policy...
chrome/test/pyautolib/policy_base.py:134: flags.remove('--skip-oauth-login')
We can make things hermetic just as easily. The moment we have a local auth
server, we will receive OAuth tokens from it that are not valid on the prod
infrastructure, so issues such as sync kicking in will go away. We do gain a lot
by using the OAuth login flow though: We can test that the browser handles
registration correctly; we can test that the browser respects policy right from
the start; we can use a local sync server to test sync as well...

So, disabling sync is a short-term solution that cuts us off from that service.
Once we have local auth, we will be able to safely enable sync again.

[Nirnimesh, 2012-04-05 19:05:30]

I have minor comments only.

http://codereview.chromium.org/9791023/diff/6003/chrome/test/functional/chrom...
File chrome/test/functional/chromeos_onc.py (right):

http://codereview.chromium.org/9791023/diff/6003/chrome/test/functional/chrom...
chrome/test/functional/chromeos_onc.py:28: if self.IsChromeOS():
This file applies to chromeos only. This if is redundant.

http://codereview.chromium.org/9791023/diff/6003/chrome/test/pyautolib/asn1de...
File chrome/test/pyautolib/asn1der.py (right):

http://codereview.chromium.org/9791023/diff/6003/chrome/test/pyautolib/asn1de...
chrome/test/pyautolib/asn1der.py:12: INTEGER = 2
global vars local to this file should have _ prefix

http://codereview.chromium.org/9791023/diff/6003/chrome/test/pyautolib/asn1de...
chrome/test/pyautolib/asn1der.py:17: 
nit: need 2 blank lines between functions in global scope.
Repeat below

http://codereview.chromium.org/9791023/diff/6003/chrome/test/pyautolib/asn1de...
chrome/test/pyautolib/asn1der.py:23: data: the data for the given tag.
nit: leave a blank line before Returns: section
Repeat below

http://codereview.chromium.org/9791023/diff/6003/chrome/test/pyautolib/policy...
File chrome/test/pyautolib/policy_base.py (right):

http://codereview.chromium.org/9791023/diff/6003/chrome/test/pyautolib/policy...
chrome/test/pyautolib/policy_base.py:45: warnings.filterwarnings("ignore",
category=DeprecationWarning)
nit: use ' instead of " for consistency

http://codereview.chromium.org/9791023/diff/6003/chrome/test/pyautolib/policy...
chrome/test/pyautolib/policy_base.py:364: lambda:
self._WriteDevicePolicyWithSessionManagerStopped())
lambda is redundant.

self.WaitForSessionManagerRestart(self._WriteDevicePolicyWithSessionManagerStopped)

will do

http://codereview.chromium.org/9791023/diff/6003/chrome/test/pyautolib/pyauto.py
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/9791023/diff/6003/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:443: 
This has creeped in from your other CL. Remove.

http://codereview.chromium.org/9791023/diff/6003/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:3675: def SetPolicies(self,
managed_platform=None, recommended_platform=None,
Isn't this obsolete? Remove.

[bartfab (slow), 2012-04-10 15:27:19]

Comments addressed.

https://chromiumcodereview.appspot.com/9791023/diff/6003/chrome/test/function...
File chrome/test/functional/chromeos_onc.py (right):

https://chromiumcodereview.appspot.com/9791023/diff/6003/chrome/test/function...
chrome/test/functional/chromeos_onc.py:28: if self.IsChromeOS():
On 2012/04/05 19:05:30, Nirnimesh wrote:
> This file applies to chromeos only. This if is redundant.

Done.

https://chromiumcodereview.appspot.com/9791023/diff/6003/chrome/test/pyautoli...
File chrome/test/pyautolib/asn1der.py (right):

https://chromiumcodereview.appspot.com/9791023/diff/6003/chrome/test/pyautoli...
chrome/test/pyautolib/asn1der.py:12: INTEGER = 2
I had actually copied this file from another place in the source tree. I will
fix it up as per your nits of course... but the other copy will remain deficient
:).

https://chromiumcodereview.appspot.com/9791023/diff/6003/chrome/test/pyautoli...
chrome/test/pyautolib/asn1der.py:17: 
On 2012/04/05 19:05:30, Nirnimesh wrote:
> nit: need 2 blank lines between functions in global scope.
> Repeat below

Done.

https://chromiumcodereview.appspot.com/9791023/diff/6003/chrome/test/pyautoli...
chrome/test/pyautolib/asn1der.py:23: data: the data for the given tag.
On 2012/04/05 19:05:30, Nirnimesh wrote:
> nit: leave a blank line before Returns: section
> Repeat below

Done.

https://chromiumcodereview.appspot.com/9791023/diff/6003/chrome/test/pyautoli...
File chrome/test/pyautolib/policy_base.py (right):

https://chromiumcodereview.appspot.com/9791023/diff/6003/chrome/test/pyautoli...
chrome/test/pyautolib/policy_base.py:45: warnings.filterwarnings("ignore",
category=DeprecationWarning)
On 2012/04/05 19:05:30, Nirnimesh wrote:
> nit: use ' instead of " for consistency

Done.

https://chromiumcodereview.appspot.com/9791023/diff/6003/chrome/test/pyautoli...
chrome/test/pyautolib/policy_base.py:364: lambda:
self._WriteDevicePolicyWithSessionManagerStopped())
On 2012/04/05 19:05:30, Nirnimesh wrote:
> lambda is redundant.
> 
>
self.WaitForSessionManagerRestart(self._WriteDevicePolicyWithSessionManagerStopped)
> 
> will do

Done.

https://chromiumcodereview.appspot.com/9791023/diff/6003/chrome/test/pyautoli...
File chrome/test/pyautolib/pyauto.py (right):

https://chromiumcodereview.appspot.com/9791023/diff/6003/chrome/test/pyautoli...
chrome/test/pyautolib/pyauto.py:443: 
No, this is actually used by PolicyTestBase.setUp(): Any existing vaults are
removed so that Chrome requests a DM token during login and so that tests are
properly isolated.

https://chromiumcodereview.appspot.com/9791023/diff/6003/chrome/test/pyautoli...
chrome/test/pyautolib/pyauto.py:3675: def SetPolicies(self,
managed_platform=None, recommended_platform=None,
Done, along with the TestingAutomationProvider method that this used to call.

[bartfab (slow), 2012-04-10 15:28:19]

Adding jhawkins as reviewer for the browser part.

[Nirnimesh, 2012-04-10 17:48:55]

LGTM

https://chromiumcodereview.appspot.com/9791023/diff/20001/chrome/test/pyautol...
File chrome/test/pyautolib/asn1der.py (right):

https://chromiumcodereview.appspot.com/9791023/diff/20001/chrome/test/pyautol...
chrome/test/pyautolib/asn1der.py:16: 
nit: need 2 blank lines here

[commit-bot: I haz the power, 2012-04-10 19:42:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/9791023/22001

[bartfab (slow), 2012-04-10 19:42:56]

Oops, forgot to wait for review by jhawkins. I unchecked the commit bit. Sorry.

[bartfab (slow), 2012-04-11 08:35:15]

Since Nirnimesh has actually worked on the TestingAutomationProvider before, his
review should be good for this entire CL after all.

[commit-bot: I haz the power, 2012-04-11 08:35:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/9791023/22001

[commit-bot: I haz the power, 2012-04-11 09:58:14]

Change committed as 131743