issue #4097 add warning when manually importing public keys by limonte · Pull Request #4110 · FlowCrypt/flowcrypt-browser

limonte · 2021-11-10T16:32:22Z

This PR adds an orange warning when manually importing public keys. It also fixes the broken layout of chrome/elements/pgp_pubkey.htm

close #4097
close #4104

Tests (delete all except exactly one):

Does not need tests (refactor only, docs or internal changes)

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

rrrooommmaaa

The Bulk Import screenshot says "verify that the fingerprint matches" however, the fingerprint itself isn't displayed -- not very user-friendly.
Also, we have add_pubkey.htm dialog invoked from composer as I can see (example is demonstrated in compose - settings - manually copied pubkey test and some other tests). Apart from "Copy from Contact" options, there are also options to load from file or paste directly without any warnings whatsoever -- not even showing the email address inside the key/certificate. Perhaps, we should make use of pgp_pubkey.htm frame here somwehow -- file it in a separate issue?

rrrooommmaaa · 2021-11-11T10:58:10Z

Actually, the "Copy from Contact" option is looking ugly -- I'm filing an issue for that too

tomholub · 2021-11-12T13:36:10Z

Thank you for these observations.

@limonte Let's do the following in this PR:

The Bulk Import screenshot says "verify that the fingerprint matches" however, the fingerprint itself isn't displayed -- not very user-friendly.

Let's change the sentence in bulk import to: Manually importing Public Keys can be dangerous

Also, we have add_pubkey.htm dialog invoked from composer as I can see (example is demonstrated in compose - settings - manually copied pubkey test and some other tests). Apart from "Copy from Contact" options, there are also options to load from file or paste directly without any warnings whatsoever

add_pubkey.htm please add orange warning Manually importing Public Keys can be dangerous

Additionally in this PR:

Please change the color of Update Key in pgp_pubkey.htm to orange
similarly button color in bulk import to be orange
the OK button in add_pubkey.htm also orange

That should cover it. Didn't notice this when writing the issue. Thanks!

limonte · 2021-11-12T22:49:30Z

Let's change the sentence in bulk import to: Manually importing Public Keys can be dangerous

Done:

add_pubkey.htm please add orange warning Manually importing Public Keys can be dangerous

Done:

Please change the color of Update Key in pgp_pubkey.htm to orange

Done:

similarly button color in bulk import to be orange

Done, see screenshot above.

the OK button in add_pubkey.htm also orange

Done, see screenshot above.

Thank you both for the detailed review and suggestions!

I personally have one UX concern. On both bulk import and add_pubkey.htm pages, we mention that "Manually importing Public Keys can be dangerous" without any further details answering "Why?" and "What to do about it?". Here's what GitHub co-pilot was suggesting:

It would be indeed helpful to create a small page with the information about importing and link that page on bulk import and add_pubkey.htm pages.

tomholub · 2021-11-12T22:50:38Z

Good idea about explaining it. I'll file an issue.

limonte · 2021-11-12T22:56:07Z

extension/chrome/settings/modules/contacts.ts

        }
        container.css('display', 'block');
-        $('#bulk_import .input_pubkey, #bulk_import .action_process, #file_import #fineuploader_button').css('display', 'none');
+        $('#bulk_import .input_pubkey, #bulk_import .action_process, #file_import, #fineuploader_button').css('display', 'none');


This is the fix for the unrelated issue when the unwanted block was shown after doing bulk import:

* issue #4097 add warning when manually importing public keys * Improve wording, make import buttons orange

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

limonte requested a review from rrrooommmaaa as a code owner November 10, 2021 16:32

rrrooommmaaa suggested changes Nov 11, 2021

View reviewed changes

rrrooommmaaa mentioned this pull request Nov 11, 2021

"Copy from Contact" option in add_pubkey.htm shows list with duplicate emails #4112

Closed

Limon Monte added 2 commits November 12, 2021 22:30

issue #4097 add warning when manually importing public keys

13f5e2c

Improve wording, make import buttons orange

4afdb86

limonte force-pushed the issue-4097-import-pubkey-warning branch from 319a217 to 4afdb86 Compare November 12, 2021 22:42

tomholub mentioned this pull request Nov 12, 2021

explain the "importing pubkey dangerous" notes with more nuance #4114

Closed

limonte commented Nov 12, 2021

View reviewed changes

tomholub requested a review from rrrooommmaaa November 12, 2021 23:05

rrrooommmaaa approved these changes Nov 13, 2021

View reviewed changes

rrrooommmaaa merged commit 9b13e4b into master Nov 13, 2021

rrrooommmaaa deleted the issue-4097-import-pubkey-warning branch November 13, 2021 07:06

tomholub mentioned this pull request Nov 13, 2021

add warning when manually importing public keys FlowCrypt/flowcrypt-android#1544

Closed

rrrooommmaaa pushed a commit that referenced this pull request Nov 14, 2021

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

a61b0b3

* issue #4097 add warning when manually importing public keys * Improve wording, make import buttons orange

limonte mentioned this pull request Dec 6, 2021

Fix UI for manually importing public keys #4168

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

issue #4097 add warning when manually importing public keys#4110

issue #4097 add warning when manually importing public keys#4110
rrrooommmaaa merged 2 commits intomasterfrom
issue-4097-import-pubkey-warning

limonte commented Nov 10, 2021

Uh oh!

rrrooommmaaa left a comment •

edited

Loading

Uh oh!

rrrooommmaaa commented Nov 11, 2021

Uh oh!

tomholub commented Nov 12, 2021 •

edited

Loading

Uh oh!

limonte commented Nov 12, 2021

Uh oh!

tomholub commented Nov 12, 2021

Uh oh!

limonte Nov 12, 2021 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

limonte commented Nov 10, 2021

To be filled by reviewers

Uh oh!

rrrooommmaaa left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rrrooommmaaa commented Nov 11, 2021

Uh oh!

tomholub commented Nov 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

limonte commented Nov 12, 2021

Uh oh!

tomholub commented Nov 12, 2021

Uh oh!

limonte Nov 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rrrooommmaaa left a comment •

edited

Loading

tomholub commented Nov 12, 2021 •

edited

Loading

limonte Nov 12, 2021 •

edited

Loading