1672851 - Fix and improve decryption usability for PGP inline messages

Status: RESOLVED FIXED

(MailNews Core :: Security: OpenPGP, enhancement)

Description

[Alessandro Castellani [:aleca]]

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.

Comment 1

[Alessandro Castellani [:aleca]]

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.

Comment 2

[Alessandro Castellani [:aleca]]

Attachment: 1672851-inline-decrypt.diff

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?

Comment 3

[Magnus Melin [:mkmelin]]

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

Comment 4

[Magnus Melin [:mkmelin]]

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.

Comment 5

[Alessandro Castellani [:aleca]]

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.

Comment 6

[Alessandro Castellani [:aleca]]

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?

Comment 7

[Alessandro Castellani [:aleca]]

Attachment: 1672851-inline-decrypt.diff

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?

Comment 8

[Magnus Melin [:mkmelin]]

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?

Comment 9

[Magnus Melin [:mkmelin]]

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

Comment 10

[Kai Engert (:KaiE:)]

(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

Comment 11

[Kai Engert (: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.

Comment 12

[Kai Engert (:KaiE:)]

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

Comment 13

[Alessandro Castellani [:aleca]]

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

Comment 14

[Alessandro Castellani [:aleca]]

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

Comment 15

[Alessandro Castellani [:aleca]]

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?

Comment 16

[Alessandro Castellani [:aleca]]

Attachment: 1672851-inline-decrypt.diff

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.

Comment 17

[Kai Engert (:KaiE:)]

Attachment: alice-broken-exchange.eml

Comment 18

[Kai Engert (:KaiE:)]

Attachment: alice-partially-encrypted.eml

Comment 19

[Kai Engert (:KaiE:)]

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

Comment 20

[Arvidt]

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

Comment 21

[Kai Engert (:KaiE:)]

Attachment: 1672851-inline-decrypt-78.diff

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

Comment 22

[Alessandro Castellani [:aleca]]

Attachment: 1672851-inline-decrypt.diff

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

Comment 23

[Magnus Melin [:mkmelin]]

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)

Comment 24

[Kai Engert (:KaiE:)]

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

Comment 26

[Kai Engert (:KaiE:)]

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+

Comment 27

[Kai Engert (:KaiE:)]

Attachment: 1672851-inline-decrypt-v2-78.diff (esr78)

Comment 28

[Alessandro Castellani [:aleca]]

Attachment: 1672851-inline-decrypt.diff

Test click event updated.

Comment 29

[Kai Engert (:KaiE:)]

Attachment: 1672851-inline-decrypt-v3-78.diff (esr78)

Comment 30

[Alessandro Castellani [:aleca]]

Attachment: 1672851-inline-decrypt.diff

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.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e46e9d947285
Fix missing openpgp encryption label for PGP inline encrypted messages. r=mkmelin

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/197397b636fe
followup - fix linting. rs=me DONTBUILD

Comment 33

[Magnus Melin [:mkmelin]]

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

Comment 34

[Alessandro Castellani [:aleca]]

On it!

Comment 35

[Alessandro Castellani [:aleca]]

Attachment: 1672851-draft-notification.diff

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.

Comment 36

[Magnus Melin [:mkmelin]]

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

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

Thanks! r=mkmelin

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f9e1740a5a15
Follow up: Fix broken draft notification. r=mkmelin

Comment 38

[Alessandro Castellani [:aleca]]

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

Comment 39

[Magnus Melin [:mkmelin]]

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 40

[Alessandro Castellani [:aleca]]

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

Comment 41

[Alessandro Castellani [:aleca]]

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

Comment 42

[Alessandro Castellani [:aleca]]

Attachment: 1672851-inline-decrypt-v4-78.diff (esr78)

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.

Comment 43

[Kai Engert (:KaiE:)]

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?

Comment 44

[Kai Engert (:KaiE:)]

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.

Comment 45

[Kai Engert (:KaiE:)]

Attachment: 1672851-v5-78.patch (esr78)

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.

Comment 46

[Kai Engert (:KaiE:)]

testing of this patch with 78 seems fine

Comment 47

[Wayne Mery (:wsmwk)]

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

[Triage Comment]
Approved for beta

Comment 48

[Wayne Mery (:wsmwk)]

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

[Triage Comment]
Approved for beta

Comment 49

[Rob Lemley [:rjl]]

Thunderbird 83.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/615c6b088a7e
https://hg.mozilla.org/releases/comm-beta/rev/761b70c4f3c6
https://hg.mozilla.org/releases/comm-beta/rev/d69c10af1482

Comment 50

[Kai Engert (:KaiE:)]

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.

Comment 51

[Wayne Mery (:wsmwk)]

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

[Triage Comment]
Approved for esr78

Comment 52

[Kai Engert (:KaiE:)]

78.5.0
https://hg.mozilla.org/releases/comm-esr78/rev/b379b74ed3c17d8488a8573f933259b76f33dcc1