From 7fcd76342c0d35a553da49d44b970af514538712 Mon Sep 17 00:00:00 2001 From: Guillaume Hain Date: Thu, 6 Feb 2025 10:00:25 +0100 Subject: [PATCH] Allows multiple messages This commit adapts Devise to support multiple flash messages from the Models, utilizing the Warden multi-messages feature. --- Earthfile | 84 +++++++++++++++++++ Gemfile | 2 + Gemfile.lock | 9 +- docker-compose-earthly.yml | 7 ++ docker-compose.yml | 11 +++ lib/devise/failure_app.rb | 26 +++--- lib/devise/hooks/activatable.rb | 2 +- lib/devise/hooks/timeoutable.rb | 2 +- lib/devise/models/authenticatable.rb | 11 ++- lib/devise/models/confirmable.rb | 9 +- lib/devise/models/lockable.rb | 21 +++-- lib/devise/strategies/authenticatable.rb | 15 ++-- .../strategies/database_authenticatable.rb | 2 +- test/failure_app_test.rb | 46 +++++----- test/integration/http_authenticatable_test.rb | 2 +- test/test/controller_helpers_test.rb | 2 +- 16 files changed, 188 insertions(+), 63 deletions(-) create mode 100644 Earthfile create mode 100644 docker-compose-earthly.yml create mode 100644 docker-compose.yml diff --git a/Earthfile b/Earthfile new file mode 100644 index 0000000000..e2f757f9c8 --- /dev/null +++ b/Earthfile @@ -0,0 +1,84 @@ +VERSION 0.6 + +# This allows one to change the running Ruby version with: +# +# `earthly --build-arg EARTHLY_RUBY_VERSION=2.7 --allow-privileged +test` +ARG EARTHLY_RUBY_VERSION=3.3 +ARG BUNDLER_VERSION=2.4.5 + +FROM ruby:$EARTHLY_RUBY_VERSION +WORKDIR /gem + +deps: + # No need to keep a single `RUN` here since this target uses `SAVE ARTIFACT` + # which means there's no Docker image created here. + RUN apt update \ + && apt install --yes \ + --no-install-recommends \ + build-essential \ + git \ + && mkdir /gems \ + && git clone https://github.com/Pharmony/warden.git /gems/warden \ + && cd /gems/warden \ + && git checkout features/support-multiple-messages \ + && gem install bundler:${BUNDLER_VERSION} + + COPY Gemfile /gem/Gemfile + COPY Gemfile.lock /gem/Gemfile.lock + COPY *.gemspec /gem + COPY lib/devise/version.rb /gem/lib/devise/version.rb + + RUN bundle install --jobs $(nproc) + + SAVE ARTIFACT /gems git-gems + SAVE ARTIFACT /usr/local/bundle bundler + SAVE ARTIFACT /gem/Gemfile Gemfile + SAVE ARTIFACT /gem/Gemfile.lock Gemfile.lock + +dev: + RUN apt update \ + && apt install --yes \ + --no-install-recommends \ + git \ + && gem install bundler:${BUNDLER_VERSION} + + # Import cached gems + COPY +deps/git-gems /gems + COPY +deps/bundler /usr/local/bundle + COPY +deps/Gemfile /gem/Gemfile + COPY +deps/Gemfile.lock /gem/Gemfile.lock + + # Import gem files + FOR gem_folder IN app config lib test *.gemspec Rakefile + COPY $gem_folder /gem/$gem_folder + END + + ENTRYPOINT ["bundle", "exec"] + CMD ["rake"] + + # Run `earthly +dev` in order to get the Docker image exported to your + # Docker images. + SAVE IMAGE heartcombo/devise:latest + +# +# This target runs the test suite. +# +# On you local machine you would likely use `docker compose run --rm gem` +# instead, avoiding to refresh the Docker image which takes some seconds. +# +# Use the following command in order to run the tests suite: +# earthly --allow-privileged +test [--TEST_COMMAND="rake test TEST=test/test_foobar.rb"] +test: + FROM earthly/dind:alpine + + COPY docker-compose-earthly.yml ./docker-compose.yml + + # Optionnal argument in the case you'd like to run something else than + # `rake test` + ARG TEST_COMMAND + + # Creates a temporary Docker image using the output from the +dev target, + # that will be used within the `WITH DOCKER ... END` block only. + WITH DOCKER --load heartcombo/devise:latest=+dev + RUN docker-compose run --rm gem $TEST_COMMAND + END diff --git a/Gemfile b/Gemfile index 722eb59a0d..b1641861a5 100644 --- a/Gemfile +++ b/Gemfile @@ -36,3 +36,5 @@ end # group :mongoid do # gem "mongoid", "~> 4.0.0" # end + +gem "warden", "~> 1.2.3", path: '/gems/warden' diff --git a/Gemfile.lock b/Gemfile.lock index 92779c4c3d..a4b2de2559 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -17,6 +17,12 @@ PATH responders warden (~> 1.2.3) +PATH + remote: ../gems/warden + specs: + warden (1.2.9) + rack (>= 2.2.3) + GEM remote: https://rubygems.org/ specs: @@ -236,8 +242,6 @@ GEM tzinfo (2.0.6) concurrent-ruby (~> 1.0) version_gem (1.1.3) - warden (1.2.9) - rack (>= 2.0.9) webrat (0.7.3) nokogiri (>= 1.2.0) rack (>= 1.0) @@ -265,6 +269,7 @@ DEPENDENCIES rexml sqlite3 (~> 1.4) timecop + warden (~> 1.2.3)! webrat (= 0.7.3) BUNDLED WITH diff --git a/docker-compose-earthly.yml b/docker-compose-earthly.yml new file mode 100644 index 0000000000..81433934f5 --- /dev/null +++ b/docker-compose-earthly.yml @@ -0,0 +1,7 @@ +# This file is only used by Earthly + +name: devise + +services: + gem: + image: heartcombo/devise:latest diff --git a/docker-compose.yml b/docker-compose.yml new file mode 100644 index 0000000000..1bd97a087f --- /dev/null +++ b/docker-compose.yml @@ -0,0 +1,11 @@ +# This file allows you to use the `docker compose` command in order to run Ruby +# commands or tests. + +name: devise + +services: + # docker compose run --rm gem [rspec [path to spec file]] + gem: + image: heartcombo/devise:latest + volumes: + - $PWD:/gem/ diff --git a/lib/devise/failure_app.rb b/lib/devise/failure_app.rb index e1e24be42e..927adbe543 100644 --- a/lib/devise/failure_app.rb +++ b/lib/devise/failure_app.rb @@ -75,7 +75,7 @@ def recall end end - flash.now[:alert] = i18n_message(:invalid) if is_flashing_format? + flash.now[:alert] = i18n_messages(:invalid) if is_flashing_format? self.response = recall_app(warden_options[:recall]).call(request.env).tap { |response| response[0] = Rack::Utils.status_code( response[0].in?(300..399) ? Devise.responder.redirect_status : Devise.responder.error_status @@ -90,7 +90,7 @@ def redirect flash.keep(:timedout) flash.keep(:alert) else - flash[:alert] = i18n_message + flash[:alert] = i18n_messages end end redirect_to redirect_url @@ -102,9 +102,13 @@ def i18n_options(options) options end - def i18n_message(default = nil) - message = warden_message || default || :unauthenticated + def i18n_messages(default = nil) + Array(warden_messages || default || :unauthenticated).map do |message| + i18n_message(message) + end + end + def i18n_message(message) if message.is_a?(Symbol) options = {} options[:resource_name] = scope @@ -126,7 +130,7 @@ def i18n_locale end def redirect_url - if warden_message == :timeout + if Array(warden_messages).include?(:timeout) flash[:timedout] = true if is_flashing_format? path = if request.get? @@ -199,14 +203,14 @@ def http_auth_header? end def http_auth_body - return i18n_message unless request_format + return i18n_messages unless request_format method = "to_#{request_format}" if method == "to_xml" - { error: i18n_message }.to_xml(root: "errors") + i18n_messages.to_xml(root: 'errors', skip_types: true) elsif {}.respond_to?(method) - { error: i18n_message }.send(method) + { errors: i18n_messages }.send(method) else - i18n_message + i18n_messages end end @@ -225,8 +229,8 @@ def warden_options request.respond_to?(:get_header) ? request.get_header("warden.options") : request.env["warden.options"] end - def warden_message - @message ||= warden.message || warden_options[:message] + def warden_messages + @messages ||= warden.messages.presence || warden_options[:messages].presence end def scope diff --git a/lib/devise/hooks/activatable.rb b/lib/devise/hooks/activatable.rb index b2eaea199f..042b5e550c 100644 --- a/lib/devise/hooks/activatable.rb +++ b/lib/devise/hooks/activatable.rb @@ -7,6 +7,6 @@ if record && record.respond_to?(:active_for_authentication?) && !record.active_for_authentication? scope = options[:scope] warden.logout(scope) - throw :warden, scope: scope, message: record.inactive_message + throw :warden, scope: scope, messages: record.inactive_message end end diff --git a/lib/devise/hooks/timeoutable.rb b/lib/devise/hooks/timeoutable.rb index 772eb142b7..3cafe2d30d 100644 --- a/lib/devise/hooks/timeoutable.rb +++ b/lib/devise/hooks/timeoutable.rb @@ -25,7 +25,7 @@ record.timedout?(last_request_at) && !proxy.remember_me_is_active?(record) Devise.sign_out_all_scopes ? proxy.sign_out : proxy.sign_out(scope) - throw :warden, scope: scope, message: :timeout + throw :warden, scope: scope, messages: [:timeout] end unless env['devise.skip_trackable'] diff --git a/lib/devise/models/authenticatable.rb b/lib/devise/models/authenticatable.rb index df964537ea..8408c2df8d 100644 --- a/lib/devise/models/authenticatable.rb +++ b/lib/devise/models/authenticatable.rb @@ -64,6 +64,9 @@ module Authenticatable class_attribute :devise_modules, instance_writer: false self.devise_modules ||= [] + class_attribute :devise_messages + self.devise_messages = [] + before_validation :downcase_keys before_validation :strip_whitespace end @@ -82,10 +85,6 @@ def valid_for_authentication? block_given? ? yield : true end - def unauthenticated_message - :invalid - end - def active_for_authentication? true end @@ -124,6 +123,10 @@ def inspect "#<#{self.class} #{inspection.join(", ")}>" end + def reset_devise_messages! + self.devise_messages = [] + end + protected def devise_mailer diff --git a/lib/devise/models/confirmable.rb b/lib/devise/models/confirmable.rb index 6ce22c30f0..5dc2cddcda 100644 --- a/lib/devise/models/confirmable.rb +++ b/lib/devise/models/confirmable.rb @@ -142,10 +142,15 @@ def resend_confirmation_instructions # is already confirmed, it should never be blocked. Otherwise we need to # calculate if the confirm time has not expired for this user. def active_for_authentication? - super && (!confirmation_required? || confirmed? || confirmation_period_valid?) + valid = super && (!confirmation_required? || confirmed? || confirmation_period_valid?) + + devise_messages << :unconfirmed unless valid + + valid end - # The message to be shown if the account is inactive. + # Devise::RegistrationsController uses this method to determine the flash + # message to be shown to the user with `signed_up_but_#{inactive_message}` def inactive_message !confirmed? ? :unconfirmed : super end diff --git a/lib/devise/models/lockable.rb b/lib/devise/models/lockable.rb index 65bb400d0e..c3e15c079a 100644 --- a/lib/devise/models/lockable.rb +++ b/lib/devise/models/lockable.rb @@ -41,6 +41,7 @@ def self.required_fields(klass) # `{ send_instructions: false } as option`. def lock_access!(opts = { }) self.locked_at = Time.now.utc + devise_messages << :locked if unlock_strategy_enabled?(:email) && opts.fetch(:send_instructions, true) send_unlock_instructions @@ -87,13 +88,11 @@ def resend_unlock_instructions # Overwrites active_for_authentication? from Devise::Models::Activatable for locking purposes # by verifying whether a user is active to sign in or not based on locked? def active_for_authentication? - super && !access_locked? - end + valid = super && !access_locked? + + devise_messages << :locked unless valid - # Overwrites invalid_message from Devise::Models::Authenticatable to define - # the correct reason for blocking the sign in. - def inactive_message - access_locked? ? :locked : super + valid end # Overwrites valid_for_authentication? from Devise::Models::Authenticatable @@ -115,6 +114,8 @@ def valid_for_authentication? else save(validate: false) end + + add_unauthenticated_message false end end @@ -124,17 +125,21 @@ def increment_failed_attempts reload end + def add_unauthenticated_message + devise_messages << unauthenticated_message + end + def unauthenticated_message # If set to paranoid mode, do not show the locked message because it # leaks the existence of an account. if Devise.paranoid - super + :invalid elsif access_locked? || (lock_strategy_enabled?(:failed_attempts) && attempts_exceeded?) :locked elsif lock_strategy_enabled?(:failed_attempts) && last_attempt? && self.class.last_attempt_warning :last_attempt else - super + :invalid end end diff --git a/lib/devise/strategies/authenticatable.rb b/lib/devise/strategies/authenticatable.rb index 2af7a741cf..05ea3186bf 100644 --- a/lib/devise/strategies/authenticatable.rb +++ b/lib/devise/strategies/authenticatable.rb @@ -35,16 +35,15 @@ def clean_up_csrf? # In case the resource can't be validated, it will fail with the given # unauthenticated_message. def validate(resource, &block) - result = resource && resource.valid_for_authentication?(&block) + result = resource && resource.reset_devise_messages! && resource.valid_for_authentication?(&block) - if result - true - else - if resource - fail!(resource.unauthenticated_message) - end - false + return true if result + + if resource + fail!(resource.devise_messages) end + + false end # Get values from params and set in the resource. diff --git a/lib/devise/strategies/database_authenticatable.rb b/lib/devise/strategies/database_authenticatable.rb index f7e007d144..a2435fa44e 100644 --- a/lib/devise/strategies/database_authenticatable.rb +++ b/lib/devise/strategies/database_authenticatable.rb @@ -7,7 +7,7 @@ module Strategies # Default strategy for signing in a user, based on their email and password in the database. class DatabaseAuthenticatable < Authenticatable def authenticate! - resource = password.present? && mapping.to.find_for_database_authentication(authentication_hash) + resource = password.present? && mapping.to.find_for_database_authentication(authentication_hash) hashed = false if validate(resource){ hashed = true; resource.valid_password?(password) } diff --git a/test/failure_app_test.rb b/test/failure_app_test.rb index e8f316f0db..1049a6022d 100644 --- a/test/failure_app_test.rb +++ b/test/failure_app_test.rb @@ -81,7 +81,7 @@ def call_failure(env_params = {}) 'warden.options' => { scope: :user }, 'action_dispatch.request.formats' => Array(env_params.delete('formats') || Mime[:html]), 'rack.input' => "", - 'warden' => OpenStruct.new(message: nil) + 'warden' => OpenStruct.new(messages: nil) }.merge!(env_params) # Passing nil for action_dispatch.request.formats prevents the default from being used in Rails 5, need to remove it @@ -97,14 +97,14 @@ def call_failure(env_params = {}) test 'returns to the default redirect location' do call_failure assert_equal 302, @response.first - assert_equal 'You need to sign in or sign up before continuing.', @request.flash[:alert] + assert_equal ['You need to sign in or sign up before continuing.'], @request.flash[:alert] assert_equal 'http://test.host/users/sign_in', @response.second['Location'] end test 'returns to the default redirect location considering subdomain' do call_failure('warden.options' => { scope: :subdomain_user }) assert_equal 302, @response.first - assert_equal 'You need to sign in or sign up before continuing.', @request.flash[:alert] + assert_equal ['You need to sign in or sign up before continuing.'], @request.flash[:alert] assert_equal 'http://sub.test.host/subdomain_users/sign_in', @response.second['Location'] end @@ -118,7 +118,7 @@ def call_failure(env_params = {}) swap Devise, router_name: :fake_app do call_failure app: RootFailureApp assert_equal 302, @response.first - assert_equal 'You need to sign in or sign up before continuing.', @request.flash[:alert] + assert_equal ['You need to sign in or sign up before continuing.'], @request.flash[:alert] assert_equal 'http://test.host/', @response.second['Location'] end end @@ -126,7 +126,7 @@ def call_failure(env_params = {}) test 'returns to the root path even when it\'s not defined' do call_failure app: FailureWithoutRootPath assert_equal 302, @response.first - assert_equal 'You need to sign in or sign up before continuing.', @request.flash[:alert] + assert_equal ['You need to sign in or sign up before continuing.'], @request.flash[:alert] assert_equal 'http://test.host/', @response.second['Location'] end @@ -134,7 +134,7 @@ def call_failure(env_params = {}) swap Devise, router_name: :fake_app do call_failure app: FailureWithSubdomain assert_equal 302, @response.first - assert_equal 'You need to sign in or sign up before continuing.', @request.flash[:alert] + assert_equal ['You need to sign in or sign up before continuing.'], @request.flash[:alert] assert_equal 'http://sub.test.host/', @response.second['Location'] end end @@ -142,7 +142,7 @@ def call_failure(env_params = {}) test 'returns to the default redirect location considering the router for supplied scope' do call_failure app: FakeEngineApp, 'warden.options' => { scope: :user_on_engine } assert_equal 302, @response.first - assert_equal 'You need to sign in or sign up before continuing.', @request.flash[:alert] + assert_equal ['You need to sign in or sign up before continuing.'], @request.flash[:alert] assert_equal 'http://test.host/user_on_engines/sign_in', @response.second['Location'] end @@ -183,33 +183,33 @@ def call_failure(env_params = {}) end test 'uses the proxy failure message as symbol' do - call_failure('warden' => OpenStruct.new(message: :invalid)) - assert_equal 'Invalid Email or password.', @request.flash[:alert] + call_failure('warden' => OpenStruct.new(messages: :invalid)) + assert_equal ['Invalid Email or password.'], @request.flash[:alert] assert_equal 'http://test.host/users/sign_in', @response.second["Location"] end test 'supports authentication_keys as a Hash for the flash message' do swap Devise, authentication_keys: { email: true, login: true } do - call_failure('warden' => OpenStruct.new(message: :invalid)) - assert_equal 'Invalid Email, Login or password.', @request.flash[:alert] + call_failure('warden' => OpenStruct.new(messages: :invalid)) + assert_equal ['Invalid Email, Login or password.'], @request.flash[:alert] end end test 'uses custom i18n options' do - call_failure('warden' => OpenStruct.new(message: :does_not_exist), app: FailureWithI18nOptions) - assert_equal 'User Steve does not exist', @request.flash[:alert] + call_failure('warden' => OpenStruct.new(messages: :does_not_exist), app: FailureWithI18nOptions) + assert_equal ['User Steve does not exist'], @request.flash[:alert] end test 'respects the i18n locale passed via warden options when redirecting' do - call_failure('warden' => OpenStruct.new(message: :invalid), 'warden.options' => { locale: :"pt-BR" }) + call_failure('warden' => OpenStruct.new(messages: :invalid), 'warden.options' => { locale: :"pt-BR" }) - assert_equal 'Email ou senha inválidos.', @request.flash[:alert] + assert_equal ['Email ou senha inválidos.'], @request.flash[:alert] assert_equal 'http://test.host/users/sign_in', @response.second["Location"] end test 'uses the proxy failure message as string' do - call_failure('warden' => OpenStruct.new(message: 'Hello world')) - assert_equal 'Hello world', @request.flash[:alert] + call_failure('warden' => OpenStruct.new(messages: 'Hello world')) + assert_equal ['Hello world'], @request.flash[:alert] assert_equal 'http://test.host/users/sign_in', @response.second["Location"] end @@ -258,7 +258,7 @@ def call_failure(env_params = {}) test 'return appropriate body for json' do call_failure('formats' => Mime[:json]) - result = %({"error":"You need to sign in or sign up before continuing."}) + result = %({"errors":["You need to sign in or sign up before continuing."]}) assert_equal result, @response.last.body end @@ -287,14 +287,14 @@ def call_failure(env_params = {}) end test 'uses the failure message as response body' do - call_failure('formats' => Mime[:xml], 'warden' => OpenStruct.new(message: :invalid)) + call_failure('formats' => Mime[:xml], 'warden' => OpenStruct.new(messages: :invalid)) assert_match 'Invalid Email or password.', @response.third.body end test 'respects the i18n locale passed via warden options when responding to HTTP request' do - call_failure('formats' => Mime[:json], 'warden' => OpenStruct.new(message: :invalid), 'warden.options' => { locale: :"pt-BR" }) + call_failure('formats' => Mime[:json], 'warden' => OpenStruct.new(messages: :invalid), 'warden.options' => { locale: :"pt-BR" }) - assert_equal %({"error":"Email ou senha inválidos."}), @response.third.body + assert_equal %({"errors":["Email ou senha inválidos."]}), @response.third.body end context 'on ajax call' do @@ -348,7 +348,7 @@ def call_failure(env_params = {}) test 'calls the original controller if not confirmed email' do env = { - "warden.options" => { recall: "devise/sessions#new", attempted_path: "/users/sign_in", message: :unconfirmed }, + "warden.options" => { recall: "devise/sessions#new", attempted_path: "/users/sign_in", messages: :unconfirmed }, "devise.mapping" => Devise.mappings[:user], "warden" => stub_everything } @@ -359,7 +359,7 @@ def call_failure(env_params = {}) test 'calls the original controller if inactive account' do env = { - "warden.options" => { recall: "devise/sessions#new", attempted_path: "/users/sign_in", message: :inactive }, + "warden.options" => { recall: "devise/sessions#new", attempted_path: "/users/sign_in", messages: :inactive }, "devise.mapping" => Devise.mappings[:user], "warden" => stub_everything } diff --git a/test/integration/http_authenticatable_test.rb b/test/integration/http_authenticatable_test.rb index 707a070567..1525a68046 100644 --- a/test/integration/http_authenticatable_test.rb +++ b/test/integration/http_authenticatable_test.rb @@ -52,7 +52,7 @@ class HttpAuthenticationTest < Devise::IntegrationTest sign_in_as_new_user_with_http("unknown") assert_equal 401, status assert_equal "application/json; charset=utf-8", headers["Content-Type"] - assert_match '"error":"Invalid Email or password."', response.body + assert_match '"errors":["Invalid Email or password."]', response.body end test 'returns a custom response with www-authenticate and chosen realm' do diff --git a/test/test/controller_helpers_test.rb b/test/test/controller_helpers_test.rb index 7ba9f3c678..b1560d40d4 100644 --- a/test/test/controller_helpers_test.rb +++ b/test/test/controller_helpers_test.rb @@ -9,7 +9,7 @@ class TestControllerHelpersTest < Devise::ControllerTestCase test "redirects if attempting to access a page unauthenticated" do get :index assert_redirected_to new_user_session_path - assert_equal "You need to sign in or sign up before continuing.", flash[:alert] + assert_equal ['You need to sign in or sign up before continuing.'], flash[:alert] end test "redirects if attempting to access a page with an unconfirmed account" do