From: Tom Hughes <tom@compton.nu>
Date: Mon, 25 Feb 2019 11:44:24 +0000 (+0000)
Subject: Avoid CSP issues with OpenID login
X-Git-Tag: live~2666
X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/d2ff1491b4b6f8fa3b64ab88414d0dbf01cc520e?ds=sidebyside

Avoid CSP issues with OpenID login

To avoid Chrom getting upset about sending form data to sites
that our policy doesn't allow, even when it isn't, use Javascript
to jump straight to Omniauth as the direct OpenID based login
buttons were already doing.

Fixes #1909
---

diff --git a/app/assets/javascripts/login.js b/app/assets/javascripts/login.js
index b7a540f6e..3496587b4 100644
--- a/app/assets/javascripts/login.js
+++ b/app/assets/javascripts/login.js
@@ -22,4 +22,18 @@ $(document).ready(function() {
   // Hide OpenID field for now
   $("#login_openid_url").hide();
   $("#login_openid_submit").hide();
+
+  // Handle OpenID submission by redirecting to omniauth
+  $("#openid_login_form").submit(function() {
+    var action = $(this).prop("action"),
+        openid_url = $(this).find("#openid_url").val(),
+        referer = $(this).find("#openid_referer").val(),
+        args = {};
+    args.openid_url = openid_url;
+    if (referer) {
+      args.referer = referer;
+    }
+    window.location = action + "?" + querystring.stringify(args);
+    return false;
+  });
 });
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index d1a94b9f6..e872712c1 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -264,9 +264,6 @@ class UsersController < ApplicationController
     if params[:username].present? && params[:password].present?
       session[:remember_me] ||= params[:remember_me]
       password_authentication(params[:username], params[:password])
-    elsif params[:openid_url].present?
-      session[:remember_me] ||= params[:remember_me_openid]
-      redirect_to auth_url("openid", params[:openid_url], params[:referer])
     end
   end
 
diff --git a/app/views/users/login.html.erb b/app/views/users/login.html.erb
index 0944d7809..04a04158c 100644
--- a/app/views/users/login.html.erb
+++ b/app/views/users/login.html.erb
@@ -40,6 +40,13 @@
         <%= submit_tag t('.login_button'), :tabindex => 4 %>
       </fieldset>
 
+    </div>
+
+  <% end %>
+
+  <%= form_tag(auth_path(:provider => "openid"), { :id => "openid_login_form" }) do %>
+    <div id="loginForm" class="standard-form">
+
       <fieldset class='form-divider'>
 
         <p class='standard-label'><%= t '.with external' %></p>
@@ -68,15 +75,11 @@
 
         <div id='login_openid_url' class='form-row'>
           <label for='openid_url' class="standard-label"><%= raw t '.openid', :logo => openid_logo %></label>
+          <%= hidden_field_tag("openid_referer", params[:referer]) if params[:referer] %>
           <%= text_field_tag("openid_url", "", { :tabindex => 3, :class => "openid_url" }) %>
           <span class="minorNote">(<a href="<%= t 'users.account.openid.link' %>" target="_new"><%= t 'users.account.openid.link text' %></a>)</span>
         </div>
 
-        <div class='form-row'>
-          <%= check_box_tag "remember_me_openid", "yes", false, :tabindex => 5 %>
-          <label class="standard-label" for="remember_me_openid"><%= t '.remember' %></label>
-        </div>
-
         <%= submit_tag t('.login_button'), :tabindex => 6, :id => "login_openid_submit" %>
 
       </fieldset>
diff --git a/test/integration/user_login_test.rb b/test/integration/user_login_test.rb
index 89c3a8ac4..b83b90acf 100644
--- a/test/integration/user_login_test.rb
+++ b/test/integration/user_login_test.rb
@@ -359,10 +359,7 @@ class UserLoginTest < ActionDispatch::IntegrationTest
     follow_redirect!
     assert_response :success
     assert_template "users/login"
-    post "/login", :params => { :openid_url => "http://localhost:1123/john.doe", :referer => "/history" }
-    assert_response :redirect
-    assert_redirected_to auth_path(:provider => "openid", :openid_url => "http://localhost:1123/john.doe", :origin => "/login?referer=%2Fhistory", :referer => "/history")
-    follow_redirect!
+    get auth_path(:provider => "openid", :openid_url => "http://localhost:1123/john.doe", :origin => "/login?referer=%2Fhistory", :referer => "/history")
     assert_response :redirect
     assert_redirected_to auth_success_path(:provider => "openid", :openid_url => "http://localhost:1123/john.doe", :origin => "/login?referer=%2Fhistory", :referer => "/history")
     follow_redirect!
@@ -373,31 +370,6 @@ class UserLoginTest < ActionDispatch::IntegrationTest
     assert_select "span.username", user.display_name
   end
 
-  def test_login_openid_remember_me
-    user = create(:user, :auth_provider => "openid", :auth_uid => "http://example.com/john.doe")
-    OmniAuth.config.add_mock(:openid, :uid => user.auth_uid)
-
-    get "/login", :params => { :referer => "/history" }
-    assert_response :redirect
-    assert_redirected_to :controller => :users, :action => :login, :cookie_test => true, :referer => "/history"
-    follow_redirect!
-    assert_response :success
-    assert_template "users/login"
-    post "/login", :params => { :openid_url => user.auth_uid, :remember_me_openid => true, :referer => "/history" }
-    assert_response :redirect
-    assert_redirected_to auth_path(:provider => "openid", :openid_url => user.auth_uid, :origin => "/login?referer=%2Fhistory", :referer => "/history")
-    follow_redirect!
-    assert_response :redirect
-    assert_redirected_to auth_success_path(:provider => "openid", :openid_url => user.auth_uid, :origin => "/login?referer=%2Fhistory", :referer => "/history")
-    follow_redirect!
-    assert_response :redirect
-    follow_redirect!
-    assert_response :success
-    assert_template "changesets/history"
-    assert_select "span.username", user.display_name
-    assert session.key?(:_remember_for)
-  end
-
   def test_login_openid_connection_failed
     user = create(:user, :auth_provider => "openid", :auth_uid => "http://example.com/john.doe")
     OmniAuth.config.mock_auth[:openid] = :connection_failed
@@ -408,10 +380,7 @@ class UserLoginTest < ActionDispatch::IntegrationTest
     follow_redirect!
     assert_response :success
     assert_template "users/login"
-    post "/login", :params => { :openid_url => user.auth_uid, :referer => "/history" }
-    assert_response :redirect
-    assert_redirected_to auth_path(:provider => "openid", :openid_url => user.auth_uid, :origin => "/login?referer=%2Fhistory", :referer => "/history")
-    follow_redirect!
+    get auth_path(:provider => "openid", :openid_url => user.auth_uid, :origin => "/login?referer=%2Fhistory", :referer => "/history")
     assert_response :redirect
     assert_redirected_to auth_success_path(:provider => "openid", :openid_url => user.auth_uid, :origin => "/login?referer=%2Fhistory", :referer => "/history")
     follow_redirect!
@@ -436,10 +405,7 @@ class UserLoginTest < ActionDispatch::IntegrationTest
     follow_redirect!
     assert_response :success
     assert_template "users/login"
-    post "/login", :params => { :openid_url => user.auth_uid, :referer => "/history" }
-    assert_response :redirect
-    assert_redirected_to auth_path(:provider => "openid", :openid_url => user.auth_uid, :origin => "/login?referer=%2Fhistory", :referer => "/history")
-    follow_redirect!
+    get auth_path(:provider => "openid", :openid_url => user.auth_uid, :origin => "/login?referer=%2Fhistory", :referer => "/history")
     assert_response :redirect
     assert_redirected_to auth_success_path(:provider => "openid", :openid_url => user.auth_uid, :origin => "/login?referer=%2Fhistory", :referer => "/history")
     follow_redirect!
@@ -463,10 +429,7 @@ class UserLoginTest < ActionDispatch::IntegrationTest
     follow_redirect!
     assert_response :success
     assert_template "users/login"
-    post "/login", :params => { :openid_url => "http://localhost:1123/fred.bloggs", :referer => "/history" }
-    assert_response :redirect
-    assert_redirected_to auth_path(:provider => "openid", :openid_url => "http://localhost:1123/fred.bloggs", :origin => "/login?referer=%2Fhistory", :referer => "/history")
-    follow_redirect!
+    get auth_path(:provider => "openid", :openid_url => "http://localhost:1123/fred.bloggs", :origin => "/login?referer=%2Fhistory", :referer => "/history")
     assert_response :redirect
     assert_redirected_to auth_success_path(:provider => "openid", :openid_url => "http://localhost:1123/fred.bloggs", :origin => "/login?referer=%2Fhistory", :referer => "/history")
     follow_redirect!