Closed Bug 1651492 Opened 4 years ago Closed 4 years ago

add mochitest for viewing openpgp messages

Categories

(MailNews Core :: Security: OpenPGP, task)

Tracking

(thunderbird_esr78+ fixed)

RESOLVED FIXED
82 Branch
Tracking Status
thunderbird_esr78 + fixed

People

(Reporter: mkmelin, Assigned: lasana)

Details

Attachments

(1 file, 2 obsolete files)

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

Status: NEW → ASSIGNED

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!).

Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(kaie)

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

Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(kaie)
Attached patch mochitests.patch (obsolete) — Splinter Review

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.

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

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

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 alice@example.com-0xAE3019F3B2E3EBD8-pub.asc (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.
Attachment #9171287 - Flags: feedback?(mkmelin+mozilla)
Attached patch mochitestsv2.patch (obsolete) — Splinter Review

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

Attachment #9172743 - Flags: review?(mkmelin+mozilla)

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

Attachment #9171287 - Attachment is obsolete: true
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
Attachment #9172743 - Flags: review?(mkmelin+mozilla) → feedback+

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

Attachment #9172743 - Attachment is obsolete: true
Attachment #9175591 - Flags: review?(mkmelin+mozilla)
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.
Attachment #9175591 - Flags: review?(mkmelin+mozilla) → review+
Target Milestone: --- → 82 Branch

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/8581dab5fac5
Add initial mochitests for opening various OpenPGP messages. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED

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

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

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

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

[Triage Comment]
Approved for esr78

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

Attachment

General

Creator:
Created:
Updated:
Size: