1662279 - macOS: Tools OpenPGP Key Manager is missing

Status: VERIFIED FIXED

(MailNews Core :: Security: OpenPGP, defect)

Description

[Kai Engert (:KaiE:)]

Using Thunderbird 78.2.1 on macOS 10.15.x

The top menu bar Tools does not contain an entry "OpenPGP Key Manager", it's missing.

Comment 1

[Magnus Melin [:mkmelin]]

Looks like it would be there: https://searchfox.org/comm-central/rev/78a165ed474d0278d4c23c947929d4bf17d6c593/mail/base/content/mainNavigationToolbox.inc.xhtml#1109

Comment 2

[Kai Engert (:KaiE:)]

Maybe this is the same as bug 1665677

Comment 4

[Kai Engert (:KaiE:)]

I think this is related to our code that disables UI elements if OpenPGP cannot be enabled correctly.

We have tagged various UI elements with class "openpgp-item" and if there's any problem discovered with OpenPGP init at startup, we disable those items.

Being unable to enter the master password quickly at startup might prevent the OpenPGP init.
But 1664016 (not yet landed into esr78) might prevent the blocking init of OpenPGP, and cause the UI elements to get disabled early.

Comment 5

[Kenny]

I found a workaround for this issue: Setting the config setting "security.prompt_for_master_password_on_startup" to true forces Thunderbird to wait for the input of the master password. The second password prompt now opens within the first password prompt. The Thunderbird UI only opens after the second password entry and the "OpenPGP Key Manager" menu item is visible afterwards.

Comment 6

[Kai Engert (:KaiE:)]

Attachment: Bug 1662279 - Move retrieveOpenPGPPassword() out of startup path to fix master password related failures. r=PatrickBrunschwig

Comment 7

[Kai Engert (:KaiE:)]

The existing patch didn't fix the menu item. But it fixed the multiple master password prompts on macOS! So we should take it.

Regarding the issue reported here: I discovered the reason is an init race, that's why it sometimes works.
The race is in BondOpenPGP.allDependenciesLoaded() which calls the async function BondOpenPGP.init() without waiting for it. But we cannot use await, we cannot turn init() into an async function easily, because the Enigmail migrator calls this function, and we shouldn't have to change the migrator.

I think we can fix it differently. Originally, the call to .allDependenciesLoaded() was intended to help while OpenPGP was still disabled by default, or with our RNP library setup not yet being reliable.

Both conditions have changed. We enable by default, and our libraries are now stable. Therefore I think we can simplify this code.

I suggest that the code that enables UI elements simply checks for the pref to be true. Although no longer strictly required, let's continue to support the scenario "pref disabled on startup, pref gets enabled later at runtime and trigger init".

Furthermore, IIUC the code to enable items in msgHdrView is redundant, because we already do it in msgMail3PaneWindow.js

Comment 8

[Kai Engert (:KaiE:)]

Attachment: Bug 1662279 - Simplify UI element enabling and OpenPGP init code. r=PatrickBrunschwig

Comment 9

[Patrick Brunschwig]

Both conditions have changed. We enable by default, and our libraries are now stable. Therefore I think we can simplify this code.

I suggest that the code that enables UI elements simply checks for the pref to be true. Although no longer strictly required, let's continue to support the scenario "pref disabled on startup, pref gets enabled later at runtime and trigger init".

Just FTR, I think we should keep the possibility to have the pref disabled. I know that for example the pEp plugin does this, such that there is no conflict between pEp and OpenPGP.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/119b111ffb7f
Move retrieveOpenPGPPassword() out of startup path to fix master password related failures. r=PatrickBrunschwig
https://hg.mozilla.org/comm-central/rev/e14a1d568179
Simplify UI element enabling and OpenPGP init code. r=PatrickBrunschwig

Comment 11

[Kai Engert (:KaiE:)]

Comment on attachment 9178583 [details]
Bug 1662279 - Move retrieveOpenPGPPassword() out of startup path to fix master password related failures. r=PatrickBrunschwig

[Approval Request Comment]
Regression caused by (bug #): none
User impact if declined: UI issues
Testing completed (on c-c, etc.): only manual
Risk to taking this patch (and alternatives if risky): minor risk that reordering of startup events causes something to not work (but that will be noticed quickly)

This will be a very helpful patch for the stable branch, but because of minor risks, I'd like to see as much beta testing as possible.

Comment 12

[Kai Engert (:KaiE:)]

NOTE:

The version in phab isn't identical with the landed patch. A minor merging was necessary because of overlap with bug 1647039.
That might be the reason why the revision in phab wasn't closed automatically.

Comment 13

[Kai Engert (:KaiE:)]

Comment on attachment 9178851 [details]
Bug 1662279 - Simplify UI element enabling and OpenPGP init code. r=PatrickBrunschwig

need both patches

Comment 14

[Kai Engert (:KaiE:)]

(In reply to Patrick Brunschwig from comment #9)

Just FTR, I think we should keep the possibility to have the pref disabled. I know that for example the pEp plugin does this, such that there is no conflict between pEp and OpenPGP.

I'm OK with that for the esr78 branch. I'm not sure if we can promise that it will remain available for the Thunderbird summer 2021 release.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/531b822c482b
Follow-up to fix lint. rs=bustage DONTBUILD

Comment 16

[Kai Engert (:KaiE:)]

The checked in patch doesn't work.
Our startup init is failing, isn't triggered.

It doesn't work because of the change I made in response to Magnus request to introduce a three-state variable for tracking.

Comment 17

[Kai Engert (:KaiE:)]

Attachment: Bug 1662279 - Follow-up, undo a regression in init logic. r=PatrickBrunschwig

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1e9cdae80771
Backed out changeset 531b822c482b. r=me
https://hg.mozilla.org/comm-central/rev/995dfa231696
Backed out changeset e14a1d568179 for breaking OpenPGP init logic. r=me

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2ae6a0dda8a6
Simplify UI element enabling and OpenPGP init code. r=PatrickBrunschwig (relanding)
https://hg.mozilla.org/comm-central/rev/a5438ea8d84d
Follow-up, undo a regression in init logic. r=PatrickBrunschwig

Comment 20

[Kai Engert (:KaiE:)]

Comment on attachment 9179376 [details]
Bug 1662279 - Follow-up, undo a regression in init logic. r=PatrickBrunschwig

need all 3 patches

Comment 21

[Wayne Mery (:wsmwk)]

Comment on attachment 9178583 [details]
Bug 1662279 - Move retrieveOpenPGPPassword() out of startup path to fix master password related failures. r=PatrickBrunschwig

[Triage Comment]
Approved for beta

Comment 22

[Wayne Mery (:wsmwk)]

Comment on attachment 9178851 [details]
Bug 1662279 - Simplify UI element enabling and OpenPGP init code. r=PatrickBrunschwig

[Triage Comment]
Approved for beta

Comment 23

[Wayne Mery (:wsmwk)]

Comment on attachment 9179376 [details]
Bug 1662279 - Follow-up, undo a regression in init logic. r=PatrickBrunschwig

[Triage Comment]
Approved for beta

Comment 24

[Rob Lemley [:rjl]]

Thunderbird 82.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/3d0663d7e42a
Thunderbird 82.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/12943d122a35
Thunderbird 82.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/2ae4ef3b5de8

Comment 25

[Magnus Melin [:mkmelin]]

Comment on attachment 9179376 [details]
Bug 1662279 - Follow-up, undo a regression in init logic. r=PatrickBrunschwig

[Approval Request Comment]
I guess we need these 3 changesets on 78 too.

Comment 27

[Rob Lemley [:rjl]]

Comment on attachment 9179376 [details]
Bug 1662279 - Follow-up, undo a regression in init logic. r=PatrickBrunschwig

[Triage Comment]
Approval for the three changes that are on beta.

https://hg.mozilla.org/releases/comm-beta/rev/3d0663d7e42a
https://hg.mozilla.org/releases/comm-beta/rev/12943d122a35
https://hg.mozilla.org/releases/comm-beta/rev/2ae4ef3b5de8

Comment 28

[Rob Lemley [:rjl]]

Thunderbird 78.3.3:
https://hg.mozilla.org/releases/comm-esr78/rev/8b16b0b3809d
https://hg.mozilla.org/releases/comm-esr78/rev/4c5cc09bfdf8
https://hg.mozilla.org/releases/comm-esr78/rev/0a8b6a07cfe8

Comment 29

[Vlad Lucaci, Thunderbird QA (:vlucaci)]

Confirming this issue as verified fixed on macOS 10.15 using ESR 128.0(20240703144221).