1652537 - [OpenPGP] Improve the UI and UX of Key Manager

Status: RESOLVED FIXED

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

Description

[Alessandro Castellani [:aleca]]

The OpenPGP Key Manager needs a round of improvements to adapt the UI and general usability approach to the current standard of Thunderbird.

Here's a list of actionable items:

• Rename from "OpenPGP Key Management" to "OpenPGP Key Manager".
• Open as SubDialog in the preferences.
• Integrated the accountManage.css to inherit proper styling.
• Remove the menu bar and convert all those menu items to buttons inside the dialog.
• Improve the filtering and search UI.

Comment 1

[Alessandro Castellani [:aleca]]

Attachment: Key Manager.png

Here's the proposed UI for the refreshed key manager.

Opening it as a subdialog of the account settings will allow us to more easily reload the keys if anything changes.
Adding an extra "Status" column will better communicate the status of a key, instead of only using bold, italic, and grey colours as of right now.
When selecting a key, the buttons to the right will be enabled, mimicking what we have in the e2e account settings section to keep things consistent.
If the user selects multiple keys, the Key Properties button will remain disabled, and the options in the More menulist will update accordingly.
Splitting the list between Public and Private keys will help the user better distinguish them, and simplifies the necessity of handling exports and imports when selecting multiple keys.

Comment 2

[Kai Engert [:KaiE:]]

I think we currently use either the term "secret key" or "personal key". We should avoid introducing yet another term, so I'd prefer to avoid adding "Private key".

We are using the terms "secret key" (which is a technical term that is always correct, as soon as you own the secret key") and we also use the term "personal key" (which in our definition, is a secret key that you have confirmed to be your own key, created by yourself, in your identity).

If you explicitly label that category, we must find a term matches both categories. Previously we had avoided having to do that.

Comment 3

[Kai Engert [:KaiE:]]

Your top label says manage your "public keys from all your identities". The public keys of other people are completely unrelated to your identities, so I think this isn't a good general label.

Comment 4

[Kai Engert [:KaiE:]]

I'd change "refresh key cache" to "refresh list of keys"

Comment 5

[Kai Engert [:KaiE:]]

I'd change the header "Private Keys" to "Your personal/secret keys" to indicate that this list includes both, and doesn't require us to limit it one term.

I'd change the header "Public Keys" to "Public Keys of other people". Or shorter, "People's public keys".

For example we have a similar dialog for S/MIME, the certificate manager. In that window the equivalent headers use "Your Certificates" (it doesn't have to distinguish between personal/secret) and "People".

Comment 6

[Kai Engert [:KaiE:]]

I'd recommend to delay the complete rework of this dialog to a later time. I consider it a high risk to make this change at this time, given that we have only little time left for string changes. I'd prefer starting with incremental improvements, such as the suggestions from bug 1653813.

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 9164851 [details]
Key Manager.png

I think we can simply remove the "Manage all the saved..... " header. It doesn't seem to add too much value. 

Otherwise looks pretty good, clearly much more understandable than the original. 

The "Add private key..." seems misplaced. Shouldn't it be at the bottom?

If we don't want to use "Private keys", then maybe simply "Secret Keys", so that we don't make the labels overly long and confusing. 
Or maybe we should consider wholesale changing "secret key" to "private key". I think that's the more common terminology. E.g. reading up on wikipedia about PGP you don't find *any* reference to "secret key" nor "personal key". 

About the buttons, maybe importing public and private keys should be the only main buttons. Then there could be a More button for things like importing revocations. I don't think any average user would have *any* idea about that that is.

Comment 8

[Richard Marti (:Paenglab)]

Comment on attachment 9164851 [details]
Key Manager.png

Looks good for me. +1 for moving the Revocation button into the "More" popup like it's in the main Account manager UI for the Private key.

Comment 9

[Renata Gegaj]

What is the search bar for, can a user search for all available keys?
I think the "Add a Private Key" next to it gives the impression that you can only search for private keys. Also, it's a common pattern to have a "Search" button next to the search bar, therefore the add key button feels misplaced.

I'd suggest to move the button and leave just the search bar. If the functionality is just for a certain type of key then specify it, for example, smth like "Search Public Key".

+1 for Import Revocation to go to "More", if it's a feature not often used.

Which pop up does the "Add Private Key" trigger?
If it triggers the same pop up the "Add Key..." does, on the main settings screen, I'd suggest you use the same terms "Add Private Key..." on both.
The inconsistency of the button description can lead to confusion. "Add Private Key" has a key button while "Import Public Keys" doesn't and it's in plural. Does that mean I can only import multiple keys?
The same goes for the words "Add" and "Import".
The "Import Public Keys" looks like it's a dropdown, I was expecting it to be a button instead. What options will it show?

Suggestion: I'm not sure how this would perform, I't hard to give exact feedback on a LoFi mockup, without seeing the functionalities but I'll leave it here anyway.
Basically, we could use two modes here: "Public Keys" and "Private Keys" and have the corresponding buttons appear accordingly. For example, when users are on the "Public Keys" tab, they will see the "Import Public Key" option but when they are on "Private Keys" they would see "Add Private Key...". That would eliminate the confusion and clean up space a bit. But again, this is just an idea, I don't have enough context to judge that at this point.

Comment 10

[Kai Engert [:KaiE:]]

I'd like to ask to start by changing the import secret key part to use the new wizard. It's important that we have import handle the "treat as personal key" attribute. Please do this with a higher priority than other reorganization. Thanks.

Comment 11

[Andreas]

I like the overall look of the proposed new Key Manager. Just checked how the S/MIME Certificate Manager handles things. Maybe you should have an eye on that also and keep the usage of both managers consistent. In the S/MIME Manager, the buttons to interact with a selected certificate as well as a button to import new items are placed below the list. However, it might be an enhancement to have a better visual separation between Import and Add Key on the one hand (applies to new items) and Key Properties and More on the other hand (applies to items already existing in the Manager).
Layout might be:
Public Keys tab
Import Public Key(s) --------- Key Properties -- More ------------- Further Actions

Secret Keys tab
Add Secret Key --------- Key Properties -- More ------------- Further Actions

I don't know an appropriate expression for the menu where reload cache and import revocations would be placed, so please ignore the name "Further Actions".
I would also add the column "Expires On", which exists in the S/MIME manager and can be added in the current OpenPGP manager.

Comment 12

[Alessandro Castellani [:aleca]]

What is the search bar for, can a user search for all available keys?

It filters the listed keys when the user types, showing only the matches.

I think the "Add a Private Key" next to it gives the impression that you can only search for private keys. Also, it's a common pattern to have a "Search" button next to the search bar, therefore the add key button feels misplaced.
I'd suggest to move the button and leave just the search bar. If the functionality is just for a certain type of key then specify it, for example, smth like "Search Public Key".

Indeed, that button will most likely be at the bottom right of this dialog.

Which pop up does the "Add Private Key" trigger?
If it triggers the same pop up the "Add Key..." does, on the main settings screen, I'd suggest you use the same terms "Add Private Key..." on both.

Yes it does, and sounds good.

The "Import Public Keys" looks like it's a dropdown, I was expecting it to be a button instead. What options will it show?

Public Keys can be imported from file, via URL, or if stored in the clipboard.

Basically, we could use two modes here: "Public Keys" and "Private Keys" and have the corresponding buttons appear accordingly. For example, when users are on the "Public Keys" tab, they will see the "Import Public Key" option but when they are on "Private Keys" they would see "Add Private Key...". That would eliminate the confusion and clean up space a bit. But again, this is just an idea, I don't have enough context to judge that at this point.

I was exploring this idea as well.
I'll do some more iterations after I complete some technical rework that has a bit of a higher priority.

Thanks for the feedback, the next mock-up will be more refined.

Comment 13

[Alessandro Castellani [:aleca]]

Attachment: 1652537-key-manager-0.diff

First rough patch in order to implement the basic functionalities and land this before 78.1
This is what I did:

• Rename "Key Management" to "Key Manager".
• Open the new Key Wizard to generate or import a key form the Key Manager.
• Reload key list when necessary.
• Redirect to the e2e section if the user selects OpenPGP Key Manager from the menu bar or app menu.

Current issues and shortcomings of this patch

• Since the manager is not a subdialog, the menubar looks pretty bad, but I guess it's acceptable unless I can get another couple of days to do the full conversion.
• Also, the key wizard and key dialog need to be opened are regular dialogs and not subdialogs, resulting in visual inconsistencies and not great interface.
• The "Display all keys by default" checkbox doesn't work and prompts the error: chrome://openpgp/content/ui/enigmailKeyManager.js, line 1673: TypeError: initHint.showPopup is not a function. This happens also without my patch, so it's a pre-existing issue. Kai, are you able to fix it?

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 9165605 [details] [diff] [review]
1652537-key-manager-0.diff

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

I'm not sure this works out. You open the e2e section of one of the account in the account manager instead of the Key Manager. It doesn't work if account settings are already open, but even when they are not, if you have many accounts, which one should it select... This is all quite confusing.

Not strictly this bug, but I think we should drop the "Manage OpenPGP Keys" header in the account settings. It's now on the same level as the technologies. And for some reason a legend in the middle there - fieldsets can really only have one legend.
https://searchfox.org/comm-central/rev/9f453dd597f0b985b8fe7689b239e3b70de7749d/mail/extensions/am-e2e/am-e2e.inc.xhtml#99

::: mail/extensions/openpgp/content/modules/commandLine.jsm
@@ +20,5 @@
>  
>  function Handler() {}
>  
>  Handler.prototype = {
> +  classDescription: "Enigmail Key Manager CommandLine Service",

OpenPGP instead of Enigmail

::: mail/extensions/openpgp/content/strings/enigmail.ftl
@@ +292,5 @@
>  openpgp-key-edit-date-title = Extend Expiration Date
>  
>  openpgp-manager-legend = Manage OpenPGP Keys
>  
> +openpgp-manager-description = Use the Key Manager dialog to view and manage public keys of your correspondents and all other keys not listed above.

would remove the "dialog" part of this. Not sure it would always remain a dialog

Comment 15

[Kai Engert [:KaiE:]]

Comment on attachment 9165605 [details] [diff] [review]
1652537-key-manager-0.diff

Patch doesn't work for me.

I use the menu, but no window opens.

In a debug build in the console I see this error:

NS_ERROR_XPC_SECURITY_MANAGER_VETO: 3 browsing-context.js:914
    _notifyDocShellDestroy resource://devtools/server/actors/targets/browsing-context.js:914
    _onDocShellDestroy resource://devtools/server/actors/targets/browsing-context.js:808
    observe resource://devtools/server/actors/targets/browsing-context.js:771
    observe resource://devtools/server/actors/targets/parent-process.js:106

Comment 16

[Kai Engert [:KaiE:]]

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

• Redirect to the e2e section if the user selects OpenPGP Key Manager from the menu bar or app menu.

I disagree.
The key manager is more powerful than the subset of commands offered inside account settings.

If you want to manage keys, it's not clear why you would want to go to account settings.

Comment 17

[Kai Engert [:KaiE:]]

Ok, I discovered that I can still open the key manager window, if I click it from within the account settings.
Please keep the ability to open the key manager from the menu directly. "Tools Key Manager" has nothing to with account settings, IMHO.

File Import Secret Key -> this jumps directly to the import section of the wizard -> good

Menu Generate Key -> this currently jumps to the main entry of the wizard, which asks "create/import". I think this should jump directly to the "import" section of the wizard.

Comment 18

[Kai Engert [:KaiE:]]

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

Not strictly this bug, but I think we should drop the "Manage OpenPGP Keys"
header in the account settings. It's now on the same level as the
technologies.

Agreed. Drop heading "Manage OpenPGP Keys".

However, keep the the sentence "Use the Key Manager to view and manage public keys ..."

Comment 19

[Kai Engert [:KaiE:]]

Key manager window has a "OK" button at the buttom. IMHO either remove the button, or change it to "Close"

Comment 20

[Kai Engert [:KaiE:]]

Menu command "file close" in key manager does nothing.

Comment 21

[Kai Engert [:KaiE:]]

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

• The "Display all keys by default" checkbox doesn't work and prompts the error: chrome://openpgp/content/ui/enigmailKeyManager.js, line 1673: TypeError: initHint.showPopup is not a function. This happens also without my patch, so it's a pre-existing issue. Kai, are you able to fix it?

Thanks, I probably never played with that control before. It seems broken, even in the latest Enigmail release. We should remove it. I'll clean that up in a separate patch. I also want to check the "view untrusted keys" item, maybe we remove that, too, because our code works differently. The new correct mechanism would be "view keys that are not accepted". But that filtering isn't implemented currently.

Comment 22

[Kai Engert [:KaiE:]]

Attachment: remove-all-default-and-untrusted.patch

Comment 23

[Alessandro Castellani [:aleca]]

Attachment: 1652537-key-manager-0.diff [checked in]

Thank you both for the detail feedback.
Here's an updated patch:

• I merged your patch to remove the checkbox pref, thanks.
• Now the Key Manager can be opened from the menu items.
• "File > Close" fixed.
• Dialog button updated to "Close"
• Removed the "Manage OpenPGP Keys" legend.
• "Menu > Generate Key" opens the Key Wizard directly on the create section.
• Added temporary inline style to remove the spacing around the menubar.

Comment 24

[Kai Engert [:KaiE:]]

Comment on attachment 9165724 [details] [diff] [review]
1652537-key-manager-0.diff [checked in]

Thanks! r+

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5fffded97b81
Use the new Key Wizard, update strings, and reload key cache in OpenPGP Key Manager. r=KaiE

Comment 26

[Kai Engert [:KaiE:]]

Comment on attachment 9165724 [details] [diff] [review]
1652537-key-manager-0.diff [checked in]

Important correctness and completeness fixes for OpenPGP UI

Comment 27

[Wayne Mery (:wsmwk)]

Comment on attachment 9165724 [details] [diff] [review]
1652537-key-manager-0.diff [checked in]

Approved for beta
Approved for esr78

Comment 28

[Kai Engert [:KaiE:]]

https://hg.mozilla.org/releases/comm-esr78/rev/c29bd316396e87a8b2bc3ef4eb6764dc4ccddfe7
https://hg.mozilla.org/releases/comm-beta/rev/73f5837c5860509f7932570ea4c2eb9143be1ab3

Comment 29

[Elio Qoshi [:Elio]]

Attachment: Key Manager.png

Small UI improvement proposal for the key manager view from my side:

• Changed Add Private Key to a Primary Button
• changed revoked and expired color coding to be accessible
• Merged Import Revokation Files and Import Publik Keys into a single "Import Dropdown"

On Figma:
https://www.figma.com/file/IJ5yiOo3VqJjaoYBG6xNIS/Thunderbird-OpenPGP?node-id=115%3A4

Comment 30

[Alessandro Castellani [:aleca]]

Nice, thanks for iterating on this.
I got other feedback meanwhile, and we added a couple of extra features to the dialog that need to be added.
I'll upload another mock-up in a bit.

Comment 31

[Alessandro Castellani [:aleca]]

I'm gonna close this since the most important patch landed on 78, and then open a follow up to focus on the UI with a lower priority.