Combine verified and purposes column in certificate manager

VERIFIED FIXED

Status

Core Graveyard
Security: UI
VERIFIED FIXED
16 years ago
a year ago

People

(Reporter: kaie, Assigned: kaie)

Tracking

Other Branch

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

16 years ago
The columns verified and purposes and certificate manager should be combined
into one.

If a cert is not verified we display the verification failure reason.

If a cert is verified, we display the content that is currently shown in the
purpose column.
(Assignee)

Comment 1

16 years ago
-> me
Assignee: ssaux → kaie
Keywords: nsbeta1
(Assignee)

Comment 2

16 years ago
Created attachment 94633 [details] [diff] [review]
Patch v1

Updated

16 years ago
Attachment #94633 - Flags: review+

Comment 3

16 years ago
Comment on attachment 94633 [details] [diff] [review]
Patch v1

r=javi

Only issue I have with this is that we no longer notify the user that the OCSP
is turned *off* when we verify the cert for display in the Cert manager.  If
ssaux says this is OK, then I'm fine with it.
(Assignee)

Comment 4

16 years ago
Ah, now I understand. I had filed bug 161907 earlier today, because I realized
that we never use OCSP for the information shown in the verified column.

I now realize that before my patch, the column heading says "Verified (NO OCSP)".

My concern is that most people will not see the "(NO OCSP)" string, because on
most people's screens, the column will not be wide enough.

I understand that verifying every certificate all the time would be too slow.

What about the following idea: Only if OCSP is enabled, we could display an
additional string above the tree control saying "Displayed information has not
been verified with OCSP. Click view to do so.".
(Assignee)

Comment 5

16 years ago
*** Bug 161907 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 6

16 years ago
Sean, what do you think about the suggested wording in the previous comment
(final paragraph)?

Comment 7

16 years ago
I don't think this is the correct way to solve this. There is a more fundamental
problem here with the place where the verification is happening. Verification
should not be done in the paint processing because it's a long-lived operation.
This is not just true for OCSP but my experience shows that it is also true for
CRLs. The verification should be done outside of the GUI thread, and the
verification results should be cached for the life of the window. Please see bug
162033 for more details.
(Assignee)

Comment 8

16 years ago
Julien, I agree to your comment, and I have also read your comment in bug 162033.

However, this is a separate issue. This bug here is about optimizing the used
space on screen primarily. This patch does not make the OCSP situation worse.

If we eventually implement your proposal in bug 162033, we can easily remove the
warning sentence I'm proposing in comment 4.

You proposal is possible but not trivial to implement quickly. I suspect it will
take a while before your proposal gets implemented. I think the simple solution
I suggest in comment 4 is an enhancement, because it warns the user more
clearly, and it would a very simple change.
(Assignee)

Comment 9

16 years ago
After having talked to Sean, here's a new idea:


Let's not make a positive verification statement in the display.
Let's not name the column "verified purposes", but just name it "purposes".

We continue to do only non-OCSP verification.
(as long as Julien's proposal is not implemented).

By doing so, we have the following advantages:
- we do not make a possibly false statement, by not saying "verified" in the UI
- we still show the obvious <expired>, <issuer not trusted> data in the column,
which is helpful if certs are obviously invalid or untrusted.
- we show the general certificate type in the column if it is not expired/untrusted
- it is also acceptable to add the purposes column to the web sites and other
people's tab, since we won't make statements about their verification status

If we do it that way, I think we do not need the additional warning message.


However, we could still display an additional informational message. Sean
suggested a text, and I would to use it in slightly modified form.

Next to the help button, we could display the message:
  "Certificates have not been validated with OCSP. Click View to do so."

However, I'd restrict the visibility of that message to those situations where
the user has actually enabled OCSP in the prefs.


(By doing the above, we also prepare the cert manager UI for another simple fix.
It has been requested to list the trust in the CA tab, i.e. whether it is
trusted for issueing email/ssl/software certs. We could list that in a new
column in the CA tab.)


Does that seem reasonable?
(Assignee)

Comment 10

16 years ago
There is one more alternative.

We could, if OCSP is enabled, hide the verified purposes column completely, as
long as we have not implemented a good dynamic OCSP fetching and UI updating.
(Assignee)

Comment 11

16 years ago
Created attachment 95509 [details] [diff] [review]
Patch v2

This patch implements what is being described in comment 9, it is a slightly
modified version of the previous patch.

(Comment 10 is only an idea, not implemented.)
Attachment #94633 - Attachment is obsolete: true
(Assignee)

Comment 12

16 years ago
Created attachment 95510 [details] [diff] [review]
Patch v3

The previous patch didn't do a good job with aligning the new messages next to
the help button. After asking on #mozilla, this new patch aligns it nicely.
Attachment #95509 - Attachment is obsolete: true

Comment 13

16 years ago
Comment on attachment 95510 [details] [diff] [review]
Patch v3

r=javi
Attachment #95510 - Flags: review+

Comment 14

16 years ago
Comment on attachment 95510 [details] [diff] [review]
Patch v3

Shouldn't it be called mNSSComponent?

Also, is it really a good idea to keep that service as a member variable? I'm
guessing you're doing it as a potential speed-up, but does it really? Also, if
anything goes wrong, you'll have a null nsCOMPtr, and you're not checking for
that, so you'll now crash.
(Assignee)

Comment 15

16 years ago
Jag, I think you overlooked that I also changed:
-  } else if (strcmp(col, "verifiedcol") == 0) {
+  } else if (strcmp(col, "purposecol") == 0 && nssComponent) {

The code inside that bracket is the only code accessing the nssComponent.

I think it indeed can speed up things, because that function is inside the
tree's paint function and will get executed often.

Do you give sr= if I rename nssComponent to mNSSComponent?

Comment 16

16 years ago
Ah, right. Yeah, good call on caching the service for the paint method. sr=jag
(Assignee)

Comment 17

16 years ago
fixed on trunk
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 18

16 years ago
Verified.
Status: RESOLVED → VERIFIED

Updated

13 years ago
Component: Security: UI → Security: UI
Product: PSM → Core
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.