Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Don't let authorization_adapter overwrite id when creating or updating an object #1243

Closed
wants to merge 2 commits into from

5 participants

@christophermanning

This is related to Issue #1203

If a permission like can :manage, Player, :id => 2 was set (when you want to only allow a user to manage certain Players) it would overwrite the id of the object and could cause an update of the wrong object or creating an object with a duplicate primary key.

@travisbot

This pull request fails (merged 9d2547f into 054a537).

@travisbot

This pull request fails (merged d744012b into 054a537).

@travisbot

This pull request passes (merged 086ae6c into 054a537).

@mshibuya
Collaborator

I don't like this 'workaroundish' fix.
When Ability is declared as can :manage, Player, :id => 2, we should never permit managing(i.e. creating, updating, ...) of Player with an id other than 2.
So bypassing assignment of :id violates the intended restriction.

Instead of this, I think an unauthorized attempt(e.g. creating Player with ID=3 when the user can :manage, Player, :id => 2) should result in an error saying he is not authorized to do so.

@christophermanning

I initially ran into this error when there were abilities defined for an object with different allowed ids:

can :manage, Player, :id => 2
can :manage, Player, :id => 3

and it changed the id when a user who had access to id 2 and 3 edited player/2. The problem seems to happen because cancan attributes_for gets the relevant rules and would return a different id from the one that is being edited.

So it seems like @authorization_adapter.attributes_for(:update, @abstract_model) would have to be updated one way or another.

I hope this makes sense and thank you for your help :D

@mshibuya
Collaborator

Yeah, of course I know how your problem occurred.
I'm insisting that yours is not the right way to fix this issue.

@christophermanning

Great. I wanted to make sure I was clear.

I don't know how to go about fixing it the way you suggested. Do you know who could work on a fix for this?

@michalmuskala

I have encountered a similar issue.
I have an Article model with user_id key. Every times a user edits an Article its user_id is changed to the editor's.

@mshibuya mshibuya was assigned
@bbenezech
Collaborator

I don't get it, how do you guys end up with a link to an unauthorized instance?
Links should be filtered against cancan conditions anyway.

The bug you get is expected.

Plus it sounds really ugly to add a condition on an id, this is not the way cancan is supposed to be used IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
2  lib/rails_admin/config/actions/edit.rb
@@ -33,7 +33,7 @@ class Edit < RailsAdmin::Config::Actions::Base
@object.set_attributes(params[@abstract_model.param_key], _attr_accessible_role)
@authorization_adapter && @authorization_adapter.attributes_for(:update, @abstract_model).each do |name, value|
- @object.send("#{name}=", value)
+ @object.send("#{name}=", value) unless name == :id
end
if @object.save
View
4 lib/rails_admin/config/actions/new.rb
@@ -38,9 +38,9 @@ class New < RailsAdmin::Config::Actions::Base
@object.set_attributes(params[@abstract_model.param_key], _attr_accessible_role)
@authorization_adapter && @authorization_adapter.attributes_for(:create, @abstract_model).each do |name, value|
- @object.send("#{name}=", value)
+ @object.send("#{name}=", value) unless name == :id
end
-
+
if @object.save
@auditing_adapter && @auditing_adapter.create_object("Created #{@model_config.with(:object => @object).object_label}", @object, @abstract_model, _current_user)
respond_to do |format|
View
3  spec/integration/authorization/cancan_spec.rb
@@ -9,7 +9,9 @@ def initialize(user)
can :manage, Player if user.roles.include? :manage_player
can :read, Player, :retired => false if user.roles.include? :read_player
can :create, Player, :suspended => true if user.roles.include? :create_player
+ can :create, Player, :id => 12345 if user.roles.include? :create_player
can :update, Player, :retired => false if user.roles.include? :update_player
+ can :update, Player, :id => 2 if user.roles.include? :update_player
can :destroy, Player, :retired => false if user.roles.include? :destroy_player
can :history, Player, :retired => false if user.roles.include? :history_player
can :show_in_app, Player, :retired => false if user.roles.include? :show_in_app_player
@@ -128,6 +130,7 @@ def initialize(user)
should_not have_content("Edit")
@player = RailsAdmin::AbstractModel.new("Player").first
+ @player.id.should_not eql(12345)
@player.name.should eql("Jackie Robinson")
@player.number.should eql(42)
@player.position.should eql("Second baseman")
Something went wrong with that request. Please try again.