Closed Bug 335635 Opened 18 years ago Closed 16 years ago

Improve UI for certificate import/restore

Categories

(Camino Graveyard :: Security, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: alqahira, Assigned: stuart.morgan+bugzilla)

References

Details

Attachments

(3 files, 1 obsolete file)

From bug 335393:

Simon's got it.  You add your own certificates in PKCS#12 format to the
database with Restore, not Import.  This works.

Having options available for both Restore and Import seems a little dubious to
me.  Firefox calls the Restore (of PKCS#12) of personal certificates "Import" -
the Import button performs a PKCS#12 restore to the personal certificate store
when clicked in the "Your Certificates" tab.  "Import" imports (non-personal)
certificates when clicked in any other tab, and the "Backup" buttons aren't
available in those cases either.  I think that the Firefox UI is a little bit
more intuitive, and maybe the Camino action menu should behave similarly. 
Someone can file a bug on that.

---

IOW, have one "Import" option that just does the right thing automatically depending on what type of certificate the user selects?
Mass un-setting milestone per 1.6 roadmap.

Filter on RemoveRedonkulousBuglist to remove bugspam.

Developers: if you have a patch in hand for one of these bugs, you may pull the bug back to 1.6 *at that point*.
Target Milestone: Camino1.6 → ---
In the meantime, would it be correct to rename "Restore…" to "Import Personal Certificate…" and "Import…" to something like "Import Website Certificate…" ?

That is, is the codepath behind "Restore…" only used for importing personal certificates and the codepath behind "Import…" only used for importing website certificates?

The UI would be a bit silly, yes, but this would be a simple step we could take to actually make the UI more clear to our users without needing to devote coding resources to fixing it "the right way".  

If those assumptions are correct, I'd take this for b1.
In bug 329081 smfr says "Restore is intended to be used with a file produced by Backup. I forget if it replaces existing certs with those read in."

As far as I can tell, we don't support "Back Up" on the Websites and Authorities, and code inspection leads me to believe that backups are always written as .p12 files 

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/camino/src/security/CertificatesWindowController.mm&rev=1.14&mark=772-775#772

so the assumptions I made before seem reasonable (with the exception of not knowing anything about what the "Other People’s" section is).
Attached patch fix?Splinter Review
With the caveat that I don't actually know anything about the various cert types (either file type or our UI distinctions), so I'm not sure what the ideal behavior is, this should work. It gives us one import item that should do the right thing based on file type.

I get the feeling that maybe we should be allowing different types based on the group we are in, rather than always allowing them all--but when I look at the UI in current builds, I always have Restore and Import as options in the action menu (although not the context menu--why are those different?) in every group, so this is not really any worse than the current situation and hopefully less confusing.

I haven't actually tested this, since I don't know where to get the various kinds of certs. Smokey, can you try this out?
Attachment #349653 - Flags: review?(alqahira)
Attached file corresponding nib (obsolete) —
I think may have zipped the first nib before saving the last time.
Attachment #349654 - Attachment is obsolete: true
Comment on attachment 349653 [details] [diff] [review]
fix?

> - (IBAction)importCertificates:(id)sender
> {
>   NSOpenPanel* theOpenPanel = [NSOpenPanel openPanel];
>   
>   [theOpenPanel beginSheetForDirectory:nil
>                                   file:nil
>-                                 types:[NSArray arrayWithObjects:@"crt", @"cert", @"cer", @"pem", @"der", @"p7b", @"pkcs7", nil]
>+                                 types:[NSArray arrayWithObjects:
>+                                         @"crt", @"cert", @"cer", @"pem", @"der", @"p7b", @"pkcs7",
>+                                         @"p12", @"pfx", @"pkcs12",
>+                                         nil]
>                         modalForWindow:[self window]
>                          modalDelegate:self
>                         didEndSelector:@selector(importPanelDidEnd:returnCode:contextInfo:)
>                            contextInfo:NULL];

Can you add a comment about these two arrays, since we no longer have the distinction in code that could help us figure out which is which (website/CA vs. personal)?  Also, add a comment about the need to keep the "personal" array in sync with the extension analysis below where the proper action is chosen?

>+  if (certDB) {
>+    NSString* extension = [inFilePath pathExtension];
>+    nsresult rv;
>+    if ([extension caseInsensitiveCompare:@"p12"] == NSOrderedSame ||
>+        [extension caseInsensitiveCompare:@"pfx"] == NSOrderedSame ||
>+        [extension caseInsensitiveCompare:@"pkcs12"] == NSOrderedSame)
>+    {
>+      rv = certDB->ImportPKCS12File(nsnull /* any token */, destFile);
>+    }

Otherwise, this works as expected within the confines of my ability to test, and within the confines of our existing buggy behavior. r=ardissone with those comments.

We also need to file a follow-up on having the right menus/import behaviors correspond to the selected Category, since we get different behaviors/errors depending on which Category is (erroneously) selected, but that's existing bugginess.
Attachment #349653 - Flags: review?(alqahira) → review+
Assignee: nobody → stuart.morgan+bugzilla
With comments.

To summarize: we currently have two menu items for importing two different kinds of certs, and users can't figure out how to import personal certs because they aren't expecting the action to be called Restore (we've had feedback/forum posts/bugs). This patch combines them into one menu item that calls the appropriate code based on file extension.

From Smokey's testing it looks like what we really need is to change the types of certs you can import based on what group you are in in the UI, but this patch would be the first step toward that, it addresses the problem users have been running into, and it doesn't actually allow any incorrect behavior that wasn't already possible, so I'd like to start with this and let someone who understands certs do the rest later.
Attachment #349661 - Flags: superreview?(mikepinkerton)
Comment on attachment 349661 [details] [diff] [review]
incremental improvement, v2 [landed]

sr=pink
Attachment #349661 - Flags: superreview?(mikepinkerton) → superreview+
Comment on attachment 349661 [details] [diff] [review]
incremental improvement, v2 [landed]

Landed on CVS trunk. I'll leave this open for now; Smokey, if you'd rather file a new bug for the rest feel free to close this.
Attachment #349661 - Attachment description: incremental improvement, v2 → incremental improvement, v2 [landed]
Assignee: stuart.morgan+bugzilla → nobody
We landed a fix that fixed the main issue that spawned this bug, so I'd rather call this FIXED and spin a new bug with a bit more specifics on the remaining part (bug 466596).
Assignee: nobody → stuart.morgan+bugzilla
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: