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