Initial attempt at refactoring API scope declarations.

- Declaring an endpoint's scopes in a `before` block has proved to be
  unreliable. For example, if we're accessing the `API::Users` endpoint - code
  in a `before` block in `API::API` wouldn't be able to see the scopes set in
  `API::Users` since the `API::API` `before` block runs first.

- This commit moves these declarations to the class level, since they don't need
  to change once set.

app/services/access_token_validation_service.rb

@@ -31,8 +31,11 @@ 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.
-      Set.new(scopes).intersection(Set.new(token.scopes)).present?
+
+      scope_names = scopes.map { |scope| scope[:name].to_s }
+      Set.new(scope_names).intersection(Set.new(token.scopes)).present?
     end
   end
 end

lib/api/api.rb

@@ -2,6 +2,8 @@ module API
   class API < Grape::API
     include APIGuard
 
+    allow_access_with_scope :api
+
     version %w(v3 v4), using: :path
 
     version 'v3', using: :path do
@@ -44,7 +46,6 @@ module API
       mount ::API::V3::Variables
     end
 
-    before { allow_access_with_scope :api }
     before { header['X-Frame-Options'] = 'SAMEORIGIN' }
     before { Gitlab::I18n.locale = current_user&.preferred_language }
 

lib/api/api_guard.rb

@@ -23,6 +23,27 @@ module API
       install_error_responders(base)
     end
 
+    class_methods do
+      # Set the authorization scope(s) allowed for the current request.
+      #
+      # A call to this method adds to any previous scopes in place, either from the same class, or
+      # higher up in the inheritance chain. For example, if we call `allow_access_with_scope :api` from
+      # `API::API`, and `allow_access_with_scope :read_user` from `API::Users` (which inherits from `API::API`),
+      # `API::Users` will allow access with either the `api` or `read_user` scope. `API::API` will allow
+      # access only with the `api` scope.
+      def allow_access_with_scope(scopes, options = {})
+        @scopes ||= []
+
+        params = Array.wrap(scopes).map { |scope| { name: scope, if: options[:if] } }
+
+        @scopes.concat(params)
+      end
+
+      def scopes
+        @scopes
+      end
+    end
+
     # Helper Methods for Grape Endpoint
     module HelperMethods
       # Invokes the doorkeeper guard.
@@ -74,18 +95,6 @@ module API
         @current_user
       end
 
-      # Set the authorization scope(s) allowed for the current request.
-      #
-      # Note: A call to this method adds to any previous scopes in place. This is done because
-      # `Grape` callbacks run from the outside-in: the top-level callback (API::API) runs first, then
-      # the next-level callback (API::API::Users, for example) runs. All these scopes are valid for the
-      # given endpoint (GET `/api/users` is accessible by the `api` and `read_user` scopes), and so they
-      # need to be stored.
-      def allow_access_with_scope(*scopes)
-        @scopes ||= []
-        @scopes.concat(scopes.map(&:to_s))
-      end
-
       private
 
       def find_user_by_authentication_token(token_string)

lib/api/helpers.rb

@@ -340,10 +340,12 @@ module API
     end
 
     def initial_current_user
+      endpoint_class = options[:for]
+
       return @initial_current_user if defined?(@initial_current_user)
       Gitlab::Auth::UniqueIpsLimiter.limit_user! do
-        @initial_current_user ||= find_user_by_private_token(scopes: @scopes)
-        @initial_current_user ||= doorkeeper_guard(scopes: @scopes)
+        @initial_current_user ||= find_user_by_private_token(scopes: endpoint_class.scopes)
+        @initial_current_user ||= doorkeeper_guard(scopes: endpoint_class.scopes)
         @initial_current_user ||= find_user_from_warden
 
         unless @initial_current_user && Gitlab::UserAccess.new(@initial_current_user).allowed?

lib/api/users.rb

@@ -1,9 +1,11 @@
 module API
   class Users < Grape::API
     include PaginationParams
+    include APIGuard
+
+    allow_access_with_scope :read_user, if: -> (request) { request.get? }
 
     before do
-      allow_access_with_scope :read_user if request.get?
       authenticate!
     end
 

lib/api/v3/users.rb

@@ -2,9 +2,11 @@ module API
   module V3
     class Users < Grape::API
       include PaginationParams
+      include APIGuard
+
+      allow_access_with_scope :read_user, if: -> (request) { request.get? }
 
       before do
-        allow_access_with_scope :read_user if request.get?
         authenticate!
       end
 

spec/requests/api/users_spec.rb

@@ -50,6 +50,28 @@ describe API::Users do
         end['username']).to eq(username)
       end
 
+      context "scopes" do
+        context 'when the requesting token has the "read_user" scope' do
+          let(:token) { create(:personal_access_token, scopes: ['read_user']) }
+
+          it 'returns a "200" response' do
+            get api("/users", user, personal_access_token: token)
+
+            expect(response).to have_http_status(200)
+          end
+        end
+
+        context 'when the requesting token does not have any required scope' do
+          let(:token) { create(:personal_access_token, scopes: ['read_registry']) }
+
+          it 'returns a "401" response' do
+            get api("/users", user, personal_access_token: token)
+
+            expect(response).to have_http_status(401)
+          end
+        end
+      end
+
       it "returns an array of blocked users" do
         ldap_blocked_user
         create(:user, state: 'blocked')

spec/support/api_helpers.rb

@@ -17,14 +17,16 @@ module ApiHelpers
   #   => "/api/v2/issues?foo=bar&private_token=..."
   #
   # Returns the relative path to the requested API resource
-  def api(path, user = nil, version: API::API.version)
+  def api(path, user = nil, version: API::API.version, personal_access_token: nil)
     "/api/#{version}#{path}" +
 
       # Normalize query string
       (path.index('?') ? '' : '?') +
 
+      if personal_access_token.present?
+        "&private_token=#{personal_access_token.token}"
       # Append private_token if given a User object
-      if user.respond_to?(:private_token)
+      elsif user.respond_to?(:private_token)
         "&private_token=#{user.private_token}"
       else
         ''