Merge branch '349933-all-group-members-read-compliance-frameworks' into 'master'

Allow all users within a group to view all compliance frameworks

See merge request gitlab-org/gitlab!79133

ee/app/graphql/resolvers/compliance_management/framework_resolver.rb

@@ -5,6 +5,8 @@ module Resolvers
     class FrameworkResolver < BaseResolver
       include Gitlab::Graphql::Authorize::AuthorizeResource
 
+      authorize :read_compliance_framework
+
       type ::Types::ComplianceManagement::ComplianceFrameworkType.connection_type, null: true
 
       argument :id, ::Types::GlobalIDType[::ComplianceManagement::Framework],
@@ -31,8 +33,6 @@ module Resolvers
       def evaluate(namespace_ids, by_namespace_id, loader)
         frameworks(namespace_ids).group_by(&:namespace_id).each do |ns_id, group|
           by_namespace_id[ns_id].each do |fw_id|
-            authorize!
-
             group.each do |fw|
               next unless fw_id.nil? || fw_id.to_i == fw.id
 
@@ -45,10 +45,6 @@ module Resolvers
       def frameworks(namespace_ids)
         ::ComplianceManagement::Framework.with_namespaces(namespace_ids)
       end
-
-      def authorize!
-        Ability.allowed?(context[:current_user], :admin_compliance_framework, object) || raise_resource_not_available_error!
-      end
     end
   end
 end

ee/app/policies/compliance_management/framework_policy.rb

@@ -12,8 +12,17 @@ module ComplianceManagement
       @subject.namespace.feature_available?(:evaluate_group_level_compliance_pipeline)
     end
 
+    condition(:read_root_group) do
+      @user.can?(:read_group, @subject.namespace.root_ancestor)
+    end
+
     rule { can?(:owner_access) & custom_compliance_frameworks_enabled }.policy do
       enable :manage_compliance_framework
+      enable :read_compliance_framework
+    end
+
+    rule { read_root_group & custom_compliance_frameworks_enabled }.policy do
+      enable :read_compliance_framework
     end
 
     rule { can?(:owner_access) & group_level_compliance_pipeline_enabled }.policy do

ee/spec/factories/compliance_management/frameworks.rb

@@ -2,7 +2,7 @@
 
 FactoryBot.define do
   factory :compliance_framework, class: 'ComplianceManagement::Framework' do
-    namespace
+    association :namespace, factory: :group
 
     name { 'GDPR' }
     description { 'The General Data Protection Regulation (GDPR) is a regulation in EU law on data protection and privacy in the European Union (EU) and the European Economic Area (EEA).' }

ee/spec/graphql/mutations/compliance_management/frameworks/destroy_spec.rb

@@ -5,13 +5,18 @@ require 'spec_helper'
 RSpec.describe Mutations::ComplianceManagement::Frameworks::Destroy do
   include GraphqlHelpers
 
-  let_it_be(:framework) { create(:compliance_framework) }
+  let_it_be(:namespace) { create(:group) }
+  let_it_be(:framework) { create(:compliance_framework, namespace: namespace) }
 
-  let(:user) { framework.namespace.owner }
+  let(:user) { create(:user) }
   let(:mutation) { described_class.new(object: nil, context: { current_user: user }, field: nil) }
 
   subject { mutation.resolve(id: global_id_of(framework)) }
 
+  before do
+    namespace.add_owner(user)
+  end
+
   shared_examples 'a compliance framework that cannot be found' do
     it 'raises an error' do
       expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)

ee/spec/graphql/mutations/compliance_management/frameworks/update_spec.rb

@@ -5,9 +5,10 @@ require 'spec_helper'
 RSpec.describe Mutations::ComplianceManagement::Frameworks::Update do
   include GraphqlHelpers
 
-  let_it_be(:framework) { create(:compliance_framework) }
+  let_it_be(:namespace) { create(:group) }
+  let_it_be(:framework) { create(:compliance_framework, namespace: namespace) }
 
-  let(:user) { framework.namespace.owner }
+  let(:user) { create(:user) }
   let(:mutation) { described_class.new(object: nil, context: { current_user: user }, field: nil) }
   let(:params) do
     {
@@ -17,6 +18,10 @@ RSpec.describe Mutations::ComplianceManagement::Frameworks::Update do
     }
   end
 
+  before do
+    namespace.add_owner(user)
+  end
+
   subject { mutation.resolve(id: global_id_of(framework), params: params) }
 
   context 'feature is licensed' do
@@ -38,7 +43,9 @@ RSpec.describe Mutations::ComplianceManagement::Frameworks::Update do
       end
 
       context 'current_user is not authorized to update framework' do
-        let_it_be(:user) { create(:user) }
+        before do
+          namespace.update!(owners: [])
+        end
 
         it 'raises an error' do
           expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)

ee/spec/policies/compliance_management/framework_policy_spec.rb

@@ -3,47 +3,63 @@
 require 'spec_helper'
 
 RSpec.describe ComplianceManagement::FrameworkPolicy do
-  let_it_be_with_refind(:framework) { create(:compliance_framework) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:group) { create(:group, :private) }
 
-  let(:user) { framework.namespace.owner }
+  let_it_be_with_refind(:framework) { create(:compliance_framework, namespace: group) }
 
   subject { described_class.new(user, framework) }
 
+  shared_examples 'full access to compliance framework administration' do
+    it { is_expected.to be_allowed(:manage_compliance_framework) }
+    it { is_expected.to be_allowed(:read_compliance_framework) }
+    it { is_expected.to be_allowed(:manage_group_level_compliance_pipeline_config) }
+  end
+
+  shared_examples 'no access to compliance framework administration' do
+    it { is_expected.to be_disallowed(:manage_compliance_framework) }
+    it { is_expected.to be_disallowed(:read_compliance_framework) }
+    it { is_expected.to be_disallowed(:manage_group_level_compliance_pipeline_config) }
+  end
+
   context 'feature is licensed' do
     before do
       stub_licensed_features(custom_compliance_frameworks: true, evaluate_group_level_compliance_pipeline: true)
     end
 
-    context 'user is namespace owner' do
-      it { is_expected.to be_allowed(:manage_compliance_framework) }
-      it { is_expected.to be_allowed(:manage_group_level_compliance_pipeline_config) }
-    end
-
     context 'user is group owner' do
-      let_it_be(:group) { create(:group) }
-      let_it_be(:framework) { create(:compliance_framework, namespace: group) }
-      let_it_be(:user) { create(:user) }
-
       before do
         group.add_owner(user)
       end
 
-      it { is_expected.to be_allowed(:manage_compliance_framework) }
-      it { is_expected.to be_allowed(:manage_group_level_compliance_pipeline_config) }
+      it_behaves_like 'full access to compliance framework administration'
     end
 
-    context 'user is not namespace owner' do
-      let(:user) { build(:user) }
+    context 'user is not a member of the namespace' do
+      let(:user) { create(:user) }
 
-      it { is_expected.to be_disallowed(:manage_compliance_framework) }
-      it { is_expected.to be_disallowed(:manage_group_level_compliance_pipeline_config) }
+      it_behaves_like 'no access to compliance framework administration'
     end
 
     context 'user is an admin', :enable_admin_mode do
       let(:user) { build(:admin) }
 
-      it { is_expected.to be_allowed(:manage_compliance_framework) }
-      it { is_expected.to be_allowed(:manage_group_level_compliance_pipeline_config) }
+      it_behaves_like 'full access to compliance framework administration'
+    end
+
+    context 'user is subgroup member but not the owner of the root namespace' do
+      let_it_be(:user) { create(:user) }
+
+      let(:subgroup) { create(:group, :private, parent: group) }
+
+      before do
+        group.add_developer(user)
+        subgroup.add_maintainer(user)
+      end
+
+      it { is_expected.to be_allowed(:read_compliance_framework) }
+      it { is_expected.to be_disallowed(:manage_compliance_framework) }
+      it { is_expected.to be_disallowed(:manage_group_level_compliance_pipeline_config) }
     end
   end
 
@@ -52,7 +68,6 @@ RSpec.describe ComplianceManagement::FrameworkPolicy do
       stub_licensed_features(custom_compliance_frameworks: false, evaluate_group_level_compliance_pipeline: false)
     end
 
-    it { is_expected.to be_disallowed(:manage_compliance_framework) }
-    it { is_expected.to be_disallowed(:manage_group_level_compliance_pipeline_config) }
+    it_behaves_like 'no access to compliance framework administration'
   end
 end

ee/spec/requests/api/graphql/mutations/compliance_management/frameworks/destroy_spec.rb

@@ -5,8 +5,10 @@ require 'spec_helper'
 RSpec.describe 'Delete a compliance framework' do
   include GraphqlHelpers
 
-  let_it_be(:framework) { create(:compliance_framework) }
+  let_it_be(:namespace) { create(:group) }
+  let_it_be(:framework) { create(:compliance_framework, namespace: namespace) }
 
+  let(:current_user) { create(:user) }
   let(:mutation) { graphql_mutation(:destroy_compliance_framework, { id: global_id_of(framework) }) }
 
   subject { post_graphql_mutation(mutation, current_user: current_user) }
@@ -16,8 +18,6 @@ RSpec.describe 'Delete a compliance framework' do
   end
 
   context 'feature is unlicensed' do
-    let_it_be(:current_user) { framework.namespace.owner }
-
     before do
       stub_licensed_features(custom_compliance_frameworks: false)
     end
@@ -36,7 +36,9 @@ RSpec.describe 'Delete a compliance framework' do
     end
 
     context 'current_user is namespace owner' do
-      let_it_be(:current_user) { framework.namespace.owner }
+      before do
+        namespace.add_owner(current_user)
+      end
 
       it 'has no errors' do
         subject
@@ -50,14 +52,12 @@ RSpec.describe 'Delete a compliance framework' do
     end
 
     context 'current_user is not namespace owner' do
-      let_it_be(:current_user) { create(:user) }
+      it_behaves_like 'a mutation that returns top-level errors',
+                      errors: [Gitlab::Graphql::Authorize::AuthorizeResource::RESOURCE_ACCESS_ERROR]
 
       it 'does not destroy a compliance framework' do
         expect { subject }.not_to change { ComplianceManagement::Framework.count }
       end
-
-      it_behaves_like 'a mutation that returns top-level errors',
-                      errors: [Gitlab::Graphql::Authorize::AuthorizeResource::RESOURCE_ACCESS_ERROR]
     end
   end
 end

ee/spec/requests/api/graphql/mutations/compliance_management/frameworks/update_spec.rb

@@ -5,10 +5,11 @@ require 'spec_helper'
 RSpec.describe 'Update a compliance framework' do
   include GraphqlHelpers
 
-  let_it_be(:framework) { create(:compliance_framework) }
+  let_it_be(:namespace) { create(:group) }
+  let_it_be(:framework) { create(:compliance_framework, namespace: namespace) }
 
+  let(:current_user) { create(:user) }
   let(:mutation) { graphql_mutation(:update_compliance_framework, { id: global_id_of(framework), **params }) }
-  let(:current_user) { framework.namespace.owner }
   let(:params) do
     {
       params: {
@@ -21,6 +22,10 @@ RSpec.describe 'Update a compliance framework' do
 
   subject { post_graphql_mutation(mutation, current_user: current_user) }
 
+  before do
+    namespace.add_owner(current_user)
+  end
+
   def mutation_response
     graphql_mutation_response(:update_compliance_framework)
   end
@@ -94,7 +99,9 @@ RSpec.describe 'Update a compliance framework' do
       end
 
       context 'current_user is not permitted to update framework' do
-        let_it_be(:current_user) { create(:user) }
+        before do
+          namespace.update!(owners: [])
+        end
 
         it_behaves_like 'a mutation that returns top-level errors',
                         errors: [Gitlab::Graphql::Authorize::AuthorizeResource::RESOURCE_ACCESS_ERROR]