[ARVADOS] created: 1.1.0-41-g0e65950

Git user git at public.curoverse.com
Wed Oct 18 16:59:57 EDT 2017


        at  0e659500b47d9bd4dc3e4c96167b3e55ac14082f (commit)


commit 0e659500b47d9bd4dc3e4c96167b3e55ac14082f
Author: Tom Clegg <tclegg at veritasgenetics.com>
Date:   Wed Oct 18 16:59:22 2017 -0400

    11453: Refactor token checks. Use base64-looking "/" delimiter.
    
    Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tclegg at veritasgenetics.com>

diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb
index b3718e1..57a187a 100644
--- a/services/api/app/controllers/application_controller.rb
+++ b/services/api/app/controllers/application_controller.rb
@@ -349,7 +349,9 @@ class ApplicationController < ActionController::Base
     if params[:remote_id] && (
          request.path.start_with?('/arvados/v1/groups') ||
          request.path.start_with?('/arvados/v1/users/current'))
-      auth = ApiClientAuthorization.validate(remote_id: params[:remote_id])
+      auth = ApiClientAuthorization.
+             validate(token: Thread.current[:supplied_token],
+                      remote: params[:remote_id])
       if auth && auth.user
         Thread.current[:user] = auth.user
         @read_auths << auth
diff --git a/services/api/app/middlewares/arvados_api_token.rb b/services/api/app/middlewares/arvados_api_token.rb
index 6dcadb0..dace944 100644
--- a/services/api/app/middlewares/arvados_api_token.rb
+++ b/services/api/app/middlewares/arvados_api_token.rb
@@ -15,64 +15,31 @@ class ArvadosApiToken
   end
 
   def call env
-    # First, clean up just in case we have a multithreaded server and thread
-    # local variables are still set from a prior request.  Also useful for
-    # tests that call this code to set up the environment.
-    Thread.current[:supplied_token] = nil
-    Thread.current[:api_client_ip_address] = nil
-    Thread.current[:api_client_authorization] = nil
-    Thread.current[:api_client_uuid] = nil
-    Thread.current[:api_client] = nil
-    Thread.current[:user] = nil
-
     request = Rack::Request.new(env)
     params = request.params
     remote_ip = env["action_dispatch.remote_ip"]
 
     Thread.current[:request_starttime] = Time.now
-    user = nil
-    api_client = nil
-    api_client_auth = nil
-    supplied_token =
+    Thread.current[:supplied_token] =
       params["api_token"] ||
       params["oauth_token"] ||
-      env["HTTP_AUTHORIZATION"].andand.match(/(OAuth2|Bearer) ([-,a-zA-Z0-9]+)/).andand[2]
-    if supplied_token
-      case supplied_token[0..2]
-      when 'v2,'
-        _, uuid, secret = supplied_token.split(',')
-        auth = ApiClientAuthorization.
-               includes(:api_client, :user).
-               where('uuid=? and api_token=? and (expires_at is null or expires_at > CURRENT_TIMESTAMP)',
-                     uuid, secret).
-               first
-      else
-        auth = ApiClientAuthorization.
-               includes(:api_client, :user).
-               where('api_token=? and (expires_at is null or expires_at > CURRENT_TIMESTAMP)',
-                     supplied_token).
-               first
-      end
-      if auth.andand.user
-        user = auth.user
-        api_client = auth.api_client
-      else
-        # Token is invalid, or belongs to a non-existent (deleted?) user.
-        auth = nil
-      end
-    end
-    Thread.current[:supplied_token] = supplied_token
-    Thread.current[:api_client_ip_address] = remote_ip
-    Thread.current[:api_client_authorization] = auth
-    Thread.current[:api_client_uuid] = api_client.andand.uuid
-    Thread.current[:api_client] = api_client
-    Thread.current[:user] = user
+      env["HTTP_AUTHORIZATION"].andand.
+        match(/(OAuth2|Bearer) ([-\/a-zA-Z0-9]+)/).andand[2]
+
+    auth = ApiClientAuthorization.
+           validate(token: Thread.current[:supplied_token], remote: false)
     if auth
       auth.last_used_at = Time.now
       auth.last_used_by_ip_address = remote_ip.to_s
       auth.save validate: false
     end
 
+    Thread.current[:api_client_ip_address] = remote_ip
+    Thread.current[:api_client_authorization] = auth
+    Thread.current[:api_client_uuid] = auth.andand.api_client.andand.uuid
+    Thread.current[:api_client] = auth.andand.api_client
+    Thread.current[:user] = auth.andand.user
+
     @app.call env if @app
   end
 end
diff --git a/services/api/app/models/api_client_authorization.rb b/services/api/app/models/api_client_authorization.rb
index 3e90c0e..eb9a8dc 100644
--- a/services/api/app/models/api_client_authorization.rb
+++ b/services/api/app/models/api_client_authorization.rb
@@ -82,22 +82,34 @@ class ApiClientAuthorization < ArvadosModel
     ["#{table_name}.id desc"]
   end
 
-  def self.validate(remote_id:)
-    token = Thread.current[:supplied_token]
+  def self.validate(token:, remote:)
     return nil if !token
-    version, uuid, secret = token.split(',')
-    return nil if version != 'v2'
-    auth = ApiClientAuthorization.
-           includes(:user).
-           where('uuid=? and (expires_at is null or expires_at > CURRENT_TIMESTAMP)', uuid).
-           first
-    if auth && secret == OpenSSL::HMAC.hexdigest('sha1', auth.api_token, remote_id)
-      return auth
+    remote ||= Rails.configuration.uuid_prefix
+
+    case token[0..2]
+    when 'v2/'
+      _, uuid, secret = token.split('/')
+      auth = ApiClientAuthorization.
+             includes(:user, :api_client).
+             where('uuid=? and (expires_at is null or expires_at > CURRENT_TIMESTAMP)', uuid).
+             first
+      if auth && auth.user &&
+         (secret == auth.api_token ||
+          secret == OpenSSL::HMAC.hexdigest('sha1', auth.api_token, remote))
+        return auth
+      end
     else
-      return nil
+      auth = ApiClientAuthorization.
+             includes(:user, :api_client).
+             where('api_token=? and (expires_at is null or expires_at > CURRENT_TIMESTAMP)', token).
+             first
+      if auth && auth.user
+        return auth
+      end
     end
+    return nil
   end
-    
+
   protected
 
   def permission_to_create
diff --git a/services/api/test/functional/arvados/v1/users_controller_test.rb b/services/api/test/functional/arvados/v1/users_controller_test.rb
index 64b6234..646b96c 100644
--- a/services/api/test/functional/arvados/v1/users_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/users_controller_test.rb
@@ -878,7 +878,6 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase
                                "HTTP_AUTHORIZATION" => "Bearer #{salted_token}")
       get :current, {remote_id: 'zbbbb'}
       if token_valid_for == 'zbbbb'
-        STDERR.puts json_response.inspect
         assert_equal(users(:active).uuid, json_response['uuid'])
         assert_response 200
       else
diff --git a/services/api/test/test_helper.rb b/services/api/test/test_helper.rb
index 639b53b..e1f7542 100644
--- a/services/api/test/test_helper.rb
+++ b/services/api/test/test_helper.rb
@@ -132,7 +132,7 @@ class ActiveSupport::TestCase
     uuid = auth.uuid
     token = auth.api_token
     hmac = OpenSSL::HMAC.hexdigest('sha1', token, remote_id)
-    return "v2,#{uuid},#{hmac}"
+    return "v2/#{uuid}/#{hmac}"
   end
 
   def self.skip_slow_tests?

commit 01f52958d899c62b0b1f9a39fda960136835850c
Author: Tom Clegg <tclegg at veritasgenetics.com>
Date:   Mon Oct 16 23:24:45 2017 -0400

    11453: Accept salted tokens at /users/current and /groups.
    
    Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tclegg at veritasgenetics.com>

diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb
index d09283d..b3718e1 100644
--- a/services/api/app/controllers/application_controller.rb
+++ b/services/api/app/controllers/application_controller.rb
@@ -344,7 +344,19 @@ class ApplicationController < ActionController::Base
         .all
     end
     @read_auths.select! { |auth| auth.scopes_allow_request? request }
-    @read_users = @read_auths.map { |auth| auth.user }.uniq
+
+    # Use a salted token as a reader token for /groups/ and /users/current
+    if params[:remote_id] && (
+         request.path.start_with?('/arvados/v1/groups') ||
+         request.path.start_with?('/arvados/v1/users/current'))
+      auth = ApiClientAuthorization.validate(remote_id: params[:remote_id])
+      if auth && auth.user
+        Thread.current[:user] = auth.user
+        @read_auths << auth
+      end
+    end
+
+    @read_users = @read_auths.map(&:user).uniq
   end
 
   def require_login
diff --git a/services/api/app/middlewares/arvados_api_token.rb b/services/api/app/middlewares/arvados_api_token.rb
index b495290..6dcadb0 100644
--- a/services/api/app/middlewares/arvados_api_token.rb
+++ b/services/api/app/middlewares/arvados_api_token.rb
@@ -18,6 +18,7 @@ class ArvadosApiToken
     # First, clean up just in case we have a multithreaded server and thread
     # local variables are still set from a prior request.  Also useful for
     # tests that call this code to set up the environment.
+    Thread.current[:supplied_token] = nil
     Thread.current[:api_client_ip_address] = nil
     Thread.current[:api_client_authorization] = nil
     Thread.current[:api_client_uuid] = nil
@@ -35,29 +36,41 @@ class ArvadosApiToken
     supplied_token =
       params["api_token"] ||
       params["oauth_token"] ||
-      env["HTTP_AUTHORIZATION"].andand.match(/OAuth2 ([a-zA-Z0-9]+)/).andand[1]
+      env["HTTP_AUTHORIZATION"].andand.match(/(OAuth2|Bearer) ([-,a-zA-Z0-9]+)/).andand[2]
     if supplied_token
-      api_client_auth = ApiClientAuthorization.
-        includes(:api_client, :user).
-        where('api_token=? and (expires_at is null or expires_at > CURRENT_TIMESTAMP)', supplied_token).
-        first
-      if api_client_auth.andand.user
-        user = api_client_auth.user
-        api_client = api_client_auth.api_client
+      case supplied_token[0..2]
+      when 'v2,'
+        _, uuid, secret = supplied_token.split(',')
+        auth = ApiClientAuthorization.
+               includes(:api_client, :user).
+               where('uuid=? and api_token=? and (expires_at is null or expires_at > CURRENT_TIMESTAMP)',
+                     uuid, secret).
+               first
       else
-        # Token seems valid, but points to a non-existent (deleted?) user.
-        api_client_auth = nil
+        auth = ApiClientAuthorization.
+               includes(:api_client, :user).
+               where('api_token=? and (expires_at is null or expires_at > CURRENT_TIMESTAMP)',
+                     supplied_token).
+               first
+      end
+      if auth.andand.user
+        user = auth.user
+        api_client = auth.api_client
+      else
+        # Token is invalid, or belongs to a non-existent (deleted?) user.
+        auth = nil
       end
     end
+    Thread.current[:supplied_token] = supplied_token
     Thread.current[:api_client_ip_address] = remote_ip
-    Thread.current[:api_client_authorization] = api_client_auth
+    Thread.current[:api_client_authorization] = auth
     Thread.current[:api_client_uuid] = api_client.andand.uuid
     Thread.current[:api_client] = api_client
     Thread.current[:user] = user
-    if api_client_auth
-      api_client_auth.last_used_at = Time.now
-      api_client_auth.last_used_by_ip_address = remote_ip.to_s
-      api_client_auth.save validate: false
+    if auth
+      auth.last_used_at = Time.now
+      auth.last_used_by_ip_address = remote_ip.to_s
+      auth.save validate: false
     end
 
     @app.call env if @app
diff --git a/services/api/app/models/api_client_authorization.rb b/services/api/app/models/api_client_authorization.rb
index 10c02cc..3e90c0e 100644
--- a/services/api/app/models/api_client_authorization.rb
+++ b/services/api/app/models/api_client_authorization.rb
@@ -82,6 +82,22 @@ class ApiClientAuthorization < ArvadosModel
     ["#{table_name}.id desc"]
   end
 
+  def self.validate(remote_id:)
+    token = Thread.current[:supplied_token]
+    return nil if !token
+    version, uuid, secret = token.split(',')
+    return nil if version != 'v2'
+    auth = ApiClientAuthorization.
+           includes(:user).
+           where('uuid=? and (expires_at is null or expires_at > CURRENT_TIMESTAMP)', uuid).
+           first
+    if auth && secret == OpenSSL::HMAC.hexdigest('sha1', auth.api_token, remote_id)
+      return auth
+    else
+      return nil
+    end
+  end
+    
   protected
 
   def permission_to_create
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 ddc40a4..0fa8430 100644
--- a/services/api/test/functional/arvados/v1/groups_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/groups_controller_test.rb
@@ -706,4 +706,15 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
     end
 
   end
+
+  test "list readable groups with salted token" do
+    salted_token = salt_token(fixture: :active, remote_id: 'zbbbb')
+    ArvadosApiToken.new.call("rack.input" => "",
+                             "HTTP_AUTHORIZATION" => "Bearer #{salted_token}")
+    get :index, {remote_id: '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/functional/arvados/v1/users_controller_test.rb b/services/api/test/functional/arvados/v1/users_controller_test.rb
index b75479f..64b6234 100644
--- a/services/api/test/functional/arvados/v1/users_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/users_controller_test.rb
@@ -870,4 +870,20 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase
     }
     return return_obj
   end
+
+  ['zbbbb', 'z0000'].each do |token_valid_for|
+    test "validate #{token_valid_for}-salted token for remote cluster zbbbb" do
+      salted_token = salt_token(fixture: :active, remote_id: token_valid_for)
+      ArvadosApiToken.new.call("rack.input" => "",
+                               "HTTP_AUTHORIZATION" => "Bearer #{salted_token}")
+      get :current, {remote_id: 'zbbbb'}
+      if token_valid_for == 'zbbbb'
+        STDERR.puts json_response.inspect
+        assert_equal(users(:active).uuid, json_response['uuid'])
+        assert_response 200
+      else
+        assert_response 401
+      end
+    end
+  end
 end
diff --git a/services/api/test/test_helper.rb b/services/api/test/test_helper.rb
index d874bc4..639b53b 100644
--- a/services/api/test/test_helper.rb
+++ b/services/api/test/test_helper.rb
@@ -127,6 +127,14 @@ class ActiveSupport::TestCase
                              "HTTP_AUTHORIZATION" => "OAuth2 #{t}")
   end
 
+  def salt_token(fixture:, remote_id:)
+    auth = api_client_authorizations(fixture)
+    uuid = auth.uuid
+    token = auth.api_token
+    hmac = OpenSSL::HMAC.hexdigest('sha1', token, remote_id)
+    return "v2,#{uuid},#{hmac}"
+  end
+
   def self.skip_slow_tests?
     !(ENV['RAILS_TEST_SHORT'] || '').empty?
   end

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


hooks/post-receive
-- 




More information about the arvados-commits mailing list