From 8a774e75193fcb66eea9e76b60f10623e1e0e682 Mon Sep 17 00:00:00 2001 From: Andy Allan Date: Wed, 8 Jan 2020 14:01:17 +0100 Subject: [PATCH 1/2] Use a post link to logout This avoids needing to access the session id, which is currently only working with the memcache store. The fallback page is preserved for anyone who wants to logout without using javascript. Refs #2488 --- app/controllers/users_controller.rb | 2 +- app/views/layouts/_header.html.erb | 2 +- app/views/users/logout.html.erb | 1 - test/controllers/users_controller_test.rb | 29 +++-------------------- test/system/user_logout_test.rb | 22 +++++++++++++++++ 5 files changed, 27 insertions(+), 29 deletions(-) create mode 100644 test/system/user_logout_test.rb diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index a61a10d94f..514b3f8ee7 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -269,7 +269,7 @@ def login def logout @title = t "users.logout.title" - if params[:session] == session.id + if request.post? if session[:token] token = UserToken.find_by(:token => session[:token]) token&.destroy diff --git a/app/views/layouts/_header.html.erb b/app/views/layouts/_header.html.erb index 6df8f02da2..3963c211e1 100644 --- a/app/views/layouts/_header.html.erb +++ b/app/views/layouts/_header.html.erb @@ -102,7 +102,7 @@ <%= yield :greeting %>
  • - <%= link_to t("layouts.logout"), logout_path(:session => session.id, :referer => request.fullpath), :class => "geolink" %> + <%= link_to t("layouts.logout"), logout_path(:referer => request.fullpath), :method => "post", :class => "geolink" %>
    • diff --git a/app/views/users/logout.html.erb b/app/views/users/logout.html.erb index 273c7e1b94..5d8e2de492 100644 --- a/app/views/users/logout.html.erb +++ b/app/views/users/logout.html.erb @@ -4,6 +4,5 @@ <%= form_tag :action => "logout" do %> <%= hidden_field_tag("referer", h(params[:referer])) %> - <%= hidden_field_tag("session", session.id) %> <%= submit_tag t(".logout_button") %> <% end %> diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index feca92df56..4417d353fa 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -344,27 +344,13 @@ def test_save_referer_params end def test_logout_without_referer - get :logout - assert_response :success - assert_template :logout - assert_select "input[name=referer][value=?]", "" - - session_id = assert_select("input[name=session]").first["value"] - - get :logout, :params => { :session => session_id } + post :logout assert_response :redirect assert_redirected_to root_path end def test_logout_with_referer - get :logout, :params => { :referer => "/test" } - assert_response :success - assert_template :logout - assert_select "input[name=referer][value=?]", "/test" - - session_id = assert_select("input[name=session]").first["value"] - - get :logout, :params => { :session => session_id, :referer => "/test" } + post :logout, :params => { :referer => "/test" } assert_response :redirect assert_redirected_to "/test" end @@ -374,16 +360,7 @@ def test_logout_with_token session[:token] = token.token - get :logout - assert_response :success - assert_template :logout - assert_select "input[name=referer][value=?]", "" - assert_equal token.token, session[:token] - assert_not_nil UserToken.where(:id => token.id).first - - session_id = assert_select("input[name=session]").first["value"] - - get :logout, :params => { :session => session_id } + post :logout assert_response :redirect assert_redirected_to root_path assert_nil session[:token] diff --git a/test/system/user_logout_test.rb b/test/system/user_logout_test.rb new file mode 100644 index 0000000000..a2e145fcc6 --- /dev/null +++ b/test/system/user_logout_test.rb @@ -0,0 +1,22 @@ +require "application_system_test_case" + +class UserLogoutTest < ApplicationSystemTestCase + test "Sign out via link" do + user = create(:user) + sign_in_as(user) + + click_on user.display_name + click_on "Log Out" + assert page.has_content? "Log In" + end + + test "Sign out via fallback page" do + sign_in_as(create(:user)) + + visit logout_path + assert page.has_content? "Logout from OpenStreetMap" + + click_button "Logout" + assert page.has_content? "Log In" + end +end From 9643e3393d546ae8a7bfb68ab52b5c72c92320b2 Mon Sep 17 00:00:00 2001 From: Andy Allan Date: Wed, 8 Jan 2020 18:26:57 +0100 Subject: [PATCH 2/2] Update tests to ensure referer is working This reinstates the form tests in the controller test, but uses the system tests to actually click the button and make sure that it works --- test/controllers/users_controller_test.rb | 14 ++++++++++++ test/system/user_logout_test.rb | 26 +++++++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index 4417d353fa..c40c30b28a 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -355,6 +355,20 @@ def test_logout_with_referer assert_redirected_to "/test" end + def test_logout_fallback_without_referer + get :logout + assert_response :success + assert_template :logout + assert_select "input[name=referer][value=?]", "" + end + + def test_logout_fallback_with_referer + get :logout, :params => { :referer => "/test" } + assert_response :success + assert_template :logout + assert_select "input[name=referer][value=?]", "/test" + end + def test_logout_with_token token = create(:user).tokens.create diff --git a/test/system/user_logout_test.rb b/test/system/user_logout_test.rb index a2e145fcc6..099d2c0c02 100644 --- a/test/system/user_logout_test.rb +++ b/test/system/user_logout_test.rb @@ -4,14 +4,28 @@ class UserLogoutTest < ApplicationSystemTestCase test "Sign out via link" do user = create(:user) sign_in_as(user) + assert_not page.has_content? "Log In" click_on user.display_name click_on "Log Out" assert page.has_content? "Log In" end + test "Sign out via link with referer" do + user = create(:user) + sign_in_as(user) + visit traces_path + assert_not page.has_content? "Log In" + + click_on user.display_name + click_on "Log Out" + assert page.has_content? "Log In" + assert page.has_content? "Public GPS traces" + end + test "Sign out via fallback page" do sign_in_as(create(:user)) + assert_not page.has_content? "Log In" visit logout_path assert page.has_content? "Logout from OpenStreetMap" @@ -19,4 +33,16 @@ class UserLogoutTest < ApplicationSystemTestCase click_button "Logout" assert page.has_content? "Log In" end + + test "Sign out via fallback page with referer" do + sign_in_as(create(:user)) + assert_not page.has_content? "Log In" + + visit logout_path(:referer => "/traces") + assert page.has_content? "Logout from OpenStreetMap" + + click_button "Logout" + assert page.has_content? "Log In" + assert page.has_content? "Public GPS traces" + end end