Make DELETE require write permissions on container #1014
Conversation
ghost
assigned 
lib/ldp-middleware.js
Outdated
| @@ -26,7 +26,7 @@ function LdpMiddleware (corsSettings) { | |||
| router.post('/*', allow('Append'), post) | |||
| router.patch('/*', allow('Append'), patch) | |||
| router.put('/*', allow('Write'), put) | |||
| router.delete('/*', allow('Write'), del) | |||
| router.delete('/*', allow('Write', '..'), del) | |||
There was a problem hiding this comment.
Not sure, but wouldn't ./ be the path to the container?
There was a problem hiding this comment.
The reason why that was needed is because I used path.join, which considers everything to be a path segment, including the filename itself, so ./ would do nothing.
In any case, I rewrote this PR a bit, as it was indeed a bit confusing.
There was a problem hiding this comment.
OK; perhaps you can have a look too, @RubenVerborgh ?
lib/handlers/allow.js
Outdated
| const ACL = require('../acl-checker') | ||
| const debug = require('../debug.js').ACL | ||
| const fs = require('fs') | ||
| const { promisify } = require('util') | ||
| const HTTPError = require('../http-error') | ||
|
|
||
| function allow (mode) { | ||
| function allow (mode, testDirectory) { |
There was a problem hiding this comment.
Unclear parameter name IMHO
lib/handlers/allow.js
Outdated
| @@ -23,6 +24,11 @@ function allow (mode) { | |||
| ? res.locals.path | |||
| : req.path | |||
|
|
|||
| // If a relativePath has been provided, check permissions based on that | |||
lib/handlers/allow.js
Outdated
| @@ -23,6 +24,11 @@ function allow (mode) { | |||
| ? res.locals.path | |||
| : req.path | |||
|
|
|||
| // If a relativePath has been provided, check permissions based on that | |||
| if (testDirectory) { | |||
| reqPath = path.dirname(reqPath) | |||
There was a problem hiding this comment.
Maybe deceptive to use reqPath for it then. Perhaps should be something like resourcePath.
Too hard in the current architecture. I couldn't find a place where both the allow and delete handlers were already setup in tests, so writing a test for this would require quite a bit of effort (but should be possible to do if we want to invest time in that). All other comments have been resolved. |
DELETE will now basically always check the permissions of the parent directory.
As requested in #729.