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

Rails 4 support #194

Merged
merged 24 commits into from
Aug 13, 2014
Merged

Rails 4 support #194

merged 24 commits into from
Aug 13, 2014

Conversation

zeiv
Copy link
Contributor

@zeiv zeiv commented Jul 3, 2014

Main Changes:

Todo / Issues

All tests passing on:

  • Rails 2.3.18 (Ruby 1.8.7-p374) ✅
  • Rails 3.0.20 (Ruby 1.9.3-p484) ✅
  • Rails 3.1.12 (Ruby 1.9.3-p484) ✅
  • Rails 3.2.16 (Ruby 1.9.3-p484) ✅
  • Rails 4.0.2 (Ruby 2.1.2) ✅
  • Rails 4.1.4 (Ruby 2.1.2) ✅

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

IMPORTANT NOTE
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?

zeiv added 18 commits December 29, 2013 20:53
…ew methods work, instance variables work, all is well.
…cess and set the option to false for Rails < 4
@gordonbisnor
Copy link
Contributor

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
Copy link

mbhnyc commented Jul 14, 2014

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
Copy link

maletor commented Jul 16, 2014

ping @stffn.

@zeiv
Copy link
Contributor Author

zeiv commented Jul 17, 2014

@mbhnyc this patch is ready to merge, in my eyes. It's in @stffn's hands now. 😉 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
Copy link

mbhnyc commented Jul 17, 2014

Thanks @zeiv for providing the bridge code we need

@maletor
Copy link

maletor commented Jul 18, 2014

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
Copy link
Contributor Author

zeiv commented Jul 20, 2014

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
Copy link

maletor commented Jul 25, 2014

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

On Sunday, July 20, 2014, Xavier Bick [email protected] 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
#194 (comment)
.

@cschoene
Copy link

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
Copy link
Contributor Author

zeiv commented Jul 25, 2014

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
Copy link

maletor commented Aug 8, 2014

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
Copy link
Contributor

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
Copy link

maletor commented Aug 12, 2014

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.

@divoxx divoxx mentioned this pull request Aug 12, 2014
@stffn stffn merged commit f09effc into stffn:master Aug 13, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants