1675272 - Add "Copy Key Id" entry to OpenPGP Key Manager context menu

Status: RESOLVED FIXED

(MailNews Core :: Security: OpenPGP, enhancement)

Description

[Lasana Murray]

Sometimes, at least during testing, I need to copy the id of a key to use elsewhere. For example; to include in a test, a file name or when using gpg on the command line.

Including this option may also be convenient to more advanced users of openpgp.

Comment 1

[Kai Engert [:KaiE:]]

poor workaround:

double click, select last 4x4 characters of fingerprint (which is the same as the key id)

Comment 2

[Lasana Murray]

Attachment: bug1675272.patch

Simple patch, no test, tested manually.

As I understand it, the key manager is set to be redesigned once the details are worked out and this change may no longer be relevant. Still, I think it's a convenient feature to have in the meantime.

Comment 3

[Alessandro Castellani [:aleca]]

Comment on attachment 9186964 [details] [diff] [review]
bug1675272.patch

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

This is a good addition.
We should implement this also in the e2ee page in the list of private keys.
Every private key has a "more" menu with the same options of the context menu of the Key Manager.
We should add the same "Copy >" submenu to that popup menu.

Comment 4

[Lasana Murray]

That will have to be manually added somewhere in this function?
https://searchfox.org/comm-central/source/mail/extensions/am-e2e/am-e2e.js#672

Comment 5

[Alessandro Castellani [:aleca]]

Yes, specifically here: https://searchfox.org/comm-central/rev/93bfded213518bb71f465d6fa17baaafafb9a846/mail/extensions/am-e2e/am-e2e.js#945

This is where the menupopup is generated and all the menu items created.

Comment 6

[Magnus Melin [:mkmelin]]

Well for the e2ee page, it could be preferable to just make the key id selectable - so one can copy paste it like any normal text. I don't know why it's not selectable atm. I think we should add context menus to do things that can be done through other means (select+ctrl+c) - otherwise next we get overwhelmed by trivial context menus. Here, e.g. not clear the fingerprint wouldn't deserve the same treatment, as it's even more important.

Comment 7

[Alessandro Castellani [:aleca]]

The fingerprint is already selectable in the e2ee page. I guess we could add another row with the selectable key id.
Currently that's not selectable as it's used as the clickable label for the radio button, and we shouldn't change that.

The addition of a context menu item definitely makes sense for the Key Manager since we handle the selection and copying of multiple keys at once.
For the e2ee page, we could consider a simpler approach if you think we shouldn't add the context menu item also there.
I think we should to present the same amount of options in both locations, but I'm okay with postponing this to a follow up bug if you think otherwise.

Comment 8

[Magnus Melin [:mkmelin]]

Having it clickable shouldn't prevent the id from being selectable. Yes, let's only do the key manager at least for now.

Comment 9

[Alessandro Castellani [:aleca]]

Comment on attachment 9186964 [details] [diff] [review]
bug1675272.patch

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

In that case, this gets an r+ as it covers the objective of this bug.
r=aleca

Comment 10

[Alessandro Castellani [:aleca]]

Can you update the commit message to add r=aleca at the end?

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9186964 [details] [diff] [review]
bug1675272.patch

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

Sorry for the driveby, I think there's some things that should be fixed before landing.

::: mail/extensions/openpgp/content/ui/enigmailKeyManager.js
@@ +724,5 @@
> + * Places the key id of each key selected onto the clipboard.
> + */
> +async function enigmailCopyKeyIds() {
> +  let enigmailSvc = GetEnigmailSvc();
> +  if (!enigmailSvc) {

You're in the OpenPGP key manager, so this will never be null

@@ +730,5 @@
> +  }
> +
> +  let ids = getSelectedKeyIds();
> +  if (ids.length === 0) {
> +    let value = await document.l10n.formatValue("no-key-selected");

I don't think this can ever happen. If no key is selected,  the menu item should be disabled (and seems to be). And in context menus we don't show items that can't be used.

@@ +735,5 @@
> +    EnigmailDialog.info(window, value);
> +    return;
> +  }
> +
> +  EnigmailClipboard.setClipboardContent(ids.join(" "));

would \n be better?

Also, let's have the copied ids 0x prefixed, we should really fix it in the display as well. 
Finally, if you're adding copy key id, might as well add the fingerprint too.

::: mail/extensions/openpgp/content/ui/enigmailKeyManager.xhtml
@@ +204,5 @@
> +      <menu id="ctxmenu-copy"
> +            data-l10n-id="openpgp-key-man-ctx-copy">
> +        <menupopup id="ctxmenu-copy-popup">
> +          <menuitem data-l10n-id="openpgp-key-man-ctx-copy-key-ids"
> +                id="ctxCopyKeyIds"

please always put the id first (yes it's wrong in many places in this old code)

::: mail/locales/en-US/messenger/openpgp/openpgp.ftl
@@ +136,5 @@
> +    .label = Key Id(s)
> +    .accesskey = K
> +
> +openpgp-key-man-ctx-copy-public-keys =
> +    .label = Public Key(s)

The copy public key doesn't seem to work for multiple keys. And I'm not sure it makes sense for it to do so either.

Anyway, this should not use Key(s) but translate properly with Fluent, I think like

  .label =  { $count ->
        [one] Public Key
       *[other] Public Keys
    }

Comment 12

[Lasana Murray]

Attachment: bug1675272.patch

Like this?

Comment 13

[Alessandro Castellani [:aleca]]

Comment on attachment 9187003 [details] [diff] [review]
bug1675272.patch

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

Let's go through the suggestions left by Magnus on Comment 11

Comment 14

[Lasana Murray]

You're in the OpenPGP key manager, so this will never be null

I saw this in another function not sure if it's to circumvent some kind of bug so I used it here too.

I don't think this can ever happen. If no key is selected, the menu item should be disabled (and seems to be). And in context menus we don't
show items that can't be used.

The way the context menu is configured right now, the entry is available even if no keys are selected. This could probably be improved in a follow up bug if desirable.

would \n be better?

Not sure. That may be kind of weird, pasting text that spans multiple lines I was thinking you may want to paste them in a bash script (as a list) or spreadsheet or something.

Finally, if you're adding copy key id, might as well add the fingerprint too.

Sure... Which display are your referring to? The e2ee page or something in this patch?

The copy public key doesn't seem to work for multiple keys. And I'm not sure it makes sense for it to do so either

I'm looking into that but got a little side tracked. Should I still try to fix it?

Comment 15

[Magnus Melin [:mkmelin]]

(In reply to Lasana Murray from comment #14)

You're in the OpenPGP key manager, so this will never be null

I saw this in another function not sure if it's to circumvent some kind of bug so I used it here too.

It's for when openpgp is disabled, but the you would not get the key manager either.

I don't think this can ever happen. If no key is selected, the menu item should be disabled (and seems to be). And in context menus we don't
show items that can't be used.

The way the context menu is configured right now, the entry is available even if no keys are selected. This could probably be improved in a follow up bug if desirable.

I don't see that. How do you get a context menu without a context? As soon as I right click to get the context menu, I also get a selection.

would \n be better?

Not sure. That may be kind of weird, pasting text that spans multiple lines I was thinking you may want to paste them in a bash script (as a list) or spreadsheet or something.

Conceptually, what you would output is a list. Lists in plain text is a set of lines.

Finally, if you're adding copy key id, might as well add the fingerprint too.

Sure... Which display are your referring to? The e2ee page or something in this patch?

Only the context in the key manager you're touching yes.

The copy public key doesn't seem to work for multiple keys. And I'm not sure it makes sense for it to do so either

I'm looking into that but got a little side tracked. Should I still try to fix it?

I don't know about that, maybe it should always be for the one key only. I can't think of a case where you'd want to multi-select and copy paste all those public keys to the clipboard, though maybe such cases exist. Patrick, do you think that's something worth keeping (well, fixing, since it doesn't work)?

Comment 16

[Lasana Murray]

I don't see that. How do you get a context menu without a context? As soon as I right click to get the context menu, I also get a selection.

Right click on an empty area of the tree view, then select the "Copy Public Keys..." operation. You will get an alert saying "You should select at least one key in order to perform the selected operation". An outline is around the first key but it does not look like it's actually selected.

Comment 17

[Lasana Murray]

This seems to be using a <broadcaster> api to disable the copy operation in the dropdown menu.

https://developer.mozilla.org/en-US/docs/Archive/Mozilla/XUL/broadcaster

Reminds me a little of AngularJS. Do we still use this pattern? I'd prefer something more explicit.

Comment 18

[Patrick Brunschwig]

(In reply to Magnus Melin [:mkmelin] from comment #15)

I don't know about that, maybe it should always be for the one key only. I can't think of a case where you'd want to multi-select and copy paste all those public keys to the clipboard, though maybe such cases exist. Patrick, do you think that's something worth keeping (well, fixing, since it doesn't work)?

There are use-cases when you want to be able to copy a public key. For example to upload a key to a keyserver (since TB currently only supports keys.openpgp.org, the only quick alternative is copy & paste). But I only see a valid case for a single key, not if multiple keys are selected.

Comment 19

[Lasana Murray]

Attachment: bug1675272v2.patch

Made a few changes; removed enigmailSvc checks, disable the context menu entries when no keys are selected, changed the key ids to be prefixed with 0x, added an option to export fingerprints.

Comment 20

[Magnus Melin [:mkmelin]]

(In reply to Lasana Murray from comment #17)

This seems to be using a <broadcaster> api to disable the copy operation in the dropdown menu.

https://developer.mozilla.org/en-US/docs/Archive/Mozilla/XUL/broadcaster

Reminds me a little of AngularJS. Do we still use this pattern? I'd prefer something more explicit.

Broadcasters should not be used no. Removed the usage in bug 1489447, but yeah, looks like Enigmail brought some back.

Comment 21

[Magnus Melin [:mkmelin]]

Filed bug 1676727

Comment 22

[Magnus Melin [:mkmelin]]

Comment on attachment 9187194 [details] [diff] [review]
bug1675272v2.patch

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

Works fairly nicely, but ...

::: mail/extensions/openpgp/content/ui/enigmailKeyManager.js
@@ +243,5 @@
>        }
>      }
>    }
>  
> +  // Disable the context menu entries if no keys are selected.

Seems this function is used both for the main menu and the context menu, which is a bit bad. Should not show the context menu at all if there is no context. (Return false does it IIRC)

@@ -557,5 @@
>  function dlgOpenCallback(dlgUri, args) {
>    window.openDialog(dlgUri, "", "dialog,modal,centerscreen,resizable", args);
>  }
>  
> -async function enigmailExportKeys(which) {

It is, really async. backupSecretKeysInteractive e.g. is async, just not awaited

@@ +706,5 @@
>  
> +/**
> + * Places the fingerprint of each selected key onto the keyboard.
> + */
> +function enigmailCopyFprs() {

We should stop using enigmail in the function names. Just copyOpenPGPFingerPrints()?

@@ +716,5 @@
> +
> +/**
> + * Places the key id of each key selected onto the clipboard.
> + */
> +function enigmailCopyKeyIds() {

coypOpenPGPKeyIds()

::: mail/locales/en-US/messenger/openpgp/openpgp.ftl
@@ +134,1 @@
>  openpgp-key-man-copy-to-clipbrd =

unfortunately, changing the label, means you have to change the key (openpgp-key-man-copy-to-clipbrd) so that localizers would notice

Comment 23

[Lasana Murray]

Attachment: bug1675272v3.patch

Renamed added functions, updated fluent id.

Comment 24

[Lasana Murray]

It is, really async. backupSecretKeysInteractive e.g. is async, just not awaited

backupSecretKeysInteractive() is marked async but does not appear to await anything or return a Promise either, it's also using formatValueSync. I'll clean this up in another bug this patch already became more than I expected.

https://searchfox.org/comm-central/source/mail/extensions/openpgp/content/modules/keyRing.jsm#586

Comment 25

[Magnus Melin [:mkmelin]]

Comment on attachment 9187375 [details] [diff] [review]
bug1675272v3.patch

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

::: mail/locales/en-US/messenger/openpgp/openpgp.ftl
@@ +153,5 @@
> +    .accesskey = F
> +
> +openpgp-key-man-ctx-copy-key-ids =
> +    .label = { $count -> 
> +               [one] Key Id

Sorry forgot to mention earlier, but I think we use Key ID (uppercase) elsewhere in the UI.

Comment 26

[Lasana Murray]

Attachment: bug1675272v4.patch

Id -> ID

Comment 27

[Magnus Melin [:mkmelin]]

Comment on attachment 9187663 [details] [diff] [review]
bug1675272v4.patch

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

Great! r=mkmelin

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4b19adc64fc2
Add entries for copying key ids and fingerprints in the openpgp key manager context menu. r=aleca,mkmelin