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.

Sign up for GitHub

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

Jump to bottom

Make DELETE require write permissions on container #1014

Merged
merged 3 commits into from
Dec 10, 2018
Merged

Make DELETE require write permissions on container #1014

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
84dbff3
Make DELETE require write permissions on container
rubensworks Dec 6, 2018
22de319
Make logic for testing delete permissions less confusing
rubensworks Dec 7, 2018
8e90a7d
Improving variable naming in allow handler
rubensworks Dec 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion lib/handlers/allow.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
module.exports = allow

const $rdf = require('rdflib')
const path = require('path')
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, relativePath = '') {
return async function allowHandler (req, res, next) {
const ldp = req.app.locals.ldp || {}
if (!ldp.webid) {
Expand All @@ -23,6 +24,11 @@ function allow (mode) {
? res.locals.path
: req.path

// If a relativePath has been provided, check permissions based on that
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment out of sync?

if (relativePath) {
reqPath = path.join(reqPath, relativePath)
}

// Check whether the resource exists
let stat
try {
Expand Down
2 changes: 1 addition & 1 deletion lib/ldp-middleware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

Not sure, but wouldn't ./ be the path to the container?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

OK; perhaps you can have a look too, @RubenVerborgh ?


return router
}