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

saved browsers settings #865

Merged
merged 22 commits into from
Apr 9, 2021
Merged

saved browsers settings #865

merged 22 commits into from
Apr 9, 2021

Conversation

tanmoyAtb
Copy link
Contributor

closes #847

@render
Copy link

render bot commented Apr 1, 2021

@render
Copy link

render bot commented Apr 1, 2021

@render
Copy link

render bot commented Apr 1, 2021

@tanmoyAtb tanmoyAtb self-assigned this Apr 1, 2021
@tanmoyAtb tanmoyAtb marked this pull request as ready for review April 8, 2021 17:34
@Tbaut
Copy link
Collaborator

Tbaut commented Apr 9, 2021

Instead of having many comments and suggestions, I pushed the following changes, that should not change any logic:

  • code styling nits
  • use useCallback and promise.then.catch when only 1 await was used
  • console.error in promise
  • show the hour when the browser got added
  • use CSS instead of  
  • removed a weird await (here) before promise.then.catch and added a .finally

@Tbaut Tbaut added Status: Changes Requested ❌ Added to PRs that require further changes from the contributor. Status: Review Needed 👀 Added to PRs when they need more review labels Apr 9, 2021
Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

Now I've tested the functionality and there's something that either I don't understand, or that doesn't work:
I had 2 browser saved, I downloaded the backup phrases, and deleted the browser shares:
image

If I refresh, I'm still logged-in automatically -> from my understanding this shouldn't be possible
If I manually log-out, then try to login and chose to use a backup phrase, both backup phrases that I just downloaded don't work.
I login with the password then, that works, I'm not asked to save the browser, and I end up with the same settings as the screenshot above, refreshing logs me in automatically, so I think my browser definitely got saved.

@FSM1
Copy link
Contributor

FSM1 commented Apr 9, 2021

Now I've tested the functionality and there's something that either I don't understand, or that doesn't work:
I had 2 browser saved, I downloaded the backup phrases, and deleted the browser shares:

  1. If I refresh, I'm still logged-in automatically -> from my understanding this shouldn't be possible
  2. If I manually log-out, then try to login and chose to use a backup phrase, both backup phrases that I just downloaded don't work.
  3. I login with the password then, that works, I'm not asked to save the browser, and I end up with the same settings as the screenshot above, refreshing logs me in automatically, so I think my browser definitely got saved.

Some of these are expected behaviour. I have numbered the items so it's easier to follow:

  1. The auto-login is dependent on the serialized tkey store being in session storage. The final tkey private key is available there, so the device shares, and their presence/absence is irrelevant.

  2. The device share was deleted, so even though you have one of the co-ords for the second point, share reconstruction requires both the x and y co-ords for 2 points. When a share is deleted, the y co-ord for that point is deleted from the metadata rendering that share useless in reconstructing the key.

  3. Refreshing works whether the device is saved or not. Saving device is only used during key reconstruction, which session restoration with no session storage. My guess is that the browser is actually not saved, and if you start a new session you will be required to enter a password again. It seems that under certain circumstances the isNewDevice flag is not being set correctly, resulting in a successful login, but no ability to save the device.

Logging an issue to investigate 3 but do not feel that this edge case has anything to do with the PR at hand

@Tbaut
Copy link
Collaborator

Tbaut commented Apr 9, 2021

Understood for the 1 thanks, didn't think about the session storage.
For 2 I think it might be a mistake to let users download these backup phrases without further information, because they are not real backup phrases like the others, they are half of one iiuc?. There's this "if" that we never mention anywhere which is "if you didn't delete the browser share linked to the mnemonic, then it'll work".

@FSM1
Copy link
Contributor

FSM1 commented Apr 9, 2021

Understood for the 1 thanks, didn't think about the session storage.
For 2 I think it might be a mistake to let users download these backup phrases without further information, because they are not real backup phrases like the others, they are half of one iiuc?. There's this "if" that we never mention anywhere which is "if you didn't delete the browser share linked to the mnemonic, then it'll work".

Hmm, I think adding more explanation and text to the app will only confuse people more to be honest.

To me it feels quite natural that a mnemonic that is linked to a device share would not work if the device share is deleted, but maybe I am just too close to the mechanics of it...

@Tbaut
Copy link
Collaborator

Tbaut commented Apr 9, 2021

The problem I think is to call everything a backup phrase, and have in the end different behaviors. Don't want to block this PR further, it's not directly related. I think if I got confused, others might be. Agree that too much text doesn't help. However, a popup to confirm the deletion of a browser could tell "Look any browser recovery mnemonic that was generated will not work any more".
I think this would be small enough for users to care reading. And if they don't.. then at least we tried.

@tanmoyAtb
Copy link
Contributor Author

tanmoyAtb commented Apr 9, 2021

I agree, that a user will keep a mnemonic stored somewhere and expect it to work even if he deletes something! at lease that was my first impulse.

Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

Working well and looking good 🎉

@tanmoyAtb tanmoyAtb merged commit f00634f into dev Apr 9, 2021
@tanmoyAtb tanmoyAtb deleted the feat/saved-browsers-847 branch April 9, 2021 14:49
FSM1 added a commit that referenced this pull request Apr 12, 2021
Crash when going back after having selected Google auth #860 (PR #871)
Login optimizations (PR #879)
Minor fixes + Updates to color manipulator (#874)
Merge pull request #879 from ChainSafe/fix/login-optimizations
Merge pull request #887 from ChainSafe/fix/tbaut-hover-color
Merge pull request #886 from ChainSafe/feat/tkey-darkmode
Remove code duplication (#889)
Merge pull request #895 from ChainSafe/fix/select-all-892
Merge pull request #896 from ChainSafe/fix/mouse-highlight-issue-890
Merge pull request #894 from ChainSafe/fix/mobile-spinner-893
Merge pull request #877 from ChainSafe/feat/tbaut-settings-security-846
folder validation (#899)
saved browsers settings (#865)
Prevent the whole app from rerendering every 5s (#909)
Merge pull request #911 from ChainSafe/fix/share-transfer-modal-on-ta… …
Merge pull request #905 from ChainSafe/fix/bin-file-view-873 …
Merge pull request #906 from ChainSafe/feat/home-folder-nav-885 …
Merge pull request #907 from ChainSafe/fix/search-bucket-to-reset-on-…
Fix: Search results all show as files (#914) …
Fix double click opening the next file preview (#915) …
Rename tooltip and fixes (#912) …
@FSM1 FSM1 removed Status: Changes Requested ❌ Added to PRs that require further changes from the contributor. Status: Review Needed 👀 Added to PRs when they need more review labels Apr 12, 2021
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.

Saved browser section in Settings > Security
3 participants