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.

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