From d2b6d39d2419fc5fc78bea1b1889d8277f679f80 Mon Sep 17 00:00:00 2001 From: Moncef Belyamani Date: Wed, 30 Dec 2015 16:37:32 -0500 Subject: [PATCH] Add support for OTP secret key encryption **Why**: To provide an additional layer of security. The TOTP spec (RFC 6238) recommends encrypting the keys. http://tools.ietf.org/html/rfc6238 **How**: Borrow the encryption code from the `attr_encrypted` gem and use it to encrypt and decrypt the `otp_secret_key` attribute. Allow users to add encryption by passing in `encrypted: true` to `has_one_time_password`. This provides backwards-compatibility for existing users of the gem. See the README updates for more detailed instructions for both new and existing users. --- Gemfile | 9 +- README.md | 201 ++++++++--- .../active_record/templates/migration.rb | 17 +- lib/two_factor_authentication.rb | 3 + .../models/two_factor_authenticatable.rb | 112 ++++-- lib/two_factor_authentication/schema.rb | 16 +- spec/controllers/two_factor_auth_spec.rb | 18 - ...o_factor_authentication_controller_spec.rb | 33 ++ .../two_factor_authenticatable_spec.rb | 79 +++-- ...wo_factor_authentication_generator_spec.rb | 36 ++ .../models/two_factor_authenticatable_spec.rb | 330 +++++++++++------- spec/rails_app/app/models/encrypted_user.rb | 14 + spec/rails_app/app/models/user.rb | 3 +- spec/rails_app/config/environments/test.rb | 3 + spec/rails_app/config/initializers/devise.rb | 2 + ...224171231_add_encrypted_columns_to_user.rb | 9 + .../20151224180310_populate_otp_column.rb | 19 + ...8230340_remove_otp_secret_key_from_user.rb | 5 + spec/rails_app/db/schema.rb | 30 +- spec/spec_helper.rb | 1 + spec/support/authenticated_model_helper.rb | 28 +- spec/support/controller_helper.rb | 16 + spec/support/features_spec_helper.rb | 8 + two_factor_authentication.gemspec | 1 + 24 files changed, 718 insertions(+), 275 deletions(-) delete mode 100644 spec/controllers/two_factor_auth_spec.rb create mode 100644 spec/controllers/two_factor_authentication_controller_spec.rb create mode 100644 spec/generators/active_record/two_factor_authentication_generator_spec.rb create mode 100644 spec/rails_app/app/models/encrypted_user.rb create mode 100644 spec/rails_app/db/migrate/20151224171231_add_encrypted_columns_to_user.rb create mode 100644 spec/rails_app/db/migrate/20151224180310_populate_otp_column.rb create mode 100644 spec/rails_app/db/migrate/20151228230340_remove_otp_secret_key_from_user.rb create mode 100644 spec/support/controller_helper.rb diff --git a/Gemfile b/Gemfile index 8aee4d5..59ad495 100644 --- a/Gemfile +++ b/Gemfile @@ -1,4 +1,4 @@ -source "http://rubygems.org" +source 'https://rubygems.org' # Specify your gem's dependencies in devise_ip_filter.gemspec gemspec @@ -20,6 +20,11 @@ if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.2.0') gem "test-unit", "~> 3.0" end +group :test, :development do + gem 'sqlite3' +end + group :test do - gem "sqlite3" + gem 'rack_session_access' + gem 'ammeter' end diff --git a/README.md b/README.md index 3574b29..7a981b6 100644 --- a/README.md +++ b/README.md @@ -5,11 +5,12 @@ ## Features -* control sms code pattern -* configure max login attempts -* per user level control if he really need two factor authentication -* your own sms logic +* configurable OTP code digit length +* configurable max login attempts +* customizable logic to determine if a user needs two factor authentication +* customizable logic for sending the OTP code to the user * configurable period where users won't be asked for 2FA again +* option to encrypt the OTP secret key in the database, with iv and salt ## Configuration @@ -23,62 +24,71 @@ Once that's done, run: bundle install -### Automatic installation +### Installation -In order to add two factor authentication to a model, run the command: +#### Automatic initial setup +To set up the model and database migration file automatically, run the +following command: bundle exec rails g two_factor_authentication MODEL -Where MODEL is your model name (e.g. User or Admin). This generator will add `:two_factor_authenticatable` to your model -and create a migration in `db/migrate/`, which will add `:otp_secret_key` and `:second_factor_attempts_count` to your table. -Finally, run the migration with: +Where MODEL is your model name (e.g. User or Admin). This generator will add +`:two_factor_authenticatable` to your model's Devise options and create a +migration in `db/migrate/`, which will add the following columns to your table: - bundle exec rake db:migrate - -Add the following line to your model to fully enable two-factor auth: +- `:second_factor_attempts_count` +- `:encrypted_otp_secret_key` +- `:encrypted_otp_secret_key_iv` +- `:encrypted_otp_secret_key_salt` - has_one_time_password - -Set config values, if desired: +#### Manual initial setup +If you prefer to set up the model and migration manually, add the +`:two_factor_authentication` option to your existing devise options, such as: ```ruby -config.max_login_attempts = 3 # Maximum second factor attempts count -config.allowed_otp_drift_seconds = 30 # Allowed time drift -config.otp_length = 6 # OTP code length -config.remember_otp_session_for_seconds = 30.days # Time before browser has to enter OTP code again +devise :database_authenticatable, :registerable, :recoverable, :rememberable, + :trackable, :validatable, :two_factor_authenticatable ``` -Override the method to send one-time passwords in your model, this is automatically called when a user logs in: +Then create your migration file using the Rails generator, such as: -```ruby -def send_two_factor_authentication_code - # use Model#otp_code and send via SMS, etc. -end +``` +rails g migration AddTwoFactorFieldsToUsers second_factor_attempts_count:integer encrypted_otp_secret_key:string:index encrypted_otp_secret_key_iv:string encrypted_otp_secret_key_salt:string ``` -### Manual installation - -To manually enable two factor authentication for the User model, you should add two_factor_authentication to your devise line, like: +Open your migration file (it will be in the `db/migrate` directory and will be +named something like `20151230163930_add_two_factor_fields_to_users.rb`), and +add `unique: true` to the `add_index` line so that it looks like this: ```ruby -devise :database_authenticatable, :registerable, - :recoverable, :rememberable, :trackable, :validatable, :two_factor_authenticatable +add_index :users, :encrypted_otp_secret_key, unique: true ``` +Save the file. + +#### Complete the setup +Run the migration with: + + bundle exec rake db:migrate Add the following line to your model to fully enable two-factor auth: - has_one_time_password + has_one_time_password(encrypted: true) -Set config values to devise.rb, if desired: +Set config values in `config/initializers/devise.rb`: ```ruby -config.max_login_attempts = 3 # Maximum second factor attempts count -config.allowed_otp_drift_seconds = 30 # Allowed time drift +config.max_login_attempts = 3 # Maximum second factor attempts count. +config.allowed_otp_drift_seconds = 30 # Allowed time drift between client and server. config.otp_length = 6 # OTP code length -config.remember_otp_session_for_seconds = 30.days # Time before browser has to enter OTP code again +config.remember_otp_session_for_seconds = 30.days # Time before browser has to enter OTP code again. Default is 0. +config.otp_secret_encryption_key = ENV['OTP_SECRET_ENCRYPTION_KEY'] ``` +The `otp_secret_encryption_key` must be a random key that is not stored in the +DB, and is not checked in to your repo. It is recommended to store it in an +environment variable, and you can generate it with `bundle exec rake secret`. -Override the method to send one-time passwords in your model, this is automatically called when a user logs in: +Override the method to send one-time passwords in your model. This is +automatically called when a user logs in: ```ruby def send_two_factor_authentication_code @@ -86,10 +96,10 @@ def send_two_factor_authentication_code end ``` - ### Customisation and Usage -By default second factor authentication enabled for each user, you can change it with this method in your User model: +By default, second factor authentication is required for each user. You can +change that by overriding the following method in your model: ```ruby def need_two_factor_authentication?(request) @@ -97,19 +107,29 @@ def need_two_factor_authentication?(request) end ``` -this will disable two factor authentication for local users +In the example above, two factor authentication will not be required for local +users. -This gem is compatible with Google Authenticator (https://support.google.com/accounts/answer/1066447?hl=en). You can generate provisioning uris by invoking the following method on your model: +This gem is compatible with [Google Authenticator](https://support.google.com/accounts/answer/1066447?hl=en). +You can generate provisioning uris by invoking the following method on your model: - user.provisioning_uri #This assumes a user model with an email attributes +```ruby +user.provisioning_uri # This assumes a user model with an email attribute +``` -This provisioning uri can then be turned in to a QR code if desired so that users may add the app to Google Authenticator easily. Once this is done they may retrieve a one-time password directly from the Google Authenticator app as well as through whatever method you define in `send_two_factor_authentication_code` +This provisioning uri can then be turned in to a QR code if desired so that +users may add the app to Google Authenticator easily. Once this is done, they +may retrieve a one-time password directly from the Google Authenticator app as +well as through whatever method you define in +`send_two_factor_authentication_code`. #### Overriding the view -The default view that shows the form can be overridden by first adding a folder named: "two_factor_authentication" inside "app/views/devise", in here you want to create a "show.html.erb" view. +The default view that shows the form can be overridden by adding a +file named `show.html.erb` (or `show.html.haml` if you prefer HAML) +inside `app/views/devise/two_factor_authentication/` and customizing it. +Below is an example using ERB: -The full path should be "app/views/devise/two_factor_authentication/show.html.erb" ```html

Hi, you received a code by email, please enter it below, thanks!

@@ -125,21 +145,94 @@ The full path should be "app/views/devise/two_factor_authentication/show.html.er #### Updating existing users with OTP secret key -If you have existing users that needs to be provided with a OTP secret key, so they can take benefit of the two factor authentication, create a rake. It could look like this one below: +If you have existing users that need to be provided with a OTP secret key, so +they can use two factor authentication, create a rake task. It could look like this one below: ```ruby -desc "rake task to update users with otp secret key" +desc 'rake task to update users with otp secret key' task :update_users_with_otp_secret_key => :environment do - users = User.all - - users.each do |user| - key = ROTP::Base32.random_base32 - user.update_attributes(:otp_secret_key => key) - user.save - puts "Rake[:update_users_with_otp_secret_key] => User '#{user.email}' OTP secret key set to '#{key}'" - end + User.find_each do |user| + user.otp_secret_key = ROTP::Base32.random_base32 + user.save! + puts "Rake[:update_users_with_otp_secret_key] => OTP secret key set to '#{key}' for User '#{user.email}'" + end end ``` +Then run the task with `bundle exec rake update_users_with_otp_secret_key` + +#### Adding the OTP encryption option to an existing app + +If you've already been using this gem, and want to start encrypting the OTP +secret key in the database (recommended), you'll need to perform the following +steps: + +1. Generate a migration to add the necessary columns to your model's table: + + ``` + rails g migration AddEncryptionFieldsToUsers encrypted_otp_secret_key:string:index encrypted_otp_secret_key_iv:string encrypted_otp_secret_key_salt:string + ``` + + Open your migration file (it will be in the `db/migrate` directory and will be + named something like `20151230163930_add_encryption_fields_to_users.rb`), and + add `unique: true` to the `add_index` line so that it looks like this: + + ```ruby + add_index :users, :encrypted_otp_secret_key, unique: true + ``` + Save the file. + +2. Run the migration: `bundle exec rake db:migrate` + +2. Update the gem: `bundle update two_factor_authentication` + +3. Add `encrypted: true` to `has_one_time_password` in your model. + For example: `has_one_time_password(encrypted: true)` + +4. Generate a migration to populate the new encryption fields: + ``` + rails g migration PopulateEncryptedOtpFields + ``` + + Open the generated file, and replace its contents with the following: + ```ruby + class PopulateEncryptedOtpFields < ActiveRecord::Migration + def up + User.reset_column_information + + User.find_each do |user| + user.otp_secret_key = user.read_attribute('otp_secret_key') + user.save! + end + end + + def down + User.reset_column_information + + User.find_each do |user| + user.otp_secret_key = ROTP::Base32.random_base32 + user.save! + end + end + end + ``` + +5. Generate a migration to remove the `:otp_secret_key` column: + ``` + rails g migration RemoveOtpSecretKeyFromUsers otp_secret_key:string + ``` + +6. Run the migrations: `bundle exec rake db:migrate` + +If, for some reason, you want to switch back to the old non-encrypted version, +use these steps: + +1. Remove `(encrypted: true)` from `has_one_time_password` + +2. Roll back the last 3 migrations (assuming you haven't added any new ones +after them): + ``` + bundle exec rake db:rollback STEP=3 + ``` #### Executing some code after the user signs in and before they sign out @@ -174,6 +267,6 @@ and you need different logic for each type of user, create a second class for your admin user, such as `AdminOtpSender`, with its own logic for `#reset_otp_state`. -### Example +### Example App [TwoFactorAuthenticationExample](https://github.com/Houdini/TwoFactorAuthenticationExample) diff --git a/lib/generators/active_record/templates/migration.rb b/lib/generators/active_record/templates/migration.rb index cff6122..6689003 100644 --- a/lib/generators/active_record/templates/migration.rb +++ b/lib/generators/active_record/templates/migration.rb @@ -1,15 +1,10 @@ class TwoFactorAuthenticationAddTo<%= table_name.camelize %> < ActiveRecord::Migration - def up - change_table :<%= table_name %> do |t| - t.string :otp_secret_key - t.integer :second_factor_attempts_count, :default => 0 - end + def change + add_column :<%= table_name %>, :second_factor_attempts_count, :integer, default: 0 + add_column :<%= table_name %>, :encrypted_otp_secret_key, :string + add_column :<%= table_name %>, :encrypted_otp_secret_key_iv, :string + add_column :<%= table_name %>, :encrypted_otp_secret_key_salt, :string - add_index :<%= table_name %>, :otp_secret_key, :unique => true - end - - def down - remove_column :<%= table_name %>, :otp_secret_key - remove_column :<%= table_name %>, :second_factor_attempts_count + add_index :<%= table_name %>, :encrypted_otp_secret_key, unique: true end end diff --git a/lib/two_factor_authentication.rb b/lib/two_factor_authentication.rb index 524a58f..24ae9ab 100644 --- a/lib/two_factor_authentication.rb +++ b/lib/two_factor_authentication.rb @@ -18,6 +18,9 @@ module Devise mattr_accessor :remember_otp_session_for_seconds @@remember_otp_session_for_seconds = 0 + + mattr_accessor :otp_secret_encryption_key + @@otp_secret_encryption_key = '' end module TwoFactorAuthentication diff --git a/lib/two_factor_authentication/models/two_factor_authenticatable.rb b/lib/two_factor_authentication/models/two_factor_authenticatable.rb index 9999554..eba5135 100644 --- a/lib/two_factor_authentication/models/two_factor_authenticatable.rb +++ b/lib/two_factor_authentication/models/two_factor_authenticatable.rb @@ -1,5 +1,6 @@ require 'two_factor_authentication/hooks/two_factor_authenticatable' require 'rotp' +require 'encryptor' module Devise module Models @@ -8,46 +9,37 @@ module Devise module ClassMethods def has_one_time_password(options = {}) - - cattr_accessor :otp_column_name - self.otp_column_name = "otp_secret_key" - include InstanceMethodsOnActivation + include EncryptionInstanceMethods if options[:encrypted] == true before_create { populate_otp_column } - - if respond_to?(:attributes_protected_by_default) - def self.attributes_protected_by_default #:nodoc: - super + [self.otp_column_name] - end - end end - ::Devise::Models.config(self, :max_login_attempts, :allowed_otp_drift_seconds, :otp_length, :remember_otp_session_for_seconds) + + ::Devise::Models.config( + self, :max_login_attempts, :allowed_otp_drift_seconds, :otp_length, + :remember_otp_session_for_seconds, :otp_secret_encryption_key) end module InstanceMethodsOnActivation def authenticate_otp(code, options = {}) - totp = ROTP::TOTP.new(self.otp_column, { digits: options[:otp_length] || self.class.otp_length }) + totp = ROTP::TOTP.new( + otp_secret_key, digits: options[:otp_length] || self.class.otp_length + ) drift = options[:drift] || self.class.allowed_otp_drift_seconds totp.verify_with_drift(code, drift) end def otp_code(time = Time.now, options = {}) - ROTP::TOTP.new(self.otp_column, { digits: options[:otp_length] || self.class.otp_length }).at(time, true) + ROTP::TOTP.new( + otp_secret_key, + digits: options[:otp_length] || self.class.otp_length + ).at(time, true) end def provisioning_uri(account = nil, options = {}) account ||= self.email if self.respond_to?(:email) - ROTP::TOTP.new(self.otp_column, options).provisioning_uri(account) - end - - def otp_column - self.send(self.class.otp_column_name) - end - - def otp_column=(attr) - self.send("#{self.class.otp_column_name}=", attr) + ROTP::TOTP.new(otp_secret_key, options).provisioning_uri(account) end def need_two_factor_authentication?(request) @@ -67,9 +59,83 @@ module Devise end def populate_otp_column - self.otp_column = ROTP::Base32.random_base32 + self.otp_secret_key = ROTP::Base32.random_base32 + end + end + + module EncryptionInstanceMethods + def otp_secret_key + decrypt(encrypted_otp_secret_key) + end + + def otp_secret_key=(value) + self.encrypted_otp_secret_key = encrypt(value) + end + + private + + def decrypt(encrypted_value) + return encrypted_value if encrypted_value.blank? + + encrypted_value = encrypted_value.unpack('m').first + + value = ::Encryptor.decrypt(encryption_options_for(encrypted_value)) + + if defined?(Encoding) + encoding = Encoding.default_internal || Encoding.default_external + value = value.force_encoding(encoding.name) + end + + value + end + + def encrypt(value) + return value if value.blank? + + value = value.to_s + encrypted_value = ::Encryptor.encrypt(encryption_options_for(value)) + + encrypted_value = [encrypted_value].pack('m') + + encrypted_value + end + + def encryption_options_for(value) + { + value: value, + key: Devise.otp_secret_encryption_key, + iv: iv_for_attribute, + salt: salt_for_attribute + } + end + + def iv_for_attribute(algorithm = 'aes-256-cbc') + iv = encrypted_otp_secret_key_iv + + if iv.nil? + algo = OpenSSL::Cipher::Cipher.new(algorithm) + iv = [algo.random_iv].pack('m') + self.encrypted_otp_secret_key_iv = iv + end + + iv.unpack('m').first if iv.present? + end + + def salt_for_attribute + salt = encrypted_otp_secret_key_salt || + self.encrypted_otp_secret_key_salt = generate_random_base64_encoded_salt + + decode_salt_if_encoded(salt) end + def generate_random_base64_encoded_salt + prefix = '_' + prefix + [SecureRandom.random_bytes].pack('m') + end + + def decode_salt_if_encoded(salt) + salt.slice(0).eql?('_') ? salt.slice(1..-1).unpack('m').first : salt + end end end end diff --git a/lib/two_factor_authentication/schema.rb b/lib/two_factor_authentication/schema.rb index 2f0d14c..d82f4a6 100644 --- a/lib/two_factor_authentication/schema.rb +++ b/lib/two_factor_authentication/schema.rb @@ -1,11 +1,19 @@ module TwoFactorAuthentication module Schema - def otp_secret_key - apply_devise_schema :otp_secret_key, String - end - def second_factor_attempts_count apply_devise_schema :second_factor_attempts_count, Integer, :default => 0 end + + def encrypted_otp_secret_key + apply_devise_schema :encrypted_otp_secret_key, String + end + + def encrypted_otp_secret_key_iv + apply_devise_schema :encrypted_otp_secret_key_iv, String + end + + def encrypted_otp_secret_key_salt + apply_devise_schema :encrypted_otp_secret_key_salt, String + end end end diff --git a/spec/controllers/two_factor_auth_spec.rb b/spec/controllers/two_factor_auth_spec.rb deleted file mode 100644 index b8d91ff..0000000 --- a/spec/controllers/two_factor_auth_spec.rb +++ /dev/null @@ -1,18 +0,0 @@ -require 'spec_helper' - -include Warden::Test::Helpers - -describe HomeController, :type => :controller do - context "passed only 1st factor auth" do - let(:user) { create_user } - - describe "is_fully_authenticated helper" do - it "should be true" do - login_as user, scope: :user - visit user_two_factor_authentication_path - - expect(controller.is_fully_authenticated?).to be_truthy - end - end - end -end diff --git a/spec/controllers/two_factor_authentication_controller_spec.rb b/spec/controllers/two_factor_authentication_controller_spec.rb new file mode 100644 index 0000000..1089dbf --- /dev/null +++ b/spec/controllers/two_factor_authentication_controller_spec.rb @@ -0,0 +1,33 @@ +require 'spec_helper' + +describe Devise::TwoFactorAuthenticationController, type: :controller do + describe 'is_fully_authenticated? helper' do + before do + sign_in + end + + context 'after user enters valid OTP code' do + it 'returns true' do + post :update, code: controller.current_user.otp_code + + expect(subject.is_fully_authenticated?).to eq true + end + end + + context 'when user has not entered any OTP yet' do + it 'returns false' do + get :show + + expect(subject.is_fully_authenticated?).to eq false + end + end + + context 'when user enters an invalid OTP' do + it 'returns false' do + post :update, code: '12345' + + expect(subject.is_fully_authenticated?).to eq false + end + end + end +end diff --git a/spec/features/two_factor_authenticatable_spec.rb b/spec/features/two_factor_authenticatable_spec.rb index ef93e14..27e4595 100644 --- a/spec/features/two_factor_authenticatable_spec.rb +++ b/spec/features/two_factor_authenticatable_spec.rb @@ -1,47 +1,64 @@ require 'spec_helper' +include AuthenticatedModelHelper feature "User of two factor authentication" do - let(:user) { create_user } + context 'sending two factor authentication code via SMS' do + shared_examples 'sends and authenticates code' do |user, type| + before do + if type == 'encrypted' + allow(User).to receive(:has_one_time_password).with(encrypted: true) + end + end - scenario "must be logged in" do - visit user_two_factor_authentication_path + it 'does not send an SMS before the user has signed in' do + expect(SMSProvider.messages).to be_empty + end - expect(page).to have_content("Welcome Home") - expect(page).to have_content("You are signed out") - end + it 'sends code via SMS after sign in' do + visit new_user_session_path + complete_sign_in_form_for(user) - scenario "sends two factor authentication code after sign in" do - expect(SMSProvider.messages).to be_empty + expect(page).to have_content 'Enter your personal code' - visit new_user_session_path - complete_sign_in_form_for(user) + expect(SMSProvider.messages.size).to eq(1) + message = SMSProvider.last_message + expect(message.to).to eq(user.phone_number) + expect(message.body).to eq(user.otp_code) + end - expect(page).to have_content "Enter your personal code" + it 'authenticates a valid OTP code' do + visit new_user_session_path + complete_sign_in_form_for(user) - expect(SMSProvider.messages.size).to eq(1) - message = SMSProvider.last_message - expect(message.to).to eq(user.phone_number) - expect(message.body).to eq(user.otp_code) - end + expect(page).to have_content('You are signed in as Marissa') - context "when logged in" do + fill_in 'code', with: user.otp_code + click_button 'Submit' - background do - login_as user + within('.flash.notice') do + expect(page).to have_content('Two factor authentication successful.') + end + + expect(current_path).to eq root_path + end end - scenario "can fill in TFA code" do - visit user_two_factor_authentication_path + it_behaves_like 'sends and authenticates code', create_user('not_encrypted') + it_behaves_like 'sends and authenticates code', create_user, 'encrypted' + end - expect(page).to have_content("You are signed in as Marissa") - expect(page).to have_content("Enter your personal code") + scenario "must be logged in" do + visit user_two_factor_authentication_path - fill_in "code", with: user.otp_code - click_button "Submit" + expect(page).to have_content("Welcome Home") + expect(page).to have_content("You are signed out") + end - within(".flash.notice") do - expect(page).to have_content("Two factor authentication successful.") - end + context "when logged in" do + let(:user) { create_user } + + background do + login_as user end scenario "is redirected to TFA when path requires authentication" do @@ -122,6 +139,12 @@ feature "User of two factor authentication" do expect(page).to have_content("Enter your personal code") end end + + it 'sets the warden session need_two_factor_authentication key to true' do + session_hash = { 'need_two_factor_authentication' => true } + + expect(page.get_rack_session_key('warden.user.user.session')).to eq session_hash + end end describe 'signing in' do diff --git a/spec/generators/active_record/two_factor_authentication_generator_spec.rb b/spec/generators/active_record/two_factor_authentication_generator_spec.rb new file mode 100644 index 0000000..5a8989d --- /dev/null +++ b/spec/generators/active_record/two_factor_authentication_generator_spec.rb @@ -0,0 +1,36 @@ +require 'spec_helper' + +require 'generators/active_record/two_factor_authentication_generator' + +describe ActiveRecord::Generators::TwoFactorAuthenticationGenerator, type: :generator do + destination File.expand_path('../../../../../tmp', __FILE__) + + before do + prepare_destination + end + + it 'runs all methods in the generator' do + gen = generator %w(users) + expect(gen).to receive(:copy_two_factor_authentication_migration) + gen.invoke_all + end + + describe 'the generated files' do + before do + run_generator %w(users) + end + + describe 'the migration' do + subject { migration_file('db/migrate/two_factor_authentication_add_to_users.rb') } + + it { is_expected.to exist } + it { is_expected.to be_a_migration } + it { is_expected.to contain /def change/ } + it { is_expected.to contain /add_column :users, :second_factor_attempts_count, :integer, default: 0/ } + it { is_expected.to contain /add_column :users, :encrypted_otp_secret_key, :string/ } + it { is_expected.to contain /add_column :users, :encrypted_otp_secret_key_iv, :string/ } + it { is_expected.to contain /add_column :users, :encrypted_otp_secret_key_salt, :string/ } + it { is_expected.to contain /add_index :users, :encrypted_otp_secret_key, unique: true/ } + end + end +end diff --git a/spec/lib/two_factor_authentication/models/two_factor_authenticatable_spec.rb b/spec/lib/two_factor_authentication/models/two_factor_authenticatable_spec.rb index f44a3f5..118a02c 100644 --- a/spec/lib/two_factor_authentication/models/two_factor_authenticatable_spec.rb +++ b/spec/lib/two_factor_authentication/models/two_factor_authenticatable_spec.rb @@ -1,168 +1,264 @@ require 'spec_helper' include AuthenticatedModelHelper -describe Devise::Models::TwoFactorAuthenticatable, '#otp_code' do - let(:instance) { build_guest_user } - subject { instance.otp_code(time) } - let(:time) { 1392852456 } - - it "should return an error if no secret is set" do - expect { - subject - }.to raise_error Exception +describe Devise::Models::TwoFactorAuthenticatable do + describe '#otp_code' do + shared_examples 'otp_code' do |instance| + subject { instance.otp_code(time) } + let(:time) { 1_392_852_456 } + + it 'returns an error if no secret is set' do + expect { subject }.to raise_error Exception + end + + context 'secret is set' do + before :each do + instance.otp_secret_key = '2z6hxkdwi3uvrnpn' + end + + it 'does not return an error' do + subject + end + + it 'matches Devise configured length' do + expect(subject.length).to eq(Devise.otp_length) + end + + context 'with a known time' do + let(:time) { 1_392_852_756 } + + it 'returns a known result' do + expect(subject). + to eq('0000000524562202'.split(//).last(Devise.otp_length).join) + end + end + + context 'with a known time yielding a result with less than 6 digits' do + let(:time) { 1_393_065_856 } + + it 'returns a known result padded with zeroes' do + expect(subject). + to eq('0000001608007672'.split(//).last(Devise.otp_length).join) + end + end + end + end + + it_behaves_like 'otp_code', GuestUser.new + it_behaves_like 'otp_code', EncryptedUser.new end - context "secret is set" do - before :each do - instance.otp_secret_key = "2z6hxkdwi3uvrnpn" + describe '#authenticate_otp' do + shared_examples 'authenticate_otp' do |instance| + before :each do + instance.otp_secret_key = '2z6hxkdwi3uvrnpn' + end + + def do_invoke(code, user) + user.authenticate_otp(code) + end + + it 'authenticates a recently created code' do + code = instance.otp_code + expect(do_invoke(code, instance)).to eq(true) + end + + it 'does not authenticate an old code' do + code = instance.otp_code(1.minutes.ago.to_i) + expect(do_invoke(code, instance)).to eq(false) + end end - it "should not return an error" do - subject + it_behaves_like 'authenticate_otp', GuestUser.new + it_behaves_like 'authenticate_otp', EncryptedUser.new + end + + describe '#send_two_factor_authentication_code' do + let(:instance) { build_guest_user } + + it 'raises an error by default' do + expect { instance.send_two_factor_authentication_code }. + to raise_error(NotImplementedError) end - it "should be configured length" do - expect(subject.length).to eq(Devise.otp_length) + it 'is overrideable' do + def instance.send_two_factor_authentication_code + 'Code sent' + end + expect(instance.send_two_factor_authentication_code).to eq('Code sent') end + end - context "with a known time" do - let(:time) { 1392852756 } + describe '#provisioning_uri' do + shared_examples 'provisioning_uri' do |instance| + before do + instance.email = 'houdini@example.com' + instance.run_callbacks :create + end - it "should return a known result" do - expect(subject).to eq("0000000524562202".split(//).last(Devise.otp_length).join) + it "returns uri with user's email" do + expect(instance.provisioning_uri). + to match(%r{otpauth://totp/houdini@example.com\?secret=\w{16}}) + end + + it 'returns uri with issuer option' do + expect(instance.provisioning_uri('houdini')). + to match(%r{otpauth://totp/houdini\?secret=\w{16}$}) end - end - context "with a known time yielding a result with less than 6 digits" do - let(:time) { 1393065856 } + it 'returns uri with issuer option' do + require 'cgi' - it "should return a known result padded with zeroes" do - expect(subject).to eq("0000001608007672".split(//).last(Devise.otp_length).join) + uri = URI.parse(instance.provisioning_uri('houdini', issuer: 'Magic')) + params = CGI.parse(uri.query) + + expect(uri.scheme).to eq('otpauth') + expect(uri.host).to eq('totp') + expect(uri.path).to eq('/houdini') + expect(params['issuer'].shift).to eq('Magic') + expect(params['secret'].shift).to match(/\w{16}/) end end + + it_behaves_like 'provisioning_uri', GuestUser.new + it_behaves_like 'provisioning_uri', EncryptedUser.new end -end -describe Devise::Models::TwoFactorAuthenticatable, '#authenticate_otp' do - let(:instance) { build_guest_user } + describe '#populate_otp_column' do + shared_examples 'populate_otp_column' do |klass| + let(:instance) { klass.new } - before :each do - instance.otp_secret_key = "2z6hxkdwi3uvrnpn" - end + it 'populates otp_column on create' do + expect(instance.otp_secret_key).to be_nil - def do_invoke code, options = {} - instance.authenticate_otp(code, options) - end + # populate_otp_column called via before_create + instance.run_callbacks :create - it "should be able to authenticate a recently created code" do - code = instance.otp_code - expect(do_invoke(code)).to eq(true) - end + expect(instance.otp_secret_key).to match(/\w{16}/) + end - it "should not authenticate an old code" do - code = instance.otp_code(1.minutes.ago.to_i) - expect(do_invoke(code)).to eq(false) - end -end + it 'repopulates otp_column' do + instance.run_callbacks :create + original_key = instance.otp_secret_key -describe Devise::Models::TwoFactorAuthenticatable, '#send_two_factor_authentication_code' do - let(:instance) { build_guest_user } + instance.populate_otp_column - it "should raise an error by default" do - expect { - instance.send_two_factor_authentication_code - }.to raise_error(NotImplementedError) + expect(instance.otp_secret_key).to match(/\w{16}/) + expect(instance.otp_secret_key).to_not eq(original_key) + end + end + + it_behaves_like 'populate_otp_column', GuestUser + it_behaves_like 'populate_otp_column', EncryptedUser end - it "should be overrideable" do - def instance.send_two_factor_authentication_code - "Code sent" + describe '#max_login_attempts' do + let(:instance) { build_guest_user } + + before do + @original_max_login_attempts = GuestUser.max_login_attempts + GuestUser.max_login_attempts = 3 end - expect(instance.send_two_factor_authentication_code).to eq("Code sent") - end -end -describe Devise::Models::TwoFactorAuthenticatable, '#provisioning_uri' do - let(:instance) { build_guest_user } + after { GuestUser.max_login_attempts = @original_max_login_attempts } - before do - instance.email = "houdini@example.com" - instance.run_callbacks :create - end + it 'returns class setting' do + expect(instance.max_login_attempts).to eq(3) + end - it "should return uri with user's email" do - expect(instance.provisioning_uri).to match(%r{otpauth://totp/houdini@example.com\?secret=\w{16}}) - end + it 'returns false as boolean' do + instance.second_factor_attempts_count = nil + expect(instance.max_login_attempts?).to be_falsey + instance.second_factor_attempts_count = 0 + expect(instance.max_login_attempts?).to be_falsey + instance.second_factor_attempts_count = 1 + expect(instance.max_login_attempts?).to be_falsey + instance.second_factor_attempts_count = 2 + expect(instance.max_login_attempts?).to be_falsey + end - it "should return uri with issuer option" do - expect(instance.provisioning_uri("houdini")).to match(%r{otpauth://totp/houdini\?secret=\w{16}$}) + it 'returns true as boolean after too many attempts' do + instance.second_factor_attempts_count = 3 + expect(instance.max_login_attempts?).to be_truthy + instance.second_factor_attempts_count = 4 + expect(instance.max_login_attempts?).to be_truthy + end end - it "should return uri with issuer option" do - require 'cgi' + describe '.has_one_time_password' do + context 'when encrypted: true option is passed' do + let(:instance) { EncryptedUser.new } - uri = URI.parse(instance.provisioning_uri("houdini", issuer: 'Magic')) - params = CGI::parse(uri.query) + it 'encrypts otp_secret_key with iv, salt, and encoding' do + instance.otp_secret_key = '2z6hxkdwi3uvrnpn' - expect(uri.scheme).to eq("otpauth") - expect(uri.host).to eq("totp") - expect(uri.path).to eq("/houdini") - expect(params['issuer'].shift).to eq('Magic') - expect(params['secret'].shift).to match(%r{\w{16}}) - end -end + expect(instance.encrypted_otp_secret_key).to match(/.{44}/) + + expect(instance.encrypted_otp_secret_key_iv).to match(/.{24}/) -describe Devise::Models::TwoFactorAuthenticatable, '#populate_otp_column' do - let(:instance) { build_guest_user } + expect(instance.encrypted_otp_secret_key_salt).to match(/.{25}/) + end - it "populates otp_column on create" do - expect(instance.otp_secret_key).to be_nil + it 'does not encrypt a nil otp_secret_key' do + instance.otp_secret_key = nil - instance.run_callbacks :create # populate_otp_column called via before_create + expect(instance.encrypted_otp_secret_key).to be_nil - expect(instance.otp_secret_key).to match(%r{\w{16}}) - end + expect(instance.encrypted_otp_secret_key_iv).to be_nil - it "repopulates otp_column" do - instance.run_callbacks :create - original_key = instance.otp_secret_key + expect(instance.encrypted_otp_secret_key_salt).to be_nil + end - instance.populate_otp_column + it 'does not encrypt an empty otp_secret_key' do + instance.otp_secret_key = '' - expect(instance.otp_secret_key).to match(%r{\w{16}}) - expect(instance.otp_secret_key).to_not eq(original_key) - end -end + expect(instance.encrypted_otp_secret_key).to eq '' -describe Devise::Models::TwoFactorAuthenticatable, '#max_login_attempts' do - let(:instance) { build_guest_user } + expect(instance.encrypted_otp_secret_key_iv).to be_nil - before do - @original_max_login_attempts = GuestUser.max_login_attempts - GuestUser.max_login_attempts = 3 - end + expect(instance.encrypted_otp_secret_key_salt).to be_nil + end - after { GuestUser.max_login_attempts = @original_max_login_attempts } + it 'raises an error when Devise.otp_secret_encryption_key is not set' do + allow(Devise).to receive(:otp_secret_encryption_key).and_return nil - it "returns class setting" do - expect(instance.max_login_attempts).to eq(3) - end + # This error is raised by the encryptor gem + expect { instance.otp_secret_key = '2z6hxkdwi3uvrnpn' }. + to raise_error ArgumentError, 'must specify a :key' + end - it "returns false as boolean" do - instance.second_factor_attempts_count = nil - expect(instance.max_login_attempts?).to be_falsey - instance.second_factor_attempts_count = 0 - expect(instance.max_login_attempts?).to be_falsey - instance.second_factor_attempts_count = 1 - expect(instance.max_login_attempts?).to be_falsey - instance.second_factor_attempts_count = 2 - expect(instance.max_login_attempts?).to be_falsey - end + it 'passes in the correct options to Encryptor' do + instance.otp_secret_key = 'testing' + iv = instance.encrypted_otp_secret_key_iv + salt = instance.encrypted_otp_secret_key_salt + + encrypted = Encryptor.encrypt( + value: 'testing', + key: Devise.otp_secret_encryption_key, + iv: iv.unpack('m').first, + salt: salt.unpack('m').first + ) - it "returns true as boolean after too many attempts" do - instance.second_factor_attempts_count = 3 - expect(instance.max_login_attempts?).to be_truthy - instance.second_factor_attempts_count = 4 - expect(instance.max_login_attempts?).to be_truthy + expect(instance.encrypted_otp_secret_key).to eq [encrypted].pack('m') + end + + it 'varies the iv per instance' do + instance.otp_secret_key = 'testing' + user2 = EncryptedUser.new + user2.otp_secret_key = 'testing' + + expect(user2.encrypted_otp_secret_key_iv). + to_not eq instance.encrypted_otp_secret_key_iv + end + + it 'varies the salt per instance' do + instance.otp_secret_key = 'testing' + user2 = EncryptedUser.new + user2.otp_secret_key = 'testing' + + expect(user2.encrypted_otp_secret_key_salt). + to_not eq instance.encrypted_otp_secret_key_salt + end + end end end diff --git a/spec/rails_app/app/models/encrypted_user.rb b/spec/rails_app/app/models/encrypted_user.rb new file mode 100644 index 0000000..758a222 --- /dev/null +++ b/spec/rails_app/app/models/encrypted_user.rb @@ -0,0 +1,14 @@ +class EncryptedUser + extend ActiveModel::Callbacks + include ActiveModel::Validations + include Devise::Models::TwoFactorAuthenticatable + + define_model_callbacks :create + attr_accessor :encrypted_otp_secret_key, + :encrypted_otp_secret_key_iv, + :encrypted_otp_secret_key_salt, + :email, + :second_factor_attempts_count + + has_one_time_password(encrypted: true) +end diff --git a/spec/rails_app/app/models/user.rb b/spec/rails_app/app/models/user.rb index 8989f69..00247e7 100644 --- a/spec/rails_app/app/models/user.rb +++ b/spec/rails_app/app/models/user.rb @@ -1,7 +1,6 @@ class User < ActiveRecord::Base devise :two_factor_authenticatable, :database_authenticatable, :registerable, - :recoverable, :rememberable, :trackable, :validatable, - :two_factor_authenticatable + :recoverable, :rememberable, :trackable, :validatable has_one_time_password diff --git a/spec/rails_app/config/environments/test.rb b/spec/rails_app/config/environments/test.rb index d5538ef..7da716a 100644 --- a/spec/rails_app/config/environments/test.rb +++ b/spec/rails_app/config/environments/test.rb @@ -35,4 +35,7 @@ Dummy::Application.configure do # Print deprecation notices to the stderr config.active_support.deprecation = :stderr + + # For testing session variables in Capybara specs + config.middleware.use RackSessionAccess::Middleware end diff --git a/spec/rails_app/config/initializers/devise.rb b/spec/rails_app/config/initializers/devise.rb index 8a9d06f..4d6b064 100644 --- a/spec/rails_app/config/initializers/devise.rb +++ b/spec/rails_app/config/initializers/devise.rb @@ -253,4 +253,6 @@ Devise.setup do |config| # When using omniauth, Devise cannot automatically set Omniauth path, # so you need to do it manually. For the users scope, it would be: # config.omniauth_path_prefix = '/my_engine/users/auth' + + config.otp_secret_encryption_key = '0a8283fba984da1de24e4df1e93046cb53c5787944ef037b2dbf3e61d20fe11f25e25a855cec605fdf65b162329890d7230afdf64f681b4c32020281054e73ec' end diff --git a/spec/rails_app/db/migrate/20151224171231_add_encrypted_columns_to_user.rb b/spec/rails_app/db/migrate/20151224171231_add_encrypted_columns_to_user.rb new file mode 100644 index 0000000..67f80a6 --- /dev/null +++ b/spec/rails_app/db/migrate/20151224171231_add_encrypted_columns_to_user.rb @@ -0,0 +1,9 @@ +class AddEncryptedColumnsToUser < ActiveRecord::Migration + def change + add_column :users, :encrypted_otp_secret_key, :string + add_column :users, :encrypted_otp_secret_key_iv, :string + add_column :users, :encrypted_otp_secret_key_salt, :string + + add_index :users, :encrypted_otp_secret_key, unique: true + end +end diff --git a/spec/rails_app/db/migrate/20151224180310_populate_otp_column.rb b/spec/rails_app/db/migrate/20151224180310_populate_otp_column.rb new file mode 100644 index 0000000..a51222f --- /dev/null +++ b/spec/rails_app/db/migrate/20151224180310_populate_otp_column.rb @@ -0,0 +1,19 @@ +class PopulateOtpColumn < ActiveRecord::Migration + def up + User.reset_column_information + + User.find_each do |user| + user.otp_secret_key = user.read_attribute('otp_secret_key') + user.save! + end + end + + def down + User.reset_column_information + + User.find_each do |user| + user.otp_secret_key = ROTP::Base32.random_base32 + user.save! + end + end +end diff --git a/spec/rails_app/db/migrate/20151228230340_remove_otp_secret_key_from_user.rb b/spec/rails_app/db/migrate/20151228230340_remove_otp_secret_key_from_user.rb new file mode 100644 index 0000000..9ddd608 --- /dev/null +++ b/spec/rails_app/db/migrate/20151228230340_remove_otp_secret_key_from_user.rb @@ -0,0 +1,5 @@ +class RemoveOtpSecretKeyFromUser < ActiveRecord::Migration + def change + remove_column :users, :otp_secret_key, :string + end +end diff --git a/spec/rails_app/db/schema.rb b/spec/rails_app/db/schema.rb index 76bbd49..0c641c4 100644 --- a/spec/rails_app/db/schema.rb +++ b/spec/rails_app/db/schema.rb @@ -9,30 +9,32 @@ # from scratch. The latter is a flawed and unsustainable approach (the more migrations # you'll amass, the slower it'll run and the greater likelihood for issues). # -# It's strongly recommended to check this file into your version control system. +# It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(:version => 20140407215513) do +ActiveRecord::Schema.define(version: 20151228230340) do - create_table "users", :force => true do |t| - t.string "email", :default => "", :null => false - t.string "encrypted_password", :default => "", :null => false + create_table "users", force: :cascade do |t| + t.string "email", default: "", null: false + t.string "encrypted_password", default: "", null: false t.string "reset_password_token" t.datetime "reset_password_sent_at" t.datetime "remember_created_at" - t.integer "sign_in_count", :default => 0, :null => false + t.integer "sign_in_count", default: 0, null: false t.datetime "current_sign_in_at" t.datetime "last_sign_in_at" t.string "current_sign_in_ip" t.string "last_sign_in_ip" - t.datetime "created_at", :null => false - t.datetime "updated_at", :null => false - t.string "otp_secret_key" - t.integer "second_factor_attempts_count", :default => 0 - t.string "nickname", :limit => 64 + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.integer "second_factor_attempts_count", default: 0 + t.string "nickname", limit: 64 + t.string "encrypted_otp_secret_key" + t.string "encrypted_otp_secret_key_iv" + t.string "encrypted_otp_secret_key_salt" end - add_index "users", ["email"], :name => "index_users_on_email", :unique => true - add_index "users", ["otp_secret_key"], :name => "index_users_on_otp_secret_key", :unique => true - add_index "users", ["reset_password_token"], :name => "index_users_on_reset_password_token", :unique => true + add_index "users", ["email"], name: "index_users_on_email", unique: true + add_index "users", ["encrypted_otp_secret_key"], name: "index_users_on_encrypted_otp_secret_key", unique: true + add_index "users", ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 32803c5..6370433 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -3,6 +3,7 @@ require File.expand_path("../rails_app/config/environment.rb", __FILE__) require 'rspec/rails' require 'timecop' +require 'rack_session_access/capybara' # See http://rubydoc.info/gems/rspec-core/RSpec/Core/Configuration RSpec.configure do |config| diff --git a/spec/support/authenticated_model_helper.rb b/spec/support/authenticated_model_helper.rb index d6205a3..50b2c6e 100644 --- a/spec/support/authenticated_model_helper.rb +++ b/spec/support/authenticated_model_helper.rb @@ -1,10 +1,11 @@ module AuthenticatedModelHelper - def build_guest_user GuestUser.new end - def create_user(attributes={}) + def create_user(type = 'encrypted', attributes = {}) + create_table_for_nonencrypted_user if type == 'not_encrypted' + User.create!(valid_attributes(attributes)) end @@ -23,6 +24,29 @@ module AuthenticatedModelHelper "user#{@@email_count}@example.com" end + def create_table_for_nonencrypted_user + silence_stream(STDOUT) do + ActiveRecord::Schema.define(version: 1) do + create_table 'users', force: :cascade do |t| + t.string 'email', default: '', null: false + t.string 'encrypted_password', default: '', null: false + t.string 'reset_password_token' + t.datetime 'reset_password_sent_at' + t.datetime 'remember_created_at' + t.integer 'sign_in_count', default: 0, null: false + t.datetime 'current_sign_in_at' + t.datetime 'last_sign_in_at' + t.string 'current_sign_in_ip' + t.string 'last_sign_in_ip' + t.datetime 'created_at', null: false + t.datetime 'updated_at', null: false + t.integer 'second_factor_attempts_count', default: 0 + t.string 'nickname', limit: 64 + t.string 'otp_secret_key' + end + end + end + end end RSpec.configuration.send(:include, AuthenticatedModelHelper) diff --git a/spec/support/controller_helper.rb b/spec/support/controller_helper.rb new file mode 100644 index 0000000..ea20b83 --- /dev/null +++ b/spec/support/controller_helper.rb @@ -0,0 +1,16 @@ +module ControllerHelper + def sign_in(user = create_user('not_encrypted')) + allow(warden).to receive(:authenticated?).with(:user).and_return(true) + allow(controller).to receive(:current_user).and_return(user) + warden.session(:user)[TwoFactorAuthentication::NEED_AUTHENTICATION] = true + end +end + +RSpec.configure do |config| + config.include Devise::TestHelpers, type: :controller + config.include ControllerHelper, type: :controller + + config.before(:example, type: :controller) do + @request.env['devise.mapping'] = Devise.mappings[:user] + end +end diff --git a/spec/support/features_spec_helper.rb b/spec/support/features_spec_helper.rb index 425bb5b..e5aaa3b 100644 --- a/spec/support/features_spec_helper.rb +++ b/spec/support/features_spec_helper.rb @@ -15,5 +15,13 @@ end RSpec.configure do |config| config.include Warden::Test::Helpers, type: :feature config.include FeaturesSpecHelper, type: :feature + + config.before(:each) do + Warden.test_mode! + end + + config.after(:each) do + Warden.test_reset! + end end diff --git a/two_factor_authentication.gemspec b/two_factor_authentication.gemspec index 148dd0a..a9f23f3 100644 --- a/two_factor_authentication.gemspec +++ b/two_factor_authentication.gemspec @@ -28,6 +28,7 @@ Gem::Specification.new do |s| s.add_runtime_dependency 'devise' s.add_runtime_dependency 'randexp' s.add_runtime_dependency 'rotp' + s.add_runtime_dependency 'encryptor' s.add_development_dependency 'bundler' s.add_development_dependency 'rake'