diff --git a/Gemfile b/Gemfile index 1b064d32..b0889bc4 100644 --- a/Gemfile +++ b/Gemfile @@ -3,7 +3,6 @@ source "https://rubygems.org" gemspec gem "rails", "~> 4.0.0.beta", github: "rails/rails", branch: "master" -gem "protected_attributes", "~> 1.0.0" gem "omniauth", "~> 1.0.0" gem "omniauth-oauth2", "~> 1.0.0" gem "rdoc" diff --git a/Gemfile.lock b/Gemfile.lock index 16c64625..80b5b3d2 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -111,8 +111,6 @@ GEM origin (1.0.11) orm_adapter (0.4.0) polyglot (0.3.3) - protected_attributes (1.0.1) - activemodel (>= 4.0.0.beta, < 5.0) rack (1.5.2) rack-openid (1.3.1) rack (>= 1.1.0) @@ -162,7 +160,6 @@ DEPENDENCIES omniauth-facebook omniauth-oauth2 (~> 1.0.0) omniauth-openid (~> 1.0.1) - protected_attributes (~> 1.0.0) rails (~> 4.0.0.beta)! rdoc sqlite3 diff --git a/app/controllers/devise/confirmations_controller.rb b/app/controllers/devise/confirmations_controller.rb index 68014c92..b039a477 100644 --- a/app/controllers/devise/confirmations_controller.rb +++ b/app/controllers/devise/confirmations_controller.rb @@ -1,7 +1,7 @@ class Devise::ConfirmationsController < DeviseController # GET /resource/confirmation/new def new - build_resource({}) + self.resource = resource_class.new end # POST /resource/confirmation @@ -39,5 +39,4 @@ class Devise::ConfirmationsController < DeviseController def after_confirmation_path_for(resource_name, resource) after_sign_in_path_for(resource) end - end diff --git a/app/controllers/devise/passwords_controller.rb b/app/controllers/devise/passwords_controller.rb index a568d87f..c92421ea 100644 --- a/app/controllers/devise/passwords_controller.rb +++ b/app/controllers/devise/passwords_controller.rb @@ -5,7 +5,7 @@ class Devise::PasswordsController < DeviseController # GET /resource/password/new def new - build_resource({}) + self.resource = resource_class.new end # POST /resource/password diff --git a/app/controllers/devise/registrations_controller.rb b/app/controllers/devise/registrations_controller.rb index 281979a7..a990c93a 100644 --- a/app/controllers/devise/registrations_controller.rb +++ b/app/controllers/devise/registrations_controller.rb @@ -4,13 +4,13 @@ class Devise::RegistrationsController < DeviseController # GET /resource/sign_up def new - resource = build_resource({}) - respond_with resource + build_resource({}) + respond_with self.resource end # POST /resource def create - build_resource + self.resource = build_resource(sign_up_params) if resource.save if resource.active_for_authentication? @@ -40,7 +40,7 @@ class Devise::RegistrationsController < DeviseController self.resource = resource_class.to_adapter.get!(send(:"current_#{resource_name}").to_key) prev_unconfirmed_email = resource.unconfirmed_email if resource.respond_to?(:unconfirmed_email) - if resource.update_with_password(resource_params) + if resource.update_with_password(account_update_params) if is_navigational_format? flash_key = update_needs_confirmation?(resource, prev_unconfirmed_email) ? :update_needs_confirmation : :updated @@ -83,8 +83,7 @@ class Devise::RegistrationsController < DeviseController # Build a devise resource passing in the session. Useful to move # temporary session data to the newly created user. def build_resource(hash=nil) - hash ||= resource_params || {} - self.resource = resource_class.new_with_session(hash, session) + self.resource = resource_class.new_with_session(hash || {}, session) end # Signs in a user on sign up. You can overwrite this method in your own @@ -116,4 +115,12 @@ class Devise::RegistrationsController < DeviseController send(:"authenticate_#{resource_name}!", :force => true) self.resource = send(:"current_#{resource_name}") end + + def sign_up_params + devise_parameter_sanitizer.for(:sign_up) + end + + def account_update_params + devise_parameter_sanitizer.for(:account_update) + end end diff --git a/app/controllers/devise/sessions_controller.rb b/app/controllers/devise/sessions_controller.rb index e3760370..9c355f38 100644 --- a/app/controllers/devise/sessions_controller.rb +++ b/app/controllers/devise/sessions_controller.rb @@ -5,7 +5,7 @@ class Devise::SessionsController < DeviseController # GET /resource/sign_in def new - self.resource = build_resource(nil, :unsafe => true) + self.resource = resource_class.new(sign_in_params) clean_up_passwords(resource) respond_with(resource, serialize_options(resource)) end @@ -34,6 +34,10 @@ class Devise::SessionsController < DeviseController protected + def sign_in_params + devise_parameter_sanitizer.for(:sign_in) + end + def serialize_options(resource) methods = resource_class.authentication_keys.dup methods = methods.keys if methods.is_a?(Hash) diff --git a/app/controllers/devise/unlocks_controller.rb b/app/controllers/devise/unlocks_controller.rb index 45f6b2c1..a8c751ed 100644 --- a/app/controllers/devise/unlocks_controller.rb +++ b/app/controllers/devise/unlocks_controller.rb @@ -3,7 +3,7 @@ class Devise::UnlocksController < DeviseController # GET /resource/unlock/new def new - build_resource({}) + self.resource = resource_class.new end # POST /resource/unlock diff --git a/app/controllers/devise_controller.rb b/app/controllers/devise_controller.rb index 94359769..6799d5ac 100644 --- a/app/controllers/devise_controller.rb +++ b/app/controllers/devise_controller.rb @@ -28,10 +28,6 @@ class DeviseController < Devise.parent_controller.constantize devise_mapping.to end - def resource_params - params[resource_name] - end - # Returns a signed in resource from session (if one exists) def signed_in_resource warden.authenticate(:scope => resource_name) @@ -93,23 +89,6 @@ MESSAGE instance_variable_set(:"@#{resource_name}", new_resource) end - # Build a devise resource. - # Assignment bypasses attribute protection when :unsafe option is passed - def build_resource(hash = nil, options = {}) - hash ||= resource_params || {} - - if options[:unsafe] - self.resource = resource_class.new.tap do |resource| - hash.each do |key, value| - setter = :"#{key}=" - resource.send(setter, value) if resource.respond_to?(setter) - end - end - else - self.resource = resource_class.new(hash) - end - end - # Helper for use in before_filters where no authentication is required. # # Example: @@ -181,4 +160,8 @@ MESSAGE format.any(*navigational_formats, &block) end end + + def resource_params + params.fetch(resource_name, {}) + end end diff --git a/lib/devise.rb b/lib/devise.rb index 87e1f307..4e103436 100644 --- a/lib/devise.rb +++ b/lib/devise.rb @@ -6,12 +6,14 @@ require 'set' require 'securerandom' module Devise - autoload :Delegator, 'devise/delegator' - autoload :FailureApp, 'devise/failure_app' - autoload :OmniAuth, 'devise/omniauth' - autoload :ParamFilter, 'devise/param_filter' - autoload :TestHelpers, 'devise/test_helpers' - autoload :TimeInflector, 'devise/time_inflector' + autoload :Delegator, 'devise/delegator' + autoload :FailureApp, 'devise/failure_app' + autoload :OmniAuth, 'devise/omniauth' + autoload :ParamFilter, 'devise/param_filter' + autoload :BaseSanitizer, 'devise/parameter_sanitizer' + autoload :ParameterSanitizer, 'devise/parameter_sanitizer' + autoload :TestHelpers, 'devise/test_helpers' + autoload :TimeInflector, 'devise/time_inflector' module Controllers autoload :Helpers, 'devise/controllers/helpers' diff --git a/lib/devise/controllers/helpers.rb b/lib/devise/controllers/helpers.rb index a83a77d0..991f7ab5 100644 --- a/lib/devise/controllers/helpers.rb +++ b/lib/devise/controllers/helpers.rb @@ -80,6 +80,17 @@ module Devise is_a?(DeviseController) end + # Setup a param sanitizer to filter parameters using strong_parameters. See + # lib/devise/controllers/parameter_sanitizer.rb for more info. Override this + # method in your application controller to use your own parameter sanitizer. + def devise_parameter_sanitizer + @devise_parameter_sanitizer ||= if defined?(ActionController::StrongParameters) + Devise::ParameterSanitizer.new(resource_class, resource_name, params) + else + Devise::BaseSanitizer.new(resource_class, resource_name, params) + end + end + # Tell warden that params authentication is allowed for that specific page. def allow_params_authentication! request.env["devise.allow_params_authentication"] = true diff --git a/lib/devise/parameter_sanitizer.rb b/lib/devise/parameter_sanitizer.rb new file mode 100644 index 00000000..6ce00211 --- /dev/null +++ b/lib/devise/parameter_sanitizer.rb @@ -0,0 +1,59 @@ +module Devise + class BaseSanitizer + attr_reader :params, :resource_name, :resource_class + + def initialize(resource_class, resource_name, params) + @resource_class = resource_class + @resource_name = resource_name + @params = params + @blocks = Hash.new + end + + def for(kind, &block) + if block_given? + @blocks[kind] = block + else + block = @blocks[kind] + block ? block.call(default_params) : fallback_for(kind) + end + end + + private + + def fallback_for(kind) + default_params + end + + def default_params + params.fetch(resource_name, {}) + end + end + + class ParameterSanitizer < BaseSanitizer + private + + def fallback_for(kind) + if respond_to?(kind, true) + send(kind) + else + raise NotImplementedError, "Devise Parameter Sanitizer doesn't know how to sanitize parameters for #{kind}" + end + end + + def sign_in + default_params.permit(auth_keys) + end + + def sign_up + default_params.permit(auth_keys + [:password, :password_confirmation]) + end + + def account_update + default_params.permit(auth_keys + [:password, :password_confirmation, :current_password]) + end + + def auth_keys + resource_class.authentication_keys + end + end +end diff --git a/lib/generators/active_record/devise_generator.rb b/lib/generators/active_record/devise_generator.rb index 35541d91..f566cd15 100644 --- a/lib/generators/active_record/devise_generator.rb +++ b/lib/generators/active_record/devise_generator.rb @@ -22,10 +22,7 @@ module ActiveRecord end def inject_devise_content - content = model_contents + < 'Shirley Templar'} - @controller.stubs(:params).returns(HashWithIndifferentAccess.new({'user' => user_params})) + user_params = {'email' => 'shirley@templar.com'} + @controller.stubs(:params).returns(ActionController::Parameters.new({'user' => user_params})) + # Stub controller name so strong parameters can filter properly. + # DeviseController does not allow any parameters by default. + @controller.stubs(:controller_name).returns(:sessions_controller) - assert_equal user_params, @controller.resource_params + assert_equal user_params, @controller.send(:resource_params) end test 'resources methods are not controller actions' do diff --git a/test/generators/active_record_generator_test.rb b/test/generators/active_record_generator_test.rb index f4160034..73971967 100644 --- a/test/generators/active_record_generator_test.rb +++ b/test/generators/active_record_generator_test.rb @@ -10,13 +10,11 @@ if DEVISE_ORM == :active_record test "all files are properly created with rails31 migration syntax" do run_generator %w(monster) - assert_file "app/models/monster.rb", /devise/, /attr_accessible (:[a-z_]+(, )?)+/ assert_migration "db/migrate/devise_create_monsters.rb", /def change/ end test "all files for namespaced model are properly created" do run_generator %w(admin/monster) - assert_file "app/models/admin/monster.rb", /devise/, /attr_accessible (:[a-z_]+(, )?)+/ assert_migration "db/migrate/devise_create_admin_monsters.rb", /def change/ end @@ -68,7 +66,7 @@ if DEVISE_ORM == :active_record simulate_inside_engine(RailsEngine::Engine, RailsEngine) do run_generator ["monster"] - assert_file "app/models/rails_engine/monster.rb", /devise/,/attr_accessible (:[a-z_]+(, )?)+/ + assert_file "app/models/rails_engine/monster.rb", /devise/ end end end diff --git a/test/models/database_authenticatable_test.rb b/test/models/database_authenticatable_test.rb index 1439a51f..a8577c22 100644 --- a/test/models/database_authenticatable_test.rb +++ b/test/models/database_authenticatable_test.rb @@ -111,13 +111,6 @@ class DatabaseAuthenticatableTest < ActiveSupport::TestCase assert user.reload.valid_password?('pass4321') end - test 'should update password with valid current password and :as option' do - user = create_user - assert user.update_with_password(:current_password => '12345678', - :password => 'pass4321', :password_confirmation => 'pass4321', :as => :admin) - assert user.reload.valid_password?('pass4321') - end - test 'should add an error to current password when it is invalid' do user = create_user assert_not user.update_with_password(:current_password => 'other', @@ -170,12 +163,6 @@ class DatabaseAuthenticatableTest < ActiveSupport::TestCase assert_equal 'new@example.com', user.email end - test 'should update the user without password with :as option' do - user = create_user - user.update_without_password(:email => 'new@example.com', :as => :admin) - assert_equal 'new@example.com', user.email - end - test 'should not update password without password' do user = create_user user.update_without_password(:password => 'pass4321', :password_confirmation => 'pass4321') diff --git a/test/parameter_sanitizer_test.rb b/test/parameter_sanitizer_test.rb new file mode 100644 index 00000000..5043dd06 --- /dev/null +++ b/test/parameter_sanitizer_test.rb @@ -0,0 +1,51 @@ +require 'test_helper' +require 'devise/parameter_sanitizer' + +class BaseSanitizerTest < ActiveSupport::TestCase + def sanitizer + Devise::BaseSanitizer.new(User, :user, { user: { "email" => "jose" } }) + end + + test 'returns chosen params' do + assert_equal({ "email" => "jose" }, sanitizer.for(:sign_in)) + end +end + +if defined?(ActionController::StrongParameters) + require 'active_model/forbidden_attributes_protection' + + class ParameterSanitizerTest < ActiveSupport::TestCase + def sanitizer(params) + params = ActionController::Parameters.new(params) + Devise::ParameterSanitizer.new(User, :user, params) + end + + test 'filters some parameters on sign in by default' do + sanitizer = sanitizer(user: { "email" => "jose", "password" => "invalid" }) + assert_equal({ "email" => "jose" }, sanitizer.for(:sign_in)) + end + + test 'filters some parameters on sign up by default' do + sanitizer = sanitizer(user: { "email" => "jose", "role" => "invalid" }) + assert_equal({ "email" => "jose" }, sanitizer.for(:sign_up)) + end + + test 'filters some parameters on account update by default' do + sanitizer = sanitizer(user: { "email" => "jose", "role" => "invalid" }) + assert_equal({ "email" => "jose" }, sanitizer.for(:account_update)) + end + + test 'allows custom hooks' do + sanitizer = sanitizer(user: { "email" => "jose", "password" => "invalid" }) + sanitizer.for(:sign_in) { |user| user.permit(:email, :password) } + assert_equal({ "email" => "jose", "password" => "invalid" }, sanitizer.for(:sign_in)) + end + + test 'raises on unknown hooks' do + sanitizer = sanitizer(user: { "email" => "jose", "password" => "invalid" }) + assert_raise NotImplementedError do + sanitizer.for(:unknown) + end + end + end +end diff --git a/test/rails_app/app/mongoid/shim.rb b/test/rails_app/app/mongoid/shim.rb index 602d059c..f74e8711 100644 --- a/test/rails_app/app/mongoid/shim.rb +++ b/test/rails_app/app/mongoid/shim.rb @@ -2,7 +2,6 @@ module Shim extend ::ActiveSupport::Concern included do - include ::ActiveModel::MassAssignmentSecurity include ::Mongoid::Timestamps field :created_at, :type => DateTime end diff --git a/test/rails_app/config/application.rb b/test/rails_app/config/application.rb index 9f942f9b..5744dae7 100644 --- a/test/rails_app/config/application.rb +++ b/test/rails_app/config/application.rb @@ -32,11 +32,6 @@ module RailsApp config.action_mailer.default_url_options = { :host => "localhost:3000" } - if DEVISE_ORM == :active_record - # Disable forcing whitelist attributes from protected attributes. - config.active_record.whitelist_attributes = false - end - # This was used to break devise in some situations config.to_prepare do Devise::SessionsController.layout "application" diff --git a/test/rails_app/lib/shared_user.rb b/test/rails_app/lib/shared_user.rb index 29c26231..e4bd8712 100644 --- a/test/rails_app/lib/shared_user.rb +++ b/test/rails_app/lib/shared_user.rb @@ -7,7 +7,6 @@ module SharedUser :trackable, :validatable, :omniauthable attr_accessor :other_key - attr_accessible :username, :email, :password, :password_confirmation, :remember_me, :confirmation_sent_at # They need to be included after Devise is called. extend ExtendMethods diff --git a/test/test_models.rb b/test/test_models.rb index fb65d53c..cd7fbaa3 100644 --- a/test/test_models.rb +++ b/test/test_models.rb @@ -15,7 +15,6 @@ end class UserWithVirtualAttributes < User devise :case_insensitive_keys => [ :email, :email_confirmation ] validates :email, :presence => true, :confirmation => {:on => :create} - attr_accessible :email, :email_confirmation end class Several < Admin