Add N3 support for PATCH #516

RubenVerborgh · 2017-07-02T19:43:32Z

This pull request adds support for N3 patches, and contains extensive tests for PATCH.

Added features

exposing ACL object and user ID on the request
fine-grained permission checking of patches after parsing (for both N3 and SPARQL UPDATE)
support for Notation3 patches

Pending decisions

A final vocabulary/vocabularies for N3 patches (options on the Wiki)
Have the right decisions been taken for status codes and permissions? (See Specification below.)
The PATCH handler performs partial permission checking: it assumes that Append permissions are checked, and—after patch parsing—verifies Read and Write permissions when needed for a particular patch. We might want to either double-check Append permissions, or move permission checking into handlers in general.

As future work, we can create an abstraction that allows non-triple-based patches as well. For this, however, I would strongly recommend rewriting handlers in an object-oriented way, as the current pure functional style could become confusing.

Specification


PATCH
  with a patch document
    with an unsupported content type
      ✓ returns HTTP status code 415
      ✓ has "Unsupported patch content type: text/other" in the response
      ✓ does not modify the file
    containing invalid syntax
      ✓ returns HTTP status code 400
      ✓ has "Patch document syntax error" in the response
      ✓ does not modify the file
    without relevant patch element
      ✓ returns HTTP status code 400
      ✓ has "No patch for https://tim.localhost:7777/read-write.ttl found" in the response
      ✓ does not modify the file
    with neither insert nor delete
      ✓ returns HTTP status code 400
      ✓ has "Patch should at least contain inserts or deletes" in the response
      ✓ does not modify the file
  with insert
    on a non-existing file
      ✓ returns HTTP status code 200
      ✓ has "Patch applied successfully" in the response
      ✓ creates the file
      ✓ writes the correct contents
    on a resource with read-only access
      ✓ returns HTTP status code 403
      ✓ has "Access denied" in the response
      ✓ does not modify the file
    on a resource with append-only access
      ✓ returns HTTP status code 200
      ✓ has "Patch applied successfully" in the response
      ✓ patches the file correctly
    on a resource with write-only access
      ✓ returns HTTP status code 200
      ✓ has "Patch applied successfully" in the response
      ✓ patches the file correctly
  with insert and where
    on a non-existing file
      ✓ returns HTTP status code 409
      ✓ has "The patch could not be applied" in the response
      ✓ does not create the file
    on a resource with read-only access
      ✓ returns HTTP status code 403
      ✓ has "Access denied" in the response
      ✓ does not modify the file
    on a resource with append-only access
      ✓ returns HTTP status code 403
      ✓ has "Access denied" in the response
      ✓ does not modify the file
    on a resource with write-only access
      ✓ returns HTTP status code 403
      ✓ has "Access denied" in the response
      ✓ does not modify the file
    on a resource with read-append access
      with a matching WHERE clause
        ✓ returns HTTP status code 200
        ✓ has "Patch applied successfully" in the response
        ✓ patches the file correctly
      with a non-matching WHERE clause
        ✓ returns HTTP status code 409
        ✓ has "The patch could not be applied" in the response
        ✓ does not modify the file
    on a resource with read-write access
      with a matching WHERE clause
        ✓ returns HTTP status code 200
        ✓ has "Patch applied successfully" in the response
        ✓ patches the file correctly
      with a non-matching WHERE clause
        ✓ returns HTTP status code 409
        ✓ has "The patch could not be applied" in the response
        ✓ does not modify the file
  with delete
    on a non-existing file
      ✓ returns HTTP status code 409
      ✓ has "The patch could not be applied" in the response
      ✓ does not create the file
    on a resource with read-only access
      ✓ returns HTTP status code 403
      ✓ has "Access denied" in the response
      ✓ does not modify the file
    on a resource with append-only access
      ✓ returns HTTP status code 403
      ✓ has "Access denied" in the response
      ✓ does not modify the file
    on a resource with write-only access
      ✓ returns HTTP status code 403
      ✓ has "Access denied" in the response
      ✓ does not modify the file
    on a resource with read-append access
      ✓ returns HTTP status code 403
      ✓ has "Access denied" in the response
      ✓ does not modify the file
    on a resource with read-write access
      with a patch for existing data
        ✓ returns HTTP status code 200
        ✓ has "Patch applied successfully" in the response
        ✓ patches the file correctly
      with a patch for non-existing data
        ✓ returns HTTP status code 409
        ✓ has "The patch could not be applied" in the response
        ✓ does not modify the file
      with a matching WHERE clause
        ✓ returns HTTP status code 200
        ✓ has "Patch applied successfully" in the response
        ✓ patches the file correctly
      with a non-matching WHERE clause
        ✓ returns HTTP status code 409
        ✓ has "The patch could not be applied" in the response
        ✓ does not modify the file
  deleting and inserting
    on a non-existing file
      ✓ returns HTTP status code 409
      ✓ has "The patch could not be applied" in the response
      ✓ does not create the file
    on a resource with read-only access
      ✓ returns HTTP status code 403
      ✓ has "Access denied" in the response
      ✓ does not modify the file
    on a resource with append-only access
      ✓ returns HTTP status code 403
      ✓ has "Access denied" in the response
      ✓ does not modify the file
    on a resource with write-only access
      ✓ returns HTTP status code 403
      ✓ has "Access denied" in the response
      ✓ does not modify the file
    on a resource with read-append access
      ✓ returns HTTP status code 403
      ✓ has "Access denied" in the response
      ✓ does not modify the file
    on a resource with read-write access
      executes deletes before inserts
        ✓ returns HTTP status code 409
        ✓ has "The patch could not be applied" in the response
        ✓ does not modify the file
      with a patch for existing data
        ✓ returns HTTP status code 200
        ✓ has "Patch applied successfully" in the response
        ✓ patches the file correctly
      with a patch for non-existing data
        ✓ returns HTTP status code 409
        ✓ has "The patch could not be applied" in the response
        ✓ does not modify the file
      with a matching WHERE clause
        ✓ returns HTTP status code 200
        ✓ has "Patch applied successfully" in the response
        ✓ patches the file correctly
      with a non-matching WHERE clause
        ✓ returns HTTP status code 409
        ✓ has "The patch could not be applied" in the response
        ✓ does not modify the file

This improves reuse for future parsers.

RubenVerborgh · 2017-07-02T20:07:14Z

lib/ldp-middleware.js

  router.get('/*', index, acl.allow('Read'), get)
  router.post('/*', acl.allow('Append'), post)
-  router.patch('/*', acl.allow('Write'), patch)
+  router.patch('/*', acl.allow('Append'), patch)


@dmitrizagidulin This is a permission downgrade that, with other patch handlers, could allow unintended access. The patch handler is assumed to check for Write (and Read) permission when needed after parsing the patch.

(Note that this handler did not check for Read yet.)

Sounds good; the code will check for Write vs Append downstream, as we discussed.

Two tests are skipped; these require an ACL check inside of the handler.

The PATCH handler needs ACL for detailed permission checking, as the full set of needed permissions is unknown until the patch document has been parsed.

Improves readability and editability, and ensures consistency of preparation/cleanup and assertions.

timbl

In the original, if I remember correctly, it was critical to perform the file read/patch/write without letting go of the async thread. Hence the comment "'PATCH -- applied OK (sync)'". This was to avoid conflict with other PATCH requests being handled concurrently. Concerned that the functions to check permissions being written in the promise style, it will be easy to do some I/O operation in them and lose that atomic operation. If we don't do this sync, then we have have to have some form of locking, which I understood was not really the node style.

RubenVerborgh · 2017-07-04T18:38:20Z

Mhm, the sync-ness of the patch handler seems to have been gone for some time already. The file I started working on already performed asynchronous reads and writes (https://github.com/solid/node-solid-server/pull/511/files#diff-d83413e73094e31dc0662b194162262dL160). It seems that the "sync" comment was copied verbatim from above (https://github.com/solid/node-solid-server/pull/511/files#diff-d83413e73094e31dc0662b194162262dL56), but that code never performed any actual writing (synchronous nor asynchronous).

In any case, sync is not an appropriate solution for the atomicity problem, given that any Node.js server can be started with multiple threads, which can interfere. If cross-request atomicity is important for Solid (which I assume it is), we might need to give locking a more prominent place in the overall architecture.

dmitrizagidulin · 2017-07-05T14:36:05Z

@timbl I agree with @RubenVerborgh - switching the PATCH handler code to synchronous reads & writes won't do anything for atomicity; that has to be handled separately and explicitly (via locks / mutexes, etc).

dmitrizagidulin · 2017-07-05T14:39:29Z

lib/handlers/allow.js


    getUserId(req, function (err, userId) {
      if (err) return next(err)
+      req.userId = userId


I don't think you need this part; the userId is already recorded on the request object upstream of this handler.

As we discussed, not in all cases (could be in req.session.userId or locals.oidc.webIdFromClaims(req.claims)). Opened #517 for this.

dmitrizagidulin

Looks good, thanks! 👍

RubenVerborgh · 2017-08-16T21:27:36Z

TODO for @dmitrizagidulin and/or @timbl:

Check whether the decisions regarding status codes are okay (see Specification > Details at the top of this page).

TODO for @dmitrizagidulin:

Check whether the assumptions regarding authentication are fine (see partial permission checking argument above).

TODO For @RubenVerborgh:

Create and publish patch vocabulary

dmitrizagidulin · 2017-08-16T21:33:41Z

Should a PATCH with an insert to a non-existing file return a 201 Created instead of a 200?

RubenVerborgh · 2017-08-16T21:35:21Z

@dmitrizagidulin Seems logical. Only problem might be that a user with only Append permissions (not Read) might not be authorized to know whether the file already existed.

dmitrizagidulin · 2017-08-16T21:38:04Z

Only problem might be that a user with only Append permissions (not Read) might not be authorized to know whether the file already existed.

Good point. Is that a use case we should support, tho? I can only think of one Append-but-not-Read access, and that's the Inbox. And there the main interaction is via POST, not PATCH.

dmitrizagidulin · 2017-08-16T21:39:38Z

👍 on the status codes from me. (aside from the minor conversation about the 201 insert via patch)

RubenVerborgh · 2017-08-17T18:32:58Z

Discussed this with @dmitrizagidulin and we decided to keep the 200.

RubenVerborgh · 2017-08-17T18:39:13Z

Ready, pending approval of solid/vocab#25.

RubenVerborgh · 2017-08-18T13:30:18Z

Currently on hold because of #552.

timbl · 2017-08-18T17:56:40Z

This functionality adds the ability to parse N3 patches. Then, to send them upstream, we need to add an option to the pub/sub protocol to include patches in the upstream info in the websocket. (#554)

Depends on solid/vocab#25.

RubenVerborgh · 2017-08-18T20:03:03Z

#552 has been merged; the rv/patch/n3 branch has been merged with master, so this pull request is ready to be reviewed and merged.

RubenVerborgh · 2017-08-18T20:05:39Z

@dmitrizagidulin The final commit 1aca23f is currently a merge commit of dz_oidc onto rv/patch/n3 (resolving some conflicts). It might simplify history if I make the same merge, but then from rv/patch/n3 onto dz_oidc and then push (instead of merging on GitHub).

RubenVerborgh added 3 commits June 30, 2017 21:13

Refactor SPARQL update patcher with promises.

a3f8a77

This improves reuse for future parsers.

Add preliminary N3 patch support.

71c3101

Construct the patch URI through a hash of its contents.

aed5b6a

RubenVerborgh added enhancement ready for review labels Jul 2, 2017

RubenVerborgh requested a review from dmitrizagidulin July 2, 2017 19:43

RubenVerborgh commented Jul 2, 2017

View reviewed changes

RubenVerborgh added 12 commits July 2, 2017 16:10

Clean up SPARQL UPDATE PATCH tests.

c44c9a1

Set up new PATCH tests.

4d1e746

Syntactically and structurally validate patches.

f04b5ef

Enable and test PATCH appending.

351463d

Use single-user setup for PATCH tests.

338795e

Test PATCH deletion.

4c6c275

Two tests are skipped; these require an ACL check inside of the handler.

Test PATCH combined deletion and insertion.

1d55d0e

Add WHERE support to N3 patches.

c463d73

Refactor patch handler to perform everything but parsing.

d4315b4

Expose ACL and user ID on request.

2a895f6

The PATCH handler needs ACL for detailed permission checking, as the full set of needed permissions is unknown until the patch document has been parsed.

Verify read and write permissions for patches.

9a3597b

Refactor PATCH tests with helper method.

d108753

Improves readability and editability, and ensures consistency of preparation/cleanup and assertions.

RubenVerborgh force-pushed the rv/patch/n3 branch from 215378d to d108753 Compare July 2, 2017 20:11

timbl reviewed Jul 4, 2017

View reviewed changes

dmitrizagidulin reviewed Jul 5, 2017

View reviewed changes

RubenVerborgh mentioned this pull request Jul 5, 2017

Move user ID checking to auth #517

Closed

dmitrizagidulin approved these changes Jul 5, 2017

View reviewed changes

RubenVerborgh removed the ready for review label Jul 27, 2017

RubenVerborgh mentioned this pull request Aug 9, 2017

Expose ACL and user ID on request #528

Merged

RubenVerborgh added this to the 4.0.0 milestone Aug 10, 2017

RubenVerborgh added the in progress label Aug 11, 2017

RubenVerborgh force-pushed the rv/patch/n3 branch 2 times, most recently from f255521 to 6d9e863 Compare August 16, 2017 21:21

RubenVerborgh removed the in progress label Aug 17, 2017

RubenVerborgh added the on-hold label Aug 18, 2017

timbl approved these changes Aug 18, 2017

View reviewed changes

Migrate to Solid vocabulary.

91c8106

Depends on solid/vocab#25.

RubenVerborgh force-pushed the rv/patch/n3 branch from f25a7cc to 1aca23f Compare August 18, 2017 20:01

RubenVerborgh added ready for review and removed on-hold labels Aug 18, 2017

RubenVerborgh assigned dmitrizagidulin Aug 18, 2017

RubenVerborgh force-pushed the rv/patch/n3 branch from 1aca23f to 91c8106 Compare August 18, 2017 20:12

RubenVerborgh merged commit 91c8106 into dz_oidc Aug 18, 2017

RubenVerborgh deleted the rv/patch/n3 branch August 18, 2017 20:13

RubenVerborgh added a commit that referenced this pull request Sep 12, 2017

Add CHANGELOG entry for N3 patches (#516).

f3ceff4

RubenVerborgh mentioned this pull request Apr 3, 2020

Standardizing state changes in resources (history, undo, sync) solid/specification#161

Open

Add N3 support for PATCH #516

Add N3 support for PATCH #516

Uh oh!

Conversation

RubenVerborgh commented Jul 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Added features

Pending decisions

Specification

Uh oh!

RubenVerborgh Jul 2, 2017

Choose a reason for hiding this comment

Uh oh!

dmitrizagidulin Jul 3, 2017

Choose a reason for hiding this comment

Uh oh!

timbl left a comment

Choose a reason for hiding this comment

Uh oh!

RubenVerborgh commented Jul 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmitrizagidulin commented Jul 5, 2017

Uh oh!

dmitrizagidulin Jul 5, 2017

Choose a reason for hiding this comment

Uh oh!

RubenVerborgh Jul 5, 2017

Choose a reason for hiding this comment

Uh oh!

dmitrizagidulin left a comment

Choose a reason for hiding this comment

Uh oh!

RubenVerborgh commented Aug 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmitrizagidulin commented Aug 16, 2017

Uh oh!

RubenVerborgh commented Aug 16, 2017

Uh oh!

dmitrizagidulin commented Aug 16, 2017

Uh oh!

dmitrizagidulin commented Aug 16, 2017

Uh oh!

RubenVerborgh commented Aug 17, 2017

Uh oh!

RubenVerborgh commented Aug 17, 2017

Uh oh!

RubenVerborgh commented Aug 18, 2017

Uh oh!

timbl commented Aug 18, 2017 • edited by RubenVerborgh Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RubenVerborgh commented Aug 18, 2017

Uh oh!

RubenVerborgh commented Aug 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

RubenVerborgh commented Jul 2, 2017 •

edited

Loading

RubenVerborgh commented Jul 4, 2017 •

edited

Loading

RubenVerborgh commented Aug 16, 2017 •

edited

Loading

timbl commented Aug 18, 2017 •

edited by RubenVerborgh

Loading