Closed
Bug 152485
Opened 22 years ago
Closed 22 years ago
[RFE] Implement support for the Keychain Manager API
Categories
(Camino Graveyard :: OS Integration, enhancement)
Tracking
(Not tracked)
VERIFIED
FIXED
Chimera0.6
People
(Reporter: bentoi, Assigned: mikepinkerton)
References
()
Details
Attachments
(2 files, 13 obsolete files)
19.33 KB,
application/x-gzip
|
Details | |
39.42 KB,
patch
|
Details | Diff | Splinter Review |
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!
Unatr the keychain.tar.gz in the chimera root directory. See README.KEYCHAIN for instructions.
Benoit, the way I understand it, patches should be attached as a CVS diff.
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.)
Severity: normal → enhancement
Pinkerton in bug 149161: "use diff -u2 <file> to create context diffs."
Summary: [RFE] Storing password using the keychain → Implement support for the Keychain Manager API
See also bug 106400.
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.
Reporter | ||
Comment 10•22 years ago
|
||
Reporter | ||
Comment 11•22 years ago
|
||
Reporter | ||
Comment 12•22 years ago
|
||
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•22 years ago
|
||
->pinkerton, a patch we'll probably want to consider in a couple weeks
Assignee: saari → pinkerton
Updated•22 years ago
|
Summary: Implement support for the Keychain Manager API → [RFE] Implement support for the Keychain Manager API
Assignee | ||
Comment 14•22 years ago
|
||
sweeeeeeeeeeeeeeet. i'll take a look.
Status: NEW → ASSIGNED
Target Milestone: --- → Chimera0.4
Reporter | ||
Comment 15•22 years ago
|
||
New and better implementation
Attachment #88062 -
Attachment is obsolete: true
Attachment #88247 -
Attachment is obsolete: true
Attachment #88249 -
Attachment is obsolete: true
Attachment #88250 -
Attachment is obsolete: true
Attachment #88251 -
Attachment is obsolete: true
Attachment #88253 -
Attachment is obsolete: true
Reporter | ||
Comment 16•22 years ago
|
||
Reporter | ||
Comment 17•22 years ago
|
||
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...).
Attachment #88254 -
Attachment is obsolete: true
Comment 18•22 years ago
|
||
*** Bug 160734 has been marked as a duplicate of this bug. ***
Reporter | ||
Comment 19•22 years ago
|
||
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!
Attachment #90736 -
Attachment is obsolete: true
Attachment #90737 -
Attachment is obsolete: true
Attachment #90739 -
Attachment is obsolete: true
Reporter | ||
Comment 20•22 years ago
|
||
Please use this file instead of the old one or remove line 558 in KeychainService.mm ([password release]). Embarrasing memory management bug!
Attachment #93923 -
Attachment is obsolete: true
Comment 21•22 years ago
|
||
*** Bug 162199 has been marked as a duplicate of this bug. ***
Comment 22•22 years ago
|
||
Is there a roadmap for when this will be compiled into the nightlies?
Updated•22 years ago
|
Component: General → OS Integration
QA Contact: winnie → petersen
Assignee | ||
Updated•22 years ago
|
Target Milestone: Chimera0.4 → Chimera0.6
Reporter | ||
Comment 23•22 years ago
|
||
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.
Attachment #94026 -
Attachment is obsolete: true
Assignee | ||
Comment 24•22 years ago
|
||
looking at the patch now (finally). thanks for keeping it current. ;)
Assignee | ||
Comment 25•22 years ago
|
||
new version of the patch (just the src changes).
Comment 26•22 years ago
|
||
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.
Assignee | ||
Comment 27•22 years ago
|
||
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.
Attachment #98633 -
Attachment is obsolete: true
Comment 28•22 years ago
|
||
+ 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
Reporter | ||
Comment 29•22 years ago
|
||
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).
Assignee | ||
Comment 30•22 years ago
|
||
fixed. let's tackle the new api in a different bug (patches always welcome). i need to get this out of my tree.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 31•22 years ago
|
||
Verified in the 2002-09-17-05 NB. Checked with various webmail sites like .mac and AOL mail.
Status: RESOLVED → VERIFIED
Comment 32•22 years ago
|
||
has anyone filed an RFE on Keychain Service, yet? i couldn't find one.
You need to log in
before you can comment on or make changes to this bug.
Description
•