Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check if there is a logged user before logout. #2968

Merged
merged 1 commit into from Apr 9, 2014
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -8,6 +8,7 @@
* Allow a resource to be found based on its encrypted password token (by @karlentwistle)

* bug fix
* Check if there is a signed in user before executing the `SessionsController#destroy`.
* `SessionsController#destroy` no longer yields the `resource` to receiving block,
since the resource isn't loaded in the action. If you need access to the current
resource when overring the action use the scope helper (like `current_user`) before
Expand Down
38 changes: 31 additions & 7 deletions app/controllers/devise/sessions_controller.rb
@@ -1,6 +1,7 @@
class Devise::SessionsController < DeviseController
prepend_before_filter :require_no_authentication, only: [ :new, :create ]
prepend_before_filter :allow_params_authentication!, only: :create
prepend_before_filter :verify_signed_out_user, only: :destroy
prepend_before_filter only: [ :create, :destroy ] { request.env["devise.skip_timeout"] = true }

# GET /resource/sign_in
Expand All @@ -21,17 +22,11 @@ def create

# DELETE /resource/sign_out
def destroy
redirect_path = after_sign_out_path_for(resource_name)
signed_out = (Devise.sign_out_all_scopes ? sign_out : sign_out(resource_name))
set_flash_message :notice, :signed_out if signed_out && is_flashing_format?
yield if block_given?

# We actually need to hardcode this as Rails default responder doesn't
# support returning empty response on GET request
respond_to do |format|
format.all { head :no_content }
format.any(*navigational_formats) { redirect_to redirect_path }
end
respond_to_on_destroy
end

protected
Expand All @@ -50,4 +45,33 @@ def serialize_options(resource)
def auth_options
{ scope: resource_name, recall: "#{controller_path}#new" }
end

private

# Check if there is no signed in user before doing the sign out.
#
# If there is no signed in user, it will set the flash message and redirect
# to the after_sign_out path.
def verify_signed_out_user
if all_signed_out?
set_flash_message :notice, :already_signed_out if is_flashing_format?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for the user it doesn't matter if he's already signed out or not. Does it? I would simply do something like this:

def verify_signed_out_user
  destroy_session if user_already_signed_out?
end

Where destroy_session would be exactly this code: https://github.com/plataformatec/devise/blob/sign_out_not_logged_in_user/app/controllers/devise/sessions_controller.rb#L27-L35 shared in between the filter and the action.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, ignore me. I think your approach is better. :D

I would just move the respond_to code to a respond_to_on_destroy so we can share it in between the filter and the action.


respond_to_on_destroy
end
end

def all_signed_out?
users = Devise.mappings.keys.map { |s| warden.user(scope: s, run_callbacks: false) }

users.all?(&:blank?)
end

def respond_to_on_destroy
# We actually need to hardcode this as Rails default responder doesn't
# support returning empty response on GET request
respond_to do |format|
format.all { head :no_content }
format.any(*navigational_formats) { redirect_to after_sign_out_path_for(resource_name) }
end
end
end
1 change: 1 addition & 0 deletions config/locales/en.yml
Expand Up @@ -43,6 +43,7 @@ en:
sessions:
signed_in: "Signed in successfully."
signed_out: "Signed out successfully."
already_signed_out: "Signed out successfully."
unlocks:
send_instructions: "You will receive an email with instructions for how to unlock your account in a few minutes."
send_paranoid_instructions: "If your account exists, you will receive an email with instructions for how to unlock it in a few minutes."
Expand Down
20 changes: 18 additions & 2 deletions test/integration/authenticatable_test.rb
Expand Up @@ -118,13 +118,13 @@ class AuthenticationSanityTest < ActionDispatch::IntegrationTest
assert_not warden.authenticated?(:admin)
end

test 'unauthenticated admin does not set message on sign out' do
test 'unauthenticated admin set message on sign out' do
get destroy_admin_session_path
assert_response :redirect
assert_redirected_to root_path

get root_path
assert_not_contain 'Signed out successfully'
assert_contain 'Signed out successfully'
end

test 'scope uses custom failure app' do
Expand Down Expand Up @@ -711,3 +711,19 @@ class DoubleAuthenticationRedirectTest < ActionDispatch::IntegrationTest
assert_redirected_to '/admin_area/home'
end
end

class DoubleSignOutRedirectTest < ActionDispatch::IntegrationTest
test 'sign out after already having signed out redirects to sign in' do
sign_in_as_user

post destroy_sign_out_via_delete_or_post_session_path

get root_path
assert_contain 'Signed out successfully.'

post destroy_sign_out_via_delete_or_post_session_path

get root_path
assert_contain 'Signed out successfully.'
end
end