Rename MacKeychain to AppleKeychain

Rename MacKeychain to AppleKeychain

Rename MacKeychain to AppleKeychain and add mac and iOS specific 
implementations (crypto/apple_keychain_ios.mm and 
crypto/apple_keychain_mac.mm). Rename MockKeychain to 
MockAppleKeychain and split its implementations in 3 files 
crypto/mock_apple_keychain.cc, crypto/mock_apple_keychain_ios.cc and 
crypto/mock_apple_keychain_mac.cc).


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

[msarda, 2012-08-23 12:54:32]

PTAL.

[Ryan Sleevi, 2012-08-23 22:22:58]

Naming is hard. I like crypto::mac::Keychain better because it clearly indicates
that it's "mac" only, and has parity with base::mac::, but I don't know whether
it's worth creating a new namespace (and new directory in crypto/) for. The
other bit of code for crypto::mac:: would be the CSSM init, but that's not a lot
either.

It's probably fine as-is, unless you have a better name to suggest.

Lots of nits below

https://chromiumcodereview.appspot.com/10875029/diff/1/crypto/apple_keychain.h
File crypto/apple_keychain.h (right):

https://chromiumcodereview.appspot.com/10875029/diff/1/crypto/apple_keychain....
crypto/apple_keychain.h:16: typedef void SecKeychainAttributeList;
typo: void -> void*

However, is this necessary, since you put everything within #if defined guards
(eg: line 55)? If not, I would rather this be dropped.

https://chromiumcodereview.appspot.com/10875029/diff/1/crypto/apple_keychain_...
File crypto/apple_keychain_ios.mm (right):

https://chromiumcodereview.appspot.com/10875029/diff/1/crypto/apple_keychain_...
crypto/apple_keychain_ios.mm:17: kKeychainActionUpdate
nit: indent two spaces.

https://chromiumcodereview.appspot.com/10875029/diff/1/crypto/apple_keychain_...
crypto/apple_keychain_ios.mm:27: &kCFTypeDictionaryKeyCallBacks,
&kCFTypeDictionaryValueCallBacks);
super minor tiny nit:

Because of the second parameter (capacity), I think it might be a little more
readable/obvious to wrap at the open paren

CFDictionaryCreateMutable(
    NULL, 5, &...);

Also line 60. It took me a few glances when quickly scanning to notice they were
different pre-allocated sizes.

https://chromiumcodereview.appspot.com/10875029/diff/1/crypto/apple_keychain_...
crypto/apple_keychain_ios.mm:105: free(data);
nit?
if (data) 
  free(data)

https://chromiumcodereview.appspot.com/10875029/diff/1/crypto/apple_keychain_...
crypto/apple_keychain_ios.mm:170: *passwordData = malloc(length);
nit: Should you make sure to set *passwordData = NULL when status != noErr ?

https://chromiumcodereview.appspot.com/10875029/diff/1/crypto/mock_apple_keyc...
File crypto/mock_apple_keychain.cc (right):

https://chromiumcodereview.appspot.com/10875029/diff/1/crypto/mock_apple_keyc...
crypto/mock_apple_keychain.cc:23: static char password[] = "my_password";
nit: static const char kPassword[] = "my_password";

https://chromiumcodereview.appspot.com/10875029/diff/1/crypto/mock_apple_keyc...
crypto/mock_apple_keychain.cc:28: *passwordLength = strlen(password);
arraysize

https://chromiumcodereview.appspot.com/10875029/diff/1/crypto/mock_apple_keyc...
crypto/mock_apple_keychain.cc:53: DCHECK(passwordLength > 0);
DCHECK_GT

https://chromiumcodereview.appspot.com/10875029/diff/1/crypto/mock_apple_keyc...
File crypto/mock_apple_keychain_ios.cc (right):

https://chromiumcodereview.appspot.com/10875029/diff/1/crypto/mock_apple_keyc...
crypto/mock_apple_keychain_ios.cc:14: password_data_count_(0) {}
nit: newline

password_data_count_(0) {
}

https://chromiumcodereview.appspot.com/10875029/diff/1/crypto/mock_apple_keyc...
File crypto/mock_apple_keychain_mac.cc (right):

https://chromiumcodereview.appspot.com/10875029/diff/1/crypto/mock_apple_keyc...
crypto/mock_apple_keychain_mac.cc:68: typedef std::map<MockKeychainItemType,
SecKeychainAttributeList> mockMap;
style nit: typedefs follow class naming rules

MockMap

And perhaps this should be a typedef that is part of the .h file, rather than
localized to this method, so that things are kept in sync.

See all other instances in this class.

[stuartmorgan, 2012-08-24 06:56:03]

https://chromiumcodereview.appspot.com/10875029/diff/1/crypto/apple_keychain_...
File crypto/apple_keychain_ios.mm (right):

https://chromiumcodereview.appspot.com/10875029/diff/1/crypto/apple_keychain_...
crypto/apple_keychain_ios.mm:105: free(data);
On 2012/08/23 22:22:58, Ryan Sleevi wrote:
> nit?
> if (data) 
>   free(data)
> 

free is NULL-safe.

https://chromiumcodereview.appspot.com/10875029/diff/1/crypto/apple_keychain_...
crypto/apple_keychain_ios.mm:170: *passwordData = malloc(length);
On 2012/08/23 22:22:58, Ryan Sleevi wrote:
> nit: Should you make sure to set *passwordData = NULL when status != noErr ?

The docs for the function this mimics doesn't say, so it couldn't hurt.

Actually, something I missed before: technically it's valid to pass NULL for
passwordLength and passwordData, if you don't want that data, so we should
handle that case (I doubt we'd ever do it, but we should not crash if someone
calls the function in a way the original function can safely be called).

[msarda, 2012-08-27 14:24:07]

PTAL.

https://chromiumcodereview.appspot.com/10875029/diff/1/crypto/apple_keychain.h
File crypto/apple_keychain.h (right):

https://chromiumcodereview.appspot.com/10875029/diff/1/crypto/apple_keychain....
crypto/apple_keychain.h:16: typedef void SecKeychainAttributeList;
This is not a typo. On mac, the SecKeychainAttributeList is a struct (not a
pointer to a struct). The method ItemFreeContent needs it.
I have searched the code and ItemFreeContent is called with the first element
NULL. I see 2 options here:
* Leave typedef void SecKeychainAttributeList
* Remove it and add a method like ItemFreeContent(void* data) for both iOS and
mac and a second on ItemFreeContent(SecKeychainAttributeList* attrList, void*
data) only for mac.
 
On 2012/08/23 22:22:58, Ryan Sleevi wrote:
> typo: void -> void*
> 
> However, is this necessary, since you put everything within #if defined guards
> (eg: line 55)? If not, I would rather this be dropped.

https://chromiumcodereview.appspot.com/10875029/diff/1/crypto/apple_keychain_...
File crypto/apple_keychain_ios.mm (right):

https://chromiumcodereview.appspot.com/10875029/diff/1/crypto/apple_keychain_...
crypto/apple_keychain_ios.mm:17: kKeychainActionUpdate
On 2012/08/23 22:22:58, Ryan Sleevi wrote:
> nit: indent two spaces.

Done.

https://chromiumcodereview.appspot.com/10875029/diff/1/crypto/apple_keychain_...
crypto/apple_keychain_ios.mm:27: &kCFTypeDictionaryKeyCallBacks,
&kCFTypeDictionaryValueCallBacks);
It does not fit (line has 81 characters). I have re-indented - let me know if
this is better.

On 2012/08/23 22:22:58, Ryan Sleevi wrote:
> super minor tiny nit:
> 
> Because of the second parameter (capacity), I think it might be a little more
> readable/obvious to wrap at the open paren
> 
> CFDictionaryCreateMutable(
>     NULL, 5, &...);
> 
> Also line 60. It took me a few glances when quickly scanning to notice they
were
> different pre-allocated sizes.

https://chromiumcodereview.appspot.com/10875029/diff/1/crypto/apple_keychain_...
crypto/apple_keychain_ios.mm:105: free(data);
Kept as is following on Stuart's comment.

On 2012/08/24 06:56:03, stuartmorgan wrote:
> On 2012/08/23 22:22:58, Ryan Sleevi wrote:
> > nit?
> > if (data) 
> >   free(data)
> > 
> 
> free is NULL-safe.

https://chromiumcodereview.appspot.com/10875029/diff/1/crypto/apple_keychain_...
crypto/apple_keychain_ios.mm:170: *passwordData = malloc(length);
On 2012/08/24 06:56:03, stuartmorgan wrote:
> On 2012/08/23 22:22:58, Ryan Sleevi wrote:
> > nit: Should you make sure to set *passwordData = NULL when status != noErr ?
> 
> The docs for the function this mimics doesn't say, so it couldn't hurt.
> 
> Actually, something I missed before: technically it's valid to pass NULL for
> passwordLength and passwordData, if you don't want that data, so we should
> handle that case (I doubt we'd ever do it, but we should not crash if someone
> calls the function in a way the original function can safely be called).

Done.

https://chromiumcodereview.appspot.com/10875029/diff/1/crypto/mock_apple_keyc...
File crypto/mock_apple_keychain.cc (right):

https://chromiumcodereview.appspot.com/10875029/diff/1/crypto/mock_apple_keyc...
crypto/mock_apple_keychain.cc:23: static char password[] = "my_password";
On 2012/08/23 22:22:58, Ryan Sleevi wrote:
> nit: static const char kPassword[] = "my_password";

Done.

https://chromiumcodereview.appspot.com/10875029/diff/1/crypto/mock_apple_keyc...
crypto/mock_apple_keychain.cc:28: *passwordLength = strlen(password);
On 2012/08/23 22:22:58, Ryan Sleevi wrote:
> arraysize

Done.

https://chromiumcodereview.appspot.com/10875029/diff/1/crypto/mock_apple_keyc...
crypto/mock_apple_keychain.cc:53: DCHECK(passwordLength > 0);
On 2012/08/23 22:22:58, Ryan Sleevi wrote:
> DCHECK_GT

Done.

https://chromiumcodereview.appspot.com/10875029/diff/1/crypto/mock_apple_keyc...
File crypto/mock_apple_keychain_ios.cc (right):

https://chromiumcodereview.appspot.com/10875029/diff/1/crypto/mock_apple_keyc...
crypto/mock_apple_keychain_ios.cc:14: password_data_count_(0) {}
On 2012/08/23 22:22:58, Ryan Sleevi wrote:
> nit: newline
> 
> password_data_count_(0) {
> }

Done.

https://chromiumcodereview.appspot.com/10875029/diff/1/crypto/mock_apple_keyc...
File crypto/mock_apple_keychain_mac.cc (right):

https://chromiumcodereview.appspot.com/10875029/diff/1/crypto/mock_apple_keyc...
crypto/mock_apple_keychain_mac.cc:68: typedef std::map<MockKeychainItemType,
SecKeychainAttributeList> mockMap;
On 2012/08/23 22:22:58, Ryan Sleevi wrote:
> style nit: typedefs follow class naming rules
> 
> MockMap
> 
> And perhaps this should be a typedef that is part of the .h file, rather than
> localized to this method, so that things are kept in sync.
> 
> See all other instances in this class.

Done.

[Ryan Sleevi, 2012-08-29 01:18:38]

Hi Mihai,

I realize some of these nits aren't your original code, but since we've got eyes
looking at it, I wanted to take the chance to call them out.

LGTM, with nits below:

http://codereview.chromium.org/10875029/diff/3003/crypto/apple_keychain_ios.mm
File crypto/apple_keychain_ios.mm (right):

http://codereview.chromium.org/10875029/diff/3003/crypto/apple_keychain_ios.m...
crypto/apple_keychain_ios.mm:74: if (action == kKeychainActionCreate) {
Can you short-circuit early here?

if (action != kKeychainActionCreate)
  return keychain_data;

... remainder here

http://codereview.chromium.org/10875029/diff/3003/crypto/apple_keychain_ios.m...
crypto/apple_keychain_ios.mm:175: if (status == noErr) {
nit: Perhaps re-order the error handling here so that you can early return.

if (status != noErr) {
  if (passwordData) {
    *passwordData = NULL;
    *passwordLength = 0;
  }
  return status;
}

if (passwordData) {
  ...
}

CFRelease(...)
return noErr;

http://codereview.chromium.org/10875029/diff/3003/crypto/mock_apple_keychain.h
File crypto/mock_apple_keychain.h (right):

http://codereview.chromium.org/10875029/diff/3003/crypto/mock_apple_keychain....
crypto/mock_apple_keychain.h:25: MockKeychainAttributesMap;
This is an implementation detail of the MockAppleKeychain, so it doesn't seem it
needs to be exposed in the crypto namespace (or even exposed publicly by the
MockAppleKeychain)

Unless I'm mistaken, MockKeychainItemType is only used by private methods of the
MockAppleKeychain, so it would seem that this typedef should also be a private
typedef in the MockAppleKeychain.

[msarda, 2012-08-29 11:58:27]

Thanks for the review.

https://chromiumcodereview.appspot.com/10875029/diff/3003/crypto/apple_keycha...
File crypto/apple_keychain_ios.mm (right):

https://chromiumcodereview.appspot.com/10875029/diff/3003/crypto/apple_keycha...
crypto/apple_keychain_ios.mm:74: if (action == kKeychainActionCreate) {
On 2012/08/29 01:18:38, Ryan Sleevi wrote:
> Can you short-circuit early here?
> 
> if (action != kKeychainActionCreate)
>   return keychain_data;
> 
> ... remainder here

Done.

https://chromiumcodereview.appspot.com/10875029/diff/3003/crypto/apple_keycha...
crypto/apple_keychain_ios.mm:175: if (status == noErr) {
On 2012/08/29 01:18:38, Ryan Sleevi wrote:
> nit: Perhaps re-order the error handling here so that you can early return.
> 
> if (status != noErr) {
>   if (passwordData) {
>     *passwordData = NULL;
>     *passwordLength = 0;
>   }
>   return status;
> }
> 
> if (passwordData) {
>   ...
> }
> 
> CFRelease(...)
> return noErr;

Done.

https://chromiumcodereview.appspot.com/10875029/diff/3003/crypto/mock_apple_k...
File crypto/mock_apple_keychain.h (right):

https://chromiumcodereview.appspot.com/10875029/diff/3003/crypto/mock_apple_k...
crypto/mock_apple_keychain.h:25: MockKeychainAttributesMap;
On 2012/08/29 01:18:38, Ryan Sleevi wrote:
> This is an implementation detail of the MockAppleKeychain, so it doesn't seem
it
> needs to be exposed in the crypto namespace (or even exposed publicly by the
> MockAppleKeychain)
> 
> Unless I'm mistaken, MockKeychainItemType is only used by private methods of
the
> MockAppleKeychain, so it would seem that this typedef should also be a private
> typedef in the MockAppleKeychain.

Done.

[commit-bot: I haz the power, 2012-08-29 11:58:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msarda@chromium.org/10875029/12022

[commit-bot: I haz the power, 2012-08-29 11:59:18]

Presubmit check for 10875029-12022 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/password_manager

Presubmit checks took 1.7s to calculate.

[commit-bot: I haz the power, 2012-08-29 12:49:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msarda@chromium.org/10875029/24001

[commit-bot: I haz the power, 2012-08-29 12:49:56]

Presubmit check for 10875029-24001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/password_manager

Presubmit checks took 1.8s to calculate.

[msarda, 2012-08-29 13:00:05]

@Stuart: It seems you are a owner of chrome/browser/password_manager .

Could you please take a look?

[stuartmorgan, 2012-08-29 13:16:37]

LGTM

[commit-bot: I haz the power, 2012-08-29 13:28:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msarda@chromium.org/10875029/24001

[commit-bot: I haz the power, 2012-08-29 17:38:26]

Try job failure for 10875029-24001 (retry) on win_rel for steps
"interactive_ui_tests, browser_tests".
It's a second try, previously, steps "interactive_ui_tests, browser_tests"
failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Avi (use Gerrit), 2012-08-29 17:56:21]

https://chromiumcodereview.appspot.com/10875029/diff/24001/chrome/browser/pas...
File chrome/browser/password_manager/password_store_mac_internal.h (right):

https://chromiumcodereview.appspot.com/10875029/diff/24001/chrome/browser/pas...
chrome/browser/password_manager/password_store_mac_internal.h:159: //
keychain_forms it's entries that weren't merged into at least one database
s/it's/its/

https://chromiumcodereview.appspot.com/10875029/diff/24001/crypto/apple_keych...
File crypto/apple_keychain_ios.mm (right):

https://chromiumcodereview.appspot.com/10875029/diff/24001/crypto/apple_keych...
crypto/apple_keychain_ios.mm:38: encoding:NSUTF8StringEncoding]);
align on colons

https://chromiumcodereview.appspot.com/10875029/diff/24001/crypto/apple_keych...
crypto/apple_keychain_ios.mm:39: CFDictionarySetValue(query, kSecAttrService,
service_name_ns);
When passing in an NSObject as a CFRef, a NSToCFCast would be appropriate.

https://chromiumcodereview.appspot.com/10875029/diff/24001/crypto/apple_keych...
crypto/apple_keychain_ios.mm:45: encoding:NSUTF8StringEncoding]);
align on colons

https://chromiumcodereview.appspot.com/10875029/diff/24001/crypto/apple_keych...
crypto/apple_keychain_ios.mm:46: CFDictionarySetValue(query, kSecAttrAccount,
account_name_ns);
NSToCFCast

https://chromiumcodereview.appspot.com/10875029/diff/24001/crypto/apple_keych...
crypto/apple_keychain_ios.mm:71: CFDictionarySetValue(keychain_data,
kSecValueData, password);
NSToCFCast

https://chromiumcodereview.appspot.com/10875029/diff/24001/crypto/apple_keych...
crypto/apple_keychain_ios.mm:89: encoding:NSUTF8StringEncoding]);
align on colons

https://chromiumcodereview.appspot.com/10875029/diff/24001/crypto/apple_keych...
crypto/apple_keychain_ios.mm:90: CFDictionarySetValue(keychain_data,
kSecAttrService, service_name_ns);
NSToCFCast

https://chromiumcodereview.appspot.com/10875029/diff/24001/crypto/apple_keych...
crypto/apple_keychain_ios.mm:97: CFDictionarySetValue(keychain_data,
kSecAttrAccount, account_name_ns);
NSToCFCast

https://chromiumcodereview.appspot.com/10875029/diff/24001/crypto/apple_keych...
crypto/apple_keychain_ios.mm:130: OSStatus status =
SecItemCopyMatching((CFDictionaryRef)query, NULL);
Rather than relying on a cast, just do query.get().

https://chromiumcodereview.appspot.com/10875029/diff/24001/crypto/apple_keych...
crypto/apple_keychain_ios.mm:185: NSData* data =
base::mac::CFToNSCast(base::mac::CFCast<CFDataRef>(result));
Doing this cast is a bit silly. Just use CFDataGetLength and
CFDataGetBytes(CFRangeMake(0, length)).

https://chromiumcodereview.appspot.com/10875029/diff/24001/crypto/apple_keych...
crypto/apple_keychain_ios.mm:191: CFRelease(result);
Can you capture the value into a ScopedCFRef so you don't need to do this?

[commit-bot: I haz the power, 2012-08-30 07:49:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msarda@chromium.org/10875029/24001

[msarda, 2012-08-30 09:43:02]

Address all your comments.

https://chromiumcodereview.appspot.com/10875029/diff/24001/chrome/browser/pas...
File chrome/browser/password_manager/password_store_mac_internal.h (right):

https://chromiumcodereview.appspot.com/10875029/diff/24001/chrome/browser/pas...
chrome/browser/password_manager/password_store_mac_internal.h:159: //
keychain_forms it's entries that weren't merged into at least one database
On 2012/08/29 17:56:21, Avi wrote:
> s/it's/its/

Done.

https://chromiumcodereview.appspot.com/10875029/diff/24001/crypto/apple_keych...
File crypto/apple_keychain_ios.mm (right):

https://chromiumcodereview.appspot.com/10875029/diff/24001/crypto/apple_keych...
crypto/apple_keychain_ios.mm:38: encoding:NSUTF8StringEncoding]);
On 2012/08/29 17:56:21, Avi wrote:
> align on colons

Done.

https://chromiumcodereview.appspot.com/10875029/diff/24001/crypto/apple_keych...
crypto/apple_keychain_ios.mm:39: CFDictionarySetValue(query, kSecAttrService,
service_name_ns);
On 2012/08/29 17:56:21, Avi wrote:
> When passing in an NSObject as a CFRef, a NSToCFCast would be appropriate.

Done.

https://chromiumcodereview.appspot.com/10875029/diff/24001/crypto/apple_keych...
crypto/apple_keychain_ios.mm:45: encoding:NSUTF8StringEncoding]);
On 2012/08/29 17:56:21, Avi wrote:
> align on colons

Done.

https://chromiumcodereview.appspot.com/10875029/diff/24001/crypto/apple_keych...
crypto/apple_keychain_ios.mm:46: CFDictionarySetValue(query, kSecAttrAccount,
account_name_ns);
On 2012/08/29 17:56:21, Avi wrote:
> NSToCFCast

Done.

https://chromiumcodereview.appspot.com/10875029/diff/24001/crypto/apple_keych...
crypto/apple_keychain_ios.mm:71: CFDictionarySetValue(keychain_data,
kSecValueData, password);
On 2012/08/29 17:56:21, Avi wrote:
> NSToCFCast

Done.

https://chromiumcodereview.appspot.com/10875029/diff/24001/crypto/apple_keych...
crypto/apple_keychain_ios.mm:89: encoding:NSUTF8StringEncoding]);
On 2012/08/29 17:56:21, Avi wrote:
> align on colons

Done.

https://chromiumcodereview.appspot.com/10875029/diff/24001/crypto/apple_keych...
crypto/apple_keychain_ios.mm:90: CFDictionarySetValue(keychain_data,
kSecAttrService, service_name_ns);
On 2012/08/29 17:56:21, Avi wrote:
> NSToCFCast

Done.

https://chromiumcodereview.appspot.com/10875029/diff/24001/crypto/apple_keych...
crypto/apple_keychain_ios.mm:97: CFDictionarySetValue(keychain_data,
kSecAttrAccount, account_name_ns);
On 2012/08/29 17:56:21, Avi wrote:
> NSToCFCast

Done.

https://chromiumcodereview.appspot.com/10875029/diff/24001/crypto/apple_keych...
crypto/apple_keychain_ios.mm:130: OSStatus status =
SecItemCopyMatching((CFDictionaryRef)query, NULL);
On 2012/08/29 17:56:21, Avi wrote:
> Rather than relying on a cast, just do query.get().

Done.

[commit-bot: I haz the power, 2012-08-30 09:47:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msarda@chromium.org/10875029/28002

[commit-bot: I haz the power, 2012-08-30 13:03:49]

Change committed as 154123