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

feat(files): encrypted bucket for personal files #1790

Merged
merged 17 commits into from
Mar 10, 2022
Merged

feat(files): encrypted bucket for personal files #1790

merged 17 commits into from
Mar 10, 2022

Conversation

josephmcg
Copy link
Contributor

@josephmcg josephmcg commented Mar 4, 2022

πŸ”΄ Breaking change - need to open a new account or files won't be encrypted

What this PR does πŸ“–

  • encrypt bucket
  • store thumbnails of image files as base64 in files index
    • This allows image previews without downloading the whole image
    • images are scaled down using skaler package before conversion to base64
  • download file and store as File in fileSystem class on more fullscreen view. show loader until dl is ready
  • fix issue where a user could change folders mid upload, causing bugs

Which issue(s) this PR fixes πŸ”¨
AP-984, AP-955, AP-970

Special notes for reviewers πŸ—’οΈ

  • appreciate everyone's feedback, I was able to make the whole process much smoother
  • disabled sharing functionality until I figure out how to handle that. It seems like we probably have to move files to an unencrypted bucket to share them
  • temporarily removing FileSystem tests after adding skaler. jest doesn't play well with canvas, unsure of how to fix it
  • we won't use things like ipfs hash for encrypted buckets, but we may for unencrypted. I left it for now, may remove later

Additional comments 🎀

@github-actions github-actions bot added the Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA label Mar 4, 2022
@netlify
Copy link

netlify bot commented Mar 4, 2022

βœ”οΈ Yeeeehaw, deploy preview is ready!

πŸ”¨ Explore the source changes: eba668e

πŸ” Inspect the deploy log: https://app.netlify.com/sites/adoring-edison-dbcef8/deploys/622a24eee75b9f0008d6da9b

😎 Browse the preview: https://deploy-preview-1790--adoring-edison-dbcef8.netlify.app

@josephmcg josephmcg marked this pull request as draft March 4, 2022 08:22
@josephmcg josephmcg added draft A developer wants eyes on this PR, but they don't think it's ready to merge. and removed Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA labels Mar 4, 2022
@github-actions github-actions bot added the missing fixing conflict Conflict needs to be handled, then re-tested by devs/qa label Mar 4, 2022
@WanderingHogan
Copy link
Contributor

@josephmcg Can you generate thumnails, and only pull/decrypt those on a per folder basis so the slow feeling for the user is reduced as much as possible, and don't decrypt any full size files until the user goes to download, or, in the future, share them?

@josephmcg
Copy link
Contributor Author

@WanderingHogan Also a good idea. I'll play around with it today

@stavares843 stavares843 removed the missing fixing conflict Conflict needs to be handled, then re-tested by devs/qa label Mar 7, 2022
@josephmcg josephmcg marked this pull request as ready for review March 8, 2022 04:48
@josephmcg josephmcg added Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA and removed draft A developer wants eyes on this PR, but they don't think it's ready to merge. labels Mar 8, 2022
@molimauro molimauro added the Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. label Mar 8, 2022
@github-actions github-actions bot added the missing fixing conflict Conflict needs to be handled, then re-tested by devs/qa label Mar 8, 2022
@josephmcg
Copy link
Contributor Author

josephmcg commented Mar 9, 2022

Thanks Phill,

I made a mistake on my guard clause, so svg and heic were both broken. should be better now

I've had issues with that svg in particular as well. I think there's some sort of encoding issue in the header. I adjusted a line in the file then in started working. shared updated version below. it needed xmlns:xlink="http://www.w3.org/1999/xlink"
I've tried other svgs as well (like the one below) and it's been working fine.

card

Test

@josephmcg josephmcg removed the Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. label Mar 9, 2022
@josephmcg
Copy link
Contributor Author

at the moment, we're only supporting the embeddable image types as well as heic since everyone and their mother has an iphone.

We could certainly look into adding a thumbnail for videos, maybe even pdfs in a different ticket

@github-actions github-actions bot added the missing fixing conflict Conflict needs to be handled, then re-tested by devs/qa label Mar 9, 2022
@stavares843 stavares843 removed the missing fixing conflict Conflict needs to be handled, then re-tested by devs/qa label Mar 9, 2022
@phillsatellite
Copy link
Contributor

Hi Joe! I tested again and found something kinda strange, so I uploaded a AVIF and the thumbnail/preview was a still picture, I think uploaded the same file to Dev.Satellite.one and the thumbnail and preview were both animated

local.to.dev.satellite.mov

@phillsatellite phillsatellite added the Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. label Mar 10, 2022
@josephmcg
Copy link
Contributor Author

josephmcg commented Mar 10, 2022

good catch, that seems like a minor issue we could address in a separate ticket. I'd love to get this merged so I can work on additional features.

In the preview, we're no longer accessing the original file. I stored a thumbnail of the image as a base64 string inside the index file. In order to view encrypted files, you would need to pull them down first - slowing performance a bit.

Currently, it will only download the full file if you click and check the full screen view.

@josephmcg josephmcg removed the Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. label Mar 10, 2022
Copy link
Contributor

@WanderingHogan WanderingHogan left a comment

Choose a reason for hiding this comment

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

good job Joe, nice to see this stuff coming together

@WanderingHogan WanderingHogan added Ready for QA Ready for QA team to test, Devs approved. and removed Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA labels Mar 10, 2022
@stavares843 stavares843 added temporary blocked checking something QA Lead is checking something. and removed temporary blocked checking something QA Lead is checking something. labels Mar 10, 2022
@phillsatellite phillsatellite added QA tested One QA Person has tested - Needs QA Lead review still and removed Ready for QA Ready for QA team to test, Devs approved. labels Mar 10, 2022
@stavares843
Copy link
Member

stavares843 commented Mar 10, 2022

added 2 tickets for andre to take a look at the jest related things

AP-1061
AP-1062

libraries/Files/Fil.ts Outdated Show resolved Hide resolved
@stavares843 stavares843 merged commit 3ea1fc7 into dev Mar 10, 2022
@stavares843 stavares843 deleted the AP984 branch March 10, 2022 16:19
@github-actions github-actions bot removed the QA tested One QA Person has tested - Needs QA Lead review still label Mar 10, 2022
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.

None yet

6 participants