Add Platform Verification Mojo Services.

Add Platform Verification Mojo Services.

Render frames can use this service to verify platform on ChromeOS.

BUG=479836
Committed: https://crrev.com/9ab9648385b5455904aa91f62ce3e77df366f292
Cr-Commit-Position: refs/heads/master@{#329247}

[xhwang, 2015-04-30 23:35:55]

Patchset #2 (id:20001) has been deleted

[xhwang, 2015-05-05 21:46:48]

xhwang@chromium.org changed reviewers:
+ ddorwin@chromium.org, dkrahn@chromium.org

[xhwang, 2015-05-05 21:46:49]

A small step forward to not relying on pepper.

PTAL

[xhwang, 2015-05-05 21:53:02]

xhwang@chromium.org changed reviewers:
+ dalecurtis@chromium.org
- ddorwin@chromium.org

[xhwang, 2015-05-05 21:53:03]

Add dalecurtis (for his advice on mojo/gyp/gn) and CC ddorwin.

PTAL

https://codereview.chromium.org/1053773007/diff/60001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1053773007/diff/60001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:2345: render_frame_host));
Currently nobody is using these services, so I can remove these changes before
commit.

[DaleCurtis, 2015-05-06 00:28:28]

lgtm % adding more details to the mojom.

https://codereview.chromium.org/1053773007/diff/60001/media/mojo/interfaces/p...
File media/mojo/interfaces/platform_verification.mojom (right):

https://codereview.chromium.org/1053773007/diff/60001/media/mojo/interfaces/p...
media/mojo/interfaces/platform_verification.mojom:9: ChallengePlatform(string
service_id, string challenge) =>
Bit light on the interface details? :)

[DaleCurtis, 2015-05-06 00:28:49]

Would be nice if there was a small app test for this too.

[Darren Krahn, 2015-05-06 19:02:41]

The approach looks good. Added a few comments mostly on documentation /
readability.

https://codereview.chromium.org/1053773007/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/attestation/platform_verification_impl.cc (right):

https://codereview.chromium.org/1053773007/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/attestation/platform_verification_impl.cc:43:
content::WebContents::FromRenderFrameHost(render_frame_host_), service_id,
Side note: PlatformVerification takes a WebContents because that seemed the
right thing to do at the time. If we need to send it more environment context
(or more abstract context) I would be ok with that.

https://codereview.chromium.org/1053773007/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/attestation/platform_verification_impl.cc:58: LOG(ERROR)
<< "Platform verification failed.";
We don't need to bubble the failure status up to the caller?

https://codereview.chromium.org/1053773007/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/attestation/platform_verification_impl.h (right):

https://codereview.chromium.org/1053773007/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/attestation/platform_verification_impl.h:21: class
PlatformVerificationImpl
Class documentation please. Please mention threading properties (e.g. that this
class is entirely owned by the UI thread -- assuming that's true). Also, the
class name is not very descriptive. PlatformVerificationMojoDelegate?
PlatformVerificationMediaDelegate?

https://codereview.chromium.org/1053773007/diff/60001/media/mojo/interfaces/p...
File media/mojo/interfaces/platform_verification.mojom (right):

https://codereview.chromium.org/1053773007/diff/60001/media/mojo/interfaces/p...
media/mojo/interfaces/platform_verification.mojom:9: ChallengePlatform(string
service_id, string challenge) =>
On 2015/05/06 00:28:28, DaleCurtis wrote:
> Bit light on the interface details? :)

+1 -- I'm not sure if there are style guidelines for mojo interfaces but
something like C++ class method documentation would be good.

https://codereview.chromium.org/1053773007/diff/60001/media/mojo/interfaces/p...
media/mojo/interfaces/platform_verification.mojom:12: string?
platform_key_certificate);
What about operation status in the case of a failure? There are legitimate
failure cases like the user refusing Verified Access.

[xhwang, 2015-05-07 00:26:09]

comments addressed

[xhwang, 2015-05-07 00:27:13]

On 2015/05/06 00:28:49, DaleCurtis wrote:
> Would be nice if there was a small app test for this too.

I tried to add a unit test. The test itself is trivial. But the default
PlatformVerificationFlow needs four fake classes to work in a test environment.
Having to pull those four classes over and plumb a way to set those seems
overkill for such a simple change :(

[xhwang, 2015-05-07 00:27:37]

Comments addressed. PTAL again.

https://codereview.chromium.org/1053773007/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/attestation/platform_verification_impl.cc (right):

https://codereview.chromium.org/1053773007/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/attestation/platform_verification_impl.cc:43:
content::WebContents::FromRenderFrameHost(render_frame_host_), service_id,
On 2015/05/06 19:02:40, Darren Krahn wrote:
> Side note: PlatformVerification takes a WebContents because that seemed the
> right thing to do at the time. If we need to send it more environment context
> (or more abstract context) I would be ok with that.

This is fine for now. Agreed we can probably simplify this later.

https://codereview.chromium.org/1053773007/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/attestation/platform_verification_impl.cc:58: LOG(ERROR)
<< "Platform verification failed.";
On 2015/05/06 19:02:40, Darren Krahn wrote:
> We don't need to bubble the failure status up to the caller?

Done.

https://codereview.chromium.org/1053773007/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/attestation/platform_verification_impl.h (right):

https://codereview.chromium.org/1053773007/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/attestation/platform_verification_impl.h:21: class
PlatformVerificationImpl
On 2015/05/06 19:02:40, Darren Krahn wrote:
> Class documentation please. 

Done.

> Please mention threading properties (e.g. that this
> class is entirely owned by the UI thread -- assuming that's true).

Done.

> Also, the
> class name is not very descriptive. PlatformVerificationMojoDelegate?
> PlatformVerificationMediaDelegate?

"Impl" is actually recommended by the mojo team. I agree it's not perfect in
this context but still feels it's concise and accurate.

https://codereview.chromium.org/1053773007/diff/60001/media/mojo/interfaces/p...
File media/mojo/interfaces/platform_verification.mojom (right):

https://codereview.chromium.org/1053773007/diff/60001/media/mojo/interfaces/p...
media/mojo/interfaces/platform_verification.mojom:9: ChallengePlatform(string
service_id, string challenge) =>
On 2015/05/06 19:02:41, Darren Krahn wrote:
> On 2015/05/06 00:28:28, DaleCurtis wrote:
> > Bit light on the interface details? :)
> 
> +1 -- I'm not sure if there are style guidelines for mojo interfaces but
> something like C++ class method documentation would be good.

Done.

https://codereview.chromium.org/1053773007/diff/60001/media/mojo/interfaces/p...
media/mojo/interfaces/platform_verification.mojom:12: string?
platform_key_certificate);
On 2015/05/06 19:02:41, Darren Krahn wrote:
> What about operation status in the case of a failure? There are legitimate
> failure cases like the user refusing Verified Access.

Done.

[DaleCurtis, 2015-05-07 00:49:48]

lgtm

https://codereview.chromium.org/1053773007/diff/80001/media/mojo/interfaces/p...
File media/mojo/interfaces/platform_verification.mojom (right):

https://codereview.chromium.org/1053773007/diff/80001/media/mojo/interfaces/p...
media/mojo/interfaces/platform_verification.mojom:17: // - |success|: Whether
the platform is successfully verified. If true/false the
Consistently use upper case or lower case after the : in these messages.

[Darren Krahn, 2015-05-07 16:33:47]

On 2015/05/07 00:27:13, xhwang wrote:
> On 2015/05/06 00:28:49, DaleCurtis wrote:
> > Would be nice if there was a small app test for this too.
> 
> I tried to add a unit test. The test itself is trivial. But the default
> PlatformVerificationFlow needs four fake classes to work in a test
environment.
> Having to pull those four classes over and plumb a way to set those seems
> overkill for such a simple change :(

Agreed -- and pulling in those fakes would be poor test practice b/c you're not
testing PlatformVerificationFlow. The issue is that you have an explicit
dependency on PlatformVerificationFlow (and by extension that
PlatformVerificationFlow is not pure abstract with an Impl class -- my fault).
I've filed a bug to make this test-able. If you want to make the dependency
injected now that would make the test easier later.

lgtm

[xhwang, 2015-05-07 18:48:17]

comments addressed

[xhwang, 2015-05-07 18:49:46]

https://codereview.chromium.org/1053773007/diff/80001/media/mojo/interfaces/p...
File media/mojo/interfaces/platform_verification.mojom (right):

https://codereview.chromium.org/1053773007/diff/80001/media/mojo/interfaces/p...
media/mojo/interfaces/platform_verification.mojom:17: // - |success|: Whether
the platform is successfully verified. If true/false the
On 2015/05/07 00:49:48, DaleCurtis wrote:
> Consistently use upper case or lower case after the : in these messages.

Done.

[xhwang, 2015-05-07 18:52:33]

On 2015/05/07 16:33:47, Darren Krahn wrote:
> On 2015/05/07 00:27:13, xhwang wrote:
> > On 2015/05/06 00:28:49, DaleCurtis wrote:
> > > Would be nice if there was a small app test for this too.
> > 
> > I tried to add a unit test. The test itself is trivial. But the default
> > PlatformVerificationFlow needs four fake classes to work in a test
> environment.
> > Having to pull those four classes over and plumb a way to set those seems
> > overkill for such a simple change :(
> 
> Agreed -- and pulling in those fakes would be poor test practice b/c you're
not
> testing PlatformVerificationFlow. The issue is that you have an explicit
> dependency on PlatformVerificationFlow (and by extension that
> PlatformVerificationFlow is not pure abstract with an Impl class -- my fault).
> I've filed a bug to make this test-able. If you want to make the dependency
> injected now that would make the test easier later.

It should be trivial to change PVI to be able to have an injected PVF. I'll
leave that until we have a abstract class.
 
> lgtm

[xhwang, 2015-05-07 18:53:37]

xhwang@chromium.org changed reviewers:
+ jam@chromium.org, xiyuan@chromium.org

[xhwang, 2015-05-07 18:53:39]

xiyuan: Please OWNERS review chrome/browser/chromeos
jam: Please OWNERS review content/

[xiyuan, 2015-05-07 20:37:34]

c/b/chromeos lgtm

[jam, 2015-05-11 17:15:26]

On 2015/05/07 18:53:39, xhwang wrote:
> xiyuan: Please OWNERS review chrome/browser/chromeos
> jam: Please OWNERS review content/

lgtm

[xhwang, 2015-05-11 17:17:53]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2015-05-11 17:17:54]

The patchset sent to the CQ was uploaded after l-g-t-m from dkrahn@chromium.org,
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1053773007/#ps100001
(title: "comments addressed")

[commit-bot: I haz the power, 2015-05-11 17:25:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1053773007/100001

[commit-bot: I haz the power, 2015-05-11 17:46:19]

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

[commit-bot: I haz the power, 2015-05-11 17:46:25]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  cast_shell_android on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[xhwang, 2015-05-11 18:55:33]

rebase only

[xhwang, 2015-05-11 21:15:02]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2015-05-11 21:15:03]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
jam@chromium.org, dkrahn@chromium.org, dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1053773007/#ps120001
(title: "rebase only")

[commit-bot: I haz the power, 2015-05-11 21:16:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1053773007/120001

[commit-bot: I haz the power, 2015-05-11 22:02:49]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2015-05-11 22:03:25]

Patchset 6 (id:??) landed as
https://crrev.com/9ab9648385b5455904aa91f62ce3e77df366f292
Cr-Commit-Position: refs/heads/master@{#329247}