Last Comment Bug 324306 - No interface for allowing a user to manually specify which cert to log in with
: No interface for allowing a user to manually specify which cert to log in with
Status: RESOLVED FIXED
: fixed1.8.1, qawanted
Product: Camino Graveyard
Classification: Graveyard
Component: Security (show other bugs)
: unspecified
: PowerPC Mac OS X
-- normal (vote)
: Camino1.5
Assigned To: froodian (Ian Leue)
:
:
Mentors:
http://www.caminobrowser.org/support/...
Depends on:
Blocks: 324608
  Show dependency treegraph
 
Reported: 2006-01-21 22:08 PST by Smokey Ardisson (offline for a while; not following bugs - do not email)
Modified: 2006-08-15 08:33 PDT (History)
2 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Security.nib (4.64 KB, application/zip)
2006-06-29 13:51 PDT, froodian (Ian Leue)
no flags Details
Screenshot of Security.nib (67.08 KB, image/png)
2006-06-29 13:53 PDT, froodian (Ian Leue)
no flags Details
Patch (3.22 KB, patch)
2006-06-29 13:56 PDT, froodian (Ian Leue)
nick.kreeger: review-
Details | Diff | Splinter Review
Adresses kreeger's Comments (3.84 KB, patch)
2006-06-29 14:33 PDT, froodian (Ian Leue)
no flags Details | Diff | Splinter Review
Adresses kreeger's Comments (3.84 KB, patch)
2006-06-29 14:39 PDT, froodian (Ian Leue)
no flags Details | Diff | Splinter Review
Adresses kreeger's Comments (3.84 KB, patch)
2006-06-29 14:39 PDT, froodian (Ian Leue)
nick.kreeger: review+
Details | Diff | Splinter Review
Gets rid of weak ciphers (4.33 KB, patch)
2006-06-30 05:16 PDT, froodian (Ian Leue)
mikepinkerton: superreview+
Details | Diff | Splinter Review
Gets rid of the "uses low-grade encryption" checkbox (4.61 KB, application/zip)
2006-06-30 05:20 PDT, froodian (Ian Leue)
no flags Details
Minor IRC tweaks (6.45 KB, application/zip)
2006-06-30 22:49 PDT, froodian (Ian Leue)
no flags Details
Another nib (6.46 KB, application/zip)
2006-06-30 23:14 PDT, froodian (Ian Leue)
no flags Details
Better Text for the new pref (nib) (6.53 KB, application/zip)
2006-07-01 00:01 PDT, froodian (Ian Leue)
alqahira: review+
Details
Security prefPane as it stands now (on 10.3.9) (59.53 KB, image/png)
2006-07-01 00:15 PDT, Smokey Ardisson (offline for a while; not following bugs - do not email)
no flags Details
Adresses comment 23 (6.51 KB, application/zip)
2006-07-01 23:00 PDT, froodian (Ian Leue)
no flags Details
Screenshot of comment 23 nib (10.4, fwiw) (65.28 KB, image/png)
2006-07-01 23:01 PDT, froodian (Ian Leue)
no flags Details
Same design as "Adresses comment 23" (6.52 KB, application/zip)
2006-07-02 00:01 PDT, froodian (Ian Leue)
alqahira: review+
froodian: superreview+
Details

Description User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-01-21 22:08:08 PST
In reviewing/updating the support docs for 1.0, I came across the above reference to this.  smfr explained what it was on irc and we noted that despite the claims of the above-mentioned text that this was slated for post-1.0, there was no bug on this.

[12:32am] smfr: currently we'll automatically use the first personal cert
[12:32am] ardissone: so that text is mostly correct?
[12:33am] smfr: well, we don't offer you an interface to let you choose which certificate to use
[12:33am] smfr: but you will be able to log in if you have only one, or it chooses the appropriate one
[12:33am] ardissone: ok
[12:33am] ardissone: (is there a bug for fixing that? I didn't see anything that sounds like that)
[12:35am] smfr: no
Comment 1 User image Simon Fraser 2006-01-21 22:35:03 PST
Actually the UI is there, but the pref to make it show up is not. To get the cert picker, set "security.default_personal_cert" to "Ask Every Time" (yeah, gotta love those human readable pref values).
Comment 2 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-01-21 22:54:40 PST
So if I understand this correctly, I should modify the documentation text to add something like "if you have more than one personal certificate, set the pref to 'Ask Every Time' in order to be asked which certificate to use"?

(The revised text as it stands is "Unfortunately, Camino does not offer an interface for choosing which personal certificate to use when accessing a website that requires a personal certificate. Camino always uses the first personal certificate it finds.")

And this bug is now more about adding a prefs UI to enable the user to activate the security UI--or will simply having the options fully documented in the cb.o docs be enough?
Comment 3 User image Simon Fraser 2006-01-22 09:30:43 PST
This bug is really about adding radio buttons to the Security prefs panel to say:

When a web site requires a certificate:
  O  Select one automatically
  o  Ask me every time

If you want to mention hidden prefs on the documentation page, then by all means do so (or link to a "tips and tricks" page).
Comment 4 User image froodian (Ian Leue) 2006-06-29 12:09:03 PDT
/me loves prefpanes
Comment 5 User image froodian (Ian Leue) 2006-06-29 13:51:42 PDT
Created attachment 227600 [details]
Security.nib

This:
-Adds the radio matrix for this bug
-Fixes bug 324608
-Fixes the second issue in bug 325880 comment 0

Also, bug 324608 comment 1 is something we should think about some.

There's a potential problem here, which is that I couldn't trigger the dialog asking me to select a certificate.  It certainly sets the pref correctly (verified with about:config)... Is the underlying support for this broken, or was my browser just on crack?
Comment 6 User image froodian (Ian Leue) 2006-06-29 13:53:59 PDT
Created attachment 227601 [details]
Screenshot of Security.nib
Comment 7 User image froodian (Ian Leue) 2006-06-29 13:56:56 PDT
Created attachment 227602 [details] [diff] [review]
Patch

Does comment 3
Comment 8 User image Nick Kreeger 2006-06-29 14:09:30 PDT
Comment on attachment 227602 [details] [diff] [review]
Patch

+  if(gotPref){
+    if([certificateBehavior isEqual:@"Select Automatically"])
+      selectRow = 0;
+    else if([certificateBehavior isEqual:@"Ask Every Time"])
+      selectRow = 1;
+  }
+  [mCertificateBehavior selectCellAtRow:selectRow column:0];

Watch your spacing here, there should be once space like this:
if (gotPref) {
  ...

Perhaps you could set the row values as constants and then reference them in your if statement:
const unsigned int kAskEveryTimeMatrixRowValue = 0;
const unsigned int kSelectAutomaticallyMatrixRowValue = 1;
...
 [mCertificateBehavior selectCellAtRow:kAskEveryTimeRowValue column:0];
 [mCertificateBehavior selectCellAtRow: kSelectAutomaticallyMatrixRowValue column:0];

+-(IBAction)clickCertificateSelectionBehavior:(id)sender
+{
+  unsigned int row = [mCertificateBehavior selectedRow];
+
+  if(row == 0)
+    [self setPref:"security.default_personal_cert" toString:@"Select Automatically"];
+  else
+    [self setPref:"security.default_personal_cert" toString:@"Ask Every Time"];
+}

Setting the constants as mentioned above would work great here.

Lets give this another spin.
Comment 9 User image froodian (Ian Leue) 2006-06-29 14:33:30 PDT
Created attachment 227610 [details] [diff] [review]
Adresses kreeger's Comments
Comment 10 User image froodian (Ian Leue) 2006-06-29 14:39:40 PDT
Created attachment 227611 [details] [diff] [review]
Adresses kreeger's Comments
Comment 11 User image froodian (Ian Leue) 2006-06-29 14:39:40 PDT
Created attachment 227612 [details] [diff] [review]
Adresses kreeger's Comments
Comment 12 User image froodian (Ian Leue) 2006-06-29 14:40:46 PDT
Comment on attachment 227610 [details] [diff] [review]
Adresses kreeger's Comments

Grr.  My connection/bugzilla hicupped.  Sorry for the bugspam
Comment 13 User image Nick Kreeger 2006-06-29 14:42:21 PDT
Comment on attachment 227612 [details] [diff] [review]
Adresses kreeger's Comments

+const unsigned int kSelectAutomaticallyMatrixRowValue = 0;
+const unsigned int kAskEveryTimeMatrixRowValue  = 1;

Some spacing issues here, the committer can take care of these.

+    if ([certificateBehavior isEqual:@"Select Automatically"])
+      [mCertificateBehavior selectCellAtRow:kSelectAutomaticallyMatrixRowValue column:0];
+    else if ([certificateBehavior isEqual:@"Ask Every Time"])
+      [mCertificateBehavior selectCellAtRow:kAskEveryTimeMatrixRowValue column:0];

+  if (row == kSelectAutomaticallyMatrixRowValue)
+    [self setPref:"security.default_personal_cert" toString:@"Select Automatically"];
+  else // row == kAskEveryTimeMatrixRowValue
+    [self setPref:"security.default_personal_cert" toString:@"Ask Every Time"];


A space between the if and the else would be nice if the comitter wants to.

r=me
Comment 14 User image Simon Fraser 2006-06-29 21:03:02 PDT
> There's a potential problem here, which is that I couldn't trigger the dialog
> asking me to select a certificate.  It certainly sets the pref correctly
> (verified with about:config)... Is the underlying support for this broken, or
> was my browser just on crack?

I believe I was able to get this to happen many moons ago. It should hit SecurityDialogs::ChooseCertificate() which fires up a ChooseCertDialogController.
Comment 15 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-06-30 01:02:30 PDT
I suppose to test that, one needs 1) a site that requires a personal cert and 2) at least one, possibly more, personal certs.  I did some searching on *.mozilla.org (thinking there might be a testcase somewhere, perhaps for Litmus) without any luck.
Comment 16 User image froodian (Ian Leue) 2006-06-30 05:16:37 PDT
Created attachment 227682 [details] [diff] [review]
Gets rid of weak ciphers

Since they're no longer supported.  This just removes code, it doesn't add any, so still going w/ the kreeger r+.
Comment 17 User image froodian (Ian Leue) 2006-06-30 05:20:31 PDT
Created attachment 227683 [details]
Gets rid of the "uses low-grade encryption" checkbox

Other than that, it's the same, so not bothering with a png. Bug me if you want one.
Comment 18 User image froodian (Ian Leue) 2006-06-30 22:49:28 PDT
Created attachment 227795 [details]
Minor IRC tweaks
Comment 19 User image froodian (Ian Leue) 2006-06-30 23:14:49 PDT
Created attachment 227797 [details]
Another nib

Unfortunately there are some discrepancies between 10.3 IB and 10.4 IB.  Hopefully this will satisfy both.
Comment 20 User image froodian (Ian Leue) 2006-07-01 00:01:30 PDT
Created attachment 227800 [details]
Better Text for the new pref (nib)
Comment 21 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-07-01 00:14:31 PDT
Comment on attachment 227800 [details]
Better Text for the new pref (nib)

Sorry about all the nibspam; IB has lots of issues, and nib spacing seems to vary wildly between OS versions.

Simon, since you actually understand what all these do, can you make sure the new nib options and hint text say what they need to?  There wasn't room in the nib to do comment 3 verbatim....

"Final" screenshot to follow.
Comment 22 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-07-01 00:15:53 PDT
Created attachment 227801 [details]
Security prefPane as it stands now (on 10.3.9)
Comment 23 User image Simon Fraser 2006-07-01 10:35:55 PDT
I think it needs to be laid out like:


Certificates: When a site requires a certificate:
                        O Select one automatically
                        O Ask me which to use
                        
                        
              ( Certificates )
Comment 24 User image froodian (Ian Leue) 2006-07-01 23:00:26 PDT
Created attachment 227863 [details]
Adresses comment 23

This does essentially what smfr asks, but adds "personal."  I feel this is important to include somehow, since otherwise people who never get asked will either think that a) certificates aren't being used at all or b) the pref is broken.

Even with that though, the double-colon thing (cert: enable cert when:) here is kind of ugly.  I agree that there's no easy fix, but I personally prefer the previous incarnation to this one.
Comment 25 User image froodian (Ian Leue) 2006-07-01 23:01:01 PDT
Created attachment 227864 [details]
Screenshot of comment 23 nib (10.4, fwiw)
Comment 26 User image froodian (Ian Leue) 2006-07-02 00:01:25 PDT
Created attachment 227869 [details]
Same design as "Adresses comment 23"

Putting on the late-night IRC polish. :)
Comment 27 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-07-02 00:38:09 PDT
Comment on attachment 227869 [details]
Same design as "Adresses comment 23"

Yay for 10.3 vs. 10.4 display/layout differences.

Simon, this OK with you?

(FWIW, I tend to agree with Ian that the previous incarnation looks better, and indenting the matrix on this incarnation makes it look even more odd.)
Comment 28 User image Simon Fraser 2006-07-02 11:22:37 PDT
This one is easier to parse for the reader, I think, even if it may look a little odd. It's fine by me.
Comment 29 User image froodian (Ian Leue) 2006-07-30 11:03:59 PDT
So all in all, this bug:

- Exposes the security.default_personal_cert pref
- Removes "Ask Permission When" title, since we're really just showing a warning
- Removes the UI for weak ciphers warning, since it's deprecated
- Plays the overall prefpane polish game
Comment 30 User image Mike Pinkerton (not reading bugmail) 2006-08-02 07:43:45 PDT
Comment on attachment 227682 [details] [diff] [review]
Gets rid of weak ciphers

sr=pink
Comment 31 User image Mark Mentovai 2006-08-15 08:33:25 PDT
Checked in on the trunk and MOZILLA_1_8_BRANCH.

Note You need to log in before you can comment on or make changes to this bug.