From 00fa5f4507da806f66664787745086ce404d95b4 Mon Sep 17 00:00:00 2001 From: hatenine Date: Mon, 23 Apr 2018 18:32:27 +0200 Subject: [PATCH] 1143 extract @resource initialization out to a named overrideable method (#1144) #1143 extract @resource initialization out to a named overrideable method in registrations controller --- .../application_controller.rb | 4 ++ .../devise_token_auth/passwords_controller.rb | 20 ++----- .../registrations_controller.rb | 45 ++++++++------- lib/devise_token_auth.rb | 1 + lib/devise_token_auth/errors.rb | 5 ++ .../custom_registrations_controller_test.rb | 9 +++ .../omniauth_callbacks_controller_test.rb | 2 - .../registrations_controller_test.rb | 56 ++++++++++--------- .../custom/registrations_controller.rb | 1 - test/test_helper.rb | 13 +---- 10 files changed, 79 insertions(+), 77 deletions(-) create mode 100644 lib/devise_token_auth/errors.rb diff --git a/app/controllers/devise_token_auth/application_controller.rb b/app/controllers/devise_token_auth/application_controller.rb index 8d79e033e..510ff2ec0 100644 --- a/app/controllers/devise_token_auth/application_controller.rb +++ b/app/controllers/devise_token_auth/application_controller.rb @@ -17,6 +17,10 @@ def resource_errors protected + def blacklisted_redirect_url? + DeviseTokenAuth.redirect_whitelist && !DeviseTokenAuth::Url.whitelisted?(@redirect_url) + end + def build_redirect_headers(access_token, client, redirect_header_options = {}) { DeviseTokenAuth.headers_names[:"access-token"] => access_token, diff --git a/app/controllers/devise_token_auth/passwords_controller.rb b/app/controllers/devise_token_auth/passwords_controller.rb index d0afd45aa..8de8b2759 100644 --- a/app/controllers/devise_token_auth/passwords_controller.rb +++ b/app/controllers/devise_token_auth/passwords_controller.rb @@ -11,21 +11,13 @@ def create end # give redirect value from params priority - @redirect_url = params[:redirect_url] + @redirect_url = params.fetch( + :redirect_url, + DeviseTokenAuth.default_password_reset_url + ) - # fall back to default value if provided - @redirect_url ||= DeviseTokenAuth.default_password_reset_url - - unless @redirect_url - return render_create_error_missing_redirect_url - end - - # if whitelist is set, validate redirect_url against whitelist - if DeviseTokenAuth.redirect_whitelist - unless DeviseTokenAuth::Url.whitelisted?(@redirect_url) - return render_create_error_not_allowed_redirect_url - end - end + return render_create_error_missing_redirect_url unless @redirect_url + return render_create_error_not_allowed_redirect_url if blacklisted_redirect_url? @email = get_case_insensitive_field_from_resource_params(:email) @resource = find_resource(:uid, @email) diff --git a/app/controllers/devise_token_auth/registrations_controller.rb b/app/controllers/devise_token_auth/registrations_controller.rb index 1661e8a4a..7b6ea0996 100644 --- a/app/controllers/devise_token_auth/registrations_controller.rb +++ b/app/controllers/devise_token_auth/registrations_controller.rb @@ -6,21 +6,18 @@ class RegistrationsController < DeviseTokenAuth::ApplicationController skip_after_action :update_auth_header, only: [:create, :destroy] def create - @resource = resource_class.new(sign_up_params.except(:confirm_success_url)) - @resource.provider = provider + build_resource - # honor devise configuration for case_insensitive_keys - if resource_class.case_insensitive_keys.include?(:email) - @resource.email = sign_up_params[:email].try :downcase - else - @resource.email = sign_up_params[:email] + unless @resource.present? + raise DeviseTokenAuth::Errors::NoResourceDefinedError, + "#{self.class.name} #build_resource does not define @resource, execution stopped" end # give redirect value from params priority - @redirect_url = sign_up_params[:confirm_success_url] - - # fall back to default value if provided - @redirect_url ||= DeviseTokenAuth.default_confirm_success_url + @redirect_url = params.fetch( + :confirm_success_url, + DeviseTokenAuth.default_confirm_success_url + ) # success redirect url is required if confirmable_enabled? && !@redirect_url @@ -28,20 +25,18 @@ def create end # if whitelist is set, validate redirect_url against whitelist - if DeviseTokenAuth.redirect_whitelist - unless DeviseTokenAuth::Url.whitelisted?(@redirect_url) - return render_create_error_redirect_url_not_allowed - end - end + return render_create_error_redirect_url_not_allowed if blacklisted_redirect_url? begin # override email confirmation, must be sent manually from ctrl resource_class.set_callback("create", :after, :send_on_create_confirmation_instructions) resource_class.skip_callback("create", :after, :send_on_create_confirmation_instructions) + if @resource.respond_to? :skip_confirmation_notification! # Fix duplicate e-mails by disabling Devise confirmation e-mail @resource.skip_confirmation_notification! end + if @resource.save yield @resource if block_given? @@ -51,13 +46,10 @@ def create client_config: params[:config_name], redirect_url: @redirect_url }) - else # email auth has been bypassed, authenticate user @client_id, @token = @resource.create_token - @resource.save! - update_auth_header end render_create_success @@ -88,7 +80,6 @@ def destroy if @resource @resource.destroy yield @resource if block_given? - render_destroy_success else render_destroy_error @@ -96,7 +87,7 @@ def destroy end def sign_up_params - params.permit([*params_for_resource(:sign_up), :confirm_success_url]) + params.permit(*params_for_resource(:sign_up)) end def account_update_params @@ -105,6 +96,18 @@ def account_update_params protected + def build_resource + @resource = resource_class.new(sign_up_params) + @resource.provider = provider + + # honor devise configuration for case_insensitive_keys + if resource_class.case_insensitive_keys.include?(:email) + @resource.email = sign_up_params[:email].try(:downcase) + else + @resource.email = sign_up_params[:email] + end + end + def render_create_error_missing_confirm_success_url response = { status: 'error', diff --git a/lib/devise_token_auth.rb b/lib/devise_token_auth.rb index 7ba990e61..e80ecf04e 100644 --- a/lib/devise_token_auth.rb +++ b/lib/devise_token_auth.rb @@ -3,6 +3,7 @@ require "devise_token_auth/controllers/helpers" require "devise_token_auth/controllers/url_helpers" require "devise_token_auth/url" +require "devise_token_auth/errors" module DeviseTokenAuth end diff --git a/lib/devise_token_auth/errors.rb b/lib/devise_token_auth/errors.rb new file mode 100644 index 000000000..33762593b --- /dev/null +++ b/lib/devise_token_auth/errors.rb @@ -0,0 +1,5 @@ +module DeviseTokenAuth + module Errors + class NoResourceDefinedError < StandardError ; end + end +end diff --git a/test/controllers/custom/custom_registrations_controller_test.rb b/test/controllers/custom/custom_registrations_controller_test.rb index cd25a49b3..16c68b989 100644 --- a/test/controllers/custom/custom_registrations_controller_test.rb +++ b/test/controllers/custom/custom_registrations_controller_test.rb @@ -50,5 +50,14 @@ class Custom::RegistrationsControllerTest < ActionDispatch::IntegrationTest assert @controller.destroy_block_called?, 'destroy failed to yield resource to provided block' end + + describe 'when overriding #build_resource' do + test 'it fails' do + Custom::RegistrationsController.any_instance.stubs(:build_resource).returns(nil) + assert_raises DeviseTokenAuth::Errors::NoResourceDefinedError do + post '/nice_user_auth', params: @create_params + end + end + end end end diff --git a/test/controllers/devise_token_auth/omniauth_callbacks_controller_test.rb b/test/controllers/devise_token_auth/omniauth_callbacks_controller_test.rb index dbd106299..40ba9881c 100644 --- a/test/controllers/devise_token_auth/omniauth_callbacks_controller_test.rb +++ b/test/controllers/devise_token_auth/omniauth_callbacks_controller_test.rb @@ -1,6 +1,4 @@ require 'test_helper' -require 'mocha/minitest' - # was the web request successful? # was the user redirected to the right page? # was the user successfully authenticated? diff --git a/test/controllers/overrides/registrations_controller_test.rb b/test/controllers/overrides/registrations_controller_test.rb index 66c41fbf4..d8e55a8c9 100644 --- a/test/controllers/overrides/registrations_controller_test.rb +++ b/test/controllers/overrides/registrations_controller_test.rb @@ -8,33 +8,35 @@ class Overrides::RegistrationsControllerTest < ActionDispatch::IntegrationTest describe Overrides::RegistrationsController do - setup do - @existing_user = evil_users(:confirmed_email_user) - @auth_headers = @existing_user.create_new_auth_token - @client_id = @auth_headers['client'] - @favorite_color = 'pink' - - # ensure request is not treated as batch request - age_token(@existing_user, @client_id) - - # test valid update param - @new_operating_thetan = 1_000_000 - - put '/evil_user_auth', - params: { favorite_color: @favorite_color }, - headers: @auth_headers - - @data = JSON.parse(response.body) - @existing_user.reload - end - - test 'user was updated' do - assert_equal @favorite_color, @existing_user.favorite_color - end - - test 'controller was overridden' do - assert_equal Overrides::RegistrationsController::OVERRIDE_PROOF, - @data['override_proof'] + describe 'Succesful Registration update' do + setup do + @existing_user = evil_users(:confirmed_email_user) + @auth_headers = @existing_user.create_new_auth_token + @client_id = @auth_headers['client'] + @favorite_color = 'pink' + + # ensure request is not treated as batch request + age_token(@existing_user, @client_id) + + # test valid update param + @new_operating_thetan = 1_000_000 + + put '/evil_user_auth', + params: { favorite_color: @favorite_color }, + headers: @auth_headers + + @data = JSON.parse(response.body) + @existing_user.reload + end + + test 'user was updated' do + assert_equal @favorite_color, @existing_user.favorite_color + end + + test 'controller was overridden' do + assert_equal Overrides::RegistrationsController::OVERRIDE_PROOF, + @data['override_proof'] + end end end end diff --git a/test/dummy/app/controllers/custom/registrations_controller.rb b/test/dummy/app/controllers/custom/registrations_controller.rb index 86d98b1d6..36d90a29e 100644 --- a/test/dummy/app/controllers/custom/registrations_controller.rb +++ b/test/dummy/app/controllers/custom/registrations_controller.rb @@ -35,5 +35,4 @@ def destroy_block_called? def render_create_success render json: {custom: "foo"} end - end diff --git a/test/test_helper.rb b/test/test_helper.rb index 5817a2091..479224a07 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -1,10 +1,5 @@ require 'simplecov' -# SimpleCov.formatter = SimpleCov::Formatter::MultiFormatter[ -# SimpleCov::Formatter::HTMLFormatter, -# CodeClimate::TestReporter::Formatter -# ] - SimpleCov.start 'rails' ENV['RAILS_ENV'] = 'test' @@ -12,13 +7,7 @@ require File.expand_path('../dummy/config/environment', __FILE__) require 'rails/test_help' require 'minitest/rails' - -# To add Capybara feature tests add `gem "minitest-rails-capybara"` -# to the test group in the Gemfile and uncomment the following: -# require "minitest/rails/capybara" - -# Uncomment for awesome colorful output -# require "minitest/pride" +require 'mocha/minitest' ActiveSupport::TestCase.fixture_path = File.expand_path('../fixtures', __FILE__) ActionDispatch::IntegrationTest.fixture_path = File.expand_path('../fixtures', __FILE__)