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

Allowing finer grained permissions #15

Closed
aaronbhansen opened this issue Apr 8, 2016 · 8 comments
Closed

Allowing finer grained permissions #15

aaronbhansen opened this issue Apr 8, 2016 · 8 comments

Comments

@aaronbhansen
Copy link

Just found this gem yesterday from the json resource issue, thanks for putting it together. One issue we are running into is we want to allow a user to create a model with relationships, but not actually update the relationships in question.

The issue comes into play here https://github.com/venuu/jsonapi-authorization/blob/master/lib/jsonapi/authorization/default_pundit_authorizer.rb#L114

My initial take is instead of calling update?, is to have a finer grained permission called create_association? for example. I want to allow a user to create be able to create a new Comment that relates to a Post, but under the current code I would also have to allow them to update the post to allow that association to be created.

I could be missing something, but so far don't see a way to allow the creation of a model and its relations without also allowing the update? on the policy of the related model. Thoughts or suggestions?

@valscion
Copy link
Member

valscion commented Apr 8, 2016

My initial take is instead of calling update?, is to have a finer grained permission called create_association? for example. I want to allow a user to create be able to create a new Comment that relates to a Post, but under the current code I would also have to allow them to update the post to allow that association to be created.

Yeah, we've been thinking of the same use case and we agree that the current solution doesn't really work that well for it. I guess your use case is close enough to #14 that if we could solve it, this issue would also be solved, no?

I could be missing something, but so far don't see a way to allow the creation of a model and its relations without also allowing the update? on the policy of the related model. Thoughts or suggestions?

#14 (comment) could answer your question a bit, on why it is so. Mainly we just wanted to map JSONAPI operations to Rails/Pundit default actions so that it would be easier to try out this gem in any existing application.

Here's what we'd like to have, as quoted from the linked comment:

An option we initially considered when writing this gem was to create an Authorizer which calls much more specific actions on the policies, e.g. create_relationship?. That would come pretty close to what you are suggesting, and make it easier to define more granular permissions on the policy level.

One reason we didn't, was to avoid adding too much complexity for new users trying out jsonapi-authorization.

Another reason is that, to answer the question of create_relationship?, you usually need both the source record and the related record. Passing multiple records to a policy is explicitly discouraged by Pundit, so we'd have to create our own wrapper or other convention there. Or create some custom non-Pundit type of policies.

With that being said, I would love for us to create a more sophisticated Authorizer, and include it in the project as a suggested option to use when you reach the limits of DefaultPunditAuthorizer. That would give us all a simple default setup, with a common convention for more advanced, granular control.


We're very open discussing creating a more sophisticated Authorizer and include it in the project itself. That we we could onboard users easily with the DefaultPunditAuthorizer, but still give a common convention for switching to more advanced usage.

@aaronbhansen
Copy link
Author

Your last comment is exactly what I've been thinking about doing lately. I only found pundit recently, previously before json api we used a simple internal authorization system that was setup like pundit (except it was also user role specific). I like the architecture and design of pundit, but it doesn't feel like it works as well with apis as it does with simple crud controllers.

After I created this ticket, I actually thought about creating a new JSON Authorizer to replace the default pundit authorizer to give me more specific authorization control. I've been considering actually forking pundit and creating an API or JSON API pundit version. It would be mostly an extension of pundit, but allow finer grained control. It would allow individuals to upgrade their existing pundit policies and then include the extra method calls needed like allow_association? or allow_include?.

As far as issue 14, thats still a different concern I think would implement in the pundit fork. I initially tried to have all of my policies attempt to cover all the user role permissions in one set of policies, but the separation of concerns got messy. But with that said, json authorizer wouldn't need to know anything about the contextual based policies, as it could resolve below the json authorizer in the pundit api fork.

There are a couple of other issues that would be great to also figure out, like how to skip authorization on a specific resource call like in pundit.

Also, pundit has some simple support for allow parameters, but I would like to extend that in the api fork to also include parameters authorization based on serialization, create, update within the policies. That way an anonymous user would be allowed to have a few parameters in the serialization, while the admin could select everything for example.

Let me know your thoughts, if its something your interested in working together on, I could start on the initial pundit api fork and then work with you to ensure json authorizer will work with pundit or pundit api. I think we would need to diverge slightly from pundits policies on passing in parameters to the policy, as you mentioned above, authorizing includes does need two data points. The referenced class and the source class to see if its a valid include. For the most part things would follow pundits guidelines as they exist today, but would have to diverge a little on a few calls.

@valscion
Copy link
Member

After I created this ticket, I actually thought about creating a new JSON Authorizer to replace the default pundit authorizer to give me more specific authorization control.

That's a completely legit idea 👍. That's exactly why we're keen to make it easy to swap authorizers as larger applications are bound to have more complex logic. It would not make sense to create a library which would try to handle every possible use case — instead, we should focus on making the library as pluggable as possible in the areas where custom domain logic needs to be injected.

It would allow individuals to upgrade their existing pundit policies and then include the extra method calls needed like allow_association? or allow_include?.

That sounds interesting and doable. I'd be interested to hear more of the possible APIs you think this library would allow, and how they'd hook into the ActiveRecord classes like Pundit's finder does. Would the policy classes still be grouped on the underlying record class, or would there be another base-level type for the association validation cases?

There are a couple of other issues that would be great to also figure out, like how to skip authorization on a specific resource call like in pundit.

I don't really understand why this is necessary. Wouldn't you just have a method in the policy class that always returns true instead of trying to skip it altogether?

Also, pundit has some simple support for allow parameters, but I would like to extend that in the api fork to also include parameters authorization

This seems a bit like another concern. JSONAPI::Resources already allows for you to specify the fetchable and creatable and updatable attributes based on the context.

So what you would want to do would be just to get a more structured and separated way of filtering the attributes. What I'm afraid you'll end up with is attribute definitions scattered around different files and folders. As JR already allows you to filter those attributes based on the user, I wouldn't jump on extracting that to a separate pundit fork until the association authorization is complete.

You might find that authorizing associations and filtering parameters are two concerns which could live in different libraries. They both could hook into pundit policy classes, maybe with a similar way policy scopes are defined (adding a Scope inner class to the policy class) and most likely there won't be any major overlap in functionality between these two.

It might make more sense to just tackle one of these cases first and then figure out how to continue from there. At least it would help avoid feature creep and probably make it easier to test the libraries in isolation 😄 (kind of an important point for an authorization augmenting library, don't you think?)

Let me know your thoughts, if its something your interested in working together on

My time might be a bit limited until we hit this issue ourselves. I can help you think of the architecture and API decisions for sure. It might help to have someone to discuss ideas with. Do you think GitHub issues is a good way to do that, or should we try to use some chat service or something else..?

I could start on the initial pundit api fork

I'd be keen to first try to augment the Pundit API instead of forking the library to think of these. That library could use a convention which would allow for Pundit and the extra library to coexist. We'd just have to figure out what are the blockers for doing that, though, and what it means in terms of API usability. I'm sure there are plenty of ways to structure that functionality 😊

I think we would need to diverge slightly from pundits policies on passing in parameters to the policy, as you mentioned above, authorizing includes does need two data points.

Yeah, it could very well be. Definitely something which needs to be given separate thought on. Maybe different strategies could easily be first prototyped and then try to see what it would mean in terms of the API. Would it easily lead to a mess, or would it be easy to test?

For the most part things would follow pundits guidelines as they exist today, but would have to diverge a little on a few calls.

That seems like a very good principle.

How do you want to keep on discussing this issue? I'm afraid we might soon hit the limits of GH issues, as it takes quite a long time to discuss these with large dumps of texts. And there are bound to be long times of silence when either party is busy doing other things.

@aaronbhansen
Copy link
Author

That sounds interesting and doable. I'd be interested to hear more of the possible APIs you think this library would allow, and how they'd hook into the ActiveRecord classes like Pundit's finder does. Would the policy classes still be grouped on the underlying record class, or would there be another base-level type for the association validation cases?

From the initial look at the operations hooks and json authorizations hook-ins, it feels like a good starting point would be to separate the allowing of a relationship on create and update, and the including of a relationship based on the the source class to the child class.

I don't really understand why this is necessary. Wouldn't you just have a method in the policy class that always returns true instead of trying to skip it altogether?

Skipping methods was an edge case as we were manually constructing some of the resources into a single api call for a signup where we needed in a single transaction to create multiple related models and we were trying to run it through the json api standard calls. Probably can be ignored for now.

This seems a bit like another concern. JSONAPI::Resources already allows for you to specify the fetchable and creatable and updatable attributes based on the context.

So what you would want to do would be just to get a more structured and separated way of filtering the attributes. What I'm afraid you'll end up with is attribute definitions scattered around different files and folders. As JR already allows you to filter those attributes based on the user, I wouldn't jump on extracting that to a separate pundit fork until the association authorization is complete.

You might find that authorizing associations and filtering parameters are two concerns which could live in different libraries. They both could hook into pundit policy classes, maybe with a similar way policy scopes are defined (adding a Scope inner class to the policy class) and most likely there won't be any major overlap in functionality between these two.

It might make more sense to just tackle one of these cases first and then figure out how to continue from there. At least it would help avoid feature creep and probably make it easier to test the libraries in isolation 😄 (kind of an important point for an authorization augmenting library, don't you think?)

Within our application, these feel like the same concern (method level authorization and field level authorization). You could also say that authorization should be within the resource since it already allows for these callbacks, but it definitely feels cleaner when pulled into a separate policy.

In the scope of one set of policies to do all the authorization, I agree this doesn't make as much sense. But when you think about it with multiple policies for the same set of api calls, it really helps separate the allowed fields into a clean set of authorization not only at the api call level, but at the field level also.

For example, if you had a Project model and on there it had the following fields

name, description, launch_date, notes

In our example use case, a project could have the following sets of authorization policies.

Anonymous user:
  - index: true
  - view fields: [name, description, launch_date]
  - create / update: false

Member of project:
  - index: true
  - view fields: [name, description, launch_date, notes]
  - update: true
  - update fields: [name, description, notes]

Project Admin:
  - index: true
  - view fields: [name, description, launch_date, notes]
  - create / update: true
  - create / update fields: [name, description, launch_date, notes]

While the authorization today stops basic entry into a method, and some relationship based validation, it doesn't provide field level authorization which would be very useful when you have several user role based policies for the same api call.

I'd be keen to first try to augment the Pundit API instead of forking the library to think of these. That library could use a convention which would allow for Pundit and the extra library to coexist. We'd just have to figure out what are the blockers for doing that, though, and what it means in terms of API usability. I'm sure there are plenty of ways to structure that functionality 😊

I wouldn't mind trying to work with pundit as it exists today, but becoming more doubtful as I haven't had any responses from my initial ticket request. I think pundit is a great resource for non api based authorization, but It has some gaps that aren't being covered when working with an api. Without a faster response time, it will be easier to branch and create an fork with api focused concerns. A large concern for us at the moment is allowing a policy resolver to find policies based on roles. If I'm going to branch to create that, it will be easy to make the other changes to allow additional points specifically for json api and json authorizer.

If you think of pundit in traditional rails controllers, it works great to filter out initial authorization concerns and then the views filter out a lot of the remaining unauthorized content. You have dual layers of security. One for the initial call to the controller and a second in the view to hide remaining content.

With an api, you are missing this second layer in the view to filter out all the content in one call. As brought up in the initial ticket concern, trying to allow the creation of a record with relationships also had to allow the updating of the relationships for the authorization to successfully complete. If you had finer grained authorizations, you could avoid this.

How do you want to keep on discussing this issue? I'm afraid we might soon hit the limits of GH issues, as it takes quite a long time to discuss these with large dumps of texts. And there are bound to be long times of silence when either party is busy doing other things.

We use slack for most of out communication. I'm on the Ember JS community slack, or we could invite you to our company slack on a single channel to talk about this. If you're interesting in using that medium of communication let me know and we can communicate the details through email.

I don't think json authorization needs any initial changes for me to try out some finer grained authorization with pundit. I should be able to create a new authorizer and see where that leads. I would like to look more into field level authorization based on several contextual pundit policies. I think that would help separate out the concerns the best between authorization and resource based logic. Let me know how you would like to proceed communicating outside of github issues.

@aaronbhansen
Copy link
Author

One other thing I wanted to mention, we did some performance metrics the last two days and found some json authorization performance hits that were accounting for 20-30% of the request time. I changed these two class methods in a monkey patch to fix the performance issue.

module JSONAPI
  module Authorization
    module PunditScopedResource
      extend ActiveSupport::Concern

      def records_for(association_name)
        record_or_records = @model.public_send(association_name)
        relationship = self.class._relationships[association_name]

        case relationship
        when JSONAPI::Relationship::ToOne
          record_or_records
        when JSONAPI::Relationship::ToMany
          # ::Pundit.policy_scope!(context[:user], record_or_records)
          record_or_records
        else
          raise "Unknown relationship type #{relationship.inspect}"
        end
      end

    end

    class AuthorizingOperationsProcessor
      def authorize_include_directive
        true
      end
    end
  end
end

On the records_for call it was re-querying the database for every include. On authorize_include_directives it was also doing some additional database queries.

I like the idea behind the two methods, but they do require additional database queries. I try to avoid any extra calls to the database as they slow down the request, especially with a lot of includes which our current app has.

If the authorization took a slightly different approach, the authorization library should be able to perform authorization without out any additional queries. One example of how that could happen is when authorizing the includes per model.

For example if you had a api call to api/orders/4 from a customer and the standard includes were

company,order_lines

and lets say orders also was a possibly include from company, someone could technically modify the request to do

company.orders

to get all orders for that company, but the authorizer as it stands today would filter it out.

If we changed the policys on company to not allow including orders from a company, that also would fix the the security issue (just comparing model classes verses data). I understand there are possible security gaps if individuals didn't lock the policies down, but wanted to mention it as a way to get almost the same authorization without any additional database queries. Purely relying on the database relations and constraints on save / update to perform validations, and then on returning results allow whatever is allowed by the relations and the include checks.

Let me know if you have any thoughts on avoiding all database queries except for those by jsonapi resources and handling security for relations based on class comparisons in the policies. I think if authorizing_operations_processor class was abstracted differently it could allow for both types a more intense database security policy, and a quicker include / relation based policy.

@valscion
Copy link
Member

One other thing I wanted to mention, we did some performance metrics the last two days and found some json authorization performance hits that were accounting for 20-30% of the request time.

Nice discovery! Can you open a separate issue for this so it's easier to discuss it as a separate issue? :)

@valscion
Copy link
Member

We use slack for most of out communication. I'm on the Ember JS community slack, or we could invite you to our company slack on a single channel to talk about this. If you're interesting in using that medium of communication let me know and we can communicate the details through email.

I'd be happy to discuss these over Slack :) I have no strong feelings over whether a community slack or your company's slack is better. What I know is that I'm not on either Slack 😄. Can you send me an email? You can find it on my profile page in GitHub.

trmcnvn added a commit to trmcnvn/hummingbird that referenced this issue Jul 29, 2016
- Creates custom authorizer to match our logic on resource creation
- Active discussion of this issue upstream @ venuu/jsonapi-authorization#15
trmcnvn added a commit to trmcnvn/hummingbird that referenced this issue Jul 29, 2016
- Creates custom authorizer to match our logic on resource creation
- Active discussion of this issue upstream @ venuu/jsonapi-authorization#15
@valscion
Copy link
Member

I'll close this as I think relationship authorization works for the use cases discussed here. https://github.com/venuu/jsonapi-authorization/blob/v1.0.0.beta2/docs/relationship-authorization.md

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

No branches or pull requests

2 participants