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 b7a540f6e12426a8de4477e875c3eed33dcafa24..3496587b49848cadbd8d7f96c40cf70db970f905 100644 (file)
--- 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 d1a94b9f6898659f2af5f476625e3dc818dc52a3..e872712c1c4ed64eaa842db6e565bc486fc62cd2 100644 (file)
--- 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 0944d7809868d5f0d1324df6dee01d3b96d5fd74..04a04158c93d973569c0392edeafd16fb0b06426 100644 (file)
--- 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 89c3a8ac4184706cf71730520faa2d5d6d073425..b83b90acf15d844c890e3acb5c23f31c81cc1b8f 100644 (file)
--- 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!