No interface for allowing a user to manually specify which cert to log in with

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
Security
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Smokey Ardisson (offline for a while; not following bugs - do not email), Assigned: froodian (Ian Leue))

Tracking

({fixed1.8.1, qawanted})

unspecified
Camino1.5
PowerPC
Mac OS X
fixed1.8.1, qawanted

Details

(URL)

Attachments

(3 attachments, 12 obsolete attachments)

4.33 KB, patch
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
65.28 KB, image/png
Details
6.52 KB, application/zip
Smokey Ardisson (offline for a while; not following bugs - do not email)
: review+
froodian (Ian Leue)
: superreview+
Details
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

12 years ago
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).
Summary: no interface for allowing a user to choose which personal certificate to use → No interface for allowing a user to manually specify which cert to log in with
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

12 years ago
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).
(Assignee)

Comment 4

11 years ago
/me loves prefpanes
Assignee: mikepinkerton → stridey
Target Milestone: --- → Camino1.1
(Assignee)

Updated

11 years ago
Blocks: 324608
(Assignee)

Comment 5

11 years ago
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?
Attachment #227600 - Flags: review?(alqahira)
(Assignee)

Comment 6

11 years ago
Created attachment 227601 [details]
Screenshot of Security.nib
(Assignee)

Comment 7

11 years ago
Created attachment 227602 [details] [diff] [review]
Patch

Does comment 3
Attachment #227602 - Flags: review?(nick.kreeger)
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED

Comment 8

11 years ago
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.
Attachment #227602 - Flags: review?(nick.kreeger) → review-
(Assignee)

Comment 9

11 years ago
Created attachment 227610 [details] [diff] [review]
Adresses kreeger's Comments
Attachment #227600 - Attachment is obsolete: true
Attachment #227610 - Flags: review?(nick.kreeger)
Attachment #227600 - Flags: review?(alqahira)
(Assignee)

Comment 10

11 years ago
Created attachment 227611 [details] [diff] [review]
Adresses kreeger's Comments
Attachment #227600 - Attachment is obsolete: true
Attachment #227602 - Attachment is obsolete: true
Attachment #227611 - Flags: review?(nick.kreeger)
Attachment #227612 - Flags: review?(nick.kreeger)
(Assignee)

Comment 11

11 years ago
Created attachment 227612 [details] [diff] [review]
Adresses kreeger's Comments
(Assignee)

Comment 12

11 years ago
Comment on attachment 227610 [details] [diff] [review]
Adresses kreeger's Comments

Grr.  My connection/bugzilla hicupped.  Sorry for the bugspam
Attachment #227610 - Attachment is obsolete: true
Attachment #227610 - Flags: review?(nick.kreeger)
(Assignee)

Updated

11 years ago
Attachment #227611 - Attachment is obsolete: true
Attachment #227611 - Flags: review?(nick.kreeger)
(Assignee)

Updated

11 years ago
Attachment #227600 - Attachment is obsolete: false
Attachment #227600 - Flags: review?(alqahira)

Comment 13

11 years ago
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
Attachment #227612 - Flags: review?(nick.kreeger) → review+
(Assignee)

Updated

11 years ago
Attachment #227612 - Flags: review?(mozilla)

Comment 14

11 years ago
> 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.
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.
(Assignee)

Comment 16

11 years ago
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+.
Attachment #227612 - Attachment is obsolete: true
Attachment #227682 - Flags: review?(mozilla)
Attachment #227612 - Flags: review?(mozilla)
(Assignee)

Comment 17

11 years ago
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.
Attachment #227600 - Attachment is obsolete: true
Attachment #227683 - Flags: review?(alqahira)
Attachment #227600 - Flags: review?(alqahira)
(Assignee)

Updated

11 years ago
Keywords: qawanted
(Assignee)

Comment 18

11 years ago
Created attachment 227795 [details]
Minor IRC tweaks
Attachment #227601 - Attachment is obsolete: true
Attachment #227683 - Attachment is obsolete: true
Attachment #227795 - Flags: review?(alqahira)
Attachment #227683 - Flags: review?(alqahira)
(Assignee)

Comment 19

11 years ago
Created attachment 227797 [details]
Another nib

Unfortunately there are some discrepancies between 10.3 IB and 10.4 IB.  Hopefully this will satisfy both.
Attachment #227795 - Attachment is obsolete: true
Attachment #227795 - Flags: review?(alqahira)
(Assignee)

Comment 20

11 years ago
Created attachment 227800 [details]
Better Text for the new pref (nib)
Attachment #227797 - Attachment is obsolete: true
Attachment #227800 - Flags: review?(alqahira)
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.
Attachment #227800 - Flags: superreview?(sfraser_bugs)
Attachment #227800 - Flags: review?(alqahira)
Attachment #227800 - Flags: review+
Created attachment 227801 [details]
Security prefPane as it stands now (on 10.3.9)

Comment 23

11 years ago
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 )
(Assignee)

Comment 24

11 years ago
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.
Attachment #227863 - Flags: review?(alqahira)
(Assignee)

Comment 25

11 years ago
Created attachment 227864 [details]
Screenshot of comment 23 nib (10.4, fwiw)
(Assignee)

Comment 26

11 years ago
Created attachment 227869 [details]
Same design as "Adresses comment 23"

Putting on the late-night IRC polish. :)
Attachment #227863 - Attachment is obsolete: true
Attachment #227869 - Flags: review?(alqahira)
Attachment #227863 - Flags: review?(alqahira)
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.)
Attachment #227869 - Flags: superreview?(sfraser_bugs)
Attachment #227869 - Flags: review?(alqahira)
Attachment #227869 - Flags: review+

Comment 28

11 years ago
This one is easier to parse for the reader, I think, even if it may look a little odd. It's fine by me.
(Assignee)

Updated

11 years ago
Attachment #227800 - Flags: superreview?(sfraser_bugs)
(Assignee)

Updated

11 years ago
Attachment #227869 - Flags: superreview?(sfraser_bugs) → superreview+
(Assignee)

Updated

11 years ago
Attachment #227682 - Flags: review?(mozilla) → superreview?(mikepinkerton)
(Assignee)

Comment 29

11 years ago
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
(Assignee)

Updated

11 years ago
Attachment #227800 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #227801 - Attachment is obsolete: true
Comment on attachment 227682 [details] [diff] [review]
Gets rid of weak ciphers

sr=pink
Attachment #227682 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Updated

11 years ago
Whiteboard: [needs checkin]

Comment 31

11 years ago
Checked in on the trunk and MOZILLA_1_8_BRANCH.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Whiteboard: [needs checkin]
You need to log in before you can comment on or make changes to this bug.