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

Enable policy opts via http-api #445

Merged
merged 2 commits into from
Apr 6, 2023
Merged

Conversation

mpoffald
Copy link
Contributor

@mpoffald mpoffald commented Apr 6, 2023

Required for fluree/http-api-gateway#38

This PR enables the use of policy opts via http-api.

The only required change here was 56e770d.

I also added a change to how we deal with errors when compiling policies (f394d78). Prior to this change, if there was an error in that codepath, we would fail to enforce any policy at all, silently failing open to root access.

We should do something smarter with those errors (they are still not surfaced correctly, so I have left the TODOs intact), but this was a minimal change that ensures that we at least fail closed when policy compilation fails.

I encountered this earlier in this work, where due to some bugs I was getting errors during the policy compilation process. Since those got fixed I'm no longer getting those errors, and I couldn't readily think of a way to set up a test for this that would demonstrate this. So, we can take it or leave it, since it's untested.

When using policy via http-api, default context-type will be `:string`, and expanding/parsing this query will fail if we don't specify the context-type. This ensures the policy query will succeed regardless of the default context-type.
These fns return lists of restrictions. Before this change, if there was an error during policy compilation, we would fail open (no list of restrictions -> unrestricted access). We could still do better wrt error handling here, but at least this way we will not silently fail to enforce any policy in case of an error.
Copy link
Contributor

@dpetran dpetran left a comment

Choose a reason for hiding this comment

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

This looks good to me. Nice catch on the "internal" query needing to specify the context type in order to not cause problems, I'll keep my eyes open in case we do that elsewhere.

@mpoffald mpoffald merged commit cfab023 into main Apr 6, 2023
@mpoffald mpoffald deleted the feature/http-api-policy-opts branch April 6, 2023 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants