From 695cdc4f05bf8e2619a5b753e6e1b1aaaa89d359 Mon Sep 17 00:00:00 2001 From: Robert Richter Date: Sun, 19 Apr 2015 13:33:19 -0500 Subject: [PATCH 01/10] Migrate to built-in ActiveModel secure passwords --- Gemfile | 1 + Gemfile.lock | 2 + app/controllers/application_controller.rb | 30 +- app/controllers/sessions_controller.rb | 16 +- app/controllers/users_controller.rb | 3 +- app/models/user.rb | 13 +- ...0419134141_add_password_digest_to_users.rb | 7 + db/schema.rb | 279 +++++++++--------- db/seeds.rb | 6 +- test/controllers/sessions_controller_test.rb | 24 ++ test/fixtures/users.yml | 9 +- test/models/user_secure_password_test.rb | 86 ++++++ test/models/user_test.rb | 3 +- 13 files changed, 312 insertions(+), 167 deletions(-) create mode 100644 db/migrate/20150419134141_add_password_digest_to_users.rb create mode 100644 test/models/user_secure_password_test.rb diff --git a/Gemfile b/Gemfile index d3aaaaf1..f3f05ebf 100644 --- a/Gemfile +++ b/Gemfile @@ -2,6 +2,7 @@ source 'https://rubygems.org' +gem 'bcrypt', '~> 3.1.7' gem 'rails', '4.1.0' gem 'jquery-rails' gem 'sass-rails', '~> 4.0.3', require: 'sass' diff --git a/Gemfile.lock b/Gemfile.lock index 778500c1..26a23df9 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -36,6 +36,7 @@ GEM aws-sdk-v1 (1.63.0) json (~> 1.4) nokogiri (>= 1.4.4) + bcrypt (3.1.10-x86-mingw32) bootplus-rails (1.0.0) builder (3.2.2) capybara (2.4.4) @@ -199,6 +200,7 @@ PLATFORMS DEPENDENCIES aws-sdk (~> 1.50) + bcrypt (~> 3.1.7) bootplus-rails capybara coffee-rails (~> 4.0.0) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 032465a5..7fa3b922 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -46,17 +46,27 @@ class ApplicationController < ActionController::Base def create_anonymous_account_if_not_logged_in return if logged_in? - @user = create_anonymous_user + # layman's collision detection + 10.times do + @user = create_anonymous_user + + next unless @user.save - if @user.save session[:user] = @user.id session[:anon_user] = true - else - # layman's collision detection - create_anonymous_account_if_not_logged_in end end + def create_anonymous_user + # TODO: guarantee anonymous id is random (or just let db assign it?) + id = rand(10_000_000).to_s + rand(10_000_000).to_s # lol + + User.new( + name: 'Anonymous-' + id.to_s, + email: id.to_s + '@localhost', + password: id.to_s) + end + # Require ownership def require_ownership_of_character @@ -112,16 +122,6 @@ class ApplicationController < ActionController::Base private - def create_anonymous_user - # TODO: guarantee anonymous id is random (or just let db assign it?) - id = rand(10_000_000).to_s + rand(10_000_000).to_s # lol - - User.new( - name: 'Anonymous-' + id.to_s, - email: id.to_s + '@localhost', - password: id.to_s) - end - def redirect_if_not_owned(object_to_check, redirect_path) return if owned_by_current_user? object_to_check redirect_to redirect_path, notice: t(:no_do_permission) diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 0f7c0311..4d3a651c 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -50,9 +50,19 @@ class SessionsController < ApplicationController def user_from_params login = Session.new(session_params) - hash = SessionsController.create_password_digest(login.username, - login.password) - User.where(name: login.username, password: hash).first + user = User.find_by(name: login.username) + migrate_to_bcrypt(user, login.password) if user.old_password.present? + user.try(:authenticate, login.password) + end + + def migrate_to_bcrypt(user, password) + hash = SessionsController.create_password_digest user.name, password + + return unless user.old_password == hash + + user.old_password = nil + user.password = password + user.save end def build_session_for(user) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 31b08532..a0cea86e 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -75,6 +75,7 @@ class UsersController < ApplicationController private def user_params - params.require(:user).permit(:name, :password, :email) + params.require(:user).permit(:name, :email, + :password, :password_confirmation) end end diff --git a/app/models/user.rb b/app/models/user.rb index d5d9274a..21fd5851 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -2,10 +2,10 @@ # a person using the Indent web application. Owns all other content. class User < ActiveRecord::Base validates :name, presence: true - validates :password, presence: true validates :email, presence: true - before_save :hash_password + has_secure_password + before_save :hash_old_password has_many :characters has_many :equipment @@ -14,10 +14,13 @@ class User < ActiveRecord::Base has_many :magics has_many :universes - def hash_password + def hash_old_password require 'digest' - self.password = Digest::MD5.hexdigest( - name + "'s password IS... " + password + ' (lol!)') + + return if old_password.blank? + + self.old_password = Digest::MD5.hexdigest( + name + "'s password IS... " + old_password + ' (lol!)') end def content diff --git a/db/migrate/20150419134141_add_password_digest_to_users.rb b/db/migrate/20150419134141_add_password_digest_to_users.rb new file mode 100644 index 00000000..17b82b5a --- /dev/null +++ b/db/migrate/20150419134141_add_password_digest_to_users.rb @@ -0,0 +1,7 @@ +class AddPasswordDigestToUsers < ActiveRecord::Migration + def change + rename_column :users, :password, :old_password + change_column_null :users, :old_password, true + add_column :users, :password_digest, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index 37a8cd46..dd669321 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,157 +11,160 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20_140_713_043_535) do - create_table 'characters', force: true do |t| - t.string 'name', null: false - t.string 'role' - t.string 'gender' - t.string 'age' - t.string 'height' - t.string 'weight' - t.string 'haircolor' - t.string 'hairstyle' - t.string 'facialhair' - t.string 'eyecolor' - t.string 'race' - t.string 'skintone' - t.string 'bodytype' - t.string 'identmarks' - t.text 'bestfriend' - t.text 'religion' - t.text 'politics' - t.text 'prejudices' - t.text 'occupation' - t.text 'pets' - t.text 'mannerisms' - t.text 'birthday' - t.text 'birthplace' - t.text 'education' - t.text 'background' - t.string 'fave_color' - t.string 'fave_food' - t.string 'fave_possession' - t.string 'fave_weapon' - t.string 'fave_animal' - t.text 'father' - t.text 'mother' - t.text 'spouse' - t.text 'siblings' - t.text 'archenemy' - t.text 'notes' - t.text 'private_notes' - t.integer 'user_id' - t.integer 'universe_id' - t.datetime 'created_at' - t.datetime 'updated_at' +ActiveRecord::Schema.define(version: 20150419134141) do + + create_table "characters", force: true do |t| + t.string "name", null: false + t.string "role" + t.string "gender" + t.string "age" + t.string "height" + t.string "weight" + t.string "haircolor" + t.string "hairstyle" + t.string "facialhair" + t.string "eyecolor" + t.string "race" + t.string "skintone" + t.string "bodytype" + t.string "identmarks" + t.text "bestfriend" + t.text "religion" + t.text "politics" + t.text "prejudices" + t.text "occupation" + t.text "pets" + t.text "mannerisms" + t.text "birthday" + t.text "birthplace" + t.text "education" + t.text "background" + t.string "fave_color" + t.string "fave_food" + t.string "fave_possession" + t.string "fave_weapon" + t.string "fave_animal" + t.text "father" + t.text "mother" + t.text "spouse" + t.text "siblings" + t.text "archenemy" + t.text "notes" + t.text "private_notes" + t.integer "user_id" + t.integer "universe_id" + t.datetime "created_at" + t.datetime "updated_at" end - create_table 'equipment', force: true do |t| - t.string 'name', null: false - t.string 'equip_type' - t.text 'description' - t.string 'weight' - t.string 'original_owner' - t.string 'current_owner' - t.text 'made_by' - t.text 'materials' - t.string 'year_made' - t.text 'magic' - t.text 'notes' - t.text 'private_notes' - t.integer 'user_id' - t.integer 'universe_id' - t.datetime 'created_at' - t.datetime 'updated_at' + create_table "equipment", force: true do |t| + t.string "name", null: false + t.string "equip_type" + t.text "description" + t.string "weight" + t.string "original_owner" + t.string "current_owner" + t.text "made_by" + t.text "materials" + t.string "year_made" + t.text "magic" + t.text "notes" + t.text "private_notes" + t.integer "user_id" + t.integer "universe_id" + t.datetime "created_at" + t.datetime "updated_at" end - create_table 'languages', force: true do |t| - t.string 'name', null: false - t.text 'words' - t.string 'established_year' - t.string 'established_location' - t.text 'characters' - t.text 'locations' - t.text 'notes' - t.integer 'user_id' - t.integer 'universe_id' - t.datetime 'created_at' - t.datetime 'updated_at' + create_table "languages", force: true do |t| + t.string "name", null: false + t.text "words" + t.string "established_year" + t.string "established_location" + t.text "characters" + t.text "locations" + t.text "notes" + t.integer "user_id" + t.integer "universe_id" + t.datetime "created_at" + t.datetime "updated_at" end - create_table 'locations', force: true do |t| - t.string 'name', null: false - t.string 'type_of' - t.text 'description' - t.string 'map_file_name' - t.string 'map_content_type' - t.integer 'map_file_size' - t.datetime 'map_updated_at' - t.string 'population' - t.string 'language' - t.string 'currency' - t.string 'motto' - t.text 'capital' - t.text 'largest_city' - t.text 'notable_cities' - t.text 'area' - t.text 'crops' - t.text 'located_at' - t.string 'established_year' - t.text 'notable_wars' - t.text 'notes' - t.text 'private_notes' - t.integer 'user_id' - t.integer 'universe_id' - t.datetime 'created_at' - t.datetime 'updated_at' + create_table "locations", force: true do |t| + t.string "name", null: false + t.string "type_of" + t.text "description" + t.string "map_file_name" + t.string "map_content_type" + t.integer "map_file_size" + t.datetime "map_updated_at" + t.string "population" + t.string "language" + t.string "currency" + t.string "motto" + t.text "capital" + t.text "largest_city" + t.text "notable_cities" + t.text "area" + t.text "crops" + t.text "located_at" + t.string "established_year" + t.text "notable_wars" + t.text "notes" + t.text "private_notes" + t.integer "user_id" + t.integer "universe_id" + t.datetime "created_at" + t.datetime "updated_at" end - create_table 'magics', force: true do |t| - t.string 'name', null: false - t.string 'type_of' - t.text 'manifestation' - t.text 'symptoms' - t.string 'element' - t.string 'diety' - t.text 'harmfulness' - t.text 'helpfulness' - t.text 'neutralness' - t.text 'resource' - t.text 'skill_level' - t.text 'limitations' - t.text 'notes' - t.text 'private_notes' - t.integer 'user_id' - t.integer 'universe_id' - t.datetime 'created_at' - t.datetime 'updated_at' + create_table "magics", force: true do |t| + t.string "name", null: false + t.string "type_of" + t.text "manifestation" + t.text "symptoms" + t.string "element" + t.string "diety" + t.text "harmfulness" + t.text "helpfulness" + t.text "neutralness" + t.text "resource" + t.text "skill_level" + t.text "limitations" + t.text "notes" + t.text "private_notes" + t.integer "user_id" + t.integer "universe_id" + t.datetime "created_at" + t.datetime "updated_at" end - create_table 'sessions', force: true do |t| - t.string 'username', null: false - t.string 'password', null: false - t.datetime 'created_at' - t.datetime 'updated_at' + create_table "sessions", force: true do |t| + t.string "username", null: false + t.string "password", null: false + t.datetime "created_at" + t.datetime "updated_at" end - create_table 'universes', force: true do |t| - t.string 'name', null: false - t.text 'description' - t.text 'history' - t.text 'notes' - t.text 'private_notes' - t.string 'privacy' - t.integer 'user_id' - t.datetime 'created_at' - t.datetime 'updated_at' + create_table "universes", force: true do |t| + t.string "name", null: false + t.text "description" + t.text "history" + t.text "notes" + t.text "private_notes" + t.string "privacy" + t.integer "user_id" + t.datetime "created_at" + t.datetime "updated_at" end - create_table 'users', force: true do |t| - t.string 'name', null: false - t.string 'email', null: false - t.string 'password', null: false - t.datetime 'created_at' - t.datetime 'updated_at' + create_table "users", force: true do |t| + t.string "name", null: false + t.string "email", null: false + t.string "old_password" + t.datetime "created_at" + t.datetime "updated_at" + t.string "password_digest" end + end diff --git a/db/seeds.rb b/db/seeds.rb index 6a8069d2..79385f0d 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -4,9 +4,9 @@ tolkien = User.create(name: 'JRRTolkien', email: 'tolkien@example.com', password: 'Mellon') -Universe.create(name: 'Middle-Earth', - user: tolkien, - privacy: 'public') +middleearth = Universe.create(name: 'Middle-Earth', + user: tolkien, + privacy: 'public') Character.create(name: 'Frodo Baggins', user: tolkien, diff --git a/test/controllers/sessions_controller_test.rb b/test/controllers/sessions_controller_test.rb index bd7a1a98..346b444e 100644 --- a/test/controllers/sessions_controller_test.rb +++ b/test/controllers/sessions_controller_test.rb @@ -2,4 +2,28 @@ require 'test_helper' # Tests for the SessionsController class class SessionsControllerTest < ActionController::TestCase + test 'old password migrates to bcrypt' do + post :create, session: { + username: users(:martin).name, + password: 'HODOR' + } + + migrated_martin = User.find_by(name: users(:martin).name) + .authenticate('HODOR') + + assert_not_nil migrated_martin, + 'Could not authenticate an older user using the '\ + 'new bcrypt scheme' + end + + test 'user\'s old password is cleared when migrated' do + post :create, session: { + username: users(:martin).name, + password: 'HODOR' + } + + old_password = User.find_by(name: users(:martin).name).old_password + + assert old_password.blank? + end end diff --git a/test/fixtures/users.yml b/test/fixtures/users.yml index 4ee8d3c3..2b444015 100644 --- a/test/fixtures/users.yml +++ b/test/fixtures/users.yml @@ -1,4 +1,11 @@ tolkien: name: JRRTolkien email: tolkien@example.com - password: <%= SessionsController.create_password_digest "JRRTolkien", "Mellon" %> + password_digest: <%= BCrypt::Password.create("Mellon") %> + + + +martin: + name: GRRMartin + email: martin@localhost + old_password: <%= require 'digest'; Digest::MD5.hexdigest("GRRMartin's password IS... HODOR (lol!)") %> diff --git a/test/models/user_secure_password_test.rb b/test/models/user_secure_password_test.rb new file mode 100644 index 00000000..804f4298 --- /dev/null +++ b/test/models/user_secure_password_test.rb @@ -0,0 +1,86 @@ +require 'test_helper' + +## +# Tests the entire ActiveModel::SecurePassword API implemented by the User model +# +# Taken from this demonstration: +# http://api.rubyonrails.org/classes/ActiveModel/SecurePassword/ClassMethods.html#method-i-has_secure_password +class UserSecurePasswordTest < ActiveSupport::TestCase + setup do + @username = 'ChrisTolkien' + @email = 'chris@localhost' + @rightpass = 'HiDad' + @wrongpass = 'Mellon' + end + + test 'cannot save user with mismatched password and confirmation' do + user = User.new(name: @username, + password: @rightpass, + password_confirmation: @wrongpass) + refute user.save, + 'Was able to save user despite mismatched password and confirmation' + end + + test 'user with correct password and confirmation is a valid model' do + user = User.new(name: @username, + email: @email, + password: @rightpass, + password_confirmation: @rightpass) + assert user.valid?, "User model was not valid with a matching password + and confirmation: #{user.errors.messages}" + end + + test 'can save user with matching password and confirmation' do + user = User.new(name: @username, + email: @email, + password: @rightpass, + password_confirmation: @rightpass) + + assert user.save, + 'Wasn\'t able to save user despite matching password '\ + 'and confirmation' + end + + test 'can save a user with correct password and no confirmation' do + user = User.new(name: @username, + email: @email, + password: @rightpass) + + assert user.save, 'Wasn\'t able to save user despite having a password' + end + + test 'cannot authenticate a user with the incorrect password' do + user = User.new(name: @username, password: @rightpass) + + refute user.authenticate(@wrongpass), + 'Was able to authenticate a user with an incorrect password' + end + + test 'can authenticate a user with the correct password' do + user = User.new(name: @username, + email: @email, + password: @rightpass) + + assert_not_nil user.authenticate(@rightpass), + 'Was not able to authenticate a user with a correct password' + end + + test 'can find a user and authenticate with the correct password' do + User.create(name: @username, + email: @email, + password: @rightpass) + + assert_not_nil User.find_by(name: @username).try(:authenticate, @rightpass), + 'Was not able to find & authenticate user with a '\ + 'correct password' + end + + test 'cannot find and authenticate with an incorrect password' do + User.create(name: @username, + email: @email, + password: @rightpass) + + refute User.find_by(name: @username).try(:authenticate, @wrongpass), + 'Was able to find & authenticate a user with an incorrect password' + end +end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 69241296..f2889d98 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -25,7 +25,8 @@ class UserTest < ActiveSupport::TestCase test 'user fixture assumptions' do assert_not_nil users(:tolkien), 'User fixture is unavailable' - assert users(:tolkien).valid?, 'User fixture is not a valid user' + assert users(:tolkien).valid?, "User fixture is not a valid user: + #{users(:tolkien).errors.messages}" end test 'can get user content' do From 39d7d05698765cdb2471cd9fcccb766e043ba712 Mon Sep 17 00:00:00 2001 From: Robert Richter Date: Sun, 19 Apr 2015 14:07:18 -0500 Subject: [PATCH 02/10] Filter sensitive data from logs --- config/application.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/config/application.rb b/config/application.rb index 9019ef92..2e72476b 100644 --- a/config/application.rb +++ b/config/application.rb @@ -7,6 +7,7 @@ require 'rails/all' Bundler.require(*Rails.groups) module PlanCharacters + # Default application environment configurations class Application < Rails::Application # Settings in config/environments/* take precedence over those # specified here. @@ -25,5 +26,9 @@ module PlanCharacters # config.i18n.load_path += Dir[Rails.root.join( # 'my', 'locales', '*.{rb,yml}').to_s] # config.i18n.default_locale = :de + + # Filter sensitive parameters out of logs + config.filter_paramters << :password + config.filter_paramters << :password_confirmation end end From 34b5d56cdab2ace44077b10d33aa0783e4d38837 Mon Sep 17 00:00:00 2001 From: Robert Richter Date: Sun, 19 Apr 2015 14:11:24 -0500 Subject: [PATCH 03/10] Spell things correctly --- config/application.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/application.rb b/config/application.rb index 2e72476b..00d28cef 100644 --- a/config/application.rb +++ b/config/application.rb @@ -28,7 +28,7 @@ module PlanCharacters # config.i18n.default_locale = :de # Filter sensitive parameters out of logs - config.filter_paramters << :password - config.filter_paramters << :password_confirmation + config.filter_parameters << :password + config.filter_parameters << :password_confirmation end end From 676feb3c6ffe7fdaa7454db006130f2bf116c433 Mon Sep 17 00:00:00 2001 From: Robert Richter Date: Sun, 19 Apr 2015 14:23:39 -0500 Subject: [PATCH 04/10] Add bcrypt to Rails 4.2 Edge gemfile --- gemfiles/rails-4.2.edge.gemfile | 1 + 1 file changed, 1 insertion(+) diff --git a/gemfiles/rails-4.2.edge.gemfile b/gemfiles/rails-4.2.edge.gemfile index 636dddba..dfa1e74e 100644 --- a/gemfiles/rails-4.2.edge.gemfile +++ b/gemfiles/rails-4.2.edge.gemfile @@ -2,6 +2,7 @@ source 'https://rubygems.org' source 'https://rubygems.org' +gem 'bcrypt', '~> 3.1.7' gem 'rails', '4.1.0' gem 'jquery-rails' gem 'sass-rails', '~> 4.0.3', :require => 'sass' From a7eb5c62bcf2a9de5cc5c03e383da46c785ff7a7 Mon Sep 17 00:00:00 2001 From: Robert Richter Date: Sun, 19 Apr 2015 13:33:19 -0500 Subject: [PATCH 05/10] Migrate to built-in ActiveModel secure passwords --- Gemfile | 1 + Gemfile.lock | 2 + app/controllers/application_controller.rb | 30 +- app/controllers/sessions_controller.rb | 16 +- app/controllers/users_controller.rb | 3 +- app/models/user.rb | 13 +- ...0419134141_add_password_digest_to_users.rb | 7 + db/schema.rb | 279 +++++++++--------- test/controllers/sessions_controller_test.rb | 24 ++ test/fixtures/users.yml | 9 +- test/models/user_secure_password_test.rb | 86 ++++++ test/models/user_test.rb | 3 +- 12 files changed, 309 insertions(+), 164 deletions(-) create mode 100644 db/migrate/20150419134141_add_password_digest_to_users.rb create mode 100644 test/models/user_secure_password_test.rb diff --git a/Gemfile b/Gemfile index d3aaaaf1..f3f05ebf 100644 --- a/Gemfile +++ b/Gemfile @@ -2,6 +2,7 @@ source 'https://rubygems.org' +gem 'bcrypt', '~> 3.1.7' gem 'rails', '4.1.0' gem 'jquery-rails' gem 'sass-rails', '~> 4.0.3', require: 'sass' diff --git a/Gemfile.lock b/Gemfile.lock index 778500c1..26a23df9 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -36,6 +36,7 @@ GEM aws-sdk-v1 (1.63.0) json (~> 1.4) nokogiri (>= 1.4.4) + bcrypt (3.1.10-x86-mingw32) bootplus-rails (1.0.0) builder (3.2.2) capybara (2.4.4) @@ -199,6 +200,7 @@ PLATFORMS DEPENDENCIES aws-sdk (~> 1.50) + bcrypt (~> 3.1.7) bootplus-rails capybara coffee-rails (~> 4.0.0) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 032465a5..7fa3b922 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -46,17 +46,27 @@ class ApplicationController < ActionController::Base def create_anonymous_account_if_not_logged_in return if logged_in? - @user = create_anonymous_user + # layman's collision detection + 10.times do + @user = create_anonymous_user + + next unless @user.save - if @user.save session[:user] = @user.id session[:anon_user] = true - else - # layman's collision detection - create_anonymous_account_if_not_logged_in end end + def create_anonymous_user + # TODO: guarantee anonymous id is random (or just let db assign it?) + id = rand(10_000_000).to_s + rand(10_000_000).to_s # lol + + User.new( + name: 'Anonymous-' + id.to_s, + email: id.to_s + '@localhost', + password: id.to_s) + end + # Require ownership def require_ownership_of_character @@ -112,16 +122,6 @@ class ApplicationController < ActionController::Base private - def create_anonymous_user - # TODO: guarantee anonymous id is random (or just let db assign it?) - id = rand(10_000_000).to_s + rand(10_000_000).to_s # lol - - User.new( - name: 'Anonymous-' + id.to_s, - email: id.to_s + '@localhost', - password: id.to_s) - end - def redirect_if_not_owned(object_to_check, redirect_path) return if owned_by_current_user? object_to_check redirect_to redirect_path, notice: t(:no_do_permission) diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 0f7c0311..4d3a651c 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -50,9 +50,19 @@ class SessionsController < ApplicationController def user_from_params login = Session.new(session_params) - hash = SessionsController.create_password_digest(login.username, - login.password) - User.where(name: login.username, password: hash).first + user = User.find_by(name: login.username) + migrate_to_bcrypt(user, login.password) if user.old_password.present? + user.try(:authenticate, login.password) + end + + def migrate_to_bcrypt(user, password) + hash = SessionsController.create_password_digest user.name, password + + return unless user.old_password == hash + + user.old_password = nil + user.password = password + user.save end def build_session_for(user) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 31b08532..a0cea86e 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -75,6 +75,7 @@ class UsersController < ApplicationController private def user_params - params.require(:user).permit(:name, :password, :email) + params.require(:user).permit(:name, :email, + :password, :password_confirmation) end end diff --git a/app/models/user.rb b/app/models/user.rb index d5d9274a..21fd5851 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -2,10 +2,10 @@ # a person using the Indent web application. Owns all other content. class User < ActiveRecord::Base validates :name, presence: true - validates :password, presence: true validates :email, presence: true - before_save :hash_password + has_secure_password + before_save :hash_old_password has_many :characters has_many :equipment @@ -14,10 +14,13 @@ class User < ActiveRecord::Base has_many :magics has_many :universes - def hash_password + def hash_old_password require 'digest' - self.password = Digest::MD5.hexdigest( - name + "'s password IS... " + password + ' (lol!)') + + return if old_password.blank? + + self.old_password = Digest::MD5.hexdigest( + name + "'s password IS... " + old_password + ' (lol!)') end def content diff --git a/db/migrate/20150419134141_add_password_digest_to_users.rb b/db/migrate/20150419134141_add_password_digest_to_users.rb new file mode 100644 index 00000000..17b82b5a --- /dev/null +++ b/db/migrate/20150419134141_add_password_digest_to_users.rb @@ -0,0 +1,7 @@ +class AddPasswordDigestToUsers < ActiveRecord::Migration + def change + rename_column :users, :password, :old_password + change_column_null :users, :old_password, true + add_column :users, :password_digest, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index 37a8cd46..dd669321 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,157 +11,160 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20_140_713_043_535) do - create_table 'characters', force: true do |t| - t.string 'name', null: false - t.string 'role' - t.string 'gender' - t.string 'age' - t.string 'height' - t.string 'weight' - t.string 'haircolor' - t.string 'hairstyle' - t.string 'facialhair' - t.string 'eyecolor' - t.string 'race' - t.string 'skintone' - t.string 'bodytype' - t.string 'identmarks' - t.text 'bestfriend' - t.text 'religion' - t.text 'politics' - t.text 'prejudices' - t.text 'occupation' - t.text 'pets' - t.text 'mannerisms' - t.text 'birthday' - t.text 'birthplace' - t.text 'education' - t.text 'background' - t.string 'fave_color' - t.string 'fave_food' - t.string 'fave_possession' - t.string 'fave_weapon' - t.string 'fave_animal' - t.text 'father' - t.text 'mother' - t.text 'spouse' - t.text 'siblings' - t.text 'archenemy' - t.text 'notes' - t.text 'private_notes' - t.integer 'user_id' - t.integer 'universe_id' - t.datetime 'created_at' - t.datetime 'updated_at' +ActiveRecord::Schema.define(version: 20150419134141) do + + create_table "characters", force: true do |t| + t.string "name", null: false + t.string "role" + t.string "gender" + t.string "age" + t.string "height" + t.string "weight" + t.string "haircolor" + t.string "hairstyle" + t.string "facialhair" + t.string "eyecolor" + t.string "race" + t.string "skintone" + t.string "bodytype" + t.string "identmarks" + t.text "bestfriend" + t.text "religion" + t.text "politics" + t.text "prejudices" + t.text "occupation" + t.text "pets" + t.text "mannerisms" + t.text "birthday" + t.text "birthplace" + t.text "education" + t.text "background" + t.string "fave_color" + t.string "fave_food" + t.string "fave_possession" + t.string "fave_weapon" + t.string "fave_animal" + t.text "father" + t.text "mother" + t.text "spouse" + t.text "siblings" + t.text "archenemy" + t.text "notes" + t.text "private_notes" + t.integer "user_id" + t.integer "universe_id" + t.datetime "created_at" + t.datetime "updated_at" end - create_table 'equipment', force: true do |t| - t.string 'name', null: false - t.string 'equip_type' - t.text 'description' - t.string 'weight' - t.string 'original_owner' - t.string 'current_owner' - t.text 'made_by' - t.text 'materials' - t.string 'year_made' - t.text 'magic' - t.text 'notes' - t.text 'private_notes' - t.integer 'user_id' - t.integer 'universe_id' - t.datetime 'created_at' - t.datetime 'updated_at' + create_table "equipment", force: true do |t| + t.string "name", null: false + t.string "equip_type" + t.text "description" + t.string "weight" + t.string "original_owner" + t.string "current_owner" + t.text "made_by" + t.text "materials" + t.string "year_made" + t.text "magic" + t.text "notes" + t.text "private_notes" + t.integer "user_id" + t.integer "universe_id" + t.datetime "created_at" + t.datetime "updated_at" end - create_table 'languages', force: true do |t| - t.string 'name', null: false - t.text 'words' - t.string 'established_year' - t.string 'established_location' - t.text 'characters' - t.text 'locations' - t.text 'notes' - t.integer 'user_id' - t.integer 'universe_id' - t.datetime 'created_at' - t.datetime 'updated_at' + create_table "languages", force: true do |t| + t.string "name", null: false + t.text "words" + t.string "established_year" + t.string "established_location" + t.text "characters" + t.text "locations" + t.text "notes" + t.integer "user_id" + t.integer "universe_id" + t.datetime "created_at" + t.datetime "updated_at" end - create_table 'locations', force: true do |t| - t.string 'name', null: false - t.string 'type_of' - t.text 'description' - t.string 'map_file_name' - t.string 'map_content_type' - t.integer 'map_file_size' - t.datetime 'map_updated_at' - t.string 'population' - t.string 'language' - t.string 'currency' - t.string 'motto' - t.text 'capital' - t.text 'largest_city' - t.text 'notable_cities' - t.text 'area' - t.text 'crops' - t.text 'located_at' - t.string 'established_year' - t.text 'notable_wars' - t.text 'notes' - t.text 'private_notes' - t.integer 'user_id' - t.integer 'universe_id' - t.datetime 'created_at' - t.datetime 'updated_at' + create_table "locations", force: true do |t| + t.string "name", null: false + t.string "type_of" + t.text "description" + t.string "map_file_name" + t.string "map_content_type" + t.integer "map_file_size" + t.datetime "map_updated_at" + t.string "population" + t.string "language" + t.string "currency" + t.string "motto" + t.text "capital" + t.text "largest_city" + t.text "notable_cities" + t.text "area" + t.text "crops" + t.text "located_at" + t.string "established_year" + t.text "notable_wars" + t.text "notes" + t.text "private_notes" + t.integer "user_id" + t.integer "universe_id" + t.datetime "created_at" + t.datetime "updated_at" end - create_table 'magics', force: true do |t| - t.string 'name', null: false - t.string 'type_of' - t.text 'manifestation' - t.text 'symptoms' - t.string 'element' - t.string 'diety' - t.text 'harmfulness' - t.text 'helpfulness' - t.text 'neutralness' - t.text 'resource' - t.text 'skill_level' - t.text 'limitations' - t.text 'notes' - t.text 'private_notes' - t.integer 'user_id' - t.integer 'universe_id' - t.datetime 'created_at' - t.datetime 'updated_at' + create_table "magics", force: true do |t| + t.string "name", null: false + t.string "type_of" + t.text "manifestation" + t.text "symptoms" + t.string "element" + t.string "diety" + t.text "harmfulness" + t.text "helpfulness" + t.text "neutralness" + t.text "resource" + t.text "skill_level" + t.text "limitations" + t.text "notes" + t.text "private_notes" + t.integer "user_id" + t.integer "universe_id" + t.datetime "created_at" + t.datetime "updated_at" end - create_table 'sessions', force: true do |t| - t.string 'username', null: false - t.string 'password', null: false - t.datetime 'created_at' - t.datetime 'updated_at' + create_table "sessions", force: true do |t| + t.string "username", null: false + t.string "password", null: false + t.datetime "created_at" + t.datetime "updated_at" end - create_table 'universes', force: true do |t| - t.string 'name', null: false - t.text 'description' - t.text 'history' - t.text 'notes' - t.text 'private_notes' - t.string 'privacy' - t.integer 'user_id' - t.datetime 'created_at' - t.datetime 'updated_at' + create_table "universes", force: true do |t| + t.string "name", null: false + t.text "description" + t.text "history" + t.text "notes" + t.text "private_notes" + t.string "privacy" + t.integer "user_id" + t.datetime "created_at" + t.datetime "updated_at" end - create_table 'users', force: true do |t| - t.string 'name', null: false - t.string 'email', null: false - t.string 'password', null: false - t.datetime 'created_at' - t.datetime 'updated_at' + create_table "users", force: true do |t| + t.string "name", null: false + t.string "email", null: false + t.string "old_password" + t.datetime "created_at" + t.datetime "updated_at" + t.string "password_digest" end + end diff --git a/test/controllers/sessions_controller_test.rb b/test/controllers/sessions_controller_test.rb index bd7a1a98..346b444e 100644 --- a/test/controllers/sessions_controller_test.rb +++ b/test/controllers/sessions_controller_test.rb @@ -2,4 +2,28 @@ require 'test_helper' # Tests for the SessionsController class class SessionsControllerTest < ActionController::TestCase + test 'old password migrates to bcrypt' do + post :create, session: { + username: users(:martin).name, + password: 'HODOR' + } + + migrated_martin = User.find_by(name: users(:martin).name) + .authenticate('HODOR') + + assert_not_nil migrated_martin, + 'Could not authenticate an older user using the '\ + 'new bcrypt scheme' + end + + test 'user\'s old password is cleared when migrated' do + post :create, session: { + username: users(:martin).name, + password: 'HODOR' + } + + old_password = User.find_by(name: users(:martin).name).old_password + + assert old_password.blank? + end end diff --git a/test/fixtures/users.yml b/test/fixtures/users.yml index 4ee8d3c3..2b444015 100644 --- a/test/fixtures/users.yml +++ b/test/fixtures/users.yml @@ -1,4 +1,11 @@ tolkien: name: JRRTolkien email: tolkien@example.com - password: <%= SessionsController.create_password_digest "JRRTolkien", "Mellon" %> + password_digest: <%= BCrypt::Password.create("Mellon") %> + + + +martin: + name: GRRMartin + email: martin@localhost + old_password: <%= require 'digest'; Digest::MD5.hexdigest("GRRMartin's password IS... HODOR (lol!)") %> diff --git a/test/models/user_secure_password_test.rb b/test/models/user_secure_password_test.rb new file mode 100644 index 00000000..804f4298 --- /dev/null +++ b/test/models/user_secure_password_test.rb @@ -0,0 +1,86 @@ +require 'test_helper' + +## +# Tests the entire ActiveModel::SecurePassword API implemented by the User model +# +# Taken from this demonstration: +# http://api.rubyonrails.org/classes/ActiveModel/SecurePassword/ClassMethods.html#method-i-has_secure_password +class UserSecurePasswordTest < ActiveSupport::TestCase + setup do + @username = 'ChrisTolkien' + @email = 'chris@localhost' + @rightpass = 'HiDad' + @wrongpass = 'Mellon' + end + + test 'cannot save user with mismatched password and confirmation' do + user = User.new(name: @username, + password: @rightpass, + password_confirmation: @wrongpass) + refute user.save, + 'Was able to save user despite mismatched password and confirmation' + end + + test 'user with correct password and confirmation is a valid model' do + user = User.new(name: @username, + email: @email, + password: @rightpass, + password_confirmation: @rightpass) + assert user.valid?, "User model was not valid with a matching password + and confirmation: #{user.errors.messages}" + end + + test 'can save user with matching password and confirmation' do + user = User.new(name: @username, + email: @email, + password: @rightpass, + password_confirmation: @rightpass) + + assert user.save, + 'Wasn\'t able to save user despite matching password '\ + 'and confirmation' + end + + test 'can save a user with correct password and no confirmation' do + user = User.new(name: @username, + email: @email, + password: @rightpass) + + assert user.save, 'Wasn\'t able to save user despite having a password' + end + + test 'cannot authenticate a user with the incorrect password' do + user = User.new(name: @username, password: @rightpass) + + refute user.authenticate(@wrongpass), + 'Was able to authenticate a user with an incorrect password' + end + + test 'can authenticate a user with the correct password' do + user = User.new(name: @username, + email: @email, + password: @rightpass) + + assert_not_nil user.authenticate(@rightpass), + 'Was not able to authenticate a user with a correct password' + end + + test 'can find a user and authenticate with the correct password' do + User.create(name: @username, + email: @email, + password: @rightpass) + + assert_not_nil User.find_by(name: @username).try(:authenticate, @rightpass), + 'Was not able to find & authenticate user with a '\ + 'correct password' + end + + test 'cannot find and authenticate with an incorrect password' do + User.create(name: @username, + email: @email, + password: @rightpass) + + refute User.find_by(name: @username).try(:authenticate, @wrongpass), + 'Was able to find & authenticate a user with an incorrect password' + end +end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 69241296..f2889d98 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -25,7 +25,8 @@ class UserTest < ActiveSupport::TestCase test 'user fixture assumptions' do assert_not_nil users(:tolkien), 'User fixture is unavailable' - assert users(:tolkien).valid?, 'User fixture is not a valid user' + assert users(:tolkien).valid?, "User fixture is not a valid user: + #{users(:tolkien).errors.messages}" end test 'can get user content' do From e3f78701c8ee96d52c05736ae8ec5bc040f44b99 Mon Sep 17 00:00:00 2001 From: Robert Richter Date: Sun, 19 Apr 2015 14:07:18 -0500 Subject: [PATCH 06/10] Filter sensitive data from logs --- config/application.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/config/application.rb b/config/application.rb index 9019ef92..2e72476b 100644 --- a/config/application.rb +++ b/config/application.rb @@ -7,6 +7,7 @@ require 'rails/all' Bundler.require(*Rails.groups) module PlanCharacters + # Default application environment configurations class Application < Rails::Application # Settings in config/environments/* take precedence over those # specified here. @@ -25,5 +26,9 @@ module PlanCharacters # config.i18n.load_path += Dir[Rails.root.join( # 'my', 'locales', '*.{rb,yml}').to_s] # config.i18n.default_locale = :de + + # Filter sensitive parameters out of logs + config.filter_paramters << :password + config.filter_paramters << :password_confirmation end end From ac9f494234311ba9136fb18613f35263471e536b Mon Sep 17 00:00:00 2001 From: Robert Richter Date: Sun, 19 Apr 2015 14:11:24 -0500 Subject: [PATCH 07/10] Spell things correctly --- config/application.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/application.rb b/config/application.rb index 2e72476b..00d28cef 100644 --- a/config/application.rb +++ b/config/application.rb @@ -28,7 +28,7 @@ module PlanCharacters # config.i18n.default_locale = :de # Filter sensitive parameters out of logs - config.filter_paramters << :password - config.filter_paramters << :password_confirmation + config.filter_parameters << :password + config.filter_parameters << :password_confirmation end end From 328e76659aca45deb217ef469372ca9da45078ee Mon Sep 17 00:00:00 2001 From: Robert Richter Date: Sun, 19 Apr 2015 14:23:39 -0500 Subject: [PATCH 08/10] Add bcrypt to Rails 4.2 Edge gemfile --- gemfiles/rails-4.2.edge.gemfile | 1 + 1 file changed, 1 insertion(+) diff --git a/gemfiles/rails-4.2.edge.gemfile b/gemfiles/rails-4.2.edge.gemfile index 636dddba..dfa1e74e 100644 --- a/gemfiles/rails-4.2.edge.gemfile +++ b/gemfiles/rails-4.2.edge.gemfile @@ -2,6 +2,7 @@ source 'https://rubygems.org' source 'https://rubygems.org' +gem 'bcrypt', '~> 3.1.7' gem 'rails', '4.1.0' gem 'jquery-rails' gem 'sass-rails', '~> 4.0.3', :require => 'sass' From a76d485e0cf74122d98c0c35e7eacb06ef431d41 Mon Sep 17 00:00:00 2001 From: Robert Richter Date: Sun, 19 Apr 2015 15:47:54 -0500 Subject: [PATCH 09/10] They ought to be more specific about the bcrypt gem --- Gemfile | 2 +- Gemfile.lock | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/Gemfile b/Gemfile index f3f05ebf..46ea82ce 100644 --- a/Gemfile +++ b/Gemfile @@ -2,7 +2,7 @@ source 'https://rubygems.org' -gem 'bcrypt', '~> 3.1.7' +gem 'bcrypt-ruby', '~> 3.1.5' gem 'rails', '4.1.0' gem 'jquery-rails' gem 'sass-rails', '~> 4.0.3', require: 'sass' diff --git a/Gemfile.lock b/Gemfile.lock index 26a23df9..b2f60731 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -37,6 +37,8 @@ GEM json (~> 1.4) nokogiri (>= 1.4.4) bcrypt (3.1.10-x86-mingw32) + bcrypt-ruby (3.1.5-x86-mingw32) + bcrypt (>= 3.1.3) bootplus-rails (1.0.0) builder (3.2.2) capybara (2.4.4) @@ -200,7 +202,7 @@ PLATFORMS DEPENDENCIES aws-sdk (~> 1.50) - bcrypt (~> 3.1.7) + bcrypt-ruby (~> 3.1.5) bootplus-rails capybara coffee-rails (~> 4.0.0) From fc62f30770778415bed2d2d730e0b2c0129003f6 Mon Sep 17 00:00:00 2001 From: Robert Richter Date: Sun, 19 Apr 2015 15:54:09 -0500 Subject: [PATCH 10/10] Downgrade version of bcrypt for travis --- Gemfile | 2 +- Gemfile.lock | 7 +++---- gemfiles/rails-4.2.edge.gemfile | 2 +- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/Gemfile b/Gemfile index 46ea82ce..ea163493 100644 --- a/Gemfile +++ b/Gemfile @@ -2,7 +2,7 @@ source 'https://rubygems.org' -gem 'bcrypt-ruby', '~> 3.1.5' +gem 'bcrypt-ruby', '~> 3.0.1' gem 'rails', '4.1.0' gem 'jquery-rails' gem 'sass-rails', '~> 4.0.3', require: 'sass' diff --git a/Gemfile.lock b/Gemfile.lock index b2f60731..76ab142a 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -36,9 +36,8 @@ GEM aws-sdk-v1 (1.63.0) json (~> 1.4) nokogiri (>= 1.4.4) - bcrypt (3.1.10-x86-mingw32) - bcrypt-ruby (3.1.5-x86-mingw32) - bcrypt (>= 3.1.3) + bcrypt-ruby (3.0.1) + bcrypt-ruby (3.0.1-x86-mingw32) bootplus-rails (1.0.0) builder (3.2.2) capybara (2.4.4) @@ -202,7 +201,7 @@ PLATFORMS DEPENDENCIES aws-sdk (~> 1.50) - bcrypt-ruby (~> 3.1.5) + bcrypt-ruby (~> 3.0.1) bootplus-rails capybara coffee-rails (~> 4.0.0) diff --git a/gemfiles/rails-4.2.edge.gemfile b/gemfiles/rails-4.2.edge.gemfile index dfa1e74e..3a68d6d0 100644 --- a/gemfiles/rails-4.2.edge.gemfile +++ b/gemfiles/rails-4.2.edge.gemfile @@ -2,7 +2,7 @@ source 'https://rubygems.org' source 'https://rubygems.org' -gem 'bcrypt', '~> 3.1.7' +gem 'bcrypt-ruby', '~> 3.0.1' gem 'rails', '4.1.0' gem 'jquery-rails' gem 'sass-rails', '~> 4.0.3', :require => 'sass'