Closed Bug 1675272 Opened 11 months ago Closed 10 months ago

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

Categories

(MailNews Core :: Security: OpenPGP, enhancement)

Thunderbird 84
enhancement

Tracking

(thunderbird_esr78 wontfix)

RESOLVED FIXED
84 Branch
Tracking Status
thunderbird_esr78 --- wontfix

People

(Reporter: lasana, Assigned: lasana)

Details

Attachments

(1 file, 4 obsolete files)

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.

poor workaround:

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

Attached patch bug1675272.patch (obsolete) — Splinter Review

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.

Attachment #9186964 - Flags: review?(alessandro)
Status: NEW → ASSIGNED
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.
Attachment #9186964 - Flags: review?(alessandro) → feedback+

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

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.

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.

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.

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

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
Attachment #9186964 - Flags: feedback+ → review+

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

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
    }
Attached patch bug1675272.patch (obsolete) — Splinter Review

Like this?

Attachment #9186964 - Attachment is obsolete: true
Attachment #9187003 - Flags: review?(alessandro)
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
Attachment #9187003 - Flags: review?(alessandro)

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?

(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)?

Flags: needinfo?(patrick)

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.

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.

Flags: needinfo?(mkmelin+mozilla)

(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.

Flags: needinfo?(patrick)
Attached patch bug1675272v2.patch (obsolete) — Splinter Review

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.

Attachment #9187003 - Attachment is obsolete: true
Attachment #9187194 - Flags: review?(mkmelin+mozilla)

(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.

Flags: needinfo?(mkmelin+mozilla)
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
Attachment #9187194 - Flags: review?(mkmelin+mozilla) → review-
Attached patch bug1675272v3.patch (obsolete) — Splinter Review

Renamed added functions, updated fluent id.

Attachment #9187194 - Attachment is obsolete: true
Attachment #9187375 - Flags: review?(mkmelin+mozilla)

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 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.
Attachment #9187375 - Flags: review?(mkmelin+mozilla) → review+

Id -> ID

Attachment #9187375 - Attachment is obsolete: true
Attachment #9187663 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9187663 [details] [diff] [review]
bug1675272v4.patch

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

Great! r=mkmelin
Attachment #9187663 - Flags: review?(mkmelin+mozilla) → review+
Target Milestone: --- → 84 Branch

Pushed by mkmelin@iki.fi:
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

Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.