1186286 - Removing a certificate that lacks a CommonName from a trust store shows an empty dialog

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[TomRitterOther (don't use this account, use :tjr)]

Attachment: Sample certificate

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/43.0.2357.134 Safari/537.36

Steps to reproduce:

1) Add a certificate without a CommonName to the Trust Store
2) Delete it



Actual results:

1) Observe that the dialog list is empty


Expected results:

Dialog List should have contained something, perhaps the full Subject value (C=<data>, OU=<data> etc etc).  If the Subject field is missing, perhaps a serial number, or just "Unnamed Cert".

Comment 1

[Mike Hoye [:mhoye]]

Thanks, Tom. Moving to security UI.

Comment 2

[:Cykesiopka]

I want to write a test for deletecert.js, so I might as well fix this along the way.

Comment 3

[:Cykesiopka]

Attachment: Bug 1186286 - Move some code around to facilitate code reuse for the subsequent patch.

The existing code in head.js is moved out because it doesn't need to be there
now, and probably not in the near future.

Review commit: https://reviewboard.mozilla.org/r/83880/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/83880/

Comment 4

[:Cykesiopka]

Attachment: Bug 1186286 - Consult more than just the CN when deleting a cert so it's always clear what's being deleted.

Deleting certs via the cert manager results in a confirmation dialog popping up.
This dialog contains a list of certs that were selected for deletion.

Currently, only the Common Name of each cert is used to represent it in the list.
This is a problem for certs that don't have a CN. More jarringly, the cert
manager itself falls back to various other attributes, so for example selecting
a cert that is represented by its OU ends up popping up a dialog where an empty
list is shown.

Review commit: https://reviewboard.mozilla.org/r/83882/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/83882/

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Actually, if Mark has time to review this, that would be great.

Comment 6

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8798358 [details]
Bug 1186286 - Move some code around to facilitate code reuse for the subsequent patch.

https://reviewboard.mozilla.org/r/83880/#review82762

Comment 7

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8798359 [details]
Bug 1186286 - Consult more than just the CN when deleting a cert so it's always clear what's being deleted.

https://reviewboard.mozilla.org/r/83882/#review82758

::: security/manager/locales/en-US/chrome/pippki/pippki.properties:12
(Diff revision 1)
>  
> +# LOCALIZATION NOTE(certWithSerial): Used for semi-uniquely representing a cert.
> +# %1$S is the serial number of the cert in AA:BB:CC hex format.
> +certWithSerial=Certificate with serial number: %1$S
> +
>  #These are for dialogs

Not your fault, but this file can't make up its mind whether it wants spaces between # and the comment.

::: security/manager/ssl/tests/mochitest/browser/browser_deleteCert_ui.js:95
(Diff revision 1)
> +      cert = yield readCertificate(testCase.certFilename, ",,", gImportedCerts);
> +    }
> +    let certTreeItem = {
> +      hostPort: FAKE_HOST_PORT,
> +      cert: cert,
> +      QueryInterface(iid) {

I personally find `QueryInterface: function(iid)` more readable. Not a strong preference, though.

Comment 8

[:Cykesiopka]

Comment on attachment 8798359 [details]
Bug 1186286 - Consult more than just the CN when deleting a cert so it's always clear what's being deleted.

https://reviewboard.mozilla.org/r/83882/#review82758

Thanks!

> Not your fault, but this file can't make up its mind whether it wants spaces between # and the comment.

Fixed - the file now consistently puts a space between the # and the comment.

> I personally find `QueryInterface: function(iid)` more readable. Not a strong preference, though.

Noted. Let's keep this as is for now.

Comment 9

[:Cykesiopka]

Comment on attachment 8798358 [details]
Bug 1186286 - Move some code around to facilitate code reuse for the subsequent patch.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83880/diff/1-2/

Comment 10

[:Cykesiopka]

Comment on attachment 8798359 [details]
Bug 1186286 - Consult more than just the CN when deleting a cert so it's always clear what's being deleted.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83882/diff/1-2/

Comment 11

[:Cykesiopka]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1a5b0292fc67d0c9f12183fd8874ce9e6032094

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c693c3241fa
Move some code around to facilitate code reuse for the subsequent patch. r=mgoodwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcfc065914e5
Consult more than just the CN when deleting a cert so it's always clear what's being deleted. r=mgoodwin

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4c693c3241fa
https://hg.mozilla.org/mozilla-central/rev/fcfc065914e5