Add ClientCertStoreChromeOS which only returns the certs for a given user.

Add ClientCertStoreChromeOS which only returns the certs for a given user.

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

[Ryan Sleevi, 2013-12-11 06:52:50]

https://codereview.chromium.org/112533002/diff/20001/net/cert/nss_profile_fil...
File net/cert/nss_profile_filter_chromeos.cc (right):

https://codereview.chromium.org/112533002/diff/20001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos.cc:12: namespace {
style nit: line break

https://codereview.chromium.org/112533002/diff/20001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos.cc:15: std::string r;
naming nit: result

https://codereview.chromium.org/112533002/diff/20001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos.cc:26:
base::IntToString(PK11_GetSlotID(slot_element->slot));
base::StringAppendF("%ul:%ul") ?

In both cases, CK_SLOT_ID / SECMODModuleID are unsigned long , not ints.

https://codereview.chromium.org/112533002/diff/20001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos.cc:50: // If this is a completely different
module, allow it.
Can you expand this comment?

// If this is not the internal (file-system) module, allow it.

https://codereview.chromium.org/112533002/diff/20001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos.cc:79: << "  from:" <<
CertSlotsString(cert);
nit: Missing spaces after the ": ", and extra space before the from? Should it
be a newline instead?

https://codereview.chromium.org/112533002/diff/20001/net/cert/nss_profile_fil...
File net/cert/nss_profile_filter_chromeos.h (right):

https://codereview.chromium.org/112533002/diff/20001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos.h:30: // Predicate that can be used with
remove_if.  The operator() returns
nit: add linebreak

https://codereview.chromium.org/112533002/diff/20001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos.h:32: class Predicate {
naming nit: The naming here isn't very clear, and the comment itself makes it
hard to understand.

class NotAllowedForProfilePredicate ?

design nit: Having the double operator overloading seems dangerous per the style
guide

https://codereview.chromium.org/112533002/diff/20001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos.h:37: private:
style nit: line break

https://codereview.chromium.org/112533002/diff/20001/net/ssl/client_cert_stor...
File net/ssl/client_cert_store_chromeos.cc (right):

https://codereview.chromium.org/112533002/diff/20001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_chromeos.cc:30: base::Unretained(this),
Why is this Unretained safe?

https://codereview.chromium.org/112533002/diff/20001/net/ssl/client_cert_stor...
File net/ssl/client_cert_store_chromeos.h (right):

https://codereview.chromium.org/112533002/diff/20001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_chromeos.h:60: NSSProfileFilterChromeOS
profile_filter_;
nit: delete newline? or add comments?

https://codereview.chromium.org/112533002/diff/20001/net/ssl/client_cert_stor...
File net/ssl/client_cert_store_nss.cc (right):

https://codereview.chromium.org/112533002/diff/20001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_nss.cc:37: base::Unretained(this),
Why is this Unretained safe?

https://codereview.chromium.org/112533002/diff/20001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_nss.cc:44: callback.Run();
Let's write this differently, so that we don't run the risk at line 45

if (...::PostTaskAndReply(...))
  return

selected_certs->clear();
callback.Run();

https://codereview.chromium.org/112533002/diff/20001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_nss.cc:86: << (node->cert->nickname ?
node->cert->nickname : "");
rewrite these all as base::StringPiece(node->cert->nickname) and get the NULL
cert handling "for free" ?

[mattm, 2013-12-12 00:45:22]

https://codereview.chromium.org/112533002/diff/20001/net/cert/nss_profile_fil...
File net/cert/nss_profile_filter_chromeos.cc (right):

https://codereview.chromium.org/112533002/diff/20001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos.cc:12: namespace {
On 2013/12/11 06:52:50, Ryan Sleevi wrote:
> style nit: line break

Done.

https://codereview.chromium.org/112533002/diff/20001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos.cc:15: std::string r;
On 2013/12/11 06:52:50, Ryan Sleevi wrote:
> naming nit: result

Done.

https://codereview.chromium.org/112533002/diff/20001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos.cc:26:
base::IntToString(PK11_GetSlotID(slot_element->slot));
On 2013/12/11 06:52:50, Ryan Sleevi wrote:
> base::StringAppendF("%ul:%ul") ?
> 
> In both cases, CK_SLOT_ID / SECMODModuleID are unsigned long , not ints.

Done.

https://codereview.chromium.org/112533002/diff/20001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos.cc:50: // If this is a completely different
module, allow it.
On 2013/12/11 06:52:50, Ryan Sleevi wrote:
> Can you expand this comment?
> 
> // If this is not the internal (file-system) module, allow it.

Done.

https://codereview.chromium.org/112533002/diff/20001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos.cc:79: << "  from:" <<
CertSlotsString(cert);
On 2013/12/11 06:52:50, Ryan Sleevi wrote:
> nit: Missing spaces after the ": ", and extra space before the from? Should it
> be a newline instead?

changed the format around, should be better now.

https://codereview.chromium.org/112533002/diff/20001/net/cert/nss_profile_fil...
File net/cert/nss_profile_filter_chromeos.h (right):

https://codereview.chromium.org/112533002/diff/20001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos.h:30: // Predicate that can be used with
remove_if.  The operator() returns
On 2013/12/11 06:52:50, Ryan Sleevi wrote:
> nit: add linebreak

Done.

https://codereview.chromium.org/112533002/diff/20001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos.h:32: class Predicate {
On 2013/12/11 06:52:50, Ryan Sleevi wrote:
> naming nit: The naming here isn't very clear, and the comment itself makes it
> hard to understand.
> 
> class NotAllowedForProfilePredicate ?
> 
> design nit: Having the double operator overloading seems dangerous per the
style
> guide

Renamed and split into two.

https://codereview.chromium.org/112533002/diff/20001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos.h:37: private:
On 2013/12/11 06:52:50, Ryan Sleevi wrote:
> style nit: line break

Done.

https://codereview.chromium.org/112533002/diff/20001/net/ssl/client_cert_stor...
File net/ssl/client_cert_store_chromeos.cc (right):

https://codereview.chromium.org/112533002/diff/20001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_chromeos.cc:30: base::Unretained(this),
On 2013/12/11 06:52:50, Ryan Sleevi wrote:
> Why is this Unretained safe?

Added comment (and expanded comment in net/ssl/client_cert_store.h).

https://codereview.chromium.org/112533002/diff/20001/net/ssl/client_cert_stor...
File net/ssl/client_cert_store_chromeos.h (right):

https://codereview.chromium.org/112533002/diff/20001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_chromeos.h:60: NSSProfileFilterChromeOS
profile_filter_;
On 2013/12/11 06:52:50, Ryan Sleevi wrote:
> nit: delete newline? or add comments?

Done.

https://codereview.chromium.org/112533002/diff/20001/net/ssl/client_cert_stor...
File net/ssl/client_cert_store_nss.cc (right):

https://codereview.chromium.org/112533002/diff/20001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_nss.cc:37: base::Unretained(this),
On 2013/12/11 06:52:50, Ryan Sleevi wrote:
> Why is this Unretained safe?
added comment.

https://codereview.chromium.org/112533002/diff/20001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_nss.cc:44: callback.Run();
On 2013/12/11 06:52:50, Ryan Sleevi wrote:
> Let's write this differently, so that we don't run the risk at line 45
> 
> if (...::PostTaskAndReply(...))
>   return
> 
> selected_certs->clear();
> callback.Run();

Done.

https://codereview.chromium.org/112533002/diff/20001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_nss.cc:86: << (node->cert->nickname ?
node->cert->nickname : "");
On 2013/12/11 06:52:50, Ryan Sleevi wrote:
> rewrite these all as base::StringPiece(node->cert->nickname) and get the NULL
> cert handling "for free" ?

Done.

[Ryan Sleevi, 2013-12-12 00:53:26]

lgtm

https://codereview.chromium.org/112533002/diff/60001/net/cert/nss_profile_fil...
File net/cert/nss_profile_filter_chromeos.h (right):

https://codereview.chromium.org/112533002/diff/60001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos.h:33:
CertNotAllowedForProfilePredicate(const NSSProfileFilterChromeOS& filter);
explicit

https://codereview.chromium.org/112533002/diff/60001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos.h:42:
ModuleNotAllowedForProfilePredicate(const NSSProfileFilterChromeOS& filter);
explicit

[mattm, 2013-12-12 02:58:48]

+ajwong for profile_io_data

https://codereview.chromium.org/112533002/diff/60001/net/cert/nss_profile_fil...
File net/cert/nss_profile_filter_chromeos.h (right):

https://codereview.chromium.org/112533002/diff/60001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos.h:33:
CertNotAllowedForProfilePredicate(const NSSProfileFilterChromeOS& filter);
On 2013/12/12 00:53:27, Ryan Sleevi wrote:
> explicit

Done.

https://codereview.chromium.org/112533002/diff/60001/net/cert/nss_profile_fil...
net/cert/nss_profile_filter_chromeos.h:42:
ModuleNotAllowedForProfilePredicate(const NSSProfileFilterChromeOS& filter);
On 2013/12/12 00:53:27, Ryan Sleevi wrote:
> explicit

Done.

[awong, 2013-12-12 05:34:43]

profile_io_data: LGTM

[commit-bot: I haz the power, 2013-12-16 19:18:52]

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

[commit-bot: I haz the power, 2013-12-16 19:50:11]

Retried try job too often on mac_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-12-16 19:54:27]

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

[commit-bot: I haz the power, 2013-12-17 00:09:04]

Change committed as 241080