Skip to content

Comments

include multiple keys from a single backup#3973

Merged
rrrooommmaaa merged 70 commits intomasterfrom
issue-2482-backup-should-include-multiple-keys
Oct 19, 2021
Merged

include multiple keys from a single backup#3973
rrrooommmaaa merged 70 commits intomasterfrom
issue-2482-backup-should-include-multiple-keys

Conversation

@martgil
Copy link
Collaborator

@martgil martgil commented Sep 13, 2021

This PR will:

  • brings back the green backup button for accounts with multiple private keys but the other doesn't have backup.
  • backup multiple keys in a single file when the multiple private keys are detected but few are only having backup.

close #2482 // if this PR closes an issue

issue #2806 // if it doesn't close the issue yet

Task list:

  • backups should contain multiple keys in single file
  • all protected by the same passphrase. (uncertain. At the moment, I use the passphrase of key[0] which I believe the active key of the user to backup all the keys)
  • Let user uncheck keys they don't want to back up
  • Fix button not currently shown.
  • Missing tests for checkboxes; submitted keys after performing a successful backup

Tests (delete all except exactly one):

  • Tests will be added later (issue #...)
  • Tests added or updated

** Incomplete functionalities from issue #2482

  • Get currently used key and it's associated passphrase when encrypting multiple backup keys.
  • Render a list of backup (probably a checkboxes) for users to include/exclude keys

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
Copy link
Collaborator Author

martgil commented Sep 13, 2021

I am really uncertain of this line code. This line of code should be responsible for getting the current primary key being used by the logged-in user. But here I have temporarily use the first primary key which I'm totally uncertain if was really correct by encrypting the backup using the passphrase of the first primary key found.

const pp = await PassphraseStore.get(this.view.acctEmail, primaryKeys[0].fingerprints[0]);

@martgil
Copy link
Collaborator Author

martgil commented Sep 13, 2021

Can I also ask for a suggestion on how to generally approach fixing CI-related issues especially on "GMAIL Live tests"?

I already tried running Gmail live tests on my local machine but I think I have missed something.

npm run test_ci_chrome_consumer_live_gmail

flowcrypt-browser@8.1.4 test_ci_chrome_consumer_live_gmail
node ./node_modules/ava/cli --timeout=20m --verbose --concurrency=1 build/test/test/source/test.js -- CONSUMER-LIVE-GMAIL STANDARD-GROUP

TEST_VARIANT: CONSUMER-LIVE-GMAIL:STANDARD-GROUP, (build dir: build/chrome-consumer, poolSizeOne: false)
consts:  {"TIMEOUT_SHORT":60000,"TIMEOUT_EACH_RETRY":180000,"TIMEOUT_ALL_RETRIES":780000,"TIMEOUT_OVERALL":840000,"ATTEMPTS":3,"POOL_SIZE":3,"IS_LOCAL_DEBUG":false} 

  ✖ No tests found in build/test/test/source/test.js

@github-actions
Copy link

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.

@limonte
Copy link

limonte commented Sep 13, 2021

Can I also ask for a suggestion on how to generally approach fixing CI-related issues especially on "GMAIL Live tests"?

I already tried running Gmail live tests on my local machine but I think I have missed something.

npm run test_ci_chrome_consumer_live_gmail

flowcrypt-browser@8.1.4 test_ci_chrome_consumer_live_gmail
node ./node_modules/ava/cli --timeout=20m --verbose --concurrency=1 build/test/test/source/test.js -- CONSUMER-LIVE-GMAIL STANDARD-GROUP

TEST_VARIANT: CONSUMER-LIVE-GMAIL:STANDARD-GROUP, (build dir: build/chrome-consumer, poolSizeOne: false)
consts:  {"TIMEOUT_SHORT":60000,"TIMEOUT_EACH_RETRY":180000,"TIMEOUT_ALL_RETRIES":780000,"TIMEOUT_OVERALL":840000,"ATTEMPTS":3,"POOL_SIZE":3,"IS_LOCAL_DEBUG":false} 

  ✖ No tests found in build/test/test/source/test.js

The problem with Gmail tests is unrelated to this PR and has been fixed. I re-run tests for this PR, they are passing now.

@github-actions
Copy link

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.

@martgil
Copy link
Collaborator Author

martgil commented Sep 14, 2021

Thanks a lot for taking a look, @limonte. The tests are passing now.

Kind Regards,
Mart Gil

@martgil martgil marked this pull request as draft September 14, 2021 06:00
$('.key_backup_selection').append(dom);
}
$('.input_prvkey_backup_checkbox').click((event) => {
const email = String($($($(event.target).siblings()[0]).children()[0]).children()[0].innerText);
Copy link

@limonte limonte Sep 14, 2021

Choose a reason for hiding this comment

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

Instead of this spaghetti code I'd recommend this:

<input class="input_prvkey_backup_checkbox" type="checkbox" data-email="${primaryKi.emails}">
const email = $(event.target).data('email');

@lgtm-com
Copy link

lgtm-com bot commented Oct 13, 2021

This pull request fixes 1 alert when merging 118f5c4 into aeb1be6 - view on LGTM.com

fixed alerts:

  • 1 for Missing await

@lgtm-com
Copy link

lgtm-com bot commented Oct 13, 2021

This pull request fixes 1 alert when merging 911cb96 into aeb1be6 - view on LGTM.com

fixed alerts:

  • 1 for Missing await

@lgtm-com
Copy link

lgtm-com bot commented Oct 13, 2021

This pull request fixes 1 alert when merging 3de18e9 into aeb1be6 - view on LGTM.com

fixed alerts:

  • 1 for Missing await

@lgtm-com
Copy link

lgtm-com bot commented Oct 15, 2021

This pull request fixes 1 alert when merging 99b549c into c44ccdb - view on LGTM.com

fixed alerts:

  • 1 for Missing await

@lgtm-com
Copy link

lgtm-com bot commented Oct 15, 2021

This pull request fixes 1 alert when merging c268bd4 into 37b5415 - view on LGTM.com

fixed alerts:

  • 1 for Missing await

@lgtm-com
Copy link

lgtm-com bot commented Oct 15, 2021

This pull request fixes 1 alert when merging e2cc9a7 into 37b5415 - view on LGTM.com

fixed alerts:

  • 1 for Missing await

@lgtm-com
Copy link

lgtm-com bot commented Oct 17, 2021

This pull request fixes 1 alert when merging 0e777b8 into 37b5415 - view on LGTM.com

fixed alerts:

  • 1 for Missing await

@lgtm-com
Copy link

lgtm-com bot commented Oct 17, 2021

This pull request fixes 1 alert when merging edfda59 into 37b5415 - view on LGTM.com

fixed alerts:

  • 1 for Missing await

@lgtm-com
Copy link

lgtm-com bot commented Oct 18, 2021

This pull request fixes 1 alert when merging f865df2 into 37b5415 - view on LGTM.com

fixed alerts:

  • 1 for Missing await

@lgtm-com
Copy link

lgtm-com bot commented Oct 18, 2021

This pull request fixes 1 alert when merging b143450 into 37b5415 - view on LGTM.com

fixed alerts:

  • 1 for Missing await

@lgtm-com
Copy link

lgtm-com bot commented Oct 18, 2021

This pull request fixes 1 alert when merging 60746b0 into 37b5415 - view on LGTM.com

fixed alerts:

  • 1 for Missing await

@lgtm-com
Copy link

lgtm-com bot commented Oct 18, 2021

This pull request fixes 1 alert when merging 4ca44cd into 37b5415 - view on LGTM.com

fixed alerts:

  • 1 for Missing await

@rrrooommmaaa rrrooommmaaa merged commit f0198c0 into master Oct 19, 2021
@rrrooommmaaa rrrooommmaaa deleted the issue-2482-backup-should-include-multiple-keys branch October 19, 2021 08:45
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.

back up several/all keys

4 participants