1603813 - Unify general encryption options in composer window

Status: RESOLVED FIXED

(MailNews Core :: Security: OpenPGP, enhancement)

Description

[Kai Engert [:KaiE:]]

This is about a small subset of OpenPGP UI that we'll require. It's a UI subset that we'll need very early for testing.

Currently, composer window allows switching of some encryption settings:

• sign with S/MIME or not
• encrypt with S/MIME or not

I suggest that the user should be able to switch the encryption that will be used for the current email being composed, either S/MIME or OpenPGP. For that, we should have a new menu command. Suggestion is to use a nested submenu, labeled "Encryption Technology", with two radio items in it, to select between S/MIME and OpenPGP. (Thanks to :aleca for the guidance.)

I suggest the existing sign/encrypt switches should apply regardless of the technology selection.

To implement this, some refactoring of the existing S/MIME code will be necessary. Today, enabling S/MIME options, while not yet having configured S/MIME for that account, will trigger prompts to set up etc. We might want to change that, because it's tricky if encryption is enabled, and the user switches the technology to S/MIME, although that's not yet set up.

Comment 1

[Kai Engert [:KaiE:]]

For the encryption setting, I suggest that we use a radio button:

(*) Do not encrypt
( ) Require encryption

Why not a checkbox?

The reason is, we consider to implement a third setting at a later time, "allow encryption". I'd like to skip that mode initially, but once we'll work on it, we can easily extend it with another radio button like this:

(*) Do not encrypt
( ) Allow encryption
( ) Require encryption

I want to postpone the "Allow encryption" mode for now. It's more difficult to implement correctly.

Comment 2

[Kai Engert [:KaiE:]]

Attachment: 1603813-v1.patch

Comment 3

[Kai Engert [:KaiE:]]

Attached is a first cut of (working) refactoring, and initial UI for testing.

With openpgp disabled at build time, S/MIME behavior is unmodified, so this should be safe for nightly landing.

With openpgp enabled:

• it's still possible to send s/mime as before.
• the security button and options menu has a new entry for encryption technology, and if openpgp is selected, to attach own's public key
• key selection is currently done automatically based on email address, both when attaching your own public key, and when sending an encrypted message
• currently no key verification is done, non-expired and non-revoked keys are silently accepted (this is an initial state for testing, this will behave differently later)
• only works when manually copying RNP libraries into your openpgp enabled build, e.g. from https://kuix.de/mozilla/openpgp/libs-20191115/

Comment 4

[Kai Engert [:KaiE:]]

Attachment: current-demo-instructions.txt

Comment 5

[Kai Engert [:KaiE:]]

Note the patch attachment is mostly a backup. I need to merge to latest comm-central and verify it still works. When I have done that, I'll create a try build, as an initial demo.

Comment 6

[Kai Engert [:KaiE:]]

Attachment: 1603813-global-v2.patch

Subset of changes that apply to the global files (output of openpgp directory).

For the UI unification, it moves some code from the S/MIME specific files into the global code.

I don't claim this will be the final UI, just something that we can use during our initial development. It's fine if Alessandro already wants to give feedback, but it could also be done in a follow-up step.

Comment 7

[Kai Engert [:KaiE:]]

Attachment: 1603813-pgp-v2.patch

The other subset, the changes to the enigmail code, mostly for the composer window.

Requesting review for "ok to commit".

Calling GPG no longer worked, so I'm returning early in many functions, to prevent their execution. To be cleaned up later. Regarding the "engine" pref, we can probably remove it later, because we currently don't consider to offer two complete separate engine implementations, but might only optionally, potentially use the GPG engine for a subset of operations. I'll handle that later.

The patch removes a lot of functionality from the enigmail composer code, because I'd like to change how we track the encryption/signing state. Whenever I have code that I don't need immediately, but which I still might need to look at later, I'm not removing it, but keep it commented out. The code for "original was encrypted, so reply should be encrypted, too" isn't ready yet. I'd like to track that with a separate flag (gIsRelatedToEncryptedOriginal) instead of the old approach that used a complex mode variable to track multiple things (sendMode).

Removed some legacy code that was intended for older TB versions.

AIUI, enigmail handled sending of all signed/encrypted email, for both S/MIME and OpenPGP. I'd like to change how that works. I'd like to have the global TB code handle the sending, if S/MIME is used. The enigmail code should only have to handle sending of OpenPGP messages.

Added code that uses RNP to search by email address and obtains a public key, for attaching it to outgoing email.

I changed key generation to use RSA for now, for greater compatibility during testing. Also changed defauly expiry to 1 month, to limit trouble if people generate keys during testing and accidentally use them for real world purposes.

The enigmail composer overlay no longer handles the status icons for "what does the user want", that's now done in global code (identically for both S/MIME and OpenPGP). We'll later have to introduce new code to visualize the "is encryption possible or not".

I'm removing code for "inline encryption". We've agreed we only want to limit "inline OpenPGP" to plain text signed emails without attachments (if we'll support inline at all).

Handling the enigmail draft flags correctly is a todo for later.

I'd like to use the old enablePgp pref for migration purposes, for checking if PGP was previously enabled with Enigmail installed. For future "PGP is enabled", I'd like to use something else, so I'm removing that pref from the default list.

Comment 8

[Kai Engert [:KaiE:]]

Comment on attachment 9126948 [details] [diff] [review]
1603813-global-v2.patch

try build to check if tests still pass with the global (non-pgp) changes, if pgp built option is off
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b29e69d1eedd5176e35b4ad2cbd13ca194c77af9

Comment 9

[Kai Engert [:KaiE:]]

Attachment: Bug 1603813, Unify general encryption options in composer window. r?mkmelin

Comment 10

[Kai Engert [:KaiE:]]

Attachment: Bug 1603813, pgp changes for composer. r?mkmelin

Comment 11

[Kai Engert [:KaiE:]]

Patrick, are you able to create an account at https://phabricator.services.mozilla.com/ ?
Even if you don't plan to do your reviews over there, it will allow me to specify you as reviewer. Otherwise I need to set a dummy person as reviewer, or I cannot submit it.
Reading patches in phabricator is easier than in bugzilla.

You should be able to click the attachment in comment 10, instead of attachment 9126952 [details] [diff] [review].

Also, most developers use some sort of nickname, and edit their bugzilla name to include it, prefixed with colon ':'. Would you like to do that, too? If you don't use a nickname, it seems :patrick isn't yet used.

Comment 12

[Kai Engert [:KaiE:]]

Comment on attachment 9126948 [details] [diff] [review]
1603813-global-v2.patch

many failed tests in try build, so let's postpone the review for the global code, until I have looked into the failures.

Comment 13

[Patrick Brunschwig]

(In reply to Kai Engert (:KaiE:) from comment #11)

Patrick, are you able to create an account at https://phabricator.services.mozilla.com/ ?

Yes, I created an account now ("Patrick Brunschwig").

Comment 14

[Patrick Brunschwig]

Comment on attachment 9126952 [details] [diff] [review]
1603813-pgp-v2.patch

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

modules/rnp.jsm:
getKeyHandleByIdentifier(): 
   if RNP supports PGPv3 keys, you should also accept fingerprints with length == 32.

ui/enigmailMsgComposeOverlay.js:
  for my taste, the whole part about migrating identityEnigmailPrefsMigrated=true does not belong into UI code. There is configure.jsm, which is meant to take care of such functionality.

Comment 15

[Kai Engert [:KaiE:]]

Comment on attachment 9126948 [details] [diff] [review]
1603813-global-v2.patch

Magnus, I'm restoring the request to review.

Regarding the tests that failed on try-comm-central on Linux: I executed all those failing xpcshell and mochitest tests on my local system, with a build that used latest c-c and latest m-c, and they passed for me locally. I don't understand why they filed on the try server.

Comment 16

[Kai Engert [:KaiE:]]

(In reply to Patrick Brunschwig from comment #14)

modules/rnp.jsm:
getKeyHandleByIdentifier():
if RNP supports PGPv3 keys, you should also accept fingerprints with
length == 32.

ok

ui/enigmailMsgComposeOverlay.js:
for my taste, the whole part about migrating
identityEnigmailPrefsMigrated=true does not belong into UI code. There is
configure.jsm, which is meant to take care of such functionality.

Yes, fully agree. That code hasn't arrived in the final place yet. I'll add a TODO comment.

Comment 17

[Kai Engert [:KaiE:]]

Attachment: Bug 1603813 - expiration is relative. r?PatrickBrunschwig

Comment 18

[Magnus Melin [:mkmelin]]

Comment on attachment 9126948 [details] [diff] [review]
1603813-global-v2.patch

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1628,5 @@
> +function setSecuritySettings(menu_id) {
> +  document
> +    .getElementById("menu_securityEncryptDisable" + menu_id)
> +    .setAttribute("checked", !gSendEncrypted && !gOptionalEncryption);
> +  /*document

please move comment up to it's own line

@@ +3722,5 @@
> +  if (gSendSigned) {
> +    signStatus.classList.add("signing-msg");
> +  } else {
> +    signStatus.classList.remove("signing-msg");
> +  }

please just use
 document.getElementById("signing-status").classList.toggle("signing-msg", gSendSigned);

... and same thign below

@@ +3746,5 @@
> +function setSecurityCommand(what) {
> +  gNextSecurityButtonCommand = what;
> +}
> +
> +function doSecurityCommand() {

Gah, existing but terrible. But I suppose we can fix it later

@@ +4300,2 @@
>      if (event.defaultPrevented) {
> +      console.debug("in MsgComposeCommands: DONE default prevented, abort");

please remove these debug outputs

::: mail/components/compose/content/messengercompose.xhtml
@@ +1727,4 @@
>  
> +          <menu id="menu_encTech1"
> +                label="&menu_securityTech.label;" accesskey="&menu_securityTech.accesskey;">
> +            <menupopup id="button-encTech1">

let's not use ones and twos in the ids, but use something descriptive (especially 1 should never be used since it can be easily misinterpreted as an l)

@@ +1729,5 @@
> +                label="&menu_securityTech.label;" accesskey="&menu_securityTech.accesskey;">
> +            <menupopup id="button-encTech1">
> +              <menuitem id="encTech_OpenPGP1"
> +                        label="&menu_techPGP.label;" accesskey="&menu_techPGP.accesskey;"
> +                        oncommand="setEncTechPGP();" type="radio" name="radiogroup_encTech"/>

instead of having setEncTechPGP and setEncTechSMIME functions it would be better to have
onSetEncryptionTechnology(event)

value="OpenPGP" oncommand="setEncryptioneTechnology(event)"

Then use event.target.value to set it to whichever was selected

@@ +1743,5 @@
> +          <menuitem id="menu_securityEncryptDisable1"
> +                    type="radio" name="radiogroup_encEnable"
> +                    label="&menu_securityDoNotEncrypt.label;"
> +                    accesskey="&menu_securityDoNotEncrypt.accesskey;"
> +                    oncommand="setGlobalEncryptMessage(0);"/>

for these too, better to have the menuitem have a value and and event handler function

@@ +1908,5 @@
> +                  type="radio" name="radiogroup_encEnable"
> +                  label="&menu_securityDoNotEncrypt.label;"
> +                  accesskey="&menu_securityDoNotEncrypt.accesskey;"
> +                  oncommand="setSecurityCommand('enc0');"/>
> +        <!-- <menuitem id="menu_securityEncryptOptional2"

better to put comment on a separate line so the actual code line don't need to be touched when un-commenting later

::: mail/locales/en-US/chrome/messenger-smime/msgCompSMIMEOverlay.dtd
@@ +4,5 @@
>  
>  <!--LOCALIZATION NOTE msgCompSMIMEOverlay.dtd UI for s/mime hooks in message composition -->
>  
> +<!ENTITY menu_securityDoNotEncrypt.label "Do Not Encrypt">
> +<!ENTITY menu_securityDoNotEncrypt.accesskey "O">

accesskeys are case sensitive (but wrong case works)

@@ +11,5 @@
> +<!ENTITY menu_securityEncryptOptional.label "Allow Encryption">
> +<!ENTITY menu_securityEncryptOptional.accesskey "W">
> +-->
> +
> +<!ENTITY menu_securityEncryptRequire.label "Require Encryption">

you changed this value, but didn't change the key. The key must be changed so that localizers would know

Comment 19

[Kai Engert [:KaiE:]]

Attachment: 1603813-global-v3.patch

This tries to address all of Magnus' concerns on the global patch.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7937d619f3d8
Unify general encryption options in composer window. r=mkmelin
https://hg.mozilla.org/comm-central/rev/826a884758f8
pgp changes for composer. r=PatrickBrunschwig
https://hg.mozilla.org/comm-central/rev/e80f11af3975
Key expiration is relative. r=PatrickBrunschwig