Skip to content
This repository has been archived by the owner on Dec 6, 2023. It is now read-only.

Permission handling does not take into account if a resource already exists or not #13

Open
michielbdejong opened this issue Dec 28, 2021 · 7 comments
Labels
bug Something isn't working

Comments

@michielbdejong
Copy link
Member

Implementing Editor’s Draft, 2021-09-11 of Web Access Control

Continuation of https://github.com/solid/community-server/issues/618 where we concluded:

Currently permission handling does not take into account if a resource already exists or not. PUT will always require a Write permission, PATCH will require Append if it only has inserts, and Write if it also has deletes (and POST will always require Append). In case these should differ based on the resource already existing, existence checks will have to be added in the permission parsers.

@michielbdejong michielbdejong added the bug Something isn't working label Dec 28, 2021
@michielbdejong
Copy link
Member Author

Using a root ACL like this for testing:

@prefix acl: <http://www.w3.org/ns/auth/acl#>.
@prefix foaf: <http://xmlns.com/foaf/0.1/>.
<#container> a acl:Authorization ;
  acl:agentClass foaf:Agent ;
  acl:accessTo </>;
  acl:mode
    acl:Read,
    acl:Write,
    acl:Control.
<#resource> a acl:Authorization ;
  acl:agentClass foaf:Agent ;
  acl:default </>;
  acl:mode
    acl:Read,
    acl:Write,
    acl:Control.
curl -v -X PUT -H 'Content-Type: text/turtle' -T root-acl.ttl http://localhost:3000/.acl

@michielbdejong
Copy link
Member Author

And then seeing if a request like this succeeds or not:

curl -v -X PUT -H 'Content-Type: text/plain' -d 'Hello' http://localhost:3000/test.txt

@michielbdejong
Copy link
Member Author

The current code assumes there is only one ACL to look at, and it determines that based on the URI of the request (PermissionBasedAuthorizer, PathBasedReader). This code is really hard to spelonk, but adding some debug statements to find out what calls what.
Adding a debug statement to src/authorization/WebAclReader and then npm run build ; npm start works.

@michielbdejong
Copy link
Member Author

npm start -- -l silly

@michielbdejong
Copy link
Member Author

src/authorization/permissions/MethodModesExtractor.ts determines that:

  • any GET or HEAD request requires only read on the resource (I think this is correct)
  • any PUT or DELETE request requires write, append, create, and/or(?) delete on the resource (not sure how these map to the RAWC permissions specified in WAC).
  • any POST request requires append on the resource.

There is also src/authorization/permissions/SparqlPatchModesExtractor which says an append-only PATCH requires only append on the resource (this only true if the resource already exists) and a PATCH that needs write requires write, append, create, and/or(?) delete on the resource.

src/authorization/permissions/Permissions.ts defines read, append, write, create, delete.

I think a better model would be read/add/remove, and:

  1. on the target resource:
  • GET / HEAD requires AccessModes.read
  • PUT requires AccessModes.add and AccessModes.remove (we don't consider append-only put)
  • append-only PATCH requires AccessModes.add
  • append-and-remove PATCH requires AccessModes.add and AccessModes.remove (we don't consider read permission)
  • DELETE requires AccessModes.remove
  • POST requires AccessModes.add (we don't consider permissions on the resource that gets created)
  1. on any container that gets created:
  • requires AccessModes.add on that container
  1. on any containment triple that gets added:
  • requires AccessModes.add on that container
  1. any containment triple that gets removed:
  • requires AccessModes.remove on that container

And then:

  • for ACL resources, WAC.Control on the resource it belongs to gives AccessModes.read, AccessModes.add, AccessModes.remove
  • for all other resource:
    • WAC.Read gives AccessModes.read
    • WAC.Append gives AccessModes.add
    • WAC.Write gives AccessModes.add and AccessModes.remove

@michielbdejong
Copy link
Member Author

To do: adapt the unit tests there.

@michielbdejong
Copy link
Member Author

Second draft of what this could look like:
CommunitySolidServer/CommunitySolidServer@main...michielbdejong:permissions-refactor

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant