Merge branch '349685-restrict-crm-to-reporters' into 'master'

Restrict access to crm to reporter+

See merge request gitlab-org/gitlab!77825

app/policies/group_policy.rb

@@ -123,8 +123,6 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy
     enable :read_group_member
     enable :read_custom_emoji
     enable :read_counts
-    enable :read_crm_organization
-    enable :read_crm_contact
   end
 
   rule { ~public_group & ~has_access }.prevent :read_counts
@@ -159,6 +157,8 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy
     enable :read_prometheus
     enable :read_package
     enable :read_package_settings
+    enable :read_crm_organization
+    enable :read_crm_contact
   end
 
   rule { maintainer }.policy do

doc/user/crm/index.md

@@ -14,6 +14,15 @@ With customer relations management (CRM) you can create a record of contacts
 You can use contacts and organizations to tie work to customers for billing and reporting purposes.
 To read more about what is planned for the future, see [issue 2256](https://gitlab.com/gitlab-org/gitlab/-/issues/2256).
 
+## Permissions
+
+| Permission | Guest            | Reporter | Developer, Maintainer, and Owner |
+| ---------- | ---------------- | -------- | -------------------------------- |
+| View contacts/organizations | | ✓        | ✓ |
+| View issue contacts |         | ✓        | ✓ |
+| Add/remove issue contacts |   | ✓        | ✓ |
+| Create/edit contacts/organizations | |   | ✓ |
+
 ## Enable customer relations management (CRM)
 
 To enable customer relations management in a group:
@@ -122,10 +131,6 @@ API.
 
 ### Add or remove issue contacts
 
-Prerequisites:
-
-- You must have at least the [Developer role](../permissions.md#project-members-permissions) for a group.
-
 ### Add contacts to an issue
 
 To add contacts to an issue use the `/add_contacts`

spec/policies/group_policy_spec.rb

@@ -11,8 +11,6 @@ RSpec.describe GroupPolicy do
 
     it do
       expect_allowed(:read_group)
-      expect_allowed(:read_crm_organization)
-      expect_allowed(:read_crm_contact)
       expect_allowed(:read_counts)
       expect_allowed(*read_group_permissions)
       expect_disallowed(:upload_file)
@@ -21,11 +19,13 @@ RSpec.describe GroupPolicy do
       expect_disallowed(*maintainer_permissions)
       expect_disallowed(*owner_permissions)
       expect_disallowed(:read_namespace)
+      expect_disallowed(:read_crm_organization)
+      expect_disallowed(:read_crm_contact)
     end
   end
 
   context 'with no user and public project' do
-    let(:project) { create(:project, :public) }
+    let(:project) { create(:project, :public, group: create(:group, :crm_enabled)) }
     let(:current_user) { nil }
 
     before do
@@ -41,7 +41,7 @@ RSpec.describe GroupPolicy do
   end
 
   context 'with foreign user and public project' do
-    let(:project) { create(:project, :public) }
+    let(:project) { create(:project, :public, group: create(:group, :crm_enabled)) }
     let(:current_user) { create(:user) }
 
     before do
@@ -67,7 +67,7 @@ RSpec.describe GroupPolicy do
     it { expect_allowed(*read_group_permissions) }
 
     context 'in subgroups' do
-      let(:subgroup) { create(:group, :private, parent: group) }
+      let(:subgroup) { create(:group, :private, :crm_enabled, parent: group) }
       let(:project) { create(:project, namespace: subgroup) }
 
       it { expect_allowed(*read_group_permissions) }
@@ -235,7 +235,7 @@ RSpec.describe GroupPolicy do
 
   describe 'private nested group use the highest access level from the group and inherited permissions' do
     let_it_be(:nested_group) do
-      create(:group, :private, :owner_subgroup_creation_only, parent: group)
+      create(:group, :private, :owner_subgroup_creation_only, :crm_enabled, parent: group)
     end
 
     before_all do
@@ -342,7 +342,7 @@ RSpec.describe GroupPolicy do
       let(:current_user) { owner }
 
       context 'when the group share_with_group_lock is enabled' do
-        let(:group) { create(:group, share_with_group_lock: true, parent: parent) }
+        let(:group) { create(:group, :crm_enabled, share_with_group_lock: true, parent: parent) }
 
         before do
           group.add_owner(owner)
@@ -350,10 +350,10 @@ RSpec.describe GroupPolicy do
 
         context 'when the parent group share_with_group_lock is enabled' do
           context 'when the group has a grandparent' do
-            let(:parent) { create(:group, share_with_group_lock: true, parent: grandparent) }
+            let(:parent) { create(:group, :crm_enabled, share_with_group_lock: true, parent: grandparent) }
 
             context 'when the grandparent share_with_group_lock is enabled' do
-              let(:grandparent) { create(:group, share_with_group_lock: true) }
+              let(:grandparent) { create(:group, :crm_enabled, share_with_group_lock: true) }
 
               context 'when the current_user owns the parent' do
                 before do
@@ -379,7 +379,7 @@ RSpec.describe GroupPolicy do
             end
 
             context 'when the grandparent share_with_group_lock is disabled' do
-              let(:grandparent) { create(:group) }
+              let(:grandparent) { create(:group, :crm_enabled) }
 
               context 'when the current_user owns the parent' do
                 before do
@@ -396,7 +396,7 @@ RSpec.describe GroupPolicy do
           end
 
           context 'when the group does not have a grandparent' do
-            let(:parent) { create(:group, share_with_group_lock: true) }
+            let(:parent) { create(:group, :crm_enabled, share_with_group_lock: true) }
 
             context 'when the current_user owns the parent' do
               before do
@@ -413,7 +413,7 @@ RSpec.describe GroupPolicy do
         end
 
         context 'when the parent group share_with_group_lock is disabled' do
-          let(:parent) { create(:group) }
+          let(:parent) { create(:group, :crm_enabled) }
 
           it { expect_allowed(:change_share_with_group_lock) }
         end
@@ -698,7 +698,7 @@ RSpec.describe GroupPolicy do
   end
 
   it_behaves_like 'clusterable policies' do
-    let(:clusterable) { create(:group) }
+    let(:clusterable) { create(:group, :crm_enabled) }
     let(:cluster) do
       create(:cluster,
              :provided_by_gcp,
@@ -708,7 +708,7 @@ RSpec.describe GroupPolicy do
   end
 
   describe 'update_max_artifacts_size' do
-    let(:group) { create(:group, :public) }
+    let(:group) { create(:group, :public, :crm_enabled) }
 
     context 'when no user' do
       let(:current_user) { nil }
@@ -738,7 +738,7 @@ RSpec.describe GroupPolicy do
   end
 
   describe 'design activity' do
-    let_it_be(:group) { create(:group, :public) }
+    let_it_be(:group) { create(:group, :public, :crm_enabled) }
 
     let(:current_user) { nil }
 
@@ -935,8 +935,6 @@ RSpec.describe GroupPolicy do
 
       it { is_expected.to be_allowed(:read_package) }
       it { is_expected.to be_allowed(:read_group) }
-      it { is_expected.to be_allowed(:read_crm_organization) }
-      it { is_expected.to be_allowed(:read_crm_contact) }
       it { is_expected.to be_disallowed(:create_package) }
     end
 
@@ -946,8 +944,6 @@ RSpec.describe GroupPolicy do
       it { is_expected.to be_allowed(:create_package) }
       it { is_expected.to be_allowed(:read_package) }
       it { is_expected.to be_allowed(:read_group) }
-      it { is_expected.to be_allowed(:read_crm_organization) }
-      it { is_expected.to be_allowed(:read_crm_contact) }
       it { is_expected.to be_disallowed(:destroy_package) }
     end
 
@@ -967,7 +963,7 @@ RSpec.describe GroupPolicy do
   it_behaves_like 'Self-managed Core resource access tokens'
 
   context 'support bot' do
-    let_it_be(:group) { create(:group, :private) }
+    let_it_be(:group) { create(:group, :private, :crm_enabled) }
     let_it_be(:current_user) { User.support_bot }
 
     before do
@@ -977,7 +973,7 @@ RSpec.describe GroupPolicy do
     it { expect_disallowed(:read_label) }
 
     context 'when group hierarchy has a project with service desk enabled' do
-      let_it_be(:subgroup) { create(:group, :private, parent: group) }
+      let_it_be(:subgroup) { create(:group, :private, :crm_enabled, parent: group) }
       let_it_be(:project) { create(:project, group: subgroup, service_desk_enabled: true) }
 
       it { expect_allowed(:read_label) }
@@ -1170,7 +1166,7 @@ RSpec.describe GroupPolicy do
   end
 
   context 'when crm_enabled is false' do
-    let(:group) { create(:group) }
+    let(:group) { create(:group, :crm_enabled) }
     let(:current_user) { owner }
 
     it { is_expected.to be_disallowed(:read_crm_contact) }

spec/requests/groups/crm/contacts_controller_spec.rb

@@ -73,7 +73,7 @@ RSpec.describe Groups::Crm::ContactsController do
       let(:group) { create(:group, :public, :crm_enabled) }
 
       context 'with anonymous user' do
-        it_behaves_like 'ok response with index template'
+        it_behaves_like 'response with 404 status'
       end
     end
   end

spec/requests/groups/crm/organizations_controller_spec.rb

@@ -73,7 +73,7 @@ RSpec.describe Groups::Crm::OrganizationsController do
       let(:group) { create(:group, :public, :crm_enabled) }
 
       context 'with anonymous user' do
-        it_behaves_like 'ok response with index template'
+        it_behaves_like 'response with 404 status'
       end
     end
   end

spec/services/issues/update_service_spec.rb

@@ -22,10 +22,10 @@ RSpec.describe Issues::UpdateService, :mailer do
   end
 
   before_all do
-    project.add_maintainer(user)
-    project.add_developer(user2)
-    project.add_developer(user3)
-    project.add_guest(guest)
+    group.add_maintainer(user)
+    group.add_developer(user2)
+    group.add_developer(user3)
+    group.add_guest(guest)
   end
 
   describe 'execute' do

spec/support/shared_contexts/policies/group_policy_shared_context.rb

@@ -28,6 +28,8 @@ RSpec.shared_context 'GroupPolicy context' do
         read_metrics_dashboard_annotation
         read_prometheus
         read_package_settings
+        read_crm_contact
+        read_crm_organization
       ]
   end