Merge branch '333978-limit-changing-security-policy-project-to-project-owners' into 'master'

Limit changing security policy project to project owners

See merge request gitlab-org/gitlab!65123

doc/user/application_security/policies/index.md

@@ -191,6 +191,10 @@ changes.
 You can always change the **Security policy project** by navigating to your project's
 **Security & Compliance > Policies** and modifying the selected project.
 
+NOTE:
+Only project Owners have the [permissions](../../permissions.md#project-members-permissions)
+to select Security Policy Project.
+
 ## Roadmap
 
 See the [Category Direction page](https://about.gitlab.com/direction/protect/container_network_security/)

doc/user/permissions.md

@@ -168,6 +168,8 @@ The following table lists project permissions available for each role:
 | Manage Project Operations                         |         |            |             | ✓        | ✓      |
 | Manage Terraform state                            |         |            |             | ✓        | ✓      |
 | Manage license policy **(ULTIMATE)**              |         |            |             | ✓        | ✓      |
+| Manage security policy **(ULTIMATE)**             |         |            | ✓           | ✓        | ✓      |
+| Create or assign security policy project **(ULTIMATE)**     |         |            |             |          | ✓      |
 | Edit comments (posted by any user)                |         |            |             | ✓        | ✓      |
 | Reposition comments on images (posted by any user)|✓ (*10*) | ✓ (*10*)   |  ✓ (*10*)   | ✓        | ✓      |
 | Manage Error Tracking                             |         |            |             | ✓        | ✓      |

ee/app/controllers/projects/security/policies_controller.rb

@@ -5,9 +5,12 @@ module Projects
     class PoliciesController < Projects::ApplicationController
       include SecurityAndCompliancePermissions
 
+      before_action :authorize_security_orchestration_policies!
+      before_action :authorize_update_security_orchestration_policy_project!, only: [:assign]
+
       before_action do
         push_frontend_feature_flag(:security_orchestration_policies_configuration, project)
-        check_permissions!
+        check_feature_flag!
       end
 
       feature_category :security_orchestration
@@ -32,8 +35,8 @@ module Projects
 
       private
 
-      def check_permissions!
-        render_404 unless Feature.enabled?(:security_orchestration_policies_configuration, project) && can?(current_user, :security_orchestration_policies, project)
+      def check_feature_flag!
+        render_404 if Feature.disabled?(:security_orchestration_policies_configuration, project)
       end
 
       def policy_project_params

ee/app/graphql/mutations/security_policy/assign_security_policy_project.rb

@@ -7,7 +7,7 @@ module Mutations
 
       graphql_name 'SecurityPolicyProjectAssign'
 
-      authorize :security_orchestration_policies
+      authorize :update_security_orchestration_policy_project
 
       argument :project_path, GraphQL::ID_TYPE,
                required: true,

ee/app/graphql/mutations/security_policy/create_security_policy_project.rb

@@ -7,7 +7,7 @@ module Mutations
 
       graphql_name 'SecurityPolicyProjectCreate'
 
-      authorize :security_orchestration_policies
+      authorize :update_security_orchestration_policy_project
 
       argument :project_path, GraphQL::ID_TYPE,
                required: true,

ee/app/helpers/ee/projects_helper.rb

@@ -195,6 +195,10 @@ module EE
       end
     end
 
+    def can_update_security_orchestration_policy_project?(project)
+      can?(current_user, :update_security_orchestration_policy_project, project)
+    end
+
     def can_create_feedback?(project, feedback_type)
       feedback = Vulnerabilities::Feedback.new(project: project, feedback_type: feedback_type)
       can?(current_user, :create_vulnerability_feedback, feedback)

ee/app/policies/ee/project_policy.rb

@@ -195,6 +195,10 @@ module EE
         enable :security_orchestration_policies
       end
 
+      rule { security_orchestration_policies_enabled & can?(:owner_access) }.policy do
+        enable :update_security_orchestration_policy_project
+      end
+
       rule { security_dashboard_enabled & can?(:developer_access) }.policy do
         enable :read_security_resource
         enable :read_vulnerability_scanner

ee/app/views/projects/security/policies/show.html.haml

@@ -7,8 +7,11 @@
     = s_('SecurityOrchestration|Security policy project')
   %p
     = project_select_tag('orchestration[policy_project_id]', class: 'hidden-filter-value', toggle_class: 'js-project-search js-project-filter js-filter-submit', dropdown_class: 'dropdown-menu-selectable dropdown-menu-project js-filter-submit',
-        placeholder: _('Select project'), idAttribute: 'id', data: { order_by: 'last_activity_at', idattribute: 'id', simple_filter: true, allow_clear: true, include_groups: false, include_projects_in_subgroups: true, user_id: current_user.id }, value: @assigned_policy_id)
+        placeholder: _('Select project'), idAttribute: 'id', disabled: !can_update_security_orchestration_policy_project?(@project), data: { order_by: 'last_activity_at', idattribute: 'id', simple_filter: true, allow_clear: true, include_groups: false, include_projects_in_subgroups: true, user_id: current_user.id }, value: @assigned_policy_id)
     .text-muted
       = html_escape(s_('SecurityOrchestration|A security policy project can be used enforce policies for a given project, group, or instance. It allows you to specify security policies that are important to you  and enforce them with every commit.')) % { code_open: '<code>'.html_safe, code_close: '</code>'.html_safe }
       = link_to _('More information'), help_page_path('user/project/clusters/protect/container_network_security/quick_start_guide'), target: '_blank'
-  = field.submit _('Save changes'), class: 'btn gl-button btn-success'
+  - if can_update_security_orchestration_policy_project?(@project)
+    = field.submit _('Save changes'), class: 'btn gl-button btn-success'
+  - else
+    = field.submit _('Save changes'), class: 'btn gl-button btn-success has-tooltip', disabled: true, title: _('Only owners can update Security Policy Project')

ee/spec/graphql/mutations/security_policy/assign_security_policy_project_spec.rb

@@ -3,14 +3,17 @@
 require 'spec_helper'
 
 RSpec.describe Mutations::SecurityPolicy::AssignSecurityPolicyProject do
-  let(:mutation) { described_class.new(object: nil, context: { current_user: user }, field: nil) }
+  let(:mutation) { described_class.new(object: nil, context: { current_user: current_user }, field: nil) }
 
   describe '#resolve' do
+    let_it_be(:owner) { create(:user) }
     let_it_be(:user) { create(:user) }
-    let_it_be(:project) { create(:project, namespace: user.namespace) }
+    let_it_be(:project) { create(:project, namespace: owner.namespace) }
     let_it_be(:policy_project) { create(:project) }
     let_it_be(:policy_project_id) { GitlabSchema.id_from_object(policy_project) }
 
+    let(:current_user) { owner }
+
     subject { mutation.resolve(project_path: project.full_path, security_policy_project_id: policy_project_id) }
 
     context 'when feature is enabled and permission is set for user' do
@@ -19,12 +22,26 @@ RSpec.describe Mutations::SecurityPolicy::AssignSecurityPolicyProject do
         stub_feature_flags(security_orchestration_policies_configuration: true)
       end
 
-      it 'assigns the security policy project' do
-        result = subject
+      context 'when user is an owner of the project' do
+        it 'assigns the security policy project' do
+          result = subject
+
+          expect(result[:errors]).to be_empty
+          expect(project.security_orchestration_policy_configuration).not_to be_nil
+          expect(project.security_orchestration_policy_configuration.security_policy_management_project).to eq(policy_project)
+        end
+      end
+
+      context 'when user is not an owner' do
+        let(:current_user) { user }
+
+        before do
+          project.add_maintainer(user)
+        end
 
-        expect(result[:errors]).to be_empty
-        expect(project.security_orchestration_policy_configuration).not_to be_nil
-        expect(project.security_orchestration_policy_configuration.security_policy_management_project).to eq(policy_project)
+        it 'raises exception' do
+          expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
+        end
       end
     end
 
@@ -47,7 +64,7 @@ RSpec.describe Mutations::SecurityPolicy::AssignSecurityPolicyProject do
       end
     end
 
-    context 'when permission is not enabled' do
+    context 'when feature is not licensed' do
       before do
         stub_licensed_features(security_orchestration_policies: false)
       end

ee/spec/graphql/mutations/security_policy/create_security_policy_project_spec.rb

@@ -2,27 +2,44 @@
 require 'spec_helper'
 
 RSpec.describe Mutations::SecurityPolicy::CreateSecurityPolicyProject do
-  let(:mutation) { described_class.new(object: nil, context: { current_user: user }, field: nil) }
+  let(:mutation) { described_class.new(object: nil, context: { current_user: current_user }, field: nil) }
 
   describe '#resolve' do
+    let_it_be(:owner) { create(:user) }
     let_it_be(:user) { create(:user) }
-    let_it_be(:project) { create(:project, namespace: user.namespace) }
+    let_it_be(:project) { create(:project, namespace: owner.namespace) }
+
+    let(:current_user) { owner }
 
     subject { mutation.resolve(project_path: project.full_path) }
 
     context 'when feature is enabled and permission is set for user' do
       before do
-        project.add_maintainer(user)
-
         stub_licensed_features(security_orchestration_policies: true)
         stub_feature_flags(security_orchestration_policies_configuration: true)
       end
 
-      it 'returns project' do
-        result = subject
+      context 'when user is an owner of the project' do
+        let(:current_user) { owner }
+
+        it 'returns project' do
+          result = subject
+
+          expect(result[:errors]).to be_empty
+          expect(result[:project]).to eq(Project.last)
+        end
+      end
+
+      context 'when user is not an owner' do
+        let(:current_user) { user }
+
+        before do
+          project.add_maintainer(user)
+        end
 
-        expect(result[:errors]).to be_empty
-        expect(result[:project]).to eq(Project.last)
+        it 'raises exception' do
+          expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
+        end
       end
     end
 
@@ -37,7 +54,7 @@ RSpec.describe Mutations::SecurityPolicy::CreateSecurityPolicyProject do
       end
     end
 
-    context 'when permission is not enabled' do
+    context 'when feature is not licensed' do
       before do
         stub_licensed_features(security_orchestration_policies: false)
       end

ee/spec/helpers/projects_helper_spec.rb

@@ -20,6 +20,24 @@ RSpec.describe ProjectsHelper do
     end
   end
 
+  describe '#can_update_security_orchestration_policy_project?' do
+    let(:owner) { project.owner }
+
+    before do
+      allow(helper).to receive(:current_user) { owner }
+    end
+
+    it 'returns false when user cannot update security orchestration policy project' do
+      allow(helper).to receive(:can?).with(owner, :update_security_orchestration_policy_project, project) { false }
+      expect(helper.can_update_security_orchestration_policy_project?(project)).to eq false
+    end
+
+    it 'returns true when user can update security orchestration policy project' do
+      allow(helper).to receive(:can?).with(owner, :update_security_orchestration_policy_project, project) { true }
+      expect(helper.can_update_security_orchestration_policy_project?(project)).to eq true
+    end
+  end
+
   describe '#can_import_members?' do
     let(:owner) { project.owner }
 

ee/spec/policies/project_policy_spec.rb

@@ -748,13 +748,25 @@ RSpec.describe ProjectPolicy do
       stub_licensed_features(security_orchestration_policies: true)
     end
 
-    context 'with developer or higher role' do
-      where(role: %w[owner maintainer developer])
+    context 'with developer or maintainer role' do
+      where(role: %w[maintainer developer])
 
       with_them do
         let(:current_user) { public_send(role) }
 
         it { is_expected.to be_allowed(:security_orchestration_policies) }
+        it { is_expected.to be_disallowed(:update_security_orchestration_policy_project) }
+      end
+    end
+
+    context 'with owner role' do
+      where(role: %w[owner])
+
+      with_them do
+        let(:current_user) { public_send(role) }
+
+        it { is_expected.to be_allowed(:security_orchestration_policies) }
+        it { is_expected.to be_allowed(:update_security_orchestration_policy_project) }
       end
     end
   end

ee/spec/requests/projects/security/policies_controller_spec.rb

@@ -3,8 +3,9 @@
 require 'spec_helper'
 
 RSpec.describe Projects::Security::PoliciesController, type: :request do
-  let_it_be(:project, reload: true) { create(:project) }
+  let_it_be(:owner) { create(:user) }
   let_it_be(:user) { create(:user) }
+  let_it_be(:project, reload: true) { create(:project, namespace: owner.namespace) }
 
   before do
     project.add_developer(user)
@@ -45,17 +46,32 @@ RSpec.describe Projects::Security::PoliciesController, type: :request do
       stub_licensed_features(security_orchestration_policies: true)
     end
 
-    it 'assigns policy project to project' do
-      post assign_project_security_policy_url(project), params: { orchestration: { policy_project_id: policy_project.id } }
+    context 'when user is not an owner of the project' do
+      it 'returns error message' do
+        post assign_project_security_policy_url(project), params: { orchestration: { policy_project_id: policy_project.id } }
 
-      expect(response).to redirect_to(project_security_policy_url(project))
-      expect(project.security_orchestration_policy_configuration.security_policy_management_project_id).to eq(policy_project.id)
+        expect(response).to have_gitlab_http_status(:not_found)
+        expect(response).not_to render_template('new')
+      end
     end
 
-    it 'returns error message for invalid input' do
-      post assign_project_security_policy_url(project), params: { orchestration: { policy_project_id: nil } }
+    context 'when user is an owner of the project' do
+      before do
+        login_as(owner)
+      end
+
+      it 'assigns policy project to project' do
+        post assign_project_security_policy_url(project), params: { orchestration: { policy_project_id: policy_project.id } }
 
-      expect(flash[:alert]).to eq 'Policy project doesn\'t exist'
+        expect(response).to redirect_to(project_security_policy_url(project))
+        expect(project.security_orchestration_policy_configuration.security_policy_management_project_id).to eq(policy_project.id)
+      end
+
+      it 'returns error message for invalid input' do
+        post assign_project_security_policy_url(project), params: { orchestration: { policy_project_id: nil } }
+
+        expect(flash[:alert]).to eq 'Policy project doesn\'t exist'
+      end
     end
   end
 end

locale/gitlab.pot

@@ -22972,6 +22972,9 @@ msgstr ""
 msgid "Only include features new to your current subscription tier."
 msgstr ""
 
+msgid "Only owners can update Security Policy Project"
+msgstr ""
+
 msgid "Only policy:"
 msgstr ""