Fix mass assignment error when updating payment method #9

Closed
wants to merge 1 commit into
from

Projects

None yet

5 participants

@phillcampbell

Due to mass assignment changes in rails 3.2.3 we need to add any preferences to attr_accessible otherwise we get a mass assignment error when trying to update payment methods.

This should fix Issue #1386 in spree when used in conjunction with my pull request in spree

@radar radar added a commit that referenced this pull request Apr 13, 2012
@ph1ll ph1ll set attr_accessible in gateway models to allow updating payment metho…
…d settings

Merges #9
47a7e4f
@radar
Spree Commerce member

Merged in above commit. Thanks!

@radar radar closed this Apr 13, 2012
@pferdefleisch

Hiya,
This also will be necessary for PaymentMethods
WARNING: Can't mass-assign protected attributes: name, description, environment, display_on, active, type
I just got this when trying to add Braintree. Adding these to the attr_accessible fixed it for me. I would submit a patch but this i don't know about the security ramifications of whitelisting these. If all it is is doing what I already did, let me know and I will submit a patch.

@radar
Spree Commerce member

@pferdefleisch Please provide information about what Spree version you're running on and steps to reproduce the issue on a new issue on this project please.

@pferdefleisch

sorry man.

   spree_gateway (1.0.0)
      spree_core (>= 1.0.beta)
    spree (1.0.3)
      spree_api (= 1.0.3)
      spree_auth (= 1.0.3)
      spree_cmd (= 1.0.3)
      spree_core (= 1.0.3)
      spree_dash (= 1.0.3)
      spree_promo (= 1.0.3)
      spree_sample (= 1.0.3)
  sass-rails (3.1.6)
    coffee-rails (3.1.1)
      jquery-rails (>= 1.0.0)
      rails (>= 3.1.0)
      rails_autolink (>= 1.0.4)
      rails (>= 3.0.9)
    jquery-rails (1.0.19)
    pry-rails (0.1.6)
    rails (3.1.4)

In spree's admin section I went to
Configuration => Payment Methods
Picked "New Payment Method"
Filled out the name, description, left the selects alone except for Provider which I chose as Braintree
Clicked create
Got "You cannot save without a name" error and above on my development.log

I have white listed the above attributes in my attr_accessible branch of my fork of spree_gateway (only for braintree) https://github.com/pferdefleisch/spree_gateway/tree/attr_accessible
That is working for me and it should be secure, no?
Thanks!

@radar
Spree Commerce member

Spree 1.0.3 but running into mass assignment issues? That's most certainly different.

@pferdefleisch

swear to God man. Still broken when I switch back to the spree version of spree_gateway

Started POST "/work/admin/payment_methods" for 127.0.0.1 at 2012-04-18 21:10:20 +0200
  Processing by Spree::Admin::PaymentMethodsController#create as HTML
  Parameters: {"utf8"=>"✓", "authenticity_token"=>"220sR+0/W/NP0QOa4iaOez+P70G5Q8bxbMfVZrSBCEs=", "payment_method"=>{"name"=>"Pooooooo", "description"=>"Nisi enim bacon sunt corned beef, pastrami pork loin frankfurter venison brisket shoulder tail sint incididunt kielbasa. Consequat chicken nisi commodo, biltong do pig beef ribs corned beef irure bresaola tri-tip. Prosciutto id boudin, beef ribs culpa nulla mollit esse nostrud pancetta drumstick shankle excepteur ribeye pariatur. Voluptate pork corned beef shankle, salami ball tip tongue laboris prosciutto jerky anim commodo leberkas. Ham hock labore nulla, dolore in tri-tip leberkas ut drumstick duis pancetta elit.", "environment"=>"development", "display_on"=>"", "active"=>"true", "type"=>"Spree::Gateway::Braintree"}, "commit"=>"Create"}
  Spree::User Load (0.5ms)  SELECT "spree_users".* FROM "spree_users" WHERE "spree_users"."id" = 1 LIMIT 1
  Spree::Role Load (0.5ms)  SELECT "spree_roles".* FROM "spree_roles" INNER JOIN "spree_roles_users" ON "spree_roles"."id" = "spree_roles_users"."role_id" WHERE "spree_roles_users"."user_id" = 1
WARNING: Can't mass-assign protected attributes: name, description, environment, display_on, active, type
   (0.1ms)  BEGIN
   (0.1ms)  ROLLBACK
Deface: 2 overrides found for 'spree/layouts/admin'
Deface: 'auth_admin_login_navigation_bar' matched 1 times with '[data-hook='admin_login_navigation_bar'], #admin_login_navigation_bar[data-hook]'
Deface: [WARNING] No :original defined for 'auth_admin_login_navigation_bar', you should change its definition to include:
 :original => '0a5476d4d5db90ec8dd200ebaa0109a6a54ec6bc' 
Deface: 'promo_admin_tabs' matched 1 times with '[data-hook='admin_tabs'], #admin_tabs[data-hook]'
Deface: [WARNING] No :original defined for 'promo_admin_tabs', you should change its definition to include:
 :original => '3e847740dc3e7f924aba1ccb4cb00c7b841649e3' 
Rendered vendor/bundle/ruby/1.9.1/gems/spree_core-1.0.3/app/views/spree/admin/shared/_configuration_menu.html.erb (3.0ms)
Rendered vendor/bundle/ruby/1.9.1/gems/spree_core-1.0.3/app/views/spree/shared/_error_messages.html.erb (0.6ms)
Rendered vendor/bundle/ruby/1.9.1/gems/spree_core-1.0.3/app/views/spree/admin/payment_methods/_form.html.erb (2.1ms)
Rendered vendor/bundle/ruby/1.9.1/gems/spree_core-1.0.3/app/views/spree/admin/payment_methods/new.html.erb within spree/layouts/admin (14.1ms)
Rendered vendor/bundle/ruby/1.9.1/gems/spree_core-1.0.3/app/views/spree/admin/shared/_head.html.erb (2.0ms)
Rendered vendor/bundle/ruby/1.9.1/gems/spree_core-1.0.3/app/views/spree/admin/shared/_tabs.html.erb (1.2ms)
Rendered vendor/bundle/ruby/1.9.1/gems/spree_core-1.0.3/app/views/spree/admin/shared/_alert.html.erb (0.0ms)
Completed 200 OK in 79ms (Views: 46.0ms | ActiveRecord: 0.0ms)


Started GET "/assets/admin/bg/spree_50.png" for 127.0.0.1 at 2012-04-18 21:10:20 +0200
Served asset /admin/bg/spree_5
source 'https://rubygems.org'
gem 'nokogiri'
gem 'yajl-ruby'
gem 'rails', '~> 3.1.0'
gem 'active_reload'

gem 'pg'
gem 'bourbon'

gem 'slim-rails'
gem 'redcarpet'
gem 'jquery-rails'

# Use unicorn as the app server
gem 'unicorn'
gem 'foreman'

gem 'capistrano'
# Gems used only for assets and not required
# in production environments by default.
group :assets do
  #gem 'sass-rails',   '~> 3.2.0'
  gem 'sass-rails', :git => "git://github.com/rails/sass-rails.git", :branch => "3-1-stable"
  gem 'coffee-rails', '~> 3.1.0'
  gem 'uglifier', '>= 1.0.3'
end

gem 'comfortable_mexican_sofa', git: "git://github.com/pferdefleisch/comfortable-mexican-sofa.git", branch: "categories"
gem 'comfy_blog'

gem 'spree_usa_epay'
gem 'spree_skrill'
gem 'spree', "~> 1.0"
gem 'spree_gateway', :git => "git://github.com/spree/spree_gateway"
#gem 'spree_gateway', :git => "git://github.com/pferdefleisch/spree_gateway", :branch => "attr_accessible"

group :development do
  gem 'pry-rails'
  gem 'debugger'
end
@greinacker

I see this too, with authorize.net - the problem is the attr_accessible change that was merged whitelists a few attributes, but none of the others are accessible. Before the merge, all of them were accessible.

@radar
Spree Commerce member

@greinacker can you submit a patch to fix this issue please?

@greinacker

@radar yes, but I'm not sure the best way to fix it.

When running under Spree 1.1, the code as it now works; specifically we have (e.g. for authorize.net):

spree_gateway / app / models / spree / gateway / authorize_net.rb:

attr_accessible :preferred_login, :preferred_password

spree / core / app / models / spree / gateway.rb:

attr_accessible :preferred_server, :preferred_test_mode

spree / core / app / models / spree / payment_method.rb:

attr_accessible :name, :description, :environment, :display_on, :active

All of those attr_accessibles are what we need.

When running under Spree 1.0.3, with this spree_gateway code, we run into this error:

WARNING: Can't mass-assign protected attributes: name, description, environment, display_on, active, preferred_server, preferred_test_mode

basically all of the preferences on the base classes Gateway and PaymentMethod are no longer accessible, because of the attr_accessible white list on the derived class we're using.

I can think of three solutions:

  1. add all of the necessary attr_accessibles to spree_gateway / app / models / spree / gateway / authorize_net.rb et al. I think this should work, but we're adding attr_accessible for base class preferences, and it feels pretty ugly. Not to mention we're duplicating the attr_accessible unnecessarily for 1.1 users.

  2. Release a spree 1.0.4 with the appropriate attr_accessible's. This would work, but it would break anyone else who is subclassing Gateway or PaymentMethod without paying attention.

  3. Have a separate spree_gateway branch that's just for spree 1.0.x users.

None of those options sound great...ideas?

@greinacker

FYI for anyone stumbling across this and needing something that works right now on 1.0.3 - you can use this in your Gemfile:

gem 'spree_gateway', :git => 'git://github.com/spree/spree_gateway.git', :ref => 'e0aa8bfbb7c26786b276d1a3eaa9820c1aa08c79'
@radar
Spree Commerce member

We would release 1.0.4 with the appropriate attr accessibles. Please provide a patch that does that.

@greinacker

Ok - spree pull request here:

spree/spree#1459

Someone should probably update this spree_gateway gem spec to specify it needs spree_core 1.0.4 or later...I would have done it, but then I'd have like 3 PR's all dependent on each other, and I thought I'd try to only have a couple for now. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment