Fix and improve decryption usability for PGP inline messages
Categories
(MailNews Core :: Security: OpenPGP, enhancement)
Tracking
(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+
wsmwk
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
3.75 KB,
patch
|
mkmelin
:
review+
wsmwk
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
22.29 KB,
patch
|
wsmwk
:
approval-comm-esr78+
|
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.
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 1•4 years ago
|
||
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.
Assignee | ||
Comment 2•4 years ago
|
||
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•4 years ago
|
||
You could try it but I suspect it can't just be opened "like that" via js.
Comment 4•4 years ago
|
||
Assignee | ||
Comment 5•4 years ago
|
||
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.
Assignee | ||
Comment 6•4 years ago
|
||
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?
Assignee | ||
Comment 7•4 years ago
|
||
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•4 years ago
|
||
Comment 9•4 years ago
|
||
Note that for successful decryption, use standard keys, see bug 1669014.
Comment 10•4 years ago
|
||
(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•4 years ago
|
||
(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•4 years ago
|
||
(In reply to Alessandro Castellani (:aleca) from comment #1)
(a) If the
.eml
file is opened fromFile > 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?
Assignee | ||
Comment 13•4 years ago
|
||
(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.
Assignee | ||
Comment 14•4 years ago
|
||
(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.
Assignee | ||
Comment 15•4 years ago
|
||
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?
Assignee | ||
Comment 16•4 years ago
|
||
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•4 years ago
|
||
Comment 18•4 years ago
|
||
Comment 19•4 years ago
|
||
(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•4 years ago
|
||
According to this bugs title, maybe bug 1673798 is related.
Comment 21•4 years ago
|
||
Version of the patch merged to the esr78 branch. Only difference is in context, setTimeout -> EnigmailTimer.setTimeout
Assignee | ||
Comment 22•4 years ago
|
||
Thanks Kai for those files, perfect.
The test for the broken MS-Exchange is disabled as it fails due to bug 1674013.
Comment 23•4 years ago
|
||
Updated•4 years ago
|
Comment 24•4 years ago
|
||
I think the bug subject should clarify that it isn't just about improvements, it's also about regression fixes (bug 1673798).
Comment 26•4 years ago
|
||
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+
Updated•4 years ago
|
Comment 27•4 years ago
|
||
Assignee | ||
Comment 28•4 years ago
|
||
Test click event updated.
Comment 29•4 years ago
|
||
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 30•4 years ago
|
||
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•4 years ago
|
||
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
Comment 32•4 years ago
|
||
Comment 33•4 years ago
•
|
||
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
Updated•4 years ago
|
Assignee | ||
Comment 35•4 years ago
|
||
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•4 years ago
|
||
Comment 37•4 years ago
|
||
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/f9e1740a5a15
Follow up: Fix broken draft notification. r=mkmelin
Assignee | ||
Comment 38•4 years ago
|
||
I'm gonna create a single patch for easier uplift to beta, and also properly update the variation for 78.
Comment 39•4 years ago
|
||
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.
Assignee | ||
Comment 40•4 years ago
|
||
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
Assignee | ||
Comment 41•4 years ago
|
||
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
Assignee | ||
Comment 42•4 years ago
|
||
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.
Assignee | ||
Updated•4 years ago
|
Comment 43•4 years ago
|
||
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•4 years ago
|
||
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•4 years ago
|
||
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•4 years ago
|
||
testing of this patch with 78 seems fine
Comment 47•4 years ago
|
||
Comment on attachment 9185210 [details] [diff] [review]
1672851-draft-notification.diff
[Triage Comment]
Approved for beta
Comment 48•4 years ago
|
||
Comment on attachment 9184940 [details] [diff] [review]
1672851-inline-decrypt.diff
[Triage Comment]
Approved for beta
Comment 49•4 years ago
|
||
bugherder uplift |
Comment 50•4 years ago
|
||
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•4 years ago
|
||
Comment on attachment 9185816 [details] [diff] [review]
1672851-v5-78.patch (esr78)
[Triage Comment]
Approved for esr78
Comment 52•4 years ago
|
||
Description
•