Add functionality to OriginBoundCertStore interface and implementations.

Adds functionality to OriginBoundCertStore interface and implementations.
In particular, adds functions DeleteOriginBoundCert, DeleteAll, and
GetAllOriginBoundCerts. These functions will be used by the cert management UI.
Also changes SetOriginBoundCert's return value to void.

BUG=88782
TEST=net_unittests (in particular DefaultOriginBoundCertStoreTest.*)

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=97090

[rkn, 2011-08-09 20:58:24]

wtc: Please review.

rdsmith: Please review.

Please look at origin_bound_cert_store.h first to see the methods I added to
that interface. The other files are simply implementing those methods and
unit-testing them.

There are several miscellaneous cleanups which I would like to do in a follow-up
CL. Namely, I'd like to un-nest the DefaultOriginBoundCertStore::OriginBoundCert
class and remove the OriginBoundCertInfo struct (introduced in this CL).

I'd also like to change SetOriginBoundCert to return void because right now it
always return true.

http://codereview.chromium.org/7585037/diff/4001/net/base/origin_bound_cert_s...
File net/base/origin_bound_cert_store.h (right):

http://codereview.chromium.org/7585037/diff/4001/net/base/origin_bound_cert_s...
net/base/origin_bound_cert_store.h:28: };
The need for this struct suggests that we ought to un-nest the
DefaultOriginBoundCertStore::OriginBoundCert class. I intend to do this in a
subsequent CL.

[wtc, 2011-08-10 21:35:59]

mattm: just FYI.

rkn: the following are my review comments on Patch Set 2.

http://codereview.chromium.org/7585037/diff/4001/net/base/default_origin_boun...
File net/base/default_origin_bound_cert_store.cc (right):

http://codereview.chromium.org/7585037/diff/4001/net/base/default_origin_boun...
net/base/default_origin_bound_cert_store.cc:83: struct OriginBoundCertInfo
cert_info = {
Remove "struct".

We should be able to just do
  origin_bound_certs->push_back(*it->second);
You should add a TODO comment about this.

http://codereview.chromium.org/7585037/diff/4001/net/base/default_origin_boun...
File net/base/default_origin_bound_cert_store_unittest.cc (right):

http://codereview.chromium.org/7585037/diff/4001/net/base/default_origin_boun...
net/base/default_origin_bound_cert_store_unittest.cc:115: new
DefaultOriginBoundCertStore(persistent_store));
Consider using a local variable:
  DefaultOriginBoundCertStore store(persistent_store);

Can we do this?
  DefaultOriginBoundCertStore store(new MockPersistentStore);

I remember that's how you create the real origin-bound cert
stores in profile IO data, without using a scoped_refptr
for the persistent store.

http://codereview.chromium.org/7585037/diff/4001/net/base/origin_bound_cert_s...
File net/base/origin_bound_cert_store.h (right):

http://codereview.chromium.org/7585037/diff/4001/net/base/origin_bound_cert_s...
net/base/origin_bound_cert_store.h:9: #include <string>
We should include <vector>.

http://codereview.chromium.org/7585037/diff/4001/net/base/origin_bound_cert_s...
net/base/origin_bound_cert_store.h:28: };
Please document this struct.

http://codereview.chromium.org/7585037/diff/4001/net/base/origin_bound_cert_s...
net/base/origin_bound_cert_store.h:46: const std::string& cert) = 0;
Add "and the corresponding private key" to the comments for these methods.

The input arguments to this function are exactly the members
of the OriginBoundCertInfo structure, so this function can
also just take a const OriginBoundCertInfo& input argument.
But I'm not sure if that's a good idea.

http://codereview.chromium.org/7585037/diff/4001/net/base/origin_bound_cert_s...
net/base/origin_bound_cert_store.h:48: // Removes an origin bound cert from the
store.
The new methods probably should all return a success/failure
status.

[rkn, 2011-08-16 21:11:45]

wtc: Thanks for your comments. Please take another look.

http://codereview.chromium.org/7585037/diff/4001/net/base/default_origin_boun...
File net/base/default_origin_bound_cert_store_unittest.cc (right):

http://codereview.chromium.org/7585037/diff/4001/net/base/default_origin_boun...
net/base/default_origin_bound_cert_store_unittest.cc:115: new
DefaultOriginBoundCertStore(persistent_store));
On 2011/08/10 21:35:59, wtc wrote:
> Consider using a local variable:
>   DefaultOriginBoundCertStore store(persistent_store);
> 
> Can we do this?
>   DefaultOriginBoundCertStore store(new MockPersistentStore);
> 
> I remember that's how you create the real origin-bound cert
> stores in profile IO data, without using a scoped_refptr
> for the persistent store.

I know this isn't exactly the change you were suggesting, but
DefaultOriginBoundCertStore doesn't delete it's SQLiteOriginBoundCertStore, so I
think it makes sense to store it in a scoped_refptr.

[wtc, 2011-08-16 23:28:56]

LGTM.

Please update the Description of this CL to mention you
changed SetOriginBoundCert to return void.  Also, would be
nice to be more explicit about the new functions you added.

http://codereview.chromium.org/7585037/diff/26001/net/base/origin_bound_cert_...
File net/base/origin_bound_cert_store.h (right):

http://codereview.chromium.org/7585037/diff/26001/net/base/origin_bound_cert_...
net/base/origin_bound_cert_store.h:25: struct OriginBoundCertInfo {
Should we mention this is used by GetAllOriginBoundCerts?

http://codereview.chromium.org/7585037/diff/26001/net/base/origin_bound_cert_...
net/base/origin_bound_cert_store.h:26: std::string origin;  // Origin, for
instance "https://www.versign.com:8443".
Nit: "versign" is misspelled.  Use a typical HTTPS port (443)
or omit it.

[commit-bot: I haz the power, 2011-08-17 02:59:43]

Change committed as 97090