Allow a strategy to be added after authenticatable one and move store_location to FailureApp.

CHANGELOG.rdoc

@@ -1,4 +1,6 @@
 * enhancements
+  * Remove store_location from authenticatable strategy and add it to failure app
+  * Allow a strategy to be placed after authenticatable
   * [#45] Do not rely attribute? methods, since they are not added on Datamapper
 
 == 0.5.6

lib/devise/failure_app.rb

@@ -1,31 +1,34 @@
 module Devise
-  module FailureApp
-    mattr_accessor :default_url
+  # Failure application that will be called every time :warden is thrown from
+  # any strategy or hook. Responsible for redirect the user to the sign in
+  # page based on current scope and mapping. If no scope is given, redirect
+  # to the default_url.
+  class FailureApp
+    attr_reader :env
+    include Warden::Mixins::Common
+
+    cattr_accessor :default_url, :default_message, :instance_writer => false
+    @@default_message = :unauthenticated
 
-    # Failure application that will be called every time :warden is thrown from
-    # any strategy or hook. Responsible for redirect the user to the sign in
-    # page based on current scope and mapping. If no scope is given, redirect
-    # to the default_url.
     def self.call(env)
-      options = env['warden.options']
-      scope   = options[:scope]
-      message = env['warden'].try(:message) || options[:message]
+      new(env).respond!
+    end
 
-      params  = case message
-        when Symbol
-          { message => true }
-        when String
-          { :message => message }
-        else
-          {}
-      end
+    def initialize(env)
+      @env = env
+    end
+
+    def respond!
+      options = @env['warden.options']
+      scope   = options[:scope]
 
       redirect_path = if mapping = Devise.mappings[scope]
         "#{mapping.parsed_path}/#{mapping.path_names[:sign_in]}"
       else
         "/#{default_url}"
       end
-      query_string = Rack::Utils.build_query(params)
+      query_string = query_string_for(options)
+      store_location!(scope)
 
       headers = {}
       headers["Location"] = redirect_path
@@ -34,5 +37,29 @@ module Devise
 
       [302, headers, ["You are being redirected to #{redirect_path}"]]
     end
+
+    # Build the proper query string based on the given message.
+    def query_string_for(options)
+      message = @env['warden'].try(:message) || options[:message] || default_message
+
+      params = case message
+        when Symbol
+          { message => true }
+        when String
+          { :message => message }
+        else
+          {}
+      end
+
+      Rack::Utils.build_query(params)
+    end
+
+    # Stores requested uri to redirect the user after signing in. We cannot use
+    # scoped session provided by warden here, since the user is not authenticated
+    # yet, but we still need to store the uri based on scope, so different scopes
+    # would never use the same uri to redirect.
+    def store_location!(scope)
+      session[:"#{scope}.return_to"] ||= request.request_uri if request && request.get?
+    end
   end
 end

lib/devise/strategies/authenticatable.rb

@@ -5,40 +5,20 @@ module Devise
     class Authenticatable < Warden::Strategies::Base
       include Devise::Strategies::Base
 
+      def valid?
+        super && params[scope] && params[scope][:password].present?
+      end
+
       # Authenticate a user based on email and password params, returning to warden
       # success and the authenticated user if everything is okay. Otherwise redirect
       # to sign in page.
-      #
-      # Please notice the semantic difference between calling fail! and throw :warden.
-      # The first does not perform any action when calling authenticate, just
-      # when authenticate! is invoked. The second always perform the action.
       def authenticate!
-        if valid_attributes?
-          if resource = mapping.to.authenticate(params[scope])
-            success!(resource)
-          else
-            fail!(:invalid)
-          end
+        if resource = mapping.to.authenticate(params[scope])
+          success!(resource)
         else
-          store_location
-          fail!(:unauthenticated)
+          fail!(:invalid)
         end
       end
-
-      private
-
-        # Check if params and password are given. Others are checked inside authenticate.
-        def valid_attributes?
-          params[scope] && params[scope][:password].present?
-        end
-
-        # Stores requested uri to redirect the user after signing in. We cannot use
-        # scoped session provided by warden here, since the user is not authenticated
-        # yet, but we still need to store the uri based on scope, so different scopes
-        # would never use the same uri to redirect.
-        def store_location
-          session[:"#{mapping.name}.return_to"] ||= request.request_uri if request.get?
-        end
     end
   end
 end

test/failure_app_test.rb

@@ -12,8 +12,8 @@ class FailureTest < ActiveSupport::TestCase
     assert_equal 302, call_failure.first
   end
 
-  test 'return redirect location based on mapping with params' do
-    assert_equal '/users/sign_in', call_failure.second['Location']
+  test 'return to the default redirect location' do
+    assert_equal '/users/sign_in?unauthenticated=true', call_failure.second['Location']
   end
 
   test 'uses the proxy failure message' do
@@ -27,6 +27,6 @@ class FailureTest < ActiveSupport::TestCase
   end
 
   test 'setup a default message' do
-    assert_equal ['You are being redirected to /users/sign_in'], call_failure.last
+    assert_equal ['You are being redirected to /users/sign_in?unauthenticated=true'], call_failure.last
   end
 end