search

MatrixAI / js-encryptedfs

Encrypted Filesystem for TypeScript/JavaScript Applications
https://polykey.com
Apache License 2.0
10 stars 3 forks source link

Webworkers for Async and Jest test migration #19

Closed robert-cronin closed 4 years ago

robert-cronin commented 4 years ago

This PR is a combination of webworkers async and bringing the ava tests from VFS and migrating them to jest in EFS. This allows us to be somewhat certain that EFS exposes the same functionality as VFS.

Fixes #4 Fixes #20 Fixes #21

robert-cronin commented 4 years ago

There are still a few tests that need investigating:

CMCDragonkai commented 4 years ago

Continue with the last few tests, I'll get back to you on the review next week.

CMCDragonkai commented 4 years ago

We talked about the process context problem that real filesystems have. To model this in a VFS, we had to simulate these functions in the VFS constructor.

However since EFS is a hybrid of VFS and the real FS. Or any FS at all. Then it cannot expect that these operations (uid, gid, cwd) exist on the fs itself. They were just put into VFS for simulation purposes.

Therefore I suggest that you allow the caller to provide an object/interface to these functions. It's easy enough that VFS can be supplied twice and EFS can call this, and in the case of real fs, you supply the process object.

This means EFS takes 4 things:

  1. The upper fs
  2. The upper fs context
  3. The lower fs
  4. The lower fs context

So I'm guessing this in the TODO list. But you need to reference the issue for this problem in this PR so that way when we read it, it's possible to follow the hyperlinks.

CMCDragonkai commented 4 years ago

Final thing is that I don't see any confirmation on whether the last tests you said are failing are still failing or we are just putting them into another issue to solve later.

robert-cronin commented 4 years ago

The stream tests are integrated, there are still permissions tests left that we can put on a separate branch.

The context control parameters are already added to the constructor but the way they are used in efs will have to be worked out. For now it make sense to merge first as the context control and permissions functionality have nothing or little to do with migration to jest.

Context control is already captured in issue #18. Probably worth adding a note about the relevant tests.

CMCDragonkai commented 4 years ago

You couldn't import it? Perhaps you're missing some tsconfig or webpack config?

On 8 May 2020 13:55:39 GMT+10:00, Robbie Cronin notifications@github.com wrote:

@robert-cronin commented on this pull request.

@@ -0,0 +1,337 @@ +import { Buffer } from 'buffer' +import { nextTick } from 'process' +import { Readable, Writable } from 'stream'

Done. This can only be done with const { Readable, Writable } = require('readable-stream')

-- You are receiving this because your review was requested. Reply to this email directly or view it on GitHub: https://github.com/MatrixAI/js-encryptedfs/pull/19#discussion_r421922730

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

CMCDragonkai commented 4 years ago

can't we use fs type on both?

On 8 May 2020 12:59:42 GMT+10:00, Robbie Cronin notifications@github.com wrote:

@robert-cronin commented on this pull request.

}, "devDependencies": {

  • "@types/jest": "^24.9.1",
  • "@types/jest": "^25.0.0", "@types/node": "^13.13.1",

this is now resolved by using the fs type on lower directory and an any type on the upper directory

-- You are receiving this because your review was requested. Reply to this email directly or view it on GitHub: https://github.com/MatrixAI/js-encryptedfs/pull/19#discussion_r421909002

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

robert-cronin commented 4 years ago

You couldn't import it? Perhaps you're missing some tsconfig or webpack config? … On 8 May 2020 13:55:39 GMT+10:00, Robbie Cronin @.***> wrote: @robert-cronin commented on this pull request. > @@ -0,0 +1,337 @@ +import { Buffer } from 'buffer' +import { nextTick } from 'process' +import { Readable, Writable } from 'stream' Done. This can only be done with const { Readable, Writable } = require('readable-stream') -- You are receiving this because your review was requested. Reply to this email directly or view it on GitHub: #19 (comment) -- Sent from my Android device with K-9 Mail. Please excuse my brevity.

I was able to import it with import { Buffer } from 'buffer/'

CMCDragonkai commented 4 years ago

Can you squash this MR down to 1 or a few commits? It's time to merge this.

robert-cronin commented 4 years ago

This is now squashed into 3 distinct commits: webworkers, jest migration and docs. Ready for merging :+1:

CMCDragonkai commented 4 years ago

Congrats.

CMCDragonkai commented 4 years ago

This project isn't finished yet though! We still need to polish and ensure our crypto works properly.

robert-cronin commented 4 years ago

yes, I am researching GCM now, it will be essential to ensure EFS encryption/decryption conforms to GCM standards