-
Notifications
You must be signed in to change notification settings - Fork 2
Policy Endpoints #57
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
Policy Endpoints #57
Conversation
Some APIs might need non-JSON/form input, this allows this possibility. In the future we probably want a more generic solution, but for now this at least allows the possibility.
Due to the RoutedHttpRequestHandler catching unsupported requests with a default handler, it was impossible to combine multiple with a WaterfallHandler. This is now solved by moving most of the behaviour to the canHandle, and having a static throw handler for the 404.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work so far.
Some small changes would help.
After the changes, we can start with DELETE, and then with update (sparql update for patch and complete policy rewriting with PUT)
This prevents Components.js build errors
When looking into contracts in the future, this should be handled in a more robust way.
See the comment in AuxiliaryModesExtractor.ts for more details. This allows policies on resources to also apply to their auxiliary resources, such as .meta resources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small remarks on the docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a small amount of nit picks, but in general it looks really good!
If these are resolved, I think this PR can be merged.
The checkboxes and comments from the docs that are still unresolved by then should become issues then
docs/policy-management.md
Outdated
The PUT works as a combination of DELETE and POST. It requires a body with the same content type as the [POST request](#creating-policies). This body will be interpreted as the requested policy with some rules. | ||
|
||
The PUT process: | ||
1. Find information about the policy. If it does not exist, return with a **status code 400** to indicate that you cannot rewrite a nonexistent policy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I read this now, I am not 100% if this is the behaviour we want.
Either we allow PUT to create a resource if it does not exist yet (following the POST method).
Or we use a 404, as we performed a GET request and did not find any resource.
By typing this out, I think creating the resource (given that the identifier in the policy and the PUT policy-id match).
I would also argue similar behaviour with PATCH if the resource does not exist.
Perhaps @joachimvh has another opinion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's pretty much up to us to determine what we want, so I guess @termontwouter might want to weigh in here as he is writing the new spec. Solid allows creating new resources with PUT, the UMA registration process does not, and only allows you to PUT to existing registered resources (where I agree a 404 would make more sense).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UMA indeed does not allow registering new resources with PUT (since you cannot know the asset id in advance); but this is not an UMA endpoint, so it could make sense to allow PUT to create a completely new policy. Ultimately, this does not matter that much, since it is a "proprietary" endpoint of the AS, which we don't spec; so feel free to implement whatever is easiest.
2. The content type of the body gets validated. The content-type must be set to `application/sparql-query`. Any other type will result in a response with **status code 400**. | ||
3. We use the policy information to create a store which only contains groups **(1)**, **(3)** and **(4)** as explained [above](#get-one-policy). This will serve as an isolated store, on which we can execute the update query. This implementation has its advantages: | ||
- We do not need to validate the sparql query, since we execute it on an isolated store. | ||
- Performing DELETE queries on rules out of your scope will simply not work, since they are not part of the isolated store. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that nothing then will happen?
I understand it as such and think that is valid behaviour.
docs/policy-management.md
Outdated
Some operations require the client to specify a policy ID in the URL. Since policy ID's might contain reserved characters (e.g. `/`, `:`, ...), we have chosen to encode them with the builtin [`encodeURIComponent()` function](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/encodeURIComponent). Using this method, reserved characters will be converted to their respective UTF-8 encodings. | ||
|
||
## Testing | ||
The current implementation is tested only by the script in `scripts\test-uma-ODRL-policy.ts`. This script tests every implemented endpoint with a designated flow. Since the script initiates with an empty storage, and there is no endpoint or other way to seed it, the first requests must test the POST endpoint. These tests are designed to ensure that the storage is filled. After the POST tests, the access endpoints can be tested. Every endpoint gets tested in this script, which makes sure that the added data is removed. The current testing will be replaced with proper unit tests in the near future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and integrations tests of course :)
@@ -20,4 +20,7 @@ export interface UCRulesStorage { | |||
* @returns | |||
*/ | |||
deleteRule: (identifier: string) => Promise<void>; | |||
|
|||
// Experimental endpoint | |||
deleteRuleFromPolicy: (ruleID: string, PolicyID: string) => Promise<void>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the implementation, it seems like it deletes the usage control rule and its reference. Could you update the documentation as such?
Also remove the fact that it is experimental. At some point it will have unit tests and thus will not be experimental.
Could you also update the updateRule definition explaining that it does not delete the reference from policies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this an artefact of using the earliest POST method in combination with the FileStorage solution?
If so, I think this can be removed
|
||
// 2. Retrieve the Policy Body | ||
const contentType = request.headers['content-type']; | ||
if (!/(?:application\/sparql-update)$/i.test(contentType)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it might be useful to use this utility for parsing the content type?
https://github.com/CommunitySolidServer/CommunitySolidServer/blob/main/src/util/HeaderUtil.ts#L381
The CSS is a dependency anyway
// Sanitization | ||
sanitizeRule(policyStore, clientId); | ||
|
||
// 3.1 Check that the other rules are unchanged | ||
const newState = getPolicyInfo(policyId, policyStore, clientId); | ||
if (newState.otherRules.length !== 0 || newState.otherPolicyRules.length !== 0) | ||
throw new BadRequestHttpError("Update not allowed: attempted to modify rules not owned by client"); | ||
|
||
// 3.2 Check that only Policy/Rule changing quads are introduced and removed | ||
// The only modifications we allow are policy definitions, policy rules that define owned rules and owned rules themselves | ||
const newQuads = policyStore.getQuads(null, null, null, null); | ||
if (newQuads.length - newState.ownedRules.length - newState.ownedPolicyRules.length - newState.policyDefinitions.length | ||
!== initialQuads.length - ownedPolicyRules.length - ownedRules.length - policyDefinitions.length) | ||
throw new BadRequestHttpError("Update not allowed: this query introduces quads that have nothing to do with the policy/rules you own"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these sanitisation checks the same as the ones of rewritepolicies?
If so, maybe it is better to create a util function out of it that is re-used both here and in rewrite
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General note: the "config" in packages/uma/config/rules/odrl/
is not really config, but more scaffolding for demo's that was misplaced there out of laziness (probably my own). We should move this out of the config folder.
"comment": "Handles all remaining output. These handlers have to handle input/output parsing themselves. TODO: At some point we want more generic conneg.", | ||
"@id": "urn:uma:default:RawRoutedHttpRequestHandler", | ||
"@type": "RoutedHttpRequestHandler", | ||
"routes": [ | ||
{ "@id": "urn:uma:default:ContractRoute" }, | ||
{ "@id": "urn:uma:default:LogRoute" }, | ||
{ "@id": "urn:uma:default:VCRoute" }, | ||
{ "@id": "urn:uma:default:PolicyRoute" }, | ||
{ "@id": "urn:uma:default:OnePolicyRoute"} | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I fully understand these handlers...
- What are the ContractRoute, LogRoute, and VCRoute?
- Why are these bundled as "raw" handlers? Presumably, at least two of them talk RDF, so it might be better to put an RDF (de)serialiser around it, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing something went wrong with the rebase resulting from #56
Which was a quick fix I did to make sure to remove a blocker for the work being done here, as the current API handler I had in there only supported JSON/form-encoded.
import { HttpHandlerRequest } from "../../http/models/HttpHandler"; | ||
import { checkBaseURL, retrieveID } from "./PolicyUtil"; | ||
|
||
export function policyOptions(request: HttpHandlerRequest, baseUrl: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CSS already contains a generic CorsHandler, so might as well use that one
#### URI encoding decision | ||
Some operations require the client to specify a policy ID in the URL. Since policy ID's might contain reserved characters (e.g. `/`, `:`, ...), we have chosen to encode them with the builtin [`encodeURIComponent()` function](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/encodeURIComponent). Using this method, reserved characters will be converted to their respective UTF-8 encodings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be a nit, but preferably policy IDs are URIs themselves, namely the URI to which these requests are sent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
namely the URI to which these requests are sent.
What do you mean with that if I may ask?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean that a policy at https://example.as/policies/abc0123
already has an identifier, which is exactly that URI (https://example.as/policies/abc0123
). So while it makes sense to say that the suffix (abc0123
) should be a safe URI component string (or encoded as one), the URI itself is the actual identifier i.m.o.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That could work as well. Though, maybe not ideal when you want to export your policies from one server to another.
Another option if we do not want to decide is to implement both, make it configurable and then experiment with what works best.
On a different note, having the policy be an URL would also make creating an API for rule editing very easy. (though also future work and open for discussion right now)
#### Authorization/Authentication decisions | ||
The current implementation has insufficient authentication restrictions. Currently, the only requirement is that the 'Authorization' header is to be set to the webID of the "logged on" client. Proper procedures to authenticate this client are still to be implemented. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be resolved sooner rather than later. It should be fairly simple to add a classic OAuth authorization code flow to the UMA server, which we also need anyway to link up the AS to the RS. That might be something for @joachimvh though ...
The algorithm to GET a single policy will use a procedure to separate the policy into different parts: | ||
1. Policy Quads | ||
- Some quads in the policy define rules. Their general form looks like `<policyID> <relation> <ruleID> .`. These quads are split into rules owned by the client **(1)**, and those owned by another client **(2)**. | ||
- Other quads define other parts about the policy. These quads are set into a group of `policy definitions` **(3)**. | ||
2. Rule Quads | ||
- Owned rule quads are those where the client as an assigner **(4)**. | ||
- Other rule quads are owned by other clients **(5)**. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not entirely clear to me why this split is necessary, and which scenarios would have policies/rules by different assigners mixed up with eachother 🤔
All in all minor remarks. Seems like good work for such a short time! |
Draft PR
To keep up with progress
Documentation is found within
docs/policy-management.md
Added a script
script:seed
to seed and delete a hard coded set of policies for a variable webID, and a scriptscript:demo-uma-policy
as used in the demo:Uma-Loama-Extended.mp4