Closed Bug 1657497 Opened 3 months ago Closed 3 months ago

opening openpgp key manager with no email account: exception

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)

Details

Attachments

(1 file, 1 obsolete file)

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

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?

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

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

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

Perfect

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.

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

Assignee: alessandro → kaie
Status: NEW → ASSIGNED

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.

Assignee: kaie → alessandro
Flags: needinfo?(alessandro)

Sounds good, I'll take care of this.

Flags: needinfo?(alessandro)
Priority: -- → P1

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.

Attachment #9168301 - Attachment is obsolete: true
Attachment #9168947 - Flags: review?(mkmelin+mozilla)
Attachment #9168947 - Flags: feedback?(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.

Attachment #9168947 - Flags: review?(mkmelin+mozilla)
Attachment #9168947 - Flags: review+
Attachment #9168947 - Flags: feedback?(kaie)

Pushed by kaie@kuix.de:
https://hg.mozilla.org/comm-central/rev/34c08922aa2e
Allow opening the OpenPGP Key Manager without a default identity. r=mkmelin DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED

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

OpenPGP UI correctness and simplification fix, no risk.

Attachment #9168947 - Flags: approval-comm-esr78?
Attachment #9168947 - Flags: approval-comm-beta?
Target Milestone: --- → 81 Branch

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

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

[Triage Comment]
Approved for beta

Approved for esr78

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