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 #3885 add checkbox per email for attester key submission #3907

Merged
merged 76 commits into from Nov 12, 2021

Conversation

martgil
Copy link
Collaborator

@martgil martgil commented Aug 13, 2021

This PR grants the ability for the user to choose which email will be sent from the attester, preventing all the email aliases to be submitted.

close #3885 // if this PR closes an issue


Tests (delete all except exactly one):

  • Does not need tests (refactor only, docs or internal changes)
  • Difficult to test (explain why)
  • Not worth testing
  • Tests will be added later (issue #...)
  • Tests added or updated

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

@martgil martgil self-assigned this Aug 13, 2021
@martgil martgil marked this pull request as draft August 13, 2021 07:36
@martgil martgil changed the title add checkbox per email for attester key submission issue #3885 add checkbox per email for attester key submission Aug 13, 2021
@lgtm-com
Copy link

lgtm-com bot commented Aug 13, 2021

This pull request introduces 1 alert when merging 35a110b into 5b6b44c - view on LGTM.com

new alerts:

  • 1 for Client-side cross-site scripting

@limonte
Copy link
Contributor

limonte commented Aug 13, 2021

@martgil when you start working on a new branch, make sure that the master branch is up-to-date. You're currently working from the outdated master branch, that's why tests are failing.

Now you need to either rebase your work or do the merge commit.

@martgil
Copy link
Collaborator Author

martgil commented Aug 16, 2021

Hello,

I have tried to find an account with more than 1 email address/ alias but haven't found one. I suspect that there isn't a test for multiple email alias/addresses exist yet? Is there any hint on this?

Thanks in advance!

@martgil martgil marked this pull request as ready for review August 18, 2021 09:10
@martgil
Copy link
Collaborator Author

martgil commented Aug 18, 2021

The ability to include and exclude alias from being submitted to the attester is fully functional. But I haven't added a specific test for checking if there is a checkbox present if the user is in action-step1easyormanual-choose-manual-create phase because I haven't found an email that contains multiple alias/addresses until now.

Any suggestion will be appreciated. Thank you in advance 🙇

@martgil martgil marked this pull request as draft August 24, 2021 05:38
@tomholub
Copy link
Collaborator

Mart, somewhere in the mock server codebase, in google endpoints, will be a sendAs endpoint which returns a list, which always only has the primary email address. You could add a conditional there, so that for a particular account it returns more than just one email.

@martgil martgil marked this pull request as ready for review August 26, 2021 07:54
test/source/tests/setup.ts Outdated Show resolved Hide resolved
test/source/tests/setup.ts Outdated Show resolved Hide resolved
test/source/tests/setup.ts Outdated Show resolved Hide resolved
extension/chrome/settings/setup/setup-render.ts Outdated Show resolved Hide resolved
@martgil
Copy link
Collaborator Author

martgil commented Aug 31, 2021

Hello sir @rrrooommmaaa, I hope you're doing well. I can almost finish all the necessary changes here and I was able do detect multiple emails from the actionImportPrivateKeyHandle in setup-import-key.ts which was nearly on the final stage of the test.

I have some difficulty when trying to call the .prop() and other methods from a suppose JQuery<HtmlElement> in be4fcb7. I have tried some of the type casting method that I've known and I still get an error saying that I cannot convert the type HtmlElement to type JQuery<HtmlElement>. Thank you in advanced for any hint.

@rrrooommmaaa
Copy link
Contributor

I have some difficulty when trying to call the .prop() and other methods from a suppose JQuery<HtmlElement> in 7afacfb. I have tried some of the type casting method that I've known and I still get an error saying that I cannot convert the type HtmlElement to type JQuery<HtmlElement>. Thank you in advanced for any hint.

Perhaps, getAttribute() can be called on HtmlElement object. I'm sure @limonte has more knowledge on this.

@tomholub
Copy link
Collaborator

tomholub commented Oct 1, 2021

at the scenario #3 I think this is where the `submitKeyForAddrs where not populated causing the "import emails as contact" and the test 'settings - my own emails show as contacts' to fail.

Exactly, when a key is recovered from the inbox, there is no information about whether the key should be connected to all aliases, as in scenarios 1 and 2 this information isn't saved to backup. So we need to re-think it, @tomholub. If the "submit" option is respected for ContactStore on import, we should this option for recover from backup too?

For the purpose of this PR, let's the easiest option from code perspective. Then we can discuss the rest in a separate issue.

@rrrooommmaaa
Copy link
Contributor

@martgil is this ready for review or are there some questions pending?

@martgil
Copy link
Collaborator Author

martgil commented Oct 6, 2021

@martgil is this ready for review or are there some questions pending?

Yes sir, @rrrooommmaaa , I still have a question. I am uncertain how could I fixed the issue of "adding the current user as a contact". While this PR pulls all the addresses and adds them all as contacts. I can't find a clear way yet to deselect email aliases from a backup.

@martgil
Copy link
Collaborator Author

martgil commented Oct 20, 2021

Hello sir @rrrooommmaaa, I just fixed the later merge conflict error. I think this is now ready for a review. Thank you!

test/source/tests/page-recipe/setup-page-recipe.ts Outdated Show resolved Hide resolved
test/source/tests/setup.ts Outdated Show resolved Hide resolved
@tomholub tomholub marked this pull request as draft October 26, 2021 12:45
test/source/tests/setup.ts Outdated Show resolved Hide resolved
extension/chrome/settings/setup/setup-create-key.ts Outdated Show resolved Hide resolved
@tomholub tomholub marked this pull request as ready for review November 12, 2021 22:02
@tomholub tomholub merged commit 948ff5c into master Nov 12, 2021
@tomholub tomholub deleted the issue-3885-smart-alias-submission branch November 12, 2021 22:24
rrrooommmaaa added a commit that referenced this pull request Nov 14, 2021
* add checkbox per email for attester key submission

* fixes tslint error

* fixes tslint error

* add xss-escaped comment to pass pattern checks

* Added test for issue-3885 selectable email aliases to submit on attester

* Added a private key with two UIDs

* refactor data-test naming and add multi email alias account to google-endpoint.ts

* fix tslint error

* fix failing test on setup-page-recipe

* Added test for importing key with multiple email alias (incomplete)

* fixes tslint formatting error

* complete test for importing key with multiple email alias

* rename data-test and class container

* rename data-test and class container

* separate the test to CONSUMER-MOCK test variant

* move the render display function to key-import-ui.ts

* emails for checkboxes are default to 'unchecked' state

* remove accidental console.log

* Added automatic check/uncheck when an email is present.

* fixes tslint by adding interface property

* add event of keyup paste and change to manipulate checkboxes

* change button color from gray to green when valid private key

* remove checkEmailAliasIfPresent and uses fillOnly

* use fillOnly completely

* bring back checkEmailAliasIfPresent and wrap it in fillOnly

* rename css class name to avoid interfering with any className based checking.

* added key with multiple aliases

* simplified code and move data-test to label input

* corrected any type to string

* remove spagetti code and better checking for submitkeyforaddrs

* exclude email (uncheck checkbox) before submitting

* attester pubkey for multi alias user (failing)

* added test "setup - imported key from a file with multiple alias"

* added test if excluded email was submitted from the attester

* collect submitted keys from attester

* patch data-test (selector) trasnformer to match/replace any provided selector

* move data-test directly to the input

* fix inconsistency in checking detected email alias

* manipulate test key and added 1 UID

* complete neccessary tests by checking default detected key states

* refactor email alias process [floating-promise-error] in constructor setup.ts (key-import-ui initPrvImportSrcForm)

* remove comment

* parse aliases via already rendered input checkbox

* patched 'saveKeysAndPassPhrase' on setup.ts

* fix conflict

* fix conflict

* fixes eslint

* added callback when performing tests

* remove unnecessary undefined initialization

* fix pubkey definition

* uncheck the checkbox when submit pubkey was set to false

* better flow for pubkey submission

* check for checkbox state in the first run

* fix test title typo

* reverted back changes [proposed solution]

* clean up setup procedure

Co-authored-by: Roman Shevchenko <rrrooommmaaa@mail.ru>
Co-authored-by: Mart Gil Robles <mart@Marts-MacBook-Air.local>
Co-authored-by: Tom J <6306961+tomholub@users.noreply.github.com>
Co-authored-by: Tom <tom@flowcrypt.com>
Co-authored-by: Tom J <tom@holub.me>
tomholub added a commit that referenced this pull request Nov 16, 2021
…4109)

* test cancelling passphrase dialog in compose

* Change the 'keyup' events handlers to 'input' because text content can be changed with the mouse (#4100)

* Change the 'keyup' handlers to 'input' because text content can be changed with the mouse

* trigger 'input' events

* Upgrade to Puppeteer 11 (#4093)

* Upgrade to Puppeteer 11

* unsuccessful attempt to refactor createSecureDraft, add todo for the future

* Refactor pageHasSecureDraft() to use getFrame() instead of opening new tab

* use getFrame() in Thunderbird tests

* workaround sending in the 'secure reply btn, reply draft' test

* Simplify openGmailPage()

* fail faster - add timeout param to waitAndClick()

* wip

* handle 'Node is either not clickable' error

* wait longer for @action-step0 and @action-step1

* wait longer for @input-compatibility-fix-expire-years

* wip

* always delete local draft after sending

* cleanup

* rename mock live test

* log

* fix 'secure reply btn, reply draft' test

* cleanup

* do not rely on sleep timeouts

* typo

* timeout in seconds

* let composeBox: Controllable | undefined

* #4052 passphrase dialog for non-primary S/MIME signing

* Do not show post-it reminder for EKM (#4103)

* Do not show post-it reminder for EKM

* upd tests

* enterPp.expectPostitNoteReminder

* Remove expectPostitNoteReminder and related check

* Fix the 'Reply' button behavior, align it with Gmail (#4107)

* Fix the 'Reply' button behavior, align it with Gmail

* copy before iterating

* Add tests

* add clearRecipientsForReply() to initComposeBox()

* Revert

* fix the reply button behavior

* await promise

* Add live gmail test for the reply icon button

* simpler namings (#4111)

* align 'reply all' behavior with Gmail

Co-authored-by: Roman <rrrooommmaaa@mail.ru>

* issue #3885 add checkbox per email for attester key submission (#3907)

* add checkbox per email for attester key submission

* fixes tslint error

* fixes tslint error

* add xss-escaped comment to pass pattern checks

* Added test for issue-3885 selectable email aliases to submit on attester

* Added a private key with two UIDs

* refactor data-test naming and add multi email alias account to google-endpoint.ts

* fix tslint error

* fix failing test on setup-page-recipe

* Added test for importing key with multiple email alias (incomplete)

* fixes tslint formatting error

* complete test for importing key with multiple email alias

* rename data-test and class container

* rename data-test and class container

* separate the test to CONSUMER-MOCK test variant

* move the render display function to key-import-ui.ts

* emails for checkboxes are default to 'unchecked' state

* remove accidental console.log

* Added automatic check/uncheck when an email is present.

* fixes tslint by adding interface property

* add event of keyup paste and change to manipulate checkboxes

* change button color from gray to green when valid private key

* remove checkEmailAliasIfPresent and uses fillOnly

* use fillOnly completely

* bring back checkEmailAliasIfPresent and wrap it in fillOnly

* rename css class name to avoid interfering with any className based checking.

* added key with multiple aliases

* simplified code and move data-test to label input

* corrected any type to string

* remove spagetti code and better checking for submitkeyforaddrs

* exclude email (uncheck checkbox) before submitting

* attester pubkey for multi alias user (failing)

* added test "setup - imported key from a file with multiple alias"

* added test if excluded email was submitted from the attester

* collect submitted keys from attester

* patch data-test (selector) trasnformer to match/replace any provided selector

* move data-test directly to the input

* fix inconsistency in checking detected email alias

* manipulate test key and added 1 UID

* complete neccessary tests by checking default detected key states

* refactor email alias process [floating-promise-error] in constructor setup.ts (key-import-ui initPrvImportSrcForm)

* remove comment

* parse aliases via already rendered input checkbox

* patched 'saveKeysAndPassPhrase' on setup.ts

* fix conflict

* fix conflict

* fixes eslint

* added callback when performing tests

* remove unnecessary undefined initialization

* fix pubkey definition

* uncheck the checkbox when submit pubkey was set to false

* better flow for pubkey submission

* check for checkbox state in the first run

* fix test title typo

* reverted back changes [proposed solution]

* clean up setup procedure

Co-authored-by: Roman Shevchenko <rrrooommmaaa@mail.ru>
Co-authored-by: Mart Gil Robles <mart@Marts-MacBook-Air.local>
Co-authored-by: Tom J <6306961+tomholub@users.noreply.github.com>
Co-authored-by: Tom <tom@flowcrypt.com>
Co-authored-by: Tom J <tom@holub.me>

* issue #4097 add warning when manually importing public keys (#4110)

* issue #4097 add warning when manually importing public keys

* Improve wording, make import buttons orange

* more flexible google mock

* allow opening a draft compose box based from inbox.ts in debug mode

* fix

* added filePath option for addKeyTest

* test loading draft with a forgotten non-primary passphrase

* merge fix

* merge fix

* test fix

* remark

Co-authored-by: Limon Monte <limon.monte@gmail.com>
Co-authored-by: martgil <46025304+martgil@users.noreply.github.com>
Co-authored-by: Mart Gil Robles <mart@Marts-MacBook-Air.local>
Co-authored-by: Tom J <6306961+tomholub@users.noreply.github.com>
Co-authored-by: Tom <tom@flowcrypt.com>
Co-authored-by: Tom J <tom@holub.me>
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.

more intelligent submitting for Gmail aliases when importing key
5 participants