Copy challenge*Key methods to enterprise.platformKeys.

Copy challenge*Key methods to enterprise.platformKeys.

Up to this point the challengeMachineKey and challengeUserKey methods
were only available through enterprise.platformKeysPrivate because the
web APIs required to meaningfully use these methods had not been
published (and now have).

The methods will remain in enterprise.platformKeysPrivate until all
users have been migrated to use enterprise.platformKeys.

The implementation is identical and has been abstracted so it can be
invoked from either entry point.

BUG=chromium:329341
TEST=unit tests extended to test both entry points

Committed: https://crrev.com/3f7776ec96bc700afcb61fc2c9e795f3cbe4e227
Cr-Commit-Position: refs/heads/master@{#377845}

[Darren Krahn, 2016-02-10 19:26:36]

dkrahn@chromium.org changed reviewers:
+ emaxx@chromium.org, mkwst@chromium.org, pneubeck@chromium.org,
rdevlin.cronin@chromium.org

[Darren Krahn, 2016-02-10 19:26:36]

+rdevlin.cronin - extensions OWNER

+emaxx - enterprisePlatformKeys OWNER

+mkwst - documentation
(https://chrome-apps-doc.appspot.com/_patch/1685103003/extensions/enterprise_p...
just gives "Internal server error", not sure if I'm missing something or if this
is just down)

+pneubeck - FYI - review not necessary

[Darren Krahn, 2016-02-17 20:40:25]

On 2016/02/10 19:26:36, Darren Krahn wrote:
> +rdevlin.cronin - extensions OWNER
> 
> +emaxx - enterprisePlatformKeys OWNER
> 
> +mkwst - documentation
>
(https://chrome-apps-doc.appspot.com/_patch/1685103003/extensions/enterprise_p...
> just gives "Internal server error", not sure if I'm missing something or if
this
> is just down)
> 
> +pneubeck - FYI - review not necessary

ping

[Devlin, 2016-02-17 20:42:04]

On 2016/02/17 20:40:25, Darren Krahn wrote:
> On 2016/02/10 19:26:36, Darren Krahn wrote:
> > +rdevlin.cronin - extensions OWNER
> > 
> > +emaxx - enterprisePlatformKeys OWNER
> > 
> > +mkwst - documentation
> >
>
(https://chrome-apps-doc.appspot.com/_patch/1685103003/extensions/enterprise_p...
> > just gives "Internal server error", not sure if I'm missing something or if
> this
> > is just down)
> > 
> > +pneubeck - FYI - review not necessary
> 
> ping

Since I'm only needed for the idl, I'm gonna wait on enterprise platform keys
OWNERS to chime in in case the functionality changes as a result of their
comments.

[emaxx, 2016-02-18 23:35:18]

I agree with the implementation. I have only one concern regarding the code
structuring (please see the comment).

https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/browser/exten...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.h
(right):

https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/browser/exten...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.h:48:
class EPKPChallengeKeyBase {
Considering the old functions in enterprise.platformKeysPrivate API as becoming
sort-of deprecated, this class and its descendants should probably be renamed
("EPKP"=>"EPK") and moved out of this header.
This IMO would leave the code in a clearer state: first, the public API
implementation part won't depend on the private part, and second, the private
part will contain only the thin wrapper around the common code.

[Darren Krahn, 2016-02-19 16:45:23]

https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/browser/exten...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.h
(right):

https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/browser/exten...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.h:48:
class EPKPChallengeKeyBase {
On 2016/02/18 23:35:18, emaxx wrote:
> Considering the old functions in enterprise.platformKeysPrivate API as
becoming
> sort-of deprecated, this class and its descendants should probably be renamed
> ("EPKP"=>"EPK") and moved out of this header.
> This IMO would leave the code in a clearer state: first, the public API
> implementation part won't depend on the private part, and second, the private
> part will contain only the thin wrapper around the common code.

I agree. In this CL I was going for minimal diffs and leaving the refactor for
another CL. Is that acceptable or do you want it all in this CL?

[emaxx, 2016-02-19 17:19:11]

I've left another comment regarding the type used in the proposed extension API.
Devlin, what is your opinion on this question?


On 2016/02/19 16:45:23, Darren Krahn wrote:
>
https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/browser/exten...
> File
>
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.h
> (right):
> 
>
https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/browser/exten...
>
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.h:48:
> class EPKPChallengeKeyBase {
> On 2016/02/18 23:35:18, emaxx wrote:
> > Considering the old functions in enterprise.platformKeysPrivate API as
> becoming
> > sort-of deprecated, this class and its descendants should probably be
renamed
> > ("EPKP"=>"EPK") and moved out of this header.
> > This IMO would leave the code in a clearer state: first, the public API
> > implementation part won't depend on the private part, and second, the
private
> > part will contain only the thin wrapper around the common code.
> 
> I agree. In this CL I was going for minimal diffs and leaving the refactor for
> another CL. Is that acceptable or do you want it all in this CL?

OK, I'm perfectly fine with that.


LGTM for enterprisePlatformKeys

[emaxx, 2016-02-19 17:20:59]

(the comment was lost in previous reply)

https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/common/extens...
File chrome/common/extensions/api/enterprise_platform_keys.idl (right):

https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/common/extens...
chrome/common/extensions/api/enterprise_platform_keys.idl:114: //             
encoded using Base64.
Isn't it better to use ArrayBuffer's instead of the base64-encoded strings?

As far as I can see, the existing methods in the chrome.enterprise.platformKeys
API (or, for example, in the somehow relevant chrome.certificateProvider API)
use ArrayBuffer in order to pass and receive the binary data.

[Devlin, 2016-02-19 18:05:02]

lgtm

https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/browser/exten...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc
(right):

https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/browser/exten...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc:260:
&EPKPChallengeMachineKey::Run, base::Unretained(impl_),
nit: please comment on why Unretained is safe here.

https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/browser/exten...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc:297:
&EPKPChallengeUserKey::Run, base::Unretained(impl_),
ditto

https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/browser/exten...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h
(left):

https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/browser/exten...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h:81:
: public ChromeUIThreadExtensionFunction {
Thanks :)

https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/browser/exten...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h
(right):

https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/browser/exten...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h:100:
EnterprisePlatformKeysChallengeMachineKeyFunction(
nit: explicit

https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/browser/exten...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h:105:
ResponseAction Run() override;
nit: newline between overridden/non-overridden functions.

https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/browser/exten...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h:106:
void OnChallengedKey(bool success, const std::string& data);
nit: function comments.

https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/browser/exten...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc
(right):

https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/browser/exten...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc:557:
EnterprisePlatformKeysPrivateChallengeMachineKeyFunction::Run() {
This all seems to be pretty much copy-paste.  IIUC, this will be fixed in a
followup.  Can you add a TODO in the meantime?

https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/browser/exten...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.h
(right):

https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/browser/exten...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.h:235:
EPKPChallengeMachineKey* impl_for_testing);
same comments as other file :)

https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/common/extens...
File chrome/common/extensions/api/enterprise_platform_keys.idl (right):

https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/common/extens...
chrome/common/extensions/api/enterprise_platform_keys.idl:114: //             
encoded using Base64.
On 2016/02/19 17:20:58, emaxx wrote:
> Isn't it better to use ArrayBuffer's instead of the base64-encoded strings?
> 
> As far as I can see, the existing methods in the
chrome.enterprise.platformKeys
> API (or, for example, in the somehow relevant chrome.certificateProvider API)
> use ArrayBuffer in order to pass and receive the binary data.

I don't know that one is inherently better than the other (probably depends on
the data), but I would say that we should be consistent.  So yeah, I'd prefer
ArrayBuffer here.

[Darren Krahn, 2016-02-23 23:39:46]

https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/browser/exten...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc
(right):

https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/browser/exten...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc:260:
&EPKPChallengeMachineKey::Run, base::Unretained(impl_),
On 2016/02/19 18:05:02, Devlin wrote:
> nit: please comment on why Unretained is safe here.

Done.

https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/browser/exten...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc:297:
&EPKPChallengeUserKey::Run, base::Unretained(impl_),
On 2016/02/19 18:05:02, Devlin wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/browser/exten...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h
(right):

https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/browser/exten...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h:100:
EnterprisePlatformKeysChallengeMachineKeyFunction(
On 2016/02/19 18:05:02, Devlin wrote:
> nit: explicit

Done.

https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/browser/exten...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h:105:
ResponseAction Run() override;
On 2016/02/19 18:05:02, Devlin wrote:
> nit: newline between overridden/non-overridden functions.

Done.

https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/browser/exten...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h:106:
void OnChallengedKey(bool success, const std::string& data);
On 2016/02/19 18:05:02, Devlin wrote:
> nit: function comments.

Done.

https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/browser/exten...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc
(right):

https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/browser/exten...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc:557:
EnterprisePlatformKeysPrivateChallengeMachineKeyFunction::Run() {
On 2016/02/19 18:05:02, Devlin wrote:
> This all seems to be pretty much copy-paste.  IIUC, this will be fixed in a
> followup.  Can you add a TODO in the meantime?

Done.

https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/browser/exten...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.h
(right):

https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/browser/exten...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.h:235:
EPKPChallengeMachineKey* impl_for_testing);
On 2016/02/19 18:05:02, Devlin wrote:
> same comments as other file :)

Done.

https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/common/extens...
File chrome/common/extensions/api/enterprise_platform_keys.idl (right):

https://chromiumcodereview.appspot.com/1685103003/diff/1/chrome/common/extens...
chrome/common/extensions/api/enterprise_platform_keys.idl:114: //             
encoded using Base64.
On 2016/02/19 18:05:02, Devlin wrote:
> On 2016/02/19 17:20:58, emaxx wrote:
> > Isn't it better to use ArrayBuffer's instead of the base64-encoded strings?
> > 
> > As far as I can see, the existing methods in the
> chrome.enterprise.platformKeys
> > API (or, for example, in the somehow relevant chrome.certificateProvider
API)
> > use ArrayBuffer in order to pass and receive the binary data.
> 
> I don't know that one is inherently better than the other (probably depends on
> the data), but I would say that we should be consistent.  So yeah, I'd prefer
> ArrayBuffer here.

Done.

[Darren Krahn, 2016-02-23 23:43:20]

dkrahn@chromium.org changed reviewers:
+ bauerb@chromium.org, isherman@chromium.org

[Darren Krahn, 2016-02-23 23:43:20]

+bauerb for prefs
+isherman for histograms

[Ilya Sherman, 2016-02-24 01:03:11]

histograms.xml lgtm

[Mike West, 2016-02-24 08:19:26]

LGTM.

[Bernhard Bauer, 2016-02-24 08:44:37]

prefs/ LGTM.

[Darren Krahn, 2016-02-26 06:30:57]

The CQ bit was checked by dkrahn@chromium.org

[Darren Krahn, 2016-02-26 06:30:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, emaxx@chromium.org
Link to the patchset: https://codereview.chromium.org/1685103003/#ps20001
(title: "Fixed nits; now use ArrayBuffer")

[commit-bot: I haz the power, 2016-02-26 06:32:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1685103003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1685103003/20001

[commit-bot: I haz the power, 2016-02-26 08:13:30]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-02-26 08:15:21]

Description was changed from

==========
Copy challenge*Key methods to enterprise.platformKeys.

Up to this point the challengeMachineKey and challengeUserKey methods
were only available through enterprise.platformKeysPrivate because the
web APIs required to meaningfully use these methods had not been
published (and now have).

The methods will remain in enterprise.platformKeysPrivate until all
users have been migrated to use enterprise.platformKeys.

The implementation is identical and has been abstracted so it can be
invoked from either entry point.

BUG=chromium:329341
TEST=unit tests extended to test both entry points
==========

to

==========
Copy challenge*Key methods to enterprise.platformKeys.

Up to this point the challengeMachineKey and challengeUserKey methods
were only available through enterprise.platformKeysPrivate because the
web APIs required to meaningfully use these methods had not been
published (and now have).

The methods will remain in enterprise.platformKeysPrivate until all
users have been migrated to use enterprise.platformKeys.

The implementation is identical and has been abstracted so it can be
invoked from either entry point.

BUG=chromium:329341
TEST=unit tests extended to test both entry points

Committed: https://crrev.com/3f7776ec96bc700afcb61fc2c9e795f3cbe4e227
Cr-Commit-Position: refs/heads/master@{#377845}
==========

[commit-bot: I haz the power, 2016-02-26 08:15:23]

Patchset 2 (id:??) landed as
https://crrev.com/3f7776ec96bc700afcb61fc2c9e795f3cbe4e227
Cr-Commit-Position: refs/heads/master@{#377845}