Require 'api' scope to execute mutations

Verify that read_api tokens cannot run mutations.

Also: adds tests use of OAuth tokens for GraphQL

We make some changes to the sessionless_authentication module
in order to capture the request_authenticator, so that we can access
the token scopes, without making any extra queries.

We ensure we always authorize the mutation, which, like all resolvers,
needs to opt in to the check.

Unlike resolvers, mutations should always raise. So
`BaseMutation.authorized?` raises on failure.

Logic for handling scopes is pushed down to the `ObjectAuthorization`
class, and encapsulated in the `ScopeValidator`, which limits the
methods that can be called by resolvers.

app/controllers/concerns/sessionless_authentication.rb

@@ -7,11 +7,15 @@
 module SessionlessAuthentication
   # This filter handles personal access tokens, atom requests with rss tokens, and static object tokens
   def authenticate_sessionless_user!(request_format)
-    user = Gitlab::Auth::RequestAuthenticator.new(request).find_sessionless_user(request_format)
+    user = request_authenticator.find_sessionless_user(request_format)
 
     sessionless_sign_in(user) if user
   end
 
+  def request_authenticator
+    @request_authenticator ||= Gitlab::Auth::RequestAuthenticator.new(request)
+  end
+
   def sessionless_user?
     current_user && !session.key?('warden.user.user.key')
   end

app/controllers/graphql_controller.rb

@@ -112,7 +112,13 @@ class GraphqlController < ApplicationController
   # When modifying the context, also update GraphqlChannel#context if needed
   # so that we have similar context when executing queries, mutations, and subscriptions
   def context
-    @context ||= { current_user: current_user, is_sessionless_user: !!sessionless_user?, request: request }
+    api_user = !!sessionless_user?
+    @context ||= {
+      current_user: current_user,
+      is_sessionless_user: api_user,
+      request: request,
+      scope_validator: ::Gitlab::Auth::ScopeValidator.new(api_user, request_authenticator)
+    }
   end
 
   def build_variables(variable_info)

app/graphql/mutations/base_mutation.rb

@@ -44,9 +44,18 @@ module Mutations
       end
     end
 
+    def self.authorizes_object?
+      true
+    end
+
     def self.authorized?(object, context)
-      # we never provide an object to mutations, but we do need to have a user.
-      context[:current_user].present? && !context[:current_user].blocked?
+      auth = ::Gitlab::Graphql::Authorize::ObjectAuthorization.new(:execute_graphql_mutation, :api)
+
+      return true if auth.ok?(:global, context[:current_user],
+                              scope_validator: context[:scope_validator])
+
+      # in our mutations we raise, rather than returning a null value.
+      raise_resource_not_available_error!
     end
 
     # See: AuthorizeResource#authorized_resource?

app/policies/global_policy.rb

@@ -23,6 +23,7 @@ class GlobalPolicy < BasePolicy
     prevent :receive_notifications
     prevent :use_quick_actions
     prevent :create_group
+    prevent :execute_graphql_mutation
   end
 
   rule { default }.policy do
@@ -32,6 +33,7 @@ class GlobalPolicy < BasePolicy
     enable :receive_notifications
     enable :use_quick_actions
     enable :use_slash_commands
+    enable :execute_graphql_mutation
   end
 
   rule { inactive }.policy do
@@ -48,6 +50,8 @@ class GlobalPolicy < BasePolicy
     prevent :use_slash_commands
   end
 
+  rule { ~can?(:access_api) }.prevent :execute_graphql_mutation
+
   rule { blocked | (internal & ~migration_bot & ~security_bot) }.policy do
     prevent :access_git
   end

changelogs/unreleased/327155-prevent-mutation-execution-with-read-api-tokens.yml

+---
+title: Prevent tokens with only read_api scope from executing mutations
+merge_request:
+author:
+type: security

lib/gitlab/auth/scope_validator.rb

+# frozen_string_literal: true
+
+# Wrapper around a RequestAuthenticator to
+# perform authorization of scopes. Access is limited to
+# only those methods needed to validate that an API user
+# has at least one permitted scope.
+module Gitlab
+  module Auth
+    class ScopeValidator
+      def initialize(api_user, request_authenticator)
+        @api_user = api_user
+        @request_authenticator = request_authenticator
+      end
+
+      def valid_for?(permitted)
+        return true unless @api_user
+        return true if permitted.none?
+
+        scopes = permitted.map { |s| API::Scope.new(s) }
+        @request_authenticator.valid_access_token?(scopes: scopes)
+      end
+    end
+  end
+end

lib/gitlab/graphql/authorize/object_authorization.rb

@@ -4,10 +4,11 @@ module Gitlab
   module Graphql
     module Authorize
       class ObjectAuthorization
-        attr_reader :abilities
+        attr_reader :abilities, :permitted_scopes
 
-        def initialize(abilities)
+        def initialize(abilities, scopes = %i[api read_api])
           @abilities = Array.wrap(abilities).flatten
+          @permitted_scopes = Array.wrap(scopes)
         end
 
         def none?
@@ -18,7 +19,13 @@ module Gitlab
           abilities.present?
         end
 
-        def ok?(object, current_user)
+        def ok?(object, current_user, scope_validator: nil)
+          scopes_ok?(scope_validator) && abilities_ok?(object, current_user)
+        end
+
+        private
+
+        def abilities_ok?(object, current_user)
           return true if none?
 
           subject = object.try(:declarative_policy_subject) || object
@@ -26,6 +33,12 @@ module Gitlab
             Ability.allowed?(current_user, ability, subject)
           end
         end
+
+        def scopes_ok?(validator)
+          return true unless validator.present?
+
+          validator.valid_for?(permitted_scopes)
+        end
       end
     end
   end

spec/requests/api/graphql/mutations/notes/create/note_spec.rb

@@ -31,6 +31,8 @@ RSpec.describe 'Adding a Note' do
       project.add_developer(current_user)
     end
 
+    it_behaves_like 'a working GraphQL mutation'
+
     it_behaves_like 'a Note mutation that creates a Note'
 
     it_behaves_like 'a Note mutation when there are active record validation errors'

spec/support/helpers/graphql_helpers.rb

@@ -396,17 +396,21 @@ module GraphqlHelpers
     post api('/', current_user, version: 'graphql'), params: { _json: queries }, headers: headers
   end
 
-  def post_graphql(query, current_user: nil, variables: nil, headers: {})
+  def post_graphql(query, current_user: nil, variables: nil, headers: {}, token: {})
     params = { query: query, variables: serialize_variables(variables) }
-    post api('/', current_user, version: 'graphql'), params: params, headers: headers
+    post api('/', current_user, version: 'graphql', **token), params: params, headers: headers
 
-    if graphql_errors # Errors are acceptable, but not this one:
+    return unless graphql_errors
+
+    # Errors are acceptable, but not this one:
     expect(graphql_errors).not_to include(a_hash_including('message' => 'Internal server error'))
   end
-  end
 
-  def post_graphql_mutation(mutation, current_user: nil)
-    post_graphql(mutation.query, current_user: current_user, variables: mutation.variables)
+  def post_graphql_mutation(mutation, current_user: nil, token: {})
+    post_graphql(mutation.query,
+                 current_user: current_user,
+                 variables: mutation.variables,
+                 token: token)
   end
 
   def post_graphql_mutation_with_uploads(mutation, current_user: nil)

spec/support/shared_examples/requests/graphql_shared_examples.rb

@@ -10,6 +10,52 @@ RSpec.shared_examples 'a working graphql query' do
   end
 end
 
+RSpec.shared_examples 'a working GraphQL mutation' do
+  include GraphqlHelpers
+
+  before do
+    post_graphql_mutation(mutation, current_user: current_user, token: token)
+  end
+
+  shared_examples 'allows access to the mutation' do
+    let(:scopes) { ['api'] }
+
+    it_behaves_like 'a working graphql query' do
+      it 'returns data' do
+        expect(graphql_data.compact).not_to be_empty
+      end
+    end
+  end
+
+  shared_examples 'prevents access to the mutation' do
+    let(:scopes) { ['read_api'] }
+
+    it 'does not resolve the mutation' do
+      expect(graphql_data.compact).to be_empty
+      expect(graphql_errors).to be_present
+    end
+  end
+
+  context 'with a personal access token' do
+    let(:token) do
+      pat = create(:personal_access_token, user: current_user, scopes: scopes)
+      { personal_access_token: pat }
+    end
+
+    it_behaves_like 'prevents access to the mutation'
+    it_behaves_like 'allows access to the mutation'
+  end
+
+  context 'with an OAuth token' do
+    let(:token) do
+      { oauth_access_token: create(:oauth_access_token, resource_owner: current_user, scopes: scopes.join(' ')) }
+    end
+
+    it_behaves_like 'prevents access to the mutation'
+    it_behaves_like 'allows access to the mutation'
+  end
+end
+
 RSpec.shared_examples 'a mutation on an unauthorized resource' do
   it_behaves_like 'a mutation that returns top-level errors',
                       errors: [::Gitlab::Graphql::Authorize::AuthorizeResource::RESOURCE_ACCESS_ERROR]