Merge branch 'security-enforce-group-ip-restrictions-with-globalid-in-graphql' into 'master'

Enforce group IP restriction on GraphQL API when using global ID

See merge request gitlab-org/security/gitlab!2094

app/graphql/resolvers/base_issues_resolver.rb

@@ -36,7 +36,7 @@ module Resolvers
     def unconditional_includes
       [
         {
-          project: [:project_feature]
+          project: [:project_feature, :group]
         },
         :author
       ]

ee/app/policies/ee/group_policy.rb

@@ -328,6 +328,7 @@ module EE
 
       rule { ip_enforcement_prevents_access & ~owner & ~auditor }.policy do
         prevent :read_group
+        prevent :read_milestone
       end
 
       rule { owner & group_saml_enabled }.policy do

ee/app/policies/ee/project_policy.rb

@@ -339,6 +339,10 @@ module EE
 
       rule { ip_enforcement_prevents_access & ~admin & ~auditor }.policy do
         prevent :read_project
+        prevent :read_issue
+        prevent :read_merge_request
+        prevent :read_milestone
+        prevent :read_container_image
       end
 
       rule { locked_approvers_rules }.policy do

ee/spec/policies/group_policy_spec.rb

@@ -575,6 +575,7 @@ RSpec.describe GroupPolicy do
 
     context 'without restriction' do
       it { is_expected.to be_allowed(:read_group) }
+      it { is_expected.to be_allowed(:read_milestone) }
     end
 
     context 'with restriction' do
@@ -586,6 +587,7 @@ RSpec.describe GroupPolicy do
         let(:range) { '192.168.0.0/24' }
 
         it { is_expected.to be_allowed(:read_group) }
+        it { is_expected.to be_allowed(:read_milestone) }
       end
 
       context 'address is outside the range' do
@@ -593,18 +595,21 @@ RSpec.describe GroupPolicy do
 
         context 'as developer' do
           it { is_expected.to be_disallowed(:read_group) }
+          it { is_expected.to be_disallowed(:read_milestone) }
         end
 
         context 'as owner' do
           let(:current_user) { owner }
 
           it { is_expected.to be_allowed(:read_group) }
+          it { is_expected.to be_allowed(:read_milestone) }
         end
 
         context 'as auditor' do
           let(:current_user) { create(:user, :auditor) }
 
           it { is_expected.to be_allowed(:read_group) }
+          it { is_expected.to be_allowed(:read_milestone) }
         end
       end
     end

ee/spec/policies/project_policy_spec.rb

@@ -455,6 +455,10 @@ RSpec.describe ProjectPolicy do
 
       context 'group without restriction' do
         it { is_expected.to be_allowed(:read_project) }
+        it { is_expected.to be_allowed(:read_issue) }
+        it { is_expected.to be_allowed(:read_merge_request) }
+        it { is_expected.to be_allowed(:read_milestone) }
+        it { is_expected.to be_allowed(:read_container_image) }
       end
 
       context 'group with restriction' do
@@ -466,25 +470,45 @@ RSpec.describe ProjectPolicy do
           let(:range) { '192.168.0.0/24' }
 
           it { is_expected.to be_allowed(:read_project) }
+          it { is_expected.to be_allowed(:read_issue) }
+          it { is_expected.to be_allowed(:read_merge_request) }
+          it { is_expected.to be_allowed(:read_milestone) }
+          it { is_expected.to be_allowed(:read_container_image) }
         end
 
         context 'address is outside the range' do
           let(:range) { '10.0.0.0/8' }
 
           it { is_expected.to be_disallowed(:read_project) }
+          it { is_expected.to be_disallowed(:read_issue) }
+          it { is_expected.to be_disallowed(:read_merge_request) }
+          it { is_expected.to be_disallowed(:read_milestone) }
+          it { is_expected.to be_disallowed(:read_container_image) }
 
           context 'with admin enabled', :enable_admin_mode do
             it { is_expected.to be_allowed(:read_project) }
+            it { is_expected.to be_allowed(:read_issue) }
+            it { is_expected.to be_allowed(:read_merge_request) }
+            it { is_expected.to be_allowed(:read_milestone) }
+            it { is_expected.to be_allowed(:read_container_image) }
           end
 
           context 'with admin disabled' do
             it { is_expected.to be_disallowed(:read_project) }
+            it { is_expected.to be_disallowed(:read_issue) }
+            it { is_expected.to be_disallowed(:read_merge_request) }
+            it { is_expected.to be_disallowed(:read_milestone) }
+            it { is_expected.to be_disallowed(:read_container_image) }
           end
 
           context 'with auditor' do
             let(:current_user) { create(:user, :auditor) }
 
             it { is_expected.to be_allowed(:read_project) }
+            it { is_expected.to be_allowed(:read_issue) }
+            it { is_expected.to be_allowed(:read_merge_request) }
+            it { is_expected.to be_allowed(:read_milestone) }
+            it { is_expected.to be_allowed(:read_container_image) }
           end
         end
       end

spec/graphql/resolvers/issues_resolver_spec.rb

@@ -560,13 +560,13 @@ RSpec.describe Resolvers::IssuesResolver do
       end
 
       it 'finds a specific issue with iid', :request_store do
-        result = batch_sync(max_queries: 5) { resolve_issues(iid: issue1.iid).to_a }
+        result = batch_sync(max_queries: 6) { resolve_issues(iid: issue1.iid).to_a }
 
         expect(result).to contain_exactly(issue1)
       end
 
       it 'batches queries that only include IIDs', :request_store do
-        result = batch_sync(max_queries: 5) do
+        result = batch_sync(max_queries: 6) do
           [issue1, issue2]
             .map { |issue| resolve_issues(iid: issue.iid.to_s) }
             .flat_map(&:to_a)
@@ -576,7 +576,7 @@ RSpec.describe Resolvers::IssuesResolver do
       end
 
       it 'finds a specific issue with iids', :request_store do
-        result = batch_sync(max_queries: 5) do
+        result = batch_sync(max_queries: 6) do
           resolve_issues(iids: [issue1.iid]).to_a
         end