[ARVADOS] updated: 1.3.0-186-g0d1836a8d

[Git user]

Summary of changes:
 .../app/controllers/user_sessions_controller.rb    | 24 +++++++++-------------
 .../api/test/integration/user_sessions_test.rb     | 15 +++++++-------
 2 files changed, 18 insertions(+), 21 deletions(-)

       via  0d1836a8d4d5a0c0802881c2878a35f611e09e1f (commit)
      from  4bb449eb541e7bc22dfb09c31451d2258f189495 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.


commit 0d1836a8d4d5a0c0802881c2878a35f611e09e1f
Author: Lucas Di Pentima <ldipentima at veritasgenetics.com>
Date:   Mon Jan 28 17:12:33 2019 -0300

    14718: Changes the way remote param is packed into return_to.
    
    Also:
    * Escapes the remote param for security reasons.
    * Adds comments.
    * Updates tests.
    
    Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <ldipentima at veritasgenetics.com>

diff --git a/services/api/app/controllers/user_sessions_controller.rb b/services/api/app/controllers/user_sessions_controller.rb
index cb150bd94..7f7b47fd9 100644
--- a/services/api/app/controllers/user_sessions_controller.rb
+++ b/services/api/app/controllers/user_sessions_controller.rb
@@ -95,11 +95,12 @@ class UserSessionsController < ApplicationController
 
     @redirect_to = root_path
     if params.has_key?(:return_to)
-      rt = params[:return_to]
-      # Extracts query params as {param1 => [value1], param2 => [value2], ...}
-      p = rt.index('?').nil? ? {} : CGI::parse(rt[rt.index('?')+1..-1])
-      remote = p["remote"] && p["remote"][0]
-      return send_api_token_to(params[:return_to], user, remote)
+      # return_to param's format is 'remote,return_to_url'. This comes from login()
+      # encoding the remote=zbbbb parameter passed by a client asking for a salted
+      # token.
+      remote, return_to_url = params[:return_to].split(',', 2)
+      remote = nil if remote == ''
+      return send_api_token_to(return_to_url, user, remote)
     end
     redirect_to @redirect_to
   end
@@ -135,16 +136,11 @@ class UserSessionsController < ApplicationController
     p = []
     p << "auth_provider=#{CGI.escape(params[:auth_provider])}" if params[:auth_provider]
     if params[:return_to]
-      remote_param = ''
-      if params[:remote]
-        # Encode remote param inside return_to, so that we'll get it on the
-        # callback after login
-        remote_param += if params[:return_to].include? '?' then '&' else '?' end
-        remote_param += "remote=#{params[:remote]}"
-      end
-      p << "return_to=#{CGI.escape(params[:return_to]+remote_param)}"
+      # Encode remote param inside callback's return_to, so that we'll get it on
+      # create() after login.
+      remote_param = params[:remote].nil? ? '' : params[:remote]
+      p << "return_to=#{CGI.escape(remote_param)},#{CGI.escape(params[:return_to])}"
     end
-
     redirect_to "/auth/joshid?#{p.join('&')}"
   end
 
diff --git a/services/api/test/integration/user_sessions_test.rb b/services/api/test/integration/user_sessions_test.rb
index f221e58dd..7d1ed1873 100644
--- a/services/api/test/integration/user_sessions_test.rb
+++ b/services/api/test/integration/user_sessions_test.rb
@@ -5,9 +5,11 @@
 require 'test_helper'
 
 class UserSessionsApiTest < ActionDispatch::IntegrationTest
-  def client_url(query_string: nil)
-    url = 'https://wb.example.com'
-    url += '?'+ query_string unless query_string.nil?
+  # remote prefix & return url packed into the return_to param passed around
+  # between API and SSO provider.
+  def client_url(remote: nil)
+    url = ',https://wb.example.com'
+    url = "#{remote}#{url}" unless remote.nil?
     url
   end
 
@@ -25,9 +27,8 @@ class UserSessionsApiTest < ActionDispatch::IntegrationTest
     mock['info']['email'] = email unless email.nil?
     mock['info']['username'] = username unless username.nil?
     mock['info']['identity_url'] = identity_url unless identity_url.nil?
-    return_to = remote.nil? ? client_url : client_url(query_string: 'remote='+remote)
     post('/auth/josh_id/callback',
-         {return_to: return_to},
+         {return_to: client_url(remote: remote)},
          {'omniauth.auth' => mock})
     assert_response :redirect, 'Did not redirect to client with token'
   end
@@ -64,9 +65,9 @@ class UserSessionsApiTest < ActionDispatch::IntegrationTest
 
   test 'create new user during omniauth callback' do
     mock_auth_with(email: 'edward at example.com')
-    assert_equal(0, @response.redirect_url.index(client_url),
+    assert_equal(0, @response.redirect_url.index(client_url.split(',', 2)[1]),
                  'Redirected to wrong address after succesful login: was ' +
-                 @response.redirect_url + ', expected ' + client_url + '[...]')
+                 @response.redirect_url + ', expected ' + client_url.split(',', 2)[1] + '[...]')
     assert_not_nil(@response.redirect_url.index('api_token='),
                    'Expected api_token in query string of redirect url ' +
                    @response.redirect_url)

-----------------------------------------------------------------------


hooks/post-receive
--