Closed Bug 1653300 Opened 3 months ago Closed 2 months ago

OpenPGP e2ee pref UI should show if an external key was configured

Categories

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

enhancement

Tracking

(thunderbird_esr78 fixed, thunderbird79 fixed)

RESOLVED FIXED
Thunderbird 80.0
Tracking Status
thunderbird_esr78 --- fixed
thunderbird79 --- fixed

People

(Reporter: KaiE, Assigned: aleca)

References

Details

(Keywords: ux-efficiency)

Attachments

(2 files, 2 obsolete files)

OpenPGP e2ee pref UI should show if an external key was configured

More details to follow

Bug 1603782 adds a new preference, that distinguishes if the configured OpenPGP is an external key managed by GnuPG.

It can be queried using identity.getBoolAttribute("is_gnupg_key_id");
It would be good to indicate to display that this configuration is used.
However, I suggest that initially we use a simplify as much as possible.

A minimal way could be:

  • require that the user manually edits the prefs.js file to set this pref
  • if the pref is set, don't show the list of keys. Don't offer the button "add key".
  • instead, show a text that says "Manual configuration active. Using external GnuPG key with ID (value from openpgp_key_id)"

I'm guessing that this is very simple to do, and it would be sufficient to avoid that the UI is used to disturb the manual configuration.

Alternatively, if you think it can be done quickly, prior to the string freeze, you could an additional selection choice, similar to "None". It could use label "GnuPG Key (key ID)".
Have an edit box inside that selection, without any field validation.
Don't offer any details, don't offer any properties, don't offer any actions.

If this special entry is selected, set the identitys pref to true. Set the value of openpgp_key_id to the string entered in the input box. No validation.

There will be documentation for this mode. Several additional (manual) steps will be required to ensure this external configuration works. Therefore I think it isn't worth it rushing a more pretty UI.

And as previously mentioned, if global pref "mail.openpgp.allow_external_gnupg" is false, hide the GnuPG offer.

If mail.openpgp.allow_external_gnupg it false, ignore the account's value of is_gnupg_key_id

Alessandro, do you think this could be done?

I'm happy with the minimal implementation (hide key selection and button, don't offer input, just offer that external gnupg is used, require the user to manually edit prefs.js).

Assignee: nobody → alessandro
Status: NEW → ASSIGNED
Keywords: ux-efficiency
Priority: -- → P1
Attached patch 1653300-key-external.diff (obsolete) — Splinter Review
Attachment #9166023 - Flags: review?(kaie)

"External GnuPG Key saved successfully!"

Could confuse the user, because we're not saving a key. Also, we have not idea if the configuration was successful. I suggest to remove the optimism and say:

"External Key configuration saved."

Attached patch 1653300-key-external.diff (obsolete) — Splinter Review

Strings udpated

Attachment #9166023 - Attachment is obsolete: true
Attachment #9166023 - Flags: review?(kaie)
Attachment #9166027 - Flags: review?(kaie)

Thanks a lot for the patch. I've reviewed with Alessandro interactively in chat. We agreed on two string corrections, and he gave me another JS snippet to fix saved prefs.

I'll upload the updated patch and mark as r=kaie

Attached patch 1653300-v3.patchSplinter Review
Attachment #9166027 - Attachment is obsolete: true
Attachment #9166027 - Flags: review?(kaie)
Attachment #9166030 - Flags: review+

Pushed by kaie@kuix.de:
https://hg.mozilla.org/comm-central/rev/16c5f3137158
Implement the UI to handle OpenPGP External keys. r=KaiE DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Comment on attachment 9166030 [details] [diff] [review]
1653300-v3.patch

Only if the external-gnupg pref is enabled, this will allow users to conveniently configure their external key ID that they want to use. Avoids that users have to fiddle with account IDs in prefs.
Important for smartcard and Qubes OS users.
Attachment #9166030 - Flags: approval-comm-esr78?
Attachment #9166030 - Flags: approval-comm-beta?

received a=wsmwk for both comm-beta and comm-esr78 on Matrix

I found a small issue here:

// Always update the GnuPG boolean pref to be sure the currently used key is
  // internal or external.
  gIdentity.setBoolAttribute(
    "is_gnupg_key_id",
    newKey ==
      gIdentity.getUnicharAttribute("last_entered_external_gnupg_key_id")
  );

With this condition, is_gnupg_key_id will be set to TRUE if the user selects NONE and no previous external key was saved in the last_entered_external_gnupg_key_id.
Quick fix coming up.

This should fix it.

Attachment #9166033 - Flags: review?(kaie)
Comment on attachment 9166033 [details] [diff] [review]
1653300-key-external-PART2.diff

will uplift this obvious correctness fix, and reuse the existing a=
Attachment #9166033 - Flags: review?(kaie) → review+
Pushed by kaie@kuix.de:
https://hg.mozilla.org/comm-central/rev/16c3f6fc057e
Fix the toggle of the external GnuPG preference. r=KaiE DONTBUILD
Target Milestone: --- → Thunderbird 80.0
Comment on attachment 9166033 [details] [diff] [review]
1653300-key-external-PART2.diff

Per previous comment Approved for esr78, and beta 79.0b3
Attachment #9166033 - Flags: approval-comm-esr78+
Attachment #9166033 - Flags: approval-comm-beta+
Comment on attachment 9166030 [details] [diff] [review]
1653300-v3.patch

Per previous comment Approved for esr78, and beta 79.0b3

However, Kai, you missed this patch in the uplifts to beta and 78.

For beta, I suggest we skip the uplift since it will be in 80 by default.  Unless having attachment 9166033 [details] [diff] [review] but not having this patch will badly break 79.0b3 (which will exist for several days before 80 comes out)
Flags: needinfo?(kaie)
Attachment #9166030 - Flags: approval-comm-esr78?
Attachment #9166030 - Flags: approval-comm-esr78+
Attachment #9166030 - Flags: approval-comm-beta?
Attachment #9166030 - Flags: approval-comm-beta+

(In reply to Wayne Mery (:wsmwk) from comment #19)

However, Kai, you missed this patch in the uplifts to beta and 78.

I don't think I missed them.

See above, there were two uplifts.
Comment 12 and comment 17.

Flags: needinfo?(kaie) → needinfo?(vseerror)

super - I was looking for comments closer together. We're golden

Flags: needinfo?(vseerror)
Blocks: 1655284
You need to log in before you can comment on or make changes to this bug.