1651492 - add mochitest for viewing openpgp messages

Status: RESOLVED FIXED

(MailNews Core :: Security: OpenPGP, task)

Description

[Magnus Melin [:mkmelin]]

We should add mochitests that open various kinds of openpgp messages and check that the UI is appropriate.

The UI is still a bit in flux, so let's not check too much details. The details can be adjusted later once the UI is more finalized.

Messages to start with:

• no security (normal)
• signed
• encrypted, not signed
• encrypted and signed
• has autocrypt header
• inline PGP

Then we can move on to more problematic cases...

Comment 1

[Lasana Murray]

Is there a way to create mock email accounts for testing? I generated keys based on my own emails and have been sending messages to myself to get encrypted/signed eml files. I'm realizing that I can't remove any of the contents or the signature fails (doh!).

Comment 2

[Lasana Murray]

Never-mind. I think I found a way using the local folders.

Comment 3

[Lasana Murray]

Attachment: mochitests.patch

Added some tests, keys and helper functions for importing them. I'm not too sure how to generate an inline OpenPGP message, I need some guidance there.

Of the tests added, one will fail, the case where we open a trusted, encrypted message without a signature and no public key. The signed icon is displayed. Is this a bug? I'll update the test if it isn't.

Comment 4

[Magnus Melin [:mkmelin]]

An encrypted-only message from (?) someone trusted should not show as signed. If it does that's a bug.

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9171287 [details] [diff] [review]
mochitests.patch

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

It's a bit hard to check the logic. I think it would be better to change the naming to be consistant and tell consistently what the message is about. For the public key and secret, use the normal file conventions (.asc) you get when importing/exporting such keys. It's also confusing to use "unknown" as name of the sender. Easer to just use a normal name like Alice and Bob (later expand with other standard names depending on their roles, see https://en.wikipedia.org/wiki/Alice_and_Bob)

So instead of sender.pub you'd use no something like [e-mail] (both email and key id included)
This way doing it's easier to reason about the files and messages.

For the .eml files, for each it would be easier to understand if it's like
unsigned-encrypted-to-0xAE3019F3B2E3EBD8.eml, signed-by-0xAAAAAAABBB-encrypted-to-0xAE3019F3B2E3EBD8.eml, signed-by-0xAE3019F3B2E3EBD8-unencrypted.eml, unsigned-from-alice-unencrypted-to-bob.eml

... and then the logic of who is trusted and such can change, later on during the test. 
I would also add a description of the email in the subject of each mail.

::: mail/test/browser/openpgp/browser_viewMessage.js
@@ +71,5 @@
> +const TEST_SIGN_ICON_OK = "signed icon ok is displayed";
> +const TEST_SIGN_ICON_MISMATCH = "signed icon mismatch is displayed";
> +const TEST_NO_SIGN_ICON = "signed icon is not displayed";
> +const TEST_ENCRYPT_ICON_OK = "encrypted icon is displayed";
> +const TEST_NO_ENCRYPT_ICON = "encrypted icon is not displayed";

Please just inline these kind of texts, that makes the code easer to read.

@@ +135,5 @@
> +});
> +
> +/* Trusted Cases */
> +
> +// Open a message signed only from the trusted sender.

Please use jsdoc documentation for what the functions do.
/**
  * Test opening a signed-only message from a trusted sender.
  */

@@ +137,5 @@
> +/* Trusted Cases */
> +
> +// Open a message signed only from the trusted sender.
> +add_task(async function testOpenTrustedSignedMessage() {
> +  let msgCtrl = await openEMLFile(EML_SENDER_SIGN);

we useually name it "mc" so please use that

::: mail/test/browser/shared-modules/OpenPGPHelpers.jsm
@@ +102,5 @@
> + * Imports a public key into the keyring while also updating its acceptance.
> + *
> + * @param {nsIWindow} parent               - The parent window.
> + * @param {nsIFile} file                   - A valid file containing a public
> + *                                           OpenPGP key.

I'd prefer no spacing to try to align. E.g.

 * @param {nsIWindow} parent - The parent window.

Comment 6

[Lasana Murray]

Attachment: mochitestsv2.patch

Updated to include various feedback. The issue I mentioned previously with the icons no longer happens.

Comment 7

[Lasana Murray]

These tests fail on Windows with (NS_ERROR_NOT_AVAILABLE) [nsIMsgAccountManager.createIncomingServer]" nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE).

Am I doing something wrong with this API? Thoughts?

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=bf7d5a5d6cbc393017c8f5255e3b317c70853c85&selectedTaskRun=WIBfL1RITB6e5ThjdhItYQ.0

Comment 8

[Lasana Murray]

Ran another try job today. Windows still fails:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&selectedTaskRun=DOK0Q_iCSqu1NoOzZ973qg.0

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9172743 [details] [diff] [review]
mochitestsv2.patch

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

We did some adjustments in bug 1662881 so the test is not passing atm

::: mail/test/browser/openpgp/browser_viewMessage.js
@@ +21,5 @@
> +  hasSignedOkIcon,
> +  hasSignedMismatchIcon,
> +  hasNoSignedIcon,
> +  hasEncryptedOkIcon,
> +} = ChromeUtils.import("resource://testing-common/mozmill/OpenPGPHelpers.jsm");

I think it's better to not export a lot of random symbols. Please instead export an OpenPGPTestUtils (and rename) and call the functions as OpenPGPTestUtils.importPublicKey()
That way it's easier to follow what's local and from a library.

@@ +42,5 @@
> +  "chrome://openpgp/content/modules/uidHelper.jsm"
> +);
> +
> +const MSG_TEXT = "Sundays are nothing without callaloo.";
> +const PREF_OPENPGP_KEY_ID = "openpgp_key_id";

Please just inline the PREF_OPENPGP_KEY_ID string

@@ +122,5 @@
> +  );
> +});
> +
> +add_task(async function testOpenNoPGPSecurity() {
> +  let mc = await openEMLFile("unsigned-unencrypted-from-bob-to-alice");

I'm not a fan of helper functions for tiny functionality. You get a lot of indirection and it's harder to understand what's going on.

let mc = open_message_from_file(new FileUtils.File(`data/keys/unsigned-unencrypted-from-bob-to-alice.eml`));

... is just much easier to figure out and really long enough to use a few helpers for.

@@ +123,5 @@
> +});
> +
> +add_task(async function testOpenNoPGPSecurity() {
> +  let mc = await openEMLFile("unsigned-unencrypted-from-bob-to-alice");
> +  testHasMsgTxt(mc);

also for the tests avoid unnecessary helpers - which now due to how it's called through a helper, will show errors for the wrong lines (line of the helper failure!) and not the right function it's testing

Just using 
Assert.ok(getMsgBodyTxt(mc).includes(MSG_TEXT), "message text is in body"); 

directly there would be cleaner

Comment 10

[Lasana Murray]

Attachment: mochitestsv3.patch

Update for feedback and recent UI changes. I believe I figured out the windows issue, movemail can't be used on windows.

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9175591 [details] [diff] [review]
mochitestsv3.patch

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

I've fixed a few things per below. Will land it soon! We can now start adding more test cases, yay!

::: mail/test/browser/openpgp/browser_viewMessage.js
@@ +23,5 @@
> +const { EnigmailKeyRing } = ChromeUtils.import(
> +  "chrome://openpgp/content/modules/keyRing.jsm"
> +);
> +const { EnigmailFiles } = ChromeUtils.import(
> +  "chrome://openpgp/content/modules/files.jsm"

some of these imports are not needed

@@ +45,5 @@
> +
> +let aliceIdentity;
> +let initialKeyIdPref = "";
> +
> +add_task(async function setUp(module) {

setup module was a mozmill thing.... there never a module now

@@ +118,5 @@
> +  );
> +  close_window(mc);
> +});
> +
> +add_task(async function testOpenSignedByVerifiedUnencryptedWithAutocrypt() {

I'll remove the Autocrypt tests since they don't really do anything. We could add tests around Autocrypt, that you could e.g. import the key from there, but just testing that things were the same with and without the header is not useful enough to run a test for I think.

@@ +349,5 @@
> +  );
> +  close_window(mc);
> +});
> +
> +registerCleanupFunction(function tearDown() {

needed to remove the account too, otherwise ./mach test --verify will fail.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8581dab5fac5
Add initial mochitests for opening various OpenPGP messages. r=mkmelin

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9175591 [details] [diff] [review]
mochitestsv3.patch

[Approval Request Comment]
Test only, will give us more confidence OpenPGP works on 78.

Comment 14

[Wayne Mery (:wsmwk)]

Comment on attachment 9175591 [details] [diff] [review]
mochitestsv3.patch

[Triage Comment]
Approved for esr78

Comment 15

[Rob Lemley [:rjl]]

Thunderbird 78.3.2:
https://hg.mozilla.org/releases/comm-esr78/rev/bcb89c53ba99