Closed Bug 1486217 Opened 1 year ago Closed 1 year ago

Make sending encrypted emails extensible

Categories

(Thunderbird :: Message Compose Window, task)

All
Unspecified
task
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 64.0

People

(Reporter: patrick, Assigned: patrick)

Details

Attachments

(1 file, 3 obsolete files)

Attached patch patch v1 (obsolete) — Splinter Review
The only way for add-ons to encrypt messages is currently a bad hack: you need to overwrite the registration of the S/MIME composer ("@mozilla.org/messengercompose/composesecure;1"), such that your own code is called. If you then determine that the message is to be encrypted using S/MIME, you need to create your own instance of nsMsgComposeSecure and call the required encryption functions on your own.

The attached patch addresses this in the following way:
1. nsIMsgSMIMECompFields is merged into nsIMsgComposeSecure
2. nsIMsgComposeFields.securityInfo is replaced by nsIMsgComposeFields.composeSecure
3. creation of nsMsgComposeSecure objects is moved from nsMsgSend.cpp into the message composition window.

The approach has two advantages:
- addons can easily create and implement their own objects that implement the nsIMsgComposeSecure
- it's much easier to understand how requireEncryptMessage/signMessage and methods that implement encryption work together.
Assignee: nobody → patrick
Attachment #9003996 - Flags: review?(jorgk)
Magnus, you've worked on this code more than I have recently, would you like to take the review?
Flags: needinfo?(mkmelin+mozilla)
Comment on attachment 9003996 [details] [diff] [review]
patch v1

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

Will take a look
Attachment #9003996 - Flags: review?(jorgk) → review?(mkmelin+mozilla)
Flags: needinfo?(mkmelin+mozilla)
Comment on attachment 9003996 [details] [diff] [review]
patch v1

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

Looks ok to me, and you're probably the one knowing best what makes most sense here. r=mkmelin

::: mailnews/compose/public/nsIMsgCompFields.idl
@@ +11,5 @@
>  
>  /**
>   * A collection of headers and other attributes for building a mail message.
>   */
> +[scriptable, uuid(98a9c816-4ab8-47cf-a0be-52cad08c31f9)]

it's no longer necessary (or common practice) to bump uids when changing interfaces

@@ +93,5 @@
>     */
>    attribute boolean needToCheckCharset;
>  
> +  /**
> +   * Object implementing encryption/signing functionality (e.g. S/MIME)

... or GPG

::: mailnews/compose/public/nsIMsgComposeSecure.idl
@@ +16,4 @@
>  interface nsIMsgComposeSecure : nsISupports
>  {
> +    /**
> +     * set to true if the outgoing message shall be signed

Capitalize and end with .

@@ +20,5 @@
> +     */
> +    attribute boolean signMessage;
> +
> +    /**
> +     * set to true if the outgoing message shall be encrypted

here too
Attachment #9003996 - Flags: review?(mkmelin+mozilla) → review+
Do add-ons need to adapt to this change? Then we'd have to notify the authors of the "other" PGP add-on, "SecureMyEmail".
Of course, but this would be for next ESR.
... umm, or beta? Coming up in September, RSN(TM).
That would be up to you. I never really saw the need to rush things through the release train.
Oh and as that beta would be based on 60.1 (and not really beta as usual, right?) - no uplift there.
I think you misunderstand. If this lands now, it will go to beta in beta 63 in early September. No rushing, no uplift, just usual course of events.
Attached patch patch v1.1 (obsolete) — Splinter Review
Thanks Magnus, attached is the patch with the suggested corrections.

JorgK: yes, we need to inform the other addon authors - this definitely requires them to change their addons. I have created a small example addon that explains how to implement encryption and decryption in Thunderbird; it's now updated to cover the changes of the patch. https://gitlab.com/pbrunschwig/thunderbird-encryption-example/
Attachment #9003996 - Attachment is obsolete: true
(In reply to Jorg K (GMT+2) from comment #9)
> I think you misunderstand. If this lands now, it will go to beta in beta 63
> in early September. No rushing, no uplift, just usual course of events.

If it's important to you that the addon authors have enough time, you could wait committing this until after the beta 63 branch is created. It wouldn't hurt me ;-)
Thanks, I'll land this on TB 64 after the next branch date then. Please ping me if I should forget. Are any tests affected by this? I guess that stuff is not covered by tests. Here's a limited try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a85146d340e5f544df6daebb3379788206c9687d
Comment on attachment 9004812 [details] [diff] [review]
patch v1.1

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

::: mailnews/compose/public/nsIMsgComposeSecure.idl
@@ +11,5 @@
>  interface nsIMsgIdentity;
>  interface nsIOutputStream;
>  
>  /* Security interface */
> +[scriptable, uuid(eb39ba66-c4f8-4a77-a210-c2924783a1b0)]

That UUID has still changed, but I guess it doesn't matter.
Attachment #9004812 - Flags: review?(mkmelin+mozilla)
(In reply to Jorg K (GMT+2) from comment #12)
> Thanks, I'll land this on TB 64 after the next branch date then. Please ping
> me if I should forget. Are any tests affected by this? I guess that stuff is
> not covered by tests. Here's a limited try run:

I didn't find any tests that would cover that stuff.
Comment on attachment 9004812 [details] [diff] [review]
patch v1.1

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

::: mailnews/compose/public/nsIMsgComposeSecure.idl
@@ +32,5 @@
> +
> +    /**
> +     * Determine if encryption and/or signing is required.
> +     *
> +     *

nit: double empty line

@@ +36,5 @@
> +     *
> +     * @param aIdentity   - The sender's identity
> +     * @param compFields  - Attributes of the composed message
> +     *
> +     * @return - Returns true if the creation of the  message requires us to go through

nit: double space

@@ +45,5 @@
> +    /**
> +     * Start encryption work. Called before the encrypted data is processed.
> +     *
> +     * @param aStream     - Output stream that takes the resulting data
> +     * @param aRecipients - Comma- and/or space-speparated list of all recipients (To:, Cc:, Bcc:, ... fields)

Both comma and space are supported?
Are the recipients in decoded or raw form? complete mailbox definitions or emails only?

::: mailnews/extensions/smime/src/nsMsgSMIMECID.h
@@ +5,5 @@
>  
>  #ifndef nsMsgSMIMECID_h__
>  #define nsMsgSMIMECID_h__
>  
> +// OBSOLETE -> TODO: remove

yes, just delete the commented lines now
Attachment #9004812 - Flags: review?(mkmelin+mozilla) → review+
> Both comma and space are supported?

"Supported" is the wrong term. Both are being produced by Thunderbird, thus you have to handle both. Sometimes recipients are just split by "," and sometimes by ", ".

> Are the recipients in decoded or raw form? complete mailbox definitions or emails only?

recipients are in raw form, 1:1 how they are put into the To/Cc fields. Example:

"=?UTF-8?Q?Peter_M=c3=bcller?= <peter@müller.invalid>, Fritz <fritz@meier.invalid>,john@john.invalid"
(In reply to Patrick Brunschwig from comment #16)
> > Both comma and space are supported?
> 
> "Supported" is the wrong term. Both are being produced by Thunderbird,

I'd be curious for a case where we do that, so we can just stop using anything but comma.
Or do you mean ", ", and "," ?
The example that I copied is 1:1 taken from what Thunedrbird creates in
https://dxr.mozilla.org/comm-central/rev/e5e1510b8d914bfa8439b21ba3f73e4f2e83e957/mailnews/compose/src/nsMsgSend.cpp#1202

Lines 1202-1205 fill *recipients; line 1209 passes the data to the encryption engine implementing nsIMsgComposeSecure.

I think that if we wanted to use ", " everywhere, we'd have to modify line 1167.
Looks to me "," is correct. If there's space there they are part of the mailbox (but can be removed). I'm surprised we ever would produce a mailbox with leading whitespace.
The funny point here is that it's:
* ", " for the concatenation of all To: fields
* "," for the concatenation between To: and Cc: 
* and ", " for the concatenation of all Cc:
Please let me know when this is ready. I'm following the bug.
Attached patch patch v1.2 (obsolete) — Splinter Review
Attached is the patch that with the fixes for Magnus' nits (just changed comments, reverted change of UUID).
Flags: needinfo?(jorgk)
Comment on attachment 9007451 [details] [diff] [review]
patch v1.2

Thanks Patrick.
Flags: needinfo?(jorgk)
Attachment #9007451 - Flags: review+
This needed rebasing. Were we going to revert the UUID change in nsMsgSMIMECID.h as well?
Attachment #9007451 - Attachment is obsolete: true
Attachment #9007506 - Flags: review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d3cc5c00e5d6
Make sending encrypted emails extensible. r=mkmelin
Status: NEW → RESOLVED
Closed: 1 year ago
Keywords: checkin-needed
Resolution: --- → FIXED
I've landed that now without the change to NS_MSGCOMPOSESECURE_CID in nsMsgSMIMECID.h. If you want that change, I'm happy to land a follow-up.
Flags: needinfo?(patrick)
Target Milestone: --- → Thunderbird 64.0
Please land the change to NS_MSGCOMPOSESECURE_CID in nsMsgSMIMECID.h as well. It makes (at least) my life easier in Enigmail to find out if Thunderbird contains the change or not.
Flags: needinfo?(patrick) → needinfo?(jorgk)
Consider it done ;-)
Flags: needinfo?(jorgk)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2adc6fc14903
Follow-up: update NS_MSGCOMPOSESECURE_CID UUID. r=jorgk DONTBUILD
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.