Merge remote-tracking branch 'upstream/master' into 32815--Add-Custom-CI-Config-Path

* upstream/master: (149 commits)
  Revert change to design. Go back to scrollable page
  Fixes the column widths for the new navigation options in settings
  Migrate #submodule_url_for to Gitaly
  Add test example for external commit status retries
  Fix invalid Rails.logger call in lib/gitlab/health_checks/fs_shards_check.rb
  Fix build for !12300.
  Log rescued exceptions to Sentry
  Fix issues with non-UTF8 filenames by always fixing the encoding of tree and blob paths
  Revert "Merge branch 'revert-12499' into 'master'"
  Prevent accidental deletion of protected MR source branch by repeating checks before actual deletion
  Improve the overall UX for the new monitoring dashboard
  Document that GitLab 9.3 requires the TRIGGER permission on MySQL
  Instrument Unicorn with Ruby exporter
  Remove group modal like remove project modal. Closes #33130
  Update prometheus client gem
  Enables the option in user preferences to turn on the new navigation
  Add Jasmine tests for `OAuthRememberMe`
  Simplify authentication logic in the v4 users API for !12445.
  Use stub_application_setting when testing ApplicationHelper#support_url
  wait_for_requests is not needed when AJAX is not in play
  ...

7 files changed, 89 insertions, 28 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..0d2d71e336a 100644
--- a/lib/api/api_guard.rb
+++ b/lib/api/api_guard.rb
@@ -23,6 +23,23 @@ module API
       install_error_responders(base)
     end
 
+    class_methods do
+      # Set the authorization scope(s) allowed for an API endpoint.
+      #
+      # A call to this method maps the given scope(s) to the current API
+      # endpoint class. If this method is called multiple times on the same class,
+      # the scopes are all aggregated.
+      def allow_access_with_scope(scopes, options = {})
+        Array(scopes).each do |scope|
+          allowed_scopes << Scope.new(scope, options)
+        end
+      end
+
+      def allowed_scopes
+        @scopes ||= []
+      end
+    end
+
     # Helper Methods for Grape Endpoint
     module HelperMethods
       # Invokes the doorkeeper guard.
@@ -47,7 +64,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: request).validate(scopes: scopes)
         when AccessTokenValidationService::INSUFFICIENT_SCOPE
           raise InsufficientScopeError.new(scopes)
 
@@ -74,18 +91,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)
@@ -96,7 +101,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: request).include_any_scope?(scopes)
           User.find(access_token.user_id)
         end
       end
diff --git a/lib/api/commits.rb b/lib/api/commits.rb
index c6fc17cc391..bcb842b9211 100644
--- a/lib/api/commits.rb
+++ b/lib/api/commits.rb
@@ -67,7 +67,7 @@ module API
         result = ::Files::MultiService.new(user_project, current_user, attrs).execute
 
         if result[:status] == :success
-          commit_detail = user_project.repository.commits(result[:result], limit: 1).first
+          commit_detail = user_project.repository.commit(result[:result])
           present commit_detail, with: Entities::RepoCommitDetail
         else
           render_api_error!(result[:message], 400)
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index 2c73a6fdc4e..a2a661b205c 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -342,8 +342,8 @@ module API
     def initial_current_user
       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: scopes_registered_for_endpoint)
+        @initial_current_user ||= doorkeeper_guard(scopes: scopes_registered_for_endpoint)
         @initial_current_user ||= find_user_from_warden
 
         unless @initial_current_user && Gitlab::UserAccess.new(@initial_current_user).allowed?
@@ -407,5 +407,22 @@ module API
 
       exception.status == 500
     end
+
+    # An array of scopes that were registered (using `allow_access_with_scope`)
+    # for the current endpoint class. It also returns scopes registered on
+    # `API::API`, since these are meant to apply to all API routes.
+    def scopes_registered_for_endpoint
+      @scopes_registered_for_endpoint ||=
+        begin
+          endpoint_classes = [options[:for].presence, ::API::API].compact
+          endpoint_classes.reduce([]) do |memo, endpoint|
+            if endpoint.respond_to?(:allowed_scopes)
+              memo.concat(endpoint.allowed_scopes)
+            else
+              memo
+            end
+          end
+        end
+    end
   end
 end
diff --git a/lib/api/scope.rb b/lib/api/scope.rb
new file mode 100644
index 00000000000..d5165b2e482
--- /dev/null
+++ b/lib/api/scope.rb
@@ -0,0 +1,23 @@
+# Encapsulate a scope used for authorization, such as `api`, or `read_user`
+module API
+  class Scope
+    attr_reader :name, :if
+
+    def initialize(name, options = {})
+      @name = name.to_sym
+      @if = options[:if]
+    end
+
+    # Are the `scopes` passed in sufficient to adequately authorize the passed
+    # request for the scope represented by the current instance of this class?
+    def sufficient?(scopes, request)
+      scopes.include?(self.name) && verify_if_condition(request)
+    end
+
+    private
+
+    def verify_if_condition(request)
+      self.if.nil? || self.if.call(request)
+    end
+  end
+end
diff --git a/lib/api/users.rb b/lib/api/users.rb
index f9555842daf..88bca235692 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -1,13 +1,15 @@
 module API
   class Users < Grape::API
     include PaginationParams
+    include APIGuard
 
-    before do
-      allow_access_with_scope :read_user if request.get?
-      authenticate!
-    end
+    allow_access_with_scope :read_user, if: -> (request) { request.get? }
 
     resource :users, requirements: { uid: /[0-9]*/, id: /[0-9]*/ } do
+      before do
+        authenticate_non_get!
+      end
+
       helpers do
         def find_user(params)
           id = params[:user_id] || params[:id]
@@ -51,15 +53,22 @@ module API
         use :pagination
       end
       get do
-        unless can?(current_user, :read_users_list)
-          render_api_error!("Not authorized.", 403)
-        end
-
         authenticated_as_admin! if params[:external].present? || (params[:extern_uid].present? && params[:provider].present?)
 
         users = UsersFinder.new(current_user, params).execute
 
-        entity = current_user.admin? ? Entities::UserWithAdmin : Entities::UserBasic
+        authorized = can?(current_user, :read_users_list)
+
+        # When `current_user` is not present, require that the `username`
+        # parameter is passed, to prevent an unauthenticated user from accessing
+        # a list of all the users on the GitLab instance. `UsersFinder` performs
+        # an exact match on the `username` parameter, so we are guaranteed to
+        # get either 0 or 1 `users` here.
+        authorized &&= params[:username].present? if current_user.blank?
+
+        forbidden!("Not authorized to access /api/v4/users") unless authorized
+
+        entity = current_user&.admin? ? Entities::UserWithAdmin : Entities::UserBasic
         present paginate(users), with: entity
       end
 
@@ -398,6 +407,10 @@ module API
     end
 
     resource :user do
+      before do
+        authenticate!
+      end
+
       desc 'Get the currently authenticated user' do
         success Entities::UserPublic
       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