453076 - Add UI to see and remove certificate exceptions.

Status: RESOLVED FIXED

(Camino Graveyard :: Security, defect)

Description

[Stuart Morgan]

Bug 398417 will add the ability to create exceptions for invalid certificates, so we'll need UI in the Certificates window to list and delete those exceptions.

(In the mean time, for testing related to cert exceptions, they can be managed by editing the plain-text 'cert_override.txt' file in the profile.)

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Not going to block b1 on this (because of bug 467317).

Comment 2

[Stuart Morgan]

Removing dependency, since we can (and are going to have to) hack around it.

Comment 3

[Stuart Morgan]

Attachment: fix

Adds a sheet to the Security pane for displaying and removing cert overrides. Adds an nsICertOverrideService wrapper to give the pref pane access to it without using Gecko, and switches the existing cert override stuff to use it. (Eventually the recent-bad-cert stuff could go in there as well, but I didn't feel like dealing with that at the moment.)

Comment 4

[Stuart Morgan]

Attachment: corresponding nib

Comment 5

[Christopher Henderson]

Comment on attachment 371166 [details] [diff] [review]
fix

Behaviour is pretty good overall. I can still add exceptions just fine.

The XCode project has been changed by Breakpad checkins, so this patch applies with some offsetting and fuzzing.

The Remove All item has ellipses, so I would expect a confirmation dialog to be shown. This happens with other Remove All… commands, for example in the Keychain Exclusions sheet.

The Remove and Remove All… items should be conditionally enabled, depending on whether there are table items they can remove. This can be done in IB using bindings.

>+#pragma mark Cookie Permissions Sheet

This is the wrong label for this section.

>+  return [[[[self class] alloc] init] autorelease];

You can use self instead of [self class] here. Reference: http://lists.apple.com/archives/Objc-language/2006/Jan/msg00001.html

Code looks good otherwise.

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Out of curiosity, why did you not just add this to the Certificates window (like Firefox does) instead of making an additional sheet?

Comment 7

[Stuart Morgan]

a) Shimming it into our existing UI looked like it was going to be more work (e.g., we play games with re-using the same table, and we don't have 4 columns in this case).
b) It's more consistent with our other exceptions.
c) The certificates view is already confusing, and I thought that stuffing another slightly different concept wasn't going to help anyone.

Comment 8

[Stuart Morgan]

Attachment: v2

Addresses review comments, thanks for catching those!

I also realized as I walked though with fresh eyes that I was using our internal language ("certificate overrides") in the UI, even though they are called "security exceptions" in all the UI for creating them, so I've adjusted all the user-visible strings I'm adding to use the exception terminology.

Comment 9

[Stuart Morgan]

Attachment: nib v2

Comment 10

[Christopher Henderson]

Comment on attachment 375568 [details] [diff] [review]
v2

You forgot CHCertificateOverrideManager.* :-(.

Comment 11

[Christopher Henderson]

Comment on attachment 375568 [details] [diff] [review]
v2

Actually, you probably haven't changed those files between versions. If you restore them and s/chose/chosen in the RemoveAllSecurityExceptionsWarning string, you'll get an r+ from me.

Comment 12

[Stuart Morgan]

Attachment: v3

With typo fix and missing files

Comment 13

[Mike Pinkerton (not reading bugmail)]

+    if ([components count] != 2)
+      continue;

what's special about 2? Just a comment would help about what you're parsing for.

+- (IBAction)editOverrides:(id)aSender

Would be nice to have function-level comments on all the new methods added here.

sr=pink with those addressed.

Comment 14

[Stuart Morgan]

Landed on CVS trunk with added comments.