Rails 4 support - Githubissues

zeiv commented 10 years ago

Main Changes:

Updates test suite for Rails 4.0 and 4.1
Adds route compatibility for Rails 4
Adds gemfiles for Rails 4.0, 4.1
Updates analyzer (tests pass, no more no method `shift' error): closes #174 and closes #185, should also fix #193
Remove deprecated link_to_function and button_to_function from from development support
Removes deprecated unloadable method
Strong Parameters support: closes #184
Removes deprecated scoped method in favor of all, where(nil), and finder methods such as joins
Handles eager loading in Rails 4 with references method
Add Minitest aliases for forward compatibility with Rails > 4.1 test suite
Todo / Issues
Polymorphic associations don't quite work in if_attribute
193 probably not truly fixed after all
Nested resources have broken create action
All tests passing on:
Rails 2.3.18 (Ruby 1.8.7-p374) :white_check_mark:
Rails 3.0.20 (Ruby 1.9.3-p484) :white_check_mark:
Rails 3.1.12 (Ruby 1.9.3-p484) :white_check_mark:
Rails 3.2.16 (Ruby 1.9.3-p484) :white_check_mark:
Rails 4.0.2 (Ruby 2.1.2) :white_check_mark:
Rails 4.1.4 (Ruby 2.1.2) :white_check_mark:

For strong_parameters, I added a :strong_parameters option to filter_resource_access that accepts a boolean. It defaults to try on rails > 4 and false on rails < 4. When :strong_parameters is enabled, declarative_authorization will not create a new object or set an instance variable for the create action because strong_parameters's use of a private method to sanitize parameters expects that the object will be created in the original controller. This patch will, however, create an empty object for the instance variable of new actions.

You can see in the commit history that I initially used @gordonbisnor's fix which involved simply permitting all parameters for all filter_resource_access controllers. It was clean and it worked, but I ended up deciding that this was a too large a vulnerability and removed the fix. The best way to handle strong_parameters and in RESTful controllers with declarative_authorization is to simply use the Rails default method, which looks something like this:

def create
    @person = Person.new(person_params)
    respond_to do |format|
        if @person.save
            format.html { redirect_to @post, notice: 'Person was successfully created.' }
            format.json { render :show, status: :created, location: @person }
        else
            format.html { render :new }
            format.json { render json: @person.errors, status: :unprocessable_entity }
        end
    end
end

private
def person_params
    params.require(:person).permit(:name, :age)
end

:exclamation:IMPORTANT NOTE:exclamation: After switching rubies with RVM, Rails 4.1 returns 3 failures: HelperTest#test_has_role_with_guest_user, AuthorizationTest#test_default_role, and AuthorizationTest#test_guest_user. Raking a second time (no changes) returns only one failure: HelperTest#test_has_role_with_guest_user. After that, all tests pass, then it alternates between that one failure and all passing. Does anyone know what might be causing this? I'm marking Rails 4.1.4 as passing since my best guess is that it's a problem with the test suite.

I also recommend a version bump, possibly to 0.6.0?

gordonbisnor commented 10 years ago

Yeah I did not pull request my changes, was just a personal fork. They were intended only for an internal project. Not sure if there is a GitHub protocol to indicate that beyond not pull requesting. Thanks for this work!

mbhnyc commented 10 years ago

What's the status on this? Need these changes to get a migration to 4.1 to pass. I'm hopping on your branch @zeiv for now!!

maletor commented 10 years ago

ping @stffn.

zeiv commented 10 years ago

@mbhnyc this patch is ready to merge, in my eyes. It's in @stffn's hands now. :wink: He's pretty busy though, so feel free to use my fork for the time being. Also see my 1.0.0 branch, I'll be adding more features to that until this pull request is merged.

mbhnyc commented 10 years ago

Thanks @zeiv for providing the bridge code we need

maletor commented 10 years ago

I've tried out this patch, but I'm getting an error that wasn't present before.

 Failure/Error: post :read, id: id
 ActiveRecord::HasManyThroughCantAssociateThroughHasOneOrManyReflection:
   Cannot modify association 'Conversation#members' because the source reflection class 'Member' is associated to 'Message' via :has_many.

Using:

if_attribute members: { receiver: is { user } }

Abridged trace:

     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-4.0.8/lib/active_record/associations/through_association.rb:76:in `ensure_mutable'
     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-4.0.8/lib/active_record/associations/through_association.rb:40:in `construct_join_attributes'
     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-4.0.8/lib/active_record/associations/has_many_through_association.rb:149:in `delete_records'
     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-4.0.8/lib/active_record/associations/collection_association.rb:493:in `remove_records'
     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-4.0.8/lib/active_record/associations/collection_association.rb:486:in `block in delete_or_destroy'
     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-4.0.8/lib/active_record/associations/collection_association.rb:152:in `block in transaction'
     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-4.0.8/lib/active_record/connection_adapters/abstract/database_statements.rb:203:in `block in transaction'
     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-4.0.8/lib/active_record/connection_adapters/abstract/database_statements.rb:211:in `within_new_transaction'
     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-4.0.8/lib/active_record/connection_adapters/abstract/database_statements.rb:203:in `transaction'
     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-4.0.8/lib/active_record/transactions.rb:209:in `transaction'
     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-4.0.8/lib/active_record/associations/collection_association.rb:151:in `transaction'
     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-4.0.8/lib/active_record/associations/collection_association.rb:486:in `delete_or_destroy'
     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-4.0.8/lib/active_record/associations/collection_association.rb:236:in `delete'
     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-4.0.8/lib/active_record/associations/collection_proxy.rb:566:in `delete'
     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/bundler/gems/declarative_authorization-f09effc03599/lib/declarative_authorization/authorization.rb:630:in `object_attribute_value'
     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/bundler/gems/declarative_authorization-f09effc03599/lib/declarative_authorization/authorization.rb:516:in `block in validate?'
     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/bundler/gems/declarative_authorization-f09effc03599/lib/declarative_authorization/authorization.rb:515:in `each'
     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/bundler/gems/declarative_authorization-f09effc03599/lib/declarative_authorization/authorization.rb:515:in `all?'
     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/bundler/gems/declarative_authorization-f09effc03599/lib/declarative_authorization/authorization.rb:515:in `validate?'
     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/bundler/gems/declarative_authorization-f09effc03599/lib/declarative_authorization/authorization.rb:525:in `block in validate?'
     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/bundler/gems/declarative_authorization-f09effc03599/lib/declarative_authorization/authorization.rb:515:in `each'
     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/bundler/gems/declarative_authorization-f09effc03599/lib/declarative_authorization/authorization.rb:515:in `all?'
     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/bundler/gems/declarative_authorization-f09effc03599/lib/declarative_authorization/authorization.rb:515:in `validate?'
     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/bundler/gems/declarative_authorization-f09effc03599/lib/declarative_authorization/authorization.rb:453:in `block in validate?'
     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/bundler/gems/declarative_authorization-f09effc03599/lib/declarative_authorization/authorization.rb:451:in `each'
     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/bundler/gems/declarative_authorization-f09effc03599/lib/declarative_authorization/authorization.rb:451:in `any?'
     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/bundler/gems/declarative_authorization-f09effc03599/lib/declarative_authorization/authorization.rb:451:in `validate?'
     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/bundler/gems/declarative_authorization-f09effc03599/lib/declarative_authorization/authorization.rb:187:in `block in permit!'
     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/bundler/gems/declarative_authorization-f09effc03599/lib/declarative_authorization/authorization.rb:186:in `each'
     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/bundler/gems/declarative_authorization-f09effc03599/lib/declarative_authorization/authorization.rb:186:in `permit!'
     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/bundler/gems/declarative_authorization-f09effc03599/lib/declarative_authorization/in_controller.rb:662:in `permit!'
     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/bundler/gems/declarative_authorization-f09effc03599/lib/declarative_authorization/in_controller.rb:113:in `block in filter_access_filter'
     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/bundler/gems/declarative_authorization-f09effc03599/lib/declarative_authorization/in_controller.rb:113:in `each'
     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/bundler/gems/declarative_authorization-f09effc03599/lib/declarative_authorization/in_controller.rb:113:in `all?'
     # /Users/maletor/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/bundler/gems/declarative_authorization-f09effc03599/lib/declarative_authorization/in_controller.rb:113:in `filter_access_filter'

Could be because receiver is polymorphic?

zeiv commented 10 years ago

Hi @maletor, would you be able to post the relevant associations? I'm guessing part of it is something like:

class Message < ActiveRecord::Base
  belongs_to :members
  has_many :conversations
end

class Member < ActiveRecord::Base
  has_many :messages
  has_many :conversations, :through => :messages  
end

class Conversation < ActiveRecord::Base
  has_many :messages
  has_many :members, :through => :messages
end

But you mentioned it was polymorphic, and a Receiver class?

maletor commented 10 years ago

Hoping to get around to this at some point. Probably will be next week. Haven't forgot.

On Sunday, July 20, 2014, Xavier Bick notifications@github.com wrote:

Hi @maletor https://github.com/maletor, would you be able to post the relevant associations? I'm guessing it's something like:

class Message < ActiveRecord::Base belongs_to :members has_many :conversationsend class Member < ActiveRecord::Base has_many :messages has_many :conversations, :through => :messages end class Conversation < ActiveRecord::Base has_many :messages has_many :members, :through => :messagesend

But you mentioned it was polymorphic?

— Reply to this email directly or view it on GitHub https://github.com/stffn/declarative_authorization/pull/194#issuecomment-49547671 .

cschoene commented 10 years ago

Thanks zeiv, I really appreciate You effort on this ! There appears to be a small issue when using filter_resource_access with the :nested_in option: Declarative Authorization tries to load an object via :id for create actions (of the nested type that is to be created, not the parent), which doesn't make any sense. This occurs for the create action of every nested resource in my project since switching from 0.5.7 to
https://github.com/zeiv/declarative_authorization, regardless of whether the nesting is one or two levels deep.

zeiv commented 10 years ago

Hmm okay. It looks like I may have broken something with my "fix" to #193... I doubt I'll be able to look at it today, though. Hopefully over the weekend. Thanks for the feedback, everyone!

maletor commented 10 years ago

Experiencing the following error:

Failure/Error: get :index, id: group.id
ActiveRecord::AssociationTypeMismatch:
  Member(#70100856788920) expected, got NilClass(#70100740822840)

(byebug) current_user.id
1

(byebug) @post.group.members
#<ActiveRecord::Associations::CollectionProxy []>

filter_access_to [:index, :create], attribute_check: true, load_method: :new_post

if_attribute group: { members: { user_id: is { user.id } } }

Any ideas?

aepstein commented 10 years ago

Working off of this fix, I believe I have fixed #193 . Not sure how is best, but suggest you pull in changes from my rails4 branch. Also fixed issue reported above by @maletor . Added back the test associated with this problem with additional assertion for @maletor case.

maletor commented 10 years ago

Hi @aepstein,

We ran our gigantic 4,000 test suite against your branch and everything is green.

So I'd say it's looking good as far as our above 2 concerns and #193.

stffn / declarative_authorization

Rails 4 support #194

Main Changes:

Todo / Issues

193 probably not truly fixed after all

All tests passing on: