Closed Bug 1672851 Opened 3 months ago Closed 3 months ago

Fix and improve decryption usability for PGP inline messages

Categories

(MailNews Core :: Security: OpenPGP, enhancement)

enhancement

Tracking

(thunderbird_esr78+ fixed, thunderbird83+ fixed)

RESOLVED FIXED
84 Branch
Tracking Status
thunderbird_esr78 + fixed
thunderbird83 + fixed

People

(Reporter: aleca, Assigned: aleca)

References

Details

(Keywords: ux-discovery, ux-efficiency)

Attachments

(5 files, 9 obsolete files)

3.21 KB, message/rfc822
Details
566 bytes, message/rfc822
Details
28.23 KB, patch
aleca
: review+
aleca
: feedback+
Details | Diff | Splinter Review
3.75 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
22.29 KB, patch
Details | Diff | Splinter Review

As reported in bug1647039comment87, the Decrypt button presents some inconsistent behaviour we could improve.

  • Improve discoverability: As of now, the button and notification message is showed inside the Message Security popup. This was done on purpose to prevent the issue of having multiple notification stacked in the message pane. We could explore the possibility of exposing this notification outside as a regular NotificationBar, or implementing the notification counter on top of the encryption label like in the proposed original design.
  • Reload information: We should force a full message reload once the user clicks on the Decrypt button to properly update the message security info and prevent visual inconsistencies.
Summary: Improve usability of the Decrypt message action → Improve usability of the Decrypt message action (applies to PGP inline encrypted messages)

I noticed another problem that I'm trying to tackle.
If the .eml file is opened from File > Open > Saved message, OpenPGP is properly loaded and the encryption button is showed.
If the message is opened directly from the attachment, nothing shows up.

Attached patch 1672851-inline-decrypt.diff (obsolete) — Splinter Review

This is a WIP patch which takes care of a couple of things:

  • The Decrypt bar is now a proper notification.
  • Upon clicking Decrypt, the encryption status is properly updated.

This doesn't solve the issue when opening the message directly from an attachment.
I think the problem comes from the messenger.openAttachment method, which is a C++ method (nsMessenger::OpenAttachment).
https://searchfox.org/comm-central/rev/ccdaa27822e254ca10d9c576c9a3723e141a05ae/mail/base/content/msgHdrView.js#2060

Instead, if we open the file from File > Open > Saved message, everything is handled in JS here: https://searchfox.org/comm-central/rev/ccdaa27822e254ca10d9c576c9a3723e141a05ae/mail/base/content/mailWindowOverlay.js#2312

What should be the approach here?
Should we check if the content type is a message and open it via JS? Could we properly convert the URI to be readable?

Attachment #9183571 - Flags: feedback?(mkmelin+mozilla)

You could try it but I suspect it can't just be opened "like that" via js.

Comment on attachment 9183571 [details] [diff] [review]
1672851-inline-decrypt.diff

Review of attachment 9183571 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/msgSecurityPane.inc.xhtml
@@ -54,1 @@
>          <!-- Unable to decrypt -->

I'd think this and at least the one for Exchange should also move to a notification since the messages will be more or less unusable without that information.
Attachment #9183571 - Flags: feedback?(mkmelin+mozilla) → feedback+

I'd think this and at least the one for Exchange should also move to a notification since the messages will be more or less unusable without that information.

Sounds good.
With these changes, the inline notifications that will remain inside the security popup are Import Key and Missing Signature alerts.
I think this is good.

You could try it but I suspect it can't just be opened "like that" via js.

On yeah, I'm well aware this is not gonna be an easy change.

Kai, I'm pinging you to see if you have any insights of what we could do for the problem highlighted in comment 1.

Should we force open .eml files via JS instead of C++, or is there a way for those attachments to be scanned for encrypted parts, and therefore properly triggering the encryption information?

Flags: needinfo?(kaie)
Attached patch 1672851-inline-decrypt.diff (obsolete) — Splinter Review

This patch takes care of using the MozElements.NotificationBox for both unable to decrypt, partially encrypted, and broken exchange errors.

Unfortunately, I wasn't able to manually test the 3rd scenario.
Does anyone have the ability to send me an email to simulate the broken MS exchange error?

Attachment #9183571 - Attachment is obsolete: true
Attachment #9183912 - Flags: review?(mkmelin+mozilla)
Blocks: 1670307
Comment on attachment 9183912 [details] [diff] [review]
1672851-inline-decrypt.diff

Review of attachment 9183912 [details] [diff] [review]:
-----------------------------------------------------------------

When I get a notification about partial encryption, then (try to) decrypt, I get two notification boxes stacked on top of each other: first the "Reminder: the message shown below is only a subset of the original message" and below that, "The secret key that is required to decrypt this message is not available".  I would expect only one notification bar, messages hidden under each other by priority if need be, but here since decryption failed also there shouldn't ideally be the reminder at all.

Can you add a test that this is working? For notifications, you can check examples e.g. in https://searchfox.org/comm-central/rev/458e8a4488e9a3eccf4602f64f2145da387ebb25/mail/test/browser/composition/browser_attachmentReminder.js#351

::: mail/base/content/mailWindowOverlay.js
@@ -3155,5 @@
>    get msgNotificationBar() {
>      delete this.msgNotificationBar;
>  
>      let newNotificationBox = new MozElements.NotificationBox(element => {
> -      element.setAttribute("flex", "1");

sure this can go?
Attachment #9183912 - Flags: review?(mkmelin+mozilla)

Note that for successful decryption, use standard keys, see bug 1669014.

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

Does anyone have the ability to send me an email to simulate the broken MS exchange error?

I sent you email.

Repairing of that message worked previously, but repairing is no longer offered with 78.4

Flags: needinfo?(kaie)

(In reply to Kai Engert (:KaiE:) from comment #10)

Repairing of that message worked previously, but repairing is no longer offered with 78.4

ok, I discovered the repair button inside the openpgp button; repairing using that button works.

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

(a) If the .eml file is opened from File > Open > Saved message, OpenPGP is properly loaded and the encryption button is showed.
(b) If the message is opened directly from the attachment, nothing shows up.

I don't understand the difference.
Can you please describe in more detail what (b) is about?

Are scenarios (a) and (b) about the same, or about different messages?

(In reply to Kai Engert (:KaiE:) from comment #12)

I don't understand the difference.
Can you please describe in more detail what (b) is about?

(b) A single click on the .eml attached to the message you're currently reading. Opening an .eml directly from the list of attachments doesn't trigger the OpenPGP feature.

Are scenarios (a) and (b) about the same, or about different messages?

I'm talking about the same attached message.
But in scenario (a) I saved the message locally and then opened it via the menu File > Open > Saved message.
In scenario (b) I clicked directly on the attachment in the message pane. As I wrote in comment 2, clicking on an attached email message uses a C++ method to fetch it and to open a new window.

(In reply to Magnus Melin [:mkmelin] from comment #8)

I would expect only one notification bar, messages hidden under each other by priority if need be

I was expecting that as well, I'm not sure if that's the expected behaviour of the notifications or there's something wrong with our implementation. I'll investigate.

but here since decryption failed also there shouldn't ideally be the reminder at all.

Yeah, that makes sense. I'll postpone the reminder only if the decryption fails. The original code has the same issue, showing the reminder regardless the decryption outcome.

Can you add a test that this is working?

For sure.

  • element.setAttribute("flex", "1");

sure this can go?

I converted the "mail-notification-top" into a vbox as I noticed that multiple notifications were being added inline.
I wonder if that's an issue we always had and never noticed since we don't usually stack notifications.

All right, a little update here.

  • Opening .eml attachments is a whole other beast that we will need to tackle at some point in a follow up bug.
  • I opened bug 1674013 as I stumbled upon that issue while working on this patch. The issue is not caused by this patch.
  • I opened bug 1673958 as I noticed an incorrect implementation of the NotificationBox. I already fixed the part touching this patch.
  • I created 2 tests to cover the broken MS-Exchange and the PGP inline partial decryption. I'm running these test with a couple of emails that users sent me to recreate the issue, but of course since those contain real data and addresses, we can't use them in the patch.

How can I update our test .eml files to simulate those 2 scenarios?

Attached patch 1672851-inline-decrypt.diff (obsolete) — Splinter Review

Meanwhile, I'm uploading the patch for a review.
The two new tests currently fail because I'm loading placeholder messages, waiting for some help in creating those variations I wrote above.

Attachment #9183912 - Attachment is obsolete: true
Attachment #9184445 - Flags: review?(mkmelin+mozilla)

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

How can I update our test .eml files to simulate those 2 scenarios?

I've manually crafted the messages you requested, see attachments.

According to this bugs title, maybe bug 1673798 is related.

Attached patch 1672851-inline-decrypt-78.diff (obsolete) — Splinter Review

Version of the patch merged to the esr78 branch. Only difference is in context, setTimeout -> EnigmailTimer.setTimeout

Attached patch 1672851-inline-decrypt.diff (obsolete) — Splinter Review

Thanks Kai for those files, perfect.
The test for the broken MS-Exchange is disabled as it fails due to bug 1674013.

Attachment #9184445 - Attachment is obsolete: true
Attachment #9184445 - Flags: review?(mkmelin+mozilla)
Attachment #9184643 - Flags: review?(mkmelin+mozilla)
Attachment #9184643 - Flags: feedback?(kaie)
Comment on attachment 9184643 [details] [diff] [review]
1672851-inline-decrypt.diff

Review of attachment 9184643 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/test/browser/openpgp/browser_viewMessage.js
@@ +336,5 @@
> +    {
> +      popup: null,
> +    }
> +  );
> +  mc.click(new elementslib.Elem(prefButton));

Please for these instead use use
EventUtils.synthesizeMouseAtCenter(prefButton, {}, mc.window)
Attachment #9184643 - Flags: review?(mkmelin+mozilla) → review+
Target Milestone: --- → 84 Branch
See Also: → 1673798

I think the bug subject should clarify that it isn't just about improvements, it's also about regression fixes (bug 1673798).

Summary: Improve usability of the Decrypt message action (applies to PGP inline encrypted messages) → Fix and improve decryption usability for PGP inline messages
See Also: 1673798
Duplicate of this bug: 1673798

Comment on attachment 9184643 [details] [diff] [review]
1672851-inline-decrypt.diff

I agree that some notifications should remain top level, and I've tested it fixes the regression, so feedback+

Attachment #9184643 - Flags: feedback?(kaie) → feedback+
Attachment #9184598 - Attachment is obsolete: true
Attached patch 1672851-inline-decrypt.diff (obsolete) — Splinter Review

Test click event updated.

Attachment #9184643 - Attachment is obsolete: true
Attachment #9184849 - Flags: review+
Attachment #9184849 - Flags: feedback+
Attachment #9184782 - Attachment is obsolete: true
Attachment #9184929 - Attachment is patch: true

A little update to the patch before pushing as I found out that buttons inside MozElements.NotificationBox accept the "l10n-id" attribute, therefore no need to update any string for a safer uplift.

Attachment #9184849 - Attachment is obsolete: true
Attachment #9184940 - Flags: review+
Attachment #9184940 - Flags: feedback+

Pushed by alessandro@thunderbird.net:
https://hg.mozilla.org/comm-central/rev/e46e9d947285
Fix missing openpgp encryption label for PGP inline encrypted messages. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/197397b636fe
followup - fix linting. rs=me DONTBUILD

The tree has been orange enough to make it hard to notice, but this broke the "draft notification" - the bar you should get when looking at a draft.
Unless you have a quick fix, I'll do a hg qbackout -r 197397b636fe:e46e9d947285 later today

comm/mail/test/browser/composition/browser_draftIdentity.js is broken at least

Flags: needinfo?(alessandro)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

On it!

Flags: needinfo?(alessandro)

I was wrongly cleaning up all the notifications at the end of the decryption process.
With this follow up patch, the OpenPGP code only handles the removal of specific notifications.

Attachment #9185210 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9185210 [details] [diff] [review]
1672851-draft-notification.diff

Review of attachment 9185210 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! r=mkmelin
Attachment #9185210 - Flags: review?(mkmelin+mozilla) → review+

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/f9e1740a5a15
Follow up: Fix broken draft notification. r=mkmelin

Status: REOPENED → RESOLVED
Closed: 3 months ago3 months ago
Resolution: --- → FIXED

I'm gonna create a single patch for easier uplift to beta, and also properly update the variation for 78.

I'm suspecting this also is causing
comm/mail/test/browser/message-header/browser_phishingBar.js | A promise chain failed to handle a rejection: can't access property "getElementsByTagName", msgFrame.contentDocument is null - stack: getBodyElement@chrome://openpgp/content/ui/enigmailMessengerOverlay.js:1372:5

.... which has been showing up, but not quite consistently. Filed bug 1674895 about that.

Comment on attachment 9184940 [details] [diff] [review]
1672851-inline-decrypt.diff

[Approval Request Comment]
Regression caused by (bug #): bug 1647039,
User impact if declined: Notifications regarding partially encrypted inline message or broken MS-exchange messages won't be visible.
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): low as it exposes important notifications otherwise hidden

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

Comment on attachment 9185210 [details] [diff] [review]
1672851-draft-notification.diff

[Approval Request Comment]
Same as above, this is a follow up for a quick fix and these patches should be uplifted together

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

Kai, I updated the patch variation for 78 with the latest changes and merged the follow up fixes.
Could you test that everything looks and works properly?
Thanks.

Attachment #9184929 - Attachment is obsolete: true
Attachment #9185746 - Flags: review+
Attachment #9185746 - Flags: feedback?(kaie)
Attachment #9185746 - Attachment is patch: true

This patch is corrupted, it fails to apply with an error message in line 191. Did you manually edit a patch to create this file?

The patch seems corrupted in many ways. If you diff v3 and v4, you'll see many lines that change a single space. And for the two test .eml files, I'm guessing you have changed the line endings from CRLF to something else. OpenPGP messages must be CRLF.

I think you've simply tried to combine the three commits into one, with the adjusted setTimeout call. I didn't see other changes.

I've applied those locally to a esr78 tree, and then used hg export.
This is the resulting patch.

I'm not sure why I see a change from the earlier v4 patch and this one in the eml files, but I've verified that this patch will create files that are byte-identical with the file that was commited on trunk.

Attachment #9185746 - Attachment is obsolete: true
Attachment #9185746 - Flags: feedback?(kaie)

testing of this patch with 78 seems fine

Comment on attachment 9185210 [details] [diff] [review]
1672851-draft-notification.diff

[Triage Comment]
Approved for beta

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

Comment on attachment 9184940 [details] [diff] [review]
1672851-inline-decrypt.diff

[Triage Comment]
Approved for beta

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

Comment on attachment 9185816 [details] [diff] [review]
1672851-v5-78.patch (esr78)

Not a small patch, but limited in scope, and I think it's important to fix this regression soon.

For approval comment see comment 40.

Attachment #9185816 - Flags: approval-comm-esr78?

Comment on attachment 9185816 [details] [diff] [review]
1672851-v5-78.patch (esr78)

[Triage Comment]
Approved for esr78

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