Merge branch 'security-1072-graphql-subscription-scope-validation-16-9' into '16-9-stable-ee'

Ensure PAT scope is validated everywhere for GraphQL/ActionCable

See merge request https://gitlab.com/gitlab-org/security/gitlab/-/merge_requests/3977



Merged-by: GitLab Release Tools Bot <delivery-team+release-tools@gitlab.com>
Approved-by: Luke Duncalfe <lduncalfe@gitlab.com>
Co-authored-by: Dylan Griffith <dyl.griffith@gmail.com>

app/channels/application_cable/channel.rb

@@ -3,6 +3,15 @@
 module ApplicationCable
   class Channel < ActionCable::Channel::Base
     include Logging
+    include Gitlab::Auth::AuthFinders
+
+    before_subscribe :validate_token_scope
+
+    def validate_token_scope
+      validate_access_token!(scopes: [:api, :read_api])
+    rescue Gitlab::Auth::InsufficientScopeError
+      reject
+    end
     private

app/channels/graphql_channel.rb

@@ -48,7 +48,8 @@ def unsubscribed
   # Objects added to the context may also need to be reloaded in
   # `Subscriptions::BaseSubscription` so that they are not stale
   def context
-    # is_sessionless_user is always false because we only support cookie auth in ActionCable
-    { channel: self, current_user: current_user, is_sessionless_user: false }
+    request_authenticator = Gitlab::Auth::RequestAuthenticator.new(request)
+    scope_validator = ::Gitlab::Auth::ScopeValidator.new(current_user, request_authenticator)
+    { channel: self, current_user: current_user, is_sessionless_user: false, scope_validator: scope_validator }
   end
 end

app/graphql/resolvers/base_resolver.rb

@@ -174,7 +174,7 @@ def self.authorization
     end
     def self.authorized?(object, context)
-      authorization.ok?(object, context[:current_user])
+      authorization.ok?(object, context[:current_user], scope_validator: context[:scope_validator])
     end
   end
 end

app/graphql/types/base_enum.rb

@@ -65,7 +65,7 @@ def authorize(*abilities)
       end
       def authorized?(object, context)
-        authorization.ok?(object, context[:current_user])
+        authorization.ok?(object, context[:current_user], scope_validator: context[:scope_validator])
       end
     end
   end

app/graphql/types/base_field.rb

@@ -97,7 +97,7 @@ def constant_complexity?
     def field_authorized?(object, ctx)
       object = object.node if object.is_a?(GraphQL::Pagination::Connection::Edge)
-      authorization.ok?(object, ctx[:current_user])
+      authorization.ok?(object, ctx[:current_user], scope_validator: ctx[:scope_validator])
     end
     # Historically our resolvers have used declarative permission checks only

app/graphql/types/base_object.rb

@@ -32,7 +32,7 @@ def self.authorization
     end
     def self.authorized?(object, context)
-      authorization.ok?(object, context[:current_user])
+      authorization.ok?(object, context[:current_user], scope_validator: context[:scope_validator])
     end
     def current_user

lib/gitlab/graphql/authorize/authorize_resource.rb

@@ -64,7 +64,7 @@ def authorize!(object)
         def authorized_resource?(object)
           raise ConfigurationError, "#{self.class.name} has no authorizations" if self.class.authorization.none?
-          self.class.authorization.ok?(object, current_user)
+          self.class.authorization.ok?(object, current_user, scope_validator: context[:scope_validator])
         end
         def raise_resource_not_available_error!(...)

lib/gitlab/graphql/authorize/object_authorization.rb

@@ -19,7 +19,7 @@ def any?
           abilities.present?
         end
-        def ok?(object, current_user, scope_validator: nil)
+        def ok?(object, current_user, scope_validator:)
           scopes_ok?(scope_validator) && abilities_ok?(object, current_user)
         end

spec/channels/graphql_channel_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe GraphqlChannel, feature_category: :api do
+  let_it_be(:merge_request) { create(:merge_request) }
+  let_it_be(:user) { create(:user).tap { |u| merge_request.project.add_developer(u) } }
+  let_it_be(:read_api_token) { create(:personal_access_token, scopes: ['read_api'], user: user) }
+  let_it_be(:read_user_token) { create(:personal_access_token, scopes: ['read_user'], user: user) }
+  let_it_be(:read_api_and_read_user_token) do
+    create(:personal_access_token, scopes: %w[read_user read_api], user: user)
+  end
+
+  describe '#subscribed' do
+    let(:query) do
+      <<~GRAPHQL
+      subscription mergeRequestReviewersUpdated($issuableId: IssuableID!) {
+        mergeRequestReviewersUpdated(issuableId: $issuableId) {
+          ... on MergeRequest { id title }
+        }
+      }
+      GRAPHQL
+    end
+
+    let(:subscribe_params) do
+      {
+        query: query,
+        variables: { issuableId: merge_request.to_global_id }
+      }
+    end
+
+    before do
+      stub_action_cable_connection current_user: user
+    end
+
+    it 'subscribes to the given graphql subscription' do
+      subscribe(subscribe_params)
+
+      expect(subscription).to be_confirmed
+      expect(subscription.streams).to include(/graphql-event::mergeRequestReviewersUpdated:issuableId/)
+    end
+
+    context 'with a personal access token' do
+      before do
+        stub_action_cable_connection current_user: user, access_token: access_token
+      end
+
+      context 'with an api scoped personal access token' do
+        let(:access_token) { read_api_token }
+
+        it 'subscribes to the given graphql subscription' do
+          subscribe(subscribe_params)
+
+          expect(subscription).to be_confirmed
+          expect(subscription.streams).to include(/graphql-event::mergeRequestReviewersUpdated:issuableId/)
+        end
+      end
+
+      context 'with a read_user personal access token' do
+        let(:access_token) { read_user_token }
+
+        it 'does not subscribe to the given graphql subscription' do
+          subscribe(subscribe_params)
+
+          expect(subscription).not_to be_confirmed
+        end
+      end
+
+      context 'with a read_api and read_user personal access token' do
+        let(:access_token) { read_api_and_read_user_token }
+
+        it 'subscribes to the given graphql subscription' do
+          subscribe(subscribe_params)
+
+          expect(subscription).to be_confirmed
+          expect(subscription.streams).to include(/graphql-event::mergeRequestReviewersUpdated:issuableId/)
+        end
+      end
+    end
+  end
+end

spec/channels/noteable/notes_channel_spec.rb

@@ -4,7 +4,15 @@
 RSpec.describe Noteable::NotesChannel, feature_category: :team_planning do
   let_it_be(:project) { create(:project, :repository, :private) }
-  let_it_be(:developer) { create(:user).tap { |u| project.add_developer(u) } }
+  let_it_be(:user) { create(:user).tap { |u| project.add_developer(u) } }
+
+  let_it_be(:read_api_token) { create(:personal_access_token, scopes: ['read_api'], user: user) }
+  let_it_be(:read_user_token) { create(:personal_access_token, scopes: ['read_user'], user: user) }
+  let_it_be(:read_api_and_read_user_token) do
+    create(:personal_access_token, scopes: %w[read_user read_api], user: user)
+  end
+
+  let_it_be(:noteable) { create(:issue, project: project) }
   describe '#subscribed' do
     let(:subscribe_params) do
@@ -16,7 +24,7 @@
     end
     before do
-      stub_action_cable_connection current_user: developer
+      stub_action_cable_connection current_user: user
     end
     it 'rejects the subscription when noteable params are missing' do
@@ -26,8 +34,6 @@
     end
     context 'on an issue' do
-      let_it_be(:noteable) { create(:issue, project: project) }
-
       it_behaves_like 'handle subscription based on user access'
     end
@@ -37,4 +43,50 @@
       it_behaves_like 'handle subscription based on user access'
     end
   end
+
+  context 'with a personal access token' do
+    let(:subscribe_params) do
+      {
+        project_id: noteable.project_id,
+        noteable_type: noteable.class.underscore,
+        noteable_id: noteable.id
+      }
+    end
+
+    before do
+      stub_action_cable_connection current_user: user, access_token: access_token
+    end
+
+    context 'with an api scoped personal access token' do
+      let(:access_token) { read_api_token }
+
+      it 'subscribes to the given graphql subscription' do
+        subscribe(subscribe_params)
+
+        expect(subscription).to be_confirmed
+        expect(subscription).to have_stream_for(noteable)
+      end
+    end
+
+    context 'with a read_user personal access token' do
+      let(:access_token) { read_user_token }
+
+      it 'does not subscribe to the given graphql subscription' do
+        subscribe(subscribe_params)
+
+        expect(subscription).not_to be_confirmed
+      end
+    end
+
+    context 'with a read_api and read_user personal access token' do
+      let(:access_token) { read_api_and_read_user_token }
+
+      it 'subscribes to the given graphql subscription' do
+        subscribe(subscribe_params)
+
+        expect(subscription).to be_confirmed
+        expect(subscription).to have_stream_for(noteable)
+      end
+    end
+  end
 end

spec/graphql/resolvers/base_resolver_spec.rb

@@ -4,6 +4,7 @@
 RSpec.describe Resolvers::BaseResolver, feature_category: :api do
   include GraphqlHelpers
+  let_it_be(:user) { create(:user) }
   let(:resolver) do
     Class.new(described_class) do
@@ -247,8 +248,6 @@ def resolve(**args)
   end
   describe '#object' do
-    let_it_be(:user) { create(:user) }
-
     it 'returns object' do
       expect_next_instance_of(resolver) do |r|
         expect(r).to receive(:process).with(user)
@@ -275,4 +274,18 @@ def resolve(**args)
       expect(instance.offset_pagination(User.none)).to be_a(::Gitlab::Graphql::Pagination::OffsetPaginatedRelation)
     end
   end
+
+  describe '#authorized?' do
+    let(:object) { :object }
+    let(:scope_validator) { instance_double(::Gitlab::Auth::ScopeValidator) }
+    let(:context) { { current_user: user, scope_validator: scope_validator } }
+
+    it 'delegates to authorization' do
+      expect(resolver.authorization).to be_kind_of(::Gitlab::Graphql::Authorize::ObjectAuthorization)
+      expect(resolver.authorization).to receive(:ok?)
+        .with(object, user, scope_validator: scope_validator)
+
+      resolver.authorized?(object, context)
+    end
+  end
 end

spec/graphql/types/base_enum_spec.rb

@@ -139,4 +139,19 @@ def subject(args = {})
       enum.values['TEST_VALUE']
     end
   end
+
+  describe '#authorized?' do
+    let(:object) { :object }
+    let(:scope_validator) { instance_double(::Gitlab::Auth::ScopeValidator) }
+    let(:user) { double }
+    let(:context) { { current_user: user, scope_validator: scope_validator } }
+
+    it 'delegates to authorization' do
+      expect(described_class.authorization).to be_kind_of(::Gitlab::Graphql::Authorize::ObjectAuthorization)
+      expect(described_class.authorization).to receive(:ok?)
+        .with(object, user, scope_validator: scope_validator)
+
+      described_class.authorized?(object, context)
+    end
+  end
 end

spec/graphql/types/base_field_spec.rb

@@ -235,4 +235,21 @@ def subject(args = {})
       described_class.new(**base_args.merge(args))
     end
   end
+
+  describe '#field_authorized?' do
+    let(:object) { :object }
+    let(:scope_validator) { instance_double(::Gitlab::Auth::ScopeValidator) }
+    let(:user) { :user }
+    let(:context) { { current_user: user, scope_validator: scope_validator } }
+
+    it 'delegates to authorization' do
+      expect_next_instance_of(::Gitlab::Graphql::Authorize::ObjectAuthorization) do |authorization|
+        expect(authorization).to receive(:ok?)
+          .with(object, user, scope_validator: scope_validator)
+      end
+
+      field = described_class.new(name: 'test', type: GraphQL::Types::String, null: true)
+      field.authorized?(object, nil, context)
+    end
+  end
 end

spec/graphql/types/base_object_spec.rb

@@ -12,15 +12,18 @@ def any?
           true
         end
-        def ok?(object, _current_user)
+        def ok?(object, _current_user, scope_validator: nil)
           return false if object == { id: 100 }
           return false if object.try(:deactivated?)
+          raise "Missing scope_validator" unless scope_validator
           true
         end
       end
     end
+    let(:scope_validator) { instance_double(::Gitlab::Auth::ScopeValidator, valid_for?: true) }
+
     let_it_be(:test_schema) do
       auth = custom_auth.new(nil)
@@ -172,6 +175,7 @@ def document(path)
     let(:data) do
       {
+        scope_validator: scope_validator,
         x: {
           title: 'Hey',
           ys: [{ id: 1 }, { id: 100 }, { id: 2 }]
@@ -216,6 +220,7 @@ def document(path)
       n = 7
       data = {
+        scope_validator: scope_validator,
         x: {
           ys: (95..105).to_a.map { |id| { id: id } }
         }
@@ -262,7 +267,10 @@ def document(path)
       active_users = create_list(:user, 3, state: :active)
       inactive = create(:user, state: :deactivated)
-      data = { user_ids: [inactive, *active_users].map(&:id) }
+      data = {
+        scope_validator: scope_validator,
+        user_ids: [inactive, *active_users].map(&:id)
+      }
       doc = GraphQL.parse(<<~GQL)
       query {
@@ -281,6 +289,7 @@ def document(path)
     it 'filters polymorphic connections' do
       data = {
         current_user: :the_user,
+        scope_validator: scope_validator,
         x: {
           values: [{ value: 1 }, { value: 2 }, { value: 3 }, { value: 4 }]
         }
@@ -324,6 +333,7 @@ def document(path)
     it 'filters interface connections' do
       data = {
         current_user: :the_user,
+        scope_validator: scope_validator,
         x: {
           values: [{ value: 1 }, { value: 2 }, { value: 3 }, { value: 4 }]
         }
@@ -368,6 +378,7 @@ def document(path)
     it 'redacts polymorphic objects' do
       data = {
         current_user: :the_user,
+        scope_validator: scope_validator,
         x: {
           values: [{ value: 1 }]
         }
@@ -408,7 +419,10 @@ def document(path)
       inactive = create_list(:user, n - 1, state: :deactivated)
       active_users = create_list(:user, 2, state: :active)
-      data = { user_ids: [*inactive, *active_users].map(&:id) }
+      data = {
+        scope_validator: scope_validator,
+        user_ids: [*inactive, *active_users].map(&:id)
+      }
       doc = GraphQL.parse(<<~GQL)
       query {

spec/lib/gitlab/graphql/authorize/authorize_resource_spec.rb

@@ -7,13 +7,14 @@
     Class.new do
       include Gitlab::Graphql::Authorize::AuthorizeResource
-      attr_reader :user, :found_object
+      attr_reader :user, :found_object, :scope_validator
       authorize :read_the_thing
-      def initialize(user, found_object)
+      def initialize(user, found_object, scope_validator)
         @user = user
         @found_object = found_object
+        @scope_validator = scope_validator
       end
       def find_object
@@ -25,7 +26,7 @@ def current_user
       end
       def context
-        { current_user: user }
+        { current_user: user, scope_validator: scope_validator }
       end
       def self.authorization
@@ -35,9 +36,10 @@ def self.authorization
   end
   let(:user) { build(:user) }
+  let(:scope_validator) { instance_double(::Gitlab::Auth::ScopeValidator, valid_for?: true) }
   let(:project) { build(:project) }
-  subject(:loading_resource) { fake_class.new(user, project) }
+  subject(:loading_resource) { fake_class.new(user, project, scope_validator) }
   before do
     # don't allow anything by default
@@ -139,4 +141,16 @@ def self.authorization
       expect(child).to be_authorizes_object
     end
   end
+
+  describe '#authorized_resource?' do
+    let(:object) { :object }
+
+    it 'delegates to authorization' do
+      expect(fake_class.authorization).to be_kind_of(::Gitlab::Graphql::Authorize::ObjectAuthorization)
+      expect(fake_class.authorization).to receive(:ok?)
+        .with(object, user, scope_validator: scope_validator)
+
+      fake_class.new(user, :found_object, scope_validator).authorized_resource?(object)
+    end
+  end
 end

spec/lib/gitlab/graphql/authorize/object_authorization_spec.rb

@@ -4,9 +4,10 @@
 RSpec.describe ::Gitlab::Graphql::Authorize::ObjectAuthorization do
   describe '#ok?' do
-    subject { described_class.new(%i[go_fast go_slow]) }
+    subject(:authorization) { described_class.new(%i[go_fast go_slow]) }
     let(:user) { double(:User, id: 10001) }
+    let(:scope_validator) { instance_double(::Gitlab::Auth::ScopeValidator, valid_for?: true) }
     let(:policy) do
       Class.new(::DeclarativePolicy::Base) do
@@ -31,25 +32,25 @@
     context 'when there are no abilities' do
       subject { described_class.new([]) }
-      it { is_expected.to be_ok(double, double) }
+      it { is_expected.to be_ok(double, double, scope_validator: scope_validator) }
     end
     context 'when no ability should be allowed' do
       let(:object) { Foo.new(0, 0) }
-      it { is_expected.not_to be_ok(object, user) }
+      it { is_expected.not_to be_ok(object, user, scope_validator: scope_validator) }
     end
     context 'when go_fast should be allowed' do
       let(:object) { Foo.new(100, 0) }
-      it { is_expected.not_to be_ok(object, user) }
+      it { is_expected.not_to be_ok(object, user, scope_validator: scope_validator) }
     end
     context 'when go_fast and go_slow should be allowed' do
       let(:object) { Foo.new(100, 100) }
-      it { is_expected.to be_ok(object, user) }
+      it { is_expected.to be_ok(object, user, scope_validator: scope_validator) }
     end
     context 'when the object delegates to another subject' do
@@ -57,8 +58,19 @@ def proxy(foo)
         double(:Proxy, declarative_policy_subject: foo)
       end
-      it { is_expected.to be_ok(proxy(Foo.new(100, 100)), user) }
-      it { is_expected.not_to be_ok(proxy(Foo.new(0, 100)), user) }
+      it { is_expected.to be_ok(proxy(Foo.new(100, 100)), user, scope_validator: scope_validator) }
+      it { is_expected.not_to be_ok(proxy(Foo.new(0, 100)), user, scope_validator: scope_validator) }
+    end
+
+    context 'when scope is not valid for scope validator' do
+      let(:object) { Foo.new(100, 100) }
+
+      it 'returns false' do
+        expect(scope_validator).to receive(:valid_for?).with([:api, :read_api])
+          .and_return(false)
+
+        expect(authorization).not_to be_ok(object, user, scope_validator: scope_validator)
+      end
     end
   end
 end

spec/support/helpers/stub_action_cable_connection.rb

 # frozen_string_literal: true
 module StubActionCableConnection
-  def stub_action_cable_connection(current_user: nil, request: ActionDispatch::TestRequest.create)
+  def stub_action_cable_connection(current_user: nil, access_token: nil, request: ActionDispatch::TestRequest.create)
+    request.headers['Authorization'] = "Bearer #{access_token.token}" if access_token
     stub_connection(current_user: current_user, request: request)
   end
 end