Allow container creation via PUT #513

dmitrizagidulin · 2017-06-29T14:07:25Z

No description provided.

- WIP (fix standard warnings) - WIP (moving oidcConfig to config.json) - Add OIDC Issuer discovery link header on OPTIONS - Refactor OIDC client/handler code - Bump dep version, fix gitignore - Integrate Sign In w WebID page with OIDC login - Fix OIDC create user functionality - Fix storing redirectTo URL in session - Remove unneeded options obj (oidc.js) - WIP (create account using webid as OIDC _id) - Fix extraneous oidcIssuerHeader - WIP (fix response types) - Fix token() params - Fix authCallback() - WIP (switch to implicit workflow) - WIP (fix registration configs) - wip - Switch to authz code flow - Move createOIDCUser to oidc-rp-client (from identity-provider) - Implement OIDC /signout support - Do not create a Solid account folder if OIDC user creation fails - WIP - signin session / resume workflow - Signin api: send 500 error if oidc client not initialized - Remove invalid test - Implement user creation via api - Add username & pw auth logic to signin.js - Add bcrypt compilation Travis CI directives - Mount OIDC Identity Provider API into solid server routes - Initialize multi-rp client store collections - Change oidc packages to use github links - Disable the certificate test for the time being - Derive session cookie domain from root uri - Bump oidc-op-express dep - Output auth method at startup - Implement injected Provider logout() behavior - Remove legacy signout logic - Refactoring OIDC initialization from server config in create-app - Simplify createApp() - Move unit tests to their own test/unit/ folder - Refactor lib/api/authn/ files - Move tests to test/unit/ and test/integration/ - Implement CreateOidcAccountRequest and unit tests - Add OIDC account creation integration tests - Extract OidcManager code to external lib - Fix Provider Discover integration tests - Fix existing Login workflow integration test - Implement DiscoverProviderRequest, refactor - Extract login logic to LoginByPasswordRequest - Unit tests for LoginByPasswordRequest - Implement login via either username or webid - Add /login integration tests - Normalize webid if needed during provider discovery - Render Obtain Consent handlebars view - Save provider config on startup, install nyc / code coverage - Move options handler to before authentication or account api - Ensure local client registered on startup

* Add WWW-Authenticate to exposed CORS headers * Fix 401 error handling, add tests * Add a test for expired token error * Update to supertest 3.0.0 * Convert 401 tests to Promise interface * Rewrite error-pages handler, add tests * Change 401 error logic on empty bearer token * Return a 400 error on empty bearer token header

* Make ./data the default root folder * Add ./data to .gitignore * Fix typo in help text

The patch handler contains logic for multiple request body types. Moving them to separate files facilitates adding support for more types.

The PATCH handler for application/sparql does not perform any patching, i.e., nothing is ever written to disk. Since our next goal is to further abstract patching code (which includes writing), this incomplete code cannot be used.

This reduces the task of individual patch handlers to parsing the patch and applying it.

dmitrizagidulin · 2017-06-29T14:10:59Z

cc @dan-f @RubenVerborgh for review.

dan-f

When the LDP container already exists, it looks like you re-generate the .meta file and return a 204. But based on the current implementation, the .meta file is overwritten and the rest of the contained data remains unchanged, due to how mkdirp works.

RubenVerborgh

No other remarks from me; I missed the issue @dan-f found.

RubenVerborgh · 2017-06-29T17:18:12Z

lib/ldp.js

-    if (filePath.endsWith('/')) {
-      return callback(error(409,
-        'PUT not supported on containers, use POST instead'))
+    const statusCode = fs.existsSync(filePath) ? 204 : 201


Nitpick, but might want to have this async?

If you prefer. I used a sync operation here because exists is a pretty lightweight operation, and it avoids introducing another level of callback nesting to the function. Happy to change it tho.

Yes, indeed… I don't mind either way. Can't wait to switch to Node 8 though, where sync and async have essentially the same syntax 😄

Rule of thumb is async when available for code handling a request, sync for everything else.

dmitrizagidulin · 2017-06-29T17:21:02Z

When the LDP container already exists, it looks like you re-generate the .meta file and return a 204. But based on the current implementation, the .meta file is overwritten and the rest of the contained data remains unchanged, due to how mkdirp works.

@dan-f Are those not the expected semantics? What would you like to see happen instead?

dan-f · 2017-06-29T17:19:30Z

test/integration/acl-tls.js

        rm('/acl-tls/write-acl/empty-acl/another-empty-folder/test-file.acl')
        rm('/acl-tls/write-acl/empty-acl/test-folder/test-file')
        rm('/acl-tls/write-acl/empty-acl/test-file')
        rm('/acl-tls/write-acl/test-file')
        rm('/acl-tls/write-acl/test-file.acl')
+        rm('/acl-tls/write-acl/empty-acl/test-folder2/')


You've got this in the before and after. Not really sure what the intention is here.

dan-f · 2017-06-29T17:20:40Z

test/integration/http.js

+
+      after(() => {
+        rm('/foo/')
+      })


Again, not sure why this is in both the before and after.

It's in the before and after to ensure the directory is clear before the tests run; fairly standard procedure. Happy to take it out if you think it excessive.

I understand cleanup is necessary, but I've never had to do it both before and after a test is run. Either should suffice.

Np, will take the before() out.

Oh I see, these are before/after all the tests in the block, not each test individually. I think this intersects with the issue of shared state between tests.

dan-f · 2017-06-29T17:23:21Z

test/integration/http.js

+          .then(() => {
+            let stats = fs.statSync(path.join(__dirname, '../resources/foo/two/'))
+
+            if (!stats.isDirectory()) {


You can just write expect(statsisDirectory()).toBe(true) if you're using the expect library.

dan-f · 2017-06-29T17:25:36Z

test/integration/http.js

+            let metaFilePath = path.join(__dirname, '../resources/foo/four/.meta')
+            let meta = fs.readFileSync(metaFilePath, 'utf8')
+
+            assert.equal(meta, containerMeta)


I think @deiu has pointed this out before, but testing for graph equality based on serialization can be brittle. Better to use something like rdflib to assert that the graphs are equal.

I agree, for tests of graph equality. But there isn't any parsing or serialization taking place here. This is just straight file read/write operations.

Can I point out that hardcoding .meta is probably a bad idea? I think the config options contains a variable with the name for the meta document.

That's an implementation detail of the fact that node-solid-server doesn't yet use a database. It's generally best for integration tests not to assume any particular implementation.

dan-f · 2017-06-29T17:26:46Z

test/integration/http.js

+
+            assert.equal(meta, newMeta)
+          })
+      })


Missing a test for the case in which there are already graphs in the container.

dan-f · 2017-06-29T17:26:59Z

test/integration/ldp.js

+    })
+
+    after(() => {
+      rm('sampleContainer/example1-copy.ttl')


dan-f · 2017-06-29T17:30:39Z

test/integration/ldp.js

+      })
+    })
+
+    it('should update existing container meta', done => {


I'd much prefer if we didn't share state between tests. Having a test rely on the output of a previous test introduces a possible race condition, especially since, depending on the test runner, tests may be run concurrently.

This is actually okay; within a single describe block, we can have multiple assertions (and in fact, it's in general better to split assertions as much as possible).

I'm not objecting to multiple assertions - I'm objecting to the fact that this test (the code in the callback passed to it) relies on the fact that some other test created some data on the file system.

Ah yes, the scoping should be different. That's what I dislike about super-agent: it makes it hard to do that separation right (first create context, then assert).

dan-f · 2017-06-29T17:36:50Z

@dan-f Are those not the expected semantics? What would you like to see happen instead?

Well, I'm not exactly sure, as the LDP spec doesn't seem to specify what to do when the container already exists.

That said, I'd expect either a) to recreate the container (removing the existing container entirely) or not to modify it at all. The current implementation rewrites the .meta graph and nothing else, which seems a bit unintuitive.

dmitrizagidulin · 2017-06-29T20:24:44Z

Well, I'm not exactly sure, as the LDP spec doesn't seem to specify what to do when the container already exists.

That said, I'd expect either a) to recreate the container (removing the existing container entirely) or not to modify it at all. The current implementation rewrites the .meta graph and nothing else, which seems a bit unintuitive.

Maybe @timbl can clarify Solid semantics of PUT to a container..? (with regards to the .meta etc)?

RubenVerborgh · 2018-05-04T22:43:09Z

Would need a serious rebase.

dmitrizagidulin added 30 commits April 28, 2017 15:51

Extract /logout handler to LogoutRequest

2efc42f

Move Provider initialization logic

2aa41c1

Handle user manually going to /login without app

639828b

Extract auth api logic to oidc-auth-manager

1fb1a86

Add --db-path config option

ad6add0

Move oidc-manager test from unit to integration

308061a

Refactor config defaults. (webid: true by default)

ef9ef9a

Move default-account-template/ to default-templates/new-account

ffe2716

Move default-email-templates/ to default-templates/emails

f60abe9

Add --config-path parameter (for default templates and apps)

a4ee756

Serve public common/ dir (for shared CSS files, etc)

72b5e27

Add boostrap.min.css v3.3.7 to common/css/

2a14d94

Make views/ customizable like templates

01827fe

Rename DiscoverProviderRequest to SelectProviderRequest

a069f6f

Move goodbye.html to views

d3dfaf1

Add a /login default view

3746048

Add db configs to speed up integration tests

379c8c3

Display errors on the login form

a2ff736

Pass returnToUrl to /register, refactor

03f6b79

Add /register view

2e51a9d

Redirect to original url after account creation

47dd697

Fix logout handler

fe498cc

Add a default index.html for server root

76d6d17

Initialize a default index.html for new user accounts

9740b84

Bump oidc-auth-manager dep (moved from bcrypt to bcryptjs)

9ce5635

Pre-populate provider on select-provider view

eba36da

Extract SelectProviderRequest and auth callback code to Auth Manager

189b623

Add serverUri to startup debug, bump auth manager dep

ebcf600

Add tests

9e11367

dmitrizagidulin and others added 11 commits June 7, 2017 11:21

Make ./data the default root folder (#510)

1d8e88a

* Make ./data the default root folder * Add ./data to .gitignore * Fix typo in help text

Move patch handlers to separate files.

a381d21

The patch handler contains logic for multiple request body types. Moving them to separate files facilitates adding support for more types.

Use same patch logic regardless of content type.

7ae8282

Use "415 Unsupported Media Type" for unsupported patches.

2bd65fb

Deduplicate graph reading code.

aa49b04

Remove incomplete SPARQL PATCH handler.

750817b

The PATCH handler for application/sparql does not perform any patching, i.e., nothing is ever written to disk. Since our next goal is to further abstract patching code (which includes writing), this incomplete code cannot be used.

Move patch writing to generic PATCH handler.

6d8d770

This reduces the task of individual patch handlers to parsing the patch and applying it.

Delegate body parsing to middleware.

1a3b676

Prettify patch code.

c09b89f

Allow container creation via PUT

b5e2364

dan-f suggested changes Jun 29, 2017

View reviewed changes

RubenVerborgh approved these changes Jun 29, 2017

View reviewed changes

dan-f reviewed Jun 29, 2017

View reviewed changes

dmitrizagidulin added 4 commits June 30, 2017 10:53

Simplify isDirectory check

9bbd170

Remove before() cleanup

05acfb8

Make the PUT update test self-contained

35ab538

Do not hardcode the .meta suffix

b9f0b31

dmitrizagidulin force-pushed the dz_oidc branch from c09b89f to c92c6ad Compare June 30, 2017 19:55

dmitrizagidulin mentioned this pull request Aug 17, 2017

Remove the Container .meta mechanism #547

Closed

3 tasks

RubenVerborgh changed the base branch from dz_oidc to release/v4.0.0 September 3, 2017 23:33

RubenVerborgh closed this May 4, 2018

csarven mentioned this pull request Jan 9, 2019

Support container updates #1040

Open

dmitrizagidulin mentioned this pull request Feb 19, 2019

Support LDPC via HTTP PUT #1108

Closed

csarven mentioned this pull request Aug 17, 2020

Support creating a Container using PUT #1465

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow container creation via PUT #513

Allow container creation via PUT #513

dmitrizagidulin commented Jun 29, 2017

dmitrizagidulin commented Jun 29, 2017

dan-f left a comment

RubenVerborgh left a comment

RubenVerborgh Jun 29, 2017

dmitrizagidulin Jun 29, 2017

RubenVerborgh Jun 29, 2017

dan-f Jun 29, 2017

dmitrizagidulin commented Jun 29, 2017

dan-f Jun 29, 2017

dan-f Jun 29, 2017

dmitrizagidulin Jun 29, 2017

dan-f Jun 29, 2017 •

edited

Loading

dmitrizagidulin Jun 29, 2017

dan-f Jun 29, 2017

dan-f Jun 29, 2017

dmitrizagidulin Jun 29, 2017

dan-f Jun 29, 2017

dmitrizagidulin Jun 29, 2017

deiu Jun 29, 2017 •

edited

Loading

dan-f Jun 29, 2017

dan-f Jun 29, 2017

dan-f Jun 29, 2017

dan-f Jun 29, 2017

RubenVerborgh Jun 29, 2017

dan-f Jun 29, 2017

RubenVerborgh Jun 29, 2017

dan-f commented Jun 29, 2017

dmitrizagidulin commented Jun 29, 2017

RubenVerborgh commented May 4, 2018

Allow container creation via PUT #513

Allow container creation via PUT #513

Conversation

dmitrizagidulin commented Jun 29, 2017

dmitrizagidulin commented Jun 29, 2017

dan-f left a comment

Choose a reason for hiding this comment

RubenVerborgh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitrizagidulin commented Jun 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dan-f Jun 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deiu Jun 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dan-f commented Jun 29, 2017

dmitrizagidulin commented Jun 29, 2017

RubenVerborgh commented May 4, 2018

dan-f Jun 29, 2017 •

edited

Loading

deiu Jun 29, 2017 •

edited

Loading