Allow all users within a group to view all compliance frameworks

This fixes a bug where maintainers of a
subgroup are unable to see a list of compliance
frameworks belonging to the root group.

We've split the :manage_compliance_framework policy in to two:

* :manage_compliance_framework for owners of root groups
  to add/update/delete compliance frameworks.
* :read_compliance_framework for _all_ users of
  a group namespace to allow them to read the details
  of a compliance framework.

There's no reason to hide this information from users
within a group as they will see the name, description,
pipeline configuration through the UI anyway.

Maintainers of subgroups that do not have owner access
to the root namespace will now no longer see an error in the
group settings page too.

Also changes the FrameworkResolver GraphQL resolver
to use :read_compliance_framework rather than
:admin_compliance_framework

EE: true
Changelog: fixed

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,19 +5,23 @@ 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) }
 
+  before do
+    namespace.add_owner(current_user)
+  end
+
   def mutation_response
     graphql_mutation_response(:destroy_compliance_framework)
   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,8 +40,6 @@ RSpec.describe 'Delete a compliance framework' do
     end
 
     context 'current_user is namespace owner' do
-      let_it_be(:current_user) { framework.namespace.owner }
-
       it 'has no errors' do
         subject
 
@@ -48,16 +50,14 @@ RSpec.describe 'Delete a compliance framework' do
         expect { subject }.to change { ComplianceManagement::Framework.count }.by(-1)
       end
     end
+  end
 
-    context 'current_user is not namespace owner' do
-      let_it_be(:current_user) { create(:user) }
-
-      it 'does not destroy a compliance framework' do
-        expect { subject }.not_to change { ComplianceManagement::Framework.count }
-      end
+  context 'current_user is not namespace owner' do
+    it_behaves_like 'a mutation that returns top-level errors',
+                    errors: [Gitlab::Graphql::Authorize::AuthorizeResource::RESOURCE_ACCESS_ERROR]
 
-      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
   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]