Add "Copy Key Id" entry to OpenPGP Key Manager context menu
Categories
(MailNews Core :: Security: OpenPGP, enhancement)
Tracking
(thunderbird_esr78 wontfix)
Tracking | Status | |
---|---|---|
thunderbird_esr78 | --- | wontfix |
People
(Reporter: lasana, Assigned: lasana)
Details
Attachments
(1 file, 4 obsolete files)
12.12 KB,
patch
|
mkmelin
:
review+
|
Details | Diff | Splinter Review |
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•4 years ago
|
||
poor workaround:
double click, select last 4x4 characters of fingerprint (which is the same as the key id)
Assignee | ||
Comment 2•4 years ago
|
||
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.
Updated•4 years ago
|
Comment 3•4 years ago
|
||
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.
Assignee | ||
Comment 4•4 years ago
|
||
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•4 years ago
|
||
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•4 years ago
|
||
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•4 years ago
|
||
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•4 years ago
|
||
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•4 years ago
|
||
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
Assignee | ||
Updated•4 years ago
|
Comment 10•4 years ago
|
||
Can you update the commit message to add r=aleca
at the end?
Comment 11•4 years ago
|
||
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 }
Updated•4 years ago
|
Assignee | ||
Comment 12•4 years ago
|
||
Like this?
Comment 13•4 years ago
|
||
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
Assignee | ||
Comment 14•4 years ago
|
||
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•4 years ago
|
||
(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)?
Assignee | ||
Comment 16•4 years ago
|
||
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.
Assignee | ||
Comment 17•4 years ago
|
||
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•4 years ago
|
||
(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.
Assignee | ||
Comment 19•4 years ago
|
||
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•4 years ago
|
||
(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•4 years ago
|
||
Filed bug 1676727
Comment 22•4 years ago
|
||
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
Assignee | ||
Comment 23•4 years ago
|
||
Renamed added functions, updated fluent id.
Assignee | ||
Comment 24•4 years ago
•
|
||
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•4 years ago
|
||
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.
Assignee | ||
Comment 26•4 years ago
|
||
Id -> ID
Comment 27•4 years ago
|
||
Comment on attachment 9187663 [details] [diff] [review] bug1675272v4.patch Review of attachment 9187663 [details] [diff] [review]: ----------------------------------------------------------------- Great! r=mkmelin
Assignee | ||
Updated•4 years ago
|
Comment 28•4 years ago
|
||
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
Updated•4 years ago
|
Description
•