152485 - [RFE] Implement support for the Keychain Manager API

Status: VERIFIED FIXED

(Camino Graveyard :: OS Integration, enhancement)

Description

[Benoit]

I'm using few web sites with HTTP authentication and find it annoying to have to
fill my user/password over and over. I believe the Apple Keychain should be used
for this purpose. I've actually added support for storing password with the
keychain to Chimera. I wrote an implementation of the nsIAuthPromptWrapper and
plug it in using the signle-sign-on-prompt CID from the wallet service. I'll
attach a patch.

Note that it doesn't support storing passwords from HTML forms. I'm planning to
add this if you consider the original patch to be a good thing to be added to
chimera.

Let me know what you think of the patch!

Comment 1

[Benoit]

Attachment: Keychain implementation for nsIAuthPromptWrapper

Unatr the keychain.tar.gz in the chimera root directory. See README.KEYCHAIN
for instructions.

Comment 2

[Greg K.]

Benoit, the way I understand it, patches should be attached as a CVS diff.

Comment 3

[Greg K.]

Benoit, if you haven't already, have a look at [http://www.mozilla.org/hacking/]
for information on getting your patch reviewed and checked in.

(My impression is that this is an excellent and preferable alternative to
incorporating Mozilla's own Password Manager.)

Comment 4

[Greg K.]

Pinkerton in bug 149161: "use diff -u2 <file> to create context diffs."

Comment 5

[Greg K.]

Per Pinkerton:

     make that:
     cvs diff -u2 > file

Comment 6

[Greg K.]

See also bug 106400.

Comment 7

[Benoit]

Attachment: patch for nsCocoaBrowserService.mm

Sorry, I thought a tar.gz would be more convenient since there's 4 to 5 new
files. I'll atach the files separitly along with the patch from diff -u2.

I already had a look at patch 106400 and the code from the wallet service. I'm
not sure it's  a good idea for Chimera to use the wallet service. This would
add another component which duplicates some functionality that	is already
available with MacOS X. There's some kind of support added for keychain in the
wallet service, I'm not sure how much work would be needed to get this work and
if it's really a good idea to use this for chimera. 

In short,  I think it's better for Chimera to directly use the Keychain
service, it just needs to store/get passwords from the keychain when needed. No
need to worry about managing passwords, having a master password, preferences,
... it's all taken care of by MacOS.

Comment 8

[Benoit]

Attachment: nsIPromptAuthWrapper implementation

Comment 9

[Benoit]

Attachment: nsIAuthPromptWrapper implementation

Comment 10

[Benoit]

Attachment: Small wrapper for Keychain API

Comment 11

[Benoit]

Attachment: Small wrapper for Keychain API

Comment 12

[Benoit]

Attachment: String for checkbox title

I wasn't sure where to add this string, any recommendations? 

Add the nsKeychain*.mm|h files to your project along with the alert.strings
file. Patch the nsCocoaBrowserService.mm file and that should be it! Let me
know if this this is still not the right way to go (and appologize if it's
still not :-))

Comment 13

[saari (gone)]

->pinkerton, a patch we'll probably want to consider in a couple weeks

Comment 14

[Mike Pinkerton (not reading bugmail)]

sweeeeeeeeeeeeeeet. i'll take a look.

Comment 15

[Benoit]

Attachment: Register listener with CHBrowserWrapper

New and better implementation

Comment 16

[Benoit]

Attachment: Replaced the old nsKeychain* files with the unique KeychainService file

Comment 17

[Benoit]

Attachment: New and improved implementation

Hi,

Everything's in the KeychainService.mm source file now. It now handles updating
username and passwords. 

It also handles HTML prefill of HTML signin forms and storing passwords from
HTML signin forms. It's a little simpler than the wallet implementation. We
only handle forms with a text field and exactly one password field (it ignores
forms with 2/3 passwords).

Things that it doesn't do yet: offer preferences to disable HTML form auto
filling, or storing password from HTML forms. It also doesn't ask the user
whether or not if wants the password to be stored in the keychain, it probably
should for HTML signin forms. I'm planning to do that soon...

Oh and BTW, you need to add:

  /* for Keychain prompt */
  "KEYCHAIN_CHECK_TITLE" = "Store password in Keychain";

in Localizable.strings (couldn't find how to make a patch for unicode text
files...).

Comment 18

[Winnie Lam]

*** Bug 160734 has been marked as a duplicate of this bug. ***

Comment 19

[Benoit]

Attachment: Support for keychain manager Impl.

I'm attaching a tar.gz containing a bunch of files and 2 updated .nib files.
Let me know if you want each file as an attachement (around 10 files). I've
improved few things, it now asks the user whether or not password in sign-in
HTML forms should be stored in the keychain and add two preferences option (to
disable the keychain, or just the auto-fill functionality). Here's the detail
of all my changes to the current sources and how to get it into the project:

Untar everything in the chimera directory. The tar file contains the following
files:

 keychain.patch
 English.lproj/Localizable.strings
 alert.nib/classes.nib
 alert.nib/info.nib
 alert.nib/objects.nib
 PreferencePanes/Privacy/Privacy.nib/classes.nib
 PreferencePanes/Privacy/Privacy.nib/info.nib
 PreferencePanes/Privacy/Privacy.nib/objects.nib
 KeychainService.h
 KeychainService.mm

Alert.nib contains a new dialog to ask the user whether or not he
wants to store a password from an HTML sign-in form in the Keychain.

Privacy.nib contains two more checkbox to allow the user to disable
the keychain feature and if the key feature is enabled to only disable
the auto-fill functionality (one might want to do this for
performance, auto-fill requires adding a listener and checking for
each new loaded web page if there's form elements, there's probably
some room for optimizations here though).

Localizable.strings contains one more string which is displayed as the
title of the checkbox in the authentification prompts.

The patch adds the following:

 * MainController: manage the Keychain service, alloc the keychain
   service when it's loaded and release it when it's deallocated.
 
 * CHBrowserWrapper: it adds a new listener for each new browser
   view. The listener auto-fills sign-in prompts with values from the
   keychain.

 * nsAlertController: add support for the the confirmStorePassword
   prompt used to ask the user if he wants to store a password from an
   HTML sign-in form in the Keychain.

 * PrivacyPane.mm: support for the two checkbox and preference. The
   two new preferences are:

      chimera.store_passwords_with_keychain
      chimera.keychain_passwords_autofill
 
   The second is only taken into account if the first one's true.

Just add the KeychainService.* files to your project (I've put them in
the Application folder) and patch the sources with:

  patch -p0 < keychain.patch

Hopefully this should compile!	I've tested it with the tree as of
08/04/2002. 

Let me know if there's any problems! Also, let me know if this doesn't really
match Chimera feature requirements or if anything needs to be changed!

Comment 20

[Benoit]

Attachment: Fix minor memory mgmt bug

Please use this file instead of the old one or remove line 558 in
KeychainService.mm ([password release]). Embarrasing memory management bug!

Comment 21

[Tuukka Tolvanen (sp3000)]

*** Bug 162199 has been marked as a duplicate of this bug. ***

Comment 22

[Neil Lee [:neilio]]

Is there a roadmap for when this will be compiled into the nightlies?

Comment 23

[Benoit]

Attachment: new source tree, new patch

New tar file with nib, patch and new source files for keychain support, there's
a chimera/README.KEYCHAIN file that explains how to get this into the tree.
Tested with the tree as of 08/29.

Comment 24

[Mike Pinkerton (not reading bugmail)]

looking at the patch now (finally). thanks for keeping it current. ;)

Comment 25

[Mike Pinkerton (not reading bugmail)]

Attachment: my version of patch w/out nib changes

new version of the patch (just the src changes).

Comment 26

[Simon Fraser [no longer active]]

Comments:

+int UseKeychainChangedCallback(const char* pref, void* data)
+{
+  if ( data ) {
+    KeychainService* self = NS_REINTERPRET_CAST(KeychainService*, data);
+    self->mPrefService->GetBoolPref(gUseKeychainPref, &self->mIsEnabled);
+  }
+  return NS_OK;
+}

This worries me. Because 'KeychainService' is Obj-C, not ref-counted C++, it
seems that we can't be sure that 'KeychainService' will always exist when this
callback is fired. Maybe a C++ intermediate object would be better.

++ (KeychainService*) instance
+{
+  return sInstance ? sInstance : [[self alloc] init];
+}

This is weird. Why doesn't this assign into 'sInstance' if it does not already
exist?

+  nsCOMPtr<nsIComponentRegistrar> cr;
+  NS_GetComponentRegistrar(getter_AddRefs(cr));
+  NS_ASSERTION(cr, "Can't get the component registrar");

Why these lines? 'cr' is unused.

+  mFormSubmitObserver = new KeychainFormSubmitObserver(self);
+  svc->AddObserver(mFormSubmitObserver, NS_FORMSUBMIT_SUBJECT, PR_FALSE);

Don't we want to hold a ref? nsIObserverService will use weak refs when it can
(so maybe it should here).

+  // cache the pref service so we're not asking for it over and over again.
+  nsCOMPtr<nsIPrefBranch> pref(do_GetService(NS_PREF_CONTRACTID));
+  mPrefService = pref.get();

Should go via PreferenceManager for prefs.

+- (BOOL) getUsernameAndPassword:(NSString*)realm
user:(NSMutableString*)username password:(NSMutableString*)pwd
...
+    return false;

Should use YES and NO with BOOL.

+  if(kcfindinternetpassword([realm cString], 0, 0, kAnyPort,
kKCProtocolTypeHTTP, kKCAuthTypeHTTPDigest, 

Space after the 'if' please. And should we be passing anything in the
'*accountName' param here?

+  [pwd setString:[NSString stringWithCString:buffer length:actualSize]];

Can we guarantee that passwords are always single-byte strings? Should this use
UTF-8? I seem to recall that [NSString cString] will be deprecated at some point.

+- (id)initWithBrowser:(KeychainService*)keychain browser:(CHBrowserView*)aBrowser
+{
+  [super init];
+  mKeychain = keychain;
+  mBrowserView = aBrowser;
+  return self;
+}

Bad init method. it needs to be if ((self = [super init])) ...

+      if(isText && !*outUsername) {
+        *outUsername = inputElement;
+        NS_ADDREF(*outUsername);
+      }
+      else if (isPassword && !*outPassword) {
+        *outPassword = inputElement;
+        NS_ADDREF(*outPassword);
+      }

Don't you want to stop after finding the first pair of username/password fields?

+}
\ No newline at end of file

Fix this.

Comment 27

[Mike Pinkerton (not reading bugmail)]

Attachment: updated patch

updated to address smfr's comments. also fixes a crasher with ftp login prompts
i didn't catch before.

i couldn't find anything in the docs about the encoding of the keychain data
besides it either being a cstring or pstring. I think we can tackle that later
if need be.

Comment 28

[Simon Fraser [no longer active]]

+        pref->RegisterCallback(gUseKeychainPref, KeychainPrefChangedCallback,
nsnull);
+        pref->RegisterCallback(gAutoFillEnabledPref,
KeychainPrefChangedCallback, nsnull);

Hrm, I wonder if we should wrap pref callbacks into PreferenceManager, and maybe
translate them into cocoa NSNotifications? But not for this patch.

+    mFormSubmitObserver = new KeychainFormSubmitObserver(self);
+    if ( mFormSubmitObserver && svc ) {
+      svc->AddObserver(mFormSubmitObserver, NS_FORMSUBMIT_SUBJECT, PR_FALSE);
+      NS_ADDREF(mFormSubmitObserver);

You should addref before calling AddObserver, because a QI inside of the
AddObserver call may take your refcount back down to 0.

Otherwise, r=sfraser

Comment 29

[Benoit]

FYI, Apple deprecated the Keychain manager API and replaced it with Keychain
Service API:
 http://developer.apple.com/techpubs/macosx/CoreTechnologies/securityservices/keychainservices/keychainservices.html

I guess we'll need to port this patch to this new API at one point, this
shouldn't be very difficult and would probably cleanup some nasty string
convertions (it's a C API, no more pascal stuff I think).

Comment 30

[Mike Pinkerton (not reading bugmail)]

fixed. let's tackle the new api in a different bug (patches always welcome). i
need to get this out of my tree.

Comment 31

[Chris Petersen]

Verified in the 2002-09-17-05 NB. Checked with various webmail sites like .mac
and AOL mail.

Comment 32

[Thomas F. O'Connell]

has anyone filed an RFE on Keychain Service, yet? i couldn't find one.