Closed Bug 1650235 Opened 4 years ago Closed 4 years ago

Add missing actions in the OpenPGP Add Key Wizard

Categories

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

enhancement

Tracking

(thunderbird_esr78 fixed, thunderbird79 fixed)

RESOLVED FIXED
Thunderbird 80.0
Tracking Status
thunderbird_esr78 --- fixed
thunderbird79 --- fixed

People

(Reporter: aleca, Assigned: aleca)

References

(Depends on 1 open bug)

Details

(Keywords: ux-efficiency)

Attachments

(4 files, 6 obsolete files)

Implement the following missing actions in the new OpenPGP Key Wizard:

  • Import Public/Secret key from File
  • Import key from URL
  • Import key from Clipboard

From a conversation with Kai, here's everything that needs to happen in the import area.

  1. The user can import multiple keys at once.
  2. The use can only import Secret (personal) keys in the new Key Wizard.
  3. If the user needs to import Public keys (from other users), we redirect him to the Key Manager.
  4. Show a list of all the selected keys before the actual import.
  5. Ask the user if this is their personal key (for each imported key).
  6. The imported keys might be protected by a password, prompt the user if necessary.
  7. Remind the user to select a key from the Account Settings page to be used with their identity (after successful import).

Question for Kai.

In the new Key Wizard I'm listing all the keys inside the selected file(s), and alongside each key there's a checkbox with the label Use this as my Personal key.
How can I pass that selected attribute to the EnigmailKeyRing.importKeyFromFile() method, which takes care of importing the keys?
Is this something that can be done during import, or should I update that attribute after the import method returned the list of imported keys in the resultKeys object?

Flags: needinfo?(kaie)

It's necessary to do it in two steps, after EnigmailKeyRing.importKeyFromFile, please call PgpSqliteDb2.acceptAsPersonalKey(fingerprint);

The reason is keys and our additional attributes (like the acceptance) live in separate storage.

Flags: needinfo?(kaie)
Attached image Import workflow.png (obsolete) (deleted) —

Screenshots of the new workflow to import secret keys.

In the last dialog, the wording "you can specify which key should be USED as Personal Key" sounds wrong. As far as I understand, this setting just asks me to confirm that these keys are indeed my personal keys. But it doesn't ask me to set one key which will be USED in the future. This setting will be done in the account manager, not in the key properties.

True. To avoid this confusion, you could change wording "which key should be used as Personal Key" to -> "which keys MAY be used as Personal Keys".

I'd prefer a few additional words, borrowing from the key details dialog. It's about confirming:

  • the user confirms to have created these keys themselves
  • the user confirms the key's user ID belong to themselves

(In reply to Andreas from comment #5)

In the last dialog, the wording "you can specify which key should be USED as Personal Key" sounds wrong. As far as I understand, this setting just asks me to confirm that these keys are indeed my personal keys. But it doesn't ask me to set one key which will be USED in the future. This setting will be done in the account manager, not in the key properties.

That's true, this can be confusing, thanks for the feedback.

The checkbox is to allow the user to set the Acceptance of this key immediately at import, which he can do later in the Key Properties dialog, listed as:

  • No, don't use it as my personal key.
  • Yes, treat this key as a personal key.

True. To avoid this confusion, you could change wording "which key should be used as Personal Key" to -> "which keys MAY be used as Personal Keys".

Yes, it sounds better.

I'd prefer a few additional words, borrowing from the key details dialog. It's about confirming:

  • the user confirms to have created these keys themselves
  • the user confirms the key's user ID belong to themselves

We could add these 2 items as mandatory checkboxes, which the user will need to check in order to enable the actual import of the listed keys.

Attached image Screenshot from 2020-07-09 11-49-34.png (obsolete) (deleted) —

The final overview screen before the import.

Attached image Green Alert.jpg (deleted) —

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

Created attachment 9162590 [details]

The final overview screen before the import.

This looks solid! I have a few comments UI related, which should not affect usability really:

  • The pattern to treat the key as a personal key is inconsistent with the existing pattern to set your personal key I'd say. However I see why you didn't use that as the checkbox also has a label. Not anything huge usability wise, just feels a bit foreign in the visual language

  • The check mark in the alert message is quite neon, but I am not sure if that's an accessibility problem as the label beneath it explains the icon's purpose anyway.

  • I am curious if we could experiment with the green alert a bit more? Example attached.

  • Not sure if that's inconsistent with the rest of the software, but maybe "Go back" buttons can have an arrow icon pointing to the left? I am not sure how that affects RTL languages though

  • Also can there be a divider or more white space before the confirmation prompt? It feels a little bit too tight

I think that's all I can think of, thanks!

Well, if you REQUIRE the user to explicitly confirm "yes I create it" and "yes these are my identities", then having individual checkboxes for each key is redundant.

(a) Either tell the user to only tick the "may treat as personal" checkboxes for keys that they created and that their own identity.

(b) Or use the warning checkboxes, and skip the individual checkboxes. Disadvantage here is that the user wouldn't be able to make their decision for individual keys.

My preference is (a), just add the information text to your earlier suggestion.

I think I have a bit of confusion here.

The Treat this key as Personal Key checkbox is not to allow/disallow importing that key. it's only to mirror the Acceptance option.

If the user wants to import a list of secret keys where some are not his own, should we allow to only import some? That adds a bit of complexity.
This dialog is to exclusively import personal secret keys, or do we want to allow importing also 3rd party secret keys?

Because if so, we need a different approach and checkbox for each listed key.

The user should be allowed to import ALL secret keys.

It's ok to allow importing keys were the user says "no this is not mine".
(It's unusual, but no reason to forbid it.)

The intention is avoid that the user is being tricked with fake personal keys in someone else's name.

The intention is that for each imported key, the user needs to confirm "yes this is a key that I created and that is in my own name".

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

The Treat this key as Personal Key checkbox is not to allow/disallow importing that key. it's only to mirror the Acceptance option.

Correct.

If the user wants to import a list of secret keys where some are not his own, should we allow to only import some?

I think that's too complex. Let's just import all of them.

If that's the only file the user has (and for some reason, and a shared secret key of someone else is contained in the user's backup, we must not block the user from using their backup).

Having imported additional secret keys does not do harm, and might have a benefit for the user (being able to decrypt some stuff).

That adds a bit of complexity.

I just realized, you talk about "more complexitiy" because of the potential "only import a subset"? Yes, let's avoid that complexity. Let's always import them all. Just make sure we categorize correctly.

You already had separate checkboxes for each key. That's what we want. The user should confirm which keys are the user's own keys.

(And in the vast majority of scenarios, it will be just one key, and it will be the user's own key.)

This dialog is to exclusively import personal secret keys, or do we want to allow importing also 3rd party secret keys?

We want to allow importing of any secret keys.
We cannot judge why the user wants to import a key.
There might be several reasons we're not thinking of.

In theory, if a user wants to import another person's secret, the preferences pane for their own account probably isn't the ideal place. But as you said, some users might not discover the key manager, and this says "import key", so why wouldn't we allow them to import a secret key?

Because if so, we need a different approach and checkbox for each listed key.

But you already had a checkbox for each listed key?

It's fine to import them all. They might be necessary to decrypt some stuff.
Just don't use them as personal keys, if they aren't.

All right, thanks for the overview, now it makes sense.

So, let's stick with a checkbox for each key that says "Treat this key as a personal key".
And we can improve the top description with:
"Before completing the import process, you can specify which key may be used as personal key. Be sure to confirm you created the key you decide to use as personal and the listed identity belongs to you. You can later change this option in the Key Properties dialog."

Too verbose?

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

So, let's stick with a checkbox for each key that says "Treat this key as a personal key".

ok

And we can improve the top description with:
"Before completing the import process, you can specify which key may be used as personal key. Be sure to confirm you created the key you decide to use as personal and the listed identity belongs to you. You can later change this option in the Key Properties dialog."

I'd try to simplify.

"Please confirm which keys may be treated as personal keys. Only keys that you created yourself and that show your own identity should be used as personal keys. You can later change this option in the Key Properties dialog."

Could be better to include the word "your".

"Please confirm which keys may be treated as your personal keys. Only keys that you created yourself and that show your own identity should be used as your personal keys. You can later change this option in the Key Properties dialog."

Nice, I like it and I think it's clear.

Attached image Screenshot from 2020-07-09 16-09-56.png (obsolete) (deleted) —

More progress on this dialog, which now works, but I need to finish the final screen.

After the user clicks confirm, specific Password prompts are triggered in case the keys are protected by a passphrase.

After a successful import, I loop through the imported keys and match any possible "treat this as personal key" checked checkbox.

At last, I list all the imported keys with a button to open the Key Properties dialog for each keys.

Once the user closes this dialog, I trigger the reload of the cache to update the list of personal keys.

In case of error, I only print the error message in the page without clearing the list of keys, allowing the user to try again, or going back to select another file.

Attachment #9162590 - Attachment is obsolete: true
Attached image Import workflow.png (obsolete) (deleted) —

These are all the screenshots of the full workflow, for those who can't load the patch I'm uploading.

Attachment #9162334 - Attachment is obsolete: true
Attachment #9162645 - Attachment is obsolete: true
Attached patch 1650235-openpgp-wizard.diff (obsolete) (deleted) — Splinter Review
Attachment #9162701 - Flags: ui-review?(richard.marti)
Attachment #9162701 - Flags: review?(kaie)
Attached image Import workflow.png (deleted) —

Updated error messages

Attachment #9162700 - Attachment is obsolete: true

This looks good. A remaining concern is the wording on the first page. There's room for potential confusion on the user side.
I'll explain my concerns, and we can decide if further tweaks should be done, or if we can use this dialog as is.

The user has been starting from their own account configuration. The user decided to configure their own account.

Looking at the account preferences, we say "To send encrypted or signed messages, you need to configure an encryption technology."
We don't yet say HOW the user needs to do that. And that could cause new users to be uncertain what exactly they should do. They might be uncertain what "Add Key" is about.

As my first suggestion, in the introduction text that that we show on top on the "end-to-end encryption" pane, we could improve clarity by adding another sentence short sentence:

"... configure an encryption technology, either OpenPGP or S/MIME. To do so, you need to select the key or certificate that you will use for yourself, and for which you own the secret part. This is called your personal key or personal certificate."

I think this could make it much clearer what the rest of the page will be about: That it's only about personal keys/certificates, which (by definition) include a secret key.

With that, it should be clear that the shown list of existing keys will only be about personal/secret keys, and that the "Add Key" button will only be about personal/secret keys, too.

Now, the user proceeds to Add Key, selects import, and is presented the dialog from your first screenshot. In that dialog, you have text that talks about importing a personal key, but it also talks about "importing a public key that belongs to another user".

I think this is misleading. In particular, because you use the term "user". I think the correct term would be "correspondent" or "another person". The term "user" could incorrectly suggest that we are talking about another person who is using this computer. I think this part of the sentence should be dropped.

We should focus the wording on personal keys. And it could be useful to further clarify what this is about.

Other software (which the user might have used in the past, and which was potentially used to create the key that the user needs to import) might have called that kind of key differently.

Other potential labels are:

  • in previous Enigmai, some places use "key type: key pair"
  • in previous Enigmail, some places use "your secret key"
  • if you look at the contents of this type of file, you will see the text "private key"

So, my concern is, that the user might be confused by the various terms.
If the text also talks about "public keys", that could add more confusion.

For me, knowing that I'm using this dialog to work with a personal/secret/private key (pair), I'm surprised to see that the term "public" is mentioned in the first part of the sentence. But I get your intention. You're trying to guide the user, who might not know what part of Thunderbird they need to use for which kind of action.

So here is my suggestion for your first screenshot.

I'd change the headline. It currently says "Import an existing OpenPGP Key". I think this is imprecise, because here, we don't allow importing of any key. This is the place to import personal keys.

Therefore I'd change the bigger headline to "Import an existing personal OpenPGP Key".

Then you repeat the text "Import a personal key" in bold. I'd attempt to bring in more clarity.

Suggestion: "Import a personal key that you have previously backed up to a file. You may import personal keys that were created with other OpenPGP software. Other software might have described it using the alternative terms secret key, private key, own key or key pair."

My personal recommendation is to avoid the hint regarding public keys in this dialog, for the following reasons: The first reason is, the user should be aware WHERE they are right now. They are in the area that configures their own personal key. It should be clear this area isn't about importing public keys of other people. The second reason is: A personal keys includes a public key, too! Importing a personal key means, you are implicitly importing your own public key, too.

Yes, your suggested text had tried to make it clear by saying "public key that belongs to another user", but nevertheless I think it is confusing to mention that here.

Summary of my suggestion:

--- header of preferences pane ---
To send encrypted or digitally signed messages,
you need to configure an encryption technology, either OpenPGP or S/MIME.

To do so, select the key or certificate that you will use for yourself,
and for which you own the secret part. This is called your personal key or personal certificate.

Learn more...
--- end ---

(String ID e2eIntro.description)

--- first import dialog ---
headline: Import an existing personal OpenPGP Key

Import a personal key that you have previously backed up to a file.
You may import personal keys that were created with other OpenPGP software.
Other software might describe it using alternative terms such as your own key, secret key, private key or key pair.

[select file to import]
--- end ---

If you would like to hint the user that the OpenPGP key manager can be used to import the keys of other people, I think this should be instead done with a heading in the main preferences pane, above the "Manage OpenPGP Keys" button.

Currently, in the main e2ee encryption preferences, there is no separator between the list of keys and the button.

You could add the following text, after the list of keys, before the button:

"To manage keys that aren't shown here, and to manage public keys of your correspondents, use OpenPGP key management".
Rename the button label to "OpenPGP Key Management".
This would be aligned with the text that we are showing in the global Tools menu, which also uses this label.

Comment on attachment 9162701 [details] [diff] [review] 1650235-openpgp-wizard.diff I don't check wording. Kai is better for this. :-) Looks good. I have only a small aligning issue. I'll attach a screenshot. What happens when I have 20 keys to import? Are they all shown? If yes, does the dialog expand until the screen limits or is there a max-height and a scrollbar inside the dialog is shown?
Attachment #9162701 - Flags: ui-review?(richard.marti) → ui-review+
Attached image icon-not-centred.png (deleted) —

The error icon and the checkmark aren't centred/aligned with the text.

Thank you so much for the feedback.
Here's some follow up suggestions to iterate on your proposals.

To do so, select the key or certificate that you will use for yourself,
and for which you own the secret part. This is called your personal key or personal certificate.

This seems a bit verbose, I'd simplify it with:
"Select the personal key or personal certificate, for which you own the secret part, to use for either encryption technology."

headline: Import an existing personal OpenPGP Key

Import a personal key that you have previously backed up to a file.
You may import personal keys that were created with other OpenPGP software.
Other software might describe it using alternative terms such as your own key, secret key, private key or key pair.

The headline, first paragraph, and second paragraph sound too similar and a bit repetitive, since all have the "personal key" in it.
I'd simplify it with:

Import an existing personal OpenPGP Key

Select a previously backed up file.
You may import personal keys that were created with other OpenPGP software.
Other software might describe these keys by using alternative terms such as your own keys, secret keys, private keys or key pair.

You could add the following text, after the list of keys, before the button:

Agree, I'll improve that area.

Rename the button label to "OpenPGP Key Management".
This would be aligned with the text that we are showing in the global Tools menu, which also uses this label.

Wouldn't be more accurate to call it "OpenPGP Key Manager"?

Attached patch 1650235-openpgp-wizard.diff (obsolete) (deleted) — Splinter Review
Attachment #9162701 - Attachment is obsolete: true
Attachment #9162701 - Flags: review?(kaie)
Attachment #9163038 - Flags: ui-review?(richard.marti)
Attachment #9163038 - Flags: review?(kaie)
Comment on attachment 9163038 [details] [diff] [review] 1650235-openpgp-wizard.diff Looks good. Thanks. Only the icon of the "Add Key" button needs some spacing to the text.
Attachment #9163038 - Flags: ui-review?(richard.marti) → ui-review+

I've tested the patch, and it looks very good to me.
I only had one issue:
I imported a backup that contained 9 keys.
After the import was done, you're showing a confirmation window, that lists the imported keys. The contents were too big, I couldn't see all of it. I don't know if there were any buttons shown on the bottom, I couldn't see any. I didn't know what to do.
I tried pressing escape and that work.

If possible, can you add a scrollbar to the list of items, to ensure the buttons are always possible, and it's always possible to see controls to close that configuration window?

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

Rename the button label to "OpenPGP Key Management".
This would be aligned with the text that we are showing in the global Tools menu, which also uses this label.

Wouldn't be more accurate to call it "OpenPGP Key Manager"?

I'm fine with that, but if we want to rename it, it should be done everywhere.
Could be done in a separate follow-up patch.

If possible, can you add a scrollbar to the list of items, to ensure the buttons are always possible, and it's always possible to see controls to close that configuration window?

Ouch, that's a bug. A scrollbar should definitely appear there, it's a CSS problem I'll fix.

I'm fine with that, but if we want to rename it, it should be done everywhere.
Could be done in a separate follow-up patch.

Sounds good to me.

Regarding

Select the personal key or personal certificate, for which you own the secret part,
to use for either encryption technology.

I've been staring at this sentence for a very long time, and I would like to improve it.
I think the last part of the sentence makes it complicated, I'd like to avoid it.

I want the text to explain that it is our definition of "personal" that the user owns the corresponding secret key (my earlier suggestion "secret part" wasn't good).
I want to make sure, that the wording does NOT cause the user to wonder "do I own the corresponding secret key?". Because, we will only offer items for selecting for which the corresponding secret key is available.

Can we use the following for e2e-intro-description-more ?

"Select your personal key to enable the use of OpenPGP, or your personal certificate to enable the use of S/MIME. A personal key or certificate contains your email address and you own the corresponding secret key."

(In reply to Kai Engert (:KaiE:) from comment #31)

I've been staring at this sentence for a very long time,

... the bugzilla timestamp tells me it was 1 hour 20 minutes...

(In reply to Kai Engert (:KaiE:) from comment #31)

Can we use the following for e2e-intro-description-more ?

"Select your personal key to enable the use of OpenPGP, or your personal certificate to enable the use of S/MIME. A personal key or certificate contains your email address and you own the corresponding secret key."

Maybe mentioning the email address detail isn't strictly necessary. If we skip it, I came up with a shorter second part:

"Select your personal key to enable the use of OpenPGP, or your personal certificate to enable the use of S/MIME. For a personal key or certificate you own the corresponding secret key."

(Also, I think these suggestions make it explicit that selecting the element means enabling the technology.)

One more detail regarding:

openpgp-import-key-info = Other software might describe these keys by using alternative terms such as your own keys, secret keys, private keys or key pair.

I suggest to use singular in this sentence:

"Other software might describe a personal key using alternative terms such as your own key, secret key, private key or key pair."

FYI, after you delete the last personal key, the dialog still says "thunderbird has 1 personal key". It's OK with me to fix that nit in a follow-up, up to you.

Otherwise the patch looks good to me.

To summarize:

I suggest to change e2e-intro-description-more to
"Select your personal key to enable the use of OpenPGP, or your personal certificate to enable the use of S/MIME. For a personal key or certificate you own the corresponding secret key."

I suggest to change openpgp-import-key-info to
"Other software might describe a personal key using alternative terms such as your own key, secret key, private key or key pair."

Please fix the text info after deletion, either now, or file a follow-up bug.

Attached patch 1650235-openpgp-wizard.diff (deleted) — Splinter Review

Strings updated and scrollable overflow during recap fixed.

I will take care of the keys count not updating in a follow up patch.

Attachment #9163038 - Attachment is obsolete: true
Attachment #9163038 - Flags: review?(kaie)
Attachment #9163271 - Flags: review?(kaie)
Comment on attachment 9163271 [details] [diff] [review] 1650235-openpgp-wizard.diff Thanks Alessandro!
Attachment #9163271 - Flags: review?(kaie) → review+
Target Milestone: --- → Thunderbird 80.0
Comment on attachment 9163271 [details] [diff] [review] 1650235-openpgp-wizard.diff [Approval Request Comment] Necessary for the OpenPGP implementation on 78
Attachment #9163271 - Flags: approval-comm-esr78?

Pushed by kaie@kuix.de:
https://hg.mozilla.org/comm-central/rev/028aa55f5646
Add missing actions in the OpenPGP Add Key Wizard. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Comment on attachment 9163271 [details] [diff] [review] 1650235-openpgp-wizard.diff OpenPGP - uplift request for consistency of comm-esr78, beta79 and c-c80
Attachment #9163271 - Flags: approval-comm-beta?
Comment on attachment 9163271 [details] [diff] [review] 1650235-openpgp-wizard.diff Approved for beta Approved for esr78
Attachment #9163271 - Flags: approval-comm-esr78?
Attachment #9163271 - Flags: approval-comm-esr78+
Attachment #9163271 - Flags: approval-comm-beta?
Attachment #9163271 - Flags: approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: