Allow machine key challenges by signin profiles.

Allow machine key challenges by signin profiles.

This allows calling chrome.enterprise.platformKeys.challengeMachineKey() from a profile that does not have a user associated with it.

BUG=715121
TEST=unit tests

Review-Url: https://codereview.chromium.org/2841553002
Cr-Commit-Position: refs/heads/master@{#467991}
Committed: https://chromium.googlesource.com/chromium/src/+/8b9e9fcb9eb5c2408d8488f2f619b51ff24d0470

[The one and only Dr. Crash, 2017-04-25 15:27:35]

Description was changed from

==========
Allow machine key challenges by signin profiles.
==========

to

==========
Allow machine key challenges by signin profiles.

BUG=715121
TEST=unit tests
==========

[The one and only Dr. Crash, 2017-04-26 08:58:37]

The CQ bit was checked by drcrash@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 08:58:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 08:58:50]

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

[commit-bot: I haz the power, 2017-04-26 08:58:51]

Dry run: CLs for remote refs other than refs/heads/master must contain
NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them

[The one and only Dr. Crash, 2017-04-26 09:02:17]

drcrash@chromium.org changed reviewers:
+ emaxx@chromium.org

[The one and only Dr. Crash, 2017-04-26 09:02:18]

Whitelisted apps on the signing screen are allowed to call remote attestation
for enterprise machine keys.

[The one and only Dr. Crash, 2017-04-26 09:02:58]

I plan a second CL to ensure that an appropriate error is given for
challengeUserKey() but did not want to mix too many things in that one CL.

[The one and only Dr. Crash, 2017-04-26 13:14:54]

The CQ bit was checked by drcrash@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 13:15:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[The one and only Dr. Crash, 2017-04-26 14:58:55]

The CQ bit was checked by drcrash@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 14:59:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[The one and only Dr. Crash, 2017-04-26 15:05:51]

The CQ bit was checked by drcrash@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 15:06:07]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[The one and only Dr. Crash, 2017-04-26 15:06:56]

On 2017/04/26 09:02:58, The one and only Dr. Crash wrote:
> I plan a second CL to ensure that an appropriate error is given for
> challengeUserKey() but did not want to mix too many things in that one CL.

Second CL: https://codereview.chromium.org/2838423003

[The one and only Dr. Crash, 2017-04-26 15:09:55]

The CQ bit was checked by drcrash@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 15:10:08]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[The one and only Dr. Crash, 2017-04-26 15:20:05]

The CQ bit was checked by drcrash@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 15:20:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 15:57:02]

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

[commit-bot: I haz the power, 2017-04-26 15:57:03]

Dry run: This issue passed the CQ dry run.

[emaxx, 2017-04-26 21:19:03]

https://codereview.chromium.org/2841553002/diff/280001/chrome/browser/extensi...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc
(right):

https://codereview.chromium.org/2841553002/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc:148:
return Manifest::IsPolicyLocation(extension_->location());
nit: #include "extensions/common/extension.h"

https://codereview.chromium.org/2841553002/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc:342:
if (!chromeos::ProfileHelper::IsSigninProfile(profile_) &&
nit: Maybe drop a short explanatory comment here? The condition becomes a bit
complex to read.

https://codereview.chromium.org/2841553002/diff/280001/chrome/browser/extensi...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.h
(right):

https://codereview.chromium.org/2841553002/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.h:121:
const extensions::Extension* extension_;
I believe it's unsafe to store the raw pointer, as an extension may become
deleted and the Extension object may be destroyed while the platformKeys
operation is running.
So either store some "elementary" pieces of data here (like std::string
extension_id, bool force_installed_extension) or use scoped_refptr<const
Extension> instead here.

https://codereview.chromium.org/2841553002/diff/280001/chrome/browser/extensi...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc
(right):

https://codereview.chromium.org/2841553002/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc:153:
class EPKPChallengeKeyTestBase : public BrowserWithTestWindowTest {
What about accepting ProfileType in constructor, instead of this a bit tricky
behavior "descendant class rewrites it"?
Then it will be, IMO, easier to understand where is it coming from.

Also this would allow creating of extension_ in the constructor, like it was in
the original version. The SetUpExtension() and SetUpContext() methods could be
then removed.

https://codereview.chromium.org/2841553002/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc:155:
enum ProfileType { USER_PROFILE, SIGNIN_PROFILE };
nit: "enum class" is recommended over "enum".
Also it's recommended to use constant-style naming if possible:
https://google.github.io/styleguide/cppguide.html#Enumerator_Names
So suggesting:
enum class ProfileType { kUser, kSignin };

https://codereview.chromium.org/2841553002/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc:194:
virtual scoped_refptr<extensions::Extension> CreateExtension() {
nit: Drop "virtual" and make the method private.

https://codereview.chromium.org/2841553002/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc:194:
virtual scoped_refptr<extensions::Extension> CreateExtension() {
nit: Remove "extensions::".
(Applies to other parts in this file.)

https://codereview.chromium.org/2841553002/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc:370:
class EPKPChallengeMachineKeyAllProfilesTest
nit: Please add a short comment for this class, e.g.:
// Tests of challengeMachineKey function that should run on user profile and
// sign-in profile, depending on the supplied parameter.

https://codereview.chromium.org/2841553002/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc:422:
func_->set_extension(extension_.get());
Can this be done in one of the existing SetUp* functions?
It's a bit too much to introduce a virtual method just for this simple case.

[The one and only Dr. Crash, 2017-04-26 22:04:14]

https://codereview.chromium.org/2841553002/diff/280001/chrome/browser/extensi...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc
(right):

https://codereview.chromium.org/2841553002/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc:148:
return Manifest::IsPolicyLocation(extension_->location());
On 2017/04/26 21:19:02, emaxx wrote:
> nit: #include "extensions/common/extension.h"

Done.

https://codereview.chromium.org/2841553002/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc:342:
if (!chromeos::ProfileHelper::IsSigninProfile(profile_) &&
On 2017/04/26 21:19:03, emaxx wrote:
> nit: Maybe drop a short explanatory comment here? The condition becomes a bit
> complex to read.

Done.

https://codereview.chromium.org/2841553002/diff/280001/chrome/browser/extensi...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc
(right):

https://codereview.chromium.org/2841553002/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc:153:
class EPKPChallengeKeyTestBase : public BrowserWithTestWindowTest {
On 2017/04/26 21:19:03, emaxx wrote:
> What about accepting ProfileType in constructor, instead of this a bit tricky
> behavior "descendant class rewrites it"?
> Then it will be, IMO, easier to understand where is it coming from.
> 
> Also this would allow creating of extension_ in the constructor, like it was
in
> the original version. The SetUpExtension() and SetUpContext() methods could be
> then removed.

No, it has to be a param for the tests. It's simpler that way.

https://codereview.chromium.org/2841553002/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc:155:
enum ProfileType { USER_PROFILE, SIGNIN_PROFILE };
On 2017/04/26 21:19:03, emaxx wrote:
> nit: "enum class" is recommended over "enum".
> Also it's recommended to use constant-style naming if possible:
> https://google.github.io/styleguide/cppguide.html#Enumerator_Names
> So suggesting:
> enum class ProfileType { kUser, kSignin };

Done.

https://codereview.chromium.org/2841553002/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc:194:
virtual scoped_refptr<extensions::Extension> CreateExtension() {
On 2017/04/26 21:19:03, emaxx wrote:
> nit: Remove "extensions::".
> (Applies to other parts in this file.)

Done.

https://codereview.chromium.org/2841553002/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc:194:
virtual scoped_refptr<extensions::Extension> CreateExtension() {
On 2017/04/26 21:19:03, emaxx wrote:
> nit: Drop "virtual" and make the method private.

Done.

https://codereview.chromium.org/2841553002/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc:194:
virtual scoped_refptr<extensions::Extension> CreateExtension() {
On 2017/04/26 21:19:03, emaxx wrote:
> nit: Drop "virtual" and make the method private.

Done.

https://codereview.chromium.org/2841553002/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc:370:
class EPKPChallengeMachineKeyAllProfilesTest
On 2017/04/26 21:19:03, emaxx wrote:
> nit: Please add a short comment for this class, e.g.:
> // Tests of challengeMachineKey function that should run on user profile and
> // sign-in profile, depending on the supplied parameter.

Done.

https://codereview.chromium.org/2841553002/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc:422:
func_->set_extension(extension_.get());
On 2017/04/26 21:19:03, emaxx wrote:
> Can this be done in one of the existing SetUp* functions?
> It's a bit too much to introduce a virtual method just for this simple case.

No, but I got rid of the unnecessary SetUpContext().

[The one and only Dr. Crash, 2017-04-26 22:51:06]

The CQ bit was checked by drcrash@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 22:51:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emaxx, 2017-04-26 23:34:44]

https://codereview.chromium.org/2841553002/diff/280001/chrome/browser/extensi...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.h
(right):

https://codereview.chromium.org/2841553002/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.h:121:
const extensions::Extension* extension_;
On 2017/04/26 21:19:03, emaxx wrote:
> I believe it's unsafe to store the raw pointer, as an extension may become
> deleted and the Extension object may be destroyed while the platformKeys
> operation is running.
> So either store some "elementary" pieces of data here (like std::string
> extension_id, bool force_installed_extension) or use scoped_refptr<const
> Extension> instead here.

Ping. Or there's some evidence that this Extension instance will stay alive for
the whole lifetime of EPKPChallengeKeyBas?

https://codereview.chromium.org/2841553002/diff/280001/chrome/browser/extensi...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc
(right):

https://codereview.chromium.org/2841553002/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc:153:
class EPKPChallengeKeyTestBase : public BrowserWithTestWindowTest {
On 2017/04/26 22:04:13, The one and only Dr. Crash wrote:
> On 2017/04/26 21:19:03, emaxx wrote:
> > What about accepting ProfileType in constructor, instead of this a bit
tricky
> > behavior "descendant class rewrites it"?
> > Then it will be, IMO, easier to understand where is it coming from.
> > 
> > Also this would allow creating of extension_ in the constructor, like it was
> in
> > the original version. The SetUpExtension() and SetUpContext() methods could
be
> > then removed.
> 
> No, it has to be a param for the tests. It's simpler that way.

I suggested to make it the constructor's parameter here in
EPKPChallengeKeyTestBase. Then the derived
EPKPChallengeMachineKeyAllProfilesTest can call it with GetParam().

It will be simpler because profile_type_ will be known straight away in the base
constructor. This means that extension_ can be created in constructor as it was
before, and consequently no need for (virtual) SetUpExtension().

[commit-bot: I haz the power, 2017-04-26 23:41:53]

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

[commit-bot: I haz the power, 2017-04-26 23:41:54]

Dry run: This issue passed the CQ dry run.

[The one and only Dr. Crash, 2017-04-27 20:39:40]

The CQ bit was checked by drcrash@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-27 20:40:19]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[The one and only Dr. Crash, 2017-04-27 20:57:45]

The CQ bit was checked by drcrash@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-27 20:58:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[The one and only Dr. Crash, 2017-04-27 21:19:12]

The CQ bit was unchecked by drcrash@chromium.org

[The one and only Dr. Crash, 2017-04-27 21:19:15]

The CQ bit was checked by drcrash@chromium.org

[commit-bot: I haz the power, 2017-04-27 21:19:32]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-27 21:19:33]

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

[commit-bot: I haz the power, 2017-04-27 21:19:34]

No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[The one and only Dr. Crash, 2017-04-27 21:48:59]

The CQ bit was checked by drcrash@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-27 21:49:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 01:36:25]

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

[commit-bot: I haz the power, 2017-04-28 01:36:26]

Dry run: This issue passed the CQ dry run.

[emaxx, 2017-04-28 13:03:14]

LGTM % nits
And please add some description to the CL. At least, it should mention that this
affects the challengeMachineKey method in the chrome.enterprise.platformKeys
API.

https://codereview.chromium.org/2841553002/diff/340001/chrome/browser/extensi...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.h
(right):

https://codereview.chromium.org/2841553002/diff/340001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.h:41:
class Extension;
nit: I think I've already commented on that, so: forward declaration is not the
right thing here, you need to include "extensions/common/extension.h", because
scoped_refptr doesn't work with incomplete types by default:
https://cs.chromium.org/chromium/src/base/memory/ref_counted.h?l=558&rcl=c443...
(It works here currently probably because some other header includes
extension.h, but that's accidental.)

https://codereview.chromium.org/2841553002/diff/340001/chrome/browser/extensi...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc
(right):

https://codereview.chromium.org/2841553002/diff/340001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api_unittest.cc:260:
EPKPChallengeMachineKeyTest(
nit: explicit

[The one and only Dr. Crash, 2017-04-28 13:24:16]

Description was changed from

==========
Allow machine key challenges by signin profiles.

BUG=715121
TEST=unit tests
==========

to

==========
Allow machine key challenges by signin profiles.

This allows calling chrome.enterprise.platformKeys.challengeMachineKey() from a
profile that does not have a user associated with it.

BUG=715121
TEST=unit tests
==========

[The one and only Dr. Crash, 2017-04-28 13:31:21]

The CQ bit was checked by drcrash@chromium.org

[The one and only Dr. Crash, 2017-04-28 13:31:22]

The patchset sent to the CQ was uploaded after l-g-t-m from emaxx@chromium.org
Link to the patchset: https://codereview.chromium.org/2841553002/#ps360001
(title: "Review nits.")

[commit-bot: I haz the power, 2017-04-28 13:31:47]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 14:17:31]

CQ is committing da patch.

Bot data: {"patchset_id": 360001, "attempt_start_ts": 1493386281630810,
"parent_rev": "00b1e7279ad1c6b8b538faa9a5c4facd37dc8666", "commit_rev":
"8b9e9fcb9eb5c2408d8488f2f619b51ff24d0470"}

[commit-bot: I haz the power, 2017-04-28 14:17:41]

Description was changed from

==========
Allow machine key challenges by signin profiles.

This allows calling chrome.enterprise.platformKeys.challengeMachineKey() from a
profile that does not have a user associated with it.

BUG=715121
TEST=unit tests
==========

to

==========
Allow machine key challenges by signin profiles.

This allows calling chrome.enterprise.platformKeys.challengeMachineKey() from a
profile that does not have a user associated with it.

BUG=715121
TEST=unit tests

Review-Url: https://codereview.chromium.org/2841553002
Cr-Commit-Position: refs/heads/master@{#467991}
Committed:
https://chromium.googlesource.com/chromium/src/+/8b9e9fcb9eb5c2408d8488f2f619...
==========

[commit-bot: I haz the power, 2017-04-28 14:17:43]

Committed patchset #19 (id:360001) as
https://chromium.googlesource.com/chromium/src/+/8b9e9fcb9eb5c2408d8488f2f619...