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

CanCanCan a :create authorization #5282

Closed
pinkynrg opened this issue Dec 11, 2017 · 7 comments
Closed

CanCanCan a :create authorization #5282

pinkynrg opened this issue Dec 11, 2017 · 7 comments

Comments

@pinkynrg
Copy link

pinkynrg commented Dec 11, 2017

I created some rules and one of them is the following:

can :create, Ticket, { author_id: user.id }

This should allow the currently authenticated user to create tickets only when author_id is equal the user's id.

From rails console I can test that my rule works fine:

my_user.can? :create, Ticket.new(author: my_user)      # returns true 
my_user.can? :create, Ticket.new(author: another_user) # returns false

Considering that I'm using ActiveAdmin, I now need to use my authorization rules with it by using its ActiveAdmin::AuthorizationAdapter.

One problem I am facing is that whenever I create a "New Ticket" I get access denied.
I doubled checked what condition is failing and it seems that AA asks for the following:

my_user.can? :create, Ticket.new # which returns false!

When I believe it should ask for the following instead:

my_user.can? :create, Ticket # which returns true!

Ticket.new has all parameters set to nil:

<Ticket author_id: nil, ..., created_at: nil, updated_at: nil>

that is why my CanCanCan hash condition is failing (author_id = nil is not valid, it should be user_id instead).

Is there a a possibile fix for this? Maybe I'm setting my CanCanCan rule in the wrong way?ActiveAdmin is also offering a CanCanCan adapter out of the box so I'm wondering how this could have been overlooked at.

@pinkynrg
Copy link
Author

pinkynrg commented Dec 11, 2017

Long question short, why should AA asks if the user is authorized to create an empty ticket in this case even before the form is submitted?

The preliminary check to see if a user can create a generic Ticket in order to show/hide the create button is perfect. I just don't understand why before showing the form AA needs to check for the creation of an empty Ticket.

@varyonic
Copy link
Contributor

varyonic commented Dec 12, 2017

@pinkynrg
Copy link
Author

pinkynrg commented Dec 13, 2017

Just trying to understand if the problem is only mine or if it is an AA issue no one ever crossed before. To me it seems that AA should check the create authorization mainly in 2 cases:

  • The UI needs to show the "create new Ticket" button: here, it is useful to check if the current_user can create a generic Ticket - in cancancan you would check the permission as follows:

can(:create, Ticket)

  • AA needs to understand if the resource can actually be stored in the db after a form submission: here it is useful to check wether the current user can store that ticket with the values just "typed in" using the ticket form - in cancancan you would check the permission as follows:

can(:create, Ticket.new({author_id: current_user, some: "some", other: "other", values: "values"}))

That's it! Why should AA check the following:

can(:create, Ticket.new({author_id: nil, some: nil, other: nil, values: nil}))

What if the current_user has only permission to create tickets where author_id = his_own_user_id?

the authorization would fail and this is exactly where I'm stuck.

I hope I explained myself a little better in order to know how to address the issue.

@pinkynrg
Copy link
Author

@pinkynrg
Copy link
Author

pinkynrg commented Dec 14, 2017

Does this override makes sense at all? Can you think of any side effects:

ActiveAdmin::ResourceController::DataAccess.module_eval do
  def build_resource
    get_resource_ivar || begin
      resource = build_new_resource
        resource = apply_decorations(resource)
      run_build_callbacks resource

      # this authorization check is the one we don't need anymore
      # authorize_resource! resource

      set_resource_ivar resource
    end
  end
end

ActiveAdmin::ResourceController::DataAccess.module_eval do
  def save_resource(object)
    run_save_callbacks object do
      return false unless object.validate # added it
      authorize_resource! resource        # added it
      object.save(validate: false)        # disabled validation since i do it 2 lines up
    end
  end
end

@wspurgin
Copy link
Contributor

Wouldn't that allow anyone even without the ability to create at least go as far as the form and fill in data? They wouldn't be able to actually create anything because of where you moved the authorize_resource! call. Can't confirm that at the moment but that's my initial thought 🤷‍♂️

I say change the build_resource monkey-patch to:

def build_resource
    get_resource_ivar || begin
      resource = build_new_resource
      resource = apply_decorations(resource)
      run_build_callbacks resource

      authorize! Authorization::CREATE, active_admin_config.resource_class

      set_resource_ivar resource
    end
  end
end

That would prevent access before going to the "New Ticket" form and your present validation in save_resource would ensure that they could only create those tickets they were allowed to create.

I will say that the reason AA probably hasn't tackled it this way is how it would handle form validation and errors on the form if it was record was itself valid but the user wasn't authorized to create that resource. Not sure how it's currently handle, but I'm pretty sure it wouldn't be a nice recovery from that kind of problem (user input lost, navigated to a different page, etc.).

@agbodike
Copy link

For anybody else encountering this problem, I believe the simplest solution is to use a before_build block. For example, given the original sample code, I believe the following would resolve the issue:

before_build do |ticket|
  ticket.author = current_user
end

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

No branches or pull requests

5 participants