From: Tom Hughes <tom@compton.nu>
Date: Wed, 26 Jun 2013 21:53:50 +0000 (+0100)
Subject: Replace attr_accessible with strong parameters
X-Git-Tag: live~5342
X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/f0feca800d91ac1d23eb63ca17a45d8fd4d41920

Replace attr_accessible with strong parameters
---

diff --git a/app/controllers/diary_entry_controller.rb b/app/controllers/diary_entry_controller.rb
index e900733e4..6b8acbe71 100644
--- a/app/controllers/diary_entry_controller.rb
+++ b/app/controllers/diary_entry_controller.rb
@@ -13,7 +13,7 @@ class DiaryEntryController < ApplicationController
     @title = t 'diary_entry.new.title'
 
     if params[:diary_entry]
-      @diary_entry = DiaryEntry.new(params[:diary_entry])
+      @diary_entry = DiaryEntry.new(entry_params)
       @diary_entry.user = @user
 
       if @diary_entry.save
@@ -43,7 +43,7 @@ class DiaryEntryController < ApplicationController
 
     if @user != @diary_entry.user
       redirect_to :controller => 'diary_entry', :action => 'view', :id => params[:id]
-    elsif params[:diary_entry] and @diary_entry.update_attributes(params[:diary_entry])
+    elsif params[:diary_entry] and @diary_entry.update_attributes(entry_params)
       redirect_to :controller => 'diary_entry', :action => 'view', :id => params[:id]
     end
 
@@ -54,7 +54,7 @@ class DiaryEntryController < ApplicationController
 
   def comment
     @entry = DiaryEntry.find(params[:id])
-    @diary_comment = @entry.comments.build(params[:diary_comment])
+    @diary_comment = @entry.comments.build(comment_params)
     @diary_comment.user = @user
     if @diary_comment.save
       if @diary_comment.user != @entry.user
@@ -160,13 +160,13 @@ class DiaryEntryController < ApplicationController
 
   def hide
     entry = DiaryEntry.find(params[:id])
-    entry.update_attributes({:visible => false}, :without_protection => true)
+    entry.update_attributes(:visible => false)
     redirect_to :action => "list", :display_name => entry.user.display_name
   end
 
   def hidecomment
     comment = DiaryComment.find(params[:comment])
-    comment.update_attributes({:visible => false}, :without_protection => true)
+    comment.update_attributes(:visible => false)
     redirect_to :action => "view", :display_name => comment.diary_entry.user.display_name, :id => comment.diary_entry.id
   end
 
@@ -181,6 +181,18 @@ class DiaryEntryController < ApplicationController
     @page = (params[:page] || 1).to_i
   end
 private
+  ##
+  # return permitted diary entry parameters
+  def entry_params
+    params.require(:diary_entry).permit(:title, :body, :language_code, :latitude, :longitude)
+  end
+
+  ##
+  # return permitted diary comment parameters
+  def comment_params
+    params.require(:diary_comment).permit(:body)
+  end
+
   ##
   # require that the user is a administrator, or fill out a helpful error message
   # and return them to the user page.
diff --git a/app/controllers/message_controller.rb b/app/controllers/message_controller.rb
index 8d03811a9..38c9b2f3d 100644
--- a/app/controllers/message_controller.rb
+++ b/app/controllers/message_controller.rb
@@ -17,7 +17,7 @@ class MessageController < ApplicationController
       if @user.sent_messages.where("sent_on >= ?", Time.now.getutc - 1.hour).count >= MAX_MESSAGES_PER_HOUR
         flash[:error] = t 'message.new.limit_exceeded'
       else
-        @message = Message.new(params[:message])
+        @message = Message.new(message_params)
         @message.to_user_id = @this_user.id
         @message.from_user_id = @user.id
         @message.sent_on = Time.now.getutc
@@ -127,4 +127,10 @@ class MessageController < ApplicationController
     @title = t'message.no_such_message.title'
     render :action => 'no_such_message', :status => :not_found
   end
+private
+  ##
+  # return permitted message parameters
+  def message_params
+    params.require(:message).permit(:title, :body)
+  end
 end
diff --git a/app/controllers/notes_controller.rb b/app/controllers/notes_controller.rb
index 3eb1ac3f9..cab04a0b9 100644
--- a/app/controllers/notes_controller.rb
+++ b/app/controllers/notes_controller.rb
@@ -347,7 +347,7 @@ private
       attributes[:author_ip] = request.remote_ip
     end
 
-    comment = note.comments.create(attributes, :without_protection => true)
+    comment = note.comments.create(attributes)
 
     note.comments.map { |c| c.author }.uniq.each do |user|
       if notify and user and user != @user
diff --git a/app/controllers/oauth_clients_controller.rb b/app/controllers/oauth_clients_controller.rb
index 56f19dbda..32fbbdd62 100644
--- a/app/controllers/oauth_clients_controller.rb
+++ b/app/controllers/oauth_clients_controller.rb
@@ -15,7 +15,7 @@ class OauthClientsController < ApplicationController
   end
 
   def create
-    @client_application = @user.client_applications.build(params[:client_application])
+    @client_application = @user.client_applications.build(application_params)
     if @client_application.save
       flash[:notice] = t'oauth_clients.create.flash'
       redirect_to :action => "show", :id => @client_application.id
@@ -37,7 +37,7 @@ class OauthClientsController < ApplicationController
 
   def update
     @client_application = @user.client_applications.find(params[:id])
-    if @client_application.update_attributes(params[:client_application])
+    if @client_application.update_attributes(application_params)
       flash[:notice] = t'oauth_clients.update.flash'
       redirect_to :action => "show", :id => @client_application.id
     else
@@ -51,4 +51,8 @@ class OauthClientsController < ApplicationController
     flash[:notice] = t'oauth_clients.destroy.flash'
     redirect_to :action => "index"
   end
+private
+  def application_params
+    params.require(:client_application).permit(:name, :url, :callback_url, :support_url, ClientApplication.all_permissions)
+  end
 end
diff --git a/app/controllers/trace_controller.rb b/app/controllers/trace_controller.rb
index 32369da21..db86684a4 100644
--- a/app/controllers/trace_controller.rb
+++ b/app/controllers/trace_controller.rb
@@ -139,7 +139,7 @@ class TraceController < ApplicationController
         @trace.errors.add(:gpx_file, "can't be blank")
       end
     else
-      @trace = Trace.new({:visibility => default_visibility}, :without_protection => true)
+      @trace = Trace.new(:visibility => default_visibility)
     end
 
     @title = t 'trace.create.upload_trace'
@@ -352,7 +352,7 @@ private
 
     # Create the trace object, falsely marked as already
     # inserted to stop the import daemon trying to load it
-    @trace = Trace.new({
+    @trace = Trace.new(
       :name => name,
       :tagstring => tags,
       :description => description,
@@ -360,7 +360,7 @@ private
       :inserted => true,
       :user => @user,
       :timestamp => Time.now.getutc
-    }, :without_protection => true)
+    )
 
     Trace.transaction do
       begin
diff --git a/app/controllers/user_blocks_controller.rb b/app/controllers/user_blocks_controller.rb
index 455e45c3f..2284174be 100644
--- a/app/controllers/user_blocks_controller.rb
+++ b/app/controllers/user_blocks_controller.rb
@@ -35,13 +35,13 @@ class UserBlocksController < ApplicationController
 
   def create
     if @valid_params 
-      @user_block = UserBlock.new({
+      @user_block = UserBlock.new(
         :user_id => @this_user.id,
         :creator_id => @user.id,
         :reason => params[:user_block][:reason],
         :ends_at => Time.now.getutc() + @block_period.hours,
         :needs_view => params[:user_block][:needs_view]
-      }, :without_protection => true)
+      )
     
       if @user_block.save
         flash[:notice] = t('user_block.create.flash', :name => @this_user.display_name)
@@ -59,11 +59,11 @@ class UserBlocksController < ApplicationController
       if @user_block.creator_id != @user.id
         flash[:error] = t('user_block.update.only_creator_can_edit')
         redirect_to :action => "edit"
-      elsif @user_block.update_attributes({
+      elsif @user_block.update_attributes(
               :ends_at => Time.now.getutc() + @block_period.hours,
               :reason => params[:user_block][:reason],
               :needs_view => params[:user_block][:needs_view]
-            }, :without_protection => true)
+            )
         flash[:notice] = t('user_block.update.success')
         redirect_to(@user_block)
       else
diff --git a/app/controllers/user_controller.rb b/app/controllers/user_controller.rb
index db37d1131..6f2894e3d 100644
--- a/app/controllers/user_controller.rb
+++ b/app/controllers/user_controller.rb
@@ -251,7 +251,7 @@ class UserController < ApplicationController
     else
       session[:referer] = params[:referer]
 
-      @user = User.new(params[:user])
+      @user = User.new(user_params)
       @user.status = "pending"
 
       if @user.openid_url.present? && @user.pass_crypt.empty?
@@ -809,4 +809,10 @@ private
     # it's .now so that this doesn't propagate to other pages.
     flash.now[:skip_terms] = true
   end
+
+  ##
+  # return permitted user parameters
+  def user_params
+    params.require(:user).permit(:email, :email_confirmation, :display_name, :openid_url, :pass_crypt, :pass_crypt_confirmation)
+  end
 end
diff --git a/app/controllers/user_roles_controller.rb b/app/controllers/user_roles_controller.rb
index 7dad891e2..8f623a04d 100644
--- a/app/controllers/user_roles_controller.rb
+++ b/app/controllers/user_roles_controller.rb
@@ -10,9 +10,7 @@ class UserRolesController < ApplicationController
   before_filter :in_role, :only => [:revoke]
 
   def grant
-    @this_user.roles.create({
-      :role => @role, :granter_id => @user.id
-    }, :without_protection => true)
+    @this_user.roles.create(:role => @role, :granter_id => @user.id)
     redirect_to :controller => 'user', :action => 'view', :display_name => @this_user.display_name
   end
 
diff --git a/app/models/client_application.rb b/app/models/client_application.rb
index 0619e75a3..ff031d62b 100644
--- a/app/models/client_application.rb
+++ b/app/models/client_application.rb
@@ -13,12 +13,6 @@ class ClientApplication < ActiveRecord::Base
   validates_format_of :support_url, :with => /\Ahttp(s?):\/\/(\w+:{0,1}\w*@)?(\S+)(:[0-9]+)?(\/|\/([\w#!:.?+=&%@!\-\/]))?/i, :allow_blank=>true
   validates_format_of :callback_url, :with => /\A[a-z][a-z0-9.+-]*:\/\/(\w+:{0,1}\w*@)?(\S+)(:[0-9]+)?(\/|\/([\w#!:.?+=&%@!\-\/]))?/i, :allow_blank=>true
 
-  attr_accessible :name, :url, :support_url, :callback_url,
-                  :allow_read_prefs, :allow_write_prefs,
-                  :allow_write_diary, :allow_write_api,
-                  :allow_read_gpx, :allow_write_gpx,
-                  :allow_write_notes
-
   before_validation :generate_keys, :on => :create
 
   attr_accessor :token_callback_url
@@ -60,7 +54,7 @@ class ClientApplication < ActiveRecord::Base
     permissions.each do |p|
       params[p] = true
     end
-    RequestToken.create(params, :without_protection => true)
+    RequestToken.create(params)
   end
 
   def access_token_for_user(user)
@@ -71,7 +65,7 @@ class ClientApplication < ActiveRecord::Base
         params[p] = true
       end
 
-      token = access_tokens.create(params, :without_protection => true)
+      token = access_tokens.create(params)
     end
     
     token
diff --git a/app/models/diary_comment.rb b/app/models/diary_comment.rb
index bea1c7f0d..9d29f52b7 100644
--- a/app/models/diary_comment.rb
+++ b/app/models/diary_comment.rb
@@ -5,8 +5,6 @@ class DiaryComment < ActiveRecord::Base
   validates_presence_of :body
   validates_associated :diary_entry
 
-  attr_accessible :body
-
   after_initialize :set_defaults
   after_save :spam_check
 
diff --git a/app/models/diary_entry.rb b/app/models/diary_entry.rb
index de2a42ae3..f7584c637 100644
--- a/app/models/diary_entry.rb
+++ b/app/models/diary_entry.rb
@@ -24,8 +24,6 @@ class DiaryEntry < ActiveRecord::Base
                             :greater_than_or_equal_to => -180, :less_than_or_equal_to => 180
   validates_associated :language
 
-  attr_accessible :title, :body, :language_code, :latitude, :longitude
-
   after_initialize :set_defaults
   after_save :spam_check
 
diff --git a/app/models/message.rb b/app/models/message.rb
index f897af3c2..b51c59f43 100644
--- a/app/models/message.rb
+++ b/app/models/message.rb
@@ -9,8 +9,6 @@ class Message < ActiveRecord::Base
   validates_inclusion_of :message_read, :in => [ true, false ]
   validates_as_utf8 :title
 
-  attr_accessible :title, :body
-
   after_initialize :set_defaults
 
   def self.from_mail(mail, from, to)
@@ -26,14 +24,14 @@ class Message < ActiveRecord::Base
       body = mail.decoded
     end
 
-    message = Message.new({
+    message = Message.new(
       :sender => from,
       :recipient => to,
       :sent_on => mail.date.new_offset(0),
       :title => mail.subject.sub(/\[OpenStreetMap\] */, ""),
       :body => body,
       :body_format => "text"
-    }, :without_protection => true)
+    )
   end
 
   def body
diff --git a/app/models/note.rb b/app/models/note.rb
index 10b74d8a6..67222191d 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -14,8 +14,6 @@ class Note < ActiveRecord::Base
   validates_inclusion_of :status, :in => ["open", "closed", "hidden"]
   validate :validate_position
 
-  attr_accessible :lat, :lon
-
   after_initialize :set_defaults
 
   # Sanity check the latitude and longitude and add an error if it's broken
diff --git a/app/models/oauth_nonce.rb b/app/models/oauth_nonce.rb
index 3ae50d3a4..84211e31d 100644
--- a/app/models/oauth_nonce.rb
+++ b/app/models/oauth_nonce.rb
@@ -4,8 +4,6 @@ class OauthNonce < ActiveRecord::Base
   validates_presence_of :nonce, :timestamp
   validates_uniqueness_of :nonce, :scope => :timestamp
 
-  attr_accessible :nonce, :timestamp
-
   # Remembers a nonce and it's associated timestamp. It returns false if it has already been used
   def self.remember(nonce, timestamp)
     oauth_nonce = OauthNonce.create(:nonce => nonce, :timestamp => timestamp)
diff --git a/app/models/oauth_token.rb b/app/models/oauth_token.rb
index c9595e870..f9255e56c 100644
--- a/app/models/oauth_token.rb
+++ b/app/models/oauth_token.rb
@@ -14,9 +14,7 @@ class OauthToken < ActiveRecord::Base
   end
   
   def invalidate!
-    update_attributes({
-      :invalidated_at => Time.now
-    }, :without_protection => true)
+    update_attributes(:invalidated_at => Time.now)
   end
   
   def authorized?
diff --git a/app/models/request_token.rb b/app/models/request_token.rb
index 6e4ec40c3..1ac502bc7 100644
--- a/app/models/request_token.rb
+++ b/app/models/request_token.rb
@@ -21,7 +21,7 @@ class RequestToken < OauthToken
         params[p] = read_attribute(p)
       }
 
-      access_token = AccessToken.create(params, :without_protection => true)
+      access_token = AccessToken.create(params)
       invalidate!
       access_token
     end
diff --git a/app/models/tracetag.rb b/app/models/tracetag.rb
index 58d1d782f..00f195e69 100644
--- a/app/models/tracetag.rb
+++ b/app/models/tracetag.rb
@@ -4,7 +4,5 @@ class Tracetag < ActiveRecord::Base
   validates_format_of :tag, :with => /\A[^\/;.,?]*\z/
   validates_length_of :tag, :within => 1..255
 
-  attr_accessible :tag
-
   belongs_to :trace, :foreign_key => 'gpx_id'
 end
diff --git a/app/models/user.rb b/app/models/user.rb
index f5c435309..20f1ba432 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -46,10 +46,6 @@ class User < ActiveRecord::Base
   validates_numericality_of :home_zoom, :only_integer => true, :allow_nil => true
   validates_inclusion_of :preferred_editor, :in => Editors::ALL_EDITORS, :allow_nil => true
 
-  attr_accessible :display_name, :email, :email_confirmation, :openid_url,
-                  :pass_crypt, :pass_crypt_confirmation, :consider_pd,
-                  :image_use_gravatar
-
   after_initialize :set_defaults
   before_save :encrypt_password
   after_save :spam_check
@@ -246,7 +242,9 @@ private
   end
 
   def encrypt_password
+logger.info "XXX"
     if pass_crypt_confirmation
+logger.info "YYY"
       self.pass_crypt, self.pass_salt = PasswordHash.create(pass_crypt)
       self.pass_crypt_confirmation = nil
     end
diff --git a/app/models/user_block.rb b/app/models/user_block.rb
index 2cf0eefc4..cb1a97dca 100644
--- a/app/models/user_block.rb
+++ b/app/models/user_block.rb
@@ -32,11 +32,11 @@ class UserBlock < ActiveRecord::Base
   # revokes the block, allowing the user to use the API again. the argument
   # is the user object who is revoking the ban.
   def revoke!(revoker)
-    update_attributes({
+    update_attributes(
       :ends_at => Time.now.getutc(),
       :revoker_id => revoker.id,
       :needs_view => false
-    }, :without_protection => true)
+    )
   end
 
 private
diff --git a/app/models/user_preference.rb b/app/models/user_preference.rb
index f10d0a3f9..b5110bbf5 100644
--- a/app/models/user_preference.rb
+++ b/app/models/user_preference.rb
@@ -6,8 +6,6 @@ class UserPreference < ActiveRecord::Base
   validates_length_of :k, :within => 1..255
   validates_length_of :v, :within => 1..255
 
-  attr_accessible :k, :v
-
   # Turn this Node in to an XML Node without the <osm> wrapper.
   def to_xml_node
     el1 = XML::Node.new 'preference'
diff --git a/app/models/user_token.rb b/app/models/user_token.rb
index 3060b33ea..735fd8485 100644
--- a/app/models/user_token.rb
+++ b/app/models/user_token.rb
@@ -1,8 +1,6 @@
 class UserToken < ActiveRecord::Base
   belongs_to :user
 
-  attr_accessible :referer
-
   after_initialize :set_defaults
 
   def expired?
diff --git a/test/integration/user_blocks_test.rb b/test/integration/user_blocks_test.rb
index 11fcae91a..fda9de6f7 100644
--- a/test/integration/user_blocks_test.rb
+++ b/test/integration/user_blocks_test.rb
@@ -17,12 +17,12 @@ class UserBlocksTest < ActionController::IntegrationTest
     assert_response :success
 
     # now block the user
-    UserBlock.create({
+    UserBlock.create(
       :user_id => blocked_user.id,
       :creator_id => users(:moderator_user).id,
       :reason => "testing",
       :ends_at => Time.now.getutc + 5.minutes
-    }, :without_protection => true)
+    )
     get "/api/#{API_VERSION}/user/details", nil, auth_header(blocked_user.display_name, "test")
     assert_response :forbidden
   end
@@ -31,12 +31,12 @@ class UserBlocksTest < ActionController::IntegrationTest
     blocked_user = users(:public_user)
     moderator = users(:moderator_user)
 
-    block = UserBlock.create({
+    block = UserBlock.create(
       :user_id => blocked_user.id,
       :creator_id => moderator.id,
       :reason => "testing",
       :ends_at => Time.now.getutc + 5.minutes
-    }, :without_protection => true)
+    )
     get "/api/#{API_VERSION}/user/details", nil, auth_header(blocked_user.display_name, "test")
     assert_response :forbidden
 
diff --git a/test/unit/diary_entry_test.rb b/test/unit/diary_entry_test.rb
index 2d5c34f83..0b64cbac3 100644
--- a/test/unit/diary_entry_test.rb
+++ b/test/unit/diary_entry_test.rb
@@ -44,8 +44,8 @@ class DiaryEntryTest < ActiveSupport::TestCase
 private
 
   def diary_entry_valid(attrs, result = true)
-    entry = DiaryEntry.new(diary_entries(:normal_user_entry_1).attributes, :without_protection => true)
-    entry.assign_attributes(attrs, :without_protection => true)
+    entry = DiaryEntry.new(diary_entries(:normal_user_entry_1).attributes)
+    entry.assign_attributes(attrs)
     assert_equal result, entry.valid?, "Expected #{attrs.inspect} to be #{result}"
   end  
 end
diff --git a/test/unit/node_test.rb b/test/unit/node_test.rb
index 753e6a95c..c62e15b29 100644
--- a/test/unit/node_test.rb
+++ b/test/unit/node_test.rb
@@ -77,13 +77,13 @@ class NodeTest < ActiveSupport::TestCase
   
   # Check that you can create a node and store it
   def test_create
-    node_template = Node.new({
+    node_template = Node.new(
       :latitude => 12.3456,
       :longitude => 65.4321,
       :changeset_id => changesets(:normal_user_first_change).id,
       :visible => 1, 
       :version => 1
-    }, :without_protection => true)
+    )
     assert node_template.create_with_history(users(:normal_user))
 
     node = Node.find(node_template.id)
diff --git a/test/unit/oauth_token_test.rb b/test/unit/oauth_token_test.rb
index ff0353791..9ec005c96 100644
--- a/test/unit/oauth_token_test.rb
+++ b/test/unit/oauth_token_test.rb
@@ -15,9 +15,7 @@ class OauthTokenTest < ActiveSupport::TestCase
   ##
   # check that an authorized token is authorised and can be invalidated
   def test_token_authorisation
-    tok = RequestToken.create({
-      :client_application => client_applications(:oauth_web_app)
-    }, :without_protection => true)
+    tok = RequestToken.create(:client_application => client_applications(:oauth_web_app))
     assert_equal false, tok.authorized?, "Token should be created unauthorised."
     tok.authorize!(users(:public_user))
     assert_equal true, tok.authorized?, "Token should now be authorised."
diff --git a/test/unit/trace_test.rb b/test/unit/trace_test.rb
index b8cf6b88a..f840d6c5c 100644
--- a/test/unit/trace_test.rb
+++ b/test/unit/trace_test.rb
@@ -84,8 +84,8 @@ private
   end
 
   def trace_valid(attrs, result = true)
-    entry = Trace.new(gpx_files(:public_trace_file).attributes, :without_protection => true)
-    entry.assign_attributes(attrs, :without_protection => true)
+    entry = Trace.new(gpx_files(:public_trace_file).attributes)
+    entry.assign_attributes(attrs)
     assert_equal result, entry.valid?, "Expected #{attrs.inspect} to be #{result}"
   end
 end
diff --git a/test/unit/tracetag_test.rb b/test/unit/tracetag_test.rb
index 0a22919b5..473071088 100644
--- a/test/unit/tracetag_test.rb
+++ b/test/unit/tracetag_test.rb
@@ -24,8 +24,8 @@ class TracetagTest < ActiveSupport::TestCase
 private
 
   def tracetag_valid(attrs, result = true)
-    entry = Tracetag.new(gpx_file_tags(:first_trace_1).attributes, :without_protection => true)
-    entry.assign_attributes(attrs, :without_protection => true)
+    entry = Tracetag.new(gpx_file_tags(:first_trace_1).attributes)
+    entry.assign_attributes(attrs)
     assert_equal result, entry.valid?, "Expected #{attrs.inspect} to be #{result}"
   end
 end
diff --git a/test/unit/user_test.rb b/test/unit/user_test.rb
index 1346c855c..175d47e22 100644
--- a/test/unit/user_test.rb
+++ b/test/unit/user_test.rb
@@ -18,27 +18,27 @@ class UserTest < ActiveSupport::TestCase
   end
   
   def test_unique_email
-    new_user = User.new({
+    new_user = User.new(
       :email => users(:normal_user).email,
       :status => "active", 
       :pass_crypt => Digest::MD5.hexdigest('test'),
       :display_name => "new user",
       :data_public => 1,
       :description => "desc"
-    }, :without_protection => true)
+    )
     assert !new_user.save
     assert new_user.errors[:email].include?("has already been taken")
   end
   
   def test_unique_display_name
-    new_user = User.new({
+    new_user = User.new(
       :email => "tester@openstreetmap.org",
       :status => "pending",
       :pass_crypt => Digest::MD5.hexdigest('test'),
       :display_name => users(:normal_user).display_name, 
       :data_public => 1,
       :description => "desc"
-    }, :without_protection => true)
+    )
     assert !new_user.save
     assert new_user.errors[:display_name].include?("has already been taken")
   end