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.

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

Let Authorizers return allowed permissions #953

Closed
joachimvh opened this issue Sep 9, 2021 · 2 comments
Closed

Let Authorizers return allowed permissions #953

joachimvh opened this issue Sep 9, 2021 · 2 comments
Assignees

Comments

@joachimvh
Copy link
Member

See #938 (comment)

The idea would be that instead of failing or succeeding to indicate if the input permissions are allowed, they instead would return an object indicating for each existing permissions which of these would be allowed.

Some considerations:

  • Do we return an instance of PermissionSet or do we use different terms?
  • Would the LdpHandler then compare the output with the permissions it got from the RequestParser? Or do we scrap the permission parser and let the OperationHandlers reject based on the received permissions?
  • How do we generate the WAC-Allow header then? The main issue is that we need 2 objects there: the permissions for the current credentials AND the permissions for a public agent. We could have the authorizer return both, but this couples the behaviour closer to acl again since we're only doing this for an acl header.
  • Is this a good time to perhaps change the values of PermissionSet? Currently these values are tightly coupled to the acl predicates but perhaps we want to go to more generic terms.
  • The linked comment suggests making a CombinedAuthenticationHandler to combine the results, but we could also do this with a sequence/parallel handler that modified the input object, or create a new generic reduce handler or similar.

Regarding using more generic terms, those would be read, create, write, append, delete instead of the current control, read, write, append. I personally am in favor of making that switch (although I personally think delete doesn't have that much added value when you have write). One issue here would be the control boolean, but perhaps we can make our acl authorizer smart enough to know it has to convert the control value to create/write/delete when the target is an acl resource.

#938 is on hold until this is resolved.

Wondering if this will impact #952 😉

@RubenVerborgh
Copy link
Member

RubenVerborgh commented Sep 10, 2021

  • Do we return an instance of PermissionSet or do we use different terms?

I think we need to return a WebAclAuthorization. But then generalize this class a bit to:

  • not be specific to WebAcl (which it is not, only in name)
  • generalize from user (probably better agent) and everyone to other keys

So technically, WebAclAuthorization could just be (a subclass of) Map<string, PermissionSet>, where some string keys are special.

The addMetadata method can probably go.

  • Would the LdpHandler then compare the output with the permissions it got from the RequestParser?

That one could remain the same IF:

  • Authorizer remains as-is (so AsyncHandler<{ identifier, permissions, credentials }, Authorization>)
  • new interface PermissionReader which is AsyncHandler<{ identifier, credentials }, Authorization>
  • note: instead of identifier, we might want credentials[] as a list for which the PermissionReader should return, concretely (for now) the agent and the public, but might be more later

Then:

  • WebAclAuthorizer becomes WebAclPermissionReader (or WebAclReader for short) implements PermissionReader
  • a new PermissionBasedAuthorizer taking a set of child PermissionReaders in the constructor, which then collects the allowed PermissionSet from each of those children and joins them.

Then this comparison behavior is abstracted away.

Alternatively, we can use the composite pattern to create a UnionPermissionReader that does this collecting and joining, so PermissionBasedAuthorizer purely does the comparison.

Or do we scrap the permission parser and let the OperationHandlers reject based on the received permissions?

That would be solid/web-access-control-spec#97 (comment); let us hold off on that one for the moment; knowing that, indeed, this PR makes it easier/trivial to implement that behavior.

  • How do we generate the WAC-Allow header then?

Someone should get the resulting PermissionSet into the metadata. I see the authorization makes it into an operation, so perhaps we can take it from there. Or perhaps only the GET operation needs to do it?

The main issue is that we need 2 objects there: the permissions for the current credentials AND the permissions for a public agent. We could have the authorizer return both

Yes; and as I wrote above (for PermissionReader), let's even ask specifically to the authorizer which ones we want.

but this couples the behaviour closer to acl again since we're only doing this for an acl header.

I don't think it couples that; we just ask the permission reader to look up permissions for things.

  • Is this a good time to perhaps change the values of PermissionSet? Currently these values are tightly coupled to the acl predicates but perhaps we want to go to more generic terms.

I'm okay at the moment; no strong opinion. Your proposal is fine with me.

The linked comment suggests making a CombinedAuthenticationHandler to combine the results, but we could also do this with a sequence/parallel handler that modified the input object, or create a new generic reduce handler or similar.

I wouldn't reduce here; I don;'t think PermissionReaders should take permission sets as input; they should not need to know how to combine.

@joachimvh
Copy link
Member Author

Done in #962

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants