64128 - Cannot edit builtin object token.

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[John Unruh]

1.) Open the Security Manager and click on Certificates and Web Sites.
2.) Click on a builtin object token, then on Edit and Edit.
What happens: I cannot edit the token and am returned to the same popup window.

Comment 1

[Ian McGreer]

reassigning to myself

Comment 2

[Ian McGreer]

Attachment: patch for review

Comment 3

[Ian McGreer]

This patch was created to fix this bug, but it really fixes a series of related 
bugs in cert manager, all having to do with being able to delete and edit trust 
of builtin token certs.  I also included a few wording changes requested by 
Sean.

Javi or David, can you review?  I would consider this a high-priority bug, so 
it should get whatever keywords need to be attached.

Comment 4

[Javier Delgadillo]

Few observations:
1) Remove the programmer reminders to add help later.  (There is already a bug
for general Help issues in PSM)
2)In your js function doLoadForSSLCert, you have a hardcoded English string that
gets set as the "title" attribute.  That string should come from a properties
file.
3)It seems like your dtd uses sentence fragments for the following keywords:
certmgr.editsslcert.beforename
certmgr.editsslcert.aftername

These tend to cause problems for localization.  So use complete sentences w/ %S
parameters strings from the properties file if necessary to make localization
easier.

Resolve these issues and you've got r=javi

Comment 5

[Ian McGreer]

Attachment: rev 2

Comment 6

[Ian McGreer]

blizzard, can you sr=?

thanks

Comment 7

[Christopher Blizzard (:blizzard)]

|nsNSSCertTrust::HasTrustedPeer| has a misleading signature.  If you were to use
it like this:

HasTrustedPeer(PR_TRUE, PR_FALSE, PR_TRUE);

and mTrust.sslFlags was false but mTrust.objectSigningFlags was this function
would return PR_FALSE incorrectly.  In fact, how you use it is odd:

+  if (certType == nsIX509Cert::CA_CERT) {
+    if (trustType & nsIX509CertDB::TRUSTED_SSL) {
+      *_isTrusted = trust.HasTrustedCA(PR_TRUE, PR_FALSE, PR_FALSE);
+    } else if (trustType & nsIX509CertDB::TRUSTED_EMAIL) {
+      *_isTrusted = trust.HasTrustedCA(PR_FALSE, PR_TRUE, PR_FALSE);
+    } else if (trustType & nsIX509CertDB::TRUSTED_OBJSIGN) {
+      *_isTrusted = trust.HasTrustedCA(PR_FALSE, PR_FALSE, PR_TRUE);
+    } else {
+      return NS_ERROR_FAILURE;
+    }

You could probably change the way that function works so that it's looking for
trust and doesn't return early and you could remove all of those calls to the
same function.

Fix that and you can have an sr=blizzard

Comment 8

[Ian McGreer]

I'm not sure I understand.  The signature is:

+  PRBool HasTrustedPeer(PRBool checkSSL = PR_TRUE, 
+                        PRBool checkEmail = PR_TRUE,  
+                        PRBool checkObjSign = PR_TRUE);

The logic is, "For each of the trust types to check (SSL, Email, or Object
Signing), look to see if the cert has the Trusted Peer attribute.  If any of the
trust types being checked are not Trusted Peer, return false.  Otherwise, return
true."

For example, if I want to know if a particular cert is trusted for SSL, I would
call:

trust->HasTrustedPeer(PR_TRUE, PR_FALSE, PR_FALSE);

If I want to know if it is trusted for SSL and Object Signing, I would call:

trust->HasTrustedPeer(PR_TRUE, PR_FALSE, PR_TRUE);

Both conditions must be met, that is, it must be trusted for both, to return
true.  That is why there is an early exit in the function.

The reason there are three calls to the function is that the UI needs to
separate the three types of trust in order to decide which checkboxes to check.
 The question, in this case, isn't, "Is this cert trusted at all?";, it is, "Is
this cert trusted for (SSL, Email, Object Signing)?"

I understand that it's misleading.  Maybe it should be called
"HasTrustedPeerForTypes", but that would mean changing other functions in order
to be consistent.  Fortunately, this isn't an exported interface, so I'd like to
leave it as it is for now.  My bad for not being more clear in the beginning.

Comment 9

[Christopher Blizzard (:blizzard)]

No problem.  You should be set to go then.  I just thought it was confusing,
that's all.

Comment 10

[Ian McGreer]

marking fixed, checkin was 5/22

Comment 11

[John Unruh]

Verified.