Skip to content

Authorization #16

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
Samsinite opened this issue Aug 31, 2014 · 67 comments
Closed

Authorization #16

Samsinite opened this issue Aug 31, 2014 · 67 comments

Comments

@Samsinite
Copy link

Hey guys,

This gem/library looks great! What about directly supporting an option to easily hook up authorization instead of requiring the resource controller index/show/create/update/destroy methods to be overridden?

Something like a simple interface that filters for index/read_list, returns true/false for show/create/update/destroy, and if in the future updating lists or destroying lists are supported, the interface could be updated to support filtering those as well. Than it would be straight forward for whoever to override it to support their preferred method of authorization, such as pundit or cancancan. I wouldn't mind implementing this if you guys are interested.

@lgebhardt
Copy link
Member

I have been thinking about different ways to handle authorization. I stripped out my first attempt (a set of callbacks of the resource) since I wasn't sure it was quite right.

One concern I have about overriding the controller to handle authorization is that once we get patch support implemented it will be possible to have a single request create multiple operations. Authorization will need to be checked for each of these operations. I don't know how well Pundit or cancancan can be adapted to this criteria.

One method I have been playing with (in my project using this gem) has been to create a custom OperationsProcessor, derived from the ActiveRecordOperationsProcessor. In before_operation I call a custom method on each resource which I have named approve_operation. This allows each resource to have it's own authorization scheme. I'm not totally happy with this approach since the approve_operation method isn't as clean as I think it should be (due to needing to pull the details out of the Operation object).

I would certainly appreciate any work you wish to put into this problem. I just want to make sure any solution we adopt will support multiple operations, is flexible to support different authorization packages, and is resource centric (if possible).

@Samsinite
Copy link
Author

lgebhardt,

I'll put together an example showing the gist of what I was thinking, and see what you think.

I'm thinking an interface that can be specified in the controller -- similar to how context is specified -- so nothing on the controller has to be overridden. I don't see this getting in the way for a patch single request that handles multiple operations, as whatever handles the type of patch operation that occurs can know the appropriate type of authorization that it needs to do. I currently think authorization should occur in the controller and not in the resource -- thus why I think a callback on a resource is a bad idea.

@lgebhardt
Copy link
Member

@Samsinite, thanks, I look forward to seeing your proposal.

@lgebhardt
Copy link
Member

From the http://jsonapi.org/format/#patch-urls

PATCH operations MAY be allowed at the root URL of an API. In this case, every "path" within a PATCH operation MUST include the full resource URL. This allows for general "fire hose" updates to any resource represented by an API. As stated above, a server MAY limit the type, order and count of bulk operations.`

I want any authorization system we come up with to allow jsonapi-resources to support PATCH operations on the root URL.

opsb pushed a commit to opsb/jsonapi-resources that referenced this issue Sep 15, 2014
@juggy
Copy link

juggy commented Dec 14, 2014

Authorizations are also important on the UI side for the client that will use the data. I was thinking about sending along with the object, the operation the user can perform on it.

For a post the author can create, read, update, delete, but a viewer can only read. Based on that meta information, the client can change the UI to reflect it.

This does not remove the need to block actions on the server based on the user's authorization, but it would provide a much more complete approach.

@barelyknown
Copy link
Collaborator

It might help others to see how I'm handing authorization. I'm pretty happy with the approach.

I'm using the pundit gem in combination with the new callbacks that are available in v0.1.0 of jsonapi-resources.

This is what my BaseResource class looks like.

module V1
  class BaseResource < JSONAPI::Resource
    [:create, :update, :remove].each do |action|
      set_callback action, :before, :authorize
    end

    # Authorize the model for the permission required by the controller
    # action. Also, mark the controller as having been policy authorized.
    def authorize
      controller = context.fetch(:controller)
      permission = controller.action_name + "?"

      controller.policy_authorized!
      policy = Pundit.policy!(context.fetch(:current_user), model)
      unless policy.public_send(permission)
        error = Pundit::NotAuthorizedError.new("not allowed to #{permission} this #{model}")
        error.query, error.record, error.policy = permission, model, policy
        raise error
      end
    end

    class << self
      # Override the records method to policy scope the records
      # used by JSONAPI::Resource find methods. Also, mark the controller
      # as having been policy scoped.
      def records(options = {})
        options.fetch(:context).fetch(:controller).policy_scoped!
        relation = Pundit.policy_scope!(options.fetch(:context).fetch(:current_user), _model_class)
        authorize(options.fetch(:context), relation)
        relation
      end

      private

      def authorize(context, relation)
        case context.fetch(:controller).action_name
        when "show"
          resource_for(
            context.fetch(:controller).params.fetch(:controller)
          ).new(relation.first, context).authorize
        end
      end
    end
  end
end

@wzowee
Copy link

wzowee commented Jun 1, 2015

@barelyknown what are you using for authentication? My only experience with authentication in Rails so far is Devise, but that does not appear to be a good fit here.

@barelyknown
Copy link
Collaborator

@wzowee Devise could work fine. I've used that or just rolled my own using has_secure_password. In either case, I use the OAuth2 password flow and then ember-simple-auth.

@oliverbarnes
Copy link
Contributor

@barelyknown I think you're on to something, I've been also thinking it'd be nice to use a pundit-like solution. Only I'm wondering if it'd be possible to have separate Policy objects with methods for each of the operations, and this object would be passed around to both resources (for GET requests / show operations) and controllers (for POST and PATCH / create and update operations). I'm still grokking the source to be able to offer a concrete solution, though

@oliverbarnes
Copy link
Contributor

@wzowee we're also using ember-simple-auth + rolling our own token oauth, but this setup assumes you have an ember front-end, a single trusted client. Is that your scenario? It might be different for, say, services, or a public API.

@barelyknown
Copy link
Collaborator

I've evolved my approach a bit (but it's still the same basic idea). Here's some sample code. It might be helpful.

module V1
  class BaseResource < JSONAPI::Resource
    include ResourcePolicyAuthorization
    // ...
  end
end
module ResourcePolicyAuthorization
  extend ActiveSupport::Concern

  included do
    [:save, :remove].each do |action|
      set_callback action, :before, :authorize
    end
  end

  delegate :authorize_policy, to: :class

  def authorize
    authorize_policy(context[:current_user], model, context[:controller])
  end

  def records_for(association_name, options={})
    records = model.public_send(association_name)

    return records if context.nil?

    if records
      authorize_policy(context[:current_user], records, context[:controller])
    end
    records
  end

  class_methods do
    def records(options = {})
      authorized_records(
        options[:context][:current_user],
        options[:records_base] || _model_class,
        options[:context][:controller]
      )
    end

    def authorized_records(user, records, controller)
      records = Pundit.policy_scope!(user, records)
      controller.policy_scoped!

      authorize_policy(user, records, controller)

      records
    end

    def authorize_policy(user, records, controller)
      controller.policy_authorized!

      case records
      when ActiveRecord::Relation
        return if records.count == 0
      end

      policy = Pundit.policy!(user, policy_record_for(records))
      permission = permission_for(controller, policy)

      if !_authorized?(policy, permission)
        raise Pundit::NotAuthorizedError.new(
          query: permission_for(controller, policy),
          record: policy_record_for(records)
        )
      end
    end

    private

    def policy_record_for(records)
      case records
      when ActiveRecord::Base then records
      when ActiveRecord::Relation then records.first || records.model
      end
    end

    def permission_for(controller, policy)
      case controller.model_class == policy.model_class
      when true then controller.action_name.to_s + "?"
      when false then "show?"
      end
    end

    def _authorized?(policy, permission)
      policy.public_send(permission)
    end

  end
end

@wzowee
Copy link

wzowee commented Jun 3, 2015

@oliverbarnes yeah - that's pretty much my scenario at present - have gone with ember-simple-auth and devise for now (see http://johnmosesman.com/ember-simple-auth-tutorial-and-common-problems/), mainly due to time constraints and not wanting to re-implement what comes for free with devise (account locking, password reset emails etc)

@barelyknown thanks for the update on how your authorisation is evolving.

@oliverbarnes
Copy link
Contributor

@barelyknown I'm having a hard time seeing where the authorization rules are, or would go, there

@barelyknown
Copy link
Collaborator

The authorization rules are all in Pundit policies. https://github.com/elabs/pundit

@oliverbarnes
Copy link
Contributor

Example?

@barelyknown
Copy link
Collaborator

Sure. Here's an ApplicationPolicy for an app. Ever resource has a matching policy (like PostPolicy or CommentPolicy), and they all (or at least most) inherit from the ApplicationPolicy. pundit policies authorize requests and provide scopes (which are relations that represent all of the records that a given user is authorized to see). Let me know if you'd like anything explained.

class ApplicationPolicy
  attr_reader :user, :record

  def initialize(user, record)
    @user = user
    @record = record
  end

  def index?
    true
  end

  def show?
    scope.where(:id => record.id).exists?
  end

  def get_related_resource?
    show?
  end

  def get_related_resources?
    index?
  end

  def create?
    false
  end

  def update?
    false
  end

  def destroy?
    false
  end

  def scope
    self.class::Scope.new(user, record.class).resolve
  end

  def model_class
    self.class.to_s[/.+(?=Policy)/].constantize
  end

  class Scope
    attr_reader :user, :scope

    def initialize(user, scope)
      @user = user
      @scope = scope
    end

    def resolve
      scope.none
    end
  end
end

@oliverbarnes
Copy link
Contributor

Thanks. I guess my question is how would a pundit policy handle authorization that needs to be done before hitting the model. Checking the user id requested to patch a user resource against the current user's id, for example

@barelyknown
Copy link
Collaborator

It would first find the user that is the subject of the action. Then it would check if the current_user has permission to perform the action on that user (see #authorize_policy).

@oliverbarnes
Copy link
Contributor

Right, that's what I mean - how do we do that based only on the user id param, without actually fetching the user from the database? Maybe this is out of JR's concern in the end... there's a reason why there's separate specs for authorization (like OAuth2) and resources. Just learning this myself

@barelyknown
Copy link
Collaborator

Why wouldn't you want to fetch the user with the user_id param?

@oliverbarnes
Copy link
Contributor

Why fetch a record or even touch the database at all when you already know it’s off-limits? Best to respond with 403 as quickly as possible, IMHO

On June 5, 2015 at 3:09:18 PM, Sean Devine (notifications@github.com) wrote:

Why wouldn't you want to fetch the user with the user_id param?


Reply to this email directly or view it on GitHub.

@barelyknown
Copy link
Collaborator

How would you know?

@fsmanuel
Copy link
Contributor

fsmanuel commented Jun 9, 2015

@barelyknown i like the approach. I'll give it a shot and provide feedback.

@oliverbarnes
Copy link
Contributor

The client would send it? Assuming the relationship with the owner is always present in the requests

On June 7, 2015 at 7:28:12 PM, Sean Devine (notifications@github.com) wrote:

How would you know?


Reply to this email directly or view it on GitHub.

@barelyknown
Copy link
Collaborator

That doesn't sound right (at least not for the standard case). You can authenticate who the request came from based on info in the request, but you need for the server to authorize that that user can perform whatever action they're trying to perform, and that will require looking up the some info in the DB (or wherever).

@thomassnielsen
Copy link
Contributor

Would it be possible to authorize before show/index with the method barelyknown uses? As far as I can tell there are no callbacks for these methods. We need to restrict access to listing for some resources and displaying certain records in others in our API.

@barelyknown
Copy link
Collaborator

The approach that I use works for all actions including show/index. Are you running across a situation that doesn't seem like a fit?

@thomassnielsen
Copy link
Contributor

We figured it out - I was confused by the split between using callbacks for save and remove, and fetching being implemented by overriding class methods, but my colleague helped clarify it for me.

@nozpheratu
Copy link

Seems to be a bug. If I downgrade to 0.6.2 everything seems to work. Supposedly #573 fixes this issue but it doesn't seem to work for me or something else is breaking it.

@barelyknown
Copy link
Collaborator

@nozpheratu The problem is probably related to this commit ace49ff. It removed some of the modules that provide the rescuable functionality.

You can add them back you your controller class. For example, I added something like the following:

class ApplicationController < JSONAPI::ResourceController
  include ActionController::Rescue
  include ActionController::Head

  rescue_from Pundit::NotAuthorizedError do |exception|
    head :forbidden
  end
end

@GRardB
Copy link

GRardB commented Apr 23, 2016

I recently started a project with JSONAPI::Resources, and I'm now trying to implement authorization with Pundit. I stumbled across this and have read the entire thread multiple times over, but I have to admit I'm completely lost.

I'm fairly new to Rails, and this is my first time working with JSONAPI in any capacity. Does anyone have a working demo of the techniques discussed here, or perhaps a simple explanation of what's going on for someone with a bit less Rails experience? I've tried using some of the code snippets from above, but I just get a bunch of errors for each one.

Any help would be greatly appreciated.

@nozpheratu
Copy link

The code worked for me on earlier versions of the gem. From:notifications@github.heygears.comSent:April 23, 2016 3:15 PMTo:jsonapi-resources@noreply.github.heygears.comReply-to:reply@reply.github.heygears.comCc:cylehunter33@gmail.com; mention@noreply.github.heygears.comSubject:Re: [cerebris/jsonapi-resources] Authorization (#16) I recently started a project with JSONAPI::Resources, and I'm now trying to implement authorization with Pundit. I stumbled across this and have read the entire thread multiple times over, but I have to admit I'm completely lost.

I'm fairly new to Rails, and this is my first time working with JSONAPI in any capacity. Does anyone have a working demo of the techniques discussed here, or perhaps a simple explanation of what's going on for someone with a bit less Rails experience? I've tried using some of the code snippets from above, but I just get a bunch of errors for each one.

Any help would be greatly appreciated.

—You are receiving this because you were mentioned.Reply to this email directly or view it on GitHub

@valscion
Copy link
Contributor

@GRardB, give our gem a try: https://github.com/venuu/jsonapi-authorization — it should help you get started. Even though it's an alpha version and most likely still contains some bugs, it should have proper authorization hooks in place for all actions to be checked. If not, then that's a bug and we'd like to address it :)

@NuckChorris
Copy link

@valscion I should document+test this better, but I have a 60-line module which hooks JR to Pundit for hummingbird.me. I'd love to compare notes and see if we can combine efforts. I know one problem with mine is that in certain cases a create is handled as an update, due to the way is_new? works.

@valscion
Copy link
Contributor

@NuckChorris that looks like a nice approach, but I'm afraid you might not get enough protection by just applying hooks to resource classes. You might need to get all the way to the operations processor level to get all the necessary information out for authorization purposes, and that's indeed what we're doing with our gem. I'm not sure your approach protects against relationship actions.

We've got extensive request specs to test authorization. Check out e.g. POST /articles/:id/relationships/comments spec and think whether your approach would give the same level of authorization as ours. I'm not 100% sure but I think that resource level hooks are not enough in these cases

@ulitiy
Copy link

ulitiy commented May 26, 2016

Years go by. Nothing done on authorisation. Wasted a day digging into the useless gem.

@valscion
Copy link
Contributor

@ulitiy I can understand that you're disappointed, but there's no need to be rude. This gem works nicely for what it does, and authorization can be handled like we've done with our gem. You can't always get something for free and ready out-of-the-box, you might have to take your time to write the missing pieces yourself.

@lgebhardt
Copy link
Member

@ulitiy Authorization can be done in many ways. If there's a particular package that you are trying to use and it's conflicting with this gem please write up an issue specific to that problem. Gems like this intentionally do not try to solve every problem and it is a decision we have made to not advocate for one particular authorization solution.

@valscion
Copy link
Contributor

Gems like this intentionally do not try to solve every problem and it is a decision we have made to not advocate for one particular authorization solution.

That's actually a great stance. Maybe it could be written up somewhere, and closing this issue as it's highly unlikely this will ever be "resolved" per se? Perhaps a wiki page, or something?

@barelyknown
Copy link
Collaborator

I haven't announced this gem yet (need to add tests for Rails 4.2), but https://github.com/togglepro/pundit-resources will be a drop in solution for using Pundit with jsonapi-resources for authorization. I've used the technique for a long time now on multiple projects, and it works great.

@lgebhardt
Copy link
Member

lgebhardt commented May 26, 2016

@valscion We definitely should add a section on authorization to the README. We could list the common options and maybe document it further in the Wiki. I haven't checked out @barelyknown's https://github.com/togglepro/pundit-resources yet, but if it's "drop in" (and good, as I assume) we might just recommend that as the safe and simple solution.

I'll leave this issue open until we address this in the README.

@valscion
Copy link
Contributor

@barelyknown that seems like a nice approach. I'd be curious to know how it compares to https://github.com/venuu/jsonapi-authorization which handles ties authorization to Pundit, too.

Our gem just does it via operations processors as we thought that resource level callbacks weren't enough for everything. Operations processor also allows for more fine-grained permission handling.

If your gem happens to cover the same use cases as our gem with much less complexity, that's a huge win 😄.

@barelyknown
Copy link
Collaborator

@valscion I didn't even know that jsonapi-authorization existed! I hadn't looked because I've been using the code that is becoming pundit-resources for a long time and just hadn't extracted it. I'd be interested in any examples that would require the operations processor approach. It feels like they'd exist BUT I haven't come across one.

@john-griffin
Copy link
Contributor

@barelyknown I'm using operations processors to protect show and index actions.

eg after_show_operation checks :show? and before_find_operation checks :index?

@ouranos
Copy link

ouranos commented May 30, 2016

Just found this thread while trying to add punding to our JR project.
It seems that jsonapi-authorization and pundit-resources take different approaches to integrate JR and pundit.
Could anyone explain the difference between those 2 approaches?
Thanks!

@valscion
Copy link
Contributor

valscion commented May 30, 2016

Could anyone explain the difference between those 2 approaches?

Disclaimer: I know much more about jsonapi-authorization than pundit-resources, as I've co-authored it. I try to be objective when comparing these approaches but I might naturally have a bias towards the gem I've helped develop.

From the way I see it, pundit-resources relies on resource-level callbacks to tie into Pundit and call the matching Pundit policy methods when those resource-level callbacks are called. I haven't looked at what specific operations call each callback.

Now, how that compares to jsonapi-authorization?

I might still have missing details on pundit-resources, but I am very interested in seeing their gem progressing. If pundit-resources manages to handle all the authorization properly on resource level, it will be huge as writing operations processor callbacks currently is quite nasty. I hope for all the best for them, and seek to try out their gem once it has covered all JR operations somehow :)

Please correct me if I'm wrong on some aspects @barelyknown 😄

P.S.
If you want to try out jsonapi-authorization, please do so, but we encourage you to write some level of authorization tests on your own API anyway. I don't know much how our jsonapi-authorization is used in production yet as we've only protected some quite simple, non-critical internal APIs with it as of now. We intend to keep on developing this approach further as we create more APIs, but I don't know when that will be relevant.

I suppose @thibaudgg has a large-ish API using jsonapi-authorization, so he might be able to chime in on how to test for API authorization internally, too?

@aaronbhansen might also have some insight into how these two gems could possibly compare given my points.

I assume @arcreative could also tell a bit about using jsonapi-authorization, as he seems to have struggled with some issues at least?

EDIT:
Redacted the comment about relationship operations as corrected by @alyssais.

@ouranos
Copy link

ouranos commented May 30, 2016

Wow thanks for this detailed answer :)
I'm still at the early stages of implementing authorization but that'll definitely be handy!

@alyssais
Copy link

@valscion pundit-resources should be able to handle relationships, and they can be tested, I just haven't implemented the tests yet. The process for creating this gem was taking some code that we knew to work and extracting it to a gem, meaning we needed new tests, some of which just haven't been implemented yet (but I'm on it).

@valscion
Copy link
Contributor

valscion commented May 30, 2016

Thanks, @alyssais! I'll edit my comment accordingly :)

@alyssais
Copy link

No problem @valscion. Everything else you've written about pundit-resources looks good. 👍

@lgebhardt
Copy link
Member

@valscion I'm not sure if you have seen the recent changes we have made to JR regarding processors. So far these are only in the master branch, though I hope to move them into a beta release this week. I bring this up because it's a breaking change for projects that use custom OperationsProcessors.

More details on the new Processors can be found in the Operation Processors section of the README.

I just became aware of JSONAPI::Authorization in this thread, so I'm sorry for the short notice on these changes. I'm hoping the changes will be relatively minor, and if there's anything I can do to help out please let me know.

@valscion
Copy link
Contributor

@valscion I'm not sure if you have seen the recent changes we have made to JR regarding processors.

We've seen them 😉 venuu/jsonapi-authorization#22. We did anticipate that there might be changes like this, as we were fiddling with some private-ish APIs. I hope we can somehow make the approach backwards-compatible in jsonapi-authorization without having to write lots of clunky code, though 😕 .

I just became aware of JSONAPI::Authorization in this thread, so I'm sorry for the short notice on these changes.

No worries, the new approach to processors makes a lot of sense.

I'm hoping the changes will be relatively minor, and if there's anything I can do to help out please let me know.

I haven't yet figured how much trouble we're in if we want to retain backwards compatibility with JR 0.7.0 and the upcoming version. If you have any ideas, please let me know 😅 . We can discuss this in venuu/jsonapi-authorization#22 to avoid pinging all the people subscribed in this thread.

@valscion
Copy link
Contributor

By the way, was this issue now resolved when #769 was merged? 🎉

@lgebhardt: #16 (comment)

I'll leave this issue open until we address this in the README.

@lgebhardt
Copy link
Member

@valscion It's a bit circular as #769 references this. But I'll close this since the pertinent details can now be reached from the readme.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests