From bc7efaf8b3f2da5f78b7096b1c3174be36de1497 Mon Sep 17 00:00:00 2001 From: David Alexander Date: Tue, 16 Feb 2016 10:46:05 -0500 Subject: [PATCH 1/5] Fixes failing tests and indentation inconsistency --- Gemfile | 1 + Rakefile | 2 +- .../controllers/helpers.rb | 3 +- .../controllers/helpers_test.rb | 46 +++--- test/model_tests_helper.rb | 23 ++- test/models/google_authenticatable_test.rb | 131 +++++++++--------- 6 files changed, 101 insertions(+), 105 deletions(-) diff --git a/Gemfile b/Gemfile index 72faacf..f4928c5 100644 --- a/Gemfile +++ b/Gemfile @@ -14,5 +14,6 @@ group :test do gem 'timecop' gem 'railties' gem 'actionmailer' + gem 'test-unit' # gem 'debugger' end diff --git a/Rakefile b/Rakefile index 218c9dc..bf20a7b 100644 --- a/Rakefile +++ b/Rakefile @@ -18,4 +18,4 @@ task :tests do orm = File.basename(file).split(".").first system "rake test DEVISE_ORM=#{orm}" end -end \ No newline at end of file +end diff --git a/lib/devise_google_authenticatable/controllers/helpers.rb b/lib/devise_google_authenticatable/controllers/helpers.rb index 61ade7a..ffdef40 100644 --- a/lib/devise_google_authenticatable/controllers/helpers.rb +++ b/lib/devise_google_authenticatable/controllers/helpers.rb @@ -16,9 +16,8 @@ def otpauth_user(username, app, qualifier=nil) end def username_from_email(email) - (/^(.*)@/).match(email)[1] + ((/^(.*)@/).match(email) || [])[1] end - end end end diff --git a/test/lib/devise_google_authenticatable/controllers/helpers_test.rb b/test/lib/devise_google_authenticatable/controllers/helpers_test.rb index 4c0d9d8..8e0a571 100644 --- a/test/lib/devise_google_authenticatable/controllers/helpers_test.rb +++ b/test/lib/devise_google_authenticatable/controllers/helpers_test.rb @@ -2,32 +2,32 @@ require 'devise_google_authenticatable/controllers/helpers' class HelpersTest < ActiveSupport::TestCase - include DeviseGoogleAuthenticator::Controllers::Helpers + include DeviseGoogleAuthenticator::Controllers::Helpers - def setup - @user = User.new(valid_attributes({:email => 'helpers_test@test.com' })) - end + def setup + @user = User.new(valid_attributes({:email => 'helpers_test@test.com' })) + end - test "can get username from user's email" do - assert_equal 'helpers_test', username_from_email(@user.email) - end + test "can get username from user's email" do + assert_equal 'helpers_test', username_from_email(@user.email) + end - test 'can get otpauth_user' do - assert_equal "username@app", otpauth_user('username', 'app') - end + test 'can get otpauth_user' do + assert_equal "username@app", otpauth_user('username', 'app') + end - test 'can get otpauth_user with a qualifier' do - assert_equal "username@app-qualifier", otpauth_user('username', 'app', '-qualifier') - end + test 'can get otpauth_user with a qualifier' do + assert_equal "username@app-qualifier", otpauth_user('username', 'app', '-qualifier') + end - # fake image tag - def image_tag(src, *args) - src - end - test 'generate qrcode' do - assert_equal "https://chart.googleapis.com/chart?chs=200x200&chld=M|0&cht=qr&chl=otpauth%3A%2F%2Ftotp%2Fhelpers_test%40RailsApp%3Fsecret%3D", google_authenticator_qrcode(@user) - assert_equal "https://chart.googleapis.com/chart?chs=200x200&chld=M|0&cht=qr&chl=otpauth%3A%2F%2Ftotp%2Fhelpers_test%40RailsAppMyQualifier%3Fsecret%3D", google_authenticator_qrcode(@user, 'MyQualifier') - assert_equal "https://chart.googleapis.com/chart?chs=200x200&chld=M|0&cht=qr&chl=otpauth%3A%2F%2Ftotp%2Fhelpers_test%40RailsApp%3Fsecret%3D%26issuer%3DMyIssuer", google_authenticator_qrcode(@user, nil, 'MyIssuer') - assert_equal "https://chart.googleapis.com/chart?chs=200x200&chld=M|0&cht=qr&chl=otpauth%3A%2F%2Ftotp%2Fhelpers_test%40RailsAppMyQualifier%3Fsecret%3D%26issuer%3DMyIssuer", google_authenticator_qrcode(@user, 'MyQualifier', 'MyIssuer') - end + # fake image tag + def image_tag(src, *args) + src + end + test 'generate qrcode' do + assert_equal "https://chart.googleapis.com/chart?chs=200x200&chld=M|0&cht=qr&chl=otpauth%3A%2F%2Ftotp%2Fhelpers_test%40RailsApp%3Fsecret%3D", google_authenticator_qrcode(@user) + assert_equal "https://chart.googleapis.com/chart?chs=200x200&chld=M|0&cht=qr&chl=otpauth%3A%2F%2Ftotp%2Fhelpers_test%40RailsAppMyQualifier%3Fsecret%3D", google_authenticator_qrcode(@user, 'MyQualifier') + assert_equal "https://chart.googleapis.com/chart?chs=200x200&chld=M|0&cht=qr&chl=otpauth%3A%2F%2Ftotp%2Fhelpers_test%40RailsApp%3Fsecret%3D%26issuer%3DMyIssuer", google_authenticator_qrcode(@user, nil, 'MyIssuer') + assert_equal "https://chart.googleapis.com/chart?chs=200x200&chld=M|0&cht=qr&chl=otpauth%3A%2F%2Ftotp%2Fhelpers_test%40RailsAppMyQualifier%3Fsecret%3D%26issuer%3DMyIssuer", google_authenticator_qrcode(@user, 'MyQualifier', 'MyIssuer') + end end diff --git a/test/model_tests_helper.rb b/test/model_tests_helper.rb index 3f16308..bb8b04d 100644 --- a/test/model_tests_helper.rb +++ b/test/model_tests_helper.rb @@ -1,23 +1,22 @@ +# Helpers for creating new users class ActiveSupport::TestCase - - # Helpers for creating new users - # - def generate_unique_email + def generate_unique_email #:nodoc: @@email_count ||= 0 @@email_count += 1 - "test#{@@email_count}@email.com" + "test#{@@email_count}@example.com" end - def valid_attributes(attributes={}) - { :email => generate_unique_email, - :password => '123456', - :password_confirmation => '123456' }.update(attributes) + def valid_attributes(attributes = {}) #:nodoc: + { + email: generate_unique_email, + password: '123456', + password_confirmation: '123456' + }.merge(attributes) end - def new_user(attributes={}) + def new_user(attributes = {}) #:nodoc: user = User.new(valid_attributes(attributes)) - user.save + user.save! user end - end diff --git a/test/models/google_authenticatable_test.rb b/test/models/google_authenticatable_test.rb index 97f0748..a7ae779 100644 --- a/test/models/google_authenticatable_test.rb +++ b/test/models/google_authenticatable_test.rb @@ -2,71 +2,68 @@ require 'model_tests_helper' class GoogleAuthenticatableTest < ActiveSupport::TestCase + def setup + @u = new_user + end - def setup - new_user - end - - test 'new users have a non-nil secret set' do - assert_not_nil User.find(1).gauth_secret - end - - test 'new users should have gauth_enabled disabled by default' do - assert_equal 0, User.find(1).gauth_enabled.to_i - end - - test 'get_qr method works' do - assert_not_nil User.find(1).get_qr - end - - test 'updating gauth_enabled to true' do - User.find(1).set_gauth_enabled(1) - assert_equal 1, User.find(1).gauth_enabled.to_i - end - - test 'updating gauth_enabled back to false' do - User.find(1).set_gauth_enabled(0) - assert_equal 0, User.find(1).gauth_enabled.to_i - end - - test 'updating the gauth_tmp key' do - User.find(1).assign_tmp - - assert_not_nil User.find(1).gauth_tmp - assert_not_nil User.find(1).gauth_tmp_datetime - - sleep(1) - - old_tmp = User.find(1).gauth_tmp - old_dt = User.find(1).gauth_tmp_datetime - - User.find(1).assign_tmp - - assert_not_equal old_tmp, User.find(1).gauth_tmp - assert_not_equal old_dt, User.find(1).gauth_tmp_datetime - end - - test 'testing class method for finding by tmp key' do - assert User.find_by_gauth_tmp('invalid').nil? - assert !User.find_by_gauth_tmp(User.find(1).gauth_tmp).nil? - end - - test 'testing token validation' do - assert !User.find(1).validate_token('1') - assert !User.find(1).validate_token(ROTP::TOTP.new(User.find(1).get_qr).at(Time.now)) - - User.find(1).assign_tmp - - assert !User.find(1).validate_token('1') - assert User.find(1).validate_token(ROTP::TOTP.new(User.find(1).get_qr).at(Time.now)) - end - - test 'requiring token after remembertime' do - u = User.find(1) - assert u.require_token?(nil) - assert u.require_token?(u.email + "," + 2.months.ago.to_i.to_s) - assert !u.require_token?(u.email + "," + 1.day.ago.to_i.to_s) - assert u.require_token?("testxx@test.com" + "," + 1.day.ago.to_i.to_s) - end - -end \ No newline at end of file + test 'new users have a non-nil secret set' do + assert_not_nil @u.gauth_secret + end + + test 'new users should have gauth_enabled disabled by default' do + assert_equal 0, @u.gauth_enabled.to_i, "Google auth: #{@u.gauth_enabled.inspect}" + end + + test 'get_qr method works' do + assert_not_nil @u.get_qr + end + + test 'updating gauth_enabled to true' do + @u.set_gauth_enabled(1) + assert_equal 1, @u.gauth_enabled.to_i + end + + test 'updating gauth_enabled back to false' do + @u.set_gauth_enabled(0) + assert_equal 0, @u.gauth_enabled.to_i + end + + test 'updating the gauth_tmp key' do + @u.assign_tmp + + assert_not_nil @u.gauth_tmp + assert_not_nil @u.gauth_tmp_datetime + + sleep(1) + + old_tmp = @u.gauth_tmp.dup + old_dt = @u.gauth_tmp_datetime.dup + + @u.assign_tmp + + assert_not_equal old_tmp, @u.gauth_tmp + assert_not_equal old_dt, @u.gauth_tmp_datetime + end + + test 'class method for finding by tmp key' do + assert User.find_by_gauth_tmp('invalid').nil?, 'Found user by token "invalid"' + assert !User.find_by_gauth_tmp(@u.gauth_tmp).nil?, 'Unable to find user' + end + + test 'token validation' do + assert !@u.validate_token('1') + assert !@u.validate_token(ROTP::TOTP.new(@u.get_qr).at(Time.now)) + + @u.assign_tmp + + assert !@u.validate_token('1') + assert @u.validate_token(ROTP::TOTP.new(@u.get_qr).at(Time.now)) + end + + test 'requiring token after remembertime' do + assert @u.require_token?(nil) + assert @u.require_token?(@u.email + ',' + 2.months.ago.to_i.to_s) + assert !@u.require_token?(@u.email + ',' + 1.day.ago.to_i.to_s) + assert @u.require_token?('some_other_email@example.com' + ',' + 1.day.ago.to_i.to_s) + end +end From 9418d568da464854ea7190f0aab605d2c6aa141c Mon Sep 17 00:00:00 2001 From: David Alexander Date: Tue, 16 Feb 2016 11:34:48 -0500 Subject: [PATCH 2/5] Standardizes indentation, comment style and hash syntax for ruby1.9 --- Gemfile | 1 - app/controllers/devise/checkga_controller.rb | 47 ++++++------ .../devise/displayqr_controller.rb | 48 +++++++------ app/views/devise/checkga/show.html.erb | 12 ++-- app/views/devise/displayqr/show.html.erb | 24 +++---- .../controllers/helpers.rb | 8 +-- .../models/google_authenticatable.rb | 71 ++++++++----------- .../orm/active_record.rb | 8 +-- .../patches/check_ga.rb | 35 +++++---- .../patches/display_qr.rb | 55 +++++++------- lib/devise_google_authenticatable/rails.rb | 1 - lib/devise_google_authenticatable/routes.rb | 12 ++-- lib/devise_google_authenticatable/schema.rb | 12 ++-- lib/devise_google_authenticator.rb | 27 +++---- .../active_record/templates/migration.rb | 2 +- .../devise_google_authenticator_generator.rb | 2 +- .../install_generator.rb | 41 ++++++----- .../views_generator.rb | 8 ++- 18 files changed, 202 insertions(+), 212 deletions(-) diff --git a/Gemfile b/Gemfile index f4928c5..73ca909 100644 --- a/Gemfile +++ b/Gemfile @@ -15,5 +15,4 @@ group :test do gem 'railties' gem 'actionmailer' gem 'test-unit' - # gem 'debugger' end diff --git a/app/controllers/devise/checkga_controller.rb b/app/controllers/devise/checkga_controller.rb index 56f1165..d452979 100644 --- a/app/controllers/devise/checkga_controller.rb +++ b/app/controllers/devise/checkga_controller.rb @@ -1,8 +1,9 @@ class Devise::CheckgaController < Devise::SessionsController - prepend_before_filter :devise_resource, :only => [:show] - prepend_before_filter :require_no_authentication, :only => [ :show, :update ] + prepend_before_filter :devise_resource, only: [:show] + prepend_before_filter :require_no_authentication, only: [:show, :update] include Devise::Controllers::Helpers + class ErrorSigningIn < StandardError; end def show @tmpid = params[:id] @@ -16,30 +17,24 @@ def show def update resource = resource_class.find_by_gauth_tmp(params[resource_name]['tmpid']) - if not resource.nil? - - if resource.validate_token(params[resource_name]['gauth_token'].to_i) - set_flash_message(:notice, :signed_in) if is_navigational_format? - sign_in(resource_name,resource) - warden.manager._run_callbacks(:after_set_user, resource, warden, {:event => :authentication}) - respond_with resource, :location => after_sign_in_path_for(resource) - - if not resource.class.ga_remembertime.nil? - cookies.signed[:gauth] = { - :value => resource.email << "," << Time.now.to_i.to_s, - :secure => !(Rails.env.test? || Rails.env.development?), - :expires => (resource.class.ga_remembertime + 1.days).from_now - } - end - else - set_flash_message(:error, :error) - redirect_to :root - end + fail ErrorSigningIn unless resource + fail ErrorSigningIn unless resource.validate_token(params[resource_name]['gauth_token'].to_i) - else - set_flash_message(:error, :error) - redirect_to :root - end + set_flash_message(:notice, :signed_in) if is_navigational_format? + sign_in(resource_name,resource) + warden.manager._run_callbacks(:after_set_user, resource, warden, event: :authentication) + + gauth_cookie = { + value: [resource.email, Time.now.to_i.to_s].join(','), + secure: !(Rails.env.test? || Rails.env.development?), + expires: (resource.class.ga_remembertime + 1.day).from_now + } + cookies.signed[:gauth] = gauth_cookie if resource.class.ga_remembertime + + respond_with resource, location: after_sign_in_path_for(resource) + rescue ErrorSigningIn + set_flash_message(:error, :error) + redirect_to :root end private @@ -47,4 +42,4 @@ def update def devise_resource self.resource = resource_class.new end -end \ No newline at end of file +end diff --git a/app/controllers/devise/displayqr_controller.rb b/app/controllers/devise/displayqr_controller.rb index c46f6fb..4fb3241 100644 --- a/app/controllers/devise/displayqr_controller.rb +++ b/app/controllers/devise/displayqr_controller.rb @@ -1,41 +1,43 @@ class Devise::DisplayqrController < DeviseController - prepend_before_filter :authenticate_scope!, :only => [:show, :update, :refresh] + prepend_before_filter :authenticate_scope!, only: [:show, :update, :refresh] include Devise::Controllers::Helpers + class InvalidToken < StandardError; end - # GET /resource/displayqr + # GET /{resource}/displayqr def show - if resource.nil? || resource.gauth_secret.nil? - sign_in resource_class.new, resource - redirect_to stored_location_for(scope) || :root - else + if resource && resource.gauth_secret @tmpid = resource.assign_tmp render :show + else + sign_in resource_class.new, resource + redirect_to stored_location_for(scope) || :root end end def update - if resource.gauth_tmp != params[resource_name]['tmpid'] || !resource.validate_token(params[resource_name]['gauth_token'].to_i) - set_flash_message(:error, :invalid_token) - render :show - return - end + fail InvalidToken if resource.gauth_tmp != params[resource_name]['tmpid'] + fail InvalidToken unless resource.validate_token(params[resource_name]['gauth_token'].to_i) if resource.set_gauth_enabled(params[resource_name]['gauth_enabled']) set_flash_message :notice, (resource.gauth_enabled? ? :enabled : :disabled) - sign_in scope, resource, :bypass => true + sign_in scope, resource, bypass: true redirect_to stored_location_for(scope) || :root else render :show end + rescue InvalidToken + set_flash_message(:error, :invalid_token) + render :show end def refresh - unless resource.nil? + if resource resource.send(:assign_auth_secret) resource.save - set_flash_message :notice, :newtoken - sign_in scope, resource, :bypass => true + + set_flash_message(:notice, :newtoken) + sign_in(scope, resource, bypass: true) redirect_to [resource_name, :displayqr] else redirect_to :root @@ -43,22 +45,22 @@ def refresh end private + def scope resource_name.to_sym end def authenticate_scope! - send(:"authenticate_#{resource_name}!") + send("authenticate_#{resource_name}!") self.resource = send("current_#{resource_name}") end # 7/2/15 - Unsure if this is used anymore - @xntrik def resource_params - return params.require(resource_name.to_sym).permit(:gauth_enabled) if strong_parameters_enabled? - params - end - - def strong_parameters_enabled? - defined?(ActionController::StrongParameters) + if defined?(ActionController::StrongParameters) + params.require(resource_name.to_sym).permit(:gauth_enabled) + else + params + end end -end \ No newline at end of file +end diff --git a/app/views/devise/checkga/show.html.erb b/app/views/devise/checkga/show.html.erb index 18f7e84..3581b6a 100644 --- a/app/views/devise/checkga/show.html.erb +++ b/app/views/devise/checkga/show.html.erb @@ -1,7 +1,7 @@ -

<%= I18n.t('submit_token_title', {:scope => 'devise'}) %>

+

<%= I18n.t('submit_token_title', scope: 'devise') %>

-<%= form_for(resource, :as => resource_name, :url => [resource_name, :checkga], :html => { :method => :put }) do |f| %> - <%= f.hidden_field :tmpid, {:value => @tmpid} %> - <%= f.text_field :gauth_token, :autocomplete => :off%> -

<%= f.submit I18n.t('submit_token', {:scope => 'devise'}) %>

-<% end %> \ No newline at end of file +<%= form_for(resource, as: resource_name, url: [resource_name, :checkga], html: { method: :put }) do |f| %> + <%= f.hidden_field :tmpid, value: @tmpid %> + <%= f.text_field :gauth_token, autocomplete: :off %> +

<%= f.submit I18n.t('submit_token', scope: 'devise') %>

+<% end %> diff --git a/app/views/devise/displayqr/show.html.erb b/app/views/devise/displayqr/show.html.erb index be2e144..7ae7566 100644 --- a/app/views/devise/displayqr/show.html.erb +++ b/app/views/devise/displayqr/show.html.erb @@ -1,20 +1,20 @@ -

<%= I18n.t('title', {:scope => 'devise.registration'}) %>

+

<%= I18n.t('title', scope: 'devise.registration') %>

<%= google_authenticator_qrcode(resource) %> -<%= form_for(resource, :as => resource_name, :url => [:refresh, resource_name, :displayqr], :html => {:method => :post}) do |f|%> -

<%= f.submit I18n.t('newtoken', {:scope => 'devise.registration'}) %>

+<%= form_for(resource, as: resource_name, url: [:refresh, resource_name, :displayqr], html: { method: :post }) do |f|%> +

<%= f.submit I18n.t('newtoken', scope: 'devise.registration') %>

<% end %> -<%= form_for(resource, :as => resource_name, :url => [resource_name, :displayqr], :html => { :method => :put }) do |f| %> - <%= devise_error_messages! %> -

<%= I18n.t('nice_request', {:scope => 'devise.registration'}) %>

-

<%= f.label :gauth_enabled, I18n.t('qrstatus', {:scope => 'devise.registration'}) %>
- <%= f.check_box :gauth_enabled %>

- <%= f.hidden_field :tmpid, value: @tmpid %> -

<%= f.label :gauth_token, I18n.t('enter_token', {:scope => 'devise.registration'}) %>
- <%= f.number_field :gauth_token, :autocomplete => :off %> +<%= form_for(resource, as: resource_name, url: [resource_name, :displayqr], html: { method: :put }) do |f| %> + <%= devise_error_messages! %> +

<%= I18n.t('nice_request', scope: 'devise.registration') %>

+

<%= f.label :gauth_enabled, I18n.t('qrstatus', scope: 'devise.registration') %>
+ <%= f.check_box :gauth_enabled %>

+ <%= f.hidden_field :tmpid, value: @tmpid %> +

<%= f.label :gauth_token, I18n.t('enter_token', scope: 'devise.registration') %>
+ <%= f.number_field :gauth_token, autocomplete: :off %> -

<%= f.submit I18n.t('submit', {:scope => 'devise.registration'}) %>

+

<%= f.submit I18n.t('submit', scope: 'devise.registration') %>

<% end %> diff --git a/lib/devise_google_authenticatable/controllers/helpers.rb b/lib/devise_google_authenticatable/controllers/helpers.rb index ffdef40..817e2f2 100644 --- a/lib/devise_google_authenticatable/controllers/helpers.rb +++ b/lib/devise_google_authenticatable/controllers/helpers.rb @@ -1,17 +1,17 @@ -module DeviseGoogleAuthenticator +module DeviseGoogleAuthenticator #:nodoc: module Controllers # :nodoc: module Helpers # :nodoc: - def google_authenticator_qrcode(user, qualifier=nil, issuer=nil) + def google_authenticator_qrcode(user, qualifier = nil, issuer = nil) username = username_from_email(user.email) app = user.class.ga_appname || Rails.application.class.parent_name data = "otpauth://totp/#{otpauth_user(username, app, qualifier)}?secret=#{user.gauth_secret}" data << "&issuer=#{issuer}" if !issuer.nil? data = Rack::Utils.escape(data) url = "https://chart.googleapis.com/chart?chs=200x200&chld=M|0&cht=qr&chl=#{data}" - return image_tag(url, :alt => 'Google Authenticator QRCode') + return image_tag(url, alt: 'Google Authenticator QRCode') end - def otpauth_user(username, app, qualifier=nil) + def otpauth_user(username, app, qualifier = nil) "#{username}@#{app}#{qualifier}" end diff --git a/lib/devise_google_authenticatable/models/google_authenticatable.rb b/lib/devise_google_authenticatable/models/google_authenticatable.rb index 8c3f66b..f3684f1 100644 --- a/lib/devise_google_authenticatable/models/google_authenticatable.rb +++ b/lib/devise_google_authenticatable/models/google_authenticatable.rb @@ -1,11 +1,9 @@ require 'rotp' -module Devise # :nodoc: - module Models # :nodoc: - - module GoogleAuthenticatable - - def self.included(base) # :nodoc: +module Devise #:nodoc: + module Models #:nodoc: + module GoogleAuthenticatable #:nodoc: + def self.included(base) #:nodoc: base.extend ClassMethods base.class_eval do @@ -14,66 +12,55 @@ def self.included(base) # :nodoc: end end - module InstanceMethods # :nodoc: + module InstanceMethods #:nodoc: def get_qr self.gauth_secret end def set_gauth_enabled(param) #self.update_without_password(params[gauth_enabled]) - self.update_attributes(:gauth_enabled => param) + self.update_attributes(gauth_enabled: param) end def assign_tmp - self.update_attributes(:gauth_tmp => ROTP::Base32.random_base32(32), :gauth_tmp_datetime => DateTime.now) + self.update_attributes(gauth_tmp: ROTP::Base32.random_base32(32), gauth_tmp_datetime: DateTime.now) self.gauth_tmp end def validate_token(token) - return false if self.gauth_tmp_datetime.nil? - if self.gauth_tmp_datetime < self.class.ga_timeout.ago - return false - else - - valid_vals = [] - valid_vals << ROTP::TOTP.new(self.get_qr).at(Time.now) - (1..self.class.ga_timedrift).each do |cc| - valid_vals << ROTP::TOTP.new(self.get_qr).at(Time.now.ago(30*cc)) - valid_vals << ROTP::TOTP.new(self.get_qr).at(Time.now.in(30*cc)) - end - - if valid_vals.include?(token.to_i) - return true - else - return false - end + return false unless self.gauth_tmp_datetime + return false if self.gauth_tmp_datetime < self.class.ga_timeout.ago + + valid_vals = [] + valid_vals << ROTP::TOTP.new(self.get_qr).at(Time.now) + (1..self.class.ga_timedrift).each do |cc| + valid_vals << ROTP::TOTP.new(self.get_qr).at(Time.now.ago(30*cc)) + valid_vals << ROTP::TOTP.new(self.get_qr).at(Time.now.in(30*cc)) end + + return valid_vals.include?(token.to_i) end def gauth_enabled? - # Active_record seems to handle determining the status better this way - if self.gauth_enabled.respond_to?("to_i") - if self.gauth_enabled.to_i != 0 - return true - else - return false - end - # Mongoid does NOT have a .to_i for the Boolean return value, hence, we can just return it + if self.gauth_enabled.respond_to?(:to_i) + # Active_record seems to handle determining the status better this way + return self.gauth_enabled.to_i != 0 else + # Mongoid does NOT have a .to_i for the Boolean return value, hence we can just return it return self.gauth_enabled end end def require_token?(cookie) - if self.class.ga_remembertime.nil? || cookie.blank? - return true - end + return true if self.class.ga_remembertime.nil? || cookie.blank? + array = cookie.to_s.split ',' - if array.count != 2 - return true - end + + return true if array.count != 2 + last_logged_in_email = array[0] last_logged_in_time = array[1].to_i + return last_logged_in_email != self.email || (Time.now.to_i - last_logged_in_time) > self.class.ga_remembertime.to_i end @@ -82,13 +69,13 @@ def require_token?(cookie) def assign_auth_secret self.gauth_secret = ROTP::Base32.random_base32(64) end - end - module ClassMethods # :nodoc: + module ClassMethods #:nodoc: def find_by_gauth_tmp(gauth_tmp) where(gauth_tmp: gauth_tmp).first end + ::Devise::Models.config(self, :ga_timeout, :ga_timedrift, :ga_remembertime, :ga_appname, :ga_bypass_signup) end end diff --git a/lib/devise_google_authenticatable/orm/active_record.rb b/lib/devise_google_authenticatable/orm/active_record.rb index b9d1d63..43ff3b8 100644 --- a/lib/devise_google_authenticatable/orm/active_record.rb +++ b/lib/devise_google_authenticatable/orm/active_record.rb @@ -1,5 +1,5 @@ -module DeviseGoogleAuthenticator - module Orm +module DeviseGoogleAuthenticator #:nodoc: + module Orm #:nodoc: # This module contains handle schema (migrations): # # create_table :accounts do |t| @@ -7,13 +7,11 @@ module Orm # t.gauth_enabled # end # - module ActiveRecord - module Schema + module Schema #:nodoc: include DeviseGoogleAuthenticator::Schema end end - end end diff --git a/lib/devise_google_authenticatable/patches/check_ga.rb b/lib/devise_google_authenticatable/patches/check_ga.rb index 25528f6..22faa92 100644 --- a/lib/devise_google_authenticatable/patches/check_ga.rb +++ b/lib/devise_google_authenticatable/patches/check_ga.rb @@ -1,10 +1,9 @@ -module DeviseGoogleAuthenticator::Patches +module DeviseGoogleAuthenticator::Patches #:nodoc: # patch Sessions controller to check that the OTP is accurate module CheckGA extend ActiveSupport::Concern - included do - # here the patch + included do alias_method :create_original, :create define_method :checkga_resource_path_name do |resource, id| @@ -14,24 +13,24 @@ module CheckGA end define_method :create do - - resource = warden.authenticate!(:scope => resource_name, :recall => "#{controller_path}#new") - - if resource.respond_to?(:get_qr) and resource.gauth_enabled? and resource.require_token?(cookies.signed[:gauth]) #Therefore we can quiz for a QR - tmpid = resource.assign_tmp #assign a temporary key and fetch it - warden.logout #log the user out - - #we head back into the checkga controller with the temporary id - #Because the model used for google auth may not always be the same, and may be a sub-model, the eval will evaluate the appropriate path name - #This change addresses https://github.com/AsteriskLabs/devise_google_authenticator/issues/7 - respond_with resource, :location => eval(checkga_resource_path_name(resource, tmpid)) - - else #It's not using, or not enabled for Google 2FA, OR is remembering token and therefore not asking for the moment - carry on, nothing to see here. + resource = warden.authenticate!(scope: resource_name, recall: "#{controller_path}#new") + + if resource.respond_to?(:get_qr) && resource.gauth_enabled? && resource.require_token?(cookies.signed[:gauth]) + tmpid = resource.assign_tmp + warden.logout + + # Because the model used for google auth may not always be the same + # (or may be a sub-model), we use eval to determine the appropriate + # path name. + # Reference: https://github.com/AsteriskLabs/devise_google_authenticator/issues/7 + respond_with resource, location: eval(checkga_resource_path_name(resource, tmpid)) + else + # 2FA is not active on this account or 'rememberme' is set and we + # already authenticated earlier. Nothing to see here, carry on. set_flash_message(:notice, :signed_in) if is_flashing_format? sign_in(resource_name, resource) - respond_with resource, :location => after_sign_in_path_for(resource) + respond_with resource, location: after_sign_in_path_for(resource) end - end end end diff --git a/lib/devise_google_authenticatable/patches/display_qr.rb b/lib/devise_google_authenticatable/patches/display_qr.rb index 1e705d1..df356e5 100644 --- a/lib/devise_google_authenticatable/patches/display_qr.rb +++ b/lib/devise_google_authenticatable/patches/display_qr.rb @@ -1,39 +1,42 @@ -module DeviseGoogleAuthenticator::Patches +module DeviseGoogleAuthenticator::Patches #:nodoc: # patch Registrations controller to display the QR code module DisplayQR extend ActiveSupport::Concern + included do - - #arrr be the patch + # arrr be the patch alias_method :create_original, :create define_method :create do build_resource(sign_up_params) - if resource.save - yield resource if block_given? - if resource.active_for_authentication? - set_flash_message :notice, :signed_up if is_flashing_format? - sign_in(resource_name, resource) - - if resource.respond_to? :gauth_enabled? - if resource.class.ga_bypass_signup - respond_with resource, location: after_sign_up_path_for(resource) - else - respond_with resource, :location => {:controller => 'displayqr', :action => 'show'} - end - else - respond_with resource, location: after_sign_up_path_for(resource) - end - - else - set_flash_message :notice, :"signed_up_but_#{resource.inactive_message}" if is_flashing_format? - expire_data_after_sign_in! - respond_with resource, :location => after_inactive_sign_up_path_for(resource) - end - else + unless resource.save + # Error! Abort! clean_up_passwords resource - respond_with resource + return respond_with resource + end + + yield resource if block_given? + + unless resource.active_for_authentication? + # + set_flash_message :notice, :"signed_up_but_#{resource.inactive_message}" if is_flashing_format? + expire_data_after_sign_in! + return respond_with resource, location: after_inactive_sign_up_path_for(resource) + end + + set_flash_message :notice, :signed_up if is_flashing_format? + + sign_in(resource_name, resource) + + unless resource.respond_to? :gauth_enabled? + return respond_with resource, location: after_sign_up_path_for(resource) + end + + if resource.class.ga_bypass_signup + respond_with resource, location: after_sign_up_path_for(resource) + else + respond_with resource, location: { controller: 'displayqr', action: 'show' } end end end diff --git a/lib/devise_google_authenticatable/rails.rb b/lib/devise_google_authenticatable/rails.rb index 7567d48..1f05539 100644 --- a/lib/devise_google_authenticatable/rails.rb +++ b/lib/devise_google_authenticatable/rails.rb @@ -3,6 +3,5 @@ class Engine < ::Rails::Engine # :nodoc: ActionDispatch::Callbacks.to_prepare do DeviseGoogleAuthenticator::Patches.apply end - end end diff --git a/lib/devise_google_authenticatable/routes.rb b/lib/devise_google_authenticatable/routes.rb index 5ec927e..adb6e33 100644 --- a/lib/devise_google_authenticatable/routes.rb +++ b/lib/devise_google_authenticatable/routes.rb @@ -1,16 +1,14 @@ -module ActionDispatch::Routing # :nodoc: - class Mapper # :nodoc: - +module ActionDispatch::Routing #:nodoc: + class Mapper #:nodoc: protected # route for handle expired passwords def devise_displayqr(mapping, controllers) - resource :displayqr, :only => [:show, :update], :path => mapping.path_names[:displayqr], :controller => controllers[:displayqr] do - post :refresh, :path => mapping.path_names[:refresh], :as => :refresh + resource :displayqr, only: [:show, :update], path: mapping.path_names[:displayqr], controller: controllers[:displayqr] do + post :refresh, path: mapping.path_names[:refresh], as: :refresh end - resource :checkga, :only => [:show, :update], :path => mapping.path_names[:checkga], :controller => controllers[:checkga] + resource :checkga, only: [:show, :update], path: mapping.path_names[:checkga], controller: controllers[:checkga] end - end end diff --git a/lib/devise_google_authenticatable/schema.rb b/lib/devise_google_authenticatable/schema.rb index 2bb4c73..d17147f 100644 --- a/lib/devise_google_authenticatable/schema.rb +++ b/lib/devise_google_authenticatable/schema.rb @@ -1,5 +1,5 @@ -module DeviseGoogleAuthenticator - # add schema helper for migrations +module DeviseGoogleAuthenticator #:nodoc: + # Schema helper for migrations module Schema # Add gauth_secret columns in the resource's database tables # @@ -9,7 +9,6 @@ module Schema # create_table :the_resources do |t| # t.gauth_secret # t.gauth_enabled - # ... # end # # # or if the resource's table already exists, define a migration and put this in: @@ -21,18 +20,17 @@ module Schema def gauth_secret apply_devise_schema :gauth_secret, String end - + def gauth_enabled apply_devise_schema :gauth_enabled, Integer, {:default => 0} end def gauth_tmp - apply_devise_schema :gauth_tmp, String + apply_devise_schema :gauth_tmp, String end def gauth_tmp_datetime - apply_devise_schema :gauth_tmp_datetime, Datetime + apply_devise_schema :gauth_tmp_datetime, Datetime end - end end diff --git a/lib/devise_google_authenticator.rb b/lib/devise_google_authenticator.rb index 9cd1b3e..4410ac2 100644 --- a/lib/devise_google_authenticator.rb +++ b/lib/devise_google_authenticator.rb @@ -6,20 +6,20 @@ require 'devise' module Devise # :nodoc: - mattr_accessor :ga_timeout - @@ga_timeout = 3.minutes + mattr_accessor :ga_timeout + @@ga_timeout = 3.minutes - mattr_accessor :ga_timedrift - @@ga_timedrift = 3 + mattr_accessor :ga_timedrift + @@ga_timedrift = 3 - mattr_accessor :ga_remembertime - @@ga_remembertime = 1.month + mattr_accessor :ga_remembertime + @@ga_remembertime = 1.month - mattr_accessor :ga_appname - @@ga_appname = Rails.application.class.parent_name + mattr_accessor :ga_appname + @@ga_appname = Rails.application.class.parent_name - mattr_accessor :ga_bypass_signup - @@ga_bypass_signup = false + mattr_accessor :ga_bypass_signup + @@ga_bypass_signup = false end # a security extension for devise @@ -28,12 +28,13 @@ module DeviseGoogleAuthenticator autoload :Patches, 'devise_google_authenticatable/patches' end - - require 'devise_google_authenticatable/routes' require 'devise_google_authenticatable/rails' require 'devise_google_authenticatable/orm/active_record' require 'devise_google_authenticatable/controllers/helpers' ActionView::Base.send :include, DeviseGoogleAuthenticator::Controllers::Helpers -Devise.add_module :google_authenticatable, :controller => :google_authenticatable, :model => 'devise_google_authenticatable/models/google_authenticatable', :route => :displayqr \ No newline at end of file +Devise.add_module :google_authenticatable, + controller: :google_authenticatable, + model: 'devise_google_authenticatable/models/google_authenticatable', + route: :displayqr diff --git a/lib/generators/active_record/templates/migration.rb b/lib/generators/active_record/templates/migration.rb index e51e420..e475ce5 100644 --- a/lib/generators/active_record/templates/migration.rb +++ b/lib/generators/active_record/templates/migration.rb @@ -8,7 +8,7 @@ def self.up end end - + def self.down change_table :<%= table_name %> do |t| t.remove :gauth_secret, :gauth_enabled, :gauth_tmp, :gauth_tmp_datetime diff --git a/lib/generators/devise_google_authenticator/devise_google_authenticator_generator.rb b/lib/generators/devise_google_authenticator/devise_google_authenticator_generator.rb index 0598754..7ec4382 100644 --- a/lib/generators/devise_google_authenticator/devise_google_authenticator_generator.rb +++ b/lib/generators/devise_google_authenticator/devise_google_authenticator_generator.rb @@ -34,4 +34,4 @@ def strong_parameters_enabled? end end -end \ No newline at end of file +end diff --git a/lib/generators/devise_google_authenticator/install_generator.rb b/lib/generators/devise_google_authenticator/install_generator.rb index 7c1cea8..4a8e4e7 100644 --- a/lib/generators/devise_google_authenticator/install_generator.rb +++ b/lib/generators/devise_google_authenticator/install_generator.rb @@ -7,21 +7,30 @@ class InstallGenerator < Rails::Generators::Base desc "Install the devise google authenticator extension" def add_configs - inject_into_file "config/initializers/devise.rb", "\n # ==> Devise Google Authenticator Extension\n # Configure extension for devise\n\n" + - " # How long should the user have to enter their token. To change the default, uncomment and change the below:\n" + - " # config.ga_timeout = 3.minutes\n\n" + - " # Change time drift settings for valid token values. To change the default, uncomment and change the below:\n" + - " # config.ga_timedrift = 3\n\n" + - " # Change setting to how long to remember device before requiring another token. Change to nil to turn feature off.\n" + - " # To change the default, uncomment and change the below:\n" + - " # config.ga_remembertime = 1.month\n\n" + - " # Change setting to assign the application name used by code generator. Defaults to Rails.application.class.parent_name.\n" + - " # To change the default, uncomment and change the below:\n" + - " # config.ga_appname = 'example.com'\n\n" + - " # Change setting to bypass the Display QR page immediately after a user sign's up\n" + - " # To change the default, uncomment and change the below. Defaults to false:\n" + - " # config.ga_bypass_signup = true\n\n" + - "\n", :before => /end[ |\n|]+\Z/ + default_config_instructions = <<-HEREDOC + # ==> Devise Google Authenticator Extension + # Configure extension for devise + + # How long should the user have to enter their token. To change the default, uncomment and change the line below: + # config.ga_timeout = 3.minutes + + # Change time drift settings for valid token values. To change the default, uncomment and change the line below: + # config.ga_timedrift = 3 + + # Change setting to how long to remember devise before requiring another token. Change to nil to turn feature off. + # To change the default, uncomment and change the line below: + # config.ga_remembertime = 1.month + + # Change setting to assign the application name used by code generator. Defaults to `Rails.application.class.parent_name`. + # To change the default, uncomment and change the line below: + # config.ga_appname = 'example.com' + + # Change setting to bypass the Display QR page immediately after a user signs up + # To change the default, uncomment and change the line below. [Default: false] + # config.ga_bypass_signup = true + HEREDOC + + inject_into_file "config/initializers/devise.rb", default_config_instructions, before: /end\s*\Z/ end def copy_locale @@ -29,4 +38,4 @@ def copy_locale end end end -end \ No newline at end of file +end diff --git a/lib/generators/devise_google_authenticator/views_generator.rb b/lib/generators/devise_google_authenticator/views_generator.rb index 77cae5d..73e5a91 100644 --- a/lib/generators/devise_google_authenticator/views_generator.rb +++ b/lib/generators/devise_google_authenticator/views_generator.rb @@ -5,11 +5,13 @@ module Generators class ViewsGenerator < Rails::Generators::Base desc 'Copies all Devise Google Authenticator views to your application.' - argument :scope, :required => false, :default => nil, - :desc => "The scope to copy views to" + argument :scope, required: false, default: nil, + desc: 'The scope to which views should be copied' include ::Devise::Generators::ViewPathTemplates - source_root File.expand_path("../../../../app/views/devise", __FILE__) + + source_root File.expand_path('../../../../app/views/devise', __FILE__) + def copy_views view_directory :checkga view_directory :displayqr From ee5c7b4a22b6e2b944be83b2e4ebd82fd377c9c2 Mon Sep 17 00:00:00 2001 From: David Alexander Date: Tue, 16 Feb 2016 16:57:43 -0500 Subject: [PATCH 3/5] Removes dependency on Google APIs for QR code image --- devise_google_authenticator.gemspec | 6 +++--- .../controllers/helpers.rb | 11 +++++++++-- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/devise_google_authenticator.gemspec b/devise_google_authenticator.gemspec index cf357dd..ca741ca 100644 --- a/devise_google_authenticator.gemspec +++ b/devise_google_authenticator.gemspec @@ -25,10 +25,10 @@ Gem::Specification.new do |s| # removed the following to try and get past this bundle update not finding compatible versions for gem issue # 'actionmailer' => '>= 3.0', #'actionmailer' => '~> 3.2',# '>= 3.2.12', - 'devise' => '~> 3.2', - 'rotp' => '~> 1.6' + 'devise' => '~> 3.2', + 'rotp' => '~> 1.6', + 'rqrcode' => '~> 0.10.1' }.each do |lib, version| s.add_runtime_dependency(lib, *version) end - end diff --git a/lib/devise_google_authenticatable/controllers/helpers.rb b/lib/devise_google_authenticatable/controllers/helpers.rb index 817e2f2..c51e180 100644 --- a/lib/devise_google_authenticatable/controllers/helpers.rb +++ b/lib/devise_google_authenticatable/controllers/helpers.rb @@ -1,3 +1,6 @@ +require 'rqrcode' +require 'base64' + module DeviseGoogleAuthenticator #:nodoc: module Controllers # :nodoc: module Helpers # :nodoc: @@ -6,8 +9,12 @@ def google_authenticator_qrcode(user, qualifier = nil, issuer = nil) app = user.class.ga_appname || Rails.application.class.parent_name data = "otpauth://totp/#{otpauth_user(username, app, qualifier)}?secret=#{user.gauth_secret}" data << "&issuer=#{issuer}" if !issuer.nil? - data = Rack::Utils.escape(data) - url = "https://chart.googleapis.com/chart?chs=200x200&chld=M|0&cht=qr&chl=#{data}" + # data-uri is easier, so... + qrcode = RQRCode::QRCode.new(data, level: :m, mode: :byte_8bit) + png = qrcode.as_png(fill: 'white', color: 'black', border_modules: 1, module_px_size: 4) + url = "data:image/png;base64,#{Base64.encode64(png.to_s).strip}" + #url = "data:image/svg+xml;utf8,#{qrcode.as_svg}" + #url = "https://chart.googleapis.com/chart?chs=200x200&chld=M|0&cht=qr&chl=#{Rack::Utils.escape(data)}" return image_tag(url, alt: 'Google Authenticator QRCode') end From fe0d3b9ec13658d17a6660aafc3d1c9ad66cf435 Mon Sep 17 00:00:00 2001 From: David Alexander Date: Tue, 16 Feb 2016 16:58:33 -0500 Subject: [PATCH 4/5] Allows for other username fields (per Devise configuration) in QR code --- lib/devise_google_authenticatable/controllers/helpers.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/devise_google_authenticatable/controllers/helpers.rb b/lib/devise_google_authenticatable/controllers/helpers.rb index c51e180..d54c3b6 100644 --- a/lib/devise_google_authenticatable/controllers/helpers.rb +++ b/lib/devise_google_authenticatable/controllers/helpers.rb @@ -5,7 +5,9 @@ module DeviseGoogleAuthenticator #:nodoc: module Controllers # :nodoc: module Helpers # :nodoc: def google_authenticator_qrcode(user, qualifier = nil, issuer = nil) - username = username_from_email(user.email) + username = nil + Devise.authentication_keys.any? {|k| username = user.public_send(k) rescue nil } + username ||= username_from_email(user.email) app = user.class.ga_appname || Rails.application.class.parent_name data = "otpauth://totp/#{otpauth_user(username, app, qualifier)}?secret=#{user.gauth_secret}" data << "&issuer=#{issuer}" if !issuer.nil? From a34cbff4321b89ecfe26a9e670cfd16a7afb1ae2 Mon Sep 17 00:00:00 2001 From: David Alexander Date: Tue, 16 Feb 2016 17:05:20 -0500 Subject: [PATCH 5/5] Fixes tests --- .../controllers/helpers_test.rb | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/test/lib/devise_google_authenticatable/controllers/helpers_test.rb b/test/lib/devise_google_authenticatable/controllers/helpers_test.rb index 8e0a571..f610560 100644 --- a/test/lib/devise_google_authenticatable/controllers/helpers_test.rb +++ b/test/lib/devise_google_authenticatable/controllers/helpers_test.rb @@ -9,7 +9,7 @@ def setup end test "can get username from user's email" do - assert_equal 'helpers_test', username_from_email(@user.email) + assert_equal 'helpers_test', username_from_email(@user.email) end test 'can get otpauth_user' do @@ -25,9 +25,6 @@ def image_tag(src, *args) src end test 'generate qrcode' do - assert_equal "https://chart.googleapis.com/chart?chs=200x200&chld=M|0&cht=qr&chl=otpauth%3A%2F%2Ftotp%2Fhelpers_test%40RailsApp%3Fsecret%3D", google_authenticator_qrcode(@user) - assert_equal "https://chart.googleapis.com/chart?chs=200x200&chld=M|0&cht=qr&chl=otpauth%3A%2F%2Ftotp%2Fhelpers_test%40RailsAppMyQualifier%3Fsecret%3D", google_authenticator_qrcode(@user, 'MyQualifier') - assert_equal "https://chart.googleapis.com/chart?chs=200x200&chld=M|0&cht=qr&chl=otpauth%3A%2F%2Ftotp%2Fhelpers_test%40RailsApp%3Fsecret%3D%26issuer%3DMyIssuer", google_authenticator_qrcode(@user, nil, 'MyIssuer') - assert_equal "https://chart.googleapis.com/chart?chs=200x200&chld=M|0&cht=qr&chl=otpauth%3A%2F%2Ftotp%2Fhelpers_test%40RailsAppMyQualifier%3Fsecret%3D%26issuer%3DMyIssuer", google_authenticator_qrcode(@user, 'MyQualifier', 'MyIssuer') + assert_equal 'data:image/png;base64,', google_authenticator_qrcode(@user).first(22) end end