1651712 - Enable Revocation, extending expiration, and secret key backup in OpenPGP prefs

Status: RESOLVED FIXED

(MailNews Core :: Security: OpenPGP, enhancement, P1)

Description

[Kai Engert [:KaiE:]]

Alessandro, I attempted to add the code to enable the actions for revocation, extending expiration and backup of secret keys to am-e2e.js

I already did some refactoring in the base code, to allow you to share the common implementation. I'll attach a patch with my attempted changes to am-e2e.js. One of them uses a callback function to open a sub dialog, as I saw it needs to be done differently from within account prefs.

However, it doesn't work yet. After returning from the backup password dialog, I get some strange errors, about autocompletion and incorrect URIs. Maybe it's simply an async issue, but it could also be something different.

I'll attach my preparation as a starting point for you.

Please base your work on the dependent patch from bug 1651707, and there are additional base patches which you might pull in locally, and work on top of them.

Comment 1

[Kai Engert [:KaiE:]]

Attachment: patch that can be used as a starting point

Comment 2

[Alessandro Castellani [:aleca]]

I've almost completed this implementation.

I'm not sure why, but all the EnigmailDialog helper methods don't work correctly when triggered from within a subdialog.
I converted the methods that were causing failures intro regular alert and confirmEx.

There are also some UI issues here and there, which I'm fixing (eg. buttons getting pushed outside the view, missing styles, etc.)

Comment 3

[Alessandro Castellani [:aleca]]

Attachment: 1651712-openpgp-options.diff

Implemented all the methods, polished the UI of the used dialogs, and integrated successful notifications inline the account settings.

As you may notice, I didn't end up using the shared methods you created since those use the window.openDialog() method, which doesn't work in the context of the Account Settings tab.

Comment 4

[Alessandro Castellani [:aleca]]

Attachment: 1651712-openpgp-options.diff

Forgot to add a newly generated file.

Comment 5

[Richard Marti (:Paenglab)]

Comment on attachment 9163674 [details] [diff] [review]
1651712-openpgp-options.diff

Looks good. Only the icon isn't centred. Screenshot follows.

Are you planning to convert all dialogs that now open as normal dialogs to sub-dialogs?

Comment 6

[Richard Marti (:Paenglab)]

Attachment: icon-not-centred.png

The info icon isn't centred.

Comment 7

[Alessandro Castellani [:aleca]]

Thanks, I forgot to center align that.

Are you planning to convert all dialogs that now open as normal dialogs to sub-dialogs?

I wish but we can't for now. Those dialogs are shared between the account settings and the key manager, which opens as a regular dialog from other locations like the app menu.

I'd like to keep that as a low level objective, so if we can convert all those to subdialog and get rid of the repetitions and duplciate embedded styles. That would mean that when the key manager is called from other locations, we would open the account settings tab for the current identity and trigger the key manager subdialog.
I'll explore that option on bug 1652537.

Comment 8

[Alessandro Castellani [:aleca]]

Attachment: 1651712-openpgp-options.diff

Comment 9

[Kai Engert [:KaiE:]]

Key backup from within the account doesn't work, console says:

Uncaught (in promise) TypeError: window.browsingContext is undefined
    openPgpExportSecretKey chrome://messenger/content/am-e2e.js:1281

  window.browsingContext.topChromeWindow.openDialog(
    "chrome://openpgp/content/ui/backupKeyPassword.xhtml",
    "",
    "dialog,modal,centerscreen,resizable",
    args
  );

Comment 10

[Kai Engert [:KaiE:]]

You allow revoking the currently selected key.
If that is done, the revoked key will be removed from the list of keys that is shown (expected).
However, you don't update the config, the header line still has a green checkmark and says it's using the key ID that has just been revoked.

Comment 11

[Alessandro Castellani [:aleca]]

Ah, good point. I'll prevent it the same way I did if trying to delete the selected key.

Comment 12

[Alessandro Castellani [:aleca]]

Attachment: 1651712-openpgp-options.diff

This is only for trunk and 79.
The user can't revoke the currently used key and gets prompted with an alert.

Comment 13

[Alessandro Castellani [:aleca]]

Attachment: 1651712-openpgp-options-78.diff

Same patch, but compatible with 78
m-c changed the way it handles parent windows from the settings tab for 79+

Comment 14

[Kai Engert [:KaiE:]]

So the menus offer an action, but when pressed, you refuse the action.
Maybe that's not ideal.

If you really want to prevent the action, then I think the error message should explain what the user has to do to fix it, like "select a different key, or select none, then come back and try again". I'm OK with that, but but that's complicated, right?

However, it seems be fine to allow the user to delete the currently selected key. You could show an additional warning (first) "warning, you are deleting the key that you are currently using for OpenPGP, are you sure?" If the user confirms, then you could change the selection to None.

A stronger argument applies for revoke. The user's motivation might be more urgent. When "deleting" the user mightly try to clean up. But when "revoking", the user might be potentially in an emergency situation. Blocking the action that the user really wants to do quickly might frustrate the user.

Again, you could potentially show an initial extra warning "warning, you are revoking the key that you are actively using for this account are you sure?"

But showing a sequence of two warning dialogs isn't ideal either, right?

Please let me know which one you prefer

(a) keep current patch as is
(b) keep current patch, but add explanation that user must select "None", then try the action again
(c) show an additional warning "confirm action on selected key", on confirmation, show the existing warning "confirm action", and on confirmation, automatically switch selection to "None".

Comment 15

[Kai Engert [:KaiE:]]

(d) No additional warning. Show only the usual warning to confirm the delete/revoke action, and automatically switch selection to "None"

Comment 16

[Kai Engert [:KaiE:]]

FYI, I'm OK to keep (a) for now. Just wanted to bring up these thoughts.

Comment 17

[Alessandro Castellani [:aleca]]

I'd prefer preventing those actions through the alert, and extending the error message by suggesting to change key if the user wants to proceed.
Everything else adds an extra layer of complexity I'd like to avoid right now.

Comment 18

[Alessandro Castellani [:aleca]]

Attachment: 1651712-openpgp-options.diff

Comment 19

[Alessandro Castellani [:aleca]]

Let me know about this, and if approved I'll upload the 78 variation

Comment 20

[Alessandro Castellani [:aleca]]

Attachment: 1651712-openpgp-options.diff

Patch updated based on our chat on matrix.
Using the same callback for both backup export, and using a boolean variable to confirm the password was properly defined.

Comment 21

[Kai Engert [:KaiE:]]

Comment on attachment 9164189 [details] [diff] [review]
1651712-openpgp-options.diff

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

Thanks. This looks good. I suggest two changes.

r=kaie

::: mail/extensions/am-e2e/am-e2e.js
@@ +958,5 @@
> +async function enigmailRevokeKey(key) {
> +  // Interrupt if the selected key is currently being used.
> +  if (key.keyId == gIdentity.getUnicharAttribute("openpgp_key_id")) {
> +    let [alertTitle, alertDescription] = await document.l10n.formatValues([
> +      { id: "delete-key-in-use-title" },

optional nit: rename "delete-key-in-use-title" to "key-in-use-title"

@@ +1288,5 @@
> +    file: outFile,
> +    fprArray: [keyFpr],
> +  };
> +
> +  window.browsingContext.topChromeWindow.openDialog(

I suggest to use dynamic code, that will allow us to use the same code on both 78 and 79+ branches, at least while we're trying to keep both in sync.

  let w;
  if ("browsingContext" in window) {
    // 79+
    w = window.browsingContext.topChromeWindow;
  } else {
    // 78
    w = window.docShell.rootTreeItem.domWindow;
  }

  w.openDialog(

Comment 22

[Kai Engert [:KaiE:]]

And thanks for fixing the "change expiration date" button.

Comment 23

[Kai Engert [:KaiE:]]

Attachment: 1651712-openpgp-options-portable.diff

I've applied the small changes, and tested that they work on both nightly and 78 builds.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9c81e7406889
Enable Revocation, extending expiration, and secret key backup in OpenPGP prefs. r=kaie

Comment 25

[Kai Engert [:KaiE:]]

Comment on attachment 9164665 [details] [diff] [review]
1651712-openpgp-options-portable.diff

Important UI enhancement and string changes for OpenPGP

Comment 26

[Wayne Mery (:wsmwk)]

Comment on attachment 9164665 [details] [diff] [review]
1651712-openpgp-options-portable.diff

Approved for esr78 and beta - please land

Comment 27

[Kai Engert [:KaiE:]]

https://hg.mozilla.org/releases/comm-esr78/rev/3fa45718c2ecaea0d1a309e4a899e9843f45955b
https://hg.mozilla.org/releases/comm-beta/rev/0b91b40fc326560b2b4d77b8dc97d239fe1fa0ce

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e5af7184777e
followup - fix linting. rs=eslint

Comment 29

[Kai Engert [:KaiE:]]

Attachment: fix-lint.patch

lint fix

Comment 30

[Wayne Mery (:wsmwk)]

Comment on attachment 9164701 [details] [diff] [review]
fix-lint.patch

Approved for beta and esr78

Comment 31

[Kai Engert [:KaiE:]]

https://hg.mozilla.org/releases/comm-esr78/rev/052651b83953ae12490111b55bd0f7bb65bbd153
https://hg.mozilla.org/releases/comm-beta/rev/58a6f8058c44ee45d2ad2cd4c33175fcef9cd5d1

Comment 32

[Alessandro Castellani [:aleca]]

Thanks for updating and landing this.