Enable system token in platformKeys api.

Enable system token in platformKeys api.

BUG=210525
(for .idl comment change)
TBR=kalman@chromium.org

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

[pneubeck (no reviews), 2014-07-29 19:27:56]

ptal

[Joao da Silva, 2014-07-30 08:46:40]

The API impl looks good to me and the comments are only about code style.

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/platform_keys/platform_keys.h (right):

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys.h:29: const char
kTokenIdUser[] = "user";
Change to "extern const char kTokenIdUser[];" and define it in the .cc file

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys.h:29: const char
kTokenIdUser[] = "user";
Also, what are these constants for? Document them here

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys.h:122: // will contain the
token ids. If an error, |token_ids| will be NULL and
"If an error" occurs

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys.h:123: // |error_message|
will be set to an/ error message.
remove /

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys.h:129: void GetTokens(const
GetTokensCallback& callback,
Document that this must be called on UI, and |callback| will be called on UI
too.

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/platform_keys/platform_keys_nss.cc (right):

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:75: // Called back
with the NSSCertDatabase associated to the given |context|.
There is no |context| here

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:93: else if (token_id
== kTokenIdSystem)
Why isn't kTokenIdUser and kTokenIdSystem an enum?

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:122: // |token| is
not empty, the slot for |token_id|. Stores the slot in |state|
|token| -> |token_id|

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:283: // Must be
called on origin thread, use CallBack() therefore.
, therefore use Callback().

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:548:
token_ids->push_back(kTokenIdSystem);
Doesn't this depend on state->slot_, based on what DidGetCertDBOnIOThread is
doing?

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/extension...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc
(right):

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc:36:
std::string* platform_keys_token_id) {
platform_keys_token_id could be an enum declared in platform_keys.h

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc:49:
std::string PlatformKeysTokenIdToAPIId(
I think if you use the enum then this is not necessary

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc:56:
CHECK(false);
Why not Respond with an internal error instead of crashing here?

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/extension...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc
(right):

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc:31:

single newline

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc:75:
0x30, 0x82, 0x01, 0x54, 0x02, 0x01, 0x00, 0x30, 0x0d, 0x06, 0x09, 0x2a,
nit: 4 spaces for consistency with privateKeyPkcs8User

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc:163:
&loop));
It's more common to pass loop.QuitClosure() and then invoke that callback when
the task is done to unblock loop.Run()

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc:175:
&loop));
Same here

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc:189:
&loop));
And here

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc:201:
// token for which the Javscript test will import the certificate.
merge these 2 lines

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc:246:
NULL));
These lines are shared with DidGetCertDatabase() (from lines 229 to 246), move
to a helper method?

https://codereview.chromium.org/430563002/diff/20001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/enterprise_platform_keys/basic.js
(right):

https://codereview.chromium.org/430563002/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/enterprise_platform_keys/basic.js:305:
fail('token is not named "system".');
Shouldnt these checks be done for the userToken too?

https://codereview.chromium.org/430563002/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/enterprise_platform_keys/basic.js:616: 
Add newlines after the functions below too

[pneubeck (no reviews), 2014-07-30 13:53:46]

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/platform_keys/platform_keys.h (right):

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys.h:29: const char
kTokenIdUser[] = "user";
On 2014/07/30 08:46:38, Joao da Silva wrote:
> Change to "extern const char kTokenIdUser[];" and define it in the .cc file

Done.

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys.h:29: const char
kTokenIdUser[] = "user";
On 2014/07/30 08:46:38, Joao da Silva wrote:
> Also, what are these constants for? Document them here

Done.

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys.h:122: // will contain the
token ids. If an error, |token_ids| will be NULL and
On 2014/07/30 08:46:38, Joao da Silva wrote:
> "If an error" occurs

Done.

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys.h:123: // |error_message|
will be set to an/ error message.
On 2014/07/30 08:46:38, Joao da Silva wrote:
> remove /

Done.

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys.h:129: void GetTokens(const
GetTokensCallback& callback,
On 2014/07/30 08:46:38, Joao da Silva wrote:
> Document that this must be called on UI, and |callback| will be called on UI
> too.

Done.

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/platform_keys/platform_keys_nss.cc (right):

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:75: // Called back
with the NSSCertDatabase associated to the given |context|.
On 2014/07/30 08:46:38, Joao da Silva wrote:
> There is no |context| here

Done.

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:93: else if (token_id
== kTokenIdSystem)
On 2014/07/30 08:46:38, Joao da Silva wrote:
> Why isn't kTokenIdUser and kTokenIdSystem an enum?

Explained now at the declaration comment.

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:122: // |token| is
not empty, the slot for |token_id|. Stores the slot in |state|
On 2014/07/30 08:46:39, Joao da Silva wrote:
> |token| -> |token_id|

Done.

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:283: // Must be
called on origin thread, use CallBack() therefore.
On 2014/07/30 08:46:38, Joao da Silva wrote:
> , therefore use Callback().

Done.

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:548:
token_ids->push_back(kTokenIdSystem);
On 2014/07/30 08:46:39, Joao da Silva wrote:
> Doesn't this depend on state->slot_, based on what DidGetCertDBOnIOThread is
> doing?

GetCertDatabase always gets the DB for the browser context.
|token_id| (in case of the other functions) determines which slot of the DB to
use.

Here, we have to get the list of available slots of DB.

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/extension...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc
(right):

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc:36:
std::string* platform_keys_token_id) {
On 2014/07/30 08:46:39, Joao da Silva wrote:
> platform_keys_token_id could be an enum declared in platform_keys.h

Explained at the string constant declarations in platform_keys.h

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc:49:
std::string PlatformKeysTokenIdToAPIId(
On 2014/07/30 08:46:39, Joao da Silva wrote:
> I think if you use the enum then this is not necessary

How that? Even if both sides were an enum, it would require a enum <-> enum
translation.

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc:56:
CHECK(false);
On 2014/07/30 08:46:39, Joao da Silva wrote:
> Why not Respond with an internal error instead of crashing here?

Done.

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/extension...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc
(right):

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc:31:

On 2014/07/30 08:46:39, Joao da Silva wrote:
> single newline

Done.

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc:75:
0x30, 0x82, 0x01, 0x54, 0x02, 0x01, 0x00, 0x30, 0x0d, 0x06, 0x09, 0x2a,
On 2014/07/30 08:46:39, Joao da Silva wrote:
> nit: 4 spaces for consistency with privateKeyPkcs8User

Done.

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc:163:
&loop));
On 2014/07/30 08:46:39, Joao da Silva wrote:
> It's more common to pass loop.QuitClosure() and then invoke that callback when
> the task is done to unblock loop.Run()

Done.

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc:175:
&loop));
On 2014/07/30 08:46:39, Joao da Silva wrote:
> Same here

Done.

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc:189:
&loop));
On 2014/07/30 08:46:39, Joao da Silva wrote:
> And here

Done.

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc:201:
// token for which the Javscript test will import the certificate.
On 2014/07/30 08:46:39, Joao da Silva wrote:
> merge these 2 lines

Done.

https://codereview.chromium.org/430563002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc:246:
NULL));
On 2014/07/30 08:46:39, Joao da Silva wrote:
> These lines are shared with DidGetCertDatabase() (from lines 229 to 246), move
> to a helper method?

Done.

https://codereview.chromium.org/430563002/diff/20001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/enterprise_platform_keys/basic.js
(right):

https://codereview.chromium.org/430563002/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/enterprise_platform_keys/basic.js:305:
fail('token is not named "system".');
On 2014/07/30 08:46:39, Joao da Silva wrote:
> Shouldnt these checks be done for the userToken too?

Uuuhh. Yeah, something went wrong here.

https://codereview.chromium.org/430563002/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/enterprise_platform_keys/basic.js:616: 
On 2014/07/30 08:46:39, Joao da Silva wrote:
> Add newlines after the functions below too

Done.

[Joao da Silva, 2014-07-30 14:04:01]

lgtm

[pneubeck (no reviews), 2014-08-02 07:54:13]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-08-02 07:56:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pneubeck@chromium.org/430563002/40001

[commit-bot: I haz the power, 2014-08-02 08:09:31]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/36742)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/41774)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-02 08:11:27]

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

[commit-bot: I haz the power, 2014-08-02 08:11:28]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/36743)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)

[pneubeck (no reviews), 2014-08-02 14:35:19]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-08-02 14:35:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pneubeck@chromium.org/430563002/60001

[pneubeck (no reviews), 2014-08-02 15:39:34]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-08-02 15:40:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pneubeck@chromium.org/430563002/80001

[commit-bot: I haz the power, 2014-08-02 18:26:41]

Change committed as 287221

[Chirantan Ekbote, 2014-08-06 22:07:46]

https://codereview.chromium.org/430563002/diff/80001/chrome/browser/extension...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc
(right):

https://codereview.chromium.org/430563002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc:37:
// openssl pkcs8 -inform pem -in privkey.pem -topk8 \
This line is breaking my chrome os build:
 
../../chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc:37:1:
error: multi-line comment [-Werror=comment]
 // openssl pkcs8 -inform pem -in privkey.pem -topk8 \
 ^
cc1plus.elf: all warnings being treated as errors