Closed Bug 1656391 Opened 3 months ago Closed 3 months ago

OpenPGP Key Manager, generate key uses random account

Categories

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

Tracking

(thunderbird_esr78 fixed, thunderbird80 fixed)

RESOLVED FIXED
81 Branch
Tracking Status
thunderbird_esr78 --- fixed
thunderbird80 --- fixed

People

(Reporter: KaiE, Assigned: aleca)

References

Details

Attachments

(2 files, 1 obsolete file)

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.

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.

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.

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

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.

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

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

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

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?

Thanks for clarifying! Yes, only for email accounts.

Status: NEW → ASSIGNED
Attached patch 1656391-key-wizard-identity.diff (obsolete) — Splinter Review

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.

Attachment #9168018 - Flags: review?(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.

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 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.
Attachment #9168018 - Flags: review?(kaie) → review+

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.

Attachment #9168018 - Attachment is obsolete: true
Attachment #9168214 - Flags: review+
Target Milestone: --- → 81 Branch

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

Pushed by kaie@kuix.de:
https://hg.mozilla.org/comm-central/rev/4fd1b0b047a0
Allow changing identity in the OpenPGP Key Wizard. r=KaiE DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
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.
Attachment #9168214 - Flags: approval-comm-esr78?
Attachment #9168214 - Flags: approval-comm-beta?

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

[Triage Comment]
Approved for beta

Attachment #9168214 - Flags: approval-comm-beta? → approval-comm-beta+

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

[Triage Comment]
Approved for esr78

Attachment #9168214 - Flags: approval-comm-esr78? → approval-comm-esr78+
You need to log in before you can comment on or make changes to this bug.