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

Hashie::Mash doesn't play well with Rails and strong_parameters #89

Closed
weppos opened this issue Mar 25, 2013 · 20 comments · Fixed by #104
Closed

Hashie::Mash doesn't play well with Rails and strong_parameters #89

weppos opened this issue Mar 25, 2013 · 20 comments · Fixed by #104

Comments

@weppos
Copy link

weppos commented Mar 25, 2013

I upgraded an app from Hashie::Mash 1.2 to 2.0 and I started to notice an incompatibility when using strong_parameters (thus Rails 4).

Have a look at the following example:

settings = { foo: "1", attributes: { title: "Value" } } 

# Mash 1.2
record.attributes = settings.attributes
# it works

# Mash 2.x
record.attributes = settings.attributes
# raises ActiveModel::ForbiddenAttributes

This happens because in the 2.x version, when a key is a Hash, it is automatically converted into a Mash.

The Mash instance responds to :permit? thus it triggers the ActiveModel forbidden attribute check.

  module ForbiddenAttributesProtection
    def sanitize_for_mass_assignment(*options)
      new_attributes = options.first
      if !new_attributes.respond_to?(:permitted?) || new_attributes.permitted?
        super
      else
        raise ActiveModel::ForbiddenAttributes
      end
    end
  end

But because there is no attribute permit, then #permit? returns false and the error is raised.

Is it intentional? Any suggestion? Any chance that the implementation of Hashie::Mash#respond_to? would not return true for #attribute? unless attribute exists in the Mash table?

@jch
Copy link
Contributor

jch commented Mar 25, 2013

I didn't know about strong parameters until you brought it up here. This problem sounds specific to ActiveModel, so it makes sense to me to add a Hashie::ActiveModel module (or something simliar) that adds the additional methods. It sounds like you have a good grasp of the problem, would you be up trying to write up a pull request for this?

@weppos
Copy link
Author

weppos commented Apr 8, 2013

I'll try to have a look at it, but I'm not sure I can schedule some time immediately.

@Maxim-Filimonov
Copy link
Contributor

@jch The problem is specific to active model but why does Mash needs to respond to key_not_in_mash? What's would be the use case for such scenario?

@jch
Copy link
Contributor

jch commented May 14, 2013

I honestly don't remember. @mbleigh care to chime in?

@aew
Copy link

aew commented Jul 21, 2013

Any update here? This blocks upgrading active record to 4.0 in a standalone Grape app...

@Maxim-Filimonov
Copy link
Contributor

I can do a pull request which removes hash responding to attributes not in hash. Not sure that's gonna be accepted

@aew
Copy link

aew commented Jul 21, 2013

Well, I would use your pull!

@Maxim-Filimonov
Copy link
Contributor

Pull request is done. Checked - it works with rails 4 without forbidden attributes error:

# With rubygems version
2.0.0-p0 :001 > settings = {:username => 'Jonh', password: 'Bond' }
 => {:username=>"Jonh", :password=>"Bond"}
2.0.0-p0 :002 > m = Hashie::Mash.new(settings)
 => #<Hashie::Mash password="Bond" username="Jonh">
2.0.0-p0 :003 > u = User.create!
   (0.1ms)  begin transaction
  SQL (2.2ms)  INSERT INTO "users" ("created_at", "updated_at") VALUES (?, ?)  [["created_at", Sun, 21 Jul 2013 05:58:37 UTC +00:00], ["updated_at", Sun, 21 Jul 2013 05:58:37 UTC +00:00]]
   (2.3ms)  commit transaction
 => #<User id: 1, username: nil, password: nil, created_at: "2013-07-21 05:58:37", updated_at: "2013-07-21 05:58:37">
2.0.0-p0 :004 > u.update_attributes(m)
   (0.1ms)  begin transaction
   (0.1ms)  rollback transaction
ActiveModel::ForbiddenAttributesError: ActiveModel::ForbiddenAttributesError
# With pull request 
2.0.0-p0 :001 > u = User.first
  User Load (0.3ms)  SELECT "users".* FROM "users" ORDER BY "users"."id" ASC LIMIT 1
 => #<User id: 1, username: nil, password: nil, created_at: "2013-07-21 05:58:37", updated_at: "2013-07-21 05:58:37">
2.0.0-p0 :002 > settings = {:username => 'Jonh', password: 'Bond' }
 => {:username=>"Jonh", :password=>"Bond"}
2.0.0-p0 :003 > m = Hashie::Mash.new(settings)
 => #<Hashie::Mash password="Bond" username="Jonh">
2.0.0-p0 :004 > u.update_attributes(m)
   (0.2ms)  begin transaction
  SQL (5.3ms)  UPDATE "users" SET "username" = ?, "password" = ?, "updated_at" = ? WHERE "users"."id" = 1  [["username", "Jonh"], ["password", "Bond"], ["updated_at", Sun, 21 Jul 2013 06:00:29 UTC +00:00]]
   (2.6ms)  commit transaction
 => true
2.0.0-p0 :005 > u.reload
  User Load (0.4ms)  SELECT "users".* FROM "users" WHERE "users"."id" = ? LIMIT 1  [["id", 1]]
 => #<User id: 1, username: "Jonh", password: "Bond", created_at: "2013-07-21 05:58:37", updated_at: "2013-07-21 06:00:29">

As suggested in ruby-grape/grape#404 proper support for forbidden attributes would be good as a separate gem.

@hasghari
Copy link

I noticed this issue as well. The quick solution for me was to call to_hash on the Hashie::Mash instance so it's not responding to permitted? anymore.

# Mash 2.x
record.attributes = settings.attributes.to_hash

@Maxim-Filimonov
Copy link
Contributor

Thanks @hasghari, sounds like a good workaround but it feels a bit hacky to me to do it everywhere.
@jch, @mbleigh any chance of getting feedback on the proposed change?

@jch
Copy link
Contributor

jch commented Aug 26, 2013

@Maxim-Filimonov sorry, don't know commit access anymore.

@jeveloper
Copy link

Guys, i appreciate all the work Intridea is doing , but this still isn't working out the of the box and Rails 4 won't work with strong parameters properly

please fix it :)
thank you

@tamadamas
Copy link

I fixed this on #125

@Maxim-Filimonov
Copy link
Contributor

@mli-max this works for me, hope yours get merged..

@lynxnathan
Copy link

@intridea any plans on merging this?

@pruett
Copy link

pruett commented Feb 12, 2014

@intridea any thoughts on merging in @mli-max's PR?

@mogetutu
Copy link

@intridea ping!

@jorge-d
Copy link

jorge-d commented Mar 28, 2014

+1 !

@dblock
Copy link
Member

dblock commented Mar 30, 2014

I'm taking over Hashie. Fixed in #104.

@dblock
Copy link
Member

dblock commented Apr 30, 2014

The change in #104 solved this problem but introduced inconsistent behavior into a pure Mash, just for Rails. I am reverting #104 because of #146 and adding an ActiveModel extension, please take a look at #147, add your comments and suggestions.

jsonn pushed a commit to jsonn/pkgsrc that referenced this issue Feb 6, 2015
${RUBY_PKGPREFIX}-hashie2.

## 2.1.2 (5/12/2014)

* [#169](hashie/hashie#169): Hash#to_hash will also convert nested objects that implement `to_hash` - [@gregory](https://github.com/gregory).

## 2.1.1 (4/12/2014)

* [#144](hashie/hashie#144): Fixed regression invoking `to_hash` with no parameters - [@mbleigh](https://github.com/mbleigh).

## 2.1.0 (4/6/2014)

* [#134](hashie/hashie#134): Add deep_fetch extension for nested access - [@tylerdooling](https://github.com/tylerdooling).
* Removed support for Ruby 1.8.7 - [@dblock](https://github.com/dblock).
* Ruby style now enforced with Rubocop - [@dblock](https://github.com/dblock).
* [#138](hashie/hashie#138): Added Hashie::Rash, a hash whose keys can be regular expressions or ranges - [@epitron](https://github.com/epitron).
* [#131](hashie/hashie#131): Added IgnoreUndeclared, an extension to silently ignore undeclared properties at intialization - [@righi](https://github.com/righi).
* [#136](hashie/hashie#136): Removed Hashie::Extensions::Structure - [@markiz](https://github.com/markiz).
* [#107](hashie/hashie#107): Fixed excessive value conversions, poor performance of deep merge in Hashie::Mash - [@davemitchell](https://github.com/dblock), [@dblock](https://github.com/dblock).
* [#69](hashie/hashie#69): Fixed assigning multiple properties in Hashie::Trash - [@einzige](https://github.com/einzige).
* [#100](hashie/hashie#100): IndifferentAccess#store will respect indifference - [@jrochkind](https://github.com/jrochkind).
* [#103](hashie/hashie#103): Fixed support for Hashie::Dash properties that end in bang - [@thedavemarshall](https://github.com/thedavemarshall).
* [89](hashie/hashie#89): Do not respond to every method with suffix in Hashie::Mash, fixes Rails strong_parameters - [@Maxim-Filimonov](https://github.com/Maxim-Filimonov).
* [#110](hashie/hashie#110): Correctly use Hash#default from Mash#method_missing - [@ryansouza](https://github.com/ryansouza).
* [#120](hashie/hashie#120): Pass options to recursive to_hash calls - [@pwillett](https://github.com/pwillett).
* [#113](hashie/hashie#113): Fixed Hash#merge with Hashie::Dash - [@spencer1248](https://github.com/spencer1248).
* [#99](hashie/hashie#99): Hash#deep_merge raises errors when it encounters integers - [@defsprite](https://github.com/defsprite).
* [#133](hashie/hashie#133): Fixed Hash##to_hash with symbolize_keys - [@mhuggins](https://github.com/mhuggins).
* [#130](hashie/hashie#130): IndifferentAccess now works without MergeInitializer - [@npj](https://github.com/npj).
* [#111](hashie/hashie#111): Trash#translations correctly maps original to translated names - [@artm](https://github.com/artm).
* [#129](hashie/hashie#129): Added Trash#permitted_input_keys and inverse_translations - [@artm](https://github.com/artm).
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

Successfully merging a pull request may close this issue.