1633665 - Broken list of recipient S/MIME certificates in composer "view / message security info"

Status: RESOLVED FIXED

(MailNews Core :: Security: S/MIME, defect)

Description

[Kai Engert [:KaiE:]]

In a composer window, with S/MIME configured, and recipients added, click menu view / message security info (or click the security button).

The listbox should show the list of all recipients and their certificate status.

This worked in TB 68, but it's broken in nightly. I think it was regressed by bug 1594896.

Comment 1

[Magnus Melin [:mkmelin]]

Attachment: bug1633665_smime_sec_info.patch

This fixes it, and cleans up a bunch of code.
On a side note, checking those certificates is S L O W! Like 1 sec per address. But it works now.

Comment 2

[Kai Engert [:KaiE:]]

Comment on attachment 9143978 [details] [diff] [review]
bug1633665_smime_sec_info.patch

Thanks Magnus, I've tested it locally and it looks good.

Comment 3

[Ben Campbell]

Comment on attachment 9143978 [details] [diff] [review]
bug1633665_smime_sec_info.patch

Review of attachment 9143978 [details] [diff] [review]:
-----------------------------------------------------------------

All looks good to me, barring the issue in nsSMimeJSHelper.cpp.
(All my other comments are just sanity checks and nits)

::: mail/components/compose/content/MsgComposeCommands.js
@@ -4023,2 @@
>        .createInstance(Ci.nsISMimeJSHelper)
> -      .getNoCertAddresses(gMsgCompose.compFields, emailAddresses);

I don't understand how this ever worked. I'm assuming there's some magic going on in the xpidl JS bindings to return a value using the extra parameter. In any case your change makes it way clearer.

@@ +4025,5 @@
>    } catch (e) {
>      return;
>    }
>  
> +  if (emailAddresses.length === 0) {

Seeing === here makes me think I'm missing something. Would == do?

::: mail/extensions/openpgp/content/ui/enigmailMsgComposeOverlay.js
@@ +1888,3 @@
>          .createInstance(Ci.nsISMimeJSHelper)
> +        .getNoCertAddresses(gMsgCompose.compFields);
> +      return addresses.length === 0;

Again, === instead of == makes me think I'm missing some JS subtlety here...

::: mailnews/extensions/smime/content/msgCompSecurityInfo.js
@@ +34,4 @@
>  
>    while (true) {
>      try {
> +      // Out parameters - must be objects.

I had to look up the XPIDL docs to confirm this. Ugh, that is cumbersome.
Never thought I'd see the C++ side more ergonomic than JS :-)
Good reason to avoid out and inout params in idl files wherever possible.

@@ +102,5 @@
> +  let signedElement = document.getElementById("signed");
> +  let encryptedElement = document.getElementById("encrypted");
> +  if (params.smFields.requireEncryptMessage) {
> +    if (params.isEncryptionCertAvailable && canEncrypt) {
> +      encryptedElement.value = gBundle.getString("StatusYes");

I assume we're no longer worried about gBundle being unset?

::: mailnews/extensions/smime/src/nsSMimeJSHelper.cpp
@@ +46,5 @@
>    certs.SetCapacity(mailbox_count);
>  
>    nsCOMPtr<nsIX509CertDB> certdb = do_GetService(NS_X509CERTDB_CONTRACTID);
>  
> +  *canEncrypt = emailAddresses.IsEmpty();

SetCapacity doesn't change the size, so emailAddresses is always empty at this point (and isn't it backwards? don't you want the result to be false if there aren't any addresses?).

*canEncrypt = (mailbox_count > 0) ? true : false;

maybe?

Comment 4

[Magnus Melin [:mkmelin]]

(In reply to Ben Campbell from comment #3)

• if (emailAddresses.length === 0) {

Seeing === here makes me think I'm missing something. Would == do?

== would work too.

I assume we're no longer worried about gBundle being unset?

Yes, it would always be set.

*canEncrypt = (mailbox_count > 0) ? true : false;

Ah, good catch! I'll fix it.

Comment 5

[Magnus Melin [:mkmelin]]

Or well, that should be initialized to canEncrypt = true. We "can encrypt" to 0 recipients.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e1fe33ee1a8b
fix (and code cleanup) for broken list of recipient S/MIME certificates in composer "view / message security info". r=benc DONTBUILD