Closed Bug 1662279 Opened 5 months ago Closed 4 months ago

macOS: Tools OpenPGP Key Manager is missing

Categories

(MailNews Core :: Security: OpenPGP, defect)

defect

Tracking

(thunderbird_esr78+ fixed, thunderbird82 fixed)

RESOLVED FIXED
83 Branch
Tracking Status
thunderbird_esr78 + fixed
thunderbird82 --- fixed

People

(Reporter: KaiE, Assigned: KaiE)

References

(Regressed 1 open bug)

Details

Attachments

(3 files)

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.

Maybe this is the same as bug 1665677

See Also: → 1665677
See Also: 1665677
Duplicate of this bug: 1665677

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.

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.

Assignee: nobody → kaie
Status: NEW → ASSIGNED

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

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.

Pushed by kaie@kuix.de:
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

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

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.

Attachment #9178583 - Flags: approval-comm-beta?
Target Milestone: --- → 83 Branch

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 on attachment 9178851 [details]
Bug 1662279 - Simplify UI element enabling and OpenPGP init code. r=PatrickBrunschwig

need both patches

Attachment #9178851 - Flags: approval-comm-beta?

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

Pushed by kaie@kuix.de:
https://hg.mozilla.org/comm-central/rev/531b822c482b
Follow-up to fix lint. rs=bustage DONTBUILD

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.

Pushed by kaie@kuix.de:
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #9179376 - Attachment description: Bug 1662279 - Follow-up, undo a regression in init logic. r=mkmelin → Bug 1662279 - Follow-up, undo a regression in init logic. r=PatrickBrunschwig

Pushed by kaie@kuix.de:
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

Status: REOPENED → RESOLVED
Closed: 4 months ago4 months ago
Resolution: --- → FIXED

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

need all 3 patches

Attachment #9179376 - Flags: approval-comm-beta?

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

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

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

[Triage Comment]
Approved for beta

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

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

[Triage Comment]
Approved for beta

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

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.

Attachment #9179376 - Flags: approval-comm-esr78?
Duplicate of this bug: 1670067

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

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