Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 3952 improve text clarity with password protected email #3953

Conversation

martgil
Copy link
Collaborator

@martgil martgil commented Sep 3, 2021

This PR will improve text clarity with password-protected email

close #3952 // if this PR closes an issue


Tests (delete all except exactly one):

  • Does not need tests (refactor only, docs or internal changes)
  • Tests will be added later (issue #...)

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

  • addresses the issue it closes (if any)
  • code is readable and understandable
  • is accompanied with tests, or tests are not needed
  • is free of vulnerabilities
  • is documented clearly and usefully, or doesn't need documentation

@@ -231,8 +231,8 @@ export class EncryptedMsgMailFormatter extends BaseMailFormatter {
text.push(intro + '\n');
html.push(Xss.escape(intro).replace(/\n/g, '<br>') + '<br><br>');
}
text.push(Lang.compose.msgEncryptedText[lang] + msgUrl + '\n\n');
html.push(`${Lang.compose.msgEncryptedHtml[lang] + a}<br/><br/>${Lang.compose.alternativelyCopyPaste[lang] + Xss.escape(msgUrl)}<br/><br/>`);
text.push(Lang.compose.msgEncryptedText(lang, Xss.escape(this.acctEmail)) + msgUrl + '\n\n');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, acctEmail isn't same as senderEmail (as one account can have aliases, so getSender() method exists, input_from visual element appears for such accounts), so we should actually implement and test that the text in the message matches the sender email.
We can copy some of the lines from the test 'standalone - different send from, new signed message, verification in mock' to

  1. select an alias to send from (@'input-from')
  2. SettingsPageRecipe.addKeyTest -- not needed
  3. send -- we can take password sending lines from the test 'compose - PWD encrypted message with flowcrypt.com/api' (file upload is not needed).
  4. verification of the resulting message text.
    Verification actually happens in a different place.
    The mock that accepts the sent message uses TestBySubjectStrategyContext constructor to select an obejct (based on partial text from the message subject) to further test the message.
    So if (subject.includes('PWD encrypted message with flowcrypt.com/api')) it will get to PwdEncryptedMessageWithFlowCryptComApiTestStrategy. -- I added a couple of lines there to check that the text matches the sender email -- please pull the change.

So, please implement 3 tests:

  1. compatibility account, from flowcrypt.compatibility...
  2. compatibility account, from flowcryptcompatibility... alias
  3. ci.tests.gmail account -- this account doesn't have any aliases, so no input_from is visible. Make sure it is handled correctly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy sir, @rrrooommmaaa. Thanks for the very helpful context here. I'll be working on this as soon as possible.

msgEncryptedHtml: { EN: 'This&nbsp;message&nbsp;is&nbsp;encrypted: ', DE: 'Diese&nbsp;Nachricht&nbsp;ist&nbsp;verschlüsselt: ' },
msgEncryptedText: { EN: 'This message is encrypted. Follow this link to open it: ', DE: 'Diese Nachricht ist verschlüsselt. Nachricht öffnen: ' },
msgEncryptedHtml: (lang: string, acctEmail: string) => lang === 'EN' ? `${acctEmail}&nbsp;has&nbsp;sent&nbsp;you&nbsp;a&nbsp;password-encrypted&nbsp;email ` : `${acctEmail}&nbsp;hat&nbsp;Ihnen&nbsp;eine&nbsp;passwortverschlüsselte&nbsp;E-Mail&nbsp;gesendet `,
msgEncryptedText: (lang: string, acctEmail: string) => lang === 'EN' ? `${acctEmail} has sent you a password-encrypted email. Follow this link to open it: ` : `${acctEmail} hat Ihnen eine passwortverschlüsselte E-Mail gesendet. Folgen Sie diesem Link, um es zu öffnen `,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, makes more sense to test lang === 'DE', so that all other languages would default to English

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yes! It should definitely work like that! I'll be working on it. Thank you sir!

@martgil
Copy link
Collaborator Author

martgil commented Sep 8, 2021

sir, @rrrooommmaaa. I was able to follow your given instructions which found helpful as usual. When I tried to get combine it and try to read the sent email by email subject using

      const sentMsg = (await GoogleData.withInitializedData(senderEmail)).getMessageBySubject(subject)!;

From here I'm getting undefined as the returned value. I found out that the function getMessageBySubject pulls some data from AcctDataFile? Is there any chance that I need to create another file in the project which contains a message and subject that matches the same subject that I'm including in this test? The specific subject that I'm using is

      const subject = 'PWD encrypted message with flowcrypt.com/api';

which was later could be use in TestBySubjectStrategyContext

Thanks in advance.

@github-actions
Copy link

github-actions bot commented Sep 8, 2021

There were 1 email addresses found in the above comment. Please:

  1. Click three dots -> edit to remove the email addresses.
  2. Click edited in the comment header, and click on the previous revision of the comment.
  3. When viewing the old revision with an email in it, click options -> delete this revision from history.

@rrrooommmaaa
Copy link
Contributor

Hi @martgil I think we don't need to extract the sent message like this.
Messages with a subject containing 'PWD encrypted message with flowcrypt.com/api' will go to PwdEncryptedMessageWithFlowCryptComApiTestStrategy. It doesn't save the message to DATA, like some other "strategies" do, like SaveMessageInStorageStrategy.
Other strategies just make some checks and possibly throw an exception, thus making the test fail.
It's enough to set breakpoint in the PwdEncryptedMessageWithFlowCryptComApiTestStrategy, make sure it is actually hit (I've already added a check to compare text with sender e-mail address there -- you should see if you pulled my update) and see if any exception is thrown there in all 3 tests. This is what I meant by 'verification happens in some other place' -- that is, we don't need to verify in the test body. We can verify in this strategy -- it has all the necessary input -- incoming e-mail with "From" and the text. We can simply leave a comment in the test body like Tom did:

      // this test is using PwdEncryptedMessageWithFlowCryptComApiTestStrategy to check sent result based on subject "PWD encrypted message with flowcrypt.com/api"

…text-clarity-with-password-protected-email
@martgil
Copy link
Collaborator Author

martgil commented Sep 9, 2021

Ah, you're right @rrrooommmaaa. I finally get it now :) Thank you so much!!

@rrrooommmaaa rrrooommmaaa merged commit c26e338 into master Sep 9, 2021
@rrrooommmaaa rrrooommmaaa deleted the issue-3952-improve-text-clarity-with-password-protected-email branch September 9, 2021 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

improve text sent with password protected email
2 participants