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.

5 files changed, 33 insertions, 17 deletions

diff --git a/lib/api/api.rb b/lib/api/api.rb
index d767af36e8e..efcf0976a81 100644
--- a/lib/api/api.rb
+++ b/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 }
 
diff --git a/lib/api/api_guard.rb b/lib/api/api_guard.rb
index 9fcf04efa38..9a9e32a0242 100644
--- a/lib/api/api_guard.rb
+++ b/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)
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index 2c73a6fdc4e..3cf04e6df3c 100644
--- a/lib/api/helpers.rb
+++ b/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?
diff --git a/lib/api/users.rb b/lib/api/users.rb
index f9555842daf..2cac8c089f2 100644
--- a/lib/api/users.rb
+++ b/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
 
diff --git a/lib/api/v3/users.rb b/lib/api/v3/users.rb
index 37020019e07..cf106f2552d 100644
--- a/lib/api/v3/users.rb
+++ b/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