From d5b681162019e87081b0e9a5e35f0a423e1f941c Mon Sep 17 00:00:00 2001 From: Hitesh Raghuvanshi Date: Mon, 1 Jul 2024 20:03:32 +0530 Subject: [PATCH 1/7] Added mutation for updating frameworks of projects Changelog: added EE: true --- config/bounded_contexts.yml | 2 +- doc/api/graphql/reference/index.md | 22 ++++ doc/user/compliance/audit_event_types.md | 2 + ee/app/graphql/ee/types/mutation_type.rb | 1 + .../projects/update_compliance_frameworks.rb | 48 ++++++++ .../compliance_framework/project_settings.rb | 4 +- .../frameworks/update_project_service.rb | 103 ++++++++++++++++++ .../types/compliance_framework_added.yml | 10 ++ .../types/compliance_framework_removed.yml | 10 ++ .../update_compliance_frameworks_spec.rb | 69 ++++++++++++ .../update_compliance_frameworks_spec.rb | 86 +++++++++++++++ .../frameworks/update_project_service_spec.rb | 96 ++++++++++++++++ 12 files changed, 451 insertions(+), 2 deletions(-) create mode 100644 ee/app/graphql/mutations/projects/update_compliance_frameworks.rb create mode 100644 ee/app/services/compliance_management/frameworks/update_project_service.rb create mode 100644 ee/config/audit_events/types/compliance_framework_added.yml create mode 100644 ee/config/audit_events/types/compliance_framework_removed.yml create mode 100644 ee/spec/graphql/mutations/projects/update_compliance_frameworks_spec.rb create mode 100644 ee/spec/requests/api/graphql/mutations/projects/update_compliance_frameworks_spec.rb create mode 100644 ee/spec/services/compliance_management/frameworks/update_project_service_spec.rb diff --git a/config/bounded_contexts.yml b/config/bounded_contexts.yml index 3a1d223210f358..96b3668905f001 100644 --- a/config/bounded_contexts.yml +++ b/config/bounded_contexts.yml @@ -99,7 +99,7 @@ domains: feature_categories: - code_suggestions - Compliance: + ComplianceManagement: description: feature_categories: - compliance_management diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index ca2872343dc880..443b9a80c320f7 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -7837,6 +7837,28 @@ Input type: `projectTextReplaceInput` | `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | | `errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | +### `Mutation.projectUpdateComplianceFrameworks` + +Update compliance frameworks for a project. + +Input type: `ProjectUpdateComplianceFrameworksInput` + +#### Arguments + +| Name | Type | Description | +| ---- | ---- | ----------- | +| `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | +| `complianceFrameworkIds` | [`[ComplianceManagementFrameworkID!]!`](#compliancemanagementframeworkid) | IDs of the compliance framework to update for the project. | +| `projectId` | [`ProjectID!`](#projectid) | ID of the project to change the compliance framework of. | + +#### Fields + +| Name | Type | Description | +| ---- | ---- | ----------- | +| `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | +| `errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | +| `project` | [`Project`](#project) | Project after mutation. | + ### `Mutation.prometheusIntegrationCreate` Input type: `PrometheusIntegrationCreateInput` diff --git a/doc/user/compliance/audit_event_types.md b/doc/user/compliance/audit_event_types.md index 1b95280b1ad4ae..a1ddf4b3701067 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -134,8 +134,10 @@ Audit event types belong to the following product categories. | [`allow_committer_approval_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/102256) | Event triggered on updating prevent merge request approval from committers from group merge request setting| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.6](https://gitlab.com/gitlab-org/gitlab/-/issues/373949) | Group | | [`allow_overrides_to_approver_list_per_merge_request_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/102256) | Event triggered on updating prevent users from modifying MR approval rules in merge requests from group merge request setting| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.6](https://gitlab.com/gitlab-org/gitlab/-/issues/373949) | Group | | [`audit_events_streaming_headers_update`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/92068) | Triggered when a streaming header for audit events is updated| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.3](https://gitlab.com/gitlab-org/gitlab/-/issues/366350) | Group | +| [`compliance_framework_added`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/157893) | Triggered when a framework label is added to a project.| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [17.2](https://gitlab.com/gitlab-org/gitlab/-/issues/464160) | Project | | [`compliance_framework_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/65343) | Triggered when a framework gets removed from a project| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [14.1](https://gitlab.com/gitlab-org/gitlab/-/issues/329362) | Project | | [`compliance_framework_id_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/94711) | audit when compliance framework ID is updated| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.7](https://gitlab.com/gitlab-org/gitlab/-/issues/369310) | Project | +| [`compliance_framework_removed`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/157893) | Triggered when a framework label is removed from a project.| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [17.2](https://gitlab.com/gitlab-org/gitlab/-/issues/464160) | Project | | [`create_compliance_framework`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/74292) | Triggered on successful compliance framework creation| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [14.6](https://gitlab.com/gitlab-org/gitlab/-/issues/340649) | Group | | [`create_status_check`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/84624) | Event triggered when an external status check is created| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.9](https://gitlab.com/gitlab-org/gitlab/-/issues/355805) | Project | | [`delete_status_check`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/84624) | Event triggered when an external status check is deleted| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.9](https://gitlab.com/gitlab-org/gitlab/-/issues/355805) | Project | diff --git a/ee/app/graphql/ee/types/mutation_type.rb b/ee/app/graphql/ee/types/mutation_type.rb index ba1db95cf19ebb..4d2b62491ea6f9 100644 --- a/ee/app/graphql/ee/types/mutation_type.rb +++ b/ee/app/graphql/ee/types/mutation_type.rb @@ -212,6 +212,7 @@ def self.authorization_scopes alpha: { milestone: '17.2' } mount_mutation ::Mutations::Ai::SelfHostedModels::Update, alpha: { milestone: '17.2' } + mount_mutation ::Mutations::Projects::UpdateComplianceFrameworks prepend(Types::DeprecatedMutations) end diff --git a/ee/app/graphql/mutations/projects/update_compliance_frameworks.rb b/ee/app/graphql/mutations/projects/update_compliance_frameworks.rb new file mode 100644 index 00000000000000..148eca23572d0b --- /dev/null +++ b/ee/app/graphql/mutations/projects/update_compliance_frameworks.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +module Mutations + module Projects + class UpdateComplianceFrameworks < BaseMutation + graphql_name 'ProjectUpdateComplianceFrameworks' + description 'Update compliance frameworks for a project.' + + authorize :admin_compliance_framework + + argument :project_id, Types::GlobalIDType[::Project], + required: true, + description: 'ID of the project to change the compliance framework of.' + + argument :compliance_framework_ids, [Types::GlobalIDType[::ComplianceManagement::Framework]], + required: true, + description: 'IDs of the compliance framework to update for the project.' + + field :project, + Types::ProjectType, + null: true, + description: "Project after mutation." + + def resolve(project_id:, compliance_framework_ids:) + project = GitlabSchema.find_by_gid(project_id).sync + + authorize!(project) + + compliance_frameworks = compliance_frameworks(compliance_framework_ids) + + service_response = ::ComplianceManagement::Frameworks::UpdateProjectService + .new(project, current_user, compliance_frameworks) + .execute + + { project: project, errors: errors_on_object(project) + service_response.errors } + end + + private + + def compliance_frameworks(compliance_framework_ids) + GitlabSchema.parse_gids( + compliance_framework_ids, expected_type: ::ComplianceManagement::Framework).map(&:find).uniq + rescue ActiveRecord::RecordNotFound => e + raise Gitlab::Graphql::Errors::ArgumentError, e + end + end + end +end diff --git a/ee/app/models/compliance_management/compliance_framework/project_settings.rb b/ee/app/models/compliance_management/compliance_framework/project_settings.rb index d0e8f1937c3bfd..988aa96ef511bb 100644 --- a/ee/app/models/compliance_management/compliance_framework/project_settings.rb +++ b/ee/app/models/compliance_management/compliance_framework/project_settings.rb @@ -4,7 +4,7 @@ module ComplianceManagement module ComplianceFramework class ProjectSettings < ApplicationRecord self.table_name = 'project_compliance_framework_settings' - self.primary_key = :project_id + self.primary_key = :id belongs_to :project belongs_to :compliance_management_framework, class_name: "ComplianceManagement::Framework", foreign_key: :framework_id @@ -13,6 +13,8 @@ class ProjectSettings < ApplicationRecord delegate :full_path, to: :project + scope :with_framework_and_project, ->(project, framework) { where(project: project, compliance_management_framework: framework) } + def self.find_or_create_by_project(project, framework) find_or_initialize_by(project: project).tap do |setting| setting.framework_id = framework.id diff --git a/ee/app/services/compliance_management/frameworks/update_project_service.rb b/ee/app/services/compliance_management/frameworks/update_project_service.rb new file mode 100644 index 00000000000000..a24fffc94712cb --- /dev/null +++ b/ee/app/services/compliance_management/frameworks/update_project_service.rb @@ -0,0 +1,103 @@ +# frozen_string_literal: true + +module ComplianceManagement + module Frameworks + class UpdateProjectService < BaseService + def initialize(project, current_user, frameworks) + @project = project + @current_user = current_user + @frameworks = frameworks + end + + def execute + return error unless permitted? + + old_frameworks = project.compliance_management_frameworks + + frameworks_to_be_added = frameworks - old_frameworks + frameworks_to_be_removed = old_frameworks - frameworks + + frameworks_to_be_added.each do |framework| + response = add_framework_setting(framework) + return response if response.respond_to?(:error?) && response.error? + end + + frameworks_to_be_removed.each do |framework| + response = remove_framework_setting(framework) + return response if response.respond_to?(:error?) && response.error? + end + + success + end + + private + + attr_reader :project, :current_user, :frameworks + + def add_framework_setting(framework) + return unless project.root_namespace.self_and_descendants_ids.include?(framework.namespace.id) + + framework_project_setting = ComplianceManagement::ComplianceFramework::ProjectSettings.new(project: project, + compliance_management_framework: framework) + + return unless framework_project_setting.save + + track_event(::Projects::ComplianceFrameworkChangedEvent::EVENT_TYPES[:added], framework) + end + + def remove_framework_setting(framework) + framework_project_setting = ComplianceManagement::ComplianceFramework::ProjectSettings + .with_framework_and_project(project, framework).first + + return unless framework_project_setting.destroy + + track_event(::Projects::ComplianceFrameworkChangedEvent::EVENT_TYPES[:removed], framework) + end + + def permitted? + can?(current_user, :admin_compliance_framework, project) + end + + def error + ServiceResponse.error(message: _('Failed to assign the framework to the project')) + end + + def success + ServiceResponse.success + end + + def track_event(event_type, framework) + publish_event(event_type, framework) + audit_event(event_type, framework) + end + + def audit_event(event_type, framework) + audit_context = { + name: "compliance_framework_#{event_type}", + author: current_user, + scope: project, + target: framework, + message: "#{event_type.capitalize} framework label #{framework.name}", + additional_details: { + framework: { + id: framework.id, + name: framework.name + } + } + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + end + + def publish_event(event_type, framework) + event = ::Projects::ComplianceFrameworkChangedEvent.new(data: { + project_id: project.id, + compliance_framework_id: framework.id, + event_type: event_type + }) + + ::Gitlab::EventStore.publish(event) + end + end + end +end diff --git a/ee/config/audit_events/types/compliance_framework_added.yml b/ee/config/audit_events/types/compliance_framework_added.yml new file mode 100644 index 00000000000000..ec18d4f6d49a26 --- /dev/null +++ b/ee/config/audit_events/types/compliance_framework_added.yml @@ -0,0 +1,10 @@ +--- +name: compliance_framework_added +description: Triggered when a framework label is added to a project. +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/464160 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/157893 +milestone: '17.2' +feature_category: compliance_management +saved_to_database: true +streamed: true +scope: [Project] diff --git a/ee/config/audit_events/types/compliance_framework_removed.yml b/ee/config/audit_events/types/compliance_framework_removed.yml new file mode 100644 index 00000000000000..cc1a168c75c0f0 --- /dev/null +++ b/ee/config/audit_events/types/compliance_framework_removed.yml @@ -0,0 +1,10 @@ +--- +name: compliance_framework_removed +description: Triggered when a framework label is removed from a project. +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/464160 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/157893 +milestone: '17.2' +feature_category: compliance_management +saved_to_database: true +streamed: true +scope: [Project] diff --git a/ee/spec/graphql/mutations/projects/update_compliance_frameworks_spec.rb b/ee/spec/graphql/mutations/projects/update_compliance_frameworks_spec.rb new file mode 100644 index 00000000000000..a81f4ed53a5512 --- /dev/null +++ b/ee/spec/graphql/mutations/projects/update_compliance_frameworks_spec.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Mutations::Projects::UpdateComplianceFrameworks, feature_category: :compliance_management do + let_it_be(:group) { create(:group) } + let_it_be(:framework) { create(:compliance_framework, :sox, namespace: group) } + let_it_be(:project) { create(:project, :repository, :with_compliance_framework, group: group) } + let_it_be(:current_user) { create(:user) } + let_it_be(:existing_framework) { project.compliance_management_frameworks.first } + + let(:mutation) { described_class.new(object: nil, context: { current_user: current_user }, field: nil) } + + subject do + mutation.resolve(project_id: GitlabSchema.id_from_object(project), + compliance_framework_ids: [GitlabSchema.id_from_object(existing_framework), + GitlabSchema.id_from_object(framework)]) + end + + shared_examples "the user cannot update the project's compliance framework" do + it 'raises an exception' do + expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) + end + end + + shared_examples "the user can update compliance frameworks of the project" do + it 'updates the compliance frameworks to the project' do + expect { subject }.to change { project.reload.compliance_management_frameworks } + .from([existing_framework]).to([framework, existing_framework]) + end + + it 'returns the project that was updated' do + expect(subject).to include(project: project) + end + end + + describe '#resolve' do + context 'when feature is licensed' do + before do + stub_licensed_features(compliance_framework: true) + end + + context 'when current_user is a project maintainer' do + before_all do + project.add_maintainer(current_user) + end + + it_behaves_like "the user cannot update the project's compliance framework" + end + + context 'when current_user is a project owner' do + before_all do + group.add_owner(current_user) + project.add_owner(current_user) + end + + it_behaves_like "the user can update compliance frameworks of the project" + end + end + + context 'when feature is unlicensed' do + before do + stub_licensed_features(compliance_framework: false) + end + + it_behaves_like "the user cannot update the project's compliance framework" + end + end +end diff --git a/ee/spec/requests/api/graphql/mutations/projects/update_compliance_frameworks_spec.rb b/ee/spec/requests/api/graphql/mutations/projects/update_compliance_frameworks_spec.rb new file mode 100644 index 00000000000000..f5f8e0d3aa06cf --- /dev/null +++ b/ee/spec/requests/api/graphql/mutations/projects/update_compliance_frameworks_spec.rb @@ -0,0 +1,86 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Update project compliance framework', feature_category: :compliance_management do + include GraphqlHelpers + + let_it_be(:namespace) { create(:group) } + let_it_be(:project) { create(:project, namespace: namespace) } + let_it_be(:framework1) { create(:compliance_framework, namespace: namespace) } + let_it_be(:framework2) { create(:compliance_framework, :sox, namespace: namespace) } + let_it_be(:current_user) { create(:user, owner_of: namespace) } + + let(:variables) do + { + project_id: GitlabSchema.id_from_object(project).to_s, + compliance_framework_ids: [ + GitlabSchema.id_from_object(framework1).to_s, + GitlabSchema.id_from_object(framework2).to_s, + GitlabSchema.id_from_object(framework1).to_s # Adding same framework twice for checking it only gets added once + ] + } + end + + let(:mutation) do + graphql_mutation(:project_update_compliance_frameworks, variables) do + <<~QL + errors + project { + complianceFrameworks { + nodes { + name + } + } + } + QL + end + end + + def mutation_response + graphql_mutation_response(:project_update_compliance_frameworks) + end + + describe '#resolve' do + context 'when feature is not available' do + before do + stub_licensed_features(compliance_framework: false) + end + + it_behaves_like 'a mutation that returns top-level errors', + errors: ['The resource that you are attempting to access does not exist ' \ + 'or you don\'t have permission to perform this action'] + end + + context 'when feature is available' do + before do + stub_licensed_features(compliance_framework: true) + end + + context 'when there is no framework associated with the project' do + it_behaves_like 'a working GraphQL mutation' + + it 'adds the frameworks' do + expect { post_graphql_mutation(mutation, current_user: current_user) }.to change { + project.reload.compliance_management_frameworks + }.from([]).to([framework1, framework2]) + end + end + + context 'when there is a framework associated with the project' do + let_it_be(:existing_framework) { create(:compliance_framework, namespace: namespace, name: 'framework3') } + + before do + create(:compliance_framework_project_setting, project: project, + compliance_management_framework: existing_framework) + end + + it 'adds the new frameworks' do + expect { post_graphql_mutation(mutation, current_user: current_user) }.to change { + project.reload.compliance_management_frameworks + }.from([existing_framework]).to([framework1, framework2]) + end + end + end + end +end diff --git a/ee/spec/services/compliance_management/frameworks/update_project_service_spec.rb b/ee/spec/services/compliance_management/frameworks/update_project_service_spec.rb new file mode 100644 index 00000000000000..491337c0002555 --- /dev/null +++ b/ee/spec/services/compliance_management/frameworks/update_project_service_spec.rb @@ -0,0 +1,96 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ComplianceManagement::Frameworks::UpdateProjectService, feature_category: :compliance_management do + describe '#execute' do + let_it_be(:user) { create(:user) } + let_it_be(:group) { create(:group) } + let_it_be_with_reload(:project) { create(:project, group: group) } + let_it_be(:framework1) { create(:compliance_framework, name: 'framework1', namespace: group) } + let_it_be(:framework2) { create(:compliance_framework, name: 'framework2', namespace: group) } + let_it_be(:framework3) { create(:compliance_framework, name: 'framework3', namespace: group) } + let_it_be(:framework4) { create(:compliance_framework, name: 'framework4', namespace: group) } + + let(:frameworks) { [framework1, framework2] } + + let(:service) { described_class.new(project, user, frameworks) } + + subject(:update_framework) { service.execute } + + context 'when compliance framework feature is available' do + before do + allow(service).to receive(:can?).with(user, :admin_compliance_framework, project).and_return(true) + end + + context 'when the input parameters are correct' do + context 'when project has no framework associated with it' do + it 'adds the framework association' do + expect { update_framework }.to change { + project.reload.compliance_management_frameworks + }.from([]).to([framework1, framework2]) + end + + it 'logs audit events' do + expect { update_framework }.to change { + AuditEvent.where("details LIKE ?", "%compliance_framework_added%").count + }.by(2) + end + + it 'publishes Projects::ComplianceFrameworkChangedEvent' do + expect(::Gitlab::EventStore).to receive(:publish) + .with(an_instance_of(::Projects::ComplianceFrameworkChangedEvent)).twice + + update_framework + end + end + + context 'when project already has some frameworks associated with it' do + before do + create(:compliance_framework_project_setting, project: project, compliance_management_framework: framework2) + create(:compliance_framework_project_setting, project: project, compliance_management_framework: framework3) + create(:compliance_framework_project_setting, project: project, compliance_management_framework: framework4) + end + + it 'adds and removes framework associations' do + expect { update_framework }.to change { + project.reload.compliance_management_frameworks + }.from([framework2, framework3, framework4]).to([framework1, framework2]) + end + + it 'logs audit events' do + expect { update_framework }.to change { + AuditEvent.where("details LIKE ?", "%compliance_framework_added%").count + }.by(1).and change { + AuditEvent.where("details LIKE ?", "%compliance_framework_removed%").count + }.by(2) + end + + it 'publishes Projects::ComplianceFrameworkChangedEvent' do + expect(::Gitlab::EventStore).to receive(:publish) + .with(an_instance_of(::Projects::ComplianceFrameworkChangedEvent)).exactly(3).times + + update_framework + end + end + end + end + + context 'when compliance framework feature is unavailable' do + before do + stub_licensed_features(compliance_framework: false) + end + + before_all do + group.add_owner(user) + end + + it 'returns an error response' do + response = update_framework + + expect(response).to be_error + expect(response.message).to eq('Failed to assign the framework to the project') + end + end + end +end -- GitLab From 6363a76db97b4eb8a634d2dc50f88c2a20497835 Mon Sep 17 00:00:00 2001 From: Hitesh Raghuvanshi Date: Mon, 1 Jul 2024 20:26:06 +0530 Subject: [PATCH 2/7] Removed unnecessary line and added test case --- .../compliance_framework/project_settings.rb | 1 - .../compliance_framework/project_settings_spec.rb | 15 +++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/ee/app/models/compliance_management/compliance_framework/project_settings.rb b/ee/app/models/compliance_management/compliance_framework/project_settings.rb index 988aa96ef511bb..687a21746a022c 100644 --- a/ee/app/models/compliance_management/compliance_framework/project_settings.rb +++ b/ee/app/models/compliance_management/compliance_framework/project_settings.rb @@ -4,7 +4,6 @@ module ComplianceManagement module ComplianceFramework class ProjectSettings < ApplicationRecord self.table_name = 'project_compliance_framework_settings' - self.primary_key = :id belongs_to :project belongs_to :compliance_management_framework, class_name: "ComplianceManagement::Framework", foreign_key: :framework_id diff --git a/ee/spec/models/compliance_management/compliance_framework/project_settings_spec.rb b/ee/spec/models/compliance_management/compliance_framework/project_settings_spec.rb index b8eb96d66c7163..383012bd810555 100644 --- a/ee/spec/models/compliance_management/compliance_framework/project_settings_spec.rb +++ b/ee/spec/models/compliance_management/compliance_framework/project_settings_spec.rb @@ -40,6 +40,21 @@ end end + describe '.with_framework_and_project' do + let_it_be(:framework1) do + create(:compliance_framework, namespace: project.group.root_ancestor, name: 'framework1') + end + + let_it_be(:setting) do + create(:compliance_framework_project_setting, project: project, compliance_management_framework: framework1) + end + + it 'returns the setting' do + expect(described_class.with_framework_and_project(project, framework1)) + .to eq([setting]) + end + end + describe '.find_or_create_by_project' do let_it_be(:framework) { create(:compliance_framework, namespace: project.group.root_ancestor) } -- GitLab From 3904fffb7f6cb953f6aeaedd6dd1364e1f4e9c86 Mon Sep 17 00:00:00 2001 From: Hitesh Raghuvanshi Date: Tue, 2 Jul 2024 11:33:01 +0530 Subject: [PATCH 3/7] Added test cases for coverage --- .../projects/update_compliance_frameworks_spec.rb | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/ee/spec/graphql/mutations/projects/update_compliance_frameworks_spec.rb b/ee/spec/graphql/mutations/projects/update_compliance_frameworks_spec.rb index a81f4ed53a5512..57b095a186a60e 100644 --- a/ee/spec/graphql/mutations/projects/update_compliance_frameworks_spec.rb +++ b/ee/spec/graphql/mutations/projects/update_compliance_frameworks_spec.rb @@ -55,6 +55,17 @@ end it_behaves_like "the user can update compliance frameworks of the project" + + context 'when framework id is invalid' do + subject(:resolve) do + mutation.resolve(project_id: GitlabSchema.id_from_object(project), + compliance_framework_ids: ["gid://gitlab/ComplianceManagement::Framework/#{non_existing_record_id}"]) + end + + it 'returns Argument error' do + expect { resolve }.to raise_error(Gitlab::Graphql::Errors::ArgumentError) + end + end end end -- GitLab From 01c0aeb4fdef938884d2bfd3a7c1ba8df6622e52 Mon Sep 17 00:00:00 2001 From: Hitesh Raghuvanshi Date: Mon, 8 Jul 2024 13:53:04 +0530 Subject: [PATCH 4/7] Removed n+1 query --- .../projects/update_compliance_frameworks.rb | 13 +++++-- .../compliance_framework/project_settings.rb | 2 +- .../models/compliance_management/framework.rb | 1 + .../frameworks/update_project_service.rb | 21 +++++++--- .../update_compliance_frameworks_spec.rb | 3 +- .../project_settings_spec.rb | 4 +- .../frameworks/update_project_service_spec.rb | 38 +++++++++++++++++++ locale/gitlab.pot | 3 ++ 8 files changed, 71 insertions(+), 14 deletions(-) diff --git a/ee/app/graphql/mutations/projects/update_compliance_frameworks.rb b/ee/app/graphql/mutations/projects/update_compliance_frameworks.rb index 148eca23572d0b..4d77ad522689b8 100644 --- a/ee/app/graphql/mutations/projects/update_compliance_frameworks.rb +++ b/ee/app/graphql/mutations/projects/update_compliance_frameworks.rb @@ -38,10 +38,15 @@ def resolve(project_id:, compliance_framework_ids:) private def compliance_frameworks(compliance_framework_ids) - GitlabSchema.parse_gids( - compliance_framework_ids, expected_type: ::ComplianceManagement::Framework).map(&:find).uniq - rescue ActiveRecord::RecordNotFound => e - raise Gitlab::Graphql::Errors::ArgumentError, e + ids = GitlabSchema.parse_gids(compliance_framework_ids).map(&:model_id).map(&:to_i).uniq + frameworks = ::ComplianceManagement::Framework.by_ids(ids) + + if frameworks.length != ids.length + raise Gitlab::Graphql::Errors::ArgumentError, format(_("Framework id(s) %{missing_ids} are invalid."), + missing_ids: (ids - frameworks.pluck(:id))) # rubocop: disable CodeReuse/ActiveRecord -- Using pluck only + end + + frameworks end end end diff --git a/ee/app/models/compliance_management/compliance_framework/project_settings.rb b/ee/app/models/compliance_management/compliance_framework/project_settings.rb index 687a21746a022c..8919facd6d9f55 100644 --- a/ee/app/models/compliance_management/compliance_framework/project_settings.rb +++ b/ee/app/models/compliance_management/compliance_framework/project_settings.rb @@ -12,7 +12,7 @@ class ProjectSettings < ApplicationRecord delegate :full_path, to: :project - scope :with_framework_and_project, ->(project, framework) { where(project: project, compliance_management_framework: framework) } + scope :by_framework_and_project, ->(project_id, framework_id) { where(project_id: project_id, framework_id: framework_id) } def self.find_or_create_by_project(project, framework) find_or_initialize_by(project: project).tap do |setting| diff --git a/ee/app/models/compliance_management/framework.rb b/ee/app/models/compliance_management/framework.rb index ea24f06a2fde92..59288a28b1218d 100644 --- a/ee/app/models/compliance_management/framework.rb +++ b/ee/app/models/compliance_management/framework.rb @@ -35,6 +35,7 @@ class Framework < ApplicationRecord scope :with_projects, ->(project_ids) { includes(:projects).where(projects: { id: project_ids }) } scope :with_namespaces, ->(namespace_ids) { includes(:namespace).where(namespaces: { id: namespace_ids }) } + scope :by_ids, ->(ids) { where(id: ids) } def self.search(query) query.present? ? fuzzy_search(query, [:name], use_minimum_char_limit: true) : all diff --git a/ee/app/services/compliance_management/frameworks/update_project_service.rb b/ee/app/services/compliance_management/frameworks/update_project_service.rb index a24fffc94712cb..9a8afcaef0ed62 100644 --- a/ee/app/services/compliance_management/frameworks/update_project_service.rb +++ b/ee/app/services/compliance_management/frameworks/update_project_service.rb @@ -35,21 +35,30 @@ def execute attr_reader :project, :current_user, :frameworks def add_framework_setting(framework) - return unless project.root_namespace.self_and_descendants_ids.include?(framework.namespace.id) + return unless project.root_namespace.self_and_descendants_ids.include?(framework.namespace_id) framework_project_setting = ComplianceManagement::ComplianceFramework::ProjectSettings.new(project: project, compliance_management_framework: framework) - return unless framework_project_setting.save + unless framework_project_setting.save + error_message = "Error while adding framework #{framework.name}. Errors: " \ + "#{framework_project_setting.errors.full_messages.to_sentence}" + + return error(error_message) + end track_event(::Projects::ComplianceFrameworkChangedEvent::EVENT_TYPES[:added], framework) end def remove_framework_setting(framework) framework_project_setting = ComplianceManagement::ComplianceFramework::ProjectSettings - .with_framework_and_project(project, framework).first + .by_framework_and_project(project.id, framework.id).first - return unless framework_project_setting.destroy + unless framework_project_setting.destroy + error_message = "Error while removing framework #{framework.name}. Errors: " \ + "#{framework_project_setting.errors.full_messages.to_sentence}" + return error(error_message) + end track_event(::Projects::ComplianceFrameworkChangedEvent::EVENT_TYPES[:removed], framework) end @@ -58,8 +67,8 @@ def permitted? can?(current_user, :admin_compliance_framework, project) end - def error - ServiceResponse.error(message: _('Failed to assign the framework to the project')) + def error(message = "Failed to assign the framework to the project") + ServiceResponse.error(message: _(message)) end def success diff --git a/ee/spec/graphql/mutations/projects/update_compliance_frameworks_spec.rb b/ee/spec/graphql/mutations/projects/update_compliance_frameworks_spec.rb index 57b095a186a60e..75b06bc7ebb0ca 100644 --- a/ee/spec/graphql/mutations/projects/update_compliance_frameworks_spec.rb +++ b/ee/spec/graphql/mutations/projects/update_compliance_frameworks_spec.rb @@ -63,7 +63,8 @@ end it 'returns Argument error' do - expect { resolve }.to raise_error(Gitlab::Graphql::Errors::ArgumentError) + expect { resolve }.to raise_error(Gitlab::Graphql::Errors::ArgumentError, + format(_("Framework id(s) [%{record_id}] are invalid."), record_id: non_existing_record_id)) end end end diff --git a/ee/spec/models/compliance_management/compliance_framework/project_settings_spec.rb b/ee/spec/models/compliance_management/compliance_framework/project_settings_spec.rb index 383012bd810555..799189636e47af 100644 --- a/ee/spec/models/compliance_management/compliance_framework/project_settings_spec.rb +++ b/ee/spec/models/compliance_management/compliance_framework/project_settings_spec.rb @@ -40,7 +40,7 @@ end end - describe '.with_framework_and_project' do + describe '.by_framework_and_project' do let_it_be(:framework1) do create(:compliance_framework, namespace: project.group.root_ancestor, name: 'framework1') end @@ -50,7 +50,7 @@ end it 'returns the setting' do - expect(described_class.with_framework_and_project(project, framework1)) + expect(described_class.by_framework_and_project(project.id, framework1.id)) .to eq([setting]) end end diff --git a/ee/spec/services/compliance_management/frameworks/update_project_service_spec.rb b/ee/spec/services/compliance_management/frameworks/update_project_service_spec.rb index 491337c0002555..3a2a58d10db110 100644 --- a/ee/spec/services/compliance_management/frameworks/update_project_service_spec.rb +++ b/ee/spec/services/compliance_management/frameworks/update_project_service_spec.rb @@ -73,6 +73,44 @@ update_framework end end + + context 'when there is an error while saving framework project setting' do + it 'returns error' do + save_error_message = 'Not able to save project settings for compliance framework' + error_message = "Error while adding framework #{frameworks.first.name}. Errors: #{save_error_message}" + + allow_next_instance_of(ComplianceManagement::ComplianceFramework::ProjectSettings) do |instance| + allow(instance).to receive(:save).and_return(false) + + errors = ActiveModel::Errors.new(instance).tap { |e| e.add(:base, save_error_message) } + allow(instance).to receive(:errors).and_return(errors) + end + + expect(update_framework.errors).to eq([error_message]) + end + end + + context 'when there is an error while deleting a framework project setting' do + before do + create(:compliance_framework_project_setting, project: project, compliance_management_framework: framework3) + end + + let(:frameworks) { [] } + + it 'returns error' do + save_error_message = 'Not able to delete project settings for compliance framework' + error_message = "Error while removing framework framework3. Errors: #{save_error_message}" + + allow_next_found_instance_of(ComplianceManagement::ComplianceFramework::ProjectSettings) do |instance| + allow(instance).to receive(:destroy).and_return(false) + + errors = ActiveModel::Errors.new(instance).tap { |e| e.add(:base, save_error_message) } + allow(instance).to receive(:errors).and_return(errors) + end + + expect(update_framework.errors).to eq([error_message]) + end + end end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 8837463e7376dc..e4852139423dd5 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -22845,6 +22845,9 @@ msgstr "" msgid "ForksDivergence|View merge request" msgstr "" +msgid "Framework id(s) %{missing_ids} are invalid." +msgstr "" + msgid "Framework successfully deleted" msgstr "" -- GitLab From b34dc36cb3c09c8628a513c3ae71b7e8f4886d8f Mon Sep 17 00:00:00 2001 From: Hitesh Raghuvanshi Date: Wed, 10 Jul 2024 15:15:16 +0530 Subject: [PATCH 5/7] Added limit for the frameworks --- .../projects/update_compliance_frameworks.rb | 16 +++++++++++++++- .../models/compliance_management/framework.rb | 1 - .../update_compliance_frameworks_spec.rb | 17 +++++++++++++++++ 3 files changed, 32 insertions(+), 2 deletions(-) diff --git a/ee/app/graphql/mutations/projects/update_compliance_frameworks.rb b/ee/app/graphql/mutations/projects/update_compliance_frameworks.rb index 4d77ad522689b8..9d676b0511d593 100644 --- a/ee/app/graphql/mutations/projects/update_compliance_frameworks.rb +++ b/ee/app/graphql/mutations/projects/update_compliance_frameworks.rb @@ -6,6 +6,8 @@ class UpdateComplianceFrameworks < BaseMutation graphql_name 'ProjectUpdateComplianceFrameworks' description 'Update compliance frameworks for a project.' + MAX_FRAMEWORKS = 20 + authorize :admin_compliance_framework argument :project_id, Types::GlobalIDType[::Project], @@ -21,6 +23,18 @@ class UpdateComplianceFrameworks < BaseMutation null: true, description: "Project after mutation." + def ready?(**args) + if args[:compliance_framework_ids].size > MAX_FRAMEWORKS + raise Gitlab::Graphql::Errors::ArgumentError, + format( + _('No more than %{max_frameworks} compliance frameworks can be updated at the same time.'), + max_frameworks: MAX_FRAMEWORKS + ) + end + + super + end + def resolve(project_id:, compliance_framework_ids:) project = GitlabSchema.find_by_gid(project_id).sync @@ -39,7 +53,7 @@ def resolve(project_id:, compliance_framework_ids:) def compliance_frameworks(compliance_framework_ids) ids = GitlabSchema.parse_gids(compliance_framework_ids).map(&:model_id).map(&:to_i).uniq - frameworks = ::ComplianceManagement::Framework.by_ids(ids) + frameworks = ::ComplianceManagement::Framework.id_in(ids) if frameworks.length != ids.length raise Gitlab::Graphql::Errors::ArgumentError, format(_("Framework id(s) %{missing_ids} are invalid."), diff --git a/ee/app/models/compliance_management/framework.rb b/ee/app/models/compliance_management/framework.rb index 59288a28b1218d..ea24f06a2fde92 100644 --- a/ee/app/models/compliance_management/framework.rb +++ b/ee/app/models/compliance_management/framework.rb @@ -35,7 +35,6 @@ class Framework < ApplicationRecord scope :with_projects, ->(project_ids) { includes(:projects).where(projects: { id: project_ids }) } scope :with_namespaces, ->(namespace_ids) { includes(:namespace).where(namespaces: { id: namespace_ids }) } - scope :by_ids, ->(ids) { where(id: ids) } def self.search(query) query.present? ? fuzzy_search(query, [:name], use_minimum_char_limit: true) : all diff --git a/ee/spec/requests/api/graphql/mutations/projects/update_compliance_frameworks_spec.rb b/ee/spec/requests/api/graphql/mutations/projects/update_compliance_frameworks_spec.rb index f5f8e0d3aa06cf..cdef4687ba895e 100644 --- a/ee/spec/requests/api/graphql/mutations/projects/update_compliance_frameworks_spec.rb +++ b/ee/spec/requests/api/graphql/mutations/projects/update_compliance_frameworks_spec.rb @@ -81,6 +81,23 @@ def mutation_response }.from([existing_framework]).to([framework1, framework2]) end end + + context 'when there are more than 20 frameworks' do + let(:framework_ids) { (1..21).map { |num| "gid://gitlab/ComplianceManagement::Framework/#{num}" } } + + let(:variables) do + { + project_id: GitlabSchema.id_from_object(project).to_s, + compliance_framework_ids: framework_ids + } + end + + it 'return error' do + post_graphql_mutation(mutation, current_user: current_user) + + expect_graphql_errors_to_include('No more than 20 compliance frameworks can be updated at the same time.') + end + end end end end -- GitLab From 60b6a20c74c0eb6883a26bcb64b00424363e1a37 Mon Sep 17 00:00:00 2001 From: Hitesh Raghuvanshi Date: Wed, 10 Jul 2024 15:17:16 +0530 Subject: [PATCH 6/7] Changed limit to 10 --- .../graphql/mutations/projects/update_compliance_frameworks.rb | 2 +- .../mutations/projects/update_compliance_frameworks_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ee/app/graphql/mutations/projects/update_compliance_frameworks.rb b/ee/app/graphql/mutations/projects/update_compliance_frameworks.rb index 9d676b0511d593..76207d31ea94dd 100644 --- a/ee/app/graphql/mutations/projects/update_compliance_frameworks.rb +++ b/ee/app/graphql/mutations/projects/update_compliance_frameworks.rb @@ -6,7 +6,7 @@ class UpdateComplianceFrameworks < BaseMutation graphql_name 'ProjectUpdateComplianceFrameworks' description 'Update compliance frameworks for a project.' - MAX_FRAMEWORKS = 20 + MAX_FRAMEWORKS = 10 authorize :admin_compliance_framework diff --git a/ee/spec/requests/api/graphql/mutations/projects/update_compliance_frameworks_spec.rb b/ee/spec/requests/api/graphql/mutations/projects/update_compliance_frameworks_spec.rb index cdef4687ba895e..a5caef75b7e1cd 100644 --- a/ee/spec/requests/api/graphql/mutations/projects/update_compliance_frameworks_spec.rb +++ b/ee/spec/requests/api/graphql/mutations/projects/update_compliance_frameworks_spec.rb @@ -95,7 +95,7 @@ def mutation_response it 'return error' do post_graphql_mutation(mutation, current_user: current_user) - expect_graphql_errors_to_include('No more than 20 compliance frameworks can be updated at the same time.') + expect_graphql_errors_to_include('No more than 10 compliance frameworks can be updated at the same time.') end end end -- GitLab From 7f89082b72bc782160350afb0a06d15fc82a39be Mon Sep 17 00:00:00 2001 From: Hitesh Raghuvanshi Date: Wed, 10 Jul 2024 15:23:07 +0530 Subject: [PATCH 7/7] Added auto generated gitlab pot --- locale/gitlab.pot | 3 +++ 1 file changed, 3 insertions(+) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 9f3668b7795844..1eb791ff8c524c 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -34990,6 +34990,9 @@ msgstr "" msgid "No more seats in subscription" msgstr "" +msgid "No more than %{max_frameworks} compliance frameworks can be updated at the same time." +msgstr "" + msgid "No more than %{max_issues} issues can be updated at the same time" msgstr "" -- GitLab