1657497 - opening openpgp key manager with no email account: exception

Status: RESOLVED FIXED

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

Description

[Kai Engert (:KaiE:)]

Create a fresh profile.
Enable OpenPGP pref.
Cancel the request to configure an email account.
Use menu tools/openpgp key manager.

Result:
Uncaught TypeError: identity is undefined
openKeyManager chrome://messenger/content/accountUtils.js:579
oncommand chrome://messenger/content/messenger.xhtml:1
accountUtils.js:579:5
openKeyManager chrome://messenger/content/accountUtils.js:579
oncommand chrome://messenger/content/messenger.xhtml:1

Comment 1

[Alessandro Castellani [:aleca]]

I thought I covered this scenario in bug 1656391 with the null condition.
Should we prevent the usage of the Key Manager entirely if not even 1 email account have been configured?

Comment 2

[Kai Engert (:KaiE:)]

It's certainly an edge case, but I use it frequently for testing, after creating a fresh profile.
I don't see a technical justification to prevent it.

in
mailnews/base/prefs/content/accountUtils.js
in
openKeyManager()
you have:

let args = {
identity: identity[0],
cancelCallback: null,
okCallback: null,
};

This is the place where the error happens, and where the fix would have to be done. If there's no entry in the identity array, then identity[0] is an exception.

This could probably be fixed with:
identity: identity.length ? identity[0] : null

Comment 3

[Kai Engert (:KaiE:)]

Better:
identity: (identity && identity.length) ? identity[0] : null

Comment 4

[Kai Engert (:KaiE:)]

I'll attach the patch, so you can review it.

Comment 5

[Alessandro Castellani [:aleca]]

Perfect

Comment 6

[Kai Engert (:KaiE:)]

Attachment: Bug 1657497 - Allow opening OpenPGP key manager with no email account configured. r=aleca

Comment 7

[Alessandro Castellani [:aleca]]

I think we should also disable the Generate new Key button in the manager menu if the identity is null, otherwise we will have the same exception in the key wizard.

Comment 8

[Kai Engert (:KaiE:)]

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

I think we should also disable the Generate new Key button in the manager menu if the identity is null, otherwise we will have the same exception in the key wizard.

Agreed. Patch updated, with lint fixes.

Comment 9

[Kai Engert (:KaiE:)]

Based on the comments in phab, these changes weren't ideal (from bug 1652537):
https://hg.mozilla.org/comm-central/diff/5fffded97b81e12ede2b165d10599dfa03c1a8d8/mailnews/base/prefs/content/accountUtils.js

Alessandro, it seems this needs to be improved, based on guidance from Magnus - or the code should be removed (don't stick in context when opening key manager).

I'm fine with both. Can you and Magnus please resolve this?
As this needs a revision to Alessandro's original, I'd like to assign this bug to you.

For me, I primarily want the exception gone, one way or the other.

Comment 10

[Alessandro Castellani [:aleca]]

Sounds good, I'll take care of this.

Comment 11

[Alessandro Castellani [:aleca]]

Attachment: 1657497-openpgp-manager.diff

Let's simplify things where we can.

If the KeyWizard is opened directly from the Account Settings, we get the current identity in order to allow users to set up a new key with the correct identity without the necessity of changing it.

If the KeyWizard is opened from the Key Manager, we don't pass any identity and we fallback to the default one.

If no default identity is available, we allow opening the Key Manager but we disable the Key Wizard menuitem.

Comment 12

[Kai Engert (:KaiE:)]

Comment on attachment 9168947 [details] [diff] [review]
1657497-openpgp-manager.diff

Thank you!

This removes the code that was previously added, which was added without a review from Magnus, and which Magnus didn't like - I think we don't need his review for that removal.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/34c08922aa2e
Allow opening the OpenPGP Key Manager without a default identity. r=mkmelin DONTBUILD

Comment 14

[Kai Engert (:KaiE:)]

Comment on attachment 9168947 [details] [diff] [review]
1657497-openpgp-manager.diff

OpenPGP UI correctness and simplification fix, no risk.

Comment 15

[Kai Engert (:KaiE:)]

Apologies, I landed the commit with incorrect user attribution and incorrect reviewer :(

Comment 16

[Wayne Mery (:wsmwk)]

Comment on attachment 9168947 [details] [diff] [review]
1657497-openpgp-manager.diff

[Triage Comment]
Approved for beta

Approved for esr78

Comment 17

[Rob Lemley [:rjl]]

Thunderbird 80.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/88e243a20848

Comment 18

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/releases/comm-esr78/rev/1343440396c9378e525d969a92bd07ea0a15bdb1