121906 - Other People's certs not sorted

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, enhancement, P2)

Description

[John Unruh]

1.) Receive several signed emails, or copy into a new profile the *.db files
from the profile in which you normally receive lots of signed email.
2.) Open the Cert Manager, and select the Other People's tab.
What is expected: That certs would be sorted alphabetically.
What happens: There doesn't seem to be any type of sorting pattern.

Comment 1

[Stephane Saux]

kai

Comment 2

[John Unruh]

nsbeta1

Comment 3

[Kai Engert [:KaiE:]]

I suggest to sort by email address, because sorting by common name seems to be
more difficult. (If we just sort by common name string, we would sort by
whatever is listed first in the name, which might be first name, or whatever.
Sorting by last name would require to detect which part of the common name is
actually the last name, and that might be tricky).

Comment 4

[Kai Engert [:KaiE:]]

Hmm, actually I think we should re-think sort orders in cert manager completely.

Currently, everything is sorted by:
- token
- issuer organization
- organization

The interesthing this is: Comments in the code say, the third criterion is
common name, but the actual code uses the organization attribute.


The information we display in a tree where toplevel nodes shows issuer organization.
Therefore I think, it is reasonable to always use it as the first sort criterion.

It would resolve problems in the CA tab, where we display multiple nodes of a
CA, if we have certs on different tokens. Displaying all of them below the same
top node would resolve the confusion.


I think the following order makes sense for the CA tab:
1) Issuer Organization
2) Organization
3) Token


For web sites, I suggest:
1) Issuer Organization
2) Token
3) Common Name (www.host.com)


For self's certificates, I suggest:
1) Issuer Organization
2) Token
3) Serial Number descending (newest on top)


For other's certificates, I suggest:
1) Issuer Organization
2) Email Address
3) Common Name (for certs without email addresses)

Comment 5

[Kai Engert [:KaiE:]]

I disussed with Stephane, and the following seems to be a better sort order:

CA tab:
1) Issuer Organization
2) Organization
3) Token


Web Sites:
1) Issuer Organization
2) Common Name (www.host.com)


Self's certificates:
1) Issuer Organization
2) Token
3) Issued date descending (newest on top)


Other people's certificates:
1) Issuer Organization
2) Email Address
3) Common Name (for certs without email addresses)

Comment 6

[Kai Engert [:KaiE:]]

Attachment: Patch v1

Comment 7

[Kai Engert [:KaiE:]]

Javi, can you please review?

Comment 8

[Javier Delgadillo]

Comment on attachment 95367 [details] [diff] [review]
Patch v1

r=javi

The nsIX509Certificate interface has changed.  What does the embedding group
have to say about changing interfaces nowadays?

Comment 9

[Kai Engert [:KaiE:]]

The interface is not yet marked as @frozen in the IDL file.
Only files marked that way must not be changed.

I think the change in that patch should be fine, since we are the only one
implementing that interface and we are not changing anything, but only adding
new attributes.

Comment 10

[jag (Peter Annema)]

Comment on attachment 95367 [details] [diff] [review]
Patch v1

>Index: mozilla/security/manager/ssl/src/nsCertTree.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/security/manager/ssl/src/nsCertTree.cpp,v
>retrieving revision 1.23
>diff -u -d -r1.23 nsCertTree.cpp
>--- mozilla/security/manager/ssl/src/nsCertTree.cpp	6 Aug 2002 13:25:21 -0000	1.23
>+++ mozilla/security/manager/ssl/src/nsCertTree.cpp	15 Aug 2002 06:20:36 -0000

>+  if (sort_IssuedDateDescending == crit) {
>+    // reverse compare order
>+    if (str_a != nsnull && str_b != nsnull) {
>+      return -1*Compare(str_a, str_b);
>+    } else {
>+      return !str_a ? (!str_b ? 0 : 1) : -1;
>+    }
>+  }
>+  else {
>+    // standard compare order
>+    if (str_a != nsnull && str_b != nsnull) {
>+      return Compare(str_a, str_b);
>+    } else {
>+      return !str_a ? (!str_b ? 0 : -1) : 1;
>+    }
>   }
> }

You could write this as:

  PRInt32 result;
  if (str_a && str_b)
    result = Compare(str_a, str_b);
  else
    result = !str_a ? (!str_b ? 0 : -1) : 1;

  if (sort_IssuedDateDescending == crit)
    result *= -1; // reverse compare order

  return result;

>Index: mozilla/security/manager/ssl/src/nsNSSCertificate.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/security/manager/ssl/src/nsNSSCertificate.cpp,v
>retrieving revision 1.91
>diff -u -d -r1.91 nsNSSCertificate.cpp
>--- mozilla/security/manager/ssl/src/nsNSSCertificate.cpp	13 Aug 2002 01:15:05 -0000	1.91
>+++ mozilla/security/manager/ssl/src/nsNSSCertificate.cpp	15 Aug 2002 06:20:48 -0000
>@@ -1285,6 +1285,36 @@
>   return NS_OK;
> }
> 
>+nsresult
>+nsNSSCertificate::GetSortableDate(PRTime aTime, PRUnichar **_aSortableDate)
>+{
>+  PRExplodedTime explodedTime;
>+  PR_ExplodeTime(aTime, PR_GMTParameters, &explodedTime);
>+  char datebuf[20]; // 4 + 2 + 2 + 2 + 2 + 2 + 1 = 15
>+  if (0 != PR_FormatTime(datebuf, 20, "%Y%m%d%H%M%S", &explodedTime)) {

It would be better to use sizeof(datebuf) instead of the hardcoded 20.

sr=jag with those changes

Comment 11

[Kai Engert [:KaiE:]]

Attachment: Patch v2

Patch with changes requested by Jag.

Comment 12

[Kai Engert [:KaiE:]]

Comment on attachment 97003 [details] [diff] [review]
Patch v2

carrying forward reviews

Comment 13

[Kai Engert [:KaiE:]]

Fixed on trunk.

Comment 14

[Kai Engert [:KaiE:]]

Marking fixed.

Comment 15

[John Unruh]

Verified on 8/29 trunk