From 10003db4e43eb823db6b743619fe71e34d6999b5 Mon Sep 17 00:00:00 2001 From: Derek Prior Date: Fri, 29 Apr 2016 09:15:46 -0400 Subject: [PATCH 01/13] Start Clearance 2.0 development --- lib/clearance/version.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/clearance/version.rb b/lib/clearance/version.rb index 1e1c052be..808424c63 100644 --- a/lib/clearance/version.rb +++ b/lib/clearance/version.rb @@ -1,3 +1,3 @@ module Clearance - VERSION = "1.14.1".freeze + VERSION = "2.0.0.alpha".freeze end From b10a63fac559efa08453079642206b98e930f45e Mon Sep 17 00:00:00 2001 From: Derek Prior Date: Fri, 29 Apr 2016 09:28:30 -0400 Subject: [PATCH 02/13] Stop supporting older versions of Ruby and Rails - Remove support for Ruby 1.9, 2.0, and 2.1 - Remove support for Rails 3.1, 3.2, 4.0, and 4.1 --- .travis.yml | 23 ++--------------------- Appraisals | 28 ++++------------------------ Gemfile.lock | 4 ++-- NEWS.md | 8 ++++++++ clearance.gemspec | 4 ++-- gemfiles/rails32.gemfile | 18 ------------------ gemfiles/rails40.gemfile | 20 -------------------- gemfiles/rails41.gemfile | 19 ------------------- gemfiles/rails42.gemfile | 1 - 9 files changed, 18 insertions(+), 107 deletions(-) delete mode 100644 gemfiles/rails32.gemfile delete mode 100644 gemfiles/rails40.gemfile delete mode 100644 gemfiles/rails41.gemfile diff --git a/.travis.yml b/.travis.yml index b5f0baaa2..8a60a1a2d 100644 --- a/.travis.yml +++ b/.travis.yml @@ -4,32 +4,13 @@ language: - ruby rvm: - - 1.9.3 - - 2.0.0 - - 2.1.8 - - 2.2.4 - - 2.3.0 + - 2.2.5 + - 2.3.1 gemfile: - - gemfiles/rails32.gemfile - - gemfiles/rails40.gemfile - - gemfiles/rails41.gemfile - gemfiles/rails42.gemfile - gemfiles/rails50.gemfile -matrix: - exclude: - - rvm: 1.9.3 - gemfile: gemfiles/rails50.gemfile - - rvm: 2.0.0 - gemfile: gemfiles/rails50.gemfile - - rvm: 2.1.8 - gemfile: gemfiles/rails50.gemfile - - rvm: 2.2.4 - gemfile: gemfiles/rails32.gemfile - - rvm: 2.3.0 - gemfile: gemfiles/rails32.gemfile - install: - "bin/setup" diff --git a/Appraisals b/Appraisals index 3e85e1ec1..6135abe01 100644 --- a/Appraisals +++ b/Appraisals @@ -1,29 +1,9 @@ -if RUBY_VERSION < "2.2.0" - appraise 'rails32' do - gem 'rails', '~> 3.2.21' - end -end - -appraise 'rails40' do - gem 'rails', '~> 4.0.13' - gem 'test-unit' - gem 'mime-types', '~> 2.99' -end - -appraise 'rails41' do - gem 'rails', '~> 4.1.9' - gem 'mime-types', '~> 2.99' -end - appraise 'rails42' do gem 'rails', '~> 4.2.0' - gem 'mime-types', '~> 2.99' end -if RUBY_VERSION >= "2.2.0" - appraise "rails50" do - gem "rails", "~> 5.0.0.beta3" - gem "rails-controller-testing" - gem "rspec-rails", "~> 3.5.0.beta1" - end +appraise "rails50" do + gem "rails", "~> 5.0.0.beta3" + gem "rails-controller-testing" + gem "rspec-rails", "~> 3.5.0.beta1" end diff --git a/Gemfile.lock b/Gemfile.lock index 598a893cb..1d534e85a 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,10 +1,10 @@ PATH remote: . specs: - clearance (1.14.1) + clearance (2.0.0.alpha) bcrypt email_validator (~> 1.4) - rails (>= 3.1) + rails (>= 4.2) GEM remote: https://rubygems.org/ diff --git a/NEWS.md b/NEWS.md index e8cc34c9c..d674fc76f 100644 --- a/NEWS.md +++ b/NEWS.md @@ -3,6 +3,14 @@ The noteworthy changes for each Clearance version are included here. For a complete changelog, see the git history for each version via the version links. +## [2.0.0] - Unreleased + +### Removed +- Removed support for Ruby versions older than 2.2 +- Removed support for Rails versions older than 4.2 + +[2.0.0]: https://github.com/thoughtbot/clearance/compare/v1.14.1...2.0 + ## [1.14.1] - May 12, 2016 ### Fixed diff --git a/clearance.gemspec b/clearance.gemspec index a842493ac..8b9c5c742 100644 --- a/clearance.gemspec +++ b/clearance.gemspec @@ -5,7 +5,7 @@ require 'date' Gem::Specification.new do |s| s.add_dependency 'bcrypt' s.add_dependency 'email_validator', '~> 1.4' - s.add_dependency 'rails', '>= 3.1' + s.add_dependency 'rails', '>= 4.2' s.authors = [ 'Dan Croak', 'Eugene Bolshakov', @@ -35,7 +35,7 @@ Gem::Specification.new do |s| s.name = %q{clearance} s.rdoc_options = ['--charset=UTF-8'] s.require_paths = ['lib'] - s.required_ruby_version = Gem::Requirement.new('>= 1.9.2') + s.required_ruby_version = Gem::Requirement.new('>= 2.2.0') s.summary = 'Rails authentication & authorization with email & password.' s.test_files = `git ls-files -- {spec}/*`.split("\n") s.version = Clearance::VERSION diff --git a/gemfiles/rails32.gemfile b/gemfiles/rails32.gemfile deleted file mode 100644 index 6c4115354..000000000 --- a/gemfiles/rails32.gemfile +++ /dev/null @@ -1,18 +0,0 @@ -# This file was generated by Appraisal - -source "https://rubygems.org" - -gem "appraisal", "~> 1.0" -gem "ammeter" -gem "bundler", "~> 1.3" -gem "capybara", ">= 2.6.2" -gem "database_cleaner", "~> 1.0" -gem "factory_girl_rails", "~> 4.2" -gem "rspec-rails", "~> 3.1" -gem "shoulda-matchers", "~> 2.8" -gem "sqlite3", "~> 1.3" -gem "timecop", "~> 0.6" -gem "pry", :require => false -gem "rails", "~> 3.2.21" - -gemspec :path => "../" diff --git a/gemfiles/rails40.gemfile b/gemfiles/rails40.gemfile deleted file mode 100644 index 6468184b5..000000000 --- a/gemfiles/rails40.gemfile +++ /dev/null @@ -1,20 +0,0 @@ -# This file was generated by Appraisal - -source "https://rubygems.org" - -gem "appraisal" -gem "ammeter" -gem "bundler", "~> 1.3" -gem "capybara", ">= 2.6.2" -gem "database_cleaner", "~> 1.0" -gem "factory_girl_rails", "~> 4.2" -gem "rspec-rails", "~> 3.1" -gem "shoulda-matchers", "~> 2.8" -gem "sqlite3", "~> 1.3" -gem "timecop", "~> 0.6" -gem "pry", :require => false -gem "rails", "~> 4.0.13" -gem "test-unit" -gem "mime-types", "~> 2.99" - -gemspec :path => "../" diff --git a/gemfiles/rails41.gemfile b/gemfiles/rails41.gemfile deleted file mode 100644 index e30f73d8b..000000000 --- a/gemfiles/rails41.gemfile +++ /dev/null @@ -1,19 +0,0 @@ -# This file was generated by Appraisal - -source "https://rubygems.org" - -gem "appraisal" -gem "ammeter" -gem "bundler", "~> 1.3" -gem "capybara", ">= 2.6.2" -gem "database_cleaner", "~> 1.0" -gem "factory_girl_rails", "~> 4.2" -gem "rspec-rails", "~> 3.1" -gem "shoulda-matchers", "~> 2.8" -gem "sqlite3", "~> 1.3" -gem "timecop", "~> 0.6" -gem "pry", :require => false -gem "rails", "~> 4.1.9" -gem "mime-types", "~> 2.99" - -gemspec :path => "../" diff --git a/gemfiles/rails42.gemfile b/gemfiles/rails42.gemfile index 8b8a99ac6..043c9ed34 100644 --- a/gemfiles/rails42.gemfile +++ b/gemfiles/rails42.gemfile @@ -14,6 +14,5 @@ gem "sqlite3", "~> 1.3" gem "timecop", "~> 0.6" gem "pry", :require => false gem "rails", "~> 4.2.0" -gem "mime-types", "~> 2.99" gemspec :path => "../" From 6dd080f9a61c5ea8c35cb4fed00d580620244a48 Mon Sep 17 00:00:00 2001 From: Derek Prior Date: Fri, 29 Apr 2016 09:44:50 -0400 Subject: [PATCH 03/13] Remove all previously deprecated code --- NEWS.md | 1 + app/controllers/clearance/users_controller.rb | 8 -- lib/clearance.rb | 8 -- lib/clearance/authentication.rb | 8 -- lib/clearance/authorization.rb | 11 +- .../bcrypt_migration_from_sha1.rb | 77 ----------- lib/clearance/password_strategies/blowfish.rb | 61 --------- lib/clearance/password_strategies/sha1.rb | 59 --------- lib/clearance/session.rb | 10 +- lib/clearance/testing.rb | 11 -- lib/clearance/testing/helpers.rb | 15 --- lib/clearance/user.rb | 21 --- spec/clearance/session_spec.rb | 31 ----- .../bcrypt_migration_from_sha1_spec.rb | 122 ------------------ spec/password_strategies/blowfish_spec.rb | 61 --------- spec/password_strategies/sha1_spec.rb | 59 --------- 16 files changed, 3 insertions(+), 560 deletions(-) delete mode 100644 lib/clearance/password_strategies/bcrypt_migration_from_sha1.rb delete mode 100644 lib/clearance/password_strategies/blowfish.rb delete mode 100644 lib/clearance/password_strategies/sha1.rb delete mode 100644 lib/clearance/testing.rb delete mode 100644 lib/clearance/testing/helpers.rb delete mode 100644 spec/password_strategies/bcrypt_migration_from_sha1_spec.rb delete mode 100644 spec/password_strategies/blowfish_spec.rb delete mode 100644 spec/password_strategies/sha1_spec.rb diff --git a/NEWS.md b/NEWS.md index d674fc76f..06e7c2c57 100644 --- a/NEWS.md +++ b/NEWS.md @@ -8,6 +8,7 @@ complete changelog, see the git history for each version via the version links. ### Removed - Removed support for Ruby versions older than 2.2 - Removed support for Rails versions older than 4.2 +- Removed all deprecated code from Clearance 1.x [2.0.0]: https://github.com/thoughtbot/clearance/compare/v1.14.1...2.0 diff --git a/app/controllers/clearance/users_controller.rb b/app/controllers/clearance/users_controller.rb index 72be00a26..82db7569b 100644 --- a/app/controllers/clearance/users_controller.rb +++ b/app/controllers/clearance/users_controller.rb @@ -27,14 +27,6 @@ def create private - def avoid_sign_in - warn "[DEPRECATION] Clearance's `avoid_sign_in` before_filter is " + - "deprecated. Use `redirect_signed_in_users` instead. " + - "Be sure to update any instances of `skip_before_filter :avoid_sign_in`" + - " or `skip_before_action :avoid_sign_in` as well" - redirect_signed_in_users - end - def redirect_signed_in_users if signed_in? redirect_to Clearance.configuration.redirect_url diff --git a/lib/clearance.rb b/lib/clearance.rb index c72680443..1b92beebe 100644 --- a/lib/clearance.rb +++ b/lib/clearance.rb @@ -10,12 +10,4 @@ require 'clearance/constraints' module Clearance - # @deprecated Use `Gem::Specification` API if you need to access Clearance's - # Gem root. - def self.root - warn "#{Kernel.caller.first}: [DEPRECATION] `Clearance.root` is " + - "deprecated and will be removed in the next major release. If you need " + - "to find Clearance's root, you can use the `Gem::Specification` API." - File.expand_path('../..', __FILE__) - end end diff --git a/lib/clearance/authentication.rb b/lib/clearance/authentication.rb index 4541db7de..2027fb869 100644 --- a/lib/clearance/authentication.rb +++ b/lib/clearance/authentication.rb @@ -7,7 +7,6 @@ module Authentication private( :authenticate, :current_user, - :current_user=, :handle_unverified_request, :sign_in, :sign_out, @@ -37,13 +36,6 @@ def current_user clearance_session.current_user end - # @deprecated Use the {#sign_in} method instead. - def current_user=(user) - warn "#{Kernel.caller.first}: [DEPRECATION] " + - 'Assigning the current_user has been deprecated. Use the sign_in method instead.' - clearance_session.sign_in user - end - # Sign in the provided user. # @param [User] user # diff --git a/lib/clearance/authorization.rb b/lib/clearance/authorization.rb index 8628b8408..15aca029c 100644 --- a/lib/clearance/authorization.rb +++ b/lib/clearance/authorization.rb @@ -3,7 +3,7 @@ module Authorization extend ActiveSupport::Concern included do - private :authorize, :deny_access, :require_login + private :deny_access, :require_login end # Use as a `before_action` to require a user be signed in to proceed. @@ -23,15 +23,6 @@ def require_login end end - # @deprecated use {#require_login} - def authorize - warn "[DEPRECATION] Clearance's `authorize` before_action is " + - "deprecated. Use `require_login` instead. Be sure to update any " + - "instances of `skip_before_action :authorize` or " + - "`skip_before_action :authorize` as well" - require_login - end - # Responds to unauthorized requests in a manner fitting the request format. # `js`, `json`, and `xml` requests will receive a 401 with no body. All # other formats will be redirected appropriately and can optionally have the diff --git a/lib/clearance/password_strategies/bcrypt_migration_from_sha1.rb b/lib/clearance/password_strategies/bcrypt_migration_from_sha1.rb deleted file mode 100644 index f4c718ca1..000000000 --- a/lib/clearance/password_strategies/bcrypt_migration_from_sha1.rb +++ /dev/null @@ -1,77 +0,0 @@ -module Clearance - module PasswordStrategies - # @deprecated Use {BCrypt} or `clearance-deprecated_password_strategies` gem - module BCryptMigrationFromSHA1 - DEPRECATION_MESSAGE = "[DEPRECATION] The BCryptMigrationFromSha1 " \ - "password strategy has been deprecated and will be removed from " \ - "Clearance 2.0. BCrypt is the only officially supported strategy, " \ - "though you are free to provide your own. To continue using this " \ - "strategy, add clearance-deprecated_password_strategies to your " \ - "Gemfile." - - # @api private - class BCryptUser - include Clearance::PasswordStrategies::BCrypt - - def initialize(user) - @user = user - end - - delegate :encrypted_password, :encrypted_password=, to: :@user - end - - # @api private - class SHA1User - include Clearance::PasswordStrategies::SHA1 - - def initialize(user) - @user = user - end - - delegate :salt, :salt=, :encrypted_password, :encrypted_password=, to: :@user - end - - # @deprecated Use {BCrypt} or `clearance-deprecated_password_strategies` - # gem - def authenticated?(password) - warn "#{Kernel.caller.first}: #{DEPRECATION_MESSAGE}" - authenticated_with_sha1?(password) || authenticated_with_bcrypt?(password) - end - - # @deprecated Use {BCrypt} or `clearance-deprecated_password_strategies` - # gem - def password=(new_password) - warn "#{Kernel.caller.first}: #{DEPRECATION_MESSAGE}" - @password = new_password - BCryptUser.new(self).password = new_password - end - - private - - # @api private - def authenticated_with_bcrypt?(password) - begin - BCryptUser.new(self).authenticated? password - rescue ::BCrypt::Errors::InvalidHash - false - end - end - - # @api private - def authenticated_with_sha1?(password) - if sha1_password? - if SHA1User.new(self).authenticated? password - self.password = password - self.save - true - end - end - end - - # @api private - def sha1_password? - self.encrypted_password =~ /^[a-f0-9]{40}$/ - end - end - end -end diff --git a/lib/clearance/password_strategies/blowfish.rb b/lib/clearance/password_strategies/blowfish.rb deleted file mode 100644 index 9dbc8dabc..000000000 --- a/lib/clearance/password_strategies/blowfish.rb +++ /dev/null @@ -1,61 +0,0 @@ -require 'openssl' -require 'base64' - -module Clearance - module PasswordStrategies - # @deprecated Use {BCrypt} or `clearance-deprecated_password_strategies` gem - module Blowfish - DEPRECATION_MESSAGE = "[DEPRECATION] The Blowfish password strategy " \ - "has been deprecated and will be removed from Clearance 2.0. BCrypt " \ - "is the only officially supported strategy, though you are free to " \ - "provide your own. To continue using this strategy add " \ - "clearance-deprecated_password_strategies to your Gemfile." - - # @deprecated Use {BCrypt} or `clearance-deprecated_password_strategies` - # gem - def authenticated?(password) - warn "#{Kernel.caller.first}: #{DEPRECATION_MESSAGE}" - encrypted_password == encrypt(password) - end - - # @deprecated Use {BCrypt} or `clearance-deprecated_password_strategies` - # gem - def password=(new_password) - warn "#{Kernel.caller.first}: #{DEPRECATION_MESSAGE}" - @password = new_password - initialize_salt_if_necessary - - if new_password.present? - self.encrypted_password = encrypt(new_password) - end - end - - protected - - # @api private - def encrypt(string) - generate_hash("--#{salt}--#{string}--") - end - - # @api private - def generate_hash(string) - cipher = OpenSSL::Cipher::Cipher.new('bf-cbc').encrypt - cipher.key = Digest::SHA256.digest(salt) - hash = cipher.update(string) << cipher.final - Base64.encode64(hash).encode('utf-8') - end - - # @api private - def initialize_salt_if_necessary - if salt.blank? - self.salt = generate_salt - end - end - - # @api private - def generate_salt - Base64.encode64(SecureRandom.hex(20)).encode('utf-8') - end - end - end -end diff --git a/lib/clearance/password_strategies/sha1.rb b/lib/clearance/password_strategies/sha1.rb deleted file mode 100644 index 7a8564bb8..000000000 --- a/lib/clearance/password_strategies/sha1.rb +++ /dev/null @@ -1,59 +0,0 @@ -module Clearance - module PasswordStrategies - # @deprecated Use {BCrypt} or `clearance-deprecated_password_strategies` gem - module SHA1 - require 'digest/sha1' - - DEPRECATION_MESSAGE = "[DEPRECATION] The SHA1 password strategy " \ - "has been deprecated and will be removed from Clearance 2.0. BCrypt " \ - "is the only officially supported strategy, though you are free to " \ - "provide your own. To continue using this strategy add " \ - "clearance-deprecated_password_strategies to your Gemfile." - - extend ActiveSupport::Concern - - # @deprecated Use {BCrypt} or `clearance-deprecated_password_strategies` - # gem - def authenticated?(password) - warn "#{Kernel.caller.first}: #{DEPRECATION_MESSAGE}" - encrypted_password == encrypt(password) - end - - # @deprecated Use {BCrypt} or `clearance-deprecated_password_strategies` - # gem - def password=(new_password) - warn "#{Kernel.caller.first}: #{DEPRECATION_MESSAGE}" - @password = new_password - initialize_salt_if_necessary - - if new_password.present? - self.encrypted_password = encrypt(new_password) - end - end - - private - - # @api private - def encrypt(string) - generate_hash "--#{salt}--#{string}--" - end - - # @api private - def generate_hash(string) - Digest::SHA1.hexdigest(string).encode 'UTF-8' - end - - # @api private - def initialize_salt_if_necessary - if salt.blank? - self.salt = generate_salt - end - end - - # @api private - def generate_salt - SecureRandom.hex(20).encode('UTF-8') - end - end - end -end diff --git a/lib/clearance/session.rb b/lib/clearance/session.rb index ba77ea81a..87aef3380 100644 --- a/lib/clearance/session.rb +++ b/lib/clearance/session.rb @@ -108,15 +108,7 @@ def remember_token # @api private def remember_token_expires - if expires_configuration.arity == 1 - expires_configuration.call(cookies) - else - warn "#{Kernel.caller.first}: [DEPRECATION] " + - 'Clearance.configuration.cookie_expiration lambda with no parameters ' + - 'has been deprecated and will be removed from a future release. The ' + - 'lambda should accept the collection of previously set cookies.' - expires_configuration.call - end + expires_configuration.call(cookies) end # @api private diff --git a/lib/clearance/testing.rb b/lib/clearance/testing.rb deleted file mode 100644 index 93ef3f6c6..000000000 --- a/lib/clearance/testing.rb +++ /dev/null @@ -1,11 +0,0 @@ -if defined?(RSpec) - require 'clearance/rspec' -elsif defined?(ActionController::TestCase) - require 'clearance/test_unit' -end - -warn( - "#{Kernel.caller.first} [DEPRECATION] Requiring `clearance/testing` in " + - '`spec/spec_helper.rb` (or in `test/test_helper.rb`) is deprecated. ' + - 'Require `clearance/rspec` or `clearance/test_unit` instead.' -) diff --git a/lib/clearance/testing/helpers.rb b/lib/clearance/testing/helpers.rb deleted file mode 100644 index 719d2270a..000000000 --- a/lib/clearance/testing/helpers.rb +++ /dev/null @@ -1,15 +0,0 @@ -require "clerance/testing/controller_helpers" - -module Clearance - module Testing - # @deprecated Use Clearance::Testing::ControllerHelpers - module Helpers - warn( - "#{Kernel.caller.first} [DEPRECATION] Clearance::Testing::Helpers is "\ - "deprecated and has been replaced with " \ - "Clearance::Testing::ControllerHelpers. Require " \ - "clearance/testing/controller_helpers instead." - ) - end - end -end diff --git a/lib/clearance/user.rb b/lib/clearance/user.rb index a51ea27dc..fbf6a6e88 100644 --- a/lib/clearance/user.rb +++ b/lib/clearance/user.rb @@ -46,9 +46,6 @@ module Clearance # @return [String] The value used to identify this user in the password # reset link. # - # @!attribute password_changing - # @deprecated Dirty tracking is now handled automatically. - # # @!attribute [r] password # @return [String] Transient (non-persisted) attribute that is set when # updating a user's password. Only the {#encrypted_password} is persisted. @@ -110,24 +107,6 @@ def password=(value) encrypted_password_will_change! super end - - def password_changing - warn "#{Kernel.caller.first}: [DEPRECATION] " \ - "The `password_changing` attribute is deprecated. Clearance uses " \ - "the dirty state of the `encrypted_password` field to track this " \ - "automatically." - - @password_changing - end - - def password_changing=(value) - warn "#{Kernel.caller.first}: [DEPRECATION] " \ - "The `password_changing` attribute is deprecated. Clearance uses " \ - "the dirty state of the `encrypted_password` field to track this " \ - "automatically." - - @password_changing = value - end end # @api private diff --git a/spec/clearance/session_spec.rb b/spec/clearance/session_spec.rb index d77d32ec4..fca85e466 100644 --- a/spec/clearance/session_spec.rb +++ b/spec/clearance/session_spec.rb @@ -113,7 +113,6 @@ def stub_callable expect(session.current_user).to be_nil end - def stub_sign_in_guard(options) session_status = stub_status(options.fetch(:succeed)) @@ -191,36 +190,6 @@ def stub_status(success) end end - context 'configured with lambda taking no arguments' do - it 'logs a deprecation warning' do - expiration = -> { Time.now } - with_custom_expiration expiration do - session = Clearance::Session.new(env_without_remember_token) - allow(session).to receive(:warn) - session.add_cookie_to_headers headers - - expect(session).to have_received(:warn).once - end - end - - it 'is set to the value of the evaluated lambda' do - expires_at = -> { 1.day.from_now } - with_custom_expiration expires_at do - user = double("User", remember_token: "123abc") - headers = {} - session = Clearance::Session.new(env_without_remember_token) - session.sign_in user - allow(session).to receive(:warn) - session.add_cookie_to_headers headers - - expect(headers).to set_cookie( - 'remember_token', - user.remember_token, expires_at.call - ) - end - end - end - context 'configured with lambda taking one argument' do it 'it can use other cookies to set the value of the expires token' do remembered_expires = 12.hours.from_now diff --git a/spec/password_strategies/bcrypt_migration_from_sha1_spec.rb b/spec/password_strategies/bcrypt_migration_from_sha1_spec.rb deleted file mode 100644 index 652708a2f..000000000 --- a/spec/password_strategies/bcrypt_migration_from_sha1_spec.rb +++ /dev/null @@ -1,122 +0,0 @@ -require "spec_helper" -include FakeModelWithPasswordStrategy - -describe Clearance::PasswordStrategies::BCryptMigrationFromSHA1 do - around do |example| - silence_warnings do - example.run - end - end - - describe "#password=" do - it "encrypts the password into a BCrypt-encrypted encrypted_password" do - stub_bcrypt_password - - expect(model_instance.encrypted_password).to eq encrypted_password - end - - it "encrypts with BCrypt" do - stub_bcrypt_password - - expect(BCrypt::Password).to have_received(:create). - with(password, anything) - end - - it "sets the pasword on the subject" do - stub_bcrypt_password - - expect(model_instance.password).to be_present - end - - def stub_bcrypt_password - model_instance.salt = salt - digestable = "--#{salt}--#{password}--" - model_instance.encrypted_password = Digest::SHA1.hexdigest(digestable) - allow(BCrypt::Password).to receive(:create).and_return(encrypted_password) - model_instance.password = password - end - - def encrypted_password - @encrypted_password ||= double("encrypted password") - end - end - - describe "#authenticated?" do - context "with a SHA1-encrypted password" do - it "is authenticated" do - model_instance.salt = salt - model_instance.encrypted_password = sha1_hash - allow(model_instance).to receive(:save) - - expect(model_instance).to be_authenticated(password) - end - - it "changes the hash into a BCrypt-encrypted one" do - model_instance.salt = salt - model_instance.encrypted_password = sha1_hash - allow(model_instance).to receive(:save) - - model_instance.authenticated? password - - expect(model_instance.encrypted_password).not_to eq sha1_hash - end - - it "does not raise a BCrypt error for invalid passwords" do - model_instance.salt = salt - model_instance.encrypted_password = sha1_hash - - expect do - model_instance.authenticated? "bad" + password - end.not_to raise_error - end - - it "saves the subject to database" do - model_instance.salt = salt - model_instance.encrypted_password = sha1_hash - allow(model_instance).to receive(:save) - - model_instance.authenticated? password - - expect(model_instance).to have_received(:save) - end - - def sha1_hash - Digest::SHA1.hexdigest("--#{salt}--#{password}--") - end - end - - context "with a BCrypt-encrypted password" do - it "is authenticated" do - model_instance.encrypted_password = bcrypt_hash - - expect(model_instance).to be_authenticated(password) - end - - it "does not change the hash" do - model_instance.encrypted_password = bcrypt_hash - - model_instance.authenticated? password - - expect(model_instance.encrypted_password.to_s).to eq bcrypt_hash.to_s - end - - def bcrypt_hash - @bcrypt_hash ||= ::BCrypt::Password.create(password) - end - end - end - - def model_instance - @model_instance ||= fake_model_with_password_strategy( - Clearance::PasswordStrategies::BCryptMigrationFromSHA1 - ) - end - - def salt - "salt" - end - - def password - "password" - end -end diff --git a/spec/password_strategies/blowfish_spec.rb b/spec/password_strategies/blowfish_spec.rb deleted file mode 100644 index 2cc98f7ae..000000000 --- a/spec/password_strategies/blowfish_spec.rb +++ /dev/null @@ -1,61 +0,0 @@ -require "spec_helper" -include FakeModelWithPasswordStrategy - -describe Clearance::PasswordStrategies::Blowfish do - around do |example| - silence_warnings do - example.run - end - end - - describe "#password=" do - context "when the password is set" do - it "does not initialize the salt" do - model_instance = fake_model_with_blowfish_strategy - model_instance.salt = salt - model_instance.password = password - - expect(model_instance.salt).to eq salt - end - - it "encrypts the password using Blowfish and the existing salt" do - model_instance = fake_model_with_blowfish_strategy - model_instance.salt = salt - model_instance.salt = salt - model_instance.password = password - cipher = OpenSSL::Cipher::Cipher.new("bf-cbc").encrypt - cipher.key = Digest::SHA256.digest(salt) - expected = cipher.update("--#{salt}--#{password}--") << cipher.final - - encrypted_password = Base64.decode64(model_instance.encrypted_password) - - expect(encrypted_password).to eq expected - end - end - - context "when the salt is not set" do - it "should initialize the salt" do - model_instance = fake_model_with_blowfish_strategy - model_instance.salt = salt - model_instance.salt = nil - model_instance.password = password - - expect(model_instance.salt).not_to be_nil - end - end - end - - def fake_model_with_blowfish_strategy - @model_instance ||= fake_model_with_password_strategy( - Clearance::PasswordStrategies::Blowfish - ) - end - - def salt - "salt" - end - - def password - "password" - end -end diff --git a/spec/password_strategies/sha1_spec.rb b/spec/password_strategies/sha1_spec.rb deleted file mode 100644 index 0ac4c5956..000000000 --- a/spec/password_strategies/sha1_spec.rb +++ /dev/null @@ -1,59 +0,0 @@ -require "spec_helper" -include FakeModelWithPasswordStrategy - -describe Clearance::PasswordStrategies::SHA1 do - around do |example| - silence_warnings do - example.run - end - end - - describe "#password=" do - context "when the salt is set" do - it "does not initialize the salt when assigned" do - model_instance = fake_model_with_sha1_strategy - - model_instance.salt = salt - - expect(model_instance.salt).to eq salt - end - - it "encrypts the password using SHA1 and the existing salt" do - model_instance = fake_model_with_sha1_strategy - model_instance.salt = salt - model_instance.password = password - - expected = Digest::SHA1.hexdigest("--#{salt}--#{password}--") - - expect(model_instance.encrypted_password).to eq expected - end - end - - context "when the salt is set" do - it "generates the salt" do - model_instance = fake_model_with_sha1_strategy - model_instance.password = "" - - expect(model_instance.salt).not_to be_nil - end - - it "doesn't encrypt the password" do - model_instance = fake_model_with_sha1_strategy - - expect(model_instance.encrypted_password).to be_nil - end - end - end - - def fake_model_with_sha1_strategy - fake_model_with_password_strategy(Clearance::PasswordStrategies::SHA1) - end - - def salt - "salt" - end - - def password - "password" - end -end From 5263be0dca830ae64d408f68b950b38aa6cf06ec Mon Sep 17 00:00:00 2001 From: Derek Prior Date: Fri, 29 Apr 2016 10:22:13 -0400 Subject: [PATCH 04/13] Use flash alerts rather than notices These messages are used to tell users they can't access a page without signing in, that their password is incorrect, etc. These are much closer to error or alert states than notice. --- NEWS.md | 4 ++++ .../clearance/passwords_controller.rb | 4 ++-- .../clearance/sessions_controller.rb | 2 +- lib/clearance/authorization.rb | 2 +- lib/clearance/testing/deny_access_matcher.rb | 18 +++++++++--------- spec/controllers/passwords_controller_spec.rb | 10 +++++----- .../controllers/permissions_controller_spec.rb | 2 +- spec/controllers/sessions_controller_spec.rb | 2 +- 8 files changed, 24 insertions(+), 20 deletions(-) diff --git a/NEWS.md b/NEWS.md index 06e7c2c57..e8dc71f01 100644 --- a/NEWS.md +++ b/NEWS.md @@ -10,6 +10,10 @@ complete changelog, see the git history for each version via the version links. - Removed support for Rails versions older than 4.2 - Removed all deprecated code from Clearance 1.x +### Changed +- Flash messages now use `flash[:alert]` rather than `flash[:notice]` as they + were used as errors more than notices. + [2.0.0]: https://github.com/thoughtbot/clearance/compare/v1.14.1...2.0 ## [1.14.1] - May 12, 2016 diff --git a/app/controllers/clearance/passwords_controller.rb b/app/controllers/clearance/passwords_controller.rb index 897065011..664ba0630 100644 --- a/app/controllers/clearance/passwords_controller.rb +++ b/app/controllers/clearance/passwords_controller.rb @@ -97,13 +97,13 @@ def ensure_existing_user end def flash_failure_when_forbidden - flash.now[:notice] = translate(:forbidden, + flash.now[:alert] = translate(:forbidden, scope: [:clearance, :controllers, :passwords], default: t('flashes.failure_when_forbidden')) end def flash_failure_after_update - flash.now[:notice] = translate(:blank_password, + flash.now[:alert] = translate(:blank_password, scope: [:clearance, :controllers, :passwords], default: t('flashes.failure_after_update')) end diff --git a/app/controllers/clearance/sessions_controller.rb b/app/controllers/clearance/sessions_controller.rb index 7243ec08d..763199ddc 100644 --- a/app/controllers/clearance/sessions_controller.rb +++ b/app/controllers/clearance/sessions_controller.rb @@ -24,7 +24,7 @@ def create if status.success? redirect_back_or url_after_create else - flash.now.notice = status.failure_message + flash.now.alert = status.failure_message render template: "sessions/new", status: :unauthorized end end diff --git a/lib/clearance/authorization.rb b/lib/clearance/authorization.rb index 15aca029c..e6c9337fd 100644 --- a/lib/clearance/authorization.rb +++ b/lib/clearance/authorization.rb @@ -54,7 +54,7 @@ def redirect_request(flash_message) store_location if flash_message - flash[:notice] = flash_message + flash[:alert] = flash_message end if signed_in? diff --git a/lib/clearance/testing/deny_access_matcher.rb b/lib/clearance/testing/deny_access_matcher.rb index 7f19a0155..b756717e0 100644 --- a/lib/clearance/testing/deny_access_matcher.rb +++ b/lib/clearance/testing/deny_access_matcher.rb @@ -8,7 +8,7 @@ module Testing module Matchers # The `deny_access` matcher is used to assert that a # request is denied access by clearance. - # @option opts [String] :flash The expected flash notice message. Defaults + # @option opts [String] :flash The expected flash alert message. Defaults # to nil, which means the flash will not be checked. # @option opts [String] :redirect The expected redirect url. Defaults to # `'/'` if signed in or the `sign_in_url` if signed out. @@ -78,15 +78,15 @@ def clearance_session @controller.request.env[:clearance] end - def flash_notice - @controller.flash[:notice] + def flash_alert + @controller.flash[:alert] end - def flash_notice_value - if flash_notice.respond_to?(:values) - flash_notice.values.first + def flash_alert_value + if flash_alert.respond_to?(:values) + flash_alert.values.first else - flash_notice + flash_alert end end @@ -108,13 +108,13 @@ def sets_the_flash? if @flash.blank? true else - if flash_notice_value == @flash + if flash_alert_value == @flash @failure_message_when_negated << "Didn't expect to set the flash to #{@flash}" true else @failure_message << "Expected the flash to be set to #{@flash} "\ - "but was #{flash_notice_value}" + "but was #{flash_alert_value}" false end end diff --git a/spec/controllers/passwords_controller_spec.rb b/spec/controllers/passwords_controller_spec.rb index fd946dc4e..9d1fe112a 100644 --- a/spec/controllers/passwords_controller_spec.rb +++ b/spec/controllers/passwords_controller_spec.rb @@ -70,22 +70,22 @@ end context "blank token is supplied" do - it "renders the new password reset form with a flash notice" do + it "renders the new password reset form with a flash alert" do get :edit, user_id: 1, token: "" expect(response).to render_template(:new) - expect(flash.now[:notice]).to match(/double check the URL/i) + expect(flash.now[:alert]).to match(/double check the URL/i) end end context "invalid token is supplied" do - it "renders the new password reset form with a flash notice" do + it "renders the new password reset form with a flash alert" do user = create(:user, :with_forgotten_password) get :edit, user_id: 1, token: user.confirmation_token + "a" expect(response).to render_template(:new) - expect(flash.now[:notice]).to match(/double check the URL/i) + expect(flash.now[:alert]).to match(/double check the URL/i) end end end @@ -128,7 +128,7 @@ put :update, update_parameters(user, new_password: "") - expect(flash.now[:notice]).to match(/password can't be blank/i) + expect(flash.now[:alert]).to match(/password can't be blank/i) expect(response).to render_template(:edit) expect(cookies[:remember_token]).to be_nil end diff --git a/spec/controllers/permissions_controller_spec.rb b/spec/controllers/permissions_controller_spec.rb index 0791db4e0..431516bf6 100644 --- a/spec/controllers/permissions_controller_spec.rb +++ b/spec/controllers/permissions_controller_spec.rb @@ -62,7 +62,7 @@ def show it "denies access to show and display a flash message" do get :show - expect(flash[:notice]).to match(/^Please sign in to continue/) + expect(flash[:alert]).to match(/^Please sign in to continue/) end end diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb index 4db8bd7bc..302d9ef18 100644 --- a/spec/controllers/sessions_controller_spec.rb +++ b/spec/controllers/sessions_controller_spec.rb @@ -31,7 +31,7 @@ post :create, session: { email: user.email, password: user.password } expect(response).to render_template(:new) - expect(flash[:notice]).to match(/^Bad email or password/) + expect(flash[:alert]).to match(/^Bad email or password/) end end From c6f251b28783758fd044796e7bcd07e42a682a68 Mon Sep 17 00:00:00 2001 From: Derek Prior Date: Fri, 29 Apr 2016 10:24:44 -0400 Subject: [PATCH 05/13] Convert nested `if/else` to `if/elsif/else` --- lib/clearance/testing/deny_access_matcher.rb | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/lib/clearance/testing/deny_access_matcher.rb b/lib/clearance/testing/deny_access_matcher.rb index b756717e0..cbdf8b212 100644 --- a/lib/clearance/testing/deny_access_matcher.rb +++ b/lib/clearance/testing/deny_access_matcher.rb @@ -107,16 +107,14 @@ def redirects_to_url? def sets_the_flash? if @flash.blank? true + elsif flash_alert_value == @flash + @failure_message_when_negated << + "Didn't expect to set the flash to #{@flash}" + true else - if flash_alert_value == @flash - @failure_message_when_negated << - "Didn't expect to set the flash to #{@flash}" - true - else - @failure_message << "Expected the flash to be set to #{@flash} "\ - "but was #{flash_alert_value}" - false - end + @failure_message << "Expected the flash to be set to #{@flash} "\ + "but was #{flash_alert_value}" + false end end end From f30b60d2966e7f058d24c2e9c18e119d31f0eff7 Mon Sep 17 00:00:00 2001 From: Derek Prior Date: Fri, 29 Apr 2016 10:41:40 -0400 Subject: [PATCH 06/13] Remove unused method `PasswordsController#url_after_create` was never called by our code. --- app/controllers/clearance/passwords_controller.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/app/controllers/clearance/passwords_controller.rb b/app/controllers/clearance/passwords_controller.rb index 664ba0630..276df4498 100644 --- a/app/controllers/clearance/passwords_controller.rb +++ b/app/controllers/clearance/passwords_controller.rb @@ -108,10 +108,6 @@ def flash_failure_after_update default: t('flashes.failure_after_update')) end - def url_after_create - sign_in_url - end - def url_after_update Clearance.configuration.redirect_url end From 9ae46df57513b5c5250ce9a49e2e3727f43b2fa2 Mon Sep 17 00:00:00 2001 From: Derek Prior Date: Fri, 13 May 2016 10:59:07 -0400 Subject: [PATCH 07/13] Remove test setup for Rails 3.x --- spec/dummy/application.rb | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/spec/dummy/application.rb b/spec/dummy/application.rb index fc8b3be96..b1945c7b9 100644 --- a/spec/dummy/application.rb +++ b/spec/dummy/application.rb @@ -3,11 +3,6 @@ module Dummy APP_ROOT = File.expand_path("..", __FILE__).freeze - - def self.rails4? - Rails::VERSION::MAJOR >= 4 - end - I18n.enforce_available_locales = true class Application < Rails::Application @@ -29,13 +24,8 @@ class Application < Rails::Application config.paths["config/database"] = "#{APP_ROOT}/config/database.yml" config.paths["log"] = "tmp/log/development.log" config.secret_token = "SECRET_TOKEN_IS_MIN_30_CHARS_LONG" - - if Dummy.rails4? - config.paths.add "config/routes.rb", with: "#{APP_ROOT}/config/routes.rb" - config.secret_key_base = "SECRET_KEY_BASE" - else - config.paths.add "config/routes", with: "#{APP_ROOT}/config/routes.rb" - end + config.paths.add "config/routes.rb", with: "#{APP_ROOT}/config/routes.rb" + config.secret_key_base = "SECRET_KEY_BASE" if config.respond_to?(:active_job) config.active_job.queue_adapter = :inline From 3313a7e5d0b31b6292b06b37908675521140d6c9 Mon Sep 17 00:00:00 2001 From: Derek Prior Date: Fri, 13 May 2016 10:59:19 -0400 Subject: [PATCH 08/13] Update RSpec dependency --- Appraisals | 1 - Gemfile | 2 +- Gemfile.lock | 32 ++++++++++++++++---------------- gemfiles/rails42.gemfile | 2 +- gemfiles/rails50.gemfile | 2 +- 5 files changed, 19 insertions(+), 20 deletions(-) diff --git a/Appraisals b/Appraisals index 6135abe01..f70c6284e 100644 --- a/Appraisals +++ b/Appraisals @@ -5,5 +5,4 @@ end appraise "rails50" do gem "rails", "~> 5.0.0.beta3" gem "rails-controller-testing" - gem "rspec-rails", "~> 3.5.0.beta1" end diff --git a/Gemfile b/Gemfile index 463f11973..241285669 100644 --- a/Gemfile +++ b/Gemfile @@ -8,7 +8,7 @@ gem 'bundler', '~> 1.3' gem 'capybara', '>= 2.6.2' gem 'database_cleaner', '~> 1.0' gem 'factory_girl_rails', '~> 4.2' -gem 'rspec-rails', '~> 3.1' +gem 'rspec-rails', '~> 3.5.0.beta3' gem 'shoulda-matchers', '~> 2.8' gem 'sqlite3', '~> 1.3' gem 'timecop', '~> 0.6' diff --git a/Gemfile.lock b/Gemfile.lock index 1d534e85a..e0c60208e 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -123,23 +123,23 @@ GEM rake (>= 0.8.7) thor (>= 0.18.1, < 2.0) rake (11.1.2) - rspec-core (3.4.4) - rspec-support (~> 3.4.0) - rspec-expectations (3.4.0) + rspec-core (3.5.0.beta3) + rspec-support (= 3.5.0.beta3) + rspec-expectations (3.5.0.beta3) diff-lcs (>= 1.2.0, < 2.0) - rspec-support (~> 3.4.0) - rspec-mocks (3.4.1) + rspec-support (= 3.5.0.beta3) + rspec-mocks (3.5.0.beta3) diff-lcs (>= 1.2.0, < 2.0) - rspec-support (~> 3.4.0) - rspec-rails (3.4.2) - actionpack (>= 3.0, < 4.3) - activesupport (>= 3.0, < 4.3) - railties (>= 3.0, < 4.3) - rspec-core (~> 3.4.0) - rspec-expectations (~> 3.4.0) - rspec-mocks (~> 3.4.0) - rspec-support (~> 3.4.0) - rspec-support (3.4.1) + rspec-support (= 3.5.0.beta3) + rspec-rails (3.5.0.beta3) + actionpack (>= 3.0) + activesupport (>= 3.0) + railties (>= 3.0) + rspec-core (= 3.5.0.beta3) + rspec-expectations (= 3.5.0.beta3) + rspec-mocks (= 3.5.0.beta3) + rspec-support (= 3.5.0.beta3) + rspec-support (3.5.0.beta3) shoulda-matchers (2.8.0) activesupport (>= 3.0.0) slop (3.6.0) @@ -171,7 +171,7 @@ DEPENDENCIES database_cleaner (~> 1.0) factory_girl_rails (~> 4.2) pry - rspec-rails (~> 3.1) + rspec-rails (~> 3.5.0.beta3) shoulda-matchers (~> 2.8) sqlite3 (~> 1.3) timecop (~> 0.6) diff --git a/gemfiles/rails42.gemfile b/gemfiles/rails42.gemfile index 043c9ed34..533dd767e 100644 --- a/gemfiles/rails42.gemfile +++ b/gemfiles/rails42.gemfile @@ -8,7 +8,7 @@ gem "bundler", "~> 1.3" gem "capybara", ">= 2.6.2" gem "database_cleaner", "~> 1.0" gem "factory_girl_rails", "~> 4.2" -gem "rspec-rails", "~> 3.1" +gem "rspec-rails", "~> 3.5.0.beta3" gem "shoulda-matchers", "~> 2.8" gem "sqlite3", "~> 1.3" gem "timecop", "~> 0.6" diff --git a/gemfiles/rails50.gemfile b/gemfiles/rails50.gemfile index db992a3f6..3b55babd5 100644 --- a/gemfiles/rails50.gemfile +++ b/gemfiles/rails50.gemfile @@ -8,7 +8,7 @@ gem "bundler", "~> 1.3" gem "capybara", ">= 2.6.2" gem "database_cleaner", "~> 1.0" gem "factory_girl_rails", "~> 4.2" -gem "rspec-rails", "~> 3.5.0.beta1" +gem "rspec-rails", "~> 3.5.0.beta3" gem "shoulda-matchers", "~> 2.8" gem "sqlite3", "~> 1.3" gem "timecop", "~> 0.6" From 0fb0696d6780b69070b9cf3c2b80932e1f80449d Mon Sep 17 00:00:00 2001 From: Derek Prior Date: Fri, 13 May 2016 11:43:55 -0400 Subject: [PATCH 09/13] Remove conditionals for older versions of Rails These `respond_to` checks were added to support changing Rails API as new versions of Rails were released. Now that we support 4.2 or newer, we don't need them. --- .../clearance/passwords_controller.rb | 26 +++---------------- .../clearance/sessions_controller.rb | 19 ++------------ app/controllers/clearance/users_controller.rb | 11 ++------ lib/clearance/testing/deny_access_matcher.rb | 6 +---- .../clearance/install/install_generator.rb | 6 +---- spec/controllers/apis_controller_spec.rb | 6 +---- spec/controllers/forgeries_controller_spec.rb | 6 +---- .../permissions_controller_spec.rb | 6 +---- spec/dummy/application.rb | 5 +--- .../install/install_generator_spec.rb | 13 +++------- 10 files changed, 16 insertions(+), 88 deletions(-) diff --git a/app/controllers/clearance/passwords_controller.rb b/app/controllers/clearance/passwords_controller.rb index 276df4498..869cbb5c5 100644 --- a/app/controllers/clearance/passwords_controller.rb +++ b/app/controllers/clearance/passwords_controller.rb @@ -1,23 +1,8 @@ require 'active_support/deprecation' class Clearance::PasswordsController < Clearance::BaseController - if respond_to?(:before_action) - skip_before_action :require_login, - only: [:create, :edit, :new, :update], - raise: false - skip_before_action :authorize, - only: [:create, :edit, :new, :update], - raise: false - before_action :ensure_existing_user, only: [:edit, :update] - else - skip_before_filter :require_login, - only: [:create, :edit, :new, :update], - raise: false - skip_before_filter :authorize, - only: [:create, :edit, :new, :update], - raise: false - before_filter :ensure_existing_user, only: [:edit, :update] - end + before_action :ensure_existing_user, only: [:edit, :update] + skip_before_action :require_login, only: [:create, :edit, :new, :update], raise: false def create if user = find_user_for_create @@ -52,12 +37,7 @@ def update def deliver_email(user) mail = ::ClearanceMailer.change_password(user) - - if mail.respond_to?(:deliver_later) - mail.deliver_later - else - mail.deliver - end + mail.deliver_later end def password_reset_params diff --git a/app/controllers/clearance/sessions_controller.rb b/app/controllers/clearance/sessions_controller.rb index 763199ddc..90b5dd187 100644 --- a/app/controllers/clearance/sessions_controller.rb +++ b/app/controllers/clearance/sessions_controller.rb @@ -1,21 +1,6 @@ class Clearance::SessionsController < Clearance::BaseController - if respond_to?(:before_action) - before_action :redirect_signed_in_users, only: [:new] - skip_before_action :require_login, - only: [:create, :new, :destroy], - raise: false - skip_before_action :authorize, - only: [:create, :new, :destroy], - raise: false - else - before_filter :redirect_signed_in_users, only: [:new] - skip_before_filter :require_login, - only: [:create, :new, :destroy], - raise: false - skip_before_filter :authorize, - only: [:create, :new, :destroy], - raise: false - end + before_action :redirect_signed_in_users, only: [:new] + skip_before_action :require_login, only: [:create, :new, :destroy], raise: false def create @user = authenticate(params) diff --git a/app/controllers/clearance/users_controller.rb b/app/controllers/clearance/users_controller.rb index 82db7569b..373ef7dde 100644 --- a/app/controllers/clearance/users_controller.rb +++ b/app/controllers/clearance/users_controller.rb @@ -1,13 +1,6 @@ class Clearance::UsersController < Clearance::BaseController - if respond_to?(:before_action) - before_action :redirect_signed_in_users, only: [:create, :new] - skip_before_action :require_login, only: [:create, :new], raise: false - skip_before_action :authorize, only: [:create, :new], raise: false - else - before_filter :redirect_signed_in_users, only: [:create, :new] - skip_before_filter :require_login, only: [:create, :new], raise: false - skip_before_filter :authorize, only: [:create, :new], raise: false - end + before_action :redirect_signed_in_users, only: [:create, :new] + skip_before_action :require_login, only: [:create, :new], raise: false def new @user = user_from_params diff --git a/lib/clearance/testing/deny_access_matcher.rb b/lib/clearance/testing/deny_access_matcher.rb index cbdf8b212..3e657f0e4 100644 --- a/lib/clearance/testing/deny_access_matcher.rb +++ b/lib/clearance/testing/deny_access_matcher.rb @@ -83,11 +83,7 @@ def flash_alert end def flash_alert_value - if flash_alert.respond_to?(:values) - flash_alert.values.first - else - flash_alert - end + flash_alert.values.first end def redirects_to_url? diff --git a/lib/generators/clearance/install/install_generator.rb b/lib/generators/clearance/install/install_generator.rb index d233a11f9..92c1a6e99 100644 --- a/lib/generators/clearance/install/install_generator.rb +++ b/lib/generators/clearance/install/install_generator.rb @@ -102,11 +102,7 @@ def migration_name_without_timestamp(file) end def users_table_exists? - if ActiveRecord::Base.connection.respond_to?(:data_source_exists?) - ActiveRecord::Base.connection.data_source_exists?(:users) - else - ActiveRecord::Base.connection.table_exists?(:users) - end + ActiveRecord::Base.connection.data_source_exists?(:users) end def existing_users_columns diff --git a/spec/controllers/apis_controller_spec.rb b/spec/controllers/apis_controller_spec.rb index 9afa2f2ac..abd254ff5 100644 --- a/spec/controllers/apis_controller_spec.rb +++ b/spec/controllers/apis_controller_spec.rb @@ -3,11 +3,7 @@ class ApisController < ActionController::Base include Clearance::Controller - if respond_to?(:before_action) - before_action :require_login - else - before_filter :require_login - end + before_action :require_login def show head :ok diff --git a/spec/controllers/forgeries_controller_spec.rb b/spec/controllers/forgeries_controller_spec.rb index abb8e5458..3252ebd86 100644 --- a/spec/controllers/forgeries_controller_spec.rb +++ b/spec/controllers/forgeries_controller_spec.rb @@ -5,11 +5,7 @@ class ForgeriesController < ActionController::Base protect_from_forgery - if respond_to?(:before_action) - before_action :require_login - else - before_filter :require_login - end + before_action :require_login # This is off in test by default, but we need it for this test self.allow_forgery_protection = true diff --git a/spec/controllers/permissions_controller_spec.rb b/spec/controllers/permissions_controller_spec.rb index 431516bf6..736d07ad4 100644 --- a/spec/controllers/permissions_controller_spec.rb +++ b/spec/controllers/permissions_controller_spec.rb @@ -3,11 +3,7 @@ class PermissionsController < ActionController::Base include Clearance::Controller - if respond_to?(:before_action) - before_action :require_login, only: :show - else - before_filter :require_login, only: :show - end + before_action :require_login, only: :show def new head :ok diff --git a/spec/dummy/application.rb b/spec/dummy/application.rb index b1945c7b9..278ee4604 100644 --- a/spec/dummy/application.rb +++ b/spec/dummy/application.rb @@ -26,10 +26,7 @@ class Application < Rails::Application config.secret_token = "SECRET_TOKEN_IS_MIN_30_CHARS_LONG" config.paths.add "config/routes.rb", with: "#{APP_ROOT}/config/routes.rb" config.secret_key_base = "SECRET_KEY_BASE" - - if config.respond_to?(:active_job) - config.active_job.queue_adapter = :inline - end + config.active_job.queue_adapter = :inline def require_environment! initialize! diff --git a/spec/generators/clearance/install/install_generator_spec.rb b/spec/generators/clearance/install/install_generator_spec.rb index feecee590..9a9643240 100644 --- a/spec/generators/clearance/install/install_generator_spec.rb +++ b/spec/generators/clearance/install/install_generator_spec.rb @@ -118,16 +118,9 @@ def table_does_not_exist(name) connection = ActiveRecord::Base.connection - - if connection.respond_to?(:data_source_exists?) - allow(connection).to receive(:data_source_exists?). - with(name). - and_return(false) - else - allow(connection).to receive(:table_exists?). - with(name). - and_return(false) - end + allow(connection).to receive(:data_source_exists?). + with(name). + and_return(false) end def contain_models_inherit_from From 64047ca3bcaa3a0c028e24d76d76adb8c863828b Mon Sep 17 00:00:00 2001 From: Derek Prior Date: Fri, 13 May 2016 13:34:54 -0400 Subject: [PATCH 10/13] Clean up passwords controller * Remove deprecated parameter handling * Order actions more logically * Update to double quotes --- .../clearance/passwords_controller.rb | 25 ++++++++----------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/app/controllers/clearance/passwords_controller.rb b/app/controllers/clearance/passwords_controller.rb index 869cbb5c5..c3176a100 100644 --- a/app/controllers/clearance/passwords_controller.rb +++ b/app/controllers/clearance/passwords_controller.rb @@ -4,21 +4,23 @@ class Clearance::PasswordsController < Clearance::BaseController before_action :ensure_existing_user, only: [:edit, :update] skip_before_action :require_login, only: [:create, :edit, :new, :update], raise: false + def new + render template: "passwords/new" + end + def create if user = find_user_for_create user.forgot_password! deliver_email(user) end - render template: 'passwords/create' + + render template: "passwords/create" end def edit @user = find_user_for_edit - render template: 'passwords/edit' - end - def new - render template: 'passwords/new' + render template: "passwords/edit" end def update @@ -29,7 +31,7 @@ def update redirect_to url_after_update else flash_failure_after_update - render template: 'passwords/edit' + render template: "passwords/edit" end end @@ -41,12 +43,7 @@ def deliver_email(user) end def password_reset_params - if params.has_key? :user - ActiveSupport::Deprecation.warn %{Since locales functionality was added, accessing params[:user] is no longer supported.} - params[:user][:password] - else - params[:password_reset][:password] - end + params[:password_reset][:password] end def find_user_by_id_and_confirmation_token @@ -79,13 +76,13 @@ def ensure_existing_user def flash_failure_when_forbidden flash.now[:alert] = translate(:forbidden, scope: [:clearance, :controllers, :passwords], - default: t('flashes.failure_when_forbidden')) + default: t("flashes.failure_when_forbidden")) end def flash_failure_after_update flash.now[:alert] = translate(:blank_password, scope: [:clearance, :controllers, :passwords], - default: t('flashes.failure_after_update')) + default: t("flashes.failure_after_update")) end def url_after_update From 13b31ac41c0f9e6362aa7a73c59d591609c790c9 Mon Sep 17 00:00:00 2001 From: Derek Prior & Melissa Xie Date: Fri, 13 May 2016 15:53:31 -0400 Subject: [PATCH 11/13] Add expiring, databaseless password reset tokens It is a best security practice for password reset token to expire after some amount of time. In Clearance 1.x, this was not the case. A password reset email could be used months after it was originally sent so long as no other password reset was ever completed. In this change, password resets expire after 15 minutes (configurable) or after the user successfully changes their password in any manner (whichever comes first). The token, confusingly called `confirmation_token`, is no longer stored in the database. Instead, we use `ActiveSupport::MessageVerifier` to sign the token and validate it when it is used. The message verifier is configurable in case developers want to use something else. In a future refactoring, I'd like to introduce a layer between Clearance and `ActiveSupport::MessageVerifier` to make the API a bit more pleasant to use, but this is an exercise for a future PR. For instance, I'd prefer that the Clearance abstraction generate and validate tokens only by taking a user object (and using the Clearance configuration). --- NEWS.md | 10 +++ README.md | 11 ++++ .../clearance/passwords_controller.rb | 46 +++++++------ app/mailers/clearance_mailer.rb | 12 ++++ .../clearance_mailer/change_password.html.erb | 2 +- .../clearance_mailer/change_password.text.erb | 2 +- app/views/passwords/edit.html.erb | 4 +- config/locales/clearance.en.yml | 2 + .../20110111224543_create_clearance_users.rb | 1 - db/schema.rb | 3 +- lib/clearance/configuration.rb | 14 ++++ lib/clearance/user.rb | 31 +-------- .../clearance/install/install_generator.rb | 1 - .../visitor_resets_password_spec.rb.tt | 19 ++---- .../visitor_updates_password_spec.rb.tt | 16 +++-- .../support/features/clearance_helpers.rb | 6 -- lib/generators/clearance/upgrade/USAGE | 2 + ...remove_confirmation_token_from_users.rb.tt | 5 ++ .../clearance/upgrade/upgrade_generator.rb | 34 ++++++++++ spec/configuration_spec.rb | 31 +++++++++ spec/controllers/passwords_controller_spec.rb | 66 +++++++++++++------ spec/factories.rb | 4 -- .../upgrade/upgrade_generator_spec.rb | 16 +++++ spec/mailers/clearance_mailer_spec.rb | 36 +++++----- spec/user_spec.rb | 32 +-------- 25 files changed, 251 insertions(+), 155 deletions(-) create mode 100644 lib/generators/clearance/upgrade/USAGE create mode 100644 lib/generators/clearance/upgrade/templates/db/migrate/remove_confirmation_token_from_users.rb.tt create mode 100644 lib/generators/clearance/upgrade/upgrade_generator.rb create mode 100644 spec/generators/clearance/upgrade/upgrade_generator_spec.rb diff --git a/NEWS.md b/NEWS.md index e8dc71f01..0f6581768 100644 --- a/NEWS.md +++ b/NEWS.md @@ -9,11 +9,21 @@ complete changelog, see the git history for each version via the version links. - Removed support for Ruby versions older than 2.2 - Removed support for Rails versions older than 4.2 - Removed all deprecated code from Clearance 1.x +- Removed `User#confirmation_token`, `User#forgot_password!`, and + `User#generate_confirmation_token` as part of the change to expiring, + databaseless password reset tokens. ### Changed +- Password resets now use expiring signed tokens that do not require persistence + to the `users` table. By default, the tokens are generated with + `ActiveSupport::MessageVerifier` and expire in 15 minutes. - Flash messages now use `flash[:alert]` rather than `flash[:notice]` as they were used as errors more than notices. +### Added +- `rails generate clearance:upgrade` generator to prepare your Clearance 1.x + project for Clearance 2.0 + [2.0.0]: https://github.com/thoughtbot/clearance/compare/v1.14.1...2.0 ## [1.14.1] - May 12, 2016 diff --git a/README.md b/README.md index c14253cbc..03c42d564 100644 --- a/README.md +++ b/README.md @@ -56,6 +56,8 @@ Clearance.configure do |config| config.routes = true config.httponly = false config.mailer_sender = "reply@example.com" + config.message_verifier = ActiveSupport::MessageVerifier.new(secret_key_base) + config.password_reset_time_limit = 15.minutes config.password_strategy = Clearance::PasswordStrategies::BCrypt config.redirect_url = "/" config.secure_cookie = false @@ -124,6 +126,15 @@ Clearance.configure do |config| end ``` +The password reset link contained in the email is configured to expire in 15 +minutes. You can change this with the `password_reset_time_limit` configuration. + +```ruby +Clearance.configure do |config| + config.password_reset_time_limit = 1.hour +end +``` + ### Integrating with Rack Applications Clearance adds its session to the Rack environment hash so middleware and other diff --git a/app/controllers/clearance/passwords_controller.rb b/app/controllers/clearance/passwords_controller.rb index c3176a100..a81a4be8f 100644 --- a/app/controllers/clearance/passwords_controller.rb +++ b/app/controllers/clearance/passwords_controller.rb @@ -10,7 +10,6 @@ def new def create if user = find_user_for_create - user.forgot_password! deliver_email(user) end @@ -26,9 +25,9 @@ def edit def update @user = find_user_for_update - if @user.update_password password_reset_params - sign_in @user - redirect_to url_after_update + if @user.update_password(password_reset_params) + sign_in(@user) + redirect_to(url_after_update) else flash_failure_after_update render template: "passwords/edit" @@ -38,45 +37,50 @@ def update private def deliver_email(user) - mail = ::ClearanceMailer.change_password(user) - mail.deliver_later + ::ClearanceMailer.change_password(user).deliver_later end def password_reset_params params[:password_reset][:password] end - def find_user_by_id_and_confirmation_token - user_param = Clearance.configuration.user_id_parameter - - Clearance.configuration.user_model. - find_by_id_and_confirmation_token params[user_param], params[:token].to_s - end - def find_user_for_create Clearance.configuration.user_model. find_by_normalized_email params[:password][:email] end + def find_user_by_password_reset_token(token) + verifier = Clearance.configuration.message_verifier + user_model = Clearance.configuration.user_model + + begin + user_id, encrypted_password, expiration = verifier.verify(token) + rescue ActiveSupport::MessageVerifier::InvalidSignature + expiration = 1.day.ago + end + + if expiration.future? + user_model.find_by(id: user_id, encrypted_password: encrypted_password) + end + end + def find_user_for_edit - find_user_by_id_and_confirmation_token + find_user_by_password_reset_token(params[:token]) end def find_user_for_update - find_user_by_id_and_confirmation_token + find_user_by_password_reset_token(params[:token]) end def ensure_existing_user - unless find_user_by_id_and_confirmation_token - flash_failure_when_forbidden + unless find_user_by_password_reset_token(params[:token]) + flash_failure_when_invalid render template: "passwords/new" end end - def flash_failure_when_forbidden - flash.now[:alert] = translate(:forbidden, - scope: [:clearance, :controllers, :passwords], - default: t("flashes.failure_when_forbidden")) + def flash_failure_when_invalid + flash.now[:alert] = translate("flashes.failure_when_password_reset_invalid") end def flash_failure_after_update diff --git a/app/mailers/clearance_mailer.rb b/app/mailers/clearance_mailer.rb index ffd91aa01..5749e4f6d 100644 --- a/app/mailers/clearance_mailer.rb +++ b/app/mailers/clearance_mailer.rb @@ -1,6 +1,8 @@ class ClearanceMailer < ActionMailer::Base def change_password(user) @user = user + @token = generate_password_reset_token(@user) + mail( from: Clearance.configuration.mailer_sender, to: @user.email, @@ -10,4 +12,14 @@ def change_password(user) ), ) end + + private + + def generate_password_reset_token(user) + Clearance.configuration.message_verifier.generate([ + user.id, + user.encrypted_password, + Clearance.configuration.password_reset_time_limit.from_now, + ]) + end end diff --git a/app/views/clearance_mailer/change_password.html.erb b/app/views/clearance_mailer/change_password.html.erb index bc315ecba..ed96e1b4c 100644 --- a/app/views/clearance_mailer/change_password.html.erb +++ b/app/views/clearance_mailer/change_password.html.erb @@ -2,7 +2,7 @@

<%= link_to t(".link_text", default: "Change my password"), - edit_user_password_url(@user, token: @user.confirmation_token.html_safe) %> + edit_user_password_url(@user, token: @token) %>

<%= raw t(".closing") %>

diff --git a/app/views/clearance_mailer/change_password.text.erb b/app/views/clearance_mailer/change_password.text.erb index ed01f74b5..39231c289 100644 --- a/app/views/clearance_mailer/change_password.text.erb +++ b/app/views/clearance_mailer/change_password.text.erb @@ -1,5 +1,5 @@ <%= t(".opening") %> -<%= edit_user_password_url(@user, token: @user.confirmation_token.html_safe) %> +<%= edit_user_password_url(@user, token: @token) %> <%= raw t(".closing") %> diff --git a/app/views/passwords/edit.html.erb b/app/views/passwords/edit.html.erb index 6761ab321..d924a83ac 100644 --- a/app/views/passwords/edit.html.erb +++ b/app/views/passwords/edit.html.erb @@ -4,8 +4,8 @@

<%= t(".description") %>

<%= form_for :password_reset, - url: user_password_path(@user, token: @user.confirmation_token), - html: { method: :put } do |form| %> + url: user_password_path(@user, token: params[:token]), + html: { method: :patch } do |form| %>
<%= form.label :password %> <%= form.password_field :password %> diff --git a/config/locales/clearance.en.yml b/config/locales/clearance.en.yml index da8de0daa..2068c0c9d 100644 --- a/config/locales/clearance.en.yml +++ b/config/locales/clearance.en.yml @@ -17,6 +17,8 @@ en: failure_when_forbidden: Please double check the URL or try submitting the form again. failure_when_not_signed_in: Please sign in to continue. + failure_when_password_reset_invalid: Your password reset token has expired + or is invalid. helpers: label: password: diff --git a/db/migrate/20110111224543_create_clearance_users.rb b/db/migrate/20110111224543_create_clearance_users.rb index 75e22bfee..11106ec5c 100644 --- a/db/migrate/20110111224543_create_clearance_users.rb +++ b/db/migrate/20110111224543_create_clearance_users.rb @@ -4,7 +4,6 @@ def self.up t.timestamps null: false t.string :email, null: false t.string :encrypted_password, limit: 128, null: false - t.string :confirmation_token, limit: 128 t.string :remember_token, limit: 128, null: false end diff --git a/db/schema.rb b/db/schema.rb index bf7e2f009..ff34b8c90 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -13,12 +13,11 @@ ActiveRecord::Schema.define(version: 20110111224543) do - create_table "users", force: true do |t| + create_table "users", force: :cascade do |t| t.datetime "created_at", null: false t.datetime "updated_at", null: false t.string "email", null: false t.string "encrypted_password", limit: 128, null: false - t.string "confirmation_token", limit: 128 t.string "remember_token", limit: 128, null: false end diff --git a/lib/clearance/configuration.rb b/lib/clearance/configuration.rb index 32c5bfbd2..53b5a9cbc 100644 --- a/lib/clearance/configuration.rb +++ b/lib/clearance/configuration.rb @@ -47,6 +47,18 @@ class Configuration # @return [String] attr_accessor :mailer_sender + # Used to generate and verify password reset tokens + # Defaults to an `ActiveSupport::MessageVerifier` instance, initialized + # in concert with your application's `secret_key_base` via + # `Rails.application.message_verifier`. + # @return [#generate #verify] + attr_accessor :message_verifier + + # Determines how long password reset emails are valid for + # Defaults to 15 minutes + # @return [Integer] + attr_accessor :password_reset_time_limit + # The password strategy to use when authenticating and setting passwords. # Defaults to {Clearance::PasswordStrategies::BCrypt}. # @return [Module #authenticated? #password=] @@ -93,6 +105,8 @@ def initialize @cookie_name = "remember_token" @httponly = false @mailer_sender = 'reply@example.com' + @message_verifier = Rails.application.message_verifier("clearance") + @password_reset_time_limit = 15.minutes @redirect_url = '/' @routes = true @secure_cookie = false diff --git a/lib/clearance/user.rb b/lib/clearance/user.rb index fbf6a6e88..efd3126ea 100644 --- a/lib/clearance/user.rb +++ b/lib/clearance/user.rb @@ -42,10 +42,6 @@ module Clearance # @return [String] The value used to identify this user in their {Session} # cookie. # - # @!attribute confirmation_token - # @return [String] The value used to identify this user in the password - # reset link. - # # @!attribute [r] password # @return [String] Transient (non-persisted) attribute that is set when # updating a user's password. Only the {#encrypted_password} is persisted. @@ -159,20 +155,6 @@ module Callbacks end end - # Generates a {#confirmation_token} for the user, which allows them to reset - # their password via an email link. - # - # Calling `forgot_password!` will cause the user model to be saved without - # validations. Any other changes you made to this user instance will also - # be persisted, without validation. It is inteded to be called on an - # instance with no changes (`dirty? == false`). - # - # @return [Boolean] Was the save successful? - def forgot_password! - generate_confirmation_token - save validate: false - end - # Generates a new {#remember_token} for the user, which will have the effect # of signing all of the user's current sessions out. This is called # internally by {Session#sign_out}. @@ -192,8 +174,7 @@ def reset_remember_token! # the configured password strategy. By default, this is # {PasswordStrategies::BCrypt#password=}. # - # This also has the side-effect of blanking the {#confirmation_token} and - # rotating the `#remember_token`. + # This also has the side-effect of rotating the `#remember_token`. # # Validations will be run as part of this update. If the user instance is # not valid, the password change will not be persisted, and this method will @@ -204,7 +185,6 @@ def update_password(new_password) self.password = new_password if valid? - self.confirmation_token = nil generate_remember_token end @@ -246,15 +226,6 @@ def skip_password_validation? (encrypted_password.present? && !encrypted_password_changed?) end - # Sets the {#confirmation_token} on the instance to a new value generated by - # {Token.new}. The change is not automatically persisted. If you would like - # to generate and save in a single method call, use {#forgot_password!}. - # - # @return [String] The new confirmation token - def generate_confirmation_token - self.confirmation_token = Clearance::Token.new - end - # Sets the {#remember_token} on the instance to a new value generated by # {Token.new}. The change is not automatically persisted. If you would like # to generate and save in a single method call, use diff --git a/lib/generators/clearance/install/install_generator.rb b/lib/generators/clearance/install/install_generator.rb index 92c1a6e99..ef3e186b4 100644 --- a/lib/generators/clearance/install/install_generator.rb +++ b/lib/generators/clearance/install/install_generator.rb @@ -75,7 +75,6 @@ def new_columns @new_columns ||= { email: 't.string :email', encrypted_password: 't.string :encrypted_password, limit: 128', - confirmation_token: 't.string :confirmation_token, limit: 128', remember_token: 't.string :remember_token, limit: 128' }.reject { |column| existing_users_columns.include?(column.to_s) } end diff --git a/lib/generators/clearance/specs/templates/features/clearance/visitor_resets_password_spec.rb.tt b/lib/generators/clearance/specs/templates/features/clearance/visitor_resets_password_spec.rb.tt index 80deeba13..224619eba 100644 --- a/lib/generators/clearance/specs/templates/features/clearance/visitor_resets_password_spec.rb.tt +++ b/lib/generators/clearance/specs/templates/features/clearance/visitor_resets_password_spec.rb.tt @@ -3,7 +3,6 @@ require "support/features/clearance_helpers" RSpec.feature "Visitor resets password" do before { ActionMailer::Base.deliveries.clear } -<% if defined?(ActiveJob) -%> around do |example| original_adapter = ActiveJob::Base.queue_adapter @@ -11,7 +10,6 @@ RSpec.feature "Visitor resets password" do example.run ActiveJob::Base.queue_adapter = original_adapter end -<% end -%> scenario "by navigating to the page" do visit sign_in_path @@ -22,7 +20,8 @@ RSpec.feature "Visitor resets password" do end scenario "with valid email" do - user = user_with_reset_password + user = FactoryGirl.create(:user) + reset_password_for(user.email) expect_page_to_display_change_password_message expect_reset_notification_to_be_sent_to user @@ -38,26 +37,18 @@ RSpec.feature "Visitor resets password" do private def expect_reset_notification_to_be_sent_to(user) - expect(user.confirmation_token).not_to be_blank - expect_mailer_to_have_delivery( - user.email, - "password", - user.confirmation_token - ) + expect_mailer_to_have_delivery(user.email, "password") end def expect_page_to_display_change_password_message expect(page).to have_content I18n.t("passwords.create.description") end - def expect_mailer_to_have_delivery(recipient, subject, body) + def expect_mailer_to_have_delivery(recipient, subject) expect(ActionMailer::Base.deliveries).not_to be_empty message = ActionMailer::Base.deliveries.any? do |email| - email.to == [recipient] && - email.subject =~ /#{subject}/i && - email.html_part.body =~ /#{body}/ && - email.text_part.body =~ /#{body}/ + email.to == [recipient] && email.subject =~ /#{subject}/i end expect(message).to be diff --git a/lib/generators/clearance/specs/templates/features/clearance/visitor_updates_password_spec.rb.tt b/lib/generators/clearance/specs/templates/features/clearance/visitor_updates_password_spec.rb.tt index 4c7504436..2895b9fbf 100644 --- a/lib/generators/clearance/specs/templates/features/clearance/visitor_updates_password_spec.rb.tt +++ b/lib/generators/clearance/specs/templates/features/clearance/visitor_updates_password_spec.rb.tt @@ -3,14 +3,14 @@ require "support/features/clearance_helpers" RSpec.feature "Visitor updates password" do scenario "with valid password" do - user = user_with_reset_password + user = FactoryGirl.create(:user) update_password user, "newpassword" expect_user_to_be_signed_in end scenario "signs in with new password" do - user = user_with_reset_password + user = FactoryGirl.create(:user) update_password user, "newpassword" sign_out sign_in_with user.email, "newpassword" @@ -19,7 +19,7 @@ RSpec.feature "Visitor updates password" do end scenario "tries with a blank password" do - user = user_with_reset_password + user = FactoryGirl.create(:user) visit_password_reset_page_for user change_password_to "" @@ -37,10 +37,18 @@ RSpec.feature "Visitor updates password" do def visit_password_reset_page_for(user) visit edit_user_password_path( user_id: user, - token: user.confirmation_token + token: token_for(user) ) end + def token_for(user) + Clearance.configuration.message_verifier.generate([ + user.id, + user.encrypted_password, + Clearance.configuration.password_reset_time_limit.from_now, + ]) + end + def change_password_to(password) fill_in "password_reset_password", with: password click_button I18n.t("helpers.submit.password_reset.submit") diff --git a/lib/generators/clearance/specs/templates/support/features/clearance_helpers.rb b/lib/generators/clearance/specs/templates/support/features/clearance_helpers.rb index 13540614c..5b1a2714a 100644 --- a/lib/generators/clearance/specs/templates/support/features/clearance_helpers.rb +++ b/lib/generators/clearance/specs/templates/support/features/clearance_helpers.rb @@ -38,12 +38,6 @@ def expect_user_to_be_signed_in def expect_user_to_be_signed_out expect(page).to have_content I18n.t("layouts.application.sign_in") end - - def user_with_reset_password - user = FactoryGirl.create(:user) - reset_password_for user.email - user.reload - end end end diff --git a/lib/generators/clearance/upgrade/USAGE b/lib/generators/clearance/upgrade/USAGE new file mode 100644 index 000000000..82497fca8 --- /dev/null +++ b/lib/generators/clearance/upgrade/USAGE @@ -0,0 +1,2 @@ +Description: + Prepare database for upgrade to Clearance 2.0 diff --git a/lib/generators/clearance/upgrade/templates/db/migrate/remove_confirmation_token_from_users.rb.tt b/lib/generators/clearance/upgrade/templates/db/migrate/remove_confirmation_token_from_users.rb.tt new file mode 100644 index 000000000..19675856c --- /dev/null +++ b/lib/generators/clearance/upgrade/templates/db/migrate/remove_confirmation_token_from_users.rb.tt @@ -0,0 +1,5 @@ +class RemoveConfirmationTokenFromUsers < ActiveRecord::Migration<%= migration_version %> + def change + remove_column :<%= Clearance.configuration.user_model.table_name %>, :confirmation_token, type: :string, limit: 128 + end +end diff --git a/lib/generators/clearance/upgrade/upgrade_generator.rb b/lib/generators/clearance/upgrade/upgrade_generator.rb new file mode 100644 index 000000000..6c2ec88bb --- /dev/null +++ b/lib/generators/clearance/upgrade/upgrade_generator.rb @@ -0,0 +1,34 @@ +require "rails/generators/base" +require "rails/generators/active_record" + +module Clearance + module Generators + class UpgradeGenerator < Rails::Generators::Base + include Rails::Generators::Migration + source_root File.expand_path("../templates", __FILE__) + + # for generating a timestamp when using `create_migration` + def self.next_migration_number(dir) + ActiveRecord::Generators::Base.next_migration_number(dir) + end + + def change_users_table + migration_name = "remove_confirmation_token_from_users.rb" + + migration_template( + "db/migrate/#{migration_name}.tt", + "db/migrate/#{migration_name}", + migration_version: migration_version, + ) + end + + private + + def migration_version + if Rails.version >= "5.0.0" + "[#{Rails::VERSION::MAJOR}.#{Rails::VERSION::MINOR}]" + end + end + end + end +end diff --git a/spec/configuration_spec.rb b/spec/configuration_spec.rb index 79ad9d89e..3026943f2 100644 --- a/spec/configuration_spec.rb +++ b/spec/configuration_spec.rb @@ -146,4 +146,35 @@ expect(Clearance.configuration.reload_user_model).to be_nil end end + + describe "#message_verifier" do + it "returns the configured verifier if one has been configured" do + verifier = Class.new.new + Clearance.configure { |config| config.message_verifier = verifier } + + expect(Clearance.configuration.message_verifier).to be verifier + end + + it "returns an active support message verifier instance by default" do + Clearance.configuration = Clearance::Configuration.new + + expect(Clearance.configuration.message_verifier).to be_an_instance_of( + ActiveSupport::MessageVerifier, + ) + end + end + + describe "#password_reset_time_limit" do + it "is the configured amount if overridden" do + Clearance.configure { |config| config.password_reset_time_limit = 1.hour } + + expect(Clearance.configuration.password_reset_time_limit).to eq 1.hour + end + + it "is 15 minutes by default" do + Clearance.configuration = Clearance::Configuration.new + + expect(Clearance.configuration.password_reset_time_limit).to eq 15.minutes + end + end end diff --git a/spec/controllers/passwords_controller_spec.rb b/spec/controllers/passwords_controller_spec.rb index 9d1fe112a..da44fac6d 100644 --- a/spec/controllers/passwords_controller_spec.rb +++ b/spec/controllers/passwords_controller_spec.rb @@ -16,14 +16,6 @@ describe "#create" do context "email corresponds to an existing user" do - it "generates a password change token" do - user = create(:user) - - post :create, password: { email: user.email.upcase } - - expect(user.reload.confirmation_token).not_to be_nil - end - it "sends the password reset email" do ActionMailer::Base.deliveries.clear user = create(:user) @@ -59,9 +51,9 @@ describe "#edit" do context "valid id and token are supplied" do it "renders the password form for the user" do - user = create(:user, :with_forgotten_password) + user = create(:user) - get :edit, user_id: user, token: user.confirmation_token + get :edit, user_id: user, token: token_for(user) expect(response).to be_success expect(response).to render_template(:edit) @@ -74,18 +66,43 @@ get :edit, user_id: 1, token: "" expect(response).to render_template(:new) - expect(flash.now[:alert]).to match(/double check the URL/i) + expect(flash.now[:alert]).to match(/expired or is invalid/i) end end context "invalid token is supplied" do it "renders the new password reset form with a flash alert" do - user = create(:user, :with_forgotten_password) + get :edit, user_id: 1, token: "a" + + expect(response).to render_template(:new) + expect(flash.now[:alert]).to match(/expired or is invalid/i) + end + end + + context "expired token is supplied" do + it "does not allow password reset to continue" do + user = create(:user) + token = token_for(user) - get :edit, user_id: 1, token: user.confirmation_token + "a" + Timecop.freeze(1.day.from_now) do + get :edit, user_id: 1, token: token + + expect(response).to render_template(:new) + expect(flash.now[:alert]).to match(/expired or is invalid/i) + end + end + end + + context "password is changed before reset is complete" do + it "does not allow password reset to continue" do + user = create(:user) + token = token_for(user) + user.update_password("foobar") + + get :edit, user_id: 1, token: token expect(response).to render_template(:new) - expect(flash.now[:alert]).to match(/double check the URL/i) + expect(flash.now[:alert]).to match(/expired or is invalid/i) end end end @@ -93,7 +110,7 @@ describe "#update" do context "valid id, token, and new password provided" do it "updates the user's password" do - user = create(:user, :with_forgotten_password) + user = create(:user) old_encrypted_password = user.encrypted_password put :update, update_parameters(user, new_password: "my_new_password") @@ -102,7 +119,7 @@ end it "signs the user in and redirects" do - user = create(:user, :with_forgotten_password) + user = create(:user) put :update, update_parameters(user, new_password: "my_new_password") @@ -113,18 +130,17 @@ context "password update fails" do it "does not update the password" do - user = create(:user, :with_forgotten_password) + user = create(:user) old_encrypted_password = user.encrypted_password put :update, update_parameters(user, new_password: "") user.reload expect(user.encrypted_password).to eq old_encrypted_password - expect(user.confirmation_token).to be_present end it "re-renders the password edit form" do - user = create(:user, :with_forgotten_password) + user = create(:user) put :update, update_parameters(user, new_password: "") @@ -140,8 +156,16 @@ def update_parameters(user, options = {}) { user_id: user, - token: user.confirmation_token, - password_reset: { password: new_password } + token: token_for(user), + password_reset: { password: new_password }, } end + + def token_for(user) + Clearance.configuration.message_verifier.generate([ + user.id, + user.encrypted_password, + 15.minutes.from_now, + ]) + end end diff --git a/spec/factories.rb b/spec/factories.rb index 204620e6a..9ccab6cc1 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -7,10 +7,6 @@ email password 'password' - trait :with_forgotten_password do - confirmation_token Clearance::Token.new - end - factory :user_with_optional_password, class: 'UserWithOptionalPassword' do password nil encrypted_password '' diff --git a/spec/generators/clearance/upgrade/upgrade_generator_spec.rb b/spec/generators/clearance/upgrade/upgrade_generator_spec.rb new file mode 100644 index 000000000..674092779 --- /dev/null +++ b/spec/generators/clearance/upgrade/upgrade_generator_spec.rb @@ -0,0 +1,16 @@ +require "spec_helper" +require "generators/clearance/upgrade/upgrade_generator" + +describe Clearance::Generators::UpgradeGenerator, :generator do + it "copies a database migration to the host application" do + run_generator + + migration = migration_file( + "db/migrate/remove_confirmation_token_from_users.rb", + ) + + expect(migration).to exist + expect(migration).to have_correct_syntax + expect(migration).to contain("remove_column :users, :confirmation_token") + end +end diff --git a/spec/mailers/clearance_mailer_spec.rb b/spec/mailers/clearance_mailer_spec.rb index 81fae81fd..6d511efcd 100644 --- a/spec/mailers/clearance_mailer_spec.rb +++ b/spec/mailers/clearance_mailer_spec.rb @@ -3,7 +3,6 @@ describe ClearanceMailer do it "is from DO_NOT_REPLY" do user = create(:user) - user.forgot_password! email = ClearanceMailer.change_password(user) @@ -12,7 +11,6 @@ it "is sent to user" do user = create(:user) - user.forgot_password! email = ClearanceMailer.change_password(user) @@ -21,7 +19,6 @@ it "sets its subject" do user = create(:user) - user.forgot_password! email = ClearanceMailer.change_password(user) @@ -30,7 +27,6 @@ it "has html and plain text parts" do user = create(:user) - user.forgot_password! email = ClearanceMailer.change_password(user) @@ -40,19 +36,23 @@ end it "contains a link to edit the password" do - user = create(:user) - user.forgot_password! - host = ActionMailer::Base.default_url_options[:host] - link = "http://#{host}/users/#{user.id}/password/edit" \ - "?token=#{user.confirmation_token}" - - email = ClearanceMailer.change_password(user) - - expect(email.text_part.body).to include(link) - expect(email.html_part.body).to include(link) - expect(email.html_part.body).to have_css( - "a", - text: I18n.t("clearance_mailer.change_password.link_text") - ) + Timecop.freeze do + user = create(:user) + host = ActionMailer::Base.default_url_options[:host] + allow(Clearance.configuration.message_verifier).to receive(:generate). + with([user.id, user.encrypted_password, 15.minutes.from_now]). + and_return("THIS_IS_THE_TOKEN") + link = "http://#{host}/users/#{user.id}/password/edit" \ + "?token=THIS_IS_THE_TOKEN" + + email = ClearanceMailer.change_password(user) + + expect(email.text_part.body).to include(link) + expect(email.html_part.body).to include(link) + expect(email.html_part.body).to have_css( + "a", + text: I18n.t("clearance_mailer.change_password.link_text"), + ) + end end end diff --git a/spec/user_spec.rb b/spec/user_spec.rb index e3a1238e7..07bae51dc 100644 --- a/spec/user_spec.rb +++ b/spec/user_spec.rb @@ -70,7 +70,7 @@ describe "#update_password" do context "with a valid password" do it "changes the encrypted password" do - user = create(:user, :with_forgotten_password) + user = create(:user) old_encrypted_password = user.encrypted_password user.update_password("new_password") @@ -78,16 +78,8 @@ expect(user.encrypted_password).not_to eq old_encrypted_password end - it "clears the confirmation token" do - user = create(:user, :with_forgotten_password) - - user.update_password("new_password") - - expect(user.confirmation_token).to be_nil - end - it "sets the remember token" do - user = create(:user, :with_forgotten_password) + user = create(:user) user.update_password("my_new_password") @@ -98,21 +90,13 @@ context "with blank password" do it "does not change the encrypted password" do - user = create(:user, :with_forgotten_password) + user = create(:user) old_encrypted_password = user.encrypted_password user.update_password("") expect(user.encrypted_password.to_s).to eq old_encrypted_password end - - it "does not clear the confirmation token" do - user = create(:user, :with_forgotten_password) - - user.update_password("") - - expect(user.confirmation_token).not_to be_nil - end end end @@ -129,16 +113,6 @@ end end - describe "#forgot_password!" do - it "generates the confirmation token" do - user = create(:user, confirmation_token: nil) - - user.forgot_password! - - expect(user.confirmation_token).not_to be_nil - end - end - describe "a user with an optional email" do subject { user } From c3426a21960d2685abc478839bd25764a4c20a68 Mon Sep 17 00:00:00 2001 From: Derek Prior Date: Fri, 20 May 2016 16:33:34 -0400 Subject: [PATCH 12/13] Extract PasswordResetToken This cleans up some of the duplication of knowledge for how password reset tokens are generated and allows us to move tests for the various ways a reset token can be invalid into unit tests. --- .../clearance/passwords_controller.rb | 26 ++-------- app/mailers/clearance_mailer.rb | 6 +-- lib/clearance.rb | 1 + lib/clearance/password_reset_token.rb | 42 +++++++++++++++ .../visitor_updates_password_spec.rb.tt | 6 +-- spec/clearance/password_reset_token_spec.rb | 51 +++++++++++++++++++ spec/controllers/passwords_controller_spec.rb | 42 +-------------- spec/mailers/clearance_mailer_spec.rb | 4 +- 8 files changed, 102 insertions(+), 76 deletions(-) create mode 100644 lib/clearance/password_reset_token.rb create mode 100644 spec/clearance/password_reset_token_spec.rb diff --git a/app/controllers/clearance/passwords_controller.rb b/app/controllers/clearance/passwords_controller.rb index a81a4be8f..fb8205735 100644 --- a/app/controllers/clearance/passwords_controller.rb +++ b/app/controllers/clearance/passwords_controller.rb @@ -17,13 +17,12 @@ def create end def edit - @user = find_user_for_edit - + @user = find_user_by_password_reset_token(params[:token]) render template: "passwords/edit" end def update - @user = find_user_for_update + @user = find_user_by_password_reset_token(params[:token]) if @user.update_password(password_reset_params) sign_in(@user) @@ -50,26 +49,7 @@ def find_user_for_create end def find_user_by_password_reset_token(token) - verifier = Clearance.configuration.message_verifier - user_model = Clearance.configuration.user_model - - begin - user_id, encrypted_password, expiration = verifier.verify(token) - rescue ActiveSupport::MessageVerifier::InvalidSignature - expiration = 1.day.ago - end - - if expiration.future? - user_model.find_by(id: user_id, encrypted_password: encrypted_password) - end - end - - def find_user_for_edit - find_user_by_password_reset_token(params[:token]) - end - - def find_user_for_update - find_user_by_password_reset_token(params[:token]) + @user ||= Clearance::PasswordResetToken.new(token).user end def ensure_existing_user diff --git a/app/mailers/clearance_mailer.rb b/app/mailers/clearance_mailer.rb index 5749e4f6d..65adbe123 100644 --- a/app/mailers/clearance_mailer.rb +++ b/app/mailers/clearance_mailer.rb @@ -16,10 +16,6 @@ def change_password(user) private def generate_password_reset_token(user) - Clearance.configuration.message_verifier.generate([ - user.id, - user.encrypted_password, - Clearance.configuration.password_reset_time_limit.from_now, - ]) + Clearance::PasswordResetToken.generate_for(user) end end diff --git a/lib/clearance.rb b/lib/clearance.rb index 1b92beebe..62d03a508 100644 --- a/lib/clearance.rb +++ b/lib/clearance.rb @@ -8,6 +8,7 @@ require 'clearance/engine' require 'clearance/password_strategies' require 'clearance/constraints' +require "clearance/password_reset_token" module Clearance end diff --git a/lib/clearance/password_reset_token.rb b/lib/clearance/password_reset_token.rb new file mode 100644 index 000000000..1748e4ec8 --- /dev/null +++ b/lib/clearance/password_reset_token.rb @@ -0,0 +1,42 @@ +module Clearance + class PasswordResetToken + def self.generate_for(user) + token = Clearance.configuration.message_verifier.generate([ + user.id, + user.encrypted_password, + Clearance.configuration.password_reset_time_limit.from_now + ]) + + new(token) + end + + def initialize(token) + @token = token.to_s + end + + def user + user_id, encrypted_password, expires_at = verify + + if expires_at.future? + Clearance.configuration.user_model.find_by( + id: user_id, + encrypted_password: encrypted_password + ) + end + end + + def to_s + token + end + + private + + attr_reader :token + + def verify + Clearance.configuration.message_verifier.verify(token) + rescue ActiveSupport::MessageVerifier::InvalidSignature + [nil, nil, 1.hour.ago] + end + end +end diff --git a/lib/generators/clearance/specs/templates/features/clearance/visitor_updates_password_spec.rb.tt b/lib/generators/clearance/specs/templates/features/clearance/visitor_updates_password_spec.rb.tt index 2895b9fbf..9fd956c78 100644 --- a/lib/generators/clearance/specs/templates/features/clearance/visitor_updates_password_spec.rb.tt +++ b/lib/generators/clearance/specs/templates/features/clearance/visitor_updates_password_spec.rb.tt @@ -42,11 +42,7 @@ RSpec.feature "Visitor updates password" do end def token_for(user) - Clearance.configuration.message_verifier.generate([ - user.id, - user.encrypted_password, - Clearance.configuration.password_reset_time_limit.from_now, - ]) + Clearance::PasswordResetToken.generate_for(user) end def change_password_to(password) diff --git a/spec/clearance/password_reset_token_spec.rb b/spec/clearance/password_reset_token_spec.rb new file mode 100644 index 000000000..fc9a2f368 --- /dev/null +++ b/spec/clearance/password_reset_token_spec.rb @@ -0,0 +1,51 @@ +require "spec_helper" + +describe Clearance::PasswordResetToken do + describe ".generate_for" do + it "uses the configured message verifier to generate a token" do + Timecop.freeze do + user = double("User", id: 1, encrypted_password: "foo") + time_limit = Clearance.configuration.password_reset_time_limit + allow(Clearance.configuration.message_verifier).to receive(:generate). + with([1, "foo", time_limit.from_now]). + and_return("SEKRET") + + token = Clearance::PasswordResetToken.generate_for(user) + + expect(token.to_s).to eq "SEKRET" + end + end + end + + describe "#user" do + it "is the user if signature is valid and token is not expired" do + user = create(:user) + token = Clearance::PasswordResetToken.generate_for(user) + + expect(token.user).to eq user + end + + it "is nil if the signature is invalid" do + token = Clearance::PasswordResetToken.new("FOO") + + expect(token.user).to be nil + end + + it "is nil if the signature is valid but expired" do + user = double("User", id: 1, encrypted_password: "foo") + token = Clearance::PasswordResetToken.generate_for(user) + + Timecop.freeze(1.day.from_now) do + expect(token.user).to be nil + end + end + + it "is nil if the signature is valid an unexpired by password changed" do + user = create(:user, encrypted_password: "something") + token = Clearance::PasswordResetToken.generate_for(user) + user.update!(encrypted_password: "something_else") + + expect(token.user).to be nil + end + end +end diff --git a/spec/controllers/passwords_controller_spec.rb b/spec/controllers/passwords_controller_spec.rb index da44fac6d..5b67fbf7e 100644 --- a/spec/controllers/passwords_controller_spec.rb +++ b/spec/controllers/passwords_controller_spec.rb @@ -61,15 +61,6 @@ end end - context "blank token is supplied" do - it "renders the new password reset form with a flash alert" do - get :edit, user_id: 1, token: "" - - expect(response).to render_template(:new) - expect(flash.now[:alert]).to match(/expired or is invalid/i) - end - end - context "invalid token is supplied" do it "renders the new password reset form with a flash alert" do get :edit, user_id: 1, token: "a" @@ -78,33 +69,6 @@ expect(flash.now[:alert]).to match(/expired or is invalid/i) end end - - context "expired token is supplied" do - it "does not allow password reset to continue" do - user = create(:user) - token = token_for(user) - - Timecop.freeze(1.day.from_now) do - get :edit, user_id: 1, token: token - - expect(response).to render_template(:new) - expect(flash.now[:alert]).to match(/expired or is invalid/i) - end - end - end - - context "password is changed before reset is complete" do - it "does not allow password reset to continue" do - user = create(:user) - token = token_for(user) - user.update_password("foobar") - - get :edit, user_id: 1, token: token - - expect(response).to render_template(:new) - expect(flash.now[:alert]).to match(/expired or is invalid/i) - end - end end describe "#update" do @@ -162,10 +126,6 @@ def update_parameters(user, options = {}) end def token_for(user) - Clearance.configuration.message_verifier.generate([ - user.id, - user.encrypted_password, - 15.minutes.from_now, - ]) + Clearance::PasswordResetToken.generate_for(user) end end diff --git a/spec/mailers/clearance_mailer_spec.rb b/spec/mailers/clearance_mailer_spec.rb index 6d511efcd..c18d4cdbc 100644 --- a/spec/mailers/clearance_mailer_spec.rb +++ b/spec/mailers/clearance_mailer_spec.rb @@ -39,8 +39,8 @@ Timecop.freeze do user = create(:user) host = ActionMailer::Base.default_url_options[:host] - allow(Clearance.configuration.message_verifier).to receive(:generate). - with([user.id, user.encrypted_password, 15.minutes.from_now]). + allow(Clearance::PasswordResetToken).to receive(:generate_for). + with(user). and_return("THIS_IS_THE_TOKEN") link = "http://#{host}/users/#{user.id}/password/edit" \ "?token=THIS_IS_THE_TOKEN" From b88990cb48ebaff7fd16cb0a06039de40dc3f70e Mon Sep 17 00:00:00 2001 From: Derek Prior Date: Fri, 20 May 2016 16:42:12 -0400 Subject: [PATCH 13/13] Use digested encrypted password in reset tokens We were previously using the encrypted password as part of the signed password reset token. Theoretically, emailing this token out could expose the encrypted password to some adversary who would then be able to do offline attacks against it. This would likely not be very successful, but in an abundance of caution, this change exposes an MD5'd version of the encrypted password instead. --- lib/clearance/password_reset_token.rb | 13 +++++++------ spec/clearance/password_reset_token_spec.rb | 2 +- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/lib/clearance/password_reset_token.rb b/lib/clearance/password_reset_token.rb index 1748e4ec8..1adb65cfe 100644 --- a/lib/clearance/password_reset_token.rb +++ b/lib/clearance/password_reset_token.rb @@ -3,7 +3,7 @@ class PasswordResetToken def self.generate_for(user) token = Clearance.configuration.message_verifier.generate([ user.id, - user.encrypted_password, + Digest::MD5.hexdigest(user.encrypted_password), Clearance.configuration.password_reset_time_limit.from_now ]) @@ -15,13 +15,14 @@ def initialize(token) end def user - user_id, encrypted_password, expires_at = verify + user_id, digested_secret, expires_at = verify if expires_at.future? - Clearance.configuration.user_model.find_by( - id: user_id, - encrypted_password: encrypted_password - ) + user = Clearance.configuration.user_model.find_by(id: user_id) + + if digested_secret == Digest::MD5.hexdigest(user.encrypted_password) + user + end end end diff --git a/spec/clearance/password_reset_token_spec.rb b/spec/clearance/password_reset_token_spec.rb index fc9a2f368..298c467f6 100644 --- a/spec/clearance/password_reset_token_spec.rb +++ b/spec/clearance/password_reset_token_spec.rb @@ -7,7 +7,7 @@ user = double("User", id: 1, encrypted_password: "foo") time_limit = Clearance.configuration.password_reset_time_limit allow(Clearance.configuration.message_verifier).to receive(:generate). - with([1, "foo", time_limit.from_now]). + with([1, Digest::MD5.hexdigest("foo"), time_limit.from_now]). and_return("SEKRET") token = Clearance::PasswordResetToken.generate_for(user)