1665145 - Importing OpenPGP public keys: Allow multi-file selection

Status: RESOLVED FIXED

(MailNews Core :: Security: OpenPGP, enhancement)

Description

[Kai Engert [:KaiE:]]

We received a request for this enhancement, which would be very simple to implement, and improve the usability for affected users a lot:

In OpenPGP key manager, for the "import public key(s)" command:

Allow the selection of multiple files at once.
Loop over the selected files, and execute a separate import action for each file.

If one of the files has a problem, the import should continue with the other files.

Comment 1

[drebs]

It would also be nice to have the ability to import multiple keys from one file. This was the case with Enigmail, but now only the first key in a file is imported.

I don't mean to hijack this bug, but maybe a more general way to state the issue would be "Can't import multiple keys at once" ?

Comment 2

[Alessandro Castellani [:aleca]]

(In reply to drebs from comment #1)

It would also be nice to have the ability to import multiple keys from one file. This was the case with Enigmail, but now only the first key in a file is imported.

This works already. If you open a file that has multiple keys in it, all the keys are properly listed and recognized during import.
Does this work for you?

Comment 3

[drebs]

(In reply to Alessandro Castellani (:aleca) from comment #2)

(In reply to drebs from comment #1)

It would also be nice to have the ability to import multiple keys from one file. This was the case with Enigmail, but now only the first key in a file is imported.

This works already. If you open a file that has multiple keys in it, all the keys are properly listed and recognized during import.
Does this work for you?

It does not work for me. But I should've been more explicit my use case, sorry.

I'm trying to import from a file with multiple exported keys concatenated, that is, there are many -----BEGIN/END PGP PUBLIC KEY BLOCK----- blocks in my file, one for each key. I am not sure if that should actually be supported, according to standards, but as it worked in Enigmail I expected it to continue working with the new Thunderbird OpenPGP implementation. If this should not be the case, i'm sorry for the noise and you may disregard this comment. :-)

I have not tried importing from a file with many keys in only one public key block.

Comment 4

[Alessandro Castellani [:aleca]]

Attachment: 1665145-openpgp-multi-file.diff

This is a WIP patch almost entirely working.

I'm asking an early feedback regarding a couple of important points:

• I updated the EnigmailDialog.filePicker() method to enable opening multiple files, but since that method is used in many other locations, I'm passing a variable to determine the open "mode", and returning an array of files in case multiple was specified. I'm doing this as a return (return multiple ? files : files[0];) in order to avoid updating all the other methods where this function is used to open/save a single file.
• The import dialog, for the way is currently designed, shows error messages in the first slide, before the list of keys. Allowing the import of multiple files means that we might stumble upon a file that triggers an error while other files are correct. Should we continue the import of any valid key from other files, and list the error messages in the list overview, or interrupt the import altogether? If we don't interrupt the import and we show the errors, I need to update those elements to allow multiple showing errors (stacked notifications), and list the filename from which the error came from.

Comment 5

[Alessandro Castellani [:aleca]]

Attachment: 1665145-openpgp-multi-file.diff

Well, this was more complicated than expected, especially when dealing with a mixed import of valid and invalid files/keys.
These are all the scenarios I tested:

• Import an invalid single file = Error message.
• Import multiple files, with 1 invalid = 1 Error message + list of valid keys.
• Import multiple files, with 2 invalid = 2 Error messages + list of valid keys.

When importing multiple secret keys, the password prompt should appear for each key.
When the import concluded, a recap of all the imported keys should appear, alongside any error message we collected during the process.

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9182156 [details] [diff] [review]
1665145-openpgp-multi-file.diff

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

::: mail/extensions/openpgp/content/modules/dialog.jsm
@@ +480,5 @@
>        }
>  
> +      for (let file of fixIterator(filePicker.files, Ci.nsIFile)) {
> +        files.push(file);
> +      }

for (let file of filePicker.files) {

::: mail/extensions/openpgp/content/ui/keyWizard.js
@@ +47,5 @@
>  // OpenPGP variables.
>  var gKeygenRequest;
>  var gAllData = "";
>  var gGeneratedKey = null;
> +var kFiles;

k for Constant (hah!). But this would be g for Global.

@@ +842,5 @@
> + * Populate the key list in the import dialog with all the valid keys fetched
> + * from a single file.
> + *
> + * @param {Array} importKeys - The array of keys fetched from a single file.
> + */

prefer [], but an array of what type?

::: mail/extensions/openpgp/content/ui/keyWizard.xhtml
@@ +212,5 @@
>        <label id="importKeyTitle"
>               data-l10n-id="openpgp-import-key-title"
>               class="dialogheader-title"/>
>  
> +      <vbox id="openPgpImportWarning" collapsed="true"/>

if using hidden works, I think that's better for future compat

Comment 7

[Alessandro Castellani [:aleca]]

Attachment: 1665145-openpgp-multi-file.diff

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9182472 [details] [diff] [review]
1665145-openpgp-multi-file.diff

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

::: mail/extensions/openpgp/content/modules/dialog.jsm
@@ +12,5 @@
>    "resource://gre/modules/XPCOMUtils.jsm"
>  );
> +var { fixIterator } = ChromeUtils.import(
> +  "resource:///modules/iteratorUtils.jsm"
> +);

not needed

@@ +484,5 @@
> +        for (let file of filePicker.files) {
> +          files.push(file);
> +        }
> +      } else {
> +        files.push(filePicker.file.QueryInterface(Ci.nsIFile));

I think this doesn't need QI (but please check)

::: mail/extensions/openpgp/content/ui/keyWizard.js
@@ +775,5 @@
> +
> +  // Clear the key list from any previously listed key.
> +  let keyList = document.getElementById("importKeyList");
> +  for (let node of keyList.children) {
> +    keyList.removeChild(node);

AIUI it's preferable to use somthing like
while (keyList.lastChild) {
keyList.lastChild.remove();
}

@@ +958,5 @@
> +/**
> + * Populate the key list in the import dialog with all the valid keys imported
> + * from a single file.
> + *
> + * @param {Array} resultKeys - The array of keys imported from a single file.

@param {string[]}

Comment 9

[Alessandro Castellani [:aleca]]

Comment on attachment 9182472 [details] [diff] [review]
1665145-openpgp-multi-file.diff

I'm changing the review on this since the Key manager uses a different method from the Key Wizard and that needs to be updated as well.

Comment 10

[Alessandro Castellani [:aleca]]

Attachment: 1665145-openpgp-multi-file.diff

This takes care also of the import of public keys in the OpenPGP Key manager.
The workflow in that dialog is a not the best as we rely on alert notifications for each imported file. That's something we'll need to take care of once we improve this whole section.

These are the areas I manually tested:

• Import multiple secret keys from the Key Wizard dialog.
• Import multiple secret key from the Key Manager dialog.
• Import multiple public keys from the Key Manager dialog.
• Import a single file in both Key Manager and Key Wizard.
• Export public/secret key from both Key Manager and e2ee settings page.

I looked around and I think I covered every location where this import/export dialog is used.
Apologies for the noise and the extra review.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3fe34ed7818d
[OpenPGP] Allow importing multiple files. r=mkmelin

Comment 12

[Alessandro Castellani [:aleca]]

Comment on attachment 9183241 [details] [diff] [review]
1665145-openpgp-multi-file.diff

[Approval Request Comment]
Regression caused by (bug #): -
User impact if declined: Inability to import multiple public or private OpenPGP files at once.
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): moderate, as it changes the code to import public and private files and this feature is not currently covered by tests. Manual testing have been done by Magnus and I, but I recommend an extensive beta period to gather user feedback.

Comment 13

[Kai Engert [:KaiE:]]

You had decided to also cover the import of secret key, which probably was the reason why this required more changes than I had anticipated.

I think the core request of this bug has been implemented correctly. It avoids having to use the file menu and file picker dialog multiple times.

The import process treats each input file as a completely separate entity. Consequently, we bring up separate prompts for each file. I think that's acceptable.

The request from comment 3 has not yet been implemented, it would be nice to support that, too, that will require a follow-up bug.

Comment 14

[Wayne Mery (:wsmwk)]

Comment on attachment 9183241 [details] [diff] [review]
1665145-openpgp-multi-file.diff

[Triage Comment]
Approved for beta

Comment 15

[Rob Lemley [:rjl]]

Thunderbird 83.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/8b66a3104c59

Comment 16

[Kai Engert [:KaiE:]]

This improvement should be added on the 78 branch.

The code has code conflicts with the work from bug 1675342. I'd like to rebase bug 1675342 on top of this one.

Comment 17

[Kai Engert [:KaiE:]]

Comment on attachment 9183241 [details] [diff] [review]
1665145-openpgp-multi-file.diff

Copying Alessandro's earlier comments. Also note, this is a dependency for improvement patch bug 1675342.

[Approval Request Comment]
Regression caused by (bug #): -
User impact if declined: Inability to import multiple public or private OpenPGP files at once.
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): moderate, as it changes the code to import public and private files and this feature is not currently covered by tests. Manual testing have been done by Magnus and I, but I recommend an extensive beta period to gather user feedback.

Comment 18

[Wayne Mery (:wsmwk)]

Comment on attachment 9183241 [details] [diff] [review]
1665145-openpgp-multi-file.diff

[Triage Comment]
Approved for esr78

Comment 19

[Kai Engert [:KaiE:]]

78.5.1
https://hg.mozilla.org/releases/comm-esr78/rev/96ce609a4097d981b188c7e352c0fcca57d2361f