Created AttestationPolicyObserver.

Created AttestationPolicyObserver.
    
AttestationPolicyObserver performs Chrome OS attestation work in
response to policy changes.
    
This CL integrates AttestationPolicyObserver with the Chrome OS
device policy infrastructure.  It also defines a AttestationCAClient
skeleton which is necessary to instantiate AttestationFlow and changes
AttestationFlow to take ownership of its ServerProxy instance.
    
BUG=chromium:219959
TEST=unit_tests; chromeos_unittests

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

[Mattias Nissler (ping if slow), 2013-03-27 14:16:33]

https://codereview.chromium.org/12556004/diff/1/chrome/browser/chromeos/attes...
File chrome/browser/chromeos/attestation/attestation_ca_client.cc (right):

https://codereview.chromium.org/12556004/diff/1/chrome/browser/chromeos/attes...
chrome/browser/chromeos/attestation/attestation_ca_client.cc:22: //
TODO(dkrahn): Implement.
Might want to just put a NOTIMPLEMENTED() here for now and remove the comment
and code.

https://codereview.chromium.org/12556004/diff/1/chrome/browser/chromeos/attes...
File chrome/browser/chromeos/attestation/attestation_policy_observer.cc (right):

https://codereview.chromium.org/12556004/diff/1/chrome/browser/chromeos/attes...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:21:
policy::CloudPolicyManager* policy_manager,
Just pass a CloudPolicyClient pointer if that's all you need? I guess I'm
missing context on who's going to use this observer object, where it gets
instantiated etc.

https://codereview.chromium.org/12556004/diff/1/chrome/browser/chromeos/attes...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:56: //
TODO(dkrahn): Generate EMK and upload EMCert if necessary.
Are you meaning to commit this in the half-baked state here?

https://codereview.chromium.org/12556004/diff/1/chrome/browser/chromeos/setti...
File chrome/browser/chromeos/settings/device_settings_provider.cc (right):

https://codereview.chromium.org/12556004/diff/1/chrome/browser/chromeos/setti...
chrome/browser/chromeos/settings/device_settings_provider.cc:625:
new_values_cache->SetBoolean(kDeviceAttestationEnabled, attestation_enabled);
you could just pass policy.attestation_settings().attestation_enabled() here.

https://codereview.chromium.org/12556004/diff/1/chrome/browser/policy/browser...
File chrome/browser/policy/browser_policy_connector.cc (right):

https://codereview.chromium.org/12556004/diff/1/chrome/browser/policy/browser...
chrome/browser/policy/browser_policy_connector.cc:223: 
nit: remove

https://codereview.chromium.org/12556004/diff/1/chrome/browser/policy/browser...
File chrome/browser/policy/browser_policy_connector.h (right):

https://codereview.chromium.org/12556004/diff/1/chrome/browser/policy/browser...
chrome/browser/policy/browser_policy_connector.h:25: }}
braces on separate lines.

https://codereview.chromium.org/12556004/diff/1/chrome/browser/policy/browser...
chrome/browser/policy/browser_policy_connector.h:230:
attestation_policy_observer_;
Not sure this really belongs here. While this device setting also be relevant to
consumer devices? If yes, you want to move this to e.g.
chrome_browser_parts_chromeos.cc or g_browser_process.

https://codereview.chromium.org/12556004/diff/1/chromeos/attestation/attestat...
File chromeos/attestation/attestation_flow_unittest.cc (right):

https://codereview.chromium.org/12556004/diff/1/chromeos/attestation/attestat...
chromeos/attestation/attestation_flow_unittest.cc:72:
StrictMock<MockServerProxy>* proxy(new StrictMock<MockServerProxy>());
should declare this immediately as a scoped_ptr.

https://codereview.chromium.org/12556004/diff/1/chromeos/attestation/attestat...
chromeos/attestation/attestation_flow_unittest.cc:114: AttestationFlow
flow(&async_caller, &client, scoped_proxy.Pass());
And if you have a good reason not to, there's make_scoped_ptr()

[dkrahn, 2013-03-27 21:20:44]

https://codereview.chromium.org/12556004/diff/1/chrome/browser/chromeos/attes...
File chrome/browser/chromeos/attestation/attestation_ca_client.cc (right):

https://codereview.chromium.org/12556004/diff/1/chrome/browser/chromeos/attes...
chrome/browser/chromeos/attestation/attestation_ca_client.cc:22: //
TODO(dkrahn): Implement.
On 2013/03/27 14:16:34, Mattias Nissler wrote:
> Might want to just put a NOTIMPLEMENTED() here for now and remove the comment
> and code.

Done.

https://codereview.chromium.org/12556004/diff/1/chrome/browser/chromeos/attes...
File chrome/browser/chromeos/attestation/attestation_policy_observer.cc (right):

https://codereview.chromium.org/12556004/diff/1/chrome/browser/chromeos/attes...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:21:
policy::CloudPolicyManager* policy_manager,
On 2013/03/27 14:16:34, Mattias Nissler wrote:
> Just pass a CloudPolicyClient pointer if that's all you need? I guess I'm
> missing context on who's going to use this observer object, where it gets
> instantiated etc.
From my reading of the code (correct me if I'm wrong) when this object is
initialized the client does not yet exist, but it should exist when a settings
change event is received.  If we change where this class is instantiated then
this can change.

https://codereview.chromium.org/12556004/diff/1/chrome/browser/chromeos/attes...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:56: //
TODO(dkrahn): Generate EMK and upload EMCert if necessary.
On 2013/03/27 14:16:34, Mattias Nissler wrote:
> Are you meaning to commit this in the half-baked state here?
Yes.  Before I do a lot of work I want your +1 that I'm on the right track,
using the right objects, etc..

https://codereview.chromium.org/12556004/diff/1/chrome/browser/chromeos/setti...
File chrome/browser/chromeos/settings/device_settings_provider.cc (right):

https://codereview.chromium.org/12556004/diff/1/chrome/browser/chromeos/setti...
chrome/browser/chromeos/settings/device_settings_provider.cc:625:
new_values_cache->SetBoolean(kDeviceAttestationEnabled, attestation_enabled);
On 2013/03/27 14:16:34, Mattias Nissler wrote:
> you could just pass policy.attestation_settings().attestation_enabled() here.

Done.

https://codereview.chromium.org/12556004/diff/1/chrome/browser/policy/browser...
File chrome/browser/policy/browser_policy_connector.cc (right):

https://codereview.chromium.org/12556004/diff/1/chrome/browser/policy/browser...
chrome/browser/policy/browser_policy_connector.cc:223: 
On 2013/03/27 14:16:34, Mattias Nissler wrote:
> nit: remove

Done.

https://codereview.chromium.org/12556004/diff/1/chrome/browser/policy/browser...
File chrome/browser/policy/browser_policy_connector.h (right):

https://codereview.chromium.org/12556004/diff/1/chrome/browser/policy/browser...
chrome/browser/policy/browser_policy_connector.h:25: }}
On 2013/03/27 14:16:34, Mattias Nissler wrote:
> braces on separate lines.

Done.

https://codereview.chromium.org/12556004/diff/1/chrome/browser/policy/browser...
chrome/browser/policy/browser_policy_connector.h:230:
attestation_policy_observer_;
On 2013/03/27 14:16:34, Mattias Nissler wrote:
> Not sure this really belongs here. While this device setting also be relevant
to
> consumer devices? If yes, you want to move this to e.g.
> chrome_browser_parts_chromeos.cc or g_browser_process.
This is only for enterprise managed devices.  I wasn't sure where to put this, I
also considered putting it in DeviceCloudPolicyManagerChromeOS.  I chose here
because this is where a few other policy-reactive objects seem to be (e.g.
network_configuration_updater_).  Let me know where you think is best.

https://codereview.chromium.org/12556004/diff/1/chromeos/attestation/attestat...
File chromeos/attestation/attestation_flow_unittest.cc (right):

https://codereview.chromium.org/12556004/diff/1/chromeos/attestation/attestat...
chromeos/attestation/attestation_flow_unittest.cc:72:
StrictMock<MockServerProxy>* proxy(new StrictMock<MockServerProxy>());
On 2013/03/27 14:16:34, Mattias Nissler wrote:
> should declare this immediately as a scoped_ptr.

Done.

https://codereview.chromium.org/12556004/diff/1/chromeos/attestation/attestat...
chromeos/attestation/attestation_flow_unittest.cc:114: AttestationFlow
flow(&async_caller, &client, scoped_proxy.Pass());
On 2013/03/27 14:16:34, Mattias Nissler wrote:
> And if you have a good reason not to, there's make_scoped_ptr()
The compiler has trouble with the upcast here because of the Pass()
implementation; doing the two steps separately seems to be the only way.

[Mattias Nissler (ping if slow), 2013-04-02 12:49:39]

Should we get together and discuss this at greater length?

https://codereview.chromium.org/12556004/diff/1/chrome/browser/chromeos/attes...
File chrome/browser/chromeos/attestation/attestation_policy_observer.cc (right):

https://codereview.chromium.org/12556004/diff/1/chrome/browser/chromeos/attes...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:21:
policy::CloudPolicyManager* policy_manager,
On 2013/03/27 21:20:44, Darren Krahn wrote:
> On 2013/03/27 14:16:34, Mattias Nissler wrote:
> > Just pass a CloudPolicyClient pointer if that's all you need? I guess I'm
> > missing context on who's going to use this observer object, where it gets
> > instantiated etc.
> From my reading of the code (correct me if I'm wrong) when this object is
> initialized the client does not yet exist, but it should exist when a settings
> change event is received.  If we change where this class is instantiated then
> this can change.

You're right about the CloudPolicyClient not necessarily being available. I
guess the question becomes: What is the correct lifetime for this object? Should
it hold CloudPolicyClient* pointer at all?

Have you thought about moving ownership of AttestationPolicyObserver to
DeviceCloudPolicyManagerChromeOS, and then have it call Connect() here after it
created a CloudPolicyClient?

https://codereview.chromium.org/12556004/diff/1/chrome/browser/chromeos/attes...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:56: //
TODO(dkrahn): Generate EMK and upload EMCert if necessary.
On 2013/03/27 21:20:44, Darren Krahn wrote:
> On 2013/03/27 14:16:34, Mattias Nissler wrote:
> > Are you meaning to commit this in the half-baked state here?
> Yes.  Before I do a lot of work I want your +1 that I'm on the right track,
> using the right objects, etc..

Hard for me to figure out with significant pieces missing, but I'll do my very
best :)

https://codereview.chromium.org/12556004/diff/1/chrome/browser/policy/browser...
File chrome/browser/policy/browser_policy_connector.h (right):

https://codereview.chromium.org/12556004/diff/1/chrome/browser/policy/browser...
chrome/browser/policy/browser_policy_connector.h:230:
attestation_policy_observer_;
On 2013/03/27 21:20:44, Darren Krahn wrote:
> On 2013/03/27 14:16:34, Mattias Nissler wrote:
> > Not sure this really belongs here. While this device setting also be
relevant
> to
> > consumer devices? If yes, you want to move this to e.g.
> > chrome_browser_parts_chromeos.cc or g_browser_process.
> This is only for enterprise managed devices.  I wasn't sure where to put this,
I
> also considered putting it in DeviceCloudPolicyManagerChromeOS.  I chose here
> because this is where a few other policy-reactive objects seem to be (e.g.
> network_configuration_updater_).  Let me know where you think is best.

The rule is basically that we put the observers wherever the code lives that is
concerned about the respective policy. In this case, that code doesn't exist
yet. Logically, it should probably be closer to
DeviceCloudPolicyManagerChromeOS, have you thought about how that solution would
look like?

https://codereview.chromium.org/12556004/diff/4002/chromeos/attestation/attes...
File chromeos/attestation/attestation_flow.h (right):

https://codereview.chromium.org/12556004/diff/4002/chromeos/attestation/attes...
chromeos/attestation/attestation_flow.h:56: // This class takes ownership only
of |server_proxy|.
nit: Comment is redundant given that you declare a scoped_ptr parameter.

https://codereview.chromium.org/12556004/diff/4002/chromeos/attestation/attes...
File chromeos/attestation/attestation_flow_unittest.cc (right):

https://codereview.chromium.org/12556004/diff/4002/chromeos/attestation/attes...
chromeos/attestation/attestation_flow_unittest.cc:114: AttestationFlow
flow(&async_caller, &client, proxy_interface.Pass());
So the following doesn't work here?

AttestationFlow flow(&async_caller, &client,
scoped_ptr<ServerProxy>(proxy_interface));

[dkrahn, 2013-04-04 21:27:26]

I think the latest patch seems to fit a bit better.  If not, let's schedule time
to discuss.

https://codereview.chromium.org/12556004/diff/1/chrome/browser/chromeos/attes...
File chrome/browser/chromeos/attestation/attestation_policy_observer.cc (right):

https://codereview.chromium.org/12556004/diff/1/chrome/browser/chromeos/attes...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:21:
policy::CloudPolicyManager* policy_manager,
On 2013/04/02 12:49:39, Mattias Nissler wrote:
> On 2013/03/27 21:20:44, Darren Krahn wrote:
> > On 2013/03/27 14:16:34, Mattias Nissler wrote:
> > > Just pass a CloudPolicyClient pointer if that's all you need? I guess I'm
> > > missing context on who's going to use this observer object, where it gets
> > > instantiated etc.
> > From my reading of the code (correct me if I'm wrong) when this object is
> > initialized the client does not yet exist, but it should exist when a
settings
> > change event is received.  If we change where this class is instantiated
then
> > this can change.
> 
> You're right about the CloudPolicyClient not necessarily being available. I
> guess the question becomes: What is the correct lifetime for this object?
Should
> it hold CloudPolicyClient* pointer at all?
> 
> Have you thought about moving ownership of AttestationPolicyObserver to
> DeviceCloudPolicyManagerChromeOS, and then have it call Connect() here after
it
> created a CloudPolicyClient?

Moved to DeviceCloudPolicyManagerChromeOS and added a Connect method.

https://codereview.chromium.org/12556004/diff/1/chrome/browser/chromeos/attes...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:56: //
TODO(dkrahn): Generate EMK and upload EMCert if necessary.
On 2013/04/02 12:49:39, Mattias Nissler wrote:
> On 2013/03/27 21:20:44, Darren Krahn wrote:
> > On 2013/03/27 14:16:34, Mattias Nissler wrote:
> > > Are you meaning to commit this in the half-baked state here?
> > Yes.  Before I do a lot of work I want your +1 that I'm on the right track,
> > using the right objects, etc..
> 
> Hard for me to figure out with significant pieces missing, but I'll do my very
> best :)

I've added more meat here, although there are still a few missing parts.  Unit
tests have also been added.

https://codereview.chromium.org/12556004/diff/1/chrome/browser/policy/browser...
File chrome/browser/policy/browser_policy_connector.h (right):

https://codereview.chromium.org/12556004/diff/1/chrome/browser/policy/browser...
chrome/browser/policy/browser_policy_connector.h:230:
attestation_policy_observer_;
On 2013/04/02 12:49:39, Mattias Nissler wrote:
> On 2013/03/27 21:20:44, Darren Krahn wrote:
> > On 2013/03/27 14:16:34, Mattias Nissler wrote:
> > > Not sure this really belongs here. While this device setting also be
> relevant
> > to
> > > consumer devices? If yes, you want to move this to e.g.
> > > chrome_browser_parts_chromeos.cc or g_browser_process.
> > This is only for enterprise managed devices.  I wasn't sure where to put
this,
> I
> > also considered putting it in DeviceCloudPolicyManagerChromeOS.  I chose
here
> > because this is where a few other policy-reactive objects seem to be (e.g.
> > network_configuration_updater_).  Let me know where you think is best.
> 
> The rule is basically that we put the observers wherever the code lives that
is
> concerned about the respective policy. In this case, that code doesn't exist
> yet. Logically, it should probably be closer to
> DeviceCloudPolicyManagerChromeOS, have you thought about how that solution
would
> look like?

Understood, I think it does fit well with DeviceCloudPolicyManagerChromeOS.

https://codereview.chromium.org/12556004/diff/4002/chromeos/attestation/attes...
File chromeos/attestation/attestation_flow.h (right):

https://codereview.chromium.org/12556004/diff/4002/chromeos/attestation/attes...
chromeos/attestation/attestation_flow.h:56: // This class takes ownership only
of |server_proxy|.
On 2013/04/02 12:49:39, Mattias Nissler wrote:
> nit: Comment is redundant given that you declare a scoped_ptr parameter.

Done.

https://codereview.chromium.org/12556004/diff/4002/chromeos/attestation/attes...
File chromeos/attestation/attestation_flow_unittest.cc (right):

https://codereview.chromium.org/12556004/diff/4002/chromeos/attestation/attes...
chromeos/attestation/attestation_flow_unittest.cc:114: AttestationFlow
flow(&async_caller, &client, proxy_interface.Pass());
On 2013/04/02 12:49:39, Mattias Nissler wrote:
> So the following doesn't work here?
> 
> AttestationFlow flow(&async_caller, &client,
> scoped_ptr<ServerProxy>(proxy_interface));

It doesn't work without release().

[Mattias Nissler (ping if slow), 2013-04-10 17:31:03]

Sorry for the delay.

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/attestation/attestation_ca_client.cc (right):

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/attestation_ca_client.cc:12: using
std::string;
no need?

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/attestation_ca_client.cc:17:
AttestationCAClient::AttestationCAClient() {}
nit: most code puts a blank line here.

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/attestation/attestation_policy_observer.cc (right):

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:29: //  
on_true - Called when status=succes and value=true.
*success

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:47: //  
on_success - Called when status=succes and result=true.
*success

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:112: if
(!cryptohome_client_) {
So if you have curlies here, you want them in line 103-105 as well :)

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:135:
base::Bind(DBusBoolRedirectCallback, on_does_exist, on_does_not_exist));
Given you use DBusBoolRedirectCallback only once in this file, this is probably
unnecessary indirection. Just have a CreateAttestationKeyIfNecessary function
that does the conditional check?

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:164: //
TODO(dkrahn): Upload the certificate when supported by policy_client_.
I think we have that now?

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/attestation/attestation_policy_observer.h (right):

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/attestation_policy_observer.h:38: // does
not take ownership of |settings| or |policy_client|.
No |settings| passed in here?

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/attestation_policy_observer.h:46: // Setters
useful for testing.
most policy code just takes these as ctor parameters FWIW

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc
(right):

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc:57:
cros_settings->AddSettingsProvider(&stub_settings_provider_);
Ah, CrosSettings mocking... sucks. I wish we had a better way of doing this, but
I don't know one.

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc:74:
run_loop.RunUntilIdle();
You can just do base::RunLoop().RunUntilIdle() in a single line.

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc
(right):

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:163:
attestation_policy_observer_->Connect(client());
Maybe it makes sense to only create the AttestationPolicyObserver instance here?
Does it do anything useful before?

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/proto/chrome_device_policy.proto (right):

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:277: // Whether
attestation features are enabled for the device.
Might want to elaborate a bit for people who have no idea what attestation is.

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_settings_provider.cc (right):

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_settings_provider.cc:70:
kDeviceAttestationEnabled,
Alphabetize (and fix kStartUpFlags if you like :D)

https://codereview.chromium.org/12556004/diff/18001/chromeos/attestation/atte...
File chromeos/attestation/attestation_flow_unittest.cc (right):

https://codereview.chromium.org/12556004/diff/18001/chromeos/attestation/atte...
chromeos/attestation/attestation_flow_unittest.cc:120:
scoped_ptr<ServerProxy>(proxy.release()));
Hm, all right, this isn't prettier either.

[dkrahn, 2013-04-12 01:17:28]

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/attestation/attestation_ca_client.cc (right):

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/attestation_ca_client.cc:12: using
std::string;
On 2013/04/10 17:31:03, Mattias Nissler wrote:
> no need?

Done.

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/attestation_ca_client.cc:17:
AttestationCAClient::AttestationCAClient() {}
On 2013/04/10 17:31:03, Mattias Nissler wrote:
> nit: most code puts a blank line here.

Done.

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/attestation/attestation_policy_observer.cc (right):

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:29: //  
on_true - Called when status=succes and value=true.
On 2013/04/10 17:31:03, Mattias Nissler wrote:
> *success

Done.

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:47: //  
on_success - Called when status=succes and result=true.
On 2013/04/10 17:31:03, Mattias Nissler wrote:
> *success

Done.

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:112: if
(!cryptohome_client_) {
On 2013/04/10 17:31:03, Mattias Nissler wrote:
> So if you have curlies here, you want them in line 103-105 as well :)

Curlies removed...

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:135:
base::Bind(DBusBoolRedirectCallback, on_does_exist, on_does_not_exist));
On 2013/04/10 17:31:03, Mattias Nissler wrote:
> Given you use DBusBoolRedirectCallback only once in this file, this is
probably
> unnecessary indirection. Just have a CreateAttestationKeyIfNecessary function
> that does the conditional check?

This will get used again in the next CL :).  Also, it hides the
DBusMethodCallStatus which I'd rather not put in my header file.

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:164: //
TODO(dkrahn): Upload the certificate when supported by policy_client_.
On 2013/04/10 17:31:03, Mattias Nissler wrote:
> I think we have that now?

Yep - coming in the next CL, don't want to squash at this point.

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/attestation/attestation_policy_observer.h (right):

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/attestation_policy_observer.h:38: // does
not take ownership of |settings| or |policy_client|.
On 2013/04/10 17:31:03, Mattias Nissler wrote:
> No |settings| passed in here?

Done.

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/attestation_policy_observer.h:46: // Setters
useful for testing.
On 2013/04/10 17:31:03, Mattias Nissler wrote:
> most policy code just takes these as ctor parameters FWIW

I wanted to make it easy to construct this class because it's non-trivial to
create an AttestationFlow instance.  I've added an alternative constructor which
takes these.

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc
(right):

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc:57:
cros_settings->AddSettingsProvider(&stub_settings_provider_);
On 2013/04/10 17:31:03, Mattias Nissler wrote:
> Ah, CrosSettings mocking... sucks. I wish we had a better way of doing this,
but
> I don't know one.

+1, I was surprised to see this but it gets the job done.

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc:74:
run_loop.RunUntilIdle();
On 2013/04/10 17:31:03, Mattias Nissler wrote:
> You can just do base::RunLoop().RunUntilIdle() in a single line.

Done.

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc
(right):

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:163:
attestation_policy_observer_->Connect(client());
On 2013/04/10 17:31:03, Mattias Nissler wrote:
> Maybe it makes sense to only create the AttestationPolicyObserver instance
here?
> Does it do anything useful before?

Done. Also removed Connect() since it's no longer needed.

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/proto/chrome_device_policy.proto (right):

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:277: // Whether
attestation features are enabled for the device.
On 2013/04/10 17:31:03, Mattias Nissler wrote:
> Might want to elaborate a bit for people who have no idea what attestation is.

Done.

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_settings_provider.cc (right):

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_settings_provider.cc:70:
kDeviceAttestationEnabled,
On 2013/04/10 17:31:03, Mattias Nissler wrote:
> Alphabetize (and fix kStartUpFlags if you like :D)

Done.

[Mattias Nissler (ping if slow), 2013-04-12 12:51:43]

LGTM with nits.

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/attestation/attestation_policy_observer.cc (right):

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:135:
base::Bind(DBusBoolRedirectCallback, on_does_exist, on_does_not_exist));
On 2013/04/12 01:17:29, Darren Krahn wrote:
> On 2013/04/10 17:31:03, Mattias Nissler wrote:
> > Given you use DBusBoolRedirectCallback only once in this file, this is
> probably
> > unnecessary indirection. Just have a CreateAttestationKeyIfNecessary
function
> > that does the conditional check?
> 
> This will get used again in the next CL :).  Also, it hides the
> DBusMethodCallStatus which I'd rather not put in my header file.

Fair.

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:164: //
TODO(dkrahn): Upload the certificate when supported by policy_client_.
On 2013/04/12 01:17:29, Darren Krahn wrote:
> On 2013/04/10 17:31:03, Mattias Nissler wrote:
> > I think we have that now?
> 
> Yep - coming in the next CL, don't want to squash at this point.

Nit: then update the comment

https://codereview.chromium.org/12556004/diff/27002/chrome/browser/chromeos/a...
File chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc
(right):

https://codereview.chromium.org/12556004/diff/27002/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc:71:
observer_.reset(new AttestationPolicyObserver(&policy_client_,
nit: There's no need to make observer_ a class member anymore - a local is good
enough.

https://codereview.chromium.org/12556004/diff/27002/chromeos/attestation/atte...
File chromeos/attestation/attestation_flow_unittest.cc (right):

https://codereview.chromium.org/12556004/diff/27002/chromeos/attestation/atte...
chromeos/attestation/attestation_flow_unittest.cc:120:
scoped_ptr<ServerProxy>(proxy.release()));
Can you revert this to the way you had it before for consistency within this
file?

[dkrahn, 2013-04-12 21:12:19]

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/attestation/attestation_policy_observer.cc (right):

https://codereview.chromium.org/12556004/diff/18001/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:164: //
TODO(dkrahn): Upload the certificate when supported by policy_client_.
On 2013/04/12 12:51:43, Mattias Nissler wrote:
> On 2013/04/12 01:17:29, Darren Krahn wrote:
> > On 2013/04/10 17:31:03, Mattias Nissler wrote:
> > > I think we have that now?
> > 
> > Yep - coming in the next CL, don't want to squash at this point.
> 
> Nit: then update the comment

Done.

https://codereview.chromium.org/12556004/diff/27002/chrome/browser/chromeos/a...
File chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc
(right):

https://codereview.chromium.org/12556004/diff/27002/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc:71:
observer_.reset(new AttestationPolicyObserver(&policy_client_,
On 2013/04/12 12:51:43, Mattias Nissler wrote:
> nit: There's no need to make observer_ a class member anymore - a local is
good
> enough.

Done.

https://codereview.chromium.org/12556004/diff/27002/chromeos/attestation/atte...
File chromeos/attestation/attestation_flow_unittest.cc (right):

https://codereview.chromium.org/12556004/diff/27002/chromeos/attestation/atte...
chromeos/attestation/attestation_flow_unittest.cc:120:
scoped_ptr<ServerProxy>(proxy.release()));
On 2013/04/12 12:51:43, Mattias Nissler wrote:
> Can you revert this to the way you had it before for consistency within this
> file?

Done.

[commit-bot: I haz the power, 2013-04-12 21:12:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dkrahn@google.com/12556004/37004

[commit-bot: I haz the power, 2013-04-12 21:12:56]

Presubmit check for 12556004-37004 failed and returned exit status 1.

INFO:root:Found 18 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/browser/chromeos/attestation/attestation_ca_client.cc
    chrome/browser/chromeos/attestation/attestation_ca_client.h
    chromeos/attestation/attestation_flow.cc
    chrome/chrome_browser_chromeos.gypi
    chromeos/attestation/attestation_flow_unittest.cc
    chromeos/attestation/attestation_flow.h
    chrome/browser/chromeos/attestation/attestation_policy_observer.h
    chromeos/attestation/mock_attestation_flow.h
    chromeos/attestation/mock_attestation_flow.cc
    chrome/browser/chromeos/attestation/attestation_policy_observer.cc
    chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc

Presubmit checks took 1.7s to calculate.

[dkrahn, 2013-04-12 23:28:42]

+satorux as OWNER

[satorux1, 2013-04-14 23:43:24]

What about creating chrome/browser/chromeos/attestation/OWNERS and 	
chromeos/attestation/OWNERS?

[dkrahn, 2013-04-16 20:23:30]

On 2013/04/14 23:43:24, satorux1 wrote:
> What about creating chrome/browser/chromeos/attestation/OWNERS and 	
> chromeos/attestation/OWNERS?

Added these OWNERS files with the enterprise team owning.

[Mattias Nissler (ping if slow), 2013-04-17 08:51:42]

https://chromiumcodereview.appspot.com/12556004/diff/51001/chrome/browser/chr...
File chrome/browser/chromeos/attestation/OWNERS (right):

https://chromiumcodereview.appspot.com/12556004/diff/51001/chrome/browser/chr...
chrome/browser/chromeos/attestation/OWNERS:6: pneubeck@chromium.org
You probably want to trim the list a bit (most of these people are really not
qualified for reviewing attestation code) and put a comment saying that dkrahn@
should be involved in reviews.

https://chromiumcodereview.appspot.com/12556004/diff/51001/chromeos/attestati...
File chromeos/attestation/OWNERS (right):

https://chromiumcodereview.appspot.com/12556004/diff/51001/chromeos/attestati...
chromeos/attestation/OWNERS:1: mnissler@chromium.org
ditto

[dkrahn, 2013-04-17 23:45:08]

+TBR jochen@ for *.gypi changes

https://chromiumcodereview.appspot.com/12556004/diff/51001/chrome/browser/chr...
File chrome/browser/chromeos/attestation/OWNERS (right):

https://chromiumcodereview.appspot.com/12556004/diff/51001/chrome/browser/chr...
chrome/browser/chromeos/attestation/OWNERS:6: pneubeck@chromium.org
On 2013/04/17 08:51:43, Mattias Nissler wrote:
> You probably want to trim the list a bit (most of these people are really not
> qualified for reviewing attestation code) and put a comment saying that
dkrahn@
> should be involved in reviews.

Done.

https://chromiumcodereview.appspot.com/12556004/diff/51001/chromeos/attestati...
File chromeos/attestation/OWNERS (right):

https://chromiumcodereview.appspot.com/12556004/diff/51001/chromeos/attestati...
chromeos/attestation/OWNERS:1: mnissler@chromium.org
On 2013/04/17 08:51:43, Mattias Nissler wrote:
> ditto

Done.

[satorux1, 2013-04-18 05:43:16]

LGTM with nits

https://codereview.chromium.org/12556004/diff/56001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/attestation/attestation_policy_observer.cc (right):

https://codereview.chromium.org/12556004/diff/56001/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:74:
attestation_flow_(NULL) {
On what thread this project lives?

I expect it to be UI thread. If so, you might want to add the following to every
function.

DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

This is verbose, but helps a lot for preventing this class from being used from
unexpected threads, which tends to cause hard-to-reproduce issues.

https://codereview.chromium.org/12556004/diff/56001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/attestation/attestation_policy_observer.h (right):

https://codereview.chromium.org/12556004/diff/56001/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/attestation_policy_observer.h:73:
base::WeakPtrFactory<AttestationPolicyObserver> weak_factory_;
nit: Please move this to the end. Shouldn't matter in practice, but it's
conceptually a good idea to destroy and invalidate the weak pointers before any
other members.

Here's an example from elsewhere.

  // Note: This should remain the last member so it'll be destroyed and         
 
  // invalidate the weak pointers before any other members are destroyed.       
 
  base::WeakPtrFactory<DriveFileSystem> weak_ptr_factory_;

[jochen (gone - plz use gerrit), 2013-04-18 06:49:16]

*gypi* lgtm

https://codereview.chromium.org/12556004/diff/56001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/attestation/OWNERS (right):

https://codereview.chromium.org/12556004/diff/56001/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/OWNERS:4: # dkrahn@chromium.org should also
be involved in reviews.
instead of this comment, you might want to add a watchlist rule that cc's you
automatically on reviews.

https://codereview.chromium.org/12556004/diff/56001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/attestation/attestation_policy_observer.cc (right):

https://codereview.chromium.org/12556004/diff/56001/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:22: using
std::string;
we commonly don't use using for stl stuff

[satorux1, 2013-04-18 07:04:08]

https://codereview.chromium.org/12556004/diff/56001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/attestation/attestation_policy_observer.cc (right):

https://codereview.chromium.org/12556004/diff/56001/chrome/browser/chromeos/a...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:22: using
std::string;
On 2013/04/18 06:49:16, jochen wrote:
> we commonly don't use using for stl stuff

Right. Please fix!

[dkrahn, 2013-04-18 18:29:41]

https://chromiumcodereview.appspot.com/12556004/diff/56001/chrome/browser/chr...
File chrome/browser/chromeos/attestation/OWNERS (right):

https://chromiumcodereview.appspot.com/12556004/diff/56001/chrome/browser/chr...
chrome/browser/chromeos/attestation/OWNERS:4: # dkrahn@chromium.org should also
be involved in reviews.
On 2013/04/18 06:49:16, jochen wrote:
> instead of this comment, you might want to add a watchlist rule that cc's you
> automatically on reviews.

Done.

https://chromiumcodereview.appspot.com/12556004/diff/56001/chrome/browser/chr...
File chrome/browser/chromeos/attestation/attestation_policy_observer.cc (right):

https://chromiumcodereview.appspot.com/12556004/diff/56001/chrome/browser/chr...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:22: using
std::string;
On 2013/04/18 07:04:09, satorux1 wrote:
> On 2013/04/18 06:49:16, jochen wrote:
> > we commonly don't use using for stl stuff
> 
> Right. Please fix!

Done.

https://chromiumcodereview.appspot.com/12556004/diff/56001/chrome/browser/chr...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:74:
attestation_flow_(NULL) {
On 2013/04/18 05:43:16, satorux1 wrote:
> On what thread this project lives?
> 
> I expect it to be UI thread. If so, you might want to add the following to
every
> function.
> 
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
> 
> This is verbose, but helps a lot for preventing this class from being used
from
> unexpected threads, which tends to cause hard-to-reproduce issues.

Done.

https://chromiumcodereview.appspot.com/12556004/diff/56001/chrome/browser/chr...
File chrome/browser/chromeos/attestation/attestation_policy_observer.h (right):

https://chromiumcodereview.appspot.com/12556004/diff/56001/chrome/browser/chr...
chrome/browser/chromeos/attestation/attestation_policy_observer.h:73:
base::WeakPtrFactory<AttestationPolicyObserver> weak_factory_;
On 2013/04/18 05:43:16, satorux1 wrote:
> nit: Please move this to the end. Shouldn't matter in practice, but it's
> conceptually a good idea to destroy and invalidate the weak pointers before
any
> other members.
> 
> Here's an example from elsewhere.
> 
>   // Note: This should remain the last member so it'll be destroyed and       
 
>  
>   // invalidate the weak pointers before any other members are destroyed.     
 
>  
>   base::WeakPtrFactory<DriveFileSystem> weak_ptr_factory_;

Done.

[commit-bot: I haz the power, 2013-04-18 18:30:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dkrahn@google.com/12556004/64001

[commit-bot: I haz the power, 2013-04-18 18:30:46]

Failed to apply patch for
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/chromeos/policy/proto/chrome_device_policy.proto
  Hunk #1 succeeded at 279 with fuzz 1 (offset 6 lines).
  Hunk #2 FAILED at 309.
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/policy/proto/chrome_device_policy.proto.rej

Patch:       chrome/browser/chromeos/policy/proto/chrome_device_policy.proto
Index: chrome/browser/chromeos/policy/proto/chrome_device_policy.proto
diff --git a/chrome/browser/chromeos/policy/proto/chrome_device_policy.proto
b/chrome/browser/chromeos/policy/proto/chrome_device_policy.proto
index
b29121c67e6d2047801d43ad04b78151f279be77..4cd56b10b0b8d2df22dde041c5a8bf07373a20cf
100644
--- a/chrome/browser/chromeos/policy/proto/chrome_device_policy.proto
+++ b/chrome/browser/chromeos/policy/proto/chrome_device_policy.proto
@@ -273,6 +273,17 @@ message UptimeLimitProto {
   optional int64 uptime_limit = 1;
 }
 
+message AttestationSettingsProto {
+  // Attestation involves proving that a cryptographic key is protected by a
+  // legitimate Chrome OS TPM and reporting the operating mode of the platform.
+  // This setting enables attestation features at a device level.  If this is
+  // enabled a machine key will be generated and certified by the Chrome OS
+  // CA.  If this setting is disabled, the device will not communicate with the
+  // Chrome OS CA under any circumstances.  Even users with attestation
settings
+  // enabled will not be able to use those features on the device.
+  optional bool attestation_enabled = 1;
+}
+
 message ChromeDeviceSettingsProto {
   optional DevicePolicyRefreshRateProto device_policy_refresh_rate = 1;
   optional UserWhitelistProto user_whitelist = 2;
@@ -298,4 +309,5 @@ message ChromeDeviceSettingsProto {
   optional AllowRedeemChromeOsRegistrationOffersProto allow_redeem_offers = 22;
   optional StartUpFlagsProto start_up_flags = 23;
   optional UptimeLimitProto uptime_limit = 24;
+  optional AttestationSettingsProto attestation_settings = 25;
 }

[commit-bot: I haz the power, 2013-04-18 19:39:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dkrahn@google.com/12556004/69001

[commit-bot: I haz the power, 2013-04-18 23:25:05]

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

[commit-bot: I haz the power, 2013-04-19 00:45:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dkrahn@google.com/12556004/69001

[commit-bot: I haz the power, 2013-04-19 03:06:46]

Failed to apply patch for
chrome/browser/chromeos/settings/cros_settings_names.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/chromeos/settings/cros_settings_names.cc
  Hunk #1 FAILED at 120.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/chromeos/settings/cros_settings_names.cc.rej

Patch:       chrome/browser/chromeos/settings/cros_settings_names.cc
Index: chrome/browser/chromeos/settings/cros_settings_names.cc
diff --git a/chrome/browser/chromeos/settings/cros_settings_names.cc
b/chrome/browser/chromeos/settings/cros_settings_names.cc
index
6d9989c089a6c56e7886289257fedd29af5342db..66d3e6dd0909f673095523238a66cee4221669cf
100644
--- a/chrome/browser/chromeos/settings/cros_settings_names.cc
+++ b/chrome/browser/chromeos/settings/cros_settings_names.cc
@@ -120,4 +120,8 @@ const char kKioskDisableBailoutShortcut[] =
 // when pinging the Variations server.
 const char kVariationsRestrictParameter[] =
     "cros.variations_restrict_parameter";
+
+// A boolean pref that indicates whether attestation is enabled for the device.
+const char kDeviceAttestationEnabled[] = "cros.device.attestation_enabled";
+
 }  // namespace chromeos

[commit-bot: I haz the power, 2013-04-19 17:42:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dkrahn@google.com/12556004/89002

[commit-bot: I haz the power, 2013-04-19 20:34:37]

Change committed as 195266