From: Andy Allan <git@gravitystorm.co.uk>
Date: Wed, 8 Jan 2020 13:01:17 +0000 (+0100)
Subject: Use a post link to logout
X-Git-Tag: live~2937^2~1
X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/8a774e75193fcb66eea9e76b60f10623e1e0e682

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
---

diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index a61a10d94..514b3f8ee 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -269,7 +269,7 @@ class UsersController < ApplicationController
   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 6df8f02da..3963c211e 100644
--- a/app/views/layouts/_header.html.erb
+++ b/app/views/layouts/_header.html.erb
@@ -102,7 +102,7 @@
             <%= yield :greeting %>
           </li>
           <li>
-            <%= 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" %>
           </li>
         </ul>
       </div>
diff --git a/app/views/users/logout.html.erb b/app/views/users/logout.html.erb
index 273c7e1b9..5d8e2de49 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 feca92df5..4417d353f 100644
--- a/test/controllers/users_controller_test.rb
+++ b/test/controllers/users_controller_test.rb
@@ -344,27 +344,13 @@ class UsersControllerTest < ActionController::TestCase
   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 @@ class UsersControllerTest < ActionController::TestCase
 
     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 000000000..a2e145fcc
--- /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