From 6b08b8e5787c78714475bd178f4ad48aea75460c Mon Sep 17 00:00:00 2001 From: Bryce Senz Date: Mon, 9 Oct 2017 13:45:43 -0400 Subject: [PATCH 1/4] Adding in unlocks controller and specs. This should resolve #927. --- .../devise_token_auth/unlocks_controller.rb | 119 +++++++++++ app/models/devise_token_auth/concerns/user.rb | 15 ++ .../mailer/unlock_instructions.html.erb | 2 +- config/locales/en.yml | 4 + lib/devise_token_auth/rails/routes.rb | 2 +- .../passwords_controller_test.rb | 1 + .../unlocks_controller_test.rb | 197 ++++++++++++++++++ test/models/user_test.rb | 1 - 8 files changed, 338 insertions(+), 3 deletions(-) create mode 100644 app/controllers/devise_token_auth/unlocks_controller.rb create mode 100644 test/controllers/devise_token_auth/unlocks_controller_test.rb diff --git a/app/controllers/devise_token_auth/unlocks_controller.rb b/app/controllers/devise_token_auth/unlocks_controller.rb new file mode 100644 index 000000000..4a7fba153 --- /dev/null +++ b/app/controllers/devise_token_auth/unlocks_controller.rb @@ -0,0 +1,119 @@ +module DeviseTokenAuth + class UnlocksController < DeviseTokenAuth::ApplicationController + skip_after_action :update_auth_header, :only => [:create, :show] + + # this action is responsible for generating unlock tokens and + # sending emails + def create + unless resource_params[:email] + return render_create_error_missing_email + end + + # honor devise configuration for case_insensitive_keys + if resource_class.case_insensitive_keys.include?(:email) + @email = resource_params[:email].downcase + else + @email = resource_params[:email] + end + + q = "uid = ? AND provider='email'" + + # fix for mysql default case insensitivity + if ActiveRecord::Base.connection.adapter_name.downcase.starts_with? 'mysql' + q = "BINARY uid = ? AND provider='email'" + end + + @resource = resource_class.where(q, @email).first + + @errors = nil + @error_status = 400 + + if @resource + yield @resource if block_given? + + @resource.send_unlock_instructions({ + email: @email, + provider: 'email', + client_config: params[:config_name] + }) + + if @resource.errors.empty? + return render_create_success + else + @errors = @resource.errors + end + else + @errors = [I18n.t("devise_token_auth.unlocks.user_not_found", email: @email)] + @error_status = 404 + end + + if @errors + return render_create_error + end + end + + def show + @resource = resource_class.unlock_access_by_token(params[:unlock_token]) + + if @resource && @resource.id + client_id = SecureRandom.urlsafe_base64(nil, false) + token = SecureRandom.urlsafe_base64(nil, false) + token_hash = BCrypt::Password.create(token) + expiry = (Time.now + DeviseTokenAuth.token_lifespan).to_i + + @resource.tokens[client_id] = { + token: token_hash, + expiry: expiry + } + + @resource.save! + yield @resource if block_given? + + redirect_to(@resource.build_auth_url(after_unlock_path_for(@resource), { + token: token, + client_id: client_id, + unlock: true, + config: params[:config] + })) + else + render_show_error + end + end + + private + def after_unlock_path_for(resource) + #TODO: This should probably be a configuration option at the very least. + # root_url + '/' + end + + def render_create_error_missing_email + render json: { + success: false, + errors: [I18n.t("devise_token_auth.unlocks.missing_email")] + }, status: 401 + end + + def render_create_success + render json: { + success: true, + message: I18n.t("devise_token_auth.unlocks.sended", email: @email) + } + end + + def render_create_error + render json: { + success: false, + errors: @errors, + }, status: @error_status + end + + def render_show_error + raise ActionController::RoutingError.new('Not Found') + end + + def resource_params + params.permit(:email, :unlock_token, :config) + end + end +end \ No newline at end of file diff --git a/app/models/devise_token_auth/concerns/user.rb b/app/models/devise_token_auth/concerns/user.rb index 4db7f7df8..a96fa98da 100644 --- a/app/models/devise_token_auth/concerns/user.rb +++ b/app/models/devise_token_auth/concerns/user.rb @@ -88,6 +88,21 @@ def send_reset_password_instructions(opts=nil) token end + + # override devise method to include additional info as opts hash + def send_unlock_instructions(opts=nil) + raw, enc = Devise.token_generator.generate(self.class, :unlock_token) + self.unlock_token = enc + save(validate: false) + + opts ||= {} + + # fall back to "default" config name + opts[:client_config] ||= "default" + + send_devise_notification(:unlock_instructions, raw, opts) + raw + end end module ClassMethods diff --git a/app/views/devise/mailer/unlock_instructions.html.erb b/app/views/devise/mailer/unlock_instructions.html.erb index 3beae0f0c..8493410b0 100644 --- a/app/views/devise/mailer/unlock_instructions.html.erb +++ b/app/views/devise/mailer/unlock_instructions.html.erb @@ -4,4 +4,4 @@

<%= t '.unlock_link_msg' %>

-

<%= link_to t('.unlock_link'), unlock_url(@resource, unlock_token: @token) %>

+

<%= link_to t('.unlock_link'), unlock_url(@resource, unlock_token: @token, config: message['client-config'].to_s) %>

diff --git a/config/locales/en.yml b/config/locales/en.yml index 80d6f8ae9..827f7becd 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -23,6 +23,10 @@ en: password_not_required: "This account does not require a password. Sign in using your '%{provider}' account instead." missing_passwords: "You must fill out the fields labeled 'Password' and 'Password confirmation'." successfully_updated: "Your password has been successfully updated." + unlocks: + missing_email: "You must provide an email address." + sended: "An email has been sent to '%{email}' containing instructions for unlocking your account." + user_not_found: "Unable to find user with email '%{email}'." errors: messages: validate_sign_up_params: "Please submit proper sign up data in request body." diff --git a/lib/devise_token_auth/rails/routes.rb b/lib/devise_token_auth/rails/routes.rb index eed9d9e0a..e3e248a2a 100644 --- a/lib/devise_token_auth/rails/routes.rb +++ b/lib/devise_token_auth/rails/routes.rb @@ -12,7 +12,7 @@ def mount_devise_token_auth_for(resource, opts) confirmations_ctrl = opts[:controllers][:confirmations] || "devise_token_auth/confirmations" token_validations_ctrl = opts[:controllers][:token_validations] || "devise_token_auth/token_validations" omniauth_ctrl = opts[:controllers][:omniauth_callbacks] || "devise_token_auth/omniauth_callbacks" - unlocks_ctrl = opts[:controllers][:unlocks] + unlocks_ctrl = opts[:controllers][:unlocks] || "devise_token_auth/unlocks" # define devise controller mappings controllers = {:sessions => sessions_ctrl, diff --git a/test/controllers/devise_token_auth/passwords_controller_test.rb b/test/controllers/devise_token_auth/passwords_controller_test.rb index f3dc394d8..bc42e59ee 100644 --- a/test/controllers/devise_token_auth/passwords_controller_test.rb +++ b/test/controllers/devise_token_auth/passwords_controller_test.rb @@ -503,6 +503,7 @@ class DeviseTokenAuth::PasswordsControllerTest < ActionController::TestCase @resource.reload end end + describe 'unconfirmable user' do setup do @request.env['devise.mapping'] = Devise.mappings[:unconfirmable_user] diff --git a/test/controllers/devise_token_auth/unlocks_controller_test.rb b/test/controllers/devise_token_auth/unlocks_controller_test.rb new file mode 100644 index 000000000..2538e555d --- /dev/null +++ b/test/controllers/devise_token_auth/unlocks_controller_test.rb @@ -0,0 +1,197 @@ +require 'test_helper' + +# was the web request successful? +# was the user redirected to the right page? +# was the user successfully authenticated? +# was the correct object stored in the response? +# was the appropriate message delivered in the json payload? + +class DeviseTokenAuth::UnlocksControllerTest < ActionController::TestCase + describe DeviseTokenAuth::UnlocksController do + setup do + @request.env['devise.mapping'] = Devise.mappings[:lockable_user] + end + + teardown do + @request.env['devise.mapping'] = Devise.mappings[:user] + end + + before do + @original_lock_strategy = Devise.lock_strategy + @original_unlock_strategy = Devise.unlock_strategy + @original_maximum_attempts = Devise.maximum_attempts + Devise.lock_strategy = :failed_attempts + Devise.unlock_strategy = :email + Devise.maximum_attempts = 5 + end + + after do + Devise.lock_strategy = @original_lock_strategy + Devise.maximum_attempts = @original_maximum_attempts + Devise.unlock_strategy = @original_unlock_strategy + end + + describe "Unlocking user" do + before do + @resource = lockable_users(:unlocked_user) + end + + # describe 'not email should return 401' do + # before do + # @auth_headers = @resource.create_new_auth_token + # @new_password = Faker::Internet.password + + # xhr :post, :create, {} + # @data = JSON.parse(response.body) + # end + + # test 'response should fail' do + # assert_equal 401, response.status + # end + # test 'error message should be returned' do + # assert @data["errors"] + # assert_equal @data["errors"], [I18n.t("devise_token_auth.passwords.missing_email")] + # end + # end + + describe 'request unlock' do + # describe 'unknown user should return 404' do + # before do + # xhr :post, :create, { + # email: 'chester@cheet.ah' + # } + # @data = JSON.parse(response.body) + # end + # test 'unknown user should return 404' do + # assert_equal 404, response.status + # end + + # test 'errors should be returned' do + # assert @data["errors"] + # assert_equal @data["errors"], [I18n.t("devise_token_auth.passwords.user_not_found", email: 'chester@cheet.ah')] + # end + # end + + # describe 'successfully requested unlock' do + # before do + # xhr :post, :create, { + # email: @resource.email + # } + + # @data = JSON.parse(response.body) + # end + + # test 'response should not contain extra data' do + # assert_nil @data["data"] + # end + # end + + describe 'case-sensitive email' do + before do + xhr :post, :create, { + email: @resource.email + } + + @mail = ActionMailer::Base.deliveries.last + @resource.reload + @data = JSON.parse(response.body) + + @mail_config_name = CGI.unescape(@mail.body.match(/config=([^&]*)&/)[1]) + @mail_reset_token = @mail.body.match(/unlock_token=(.*)\"/)[1] + end + + test 'response should return success status' do + assert_equal 200, response.status + end + + test 'response should contains message' do + assert_equal @data["message"], I18n.t("devise_token_auth.unlocks.sended", email: @resource.email) + end + + test 'action should send an email' do + assert @mail + end + + test 'the email should be addressed to the user' do + assert_equal @mail.to.first, @resource.email + end + + test 'the client config name should fall back to "default"' do + assert_equal 'default', @mail_config_name + end + + test 'the email body should contain a link with reset token as a query param' do + user = LockableUser.unlock_access_by_token(@mail_reset_token) + assert_equal user.id, @resource.id + end + + describe 'unlock link failure' do + test 'response should return 404' do + assert_raises(ActionController::RoutingError) { + xhr :get, :show, { + unlock_token: "bogus" + } + } + end + end + + describe 'password reset link success' do + before do + xhr :get, :show, { + unlock_token: @mail_reset_token + } + + @resource.reload + + raw_qs = response.location.split('?')[1] + @qs = Rack::Utils.parse_nested_query(raw_qs) + + @client_id = @qs["client_id"] + @expiry = @qs["expiry"] + @unlock = @qs["unlock"] + @token = @qs["token"] + @uid = @qs["uid"] + end + + test 'respones should have success redirect status' do + assert_equal 302, response.status + end + + test 'response should contain auth params' do + assert @client_id + assert @expiry + assert @unlock + assert @token + assert @uid + end + + test 'response auth params should be valid' do + assert @resource.valid_token?(@token, @client_id) + end + end + end + + describe 'case-insensitive email' do + before do + @resource_class = LockableUser + @request_params = { + email: @resource.email.upcase + } + end + + test 'response should return success status if configured' do + @resource_class.case_insensitive_keys = [:email] + xhr :post, :create, @request_params + assert_equal 200, response.status + end + + test 'response should return failure status if not configured' do + @resource_class.case_insensitive_keys = [] + xhr :post, :create, @request_params + assert_equal 404, response.status + end + end + end + end + end +end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 2ca34c81d..a8bc6ae56 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -87,7 +87,6 @@ class UserTest < ActiveSupport::TestCase @resource.tokens[@client_id]['expiry'] = Time.now.to_i - 10.seconds refute @resource.token_is_current?(@token, @client_id) end - end describe 'user specific token lifespan' do From 32eaf8f046f99a740d4fb8b90c19558daed47479 Mon Sep 17 00:00:00 2001 From: Bryce Senz Date: Mon, 9 Oct 2017 15:07:29 -0400 Subject: [PATCH 2/4] Uncommenting specs --- .../devise_token_auth/unlocks_controller.rb | 1 - .../unlocks_controller_test.rb | 94 +++++++++---------- 2 files changed, 47 insertions(+), 48 deletions(-) diff --git a/app/controllers/devise_token_auth/unlocks_controller.rb b/app/controllers/devise_token_auth/unlocks_controller.rb index 4a7fba153..f0abed2be 100644 --- a/app/controllers/devise_token_auth/unlocks_controller.rb +++ b/app/controllers/devise_token_auth/unlocks_controller.rb @@ -83,7 +83,6 @@ def show private def after_unlock_path_for(resource) #TODO: This should probably be a configuration option at the very least. - # root_url '/' end diff --git a/test/controllers/devise_token_auth/unlocks_controller_test.rb b/test/controllers/devise_token_auth/unlocks_controller_test.rb index 2538e555d..12284ea68 100644 --- a/test/controllers/devise_token_auth/unlocks_controller_test.rb +++ b/test/controllers/devise_token_auth/unlocks_controller_test.rb @@ -36,55 +36,55 @@ class DeviseTokenAuth::UnlocksControllerTest < ActionController::TestCase @resource = lockable_users(:unlocked_user) end - # describe 'not email should return 401' do - # before do - # @auth_headers = @resource.create_new_auth_token - # @new_password = Faker::Internet.password - - # xhr :post, :create, {} - # @data = JSON.parse(response.body) - # end - - # test 'response should fail' do - # assert_equal 401, response.status - # end - # test 'error message should be returned' do - # assert @data["errors"] - # assert_equal @data["errors"], [I18n.t("devise_token_auth.passwords.missing_email")] - # end - # end + describe 'not email should return 401' do + before do + @auth_headers = @resource.create_new_auth_token + @new_password = Faker::Internet.password + + xhr :post, :create, {} + @data = JSON.parse(response.body) + end + + test 'response should fail' do + assert_equal 401, response.status + end + test 'error message should be returned' do + assert @data["errors"] + assert_equal @data["errors"], [I18n.t("devise_token_auth.passwords.missing_email")] + end + end describe 'request unlock' do - # describe 'unknown user should return 404' do - # before do - # xhr :post, :create, { - # email: 'chester@cheet.ah' - # } - # @data = JSON.parse(response.body) - # end - # test 'unknown user should return 404' do - # assert_equal 404, response.status - # end - - # test 'errors should be returned' do - # assert @data["errors"] - # assert_equal @data["errors"], [I18n.t("devise_token_auth.passwords.user_not_found", email: 'chester@cheet.ah')] - # end - # end - - # describe 'successfully requested unlock' do - # before do - # xhr :post, :create, { - # email: @resource.email - # } - - # @data = JSON.parse(response.body) - # end - - # test 'response should not contain extra data' do - # assert_nil @data["data"] - # end - # end + describe 'unknown user should return 404' do + before do + xhr :post, :create, { + email: 'chester@cheet.ah' + } + @data = JSON.parse(response.body) + end + test 'unknown user should return 404' do + assert_equal 404, response.status + end + + test 'errors should be returned' do + assert @data["errors"] + assert_equal @data["errors"], [I18n.t("devise_token_auth.passwords.user_not_found", email: 'chester@cheet.ah')] + end + end + + describe 'successfully requested unlock' do + before do + xhr :post, :create, { + email: @resource.email + } + + @data = JSON.parse(response.body) + end + + test 'response should not contain extra data' do + assert_nil @data["data"] + end + end describe 'case-sensitive email' do before do From abe7c82b738d5e9e9ab55f631895aa67d751ad27 Mon Sep 17 00:00:00 2001 From: Bryce Senz Date: Tue, 10 Oct 2017 10:14:11 -0400 Subject: [PATCH 3/4] Refactoring the UnlocksController, PasswordsController, and SessionsController --- .../application_controller.rb | 1 + .../concerns/resource_finder.rb | 35 +++++++++++++++++++ .../concerns/set_user_by_token.rb | 13 +------ .../devise_token_auth/passwords_controller.rb | 17 ++------- .../devise_token_auth/sessions_controller.rb | 14 ++------ .../devise_token_auth/unlocks_controller.rb | 17 ++------- 6 files changed, 43 insertions(+), 54 deletions(-) create mode 100644 app/controllers/devise_token_auth/concerns/resource_finder.rb diff --git a/app/controllers/devise_token_auth/application_controller.rb b/app/controllers/devise_token_auth/application_controller.rb index bd1dea58b..83c76eabe 100644 --- a/app/controllers/devise_token_auth/application_controller.rb +++ b/app/controllers/devise_token_auth/application_controller.rb @@ -1,6 +1,7 @@ module DeviseTokenAuth class ApplicationController < DeviseController include DeviseTokenAuth::Concerns::SetUserByToken + include DeviseTokenAuth::Concerns::ResourceFinder def resource_data(opts={}) response_data = opts[:resource_json] || @resource.as_json diff --git a/app/controllers/devise_token_auth/concerns/resource_finder.rb b/app/controllers/devise_token_auth/concerns/resource_finder.rb new file mode 100644 index 000000000..f7ceae7a2 --- /dev/null +++ b/app/controllers/devise_token_auth/concerns/resource_finder.rb @@ -0,0 +1,35 @@ +module DeviseTokenAuth::Concerns::ResourceFinder + extend ActiveSupport::Concern + include DeviseTokenAuth::Controllers::Helpers + + def get_case_insensitive_field_from_resource_params(field) + # honor Devise configuration for case_insensitive keys + q_value = resource_params[field.to_sym] + + if resource_class.case_insensitive_keys.include?(field.to_sym) + q_value.downcase! + end + q_value + end + + def find_resource(field, value, provider='email') + # fix for mysql default case insensitivity + q = "#{field.to_s} = ? AND provider='#{provider.to_s}'" + + if ActiveRecord::Base.connection.adapter_name.downcase.starts_with? 'mysql' + q = "BINARY " + q + end + + @resource = resource_class.where(q, value).first + end + + def resource_class(m=nil) + if m + mapping = Devise.mappings[m] + else + mapping = Devise.mappings[resource_name] || Devise.mappings.values.first + end + + mapping.to + end +end diff --git a/app/controllers/devise_token_auth/concerns/set_user_by_token.rb b/app/controllers/devise_token_auth/concerns/set_user_by_token.rb index cf13eefdf..0060787a5 100644 --- a/app/controllers/devise_token_auth/concerns/set_user_by_token.rb +++ b/app/controllers/devise_token_auth/concerns/set_user_by_token.rb @@ -1,5 +1,6 @@ module DeviseTokenAuth::Concerns::SetUserByToken extend ActiveSupport::Concern + include DeviseTokenAuth::Concerns::ResourceFinder include DeviseTokenAuth::Controllers::Helpers included do @@ -75,7 +76,6 @@ def set_user_by_token(mapping=nil) end end - def update_auth_header # cannot save object if model has invalid params return unless @resource && @resource.valid? && @client_id @@ -127,17 +127,6 @@ def update_auth_header end - def resource_class(m=nil) - if m - mapping = Devise.mappings[m] - else - mapping = Devise.mappings[resource_name] || Devise.mappings.values.first - end - - mapping.to - end - - private diff --git a/app/controllers/devise_token_auth/passwords_controller.rb b/app/controllers/devise_token_auth/passwords_controller.rb index 904479e1a..70bda4c3b 100644 --- a/app/controllers/devise_token_auth/passwords_controller.rb +++ b/app/controllers/devise_token_auth/passwords_controller.rb @@ -27,21 +27,8 @@ def create end end - # honor devise configuration for case_insensitive_keys - if resource_class.case_insensitive_keys.include?(:email) - @email = resource_params[:email].downcase - else - @email = resource_params[:email] - end - - q = "uid = ? AND provider='email'" - - # fix for mysql default case insensitivity - if ActiveRecord::Base.connection.adapter_name.downcase.starts_with? 'mysql' - q = "BINARY uid = ? AND provider='email'" - end - - @resource = resource_class.where(q, @email).first + @email = get_case_insensitive_field_from_resource_params(:email) + @resource = find_resource(:email, @email) @errors = nil @error_status = 400 diff --git a/app/controllers/devise_token_auth/sessions_controller.rb b/app/controllers/devise_token_auth/sessions_controller.rb index bc83cf9d8..9ca34422e 100644 --- a/app/controllers/devise_token_auth/sessions_controller.rb +++ b/app/controllers/devise_token_auth/sessions_controller.rb @@ -14,19 +14,9 @@ def create @resource = nil if field - q_value = resource_params[field] + q_value = get_case_insensitive_field_from_resource_params(field) - if resource_class.case_insensitive_keys.include?(field) - q_value.downcase! - end - - q = "#{field.to_s} = ? AND provider='#{provider}'" - - if ActiveRecord::Base.connection.adapter_name.downcase.starts_with? 'mysql' - q = "BINARY " + q - end - - @resource = resource_class.where(q, q_value).first + @resource = find_resource(field, q_value) end if @resource && valid_params?(field, q_value) && (!@resource.respond_to?(:active_for_authentication?) || @resource.active_for_authentication?) diff --git a/app/controllers/devise_token_auth/unlocks_controller.rb b/app/controllers/devise_token_auth/unlocks_controller.rb index f0abed2be..8b3d37b9f 100644 --- a/app/controllers/devise_token_auth/unlocks_controller.rb +++ b/app/controllers/devise_token_auth/unlocks_controller.rb @@ -9,21 +9,8 @@ def create return render_create_error_missing_email end - # honor devise configuration for case_insensitive_keys - if resource_class.case_insensitive_keys.include?(:email) - @email = resource_params[:email].downcase - else - @email = resource_params[:email] - end - - q = "uid = ? AND provider='email'" - - # fix for mysql default case insensitivity - if ActiveRecord::Base.connection.adapter_name.downcase.starts_with? 'mysql' - q = "BINARY uid = ? AND provider='email'" - end - - @resource = resource_class.where(q, @email).first + @email = get_case_insensitive_field_from_resource_params(:email) + @resource = find_resource(:email, @email) @errors = nil @error_status = 400 From 390bc0c0a4b533aa3caf1cd4529094a1fcb9d2a9 Mon Sep 17 00:00:00 2001 From: Bryce Senz Date: Tue, 10 Oct 2017 14:29:07 -0400 Subject: [PATCH 4/4] Incorporating the changes/bug fix proposed by @MaicolBen during the code review --- app/controllers/devise_token_auth/passwords_controller.rb | 2 +- test/controllers/devise_token_auth/unlocks_controller_test.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/devise_token_auth/passwords_controller.rb b/app/controllers/devise_token_auth/passwords_controller.rb index 70bda4c3b..0031c523a 100644 --- a/app/controllers/devise_token_auth/passwords_controller.rb +++ b/app/controllers/devise_token_auth/passwords_controller.rb @@ -28,7 +28,7 @@ def create end @email = get_case_insensitive_field_from_resource_params(:email) - @resource = find_resource(:email, @email) + @resource = find_resource(:uid, @email) @errors = nil @error_status = 400 diff --git a/test/controllers/devise_token_auth/unlocks_controller_test.rb b/test/controllers/devise_token_auth/unlocks_controller_test.rb index 12284ea68..ea6fe48a1 100644 --- a/test/controllers/devise_token_auth/unlocks_controller_test.rb +++ b/test/controllers/devise_token_auth/unlocks_controller_test.rb @@ -36,7 +36,7 @@ class DeviseTokenAuth::UnlocksControllerTest < ActionController::TestCase @resource = lockable_users(:unlocked_user) end - describe 'not email should return 401' do + describe 'request unlock without email' do before do @auth_headers = @resource.create_new_auth_token @new_password = Faker::Internet.password