1656391 - OpenPGP Key Manager, generate key uses random account

Status: RESOLVED FIXED

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

Description

[Kai Engert [:KaiE:]]

This is a regression introduced by bug 1652537

If the OpenPGP Key Manager is used to generate a new key, the UI previously asked for which account the key shall be created.

With the change to use the key wizard, this is no longer asked.
It's not clear for which identity a new key will be generated.
It's probably a random account.

Comment 1

[Andreas]

It seems that the account selected in the folder pane will be used for key generation.
I agree that there should be a way to select the identity right from the key wizard.

Comment 2

[Alessandro Castellani [:aleca]]

The key wizard matches the identity from where the key manager was opened.
The identity for which the key will be generated is specified twice, once in the title bar and another time in the confirm overlay before generation.

Comment 3

[Alessandro Castellani [:aleca]]

Attachment: Screenshot from 2020-07-31 09-06-56.png

Comment 4

[Kai Engert [:KaiE:]]

Start Thunderbird. Don't go into account settings. Use the global menu Tools OpenPGP Key Manager. Which account do you get?

Comment 5

[Alessandro Castellani [:aleca]]

The one the user has currently selected on the folder tree.

I can see how this might be confusing and not immediately understandable for some users.
Maybe putting back a dropdown to select e2e enabled identities is the way to go.
I'll take a look at it.

Comment 6

[Andreas]

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

The key wizard matches the identity from where the key manager was opened.

And I think this is not very intuitive. The key manager is the place where all my personal keys from all my identities are stored together. So I would expect to be able to create a new key for each identity no matter how I accessed the key manager.

The identity for which the key will be generated is specified twice, once in the title bar and another time in the confirm overlay before generation.

It SHOWS the identity, but it doesn't allow to SELECT the identity. The selection step should at least be possible in the key generation wizard when it is opened from the key manager.

Comment 7

[Kai Engert [:KaiE:]]

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

Maybe putting back a dropdown to select e2e enabled identities is the way to go.

I think that would be great!

Comment 8

[Kai Engert [:KaiE:]]

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

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

Maybe putting back a dropdown to select e2e enabled identities is the way to go.

I think that would be great!

Wait.

Dropdown: great

limit to e2e enabled identities: No

There's no need to limit. It's actually counterproductive. Generating a key is a way to start using e2ee with an account.

If we make the action discoverable from key manager, then IMHO generating a key for any account should be allowed.

Comment 9

[Alessandro Castellani [:aleca]]

I meant those identities that can handle OpenPGP.
I don't think we want to allow generating keys for NNTP or Chat accounts, am I wrong?

Comment 10

[Kai Engert [:KaiE:]]

Thanks for clarifying! Yes, only for email accounts.

Comment 11

[Alessandro Castellani [:aleca]]

Attachment: 1656391-key-wizard-identity.diff

Identity menulist implemented.
If the KeyWizard is opened from a location where a default identity is available, it autoselects it in the menulist, otherwise it selects the first one.

Comment 12

[Kai Engert [:KaiE:]]

Potentially, you could hide the dropdown, if we open it from inside account manager, and the account is already known.

If you think that's better, the dropdown could be shown only when opened from key manager.

Comment 13

[Kai Engert [:KaiE:]]

I don't understand what it was necessary to add || null in two places.
Did you run into a scenario that required it?

In my understanding, if identity isn't a member of arguments[0], then you'd get an exception. And if arguments[0] has member identity, then it's either set to an object, or it's null. Am I missing something?

Are you trying to safeguard against the identity member not being present? In that case, wouldn't you want to use
gIdentity = "identity" in window.arguments[0] ? window.arguments[0].identity : null ;

Comment 14

[Kai Engert [:KaiE:]]

Comment on attachment 9168018 [details] [diff] [review]
1656391-key-wizard-identity.diff

The patch is OK with me, if you want to keep it this way.
My suggestions are optional.

Comment 15

[Alessandro Castellani [:aleca]]

Potentially, you could hide the dropdown, if we open it from inside account manager, and the account is already known.

Better not as we would be changing the UI without letting the user know why and it might be confusing.

I don't understand what it was necessary to add || null in two places.
Did you run into a scenario that required it?

I think I can remove that null check in the key wizard but I need to keep it in the key manager.
Since the key manager can be opened from anywhere due to the item in the main menu, it might be opened when the user has selected the local folders, or a mailing list, or a chat account.

The key manager always gets an identity argument when opened but it might be empty in those scenarios.

Comment 16

[Alessandro Castellani [:aleca]]

Attachment: 1656391-key-wizard-identity.diff

Comment 17

[Kai Engert [:KaiE:]]

I will push this together with other bugs, as soon as the tree opens.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4fd1b0b047a0
Allow changing identity in the OpenPGP Key Wizard. r=KaiE DONTBUILD

Comment 19

[Kai Engert [:KaiE:]]

Comment on attachment 9168214 [details] [diff] [review]
1656391-key-wizard-identity.diff

Important OpenPGP UI correctness enhancement. Limited scope of change, I don't see risk.

Comment 20

[Wayne Mery (:wsmwk)]

Comment on attachment 9168214 [details] [diff] [review]
1656391-key-wizard-identity.diff

[Triage Comment]
Approved for beta

Comment 21

[Kai Engert [:KaiE:]]

https://hg.mozilla.org/releases/comm-beta/rev/69c6b7ba55a54ad754459b43769900a35573d976

Comment 22

[Wayne Mery (:wsmwk)]

Comment on attachment 9168214 [details] [diff] [review]
1656391-key-wizard-identity.diff

[Triage Comment]
Approved for esr78

Comment 23

[Kai Engert [:KaiE:]]

https://hg.mozilla.org/releases/comm-esr78/rev/61b928f825598778c35e8169d27bfade7d805d20