From 80c1ebaa83f346e45346baac584f21878652c350 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Tue, 20 Jun 2017 08:27:45 +0000 Subject: [PATCH] Allow API scope declarations to be applied conditionally. - Scope declarations of the form: allow_access_with_scope :read_user, if: -> (request) { request.get? } will only apply for `GET` requests - Add a negative test to a `POST` endpoint in the `users` API to test this. Also test for this case in the `AccessTokenValidationService` unit tests. --- .../access_token_validation_service.rb | 15 ++++---- lib/api/api_guard.rb | 4 +-- lib/api/helpers.rb | 2 +- spec/requests/api/helpers_spec.rb | 3 +- spec/requests/api/users_spec.rb | 10 ++++++ .../access_token_validation_service_spec.rb | 36 +++++++++++++++---- 6 files changed, 54 insertions(+), 16 deletions(-) diff --git a/app/services/access_token_validation_service.rb b/app/services/access_token_validation_service.rb index f171f8194bd..6d39ad245d2 100644 --- a/app/services/access_token_validation_service.rb +++ b/app/services/access_token_validation_service.rb @@ -5,10 +5,11 @@ class AccessTokenValidationService REVOKED = :revoked INSUFFICIENT_SCOPE = :insufficient_scope - attr_reader :token + attr_reader :token, :request - def initialize(token) + def initialize(token, request) @token = token + @request = request end def validate(scopes: []) @@ -31,11 +32,13 @@ class AccessTokenValidationService if scopes.blank? true else - #scopes = scopes.reject { |scope| scope[:if].presence && !scope[:if].call(request) } - # Check whether the token is allowed access to any of the required scopes. + # Remove any scopes whose `if` condition does not return `true` + scopes = scopes.reject { |scope| scope[:if].presence && !scope[:if].call(request) } - scope_names = scopes.map { |scope| scope[:name].to_s } - Set.new(scope_names).intersection(Set.new(token.scopes)).present? + # Check whether the token is allowed access to any of the required scopes. + passed_scope_names = scopes.map { |scope| scope[:name].to_sym } + token_scope_names = token.scopes.map(&:to_sym) + Set.new(passed_scope_names).intersection(Set.new(token_scope_names)).present? end end end diff --git a/lib/api/api_guard.rb b/lib/api/api_guard.rb index 9a9e32a0242..ceeecbbc00b 100644 --- a/lib/api/api_guard.rb +++ b/lib/api/api_guard.rb @@ -68,7 +68,7 @@ module API access_token = find_access_token return nil unless access_token - case AccessTokenValidationService.new(access_token).validate(scopes: scopes) + case AccessTokenValidationService.new(access_token, request).validate(scopes: scopes) when AccessTokenValidationService::INSUFFICIENT_SCOPE raise InsufficientScopeError.new(scopes) @@ -105,7 +105,7 @@ module API access_token = PersonalAccessToken.active.find_by_token(token_string) return unless access_token - if AccessTokenValidationService.new(access_token).include_any_scope?(scopes) + if AccessTokenValidationService.new(access_token, request).include_any_scope?(scopes) User.find(access_token.user_id) end end diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 3cf04e6df3c..c69e7afea8c 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -340,7 +340,7 @@ module API end def initial_current_user - endpoint_class = options[:for] + endpoint_class = options[:for].presence || ::API::API return @initial_current_user if defined?(@initial_current_user) Gitlab::Auth::UniqueIpsLimiter.limit_user! do diff --git a/spec/requests/api/helpers_spec.rb b/spec/requests/api/helpers_spec.rb index 191c60aba31..87d6f46533e 100644 --- a/spec/requests/api/helpers_spec.rb +++ b/spec/requests/api/helpers_spec.rb @@ -14,6 +14,8 @@ describe API::Helpers do let(:request) { Rack::Request.new(env) } let(:header) { } + before { allow_any_instance_of(self.class).to receive(:options).and_return({}) } + def set_env(user_or_token, identifier) clear_env clear_param @@ -167,7 +169,6 @@ describe API::Helpers do it "returns nil for a token without the appropriate scope" do personal_access_token = create(:personal_access_token, user: user, scopes: ['read_user']) env[API::APIGuard::PRIVATE_TOKEN_HEADER] = personal_access_token.token - allow_access_with_scope('write_user') expect(current_user).to be_nil end diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index c8e22799ba4..982c1a50e3b 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -321,6 +321,16 @@ describe API::Users do .to eq([Gitlab::PathRegex.namespace_format_message]) end + context 'when the requesting token has the "read_user" scope' do + let(:token) { create(:personal_access_token, scopes: ['read_user'], user: admin) } + + it 'returns a "401" response' do + post api("/users", admin, personal_access_token: token), attributes_for(:user, projects_limit: 3) + + expect(response).to have_http_status(401) + end + end + it "is not available for non admin users" do post api("/users", user), attributes_for(:user) expect(response).to have_http_status(403) diff --git a/spec/services/access_token_validation_service_spec.rb b/spec/services/access_token_validation_service_spec.rb index 87f093ee8ce..0023678dc3b 100644 --- a/spec/services/access_token_validation_service_spec.rb +++ b/spec/services/access_token_validation_service_spec.rb @@ -2,40 +2,64 @@ require 'spec_helper' describe AccessTokenValidationService, services: true do describe ".include_any_scope?" do + let(:request) { double("request") } + it "returns true if the required scope is present in the token's scopes" do token = double("token", scopes: [:api, :read_user]) - expect(described_class.new(token).include_any_scope?([:api])).to be(true) + expect(described_class.new(token, request).include_any_scope?([{ name: :api }])).to be(true) end it "returns true if more than one of the required scopes is present in the token's scopes" do token = double("token", scopes: [:api, :read_user, :other_scope]) - expect(described_class.new(token).include_any_scope?([:api, :other_scope])).to be(true) + expect(described_class.new(token, request).include_any_scope?([{ name: :api }, { name: :other_scope }])).to be(true) end it "returns true if the list of required scopes is an exact match for the token's scopes" do token = double("token", scopes: [:api, :read_user, :other_scope]) - expect(described_class.new(token).include_any_scope?([:api, :read_user, :other_scope])).to be(true) + expect(described_class.new(token, request).include_any_scope?([{ name: :api }, { name: :read_user }, { name: :other_scope }])).to be(true) end it "returns true if the list of required scopes contains all of the token's scopes, in addition to others" do token = double("token", scopes: [:api, :read_user]) - expect(described_class.new(token).include_any_scope?([:api, :read_user, :other_scope])).to be(true) + expect(described_class.new(token, request).include_any_scope?([{ name: :api }, { name: :read_user }, { name: :other_scope }])).to be(true) end it 'returns true if the list of required scopes is blank' do token = double("token", scopes: []) - expect(described_class.new(token).include_any_scope?([])).to be(true) + expect(described_class.new(token, request).include_any_scope?([])).to be(true) end it "returns false if there are no scopes in common between the required scopes and the token scopes" do token = double("token", scopes: [:api, :read_user]) - expect(described_class.new(token).include_any_scope?([:other_scope])).to be(false) + expect(described_class.new(token, request).include_any_scope?([{ name: :other_scope }])).to be(false) + end + + context "conditions" do + context "if" do + it "ignores any scopes whose `if` condition returns false" do + token = double("token", scopes: [:api, :read_user]) + + expect(described_class.new(token, request).include_any_scope?([{ name: :api, if: ->(_) { false } }])).to be(false) + end + + it "does not ignore scopes whose `if` condition is not set" do + token = double("token", scopes: [:api, :read_user]) + + expect(described_class.new(token, request).include_any_scope?([{ name: :api, if: ->(_) { false } }, { name: :read_user }])).to be(true) + end + + it "does not ignore scopes whose `if` condition returns true" do + token = double("token", scopes: [:api, :read_user]) + + expect(described_class.new(token, request).include_any_scope?([{ name: :api, if: ->(_) { true } }, { name: :read_user, if: ->(_) { false } }])).to be(true) + end + end end end end