161915 - Combine verified and purposes column in certificate manager

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[Kai Engert (:KaiE:)]

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.

Comment 1

[Kai Engert (:KaiE:)]

-> me

Comment 2

[Kai Engert (:KaiE:)]

Attachment: Patch v1

Comment 3

[Javier Delgadillo]

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.

Comment 4

[Kai Engert (:KaiE:)]

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.".

Comment 5

[Kai Engert (:KaiE:)]

*** Bug 161907 has been marked as a duplicate of this bug. ***

Comment 6

[Kai Engert (:KaiE:)]

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

Comment 7

[Julien Pierre]

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.

Comment 8

[Kai Engert (:KaiE:)]

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.

Comment 9

[Kai Engert (:KaiE:)]

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?

Comment 10

[Kai Engert (:KaiE:)]

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.

Comment 11

[Kai Engert (:KaiE:)]

Attachment: 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.)

Comment 12

[Kai Engert (:KaiE:)]

Attachment: 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.

Comment 13

[Javier Delgadillo]

Comment on attachment 95510 [details] [diff] [review]
Patch v3

r=javi

Comment 14

[jag (Peter Annema)]

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.

Comment 15

[Kai Engert (:KaiE:)]

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

[jag (Peter Annema)]

Ah, right. Yeah, good call on caching the service for the paint method. sr=jag

Comment 17

[Kai Engert (:KaiE:)]

fixed on trunk

Comment 18

[John Unruh]

Verified.