From 8086f73aca674d7533e88bdd3850042553487d2b Mon Sep 17 00:00:00 2001 From: Brett Smith Date: Tue, 22 Apr 2014 17:45:46 -0400 Subject: [PATCH] api: Introduce path-based API token scopes. Refs #1904, #2662 for background discussion. --- .../app/controllers/application_controller.rb | 11 ++--- .../arvados/v1/keep_disks_controller.rb | 2 +- .../arvados/v1/nodes_controller.rb | 2 +- .../arvados/v1/schema_controller.rb | 6 +-- .../api/app/controllers/static_controller.rb | 2 +- .../controllers/user_sessions_controller.rb | 2 +- services/api/lib/current_api_client.rb | 19 ++++---- .../fixtures/api_client_authorizations.yml | 22 +++++++++ .../api_client_authorizations_scopes_test.rb | 46 ++++++++++++++++++- 9 files changed, 89 insertions(+), 23 deletions(-) diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb index 0f144d8241..9674bb784c 100644 --- a/services/api/app/controllers/application_controller.rb +++ b/services/api/app/controllers/application_controller.rb @@ -7,7 +7,7 @@ class ApplicationController < ActionController::Base around_filter :thread_with_auth_info, :except => [:render_error, :render_not_found] before_filter :remote_ip - before_filter :require_auth_scope_all, :except => :render_not_found + before_filter :require_auth_scope, :except => :render_not_found before_filter :catch_redirect_hint before_filter :find_object_by_uuid, :except => [:index, :create, @@ -406,12 +406,9 @@ class ApplicationController < ActionController::Base end end - def require_auth_scope_all - require_login and require_auth_scope(['all']) - end - - def require_auth_scope(ok_scopes) - unless current_api_client_auth_has_scope(ok_scopes) + def require_auth_scope + return false unless require_login + unless current_api_client_auth_has_scope("#{request.method} #{request.path}") render :json => { errors: ['Forbidden'] }.to_json, status: 403 end end diff --git a/services/api/app/controllers/arvados/v1/keep_disks_controller.rb b/services/api/app/controllers/arvados/v1/keep_disks_controller.rb index 3d9191641e..47018d4b10 100644 --- a/services/api/app/controllers/arvados/v1/keep_disks_controller.rb +++ b/services/api/app/controllers/arvados/v1/keep_disks_controller.rb @@ -1,5 +1,5 @@ class Arvados::V1::KeepDisksController < ApplicationController - skip_before_filter :require_auth_scope_all, :only => :ping + skip_before_filter :require_auth_scope, :only => :ping def self._ping_requires_parameters { diff --git a/services/api/app/controllers/arvados/v1/nodes_controller.rb b/services/api/app/controllers/arvados/v1/nodes_controller.rb index eda8b0748c..d7a477d337 100644 --- a/services/api/app/controllers/arvados/v1/nodes_controller.rb +++ b/services/api/app/controllers/arvados/v1/nodes_controller.rb @@ -1,5 +1,5 @@ class Arvados::V1::NodesController < ApplicationController - skip_before_filter :require_auth_scope_all, :only => :ping + skip_before_filter :require_auth_scope, :only => :ping skip_before_filter :find_object_by_uuid, :only => :ping skip_before_filter :render_404_if_no_object, :only => :ping diff --git a/services/api/app/controllers/arvados/v1/schema_controller.rb b/services/api/app/controllers/arvados/v1/schema_controller.rb index 1cc84960de..625519e89c 100644 --- a/services/api/app/controllers/arvados/v1/schema_controller.rb +++ b/services/api/app/controllers/arvados/v1/schema_controller.rb @@ -2,7 +2,7 @@ class Arvados::V1::SchemaController < ApplicationController skip_before_filter :find_objects_for_index skip_before_filter :find_object_by_uuid skip_before_filter :render_404_if_no_object - skip_before_filter :require_auth_scope_all + skip_before_filter :require_auth_scope def index expires_in 24.hours, public: true @@ -69,7 +69,7 @@ class Arvados::V1::SchemaController < ApplicationController schemas: {}, resources: {} } - + ActiveRecord::Base.descendants.reject(&:abstract_class?).each do |k| begin ctl_class = "Arvados::V1::#{k.to_s.pluralize}Controller".constantize @@ -175,7 +175,7 @@ class Arvados::V1::SchemaController < ApplicationController description: %|List #{k.to_s.pluralize}. - The list method returns a + The list method returns a resource list of matching #{k.to_s.pluralize}. For example: diff --git a/services/api/app/controllers/static_controller.rb b/services/api/app/controllers/static_controller.rb index fda088095e..c71b85052b 100644 --- a/services/api/app/controllers/static_controller.rb +++ b/services/api/app/controllers/static_controller.rb @@ -3,7 +3,7 @@ class StaticController < ApplicationController skip_before_filter :find_object_by_uuid skip_before_filter :render_404_if_no_object - skip_before_filter :require_auth_scope_all, :only => [ :home, :login_failure ] + skip_before_filter :require_auth_scope, :only => [ :home, :login_failure ] def home if Rails.configuration.respond_to? :workbench_address diff --git a/services/api/app/controllers/user_sessions_controller.rb b/services/api/app/controllers/user_sessions_controller.rb index a7391bd732..3d4b05af4a 100644 --- a/services/api/app/controllers/user_sessions_controller.rb +++ b/services/api/app/controllers/user_sessions_controller.rb @@ -1,5 +1,5 @@ class UserSessionsController < ApplicationController - before_filter :require_auth_scope_all, :only => [ :destroy ] + before_filter :require_auth_scope, :only => [ :destroy ] skip_before_filter :find_object_by_uuid skip_before_filter :render_404_if_no_object diff --git a/services/api/lib/current_api_client.rb b/services/api/lib/current_api_client.rb index bbba4dc6dc..0803d5464d 100644 --- a/services/api/lib/current_api_client.rb +++ b/services/api/lib/current_api_client.rb @@ -29,14 +29,17 @@ module CurrentApiClient Thread.current[:api_client_ip_address] end - # Does the current API client authorization include any of ok_scopes? - def current_api_client_auth_has_scope(ok_scopes) - auth_scopes = current_api_client_authorization.andand.scopes || [] - unless auth_scopes.index('all') or (auth_scopes & ok_scopes).any? - logger.warn "Insufficient auth scope: need #{ok_scopes}, #{current_api_client_authorization.inspect} has #{auth_scopes}" - return false - end - true + # Is the current API client authorization scoped for the request? + def current_api_client_auth_has_scope(req_s) + (current_api_client_authorization.andand.scopes || []).select { |scope| + if scope == 'all' + true + elsif scope.end_with? '/' + req_s.start_with? scope + else + req_s == scope + end + }.any? end def system_user_uuid diff --git a/services/api/test/fixtures/api_client_authorizations.yml b/services/api/test/fixtures/api_client_authorizations.yml index 5a715e3a33..77e5048660 100644 --- a/services/api/test/fixtures/api_client_authorizations.yml +++ b/services/api/test/fixtures/api_client_authorizations.yml @@ -51,6 +51,28 @@ admin_noscope: expires_at: 2038-01-01 00:00:00 scopes: [] +active_userlist: + api_client: untrusted + user: active + api_token: activeuserlistabcdefghijklmnopqrstuvwxyz1234568900 + expires_at: 2038-01-01 00:00:00 + scopes: ["GET /arvados/v1/users"] + +active_specimens: + api_client: untrusted + user: active + api_token: activespecimensabcdefghijklmnopqrstuvwxyz123456890 + expires_at: 2038-01-01 00:00:00 + scopes: ["GET /arvados/v1/specimens/"] + +active_apitokens: + api_client: trusted_workbench + user: active + api_token: activeapitokensabcdefghijklmnopqrstuvwxyz123456789 + expires_at: 2038-01-01 00:00:00 + scopes: ["GET /arvados/v1/api_client_authorizations", + "POST /arvados/v1/api_client_authorizations"] + spectator: api_client: untrusted user: spectator diff --git a/services/api/test/integration/api_client_authorizations_scopes_test.rb b/services/api/test/integration/api_client_authorizations_scopes_test.rb index 7269d38d60..ba91670822 100644 --- a/services/api/test/integration/api_client_authorizations_scopes_test.rb +++ b/services/api/test/integration/api_client_authorizations_scopes_test.rb @@ -20,7 +20,6 @@ class Arvados::V1::ApiTokensScopeTest < ActionController::IntegrationTest end def request_with_auth(method, path, params={}) - https! send(method, path, @token.merge(params)) end @@ -32,6 +31,51 @@ class Arvados::V1::ApiTokensScopeTest < ActionController::IntegrationTest request_with_auth(:post_via_redirect, *args) end + test "user list token can only list users" do + auth_with :active_userlist + get_with_auth v1_url('users') + assert_response :success + get_with_auth v1_url('users', '') # Add trailing slash. + assert_response :success + get_with_auth v1_url('users', 'current') + assert_response 403 + get_with_auth v1_url('virtual_machines') + assert_response 403 + end + + test "specimens token can see exactly owned specimens" do + auth_with :active_specimens + get_with_auth v1_url('specimens') + assert_response 403 + get_with_auth v1_url('specimens', specimens(:owned_by_active_user).uuid) + assert_response :success + get_with_auth v1_url('specimens', specimens(:owned_by_spectator).uuid) + assert_includes(403..404, @response.status) + end + + test "token with multiple scopes can use them all" do + def get_token_count + get_with_auth v1_url('api_client_authorizations') + assert_response :success + token_count = JSON.parse(@response.body)['items_available'] + assert_not_nil(token_count, "could not find token count") + token_count + end + auth_with :active_apitokens + # Test the GET scope. + token_count = get_token_count + # Test the POST scope. + post_with_auth(v1_url('api_client_authorizations'), + api_client_authorization: {user_id: users(:active).id}) + assert_response :success + assert_equal(token_count + 1, get_token_count, + "token count suggests POST was not accepted") + # Test other requests are denied. + get_with_auth v1_url('api_client_authorizations', + api_client_authorizations(:active_apitokens).uuid) + assert_response 403 + end + test "token without scope has no access" do # Logs are good for this test, because logs have relatively # few access controls enforced at the model level. -- 2.30.2