Migrate to built-in ActiveModel secure passwords

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'

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)

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)

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)

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

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

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

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

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

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!)") %>

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

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