search

couchbase / couchbase-ruby-model

The Active Model implementation for Couchbase Server built on couchbase-ruby-client
61 stars 23 forks source link

Added Mass Assignment Security #11

Closed jabberfest closed 11 years ago

jabberfest commented 11 years ago

[Fixes #10] - Added ActiveModel::MassAssignmentSecurity support.

jabberfest commented 11 years ago

Pull Request isn't ready to be merged. I need to update the tests.... Not sure how you have it setup but when I run rake test I get tests failing with "NoMethodError: undefined method `sanitize_for_mass_assignment' for #Post:0x0000000205c7b8"

Can you help look into it? Looks like ActiveModel::MassAssignmentSecurity doesn't get loaded in the test environment.

    if defined?(::Rails)
      extend ActiveModel::Callbacks
      extend ActiveModel::Naming
      include ActiveModel::Conversion
      include ActiveModel::Validations
      include ActiveModel::MassAssignmentSecurity

My changes work fine if I test manually from a rails app using the same models used in the unit tests.

avsej commented 11 years ago

Looks good to me, I will merge it when some smoke tests will be available

avsej commented 11 years ago

Probably this patch will help http://review.couchbase.org/26137

jabberfest commented 11 years ago

Thanks I will try that. I have it working now. Need to finish tests before I add commits to this pull request. Will have it in a day or so.

jabberfest commented 11 years ago

Should be good to go :-) Let me know if you disagree with anything.

avsej commented 11 years ago

Moved to review system http://review.couchbase.org/26236

avsej commented 11 years ago

Fixed in 99fa4825fc74dfc2cf522b75d797b02512001d58, @ingenthr could you close the ticket?

farias-r7 commented 11 years ago

I'm confused. The commit you referenced refers to "Support for batch finding multiple objects by id".

ka8725 commented 11 years ago

@farias-r7 , your pull request on code review - http://review.couchbase.org/#/c/26236/2

jabberfest commented 11 years ago

Not sure if this review is a low priority. I know you guys are busy. I'm just using my own branch hoping this gets merged in, but I'm gonna be in a crappy state if it doesn't.

I don't want to maintain my own fork for a simple fix :-)

ka8725 commented 11 years ago

Rails 4 doesn't has mass assignment security out of the box. Developers moved it to the gem. Also today there is a trend to use strong parameters gem. So I think you have to choices - move your changes to the separated gem or use strong parameters aapproach. @avsej what do you think?

jabberfest commented 11 years ago

Closing ticket. @ka8725 you are right. I just looked into Rails 4 and it makes sense to go the strong parameters route. Thanks for the info!

Please close the associated ticket in your internal ticket system.