From f0feca800d91ac1d23eb63ca17a45d8fd4d41920 Mon Sep 17 00:00:00 2001 From: Tom Hughes Date: Wed, 26 Jun 2013 22:53:50 +0100 Subject: [PATCH] Replace attr_accessible with strong parameters --- app/controllers/diary_entry_controller.rb | 22 ++++++++++++++++----- app/controllers/message_controller.rb | 8 +++++++- app/controllers/notes_controller.rb | 2 +- app/controllers/oauth_clients_controller.rb | 8 ++++++-- app/controllers/trace_controller.rb | 6 +++--- app/controllers/user_blocks_controller.rb | 8 ++++---- app/controllers/user_controller.rb | 8 +++++++- app/controllers/user_roles_controller.rb | 4 +--- app/models/client_application.rb | 10 ++-------- app/models/diary_comment.rb | 2 -- app/models/diary_entry.rb | 2 -- app/models/message.rb | 6 ++---- app/models/note.rb | 2 -- app/models/oauth_nonce.rb | 2 -- app/models/oauth_token.rb | 4 +--- app/models/request_token.rb | 2 +- app/models/tracetag.rb | 2 -- app/models/user.rb | 6 ++---- app/models/user_block.rb | 4 ++-- app/models/user_preference.rb | 2 -- app/models/user_token.rb | 2 -- test/integration/user_blocks_test.rb | 8 ++++---- test/unit/diary_entry_test.rb | 4 ++-- test/unit/node_test.rb | 4 ++-- test/unit/oauth_token_test.rb | 4 +--- test/unit/trace_test.rb | 4 ++-- test/unit/tracetag_test.rb | 4 ++-- test/unit/user_test.rb | 8 ++++---- 28 files changed, 73 insertions(+), 75 deletions(-) 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 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 -- 2.43.2