11453: Fix remote token checks.

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tclegg@veritasgenetics.com>

diff --git a/services/api/app/middlewares/arvados_api_token.rb b/services/api/app/middlewares/arvados_api_token.rb
index de6ba6f97f94aa868e20ac228a93df9387b7088b..4098fd72ca436bdf3ee806b5a0dfb938fe7a5a9b 100644 (file)
--- a/services/api/app/middlewares/arvados_api_token.rb
+++ b/services/api/app/middlewares/arvados_api_token.rb
@@ -23,12 +23,12 @@ class ArvadosApiToken
 
     remote = false
     reader_tokens = nil
-    if params[:remote] && request.get? && (
+    if params["remote"] && request.get? && (
          request.path.start_with?('/arvados/v1/groups') ||
          request.path.start_with?('/arvados/v1/users/current'))
       # Request from a remote API server, asking to validate a salted
       # token.
-      remote = params[:remote]
+      remote = params["remote"]
     elsif request.get? || params["_method"] == 'GET'
       reader_tokens = params["reader_tokens"]
       if reader_tokens.is_a? String
@@ -42,13 +42,12 @@ class ArvadosApiToken
     auth = nil
     [params["api_token"],
      params["oauth_token"],
-     env["HTTP_AUTHORIZATION"].andand.match(/(OAuth2|Bearer) ([a-zA-Z0-9]+)/).andand[2],
+     env["HTTP_AUTHORIZATION"].andand.match(/(OAuth2|Bearer) ([-\/a-zA-Z0-9]+)/).andand[2],
      *reader_tokens,
     ].each do |supplied|
       next if !supplied
       try_auth = ApiClientAuthorization.
-                 validate(token: Thread.current[:supplied_token],
-                          remote: remote)
+                 validate(token: supplied, remote: remote)
       if try_auth.andand.user
         auth = try_auth
         break

diff --git a/services/api/app/models/api_client_authorization.rb b/services/api/app/models/api_client_authorization.rb
index fba999c60a68e922980fe9ebe828f0c7cb4b0cdf..542ab8e88df4f4ff21bc4ed304ff151bc91a8bc1 100644 (file)
--- a/services/api/app/models/api_client_authorization.rb
+++ b/services/api/app/models/api_client_authorization.rb
@@ -131,28 +131,29 @@ class ApiClientAuthorization < ArvadosModel
                            {'remote' => Rails.configuration.uuid_prefix},
                            {'Authorization' => 'Bearer ' + token}))
       rescue => e
-        logger.warn "remote authentication with token #{token.inspect} failed: #{e}"
-        STDERR.puts e.backtrace
+        Rails.logger.warn "remote authentication with token #{token.inspect} failed: #{e}"
         return nil
       end
-      if !remote_user.is_a?(Hash) || !remote_user[:uuid].is_a?(String) || remote_user[:uuid][0..4] != uuid[0..4]
-        logger.warn "remote authentication rejected: remote_user=#{remote_user.inspect}"
+      if !remote_user.is_a?(Hash) || !remote_user['uuid'].is_a?(String) || remote_user['uuid'][0..4] != uuid[0..4]
+        Rails.logger.warn "remote authentication rejected: remote_user=#{remote_user.inspect}"
         return nil
       end
       act_as_system_user do
         # Add/update user and token in our database so we can
         # validate subsequent requests faster.
 
-        user = User.find_or_create_by(uuid: remote_user[:uuid])
+        user = User.find_or_create_by(uuid: remote_user['uuid']) do |user|
+          user.is_admin = false
+        end
 
         updates = {}
         [:first_name, :last_name, :email, :prefs].each do |attr|
-          updates[attr] = remote_user[attr]
+          updates[attr] = remote_user[attr.to_s]
         end
 
         if Rails.configuration.new_users_are_active
           # Update is_active to whatever it is at the remote end
-          updates[:is_active] = remote_user[:is_active]
+          updates[:is_active] = remote_user['is_active']
         elsif !updates[:is_active]
           # Remote user is inactive; our mirror should be, too.
           updates[:is_active] = false
@@ -160,11 +161,11 @@ class ApiClientAuthorization < ArvadosModel
 
         user.update_attributes!(updates)
 
-        auth = ApiClientAuthorization.find_or_create_by(uuid: uuid)
-        auth.user = user
-        auth.api_token = token
-        auth.api_client_id = 0
-        auth.save!
+        auth = ApiClientAuthorization.find_or_create_by(uuid: uuid) do |auth|
+          auth.user = user
+          auth.api_token = token
+          auth.api_client_id = 0
+        end
 
         # Accept this token (and don't reload the user record) for
         # 5 minutes. TODO: Request the actual api_client_auth

diff --git a/services/api/test/functional/arvados/v1/groups_controller_test.rb b/services/api/test/functional/arvados/v1/groups_controller_test.rb
index 6027dcbf82fd54ca6d169d8179b764be7e7f85f9..3442eda2447aa1e75ecc254b3ffcfb2392853a8f 100644 (file)
--- a/services/api/test/functional/arvados/v1/groups_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/groups_controller_test.rb
@@ -704,17 +704,5 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
       assert_response :success
       assert_not_nil Group.readable_by(users(auth)).where(uuid: groups(:trashed_subproject).uuid).first
     end
-
-  end
-
-  test "list readable groups with salted token" do
-    salted_token = salt_token(fixture: :active, remote: 'zbbbb')
-    ArvadosApiToken.new.call("rack.input" => "",
-                             "HTTP_AUTHORIZATION" => "Bearer #{salted_token}")
-    get :index, {remote: 'zbbbb', limit: 10000}
-    assert_response 200
-    group_uuids = json_response['items'].collect { |i| i['uuid'] }
-    assert_includes(group_uuids, 'zzzzz-j7d0g-fffffffffffffff')
-    refute_includes(group_uuids, 'zzzzz-j7d0g-000000000000000')
   end
 end

diff --git a/services/api/test/integration/remote_user_test.rb b/services/api/test/integration/remote_user_test.rb
index 1201d44f26a39b247b4012a6f7052c41e3435376..6e5b9e452ec626ac21f1bede4de9ffca370d20a9 100644 (file)
--- a/services/api/test/integration/remote_user_test.rb
+++ b/services/api/test/integration/remote_user_test.rb
@@ -22,6 +22,10 @@ class RemoteUsersTest < ActionDispatch::IntegrationTest
   # Test cases can override the stub's default response to
   # .../users/current by changing @stub_status and @stub_content.
   setup do
+    clnt = HTTPClient.new
+    clnt.ssl_config.verify_mode = OpenSSL::SSL::VERIFY_NONE
+    HTTPClient.stubs(:new).returns clnt
+
     @controller = Arvados::V1::UsersController.new
     ready = Thread::Queue.new
     srv = WEBrick::HTTPServer.new(
@@ -70,14 +74,14 @@ class RemoteUsersTest < ActionDispatch::IntegrationTest
   end
 
   test 'authenticate with remote token' do
-    get '/arvados/v1/users/current', {}, auth(remote: 'zbbbb')
+    get '/arvados/v1/users/current', {format: 'json'}, auth(remote: 'zbbbb')
     assert_response :success
     assert_equal 'zbbbb-tpzed-000000000000000', json_response['uuid']
     assert_equal false, json_response['is_admin']
   end
 
   test 'authenticate with remote token from misbhehaving remote cluster' do
-    get '/arvados/v1/users/current', {}, auth(remote: 'zbork')
+    get '/arvados/v1/users/current', {format: 'json'}, auth(remote: 'zbork')
     assert_response 401
   end
 
@@ -86,14 +90,14 @@ class RemoteUsersTest < ActionDispatch::IntegrationTest
     @stub_content = {
       error: 'not authorized',
     }
-    get '/arvados/v1/users/current', {}, auth(remote: 'zbbbb')
+    get '/arvados/v1/users/current', {format: 'json'}, auth(remote: 'zbbbb')
     assert_response 401
   end
 
   test 'remote api server is not an api server' do
     @stub_status = 200
     @stub_content = '<html>bad</html>'
-    get '/arvados/v1/users/current', {}, auth(remote: 'zbbbb')
+    get '/arvados/v1/users/current', {format: 'json'}, auth(remote: 'zbbbb')
     assert_response 401
   end
 
@@ -111,4 +115,19 @@ class RemoteUsersTest < ActionDispatch::IntegrationTest
       end
     end
   end
+
+  test "list readable groups with salted token" do
+    salted_token = salt_token(fixture: :active, remote: 'zbbbb')
+    get '/arvados/v1/groups', {
+          format: 'json',
+          remote: 'zbbbb',
+          limit: 10000,
+        }, {
+          "HTTP_AUTHORIZATION" => "Bearer #{salted_token}"
+        }
+    assert_response 200
+    group_uuids = json_response['items'].collect { |i| i['uuid'] }
+    assert_includes(group_uuids, 'zzzzz-j7d0g-fffffffffffffff')
+    refute_includes(group_uuids, 'zzzzz-j7d0g-000000000000000')
+  end
 end