Enhanced and refactored the AttestationFlow interface.

Enhanced and refactored the AttestationFlow interface.

This change makes the AttestationFlow interface easier to work with.
Supported certificate profiles are explicitly defined and the clobbering
of existing keys is now optional.

All attestation constants have been consolidated in
attestation_constants.h.

BUG=chromium:219959
TEST=unit

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

[dkrahn, 2013-04-23 23:50:18]

+mnissler - primary reviewer
+derat - OWNER of chromeos/dbus, chromeos/cryptohome, chromeos/chromeos.gyp
+finnur - OWNER of chrome/browser/extensions

[Daniel Erat, 2013-04-23 23:59:23]

rubber-stamp OWNERS LGTM for chromeos/dbus, chromeos/cryptohome,
chromeos/chromeos.gyp

[Finnur, 2013-04-24 10:36:18]

extensions LGTM

[Mattias Nissler (ping if slow), 2013-04-24 12:57:53]

Please do read the existing discussions around gmock usage - proliferation of
complex gmock code is moving us closer to get gmock banned outright...

https://codereview.chromium.org/14305009/diff/1/chromeos/attestation/attestat...
File chromeos/attestation/attestation_constants.h (right):

https://codereview.chromium.org/14305009/diff/1/chromeos/attestation/attestat...
chromeos/attestation/attestation_constants.h:12: enum
AttestationCertificateOptions {
nit: We usually follow good practice for enums to share a common prefix, maybe
we should do that here as well?

https://codereview.chromium.org/14305009/diff/1/chromeos/attestation/attestat...
chromeos/attestation/attestation_constants.h:19: INCLUDE_DEVICE_STATE = 1 << 1
nit: putting a trailing colon here makes it easier to resolve potential merge
conflicts in the future.

https://codereview.chromium.org/14305009/diff/1/chromeos/attestation/attestat...
chromeos/attestation/attestation_constants.h:29: USER_KEY
ditto

https://codereview.chromium.org/14305009/diff/1/chromeos/attestation/attestat...
chromeos/attestation/attestation_constants.h:37: INCLUDE_SIGNED_PUBLIC_KEY = 1
ditto.

https://codereview.chromium.org/14305009/diff/1/chromeos/attestation/attestat...
chromeos/attestation/attestation_constants.h:45: ENTERPRISE_USER_CERTIFICATE,
ah, and here you have it :)

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

https://codereview.chromium.org/14305009/diff/1/chromeos/attestation/attestat...
chromeos/attestation/attestation_flow.cc:255: AttestationKeyType
AttestationFlow::GetKeyTypeForProfile(
nit (here and below): These functions could be moved to an anonymous namespace
at the top of the file.

https://codereview.chromium.org/14305009/diff/1/chromeos/attestation/attestat...
chromeos/attestation/attestation_flow.cc:263: NOTREACHED();
Here and below: It'd be better to not have a default branch so the compiler
flags this switch statement when somebody adds a new constant. You can just move
the NOTREACHED after the switch block.

https://codereview.chromium.org/14305009/diff/1/chromeos/attestation/attestat...
File chromeos/attestation/attestation_flow.h (right):

https://codereview.chromium.org/14305009/diff/1/chromeos/attestation/attestat...
chromeos/attestation/attestation_flow.h:195: 
remove extra blank line

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

https://codereview.chromium.org/14305009/diff/1/chromeos/attestation/attestat...
chromeos/attestation/attestation_flow_unittest.cc:16: using testing::NiceMock;
unused?

https://codereview.chromium.org/14305009/diff/1/chromeos/attestation/attestat...
chromeos/attestation/attestation_flow_unittest.cc:416: Run();
Ugh, even more hard-to-read gmock setup. To repeat: I'm with the powers that be
in considering this gmock abuse (see recent thread on blink-dev:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/q9cq3KHZesc/discus...),
and my advice would be to use fakes.

[dkrahn, 2013-04-25 01:06:52]

https://chromiumcodereview.appspot.com/14305009/diff/1/chromeos/attestation/a...
File chromeos/attestation/attestation_constants.h (right):

https://chromiumcodereview.appspot.com/14305009/diff/1/chromeos/attestation/a...
chromeos/attestation/attestation_constants.h:12: enum
AttestationCertificateOptions {
On 2013/04/24 12:57:53, Mattias Nissler wrote:
> nit: We usually follow good practice for enums to share a common prefix, maybe
> we should do that here as well?

The style guide gives this as an example of a good enum:
enum AlternateUrlTableErrors {
  OK = 0,
  OUT_OF_MEMORY = 1,
  MALFORMED_INPUT = 2,
};

I think a prefix on these would make them too long and verbose.  Especially the
options enums which are meant to be combined.  The purpose of these enums is to
make calling code more readable (e.g. func(OPTION1|OPTION2) instead of
func(true, true)) -- I don't want to defeat that so I prefer succinct names.  

Let me know if you feel strongly...

https://chromiumcodereview.appspot.com/14305009/diff/1/chromeos/attestation/a...
chromeos/attestation/attestation_constants.h:19: INCLUDE_DEVICE_STATE = 1 << 1
On 2013/04/24 12:57:53, Mattias Nissler wrote:
> nit: putting a trailing colon here makes it easier to resolve potential merge
> conflicts in the future.

Done.  Good idea.  I'm not in the habit...

https://chromiumcodereview.appspot.com/14305009/diff/1/chromeos/attestation/a...
chromeos/attestation/attestation_constants.h:29: USER_KEY
On 2013/04/24 12:57:53, Mattias Nissler wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/14305009/diff/1/chromeos/attestation/a...
chromeos/attestation/attestation_constants.h:37: INCLUDE_SIGNED_PUBLIC_KEY = 1
On 2013/04/24 12:57:53, Mattias Nissler wrote:
> ditto.

Done.

https://chromiumcodereview.appspot.com/14305009/diff/1/chromeos/attestation/a...
chromeos/attestation/attestation_constants.h:45: ENTERPRISE_USER_CERTIFICATE,
On 2013/04/24 12:57:53, Mattias Nissler wrote:
> ah, and here you have it :)

there was a third value at one time :)

https://chromiumcodereview.appspot.com/14305009/diff/1/chromeos/attestation/a...
File chromeos/attestation/attestation_flow.cc (right):

https://chromiumcodereview.appspot.com/14305009/diff/1/chromeos/attestation/a...
chromeos/attestation/attestation_flow.cc:255: AttestationKeyType
AttestationFlow::GetKeyTypeForProfile(
On 2013/04/24 12:57:53, Mattias Nissler wrote:
> nit (here and below): These functions could be moved to an anonymous namespace
> at the top of the file.

Done.

https://chromiumcodereview.appspot.com/14305009/diff/1/chromeos/attestation/a...
chromeos/attestation/attestation_flow.cc:263: NOTREACHED();
On 2013/04/24 12:57:53, Mattias Nissler wrote:
> Here and below: It'd be better to not have a default branch so the compiler
> flags this switch statement when somebody adds a new constant. You can just
move
> the NOTREACHED after the switch block.

Done.

https://chromiumcodereview.appspot.com/14305009/diff/1/chromeos/attestation/a...
File chromeos/attestation/attestation_flow.h (right):

https://chromiumcodereview.appspot.com/14305009/diff/1/chromeos/attestation/a...
chromeos/attestation/attestation_flow.h:195: 
On 2013/04/24 12:57:53, Mattias Nissler wrote:
> remove extra blank line

Done.

https://chromiumcodereview.appspot.com/14305009/diff/1/chromeos/attestation/a...
File chromeos/attestation/attestation_flow_unittest.cc (right):

https://chromiumcodereview.appspot.com/14305009/diff/1/chromeos/attestation/a...
chromeos/attestation/attestation_flow_unittest.cc:16: using testing::NiceMock;
On 2013/04/24 12:57:53, Mattias Nissler wrote:
> unused?

Done.

https://chromiumcodereview.appspot.com/14305009/diff/1/chromeos/attestation/a...
chromeos/attestation/attestation_flow_unittest.cc:416: Run();
On 2013/04/24 12:57:53, Mattias Nissler wrote:
> Ugh, even more hard-to-read gmock setup. To repeat: I'm with the powers that
be
> in considering this gmock abuse (see recent thread on blink-dev:
>
https://groups.google.com/a/chromium.org/d/topic/blink-dev/q9cq3KHZesc/discus...),
> and my advice would be to use fakes.

For testing this flow I believe I have a legitimate case for using mocks.  For
some classes I want a combination of mock / fake.  Let me break it apart:

AsyncMethodCaller - I'm reusing an existing fake (note SetUp() method) which
uses gmock.  If I used a pure fake here I would lose test coverage because I set
a few expectations on the methods (while at the same time deferring to the fake
to implement).

CryptohomeClient - Here I'm abusing gmock to create a simple fake -- until
FakeCryptohomeClient lands this is the simplest solution IMO.

ServerProxy - Again I need a mock / fake combination and am using gmock to defer
to a fake (DeferToFake()) and set expectations at the same time.

MockObserver - This is strictly a mock.

Apart from the CryptohomeClient discussion I think the approach here is
reasonable given what I'm trying to test and that this is a pure unit test.  I
am loath to give up the coverage that mocks currently give me.

[Daniel Erat, 2013-04-25 01:15:30]

https://chromiumcodereview.appspot.com/14305009/diff/1/chromeos/attestation/a...
File chromeos/attestation/attestation_constants.h (right):

https://chromiumcodereview.appspot.com/14305009/diff/1/chromeos/attestation/a...
chromeos/attestation/attestation_constants.h:12: enum
AttestationCertificateOptions {
On 2013/04/25 01:06:52, Darren Krahn wrote:
> On 2013/04/24 12:57:53, Mattias Nissler wrote:
> > nit: We usually follow good practice for enums to share a common prefix,
maybe
> > we should do that here as well?
> 
> The style guide gives this as an example of a good enum:
> enum AlternateUrlTableErrors {
>   OK = 0,
>   OUT_OF_MEMORY = 1,
>   MALFORMED_INPUT = 2,
> };
> 
> I think a prefix on these would make them too long and verbose.  Especially
the
> options enums which are meant to be combined.  The purpose of these enums is
to
> make calling code more readable (e.g. func(OPTION1|OPTION2) instead of
> func(true, true)) -- I don't want to defeat that so I prefer succinct names.  
> 
> Let me know if you feel strongly... 

I think I agree with Mattias here.  Omitting prefixes seems okay when there's
e.g. just a single enum scoped within a struct, but I find this case a bit more
confusing; there are e.g. two sets of options within the same namespace.  What
about names like the following?

CERTIFICATE_INCLUDE_STABLE_ID
CERTIFICATE_INCLUDE_DEVICE_STATE

KEY_DEVICE
KEY_USER

CHALLENGE_INCLUDE_SIGNED_PUBLIC_KEY

PROFILE_ENTERPRISE_MACHINE_CERTIFICATE
PROFILE_ENTERPRISE_USER_CERTIFICATE

[dkrahn, 2013-04-25 02:45:35]

https://chromiumcodereview.appspot.com/14305009/diff/1/chromeos/attestation/a...
File chromeos/attestation/attestation_constants.h (right):

https://chromiumcodereview.appspot.com/14305009/diff/1/chromeos/attestation/a...
chromeos/attestation/attestation_constants.h:12: enum
AttestationCertificateOptions {
On 2013/04/25 01:15:30, Daniel Erat wrote:
> On 2013/04/25 01:06:52, Darren Krahn wrote:
> > On 2013/04/24 12:57:53, Mattias Nissler wrote:
> > > nit: We usually follow good practice for enums to share a common prefix,
> maybe
> > > we should do that here as well?
> > 
> > The style guide gives this as an example of a good enum:
> > enum AlternateUrlTableErrors {
> >   OK = 0,
> >   OUT_OF_MEMORY = 1,
> >   MALFORMED_INPUT = 2,
> > };
> > 
> > I think a prefix on these would make them too long and verbose.  Especially
> the
> > options enums which are meant to be combined.  The purpose of these enums is
> to
> > make calling code more readable (e.g. func(OPTION1|OPTION2) instead of
> > func(true, true)) -- I don't want to defeat that so I prefer succinct names.
 
> > 
> > Let me know if you feel strongly... 
> 
> I think I agree with Mattias here.  Omitting prefixes seems okay when there's
> e.g. just a single enum scoped within a struct, but I find this case a bit
more
> confusing; there are e.g. two sets of options within the same namespace.  What
> about names like the following?
> 
> CERTIFICATE_INCLUDE_STABLE_ID
> CERTIFICATE_INCLUDE_DEVICE_STATE
> 
> KEY_DEVICE
> KEY_USER
> 
> CHALLENGE_INCLUDE_SIGNED_PUBLIC_KEY
> 
> PROFILE_ENTERPRISE_MACHINE_CERTIFICATE
> PROFILE_ENTERPRISE_USER_CERTIFICATE

That makes sense, and this CL does pull these out of CryptohomeClient and into
the attestation namespace.

Done!

[Mattias Nissler (ping if slow), 2013-04-25 12:05:26]

LGTM. I have further comments on the gmock discussion, but I don't want to hold
the CL over this, in particular given that there are no hard rules on gmock
usage in effect.

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

https://codereview.chromium.org/14305009/diff/1/chromeos/attestation/attestat...
chromeos/attestation/attestation_flow_unittest.cc:416: Run();
On 2013/04/25 01:06:52, Darren Krahn wrote:
> On 2013/04/24 12:57:53, Mattias Nissler wrote:
> > Ugh, even more hard-to-read gmock setup. To repeat: I'm with the powers that
> be
> > in considering this gmock abuse (see recent thread on blink-dev:
> >
>
https://groups.google.com/a/chromium.org/d/topic/blink-dev/q9cq3KHZesc/discus...),
> > and my advice would be to use fakes.
> 
> For testing this flow I believe I have a legitimate case for using mocks.  For
> some classes I want a combination of mock / fake.  Let me break it apart:
> 
> AsyncMethodCaller - I'm reusing an existing fake (note SetUp() method) which
> uses gmock.  If I used a pure fake here I would lose test coverage because I
set
> a few expectations on the methods (while at the same time deferring to the
fake
> to implement).

Yes, these seem reasonable.

> 
> CryptohomeClient - Here I'm abusing gmock to create a simple fake -- until
> FakeCryptohomeClient lands this is the simplest solution IMO.

Fair.

> 
> ServerProxy - Again I need a mock / fake combination and am using gmock to
defer
> to a fake (DeferToFake()) and set expectations at the same time.

Why is the mocking part needed here? A pure fake implementation gives you the
same kind of guarantees if you put code verifying the state of the fake after
the test. The Times(1) specification is more difficult to verify obviously, if
you're concerned about that you could just keep a counter and verify its value.

> 
> MockObserver - This is strictly a mock.

A function that just records the callback arguments and proper EXPECT_EQ() after
the test would be easier to read IMHO.
 
> Apart from the CryptohomeClient discussion I think the approach here is
> reasonable given what I'm trying to test and that this is a pure unit test.  I
> am loath to give up the coverage that mocks currently give me.

It's not about giving up coverage. It's about people reading your code being
able to find out what you're verifying in a reasonable amount of time. The
pattern complicated-setup -> run-code-under-test -> automatic-verification is
inherently harder to follow than setup-dependency-state -> run-code-under-test
-> verify-state IMO.

The other thing you seem to be worried about in terms of coverage are your
narrow expectations for parameters passed to the mocks in calls and the sequence
of the calls. If it is important to validate code behavior w.r.t. this, then
you're essentially saying you want to verify that some data flows in the right
ways through the system. In this case, you create an enroll request, send it to
the server, receive a response etc. and data gets passed on from one step to
another. With fakes that behave similar to the real thing, you could as well
just run AttestationFlow without any narrow expectations. Once the flow
finishes, just look at the state of the fakes. If the result is as expected, the
data flow must have been correct and the code must have called the fakes in the
right order (otherwise the request payload wouldn't match the request the server
fake saw etc.).

[dkrahn, 2013-04-25 22:51:48]

On 2013/04/25 12:05:26, Mattias Nissler wrote:
> LGTM. I have further comments on the gmock discussion, but I don't want to
hold
> the CL over this, in particular given that there are no hard rules on gmock
> usage in effect.
> 
>
https://codereview.chromium.org/14305009/diff/1/chromeos/attestation/attestat...
> File chromeos/attestation/attestation_flow_unittest.cc (right):
> 
>
https://codereview.chromium.org/14305009/diff/1/chromeos/attestation/attestat...
> chromeos/attestation/attestation_flow_unittest.cc:416: Run();
> On 2013/04/25 01:06:52, Darren Krahn wrote:
> > On 2013/04/24 12:57:53, Mattias Nissler wrote:
> > > Ugh, even more hard-to-read gmock setup. To repeat: I'm with the powers
that
> > be
> > > in considering this gmock abuse (see recent thread on blink-dev:
> > >
> >
>
https://groups.google.com/a/chromium.org/d/topic/blink-dev/q9cq3KHZesc/discus...),
> > > and my advice would be to use fakes.
> > 
> > For testing this flow I believe I have a legitimate case for using mocks. 
For
> > some classes I want a combination of mock / fake.  Let me break it apart:
> > 
> > AsyncMethodCaller - I'm reusing an existing fake (note SetUp() method) which
> > uses gmock.  If I used a pure fake here I would lose test coverage because I
> set
> > a few expectations on the methods (while at the same time deferring to the
> fake
> > to implement).
> 
> Yes, these seem reasonable.
> 
> > 
> > CryptohomeClient - Here I'm abusing gmock to create a simple fake -- until
> > FakeCryptohomeClient lands this is the simplest solution IMO.
> 
> Fair.
> 
> > 
> > ServerProxy - Again I need a mock / fake combination and am using gmock to
> defer
> > to a fake (DeferToFake()) and set expectations at the same time.
> 
> Why is the mocking part needed here? A pure fake implementation gives you the
> same kind of guarantees if you put code verifying the state of the fake after
> the test. The Times(1) specification is more difficult to verify obviously, if
> you're concerned about that you could just keep a counter and verify its
value.
> 
> > 
> > MockObserver - This is strictly a mock.
> 
> A function that just records the callback arguments and proper EXPECT_EQ()
after
> the test would be easier to read IMHO.
>  
> > Apart from the CryptohomeClient discussion I think the approach here is
> > reasonable given what I'm trying to test and that this is a pure unit test. 
I
> > am loath to give up the coverage that mocks currently give me.
> 
> It's not about giving up coverage. It's about people reading your code being
> able to find out what you're verifying in a reasonable amount of time. The
> pattern complicated-setup -> run-code-under-test -> automatic-verification is
> inherently harder to follow than setup-dependency-state -> run-code-under-test
> -> verify-state IMO.
> 
> The other thing you seem to be worried about in terms of coverage are your
> narrow expectations for parameters passed to the mocks in calls and the
sequence
> of the calls. If it is important to validate code behavior w.r.t. this, then
> you're essentially saying you want to verify that some data flows in the right
> ways through the system. In this case, you create an enroll request, send it
to
> the server, receive a response etc. and data gets passed on from one step to
> another. With fakes that behave similar to the real thing, you could as well
> just run AttestationFlow without any narrow expectations. Once the flow
> finishes, just look at the state of the fakes. If the result is as expected,
the
> data flow must have been correct and the code must have called the fakes in
the
> right order (otherwise the request payload wouldn't match the request the
server
> fake saw etc.).

Understood -- and good ideas.  Moving forward with this CL and filed
https://code.google.com/p/chromium/issues/detail?id=235637 for working more on
these unit tests.

[commit-bot: I haz the power, 2013-04-25 22:52:09]

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

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

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-26 00:59:26]

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

[commit-bot: I haz the power, 2013-04-26 01:09:47]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

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

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

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

Failed to apply patch for chromeos/attestation/attestation_flow.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chromeos/attestation/attestation_flow.cc
  Hunk #1 FAILED at 38.
  1 out of 4 hunks FAILED -- saving rejects to file
chromeos/attestation/attestation_flow.cc.rej

Patch:       chromeos/attestation/attestation_flow.cc
Index: chromeos/attestation/attestation_flow.cc
diff --git a/chromeos/attestation/attestation_flow.cc
b/chromeos/attestation/attestation_flow.cc
index
f36403a89b99bcec86958597ebb35eff11859c89..11fe0ea6840edc4947a0d55dbc71b0d4d16e7584
100644
--- a/chromeos/attestation/attestation_flow.cc
+++ b/chromeos/attestation/attestation_flow.cc
@@ -38,30 +38,82 @@ void DBusBoolRedirectCallback(const base::Closure& on_true,
     task.Run();
 }
 
-}  // namespace
+void DBusDataMethodCallback(
+    const AttestationFlow::CertificateCallback& callback,
+    DBusMethodCallStatus status,
+    bool result,
+    const std::string& data) {
+  if (status != DBUS_METHOD_CALL_SUCCESS) {
+    LOG(ERROR) << "Attestation: DBus data operation failed.";
+    if (!callback.is_null())
+      callback.Run(false, "");
+    return;
+  }
+  if (!callback.is_null())
+    callback.Run(result, data);
+}
+
+AttestationKeyType GetKeyTypeForProfile(
+    AttestationCertificateProfile profile) {
+  switch (profile) {
+    case PROFILE_ENTERPRISE_MACHINE_CERTIFICATE:
+      return KEY_DEVICE;
+    case PROFILE_ENTERPRISE_USER_CERTIFICATE:
+      return KEY_USER;
+  }
+  NOTREACHED();
+  return KEY_USER;
+}
 
-const char AttestationFlow::kEnterpriseMachineKey[] = "attest-ent-machine";
+std::string GetKeyNameForProfile(
+    AttestationCertificateProfile profile) {
+  switch (profile) {
+    case PROFILE_ENTERPRISE_MACHINE_CERTIFICATE:
+      return kEnterpriseMachineKey;
+    case PROFILE_ENTERPRISE_USER_CERTIFICATE:
+      return kEnterpriseUserKey;
+  }
+  NOTREACHED();
+  return "";
+}
+
+int GetCertificateOptionsForProfile(
+    AttestationCertificateProfile profile) {
+  switch (profile) {
+    case PROFILE_ENTERPRISE_MACHINE_CERTIFICATE:
+      return CERTIFICATE_INCLUDE_STABLE_ID | CERTIFICATE_INCLUDE_DEVICE_STATE;
+    case PROFILE_ENTERPRISE_USER_CERTIFICATE:
+      return CERTIFICATE_INCLUDE_DEVICE_STATE;
+  }
+  NOTREACHED();
+  return CERTIFICATE_OPTION_NONE;
+}
+
+}  // namespace
 
 AttestationFlow::AttestationFlow(cryptohome::AsyncMethodCaller* async_caller,
                                  CryptohomeClient* cryptohome_client,
                                  scoped_ptr<ServerProxy> server_proxy)
-    : ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)),
-      async_caller_(async_caller),
+    : async_caller_(async_caller),
       cryptohome_client_(cryptohome_client),
-      server_proxy_(server_proxy.Pass()) {
+      server_proxy_(server_proxy.Pass()),
+      ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {
 }
 
 AttestationFlow::~AttestationFlow() {
 }
 
-void AttestationFlow::GetCertificate(const std::string& name,
-                                     const CertificateCallback& callback) {
+void AttestationFlow::GetCertificate(
+    AttestationCertificateProfile certificate_profile,
+    bool force_new_key,
+    const CertificateCallback& callback) {
   // If this device has not enrolled with the Privacy CA, we need to do that
   // first.  Once enrolled we can proceed with the certificate request.
   base::Closure do_cert_request = base::Bind(
       &AttestationFlow::StartCertificateRequest,
       weak_factory_.GetWeakPtr(),
-      name,
+      certificate_profile,
+      force_new_key,
       callback);
   base::Closure on_enroll_failure = base::Bind(callback, false, "");
   base::Closure do_enroll = base::Bind(&AttestationFlow::StartEnroll,
@@ -143,22 +195,49 @@ void AttestationFlow::OnEnrollComplete(const
base::Closure& on_failure,
 }
 
 void AttestationFlow::StartCertificateRequest(
-    const std::string& name,
+    AttestationCertificateProfile certificate_profile,
+    bool generate_new_key,
     const CertificateCallback& callback) {
-  // Get the attestation service to create a Privacy CA certificate request.
-  int options = CryptohomeClient::INCLUDE_DEVICE_STATE;
-  if (name == kEnterpriseMachineKey)
-    options |= CryptohomeClient::INCLUDE_STABLE_ID;
-  async_caller_->AsyncTpmAttestationCreateCertRequest(
-      options,
-      base::Bind(&AttestationFlow::SendCertificateRequestToPCA,
-                 weak_factory_.GetWeakPtr(),
-                 name,
-                 callback));
+  AttestationKeyType key_type = GetKeyTypeForProfile(certificate_profile);
+  std::string key_name = GetKeyNameForProfile(certificate_profile);
+  if (generate_new_key) {
+    // Get the attestation service to create a Privacy CA certificate request.
+    async_caller_->AsyncTpmAttestationCreateCertRequest(
+        GetCertificateOptionsForProfile(certificate_profile),
+        base::Bind(&AttestationFlow::SendCertificateRequestToPCA,
+                   weak_factory_.GetWeakPtr(),
+                   key_type,
+                   key_name,
+                   callback));
+  } else {
+    // If the key already exists, query the existing certificate.
+    base::Closure on_key_exists = base::Bind(
+        &AttestationFlow::GetExistingCertificate,
+        weak_factory_.GetWeakPtr(),
+        key_type,
+        key_name,
+        callback);
+    // If the key does not exist, call this method back with |generate_new_key|
+    // set to true.
+    base::Closure on_key_not_exists = base::Bind(
+        &AttestationFlow::StartCertificateRequest,
+        weak_factory_.GetWeakPtr(),
+        certificate_profile,
+        true,
+        callback);
+    cryptohome_client_->TpmAttestationDoesKeyExist(
+        key_type,
+        key_name,
+        base::Bind(&DBusBoolRedirectCallback,
+            on_key_exists,
+            on_key_not_exists,
+            base::Bind(callback, false, "")));
+  }
 }
 
 void AttestationFlow::SendCertificateRequestToPCA(
-    const std::string& name,
+    AttestationKeyType key_type,
+    const std::string& key_name,
     const CertificateCallback& callback,
     bool success,
     const std::string& data) {
@@ -174,12 +253,14 @@ void AttestationFlow::SendCertificateRequestToPCA(
       data,
       base::Bind(&AttestationFlow::SendCertificateResponseToDaemon,
                  weak_factory_.GetWeakPtr(),
-                 name,
+                 key_type,
+                 key_name,
                  callback));
 }
 
 void AttestationFlow::SendCertificateResponseToDaemon(
-    const std::string& name,
+    AttestationKeyType key_type,
+    const std::string& key_name,
     const CertificateCallback& callback,
     bool success,
     const std::string& data) {
@@ -191,14 +272,21 @@ void AttestationFlow::SendCertificateResponseToDaemon(
   }
 
   // Forward the response to the attestation service to complete the operation.
-  CryptohomeClient::AttestationKeyType key_type = CryptohomeClient::USER_KEY;
-  if (name == kEnterpriseMachineKey)
-    key_type = CryptohomeClient::DEVICE_KEY;
   async_caller_->AsyncTpmAttestationFinishCertRequest(data,
                                                       key_type,
-                                                      name,
+                                                      key_name,
                                                       base::Bind(callback));
 }
 
+void AttestationFlow::GetExistingCertificate(
+    AttestationKeyType key_type,
+    const std::string& key_name,
+    const CertificateCallback& callback) {
+  cryptohome_client_->TpmAttestationGetCertificate(
+      key_type,
+      key_name,
+      base::Bind(&DBusDataMethodCallback, callback));
+}
+
 }  // namespace attestation
 }  // namespace chromeos

[commit-bot: I haz the power, 2013-04-26 22:12:53]

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

[commit-bot: I haz the power, 2013-04-26 22:25:58]

Retried try job too often on linux_chromeos for step(s) ash_unittests,
aura_unittests, base_unittests, browser_tests, cacheinvalidation_unittests,
check_deps, chromeos_unittests, components_unittests, content_browsertests,
content_unittests, crypto_unittests, dbus_unittests, device_unittests,
gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, net_unittests, ppapi_unittests, printing_unittests,
sandbox_linux_unittests, sql_unittests, sync_unit_tests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-04-26 23:16:00]

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

[commit-bot: I haz the power, 2013-04-27 02:32:52]

Change committed as 196926