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

Fetch on demand #1076

Merged
merged 8 commits into from
Oct 27, 2021
Merged

Fetch on demand #1076

merged 8 commits into from
Oct 27, 2021

Conversation

boekkooi-lengoo
Copy link
Contributor

Hey,

This is my second stab at resolving one of the the issues described in #741.
This PR changes the way secrets are loaded by requiring the need of having a password before loading the secret.

Please let me know what you think and if I should move 6dd70b3 to another PR or not.

Cheers,
Warnar

In order to allow for more fine-grained control and better separation of concerns.
The "enter password" part of DisplaySecret is now in it's own function.
This makes understanding the logic just a little easier and also avoid an if in `useEffect`.
Instead of loading a secret based on the key provided in the URL now load it only when we have a password.
By doing this we all users to have a way to send YoPass links using systems that fetch each page that they deliver.
By move `setSecretInfo` into the `try` we ensure that the `finalize` call is also made fixing some edge case when reloading during development.
@codecov
Copy link

codecov bot commented Sep 29, 2021

Codecov Report

Merging #1076 (853dc08) into master (1fc790c) will decrease coverage by 1.83%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1076      +/-   ##
==========================================
- Coverage   80.00%   78.16%   -1.84%     
==========================================
  Files           6        6              
  Lines         390      403      +13     
==========================================
+ Hits          312      315       +3     
- Misses         52       58       +6     
- Partials       26       30       +4     
Impacted Files Coverage Δ
pkg/server/server.go 86.17% <0.00%> (-7.46%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c55fc7...853dc08. Read the comment docs.

@jhaals
Copy link
Owner

jhaals commented Sep 30, 2021

Thanks for taking another stab at this! Will take a review in the coming days 👍

Signed-off-by: Johan Haals <[email protected]>
@jhaals
Copy link
Owner

jhaals commented Oct 12, 2021

@boekkooi-lengoo I have not forgotten this PR, It's just been super busy with work/life lately. However I took a look at this PR last night and made some modifications. I want to give it another go later this week when I have some hack time at work. I think it looks a lot better now, have a look yourself and see if you could spot some ways of improving it. Perhaps the useEffect can be reduced now as well but I haven't tried it out yet.

@boekkooi-lengoo
Copy link
Contributor Author

Hey @jhaals,
No worries having a good work/life balance is very important. I will have a look at your changes in the next couple of days.
However I think by removing the setSecretInfo calls on line 107 and 114 you maybe have re-introduced a bug that I had when using the back and forward buttons with multiple secrets being in the browser history.
Let me see if I can reproduce this and write a testcase for this scenario.

Thanks again for your time and due diligence.

@boekkooi-lengoo
Copy link
Contributor Author

boekkooi-lengoo commented Oct 18, 2021

Hey @jhaals

So I finally got some time and found the issue that your commit re-introduces which is the following.

Go to http://localhost:3000/#/ and create 2 secrets, the first one having One-time download checked and the second unchecked and store the Short links.
Now go to the Short link of the secret where One-time download was unchecked and enter some invalid decryption key.
In the same browser tab paste the Short link of the One-time download was checked and see that the secrets is fetched and decrypted with the same invalid decryption key (FYI I'm using Firefox).

Sorry but I don't have time to write a cypress test for this today but will try to do so this week.
Added the test and it seems the bug was somewhere else 😄

This adds a basic tests showing that a secret is fetched only after it is first entered.
Ensure that the password is reset when the key changes.
@jhaals
Copy link
Owner

jhaals commented Oct 27, 2021

Okay it feels like this has been waiting long enough, nice work adding some tests as well 👍

@jhaals jhaals merged commit f60624a into jhaals:master Oct 27, 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.

2 participants