Skip to content

Commit

Permalink
Ensure fail! works inside strategies, create unauthenticated and inva…
Browse files Browse the repository at this point in the history
…lid messages and do not redirect on invalid authentication.
  • Loading branch information
josevalim committed Oct 29, 2009
1 parent 475da06 commit 5172d50
Show file tree
Hide file tree
Showing 7 changed files with 35 additions and 25 deletions.
8 changes: 8 additions & 0 deletions CHANGELOG.rdoc
Original file line number Diff line number Diff line change
@@ -1,3 +1,11 @@
* enhancements
* Ensure fail! works inside strategies
* Make unauthenticated message (when you haven't logged in) different from
invalid message (when your credentials are invalid)

* bug fix
* Do not redirect on invalid authenticate

== 0.2.2

* bug fix
Expand Down
23 changes: 10 additions & 13 deletions app/controllers/sessions_controller.rb
Original file line number Diff line number Diff line change
@@ -1,12 +1,19 @@
class SessionsController < ApplicationController
include Devise::Controllers::Helpers

# Maps the messages types that comes from warden to a flash type.
WARDEN_MESSAGES = {
:unauthenticated => :success,
:unconfirmed => :failure
}

before_filter :require_no_authentication, :only => [ :new, :create ]

# GET /resource/sign_in
def new
unauthenticated! if params[:unauthenticated]
unconfirmed! if params[:unconfirmed]
WARDEN_MESSAGES.each do |message, type|
set_now_flash_message type, message if params.key?(message)
end
build_resource
end

Expand All @@ -16,7 +23,7 @@ def create
set_flash_message :success, :signed_in
redirect_back_or_to home_or_root_path
else
unauthenticated!
set_now_flash_message :failure, :invalid
build_resource
render :new
end
Expand All @@ -29,14 +36,4 @@ def destroy
redirect_to root_path
end

protected

def unauthenticated!
set_now_flash_message :failure, :unauthenticated
end

def unconfirmed!
set_now_flash_message :failure, :unconfirmed
end

end
3 changes: 2 additions & 1 deletion config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,9 @@ en:
sessions:
signed_in: 'Signed in successfully.'
signed_out: 'Signed out successfully.'
unauthenticated: 'Invalid email or password.'
unauthenticated: 'You need to sign in or sign up before continuing.'
unconfirmed: 'Your account was not confirmed and your confirmation period has expired.'
invalid: 'Invalid email or password.'
passwords:
send_instructions: 'You will receive an email with instructions about how to reset your password in a few minutes.'
updated: 'Your password was changed successfully. You are now signed in.'
Expand Down
8 changes: 6 additions & 2 deletions lib/devise/failure.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,12 @@ module Failure
# to the default_url.
def self.call(env)
options = env['warden.options']
params = options[:params] || {}
scope = options[:scope]
params = if env['warden'].try(:message)
{ env['warden'].message => true }
else
options[:params]
end

redirect_path = if mapping = Devise.mappings[scope]
"/#{mapping.as}/#{mapping.path_names[:sign_in]}"
Expand All @@ -19,7 +23,7 @@ def self.call(env)

headers = {}
headers["Location"] = redirect_path
headers["Location"] << "?" << Rack::Utils.build_query(params) unless params.empty?
headers["Location"] << "?" << Rack::Utils.build_query(params) if params
headers["Content-Type"] = 'text/plain'

message = options[:message] || "You are being redirected to #{redirect_path}"
Expand Down
6 changes: 5 additions & 1 deletion lib/devise/strategies/authenticable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,16 @@ class Authenticable < Devise::Strategies::Base
# 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? && resource = mapping.to.authenticate(attributes)
success!(resource)
else
store_location
throw :warden, :scope => scope, :params => { :unauthenticated => true }
fail!(:unauthenticated)
end
end

Expand Down
11 changes: 3 additions & 8 deletions test/integration/authenticable_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,6 @@ class AuthenticationTest < ActionController::IntegrationTest
fill_in 'email', :with => 'wrongemail@test.com'
end

assert_redirected_to new_admin_session_path(:unauthenticated => true)
follow_redirect!
assert_contain 'Invalid email or password'
assert_not warden.authenticated?(:admin)
end
Expand All @@ -92,22 +90,19 @@ class AuthenticationTest < ActionController::IntegrationTest
fill_in 'password', :with => 'abcdef'
end

assert_redirected_to new_admin_session_path(:unauthenticated => true)
follow_redirect!
assert_contain 'Invalid email or password'
assert_not warden.authenticated?(:admin)
end

test 'error message is configurable by resource name' do
begin
I18n.backend.store_translations(:en, :devise => { :sessions =>
{ :admin => { :unauthenticated => "Invalid credentials" } } })
{ :admin => { :invalid => "Invalid credentials" } } })

sign_in_as_admin do
fill_in 'password', :with => 'abcdef'
end

follow_redirect!
assert_contain 'Invalid credentials'
ensure
I18n.reload!
Expand Down Expand Up @@ -145,14 +140,14 @@ class AuthenticationTest < ActionController::IntegrationTest
assert_not_contain 'Signed out successfully'
end

test 'redirect from warden shows error message' do
test 'redirect from warden shows sign in or sign up message' do
get admins_path

warden_path = new_admin_session_path(:unauthenticated => true)
assert_redirected_to warden_path

get warden_path
assert_contain 'Invalid email or password.'
assert_contain 'You need to sign in or sign up before continuing.'
end

test 'render 404 on roles without permission' do
Expand Down
1 change: 1 addition & 0 deletions test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
ActiveRecord::Migration.verbose = false
ActiveRecord::Base.logger = Logger.new(nil)
ActiveRecord::Base.establish_connection(:adapter => "sqlite3", :database => ":memory:")

ActiveRecord::Schema.define(:version => 1) do
[:users, :admins].each do |table|
create_table table do |t|
Expand Down

0 comments on commit 5172d50

Please sign in to comment.