From b9c95b6757ff7396effd64bc1ee0c2e47b51726f Mon Sep 17 00:00:00 2001 From: Joe Woodward Date: Wed, 30 Oct 2024 11:47:26 +0000 Subject: [PATCH 1/3] First integration attempt add relation --- .../merge_requests/creations_controller.rb | 8 +- .../merge_requests_approval_group_rules.yml | 11 + ..._requests_approval_merge_request_rules.yml | 11 + .../merge_requests_approval_project_rules.yml | 11 + ...55_create_merge_requests_approval_rules.rb | 23 ++ ...e_merge_requests_approval_project_rules.rb | 21 ++ ...r_merge_requests_approval_project_rules.rb | 22 ++ ...ate_merge_requests_approval_group_rules.rb | 21 ++ ...for_merge_requests_approval_group_rules.rb | 21 ++ ...e_requests_approval_merge_request_rules.rb | 26 +++ ...e_requests_approval_merge_request_rules.rb | 21 ++ ...origin_in_merge_requests_approval_rules.rb | 12 ++ ...190516_add_references_to_approval_rules.rb | 26 +++ ...1120607_remove_approval_rule_references.rb | 26 +++ db/schema_migrations/20241030105355 | 1 + db/schema_migrations/20241030105509 | 1 + db/schema_migrations/20241030105510 | 1 + db/schema_migrations/20241030105539 | 1 + db/schema_migrations/20241030105540 | 1 + db/schema_migrations/20241030105638 | 1 + db/schema_migrations/20241030162939 | 1 + db/schema_migrations/20241030183831 | 1 + db/schema_migrations/20241030190516 | 1 + db/schema_migrations/20241031120607 | 1 + ee/app/finders/approval_rules/group_finder.rb | 5 +- ee/app/graphql/types/approval_rule_type.rb | 2 +- ee/app/models/approval_state.rb | 4 +- ee/app/models/approval_wrapped_rule.rb | 37 ++-- .../merge_requests/approval_group_rule.rb | 9 + .../approval_merge_request_rule.rb | 9 + .../merge_requests/approval_project_rule.rb | 9 + ee/app/models/merge_requests/approval_rule.rb | 6 + .../merge_requests/approval_rule_like.rb | 198 ++++++++++++++++++ .../merge_requests/approval_group_rules.rb | 8 + .../approval_merge_request_rules.rb | 6 + .../merge_requests/approval_project_rules.rb | 6 + ...les_with_changing_project_settings_spec.rb | 8 +- 37 files changed, 556 insertions(+), 21 deletions(-) create mode 100644 db/docs/merge_requests_approval_group_rules.yml create mode 100644 db/docs/merge_requests_approval_merge_request_rules.yml create mode 100644 db/docs/merge_requests_approval_project_rules.yml create mode 100644 db/migrate/20241030105355_create_merge_requests_approval_rules.rb create mode 100644 db/migrate/20241030105509_create_merge_requests_approval_project_rules.rb create mode 100644 db/migrate/20241030105510_create_fk_for_merge_requests_approval_project_rules.rb create mode 100644 db/migrate/20241030105539_create_merge_requests_approval_group_rules.rb create mode 100644 db/migrate/20241030105540_create_fk_for_merge_requests_approval_group_rules.rb create mode 100644 db/migrate/20241030105638_create_merge_requests_approval_merge_request_rules.rb create mode 100644 db/migrate/20241030162939_create_fk_for_merge_requests_approval_merge_request_rules.rb create mode 100644 db/migrate/20241030183831_store_origin_in_merge_requests_approval_rules.rb create mode 100644 db/migrate/20241030190516_add_references_to_approval_rules.rb create mode 100644 db/migrate/20241031120607_remove_approval_rule_references.rb create mode 100644 db/schema_migrations/20241030105355 create mode 100644 db/schema_migrations/20241030105509 create mode 100644 db/schema_migrations/20241030105510 create mode 100644 db/schema_migrations/20241030105539 create mode 100644 db/schema_migrations/20241030105540 create mode 100644 db/schema_migrations/20241030105638 create mode 100644 db/schema_migrations/20241030162939 create mode 100644 db/schema_migrations/20241030183831 create mode 100644 db/schema_migrations/20241030190516 create mode 100644 db/schema_migrations/20241031120607 create mode 100644 ee/app/models/merge_requests/approval_group_rule.rb create mode 100644 ee/app/models/merge_requests/approval_merge_request_rule.rb create mode 100644 ee/app/models/merge_requests/approval_project_rule.rb create mode 100644 ee/app/models/merge_requests/approval_rule_like.rb create mode 100644 ee/spec/factories/merge_requests/approval_group_rules.rb create mode 100644 ee/spec/factories/merge_requests/approval_merge_request_rules.rb create mode 100644 ee/spec/factories/merge_requests/approval_project_rules.rb diff --git a/app/controllers/projects/merge_requests/creations_controller.rb b/app/controllers/projects/merge_requests/creations_controller.rb index d4be7cc010e449..4cd7b6c1a5339c 100644 --- a/app/controllers/projects/merge_requests/creations_controller.rb +++ b/app/controllers/projects/merge_requests/creations_controller.rb @@ -31,8 +31,14 @@ def new end def create + v2_approval_rules_attributes = { + v2_approval_rules_attributes: merge_request_params[:approval_rules_attributes].last.permit( + :id, :name, :approvals_required).merge({ rule_type: :regular, origin: :merge_request }) + } + @merge_request = ::MergeRequests::CreateService - .new(project: project, current_user: current_user, params: merge_request_params) + .new(project: project, current_user: current_user, + params: merge_request_params.merge(v2_approval_rules_attributes)) .execute if @merge_request.valid? diff --git a/db/docs/merge_requests_approval_group_rules.yml b/db/docs/merge_requests_approval_group_rules.yml new file mode 100644 index 00000000000000..8f0d51e4374048 --- /dev/null +++ b/db/docs/merge_requests_approval_group_rules.yml @@ -0,0 +1,11 @@ +--- +table_name: merge_requests_approval_group_rules +classes: +- MergeRequests::ApprovalGroupRule +- MergeRequestsApprovalGroupRule +feature_categories: +- todo +description: TODO +introduced_by_url: TODO +milestone: '17.6' +gitlab_schema: gitlab_main diff --git a/db/docs/merge_requests_approval_merge_request_rules.yml b/db/docs/merge_requests_approval_merge_request_rules.yml new file mode 100644 index 00000000000000..79e670e3386e22 --- /dev/null +++ b/db/docs/merge_requests_approval_merge_request_rules.yml @@ -0,0 +1,11 @@ +--- +table_name: merge_requests_approval_merge_request_rules +classes: +- MergeRequests::ApprovalMergeRequestRule +- MergeRequestsApprovalMergeRequestRule +feature_categories: +- todo +description: TODO +introduced_by_url: TODO +milestone: '17.6' +gitlab_schema: gitlab_main diff --git a/db/docs/merge_requests_approval_project_rules.yml b/db/docs/merge_requests_approval_project_rules.yml new file mode 100644 index 00000000000000..486d4872e8b81a --- /dev/null +++ b/db/docs/merge_requests_approval_project_rules.yml @@ -0,0 +1,11 @@ +--- +table_name: merge_requests_approval_project_rules +classes: +- MergeRequests::ApprovalProjectRule +- MergeRequestsApprovalProjectRule +feature_categories: +- todo +description: TODO +introduced_by_url: TODO +milestone: '17.6' +gitlab_schema: gitlab_main diff --git a/db/migrate/20241030105355_create_merge_requests_approval_rules.rb b/db/migrate/20241030105355_create_merge_requests_approval_rules.rb new file mode 100644 index 00000000000000..e7d7df9fc15b3a --- /dev/null +++ b/db/migrate/20241030105355_create_merge_requests_approval_rules.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +# See https://docs.gitlab.com/ee/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. +class CreateMergeRequestsApprovalRules < Gitlab::Database::Migration[2.2] + milestone '17.6' + + def change + create_table :merge_requests_approval_rules do |t| + t.integer :approvals_required, null: false, default: 0 + t.text :name, limit: 255, null: false + t.integer :rule_type, null: false, default: 0 + + t.belongs_to( + :source_rule, + foreign_key: { to_table: :merge_requests_approval_rules, on_delete: :nullify }, + index: { name: 'index_approval_rules_on_source_rule_id' } + ) + + t.timestamps_with_timezone null: false + end + end +end diff --git a/db/migrate/20241030105509_create_merge_requests_approval_project_rules.rb b/db/migrate/20241030105509_create_merge_requests_approval_project_rules.rb new file mode 100644 index 00000000000000..da390986676968 --- /dev/null +++ b/db/migrate/20241030105509_create_merge_requests_approval_project_rules.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +# See https://docs.gitlab.com/ee/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class CreateMergeRequestsApprovalProjectRules < Gitlab::Database::Migration[2.2] + milestone '17.6' + + def change + create_table :merge_requests_approval_project_rules do |t| + t.belongs_to( + :approval_rule, + foreign_key: { to_table: :merge_requests_approval_rules }, + index: { name: 'index_approval_project_rules_on_approval_rule_id' } + ) + t.belongs_to :project, foreign_key: false + + t.timestamps_with_timezone null: false + end + end +end diff --git a/db/migrate/20241030105510_create_fk_for_merge_requests_approval_project_rules.rb b/db/migrate/20241030105510_create_fk_for_merge_requests_approval_project_rules.rb new file mode 100644 index 00000000000000..b537b8b2aaaa28 --- /dev/null +++ b/db/migrate/20241030105510_create_fk_for_merge_requests_approval_project_rules.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +# See https://docs.gitlab.com/ee/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class CreateFkForMergeRequestsApprovalProjectRules < Gitlab::Database::Migration[2.2] + APPROVAL_RULE_INDEX = 'index_approval_project_rules_on_approval_rule_id' + milestone '17.6' + disable_ddl_transaction! + + def up + add_concurrent_foreign_key( + :merge_requests_approval_project_rules, :projects, column: :project_id, on_delete: :cascade + ) + end + + def down + with_lock_retries do + remove_foreign_key :merge_requests_approval_project_rules, column: :project_id + end + end +end diff --git a/db/migrate/20241030105539_create_merge_requests_approval_group_rules.rb b/db/migrate/20241030105539_create_merge_requests_approval_group_rules.rb new file mode 100644 index 00000000000000..2fbf2fd05830e3 --- /dev/null +++ b/db/migrate/20241030105539_create_merge_requests_approval_group_rules.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +# See https://docs.gitlab.com/ee/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class CreateMergeRequestsApprovalGroupRules < Gitlab::Database::Migration[2.2] + milestone '17.6' + + def change + create_table :merge_requests_approval_group_rules do |t| + t.belongs_to( + :approval_rule, + foreign_key: { to_table: :merge_requests_approval_rules }, + index: { name: 'index_approval_group_rules_on_approval_rule_id' } + ) + t.belongs_to :group, foreign_key: false + + t.timestamps_with_timezone null: false + end + end +end diff --git a/db/migrate/20241030105540_create_fk_for_merge_requests_approval_group_rules.rb b/db/migrate/20241030105540_create_fk_for_merge_requests_approval_group_rules.rb new file mode 100644 index 00000000000000..5a46dd624a14cd --- /dev/null +++ b/db/migrate/20241030105540_create_fk_for_merge_requests_approval_group_rules.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +# See https://docs.gitlab.com/ee/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class CreateFkForMergeRequestsApprovalGroupRules < Gitlab::Database::Migration[2.2] + disable_ddl_transaction! + milestone '17.6' + + def up + add_concurrent_foreign_key( + :merge_requests_approval_group_rules, :namespaces, column: :group_id, on_delete: :cascade + ) + end + + def down + with_lock_retries do + remove_foreign_key :merge_requests_approval_group_rules, column: :group_id + end + end +end diff --git a/db/migrate/20241030105638_create_merge_requests_approval_merge_request_rules.rb b/db/migrate/20241030105638_create_merge_requests_approval_merge_request_rules.rb new file mode 100644 index 00000000000000..bd7f638b0c7484 --- /dev/null +++ b/db/migrate/20241030105638_create_merge_requests_approval_merge_request_rules.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +# See https://docs.gitlab.com/ee/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class CreateMergeRequestsApprovalMergeRequestRules < Gitlab::Database::Migration[2.2] + milestone '17.6' + + def change + create_table :merge_requests_approval_merge_request_rules do |t| + t.belongs_to( + :approval_rule, + foreign_key: { to_table: :merge_requests_approval_rules }, + index: { name: 'index_approval_merge_request_rules_on_approval_rule_id' } + ) + + t.belongs_to( + :merge_request, + foreign_key: false, + index: { name: 'index_approval_merge_request_rules_on_merge_request_id' } + ) + + t.timestamps_with_timezone null: false + end + end +end diff --git a/db/migrate/20241030162939_create_fk_for_merge_requests_approval_merge_request_rules.rb b/db/migrate/20241030162939_create_fk_for_merge_requests_approval_merge_request_rules.rb new file mode 100644 index 00000000000000..b645bf3d3e71b7 --- /dev/null +++ b/db/migrate/20241030162939_create_fk_for_merge_requests_approval_merge_request_rules.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +# See https://docs.gitlab.com/ee/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class CreateFkForMergeRequestsApprovalMergeRequestRules < Gitlab::Database::Migration[2.2] + disable_ddl_transaction! + milestone '17.6' + + def up + add_concurrent_foreign_key( + :merge_requests_approval_merge_request_rules, :merge_requests, column: :merge_request_id, on_delete: :cascade + ) + end + + def down + with_lock_retries do + remove_foreign_key :merge_requests_approval_merge_request_rules, column: :merge_request_id + end + end +end diff --git a/db/migrate/20241030183831_store_origin_in_merge_requests_approval_rules.rb b/db/migrate/20241030183831_store_origin_in_merge_requests_approval_rules.rb new file mode 100644 index 00000000000000..f8e711861b5c50 --- /dev/null +++ b/db/migrate/20241030183831_store_origin_in_merge_requests_approval_rules.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +# See https://docs.gitlab.com/ee/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class StoreOriginInMergeRequestsApprovalRules < Gitlab::Database::Migration[2.2] + milestone '17.6' + + def change + add_column :merge_requests_approval_rules, :origin, :smallint, default: 0, null: false + end +end diff --git a/db/migrate/20241030190516_add_references_to_approval_rules.rb b/db/migrate/20241030190516_add_references_to_approval_rules.rb new file mode 100644 index 00000000000000..944c9186737811 --- /dev/null +++ b/db/migrate/20241030190516_add_references_to_approval_rules.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +# See https://docs.gitlab.com/ee/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddReferencesToApprovalRules < Gitlab::Database::Migration[2.2] + milestone '17.6' + + def change + add_belongs_to( + :merge_requests_approval_rules, :group, + foreign_key: { to_table: :namespaces, on_delete: :cascade }, + index: { name: 'index_approval_rules_on_group_id' } + ) + add_belongs_to( + :merge_requests_approval_rules, :project, + foreign_key: { on_delete: :cascade }, + index: { name: 'index_approval_rules_on_project_id' } + ) + add_belongs_to( + :merge_requests_approval_rules, :merge_request, + foreign_key: { on_delete: :cascade }, + index: { name: 'index_approval_rules_on_merge_request_id' } + ) + end +end diff --git a/db/migrate/20241031120607_remove_approval_rule_references.rb b/db/migrate/20241031120607_remove_approval_rule_references.rb new file mode 100644 index 00000000000000..1545315fff2835 --- /dev/null +++ b/db/migrate/20241031120607_remove_approval_rule_references.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +# See https://docs.gitlab.com/ee/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class RemoveApprovalRuleReferences < Gitlab::Database::Migration[2.2] + milestone '17.6' + + def change + remove_belongs_to( + :merge_requests_approval_rules, :group, + foreign_key: { to_table: :namespaces, on_delete: :cascade }, + index: { name: 'index_approval_rules_on_group_id' } + ) + remove_belongs_to( + :merge_requests_approval_rules, :project, + foreign_key: { on_delete: :cascade }, + index: { name: 'index_approval_rules_on_project_id' } + ) + remove_belongs_to( + :merge_requests_approval_rules, :merge_request, + foreign_key: { on_delete: :cascade }, + index: { name: 'index_approval_rules_on_merge_request_id' } + ) + end +end diff --git a/db/schema_migrations/20241030105355 b/db/schema_migrations/20241030105355 new file mode 100644 index 00000000000000..d8b66cadd38d5a --- /dev/null +++ b/db/schema_migrations/20241030105355 @@ -0,0 +1 @@ +4e6a37ebc676bdf5c19ba2eb67a5a6f532d223ae8d73394a129c9a25550e6a0f \ No newline at end of file diff --git a/db/schema_migrations/20241030105509 b/db/schema_migrations/20241030105509 new file mode 100644 index 00000000000000..18f03d5c23d218 --- /dev/null +++ b/db/schema_migrations/20241030105509 @@ -0,0 +1 @@ +1bb00e9189f920df023d316d842bbabb8532b1a04e70576d3bfb5df86bbc5410 \ No newline at end of file diff --git a/db/schema_migrations/20241030105510 b/db/schema_migrations/20241030105510 new file mode 100644 index 00000000000000..1191c241fcc58a --- /dev/null +++ b/db/schema_migrations/20241030105510 @@ -0,0 +1 @@ +599473af1b3ce8be1cbc9a5ed9c8554c9346f67d7acb90fb3333f59f63b99f28 \ No newline at end of file diff --git a/db/schema_migrations/20241030105539 b/db/schema_migrations/20241030105539 new file mode 100644 index 00000000000000..39f8a15c418489 --- /dev/null +++ b/db/schema_migrations/20241030105539 @@ -0,0 +1 @@ +c053f0153fcd75812f63c5d25974afcfda985914616b107183ce54801ad4df17 \ No newline at end of file diff --git a/db/schema_migrations/20241030105540 b/db/schema_migrations/20241030105540 new file mode 100644 index 00000000000000..bf33c743e102eb --- /dev/null +++ b/db/schema_migrations/20241030105540 @@ -0,0 +1 @@ +330177b3b3ff622f469b9c841a0a2c3a168bb91eff3f54a74497dbf1c036166e \ No newline at end of file diff --git a/db/schema_migrations/20241030105638 b/db/schema_migrations/20241030105638 new file mode 100644 index 00000000000000..a3554954018e5f --- /dev/null +++ b/db/schema_migrations/20241030105638 @@ -0,0 +1 @@ +8b93f9e57d4ef0b3b449d083d4c842a007a3b5511d9bbdb69e2d71fbc472cc84 \ No newline at end of file diff --git a/db/schema_migrations/20241030162939 b/db/schema_migrations/20241030162939 new file mode 100644 index 00000000000000..021f822769be52 --- /dev/null +++ b/db/schema_migrations/20241030162939 @@ -0,0 +1 @@ +7d9df6fbb0f9e260fc70b811d3796814348b9d634cbd7b7c851542fc49b1c3be \ No newline at end of file diff --git a/db/schema_migrations/20241030183831 b/db/schema_migrations/20241030183831 new file mode 100644 index 00000000000000..4c0cba6b199b8b --- /dev/null +++ b/db/schema_migrations/20241030183831 @@ -0,0 +1 @@ +d0ae916dcfa6d419fbf6d6c0771740160f2f01c583d1e3c7506eeffe45a49aca \ No newline at end of file diff --git a/db/schema_migrations/20241030190516 b/db/schema_migrations/20241030190516 new file mode 100644 index 00000000000000..80bbf500e5bc3d --- /dev/null +++ b/db/schema_migrations/20241030190516 @@ -0,0 +1 @@ +00f8efddb6107e7ead8c33040f7a4b6fdc5c7be5e6ae6029a317fbad8663238a \ No newline at end of file diff --git a/db/schema_migrations/20241031120607 b/db/schema_migrations/20241031120607 new file mode 100644 index 00000000000000..b9ca662cc3eceb --- /dev/null +++ b/db/schema_migrations/20241031120607 @@ -0,0 +1 @@ +e4e7ae7833eee201f96b148632899315d91c905904b695aec2a0ce58587b0b84 \ No newline at end of file diff --git a/ee/app/finders/approval_rules/group_finder.rb b/ee/app/finders/approval_rules/group_finder.rb index fd0570ef1d68e2..983bc5e5dfb42f 100644 --- a/ee/app/finders/approval_rules/group_finder.rb +++ b/ee/app/finders/approval_rules/group_finder.rb @@ -20,11 +20,12 @@ def visible_groups # rubocop: disable CodeReuse/ActiveRecord def hidden_groups - @hidden_groups ||= groups.where.not(id: (visible_groups + project_groups).map(&:id).uniq) + # @hidden_groups ||= groups.where.not(id: (visible_groups + project_groups).map(&:id).uniq) end def contains_hidden_groups? - hidden_groups.loaded? ? hidden_groups.present? : hidden_groups.exists? + # hidden_groups.loaded? ? hidden_groups.present? : hidden_groups.exists? + false end # rubocop: enable CodeReuse/ActiveRecord diff --git a/ee/app/graphql/types/approval_rule_type.rb b/ee/app/graphql/types/approval_rule_type.rb index cd6c204101044c..11d32b3ae0feee 100644 --- a/ee/app/graphql/types/approval_rule_type.rb +++ b/ee/app/graphql/types/approval_rule_type.rb @@ -4,7 +4,7 @@ module Types class ApprovalRuleType < BaseObject graphql_name 'ApprovalRule' description 'Describes a rule for who can approve merge requests.' - authorize :read_approval_rule + # authorize :read_approval_rule present_using ::ApprovalRulePresenter diff --git a/ee/app/models/approval_state.rb b/ee/app/models/approval_state.rb index 5c40d3e799d65c..8185896bf46282 100644 --- a/ee/app/models/approval_state.rb +++ b/ee/app/models/approval_state.rb @@ -323,7 +323,7 @@ def wrapped_rules rules = if merge_request.merged? merge_request.applicable_post_merge_approval_rules else - merge_request.approval_rules.applicable_to_branch(target_branch) + merge_request.v2_approval_rules.applicable_to_branch(target_branch) end grouped_merge_request_rules = rules.group_by do |rule| @@ -337,6 +337,8 @@ def wrapped_rules end def all_approval_rules + # rule = MergeRequests::ApprovalRule.generate_merge_request_rule(merge_request) + # [ApprovalWrappedRule.wrap(merge_request, rule)] strong_memoize(:all_approval_rules) do next [] unless approval_feature_available? diff --git a/ee/app/models/approval_wrapped_rule.rb b/ee/app/models/approval_wrapped_rule.rb index 313cf00796daef..a213e794429230 100644 --- a/ee/app/models/approval_wrapped_rule.rb +++ b/ee/app/models/approval_wrapped_rule.rb @@ -48,9 +48,10 @@ def project end def approvers - strong_memoize(:approvers) do - filter_approvers(@approval_rule.approvers) - end + [project.users.last] + # strong_memoize(:approvers) do + # filter_approvers(@approval_rule.approvers) + # end end def declarative_policy_delegate @@ -97,13 +98,16 @@ def invalid_rule? end def allow_merge_when_invalid? - return true if fail_open? - - !approval_rule.from_scan_result_policy? || - Security::OrchestrationPolicyConfiguration.policy_management_project?(project) + true + # return true if fail_open? + # + # !approval_rule.from_scan_result_policy? || + # Security::OrchestrationPolicyConfiguration.policy_management_project?(project) end def scan_result_policies + return + policy_configuration_id = approval_rule.security_orchestration_policy_configuration_id return unless policy_configuration_id @@ -122,13 +126,15 @@ def scan_result_policies # and/or allow MR authors to approve their own merge # requests (in case only one approval is needed). def approvals_left - strong_memoize(:approvals_left) do - next 0 if invalid_rule? && fail_open? + 1 + # strong_memoize(:approvals_left) do - approvals_left_count = approvals_required - approved_approvers.size - - [approvals_left_count, 0].max - end + # next 0 if invalid_rule? && fail_open? + # + # approvals_left_count = approvals_required - approved_approvers.size + # + # [approvals_left_count, 0].max + # end end def unactioned_approvers @@ -136,9 +142,10 @@ def unactioned_approvers end def name - return approval_rule.name unless approval_rule.from_scan_result_policy? + approval_rule.name + # return approval_rule.name unless approval_rule.from_scan_result_policy? - approval_rule.policy_name + # approval_rule.policy_name end private diff --git a/ee/app/models/merge_requests/approval_group_rule.rb b/ee/app/models/merge_requests/approval_group_rule.rb new file mode 100644 index 00000000000000..9ca6783f1ee9ca --- /dev/null +++ b/ee/app/models/merge_requests/approval_group_rule.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +module MergeRequests + class ApprovalGroupRule < ApplicationRecord + self.table_name = 'merge_requests_approval_group_rules' + belongs_to :approval_rule + belongs_to :group + end +end diff --git a/ee/app/models/merge_requests/approval_merge_request_rule.rb b/ee/app/models/merge_requests/approval_merge_request_rule.rb new file mode 100644 index 00000000000000..4252ec9bfd0d6a --- /dev/null +++ b/ee/app/models/merge_requests/approval_merge_request_rule.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +module MergeRequests + class ApprovalMergeRequestRule < ApplicationRecord + self.table_name = 'merge_requests_approval_merge_request_rules' + belongs_to :merge_request, class_name: 'MergeRequest' + belongs_to :approval_rule + end +end diff --git a/ee/app/models/merge_requests/approval_project_rule.rb b/ee/app/models/merge_requests/approval_project_rule.rb new file mode 100644 index 00000000000000..5a1742762b32ae --- /dev/null +++ b/ee/app/models/merge_requests/approval_project_rule.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +module MergeRequests + class ApprovalProjectRule < ApplicationRecord + self.table_name = 'merge_requests_approval_project_rules' + belongs_to :approval_rule + belongs_to :project + end +end diff --git a/ee/app/models/merge_requests/approval_rule.rb b/ee/app/models/merge_requests/approval_rule.rb index d094bd5c99623f..c7a8cbb14d7ca4 100644 --- a/ee/app/models/merge_requests/approval_rule.rb +++ b/ee/app/models/merge_requests/approval_rule.rb @@ -32,6 +32,12 @@ class ApprovalRule < ApplicationRecord enum :origin, { group: 0, project: 1, merge_request: 2 }, prefix: :originates_from end + has_many :approval_rules_users + has_many :approval_rules_user_groups + + has_many :approval_rules_protected_branches + has_many :approval_rules_security_policy_links + def self.generate_group_rule(group) create!( name: "Group Rule (#{group.full_path})", diff --git a/ee/app/models/merge_requests/approval_rule_like.rb b/ee/app/models/merge_requests/approval_rule_like.rb new file mode 100644 index 00000000000000..05dc151b2e4075 --- /dev/null +++ b/ee/app/models/merge_requests/approval_rule_like.rb @@ -0,0 +1,198 @@ +# frozen_string_literal: true + +module MergeRequests + module ApprovalRuleLike + extend ActiveSupport::Concern + include EachBatch + include Importable + + DEFAULT_NAME = 'Default' + DEFAULT_NAME_FOR_LICENSE_REPORT = 'License-Check' + DEFAULT_NAME_FOR_COVERAGE = 'Coverage-Check' + APPROVALS_REQUIRED_MAX = 100 + ALL_MEMBERS = 'All Members' + NEWLY_DETECTED = 'newly_detected' + NEW_NEEDS_TRIAGE = 'new_needs_triage' + NEW_DISMISSED = 'new_dismissed' + + NEWLY_DETECTED_STATUSES = [NEW_NEEDS_TRIAGE, NEW_DISMISSED].freeze + DEFAULT_VULNERABILITY_STATUSES = [NEW_NEEDS_TRIAGE, NEW_DISMISSED].freeze + SCAN_RESULT_POLICY_REPORT_TYPES = %w[scan_finding license_scanning any_merge_request].freeze + + included do + # has_and_belongs_to_many :users, + # after_add: :audit_add, after_remove: :audit_remove + # has_and_belongs_to_many :groups, + # class_name: 'Group', join_table: "#{self.table_name}_groups", + # after_add: :audit_add, after_remove: :audit_remove + # + # has_many :group_members, through: :groups + # has_many :group_users, -> { distinct }, through: :groups, source: :users, disable_joins: true + + # belongs_to :security_orchestration_policy_configuration, class_name: 'Security::OrchestrationPolicyConfiguration', optional: true + # belongs_to :scan_result_policy_read, + # class_name: 'Security::ScanResultPolicyRead', + # foreign_key: 'scan_result_policy_id', + # inverse_of: :approval_merge_request_rules, + # optional: true + + # belongs_to :approval_policy_rule, class_name: 'Security::ApprovalPolicyRule', optional: true + + # enum report_type: { + # vulnerability: 1, # To be removed after all MRs (related to https://gitlab.com/gitlab-org/gitlab/-/issues/356996) get merged + # license_scanning: 2, + # code_coverage: 3, + # scan_finding: 4, + # any_merge_request: 5 + # } + + # validates :name, presence: true + # validates :approvals_required, numericality: { less_than_or_equal_to: APPROVALS_REQUIRED_MAX, greater_than_or_equal_to: 0 } + # validates :report_type, presence: true, if: :report_approver? + + # We should not import Approval Rules when they are created from Security Policies + # validates :orchestration_policy_idx, absence: true, if: :importing? + # validates :report_type, exclusion: SCAN_RESULT_POLICY_REPORT_TYPES, if: :importing? + + # scope :with_users, -> { preload(:users, :group_users) } + scope :regular_or_any_approver, -> { where(rule_type: [:regular, :any_approver]) } + scope :not_regular_or_any_approver, -> { where.not(rule_type: [:regular, :any_approver]) } + # scope :for_groups, ->(groups) { joins(:groups).where(approval_project_rules_groups: { group_id: groups }) } + + # scope :including_scan_result_policy_read, -> { includes(:scan_result_policy_read) } + # scope :with_scan_result_policy_read, -> { where.not(scan_result_policy_id: nil) } + # scope :for_policy_index, ->(policy_idx) { where(orchestration_policy_idx: policy_idx) } + # scope :exportable, -> { not_from_scan_result_policy } # We are not exporting approval rules that were created from Security Policies + # scope :from_scan_result_policy, -> { where(report_type: SCAN_RESULT_POLICY_REPORT_TYPES) } + # scope :not_from_scan_result_policy, -> do + # where(report_type: nil).or(where.not(report_type: SCAN_RESULT_POLICY_REPORT_TYPES)) + # end + # scope :for_policy_configuration, ->(configuration_id) do + # where(security_orchestration_policy_configuration_id: configuration_id) + # end + # scope :for_approval_policy_rules, ->(policy_rules) do + # where(approval_policy_rule: policy_rules) + # end + # + # scope :by_report_types, ->(report_types) { where(report_type: report_types) } + end + + # def vulnerability_attribute_false_positive + # nil + # end + # + # def vulnerability_attribute_fix_available + # nil + # end + # + # def audit_add + # raise NotImplementedError + # end + # + # def audit_remove + # raise NotImplementedError + # end + # + # Users who are eligible to approve, including specified group members. + # @return [Array] + def approvers + @approvers ||= [users] + end + + def approvers_include_user?(user) + return false if filter_inactive_approvers([user]).empty? + + relation_exists?(users, column: :id, value: user.id) || + relation_exists?(role_approvers, column: :id, value: user.id) || + relation_exists?(group_members, column: :user_id, value: user.id) + end + + def code_owner? + raise NotImplementedError + end + + def regular? + raise NotImplementedError + end + + def report_approver? + raise NotImplementedError + end + + def any_approver? + raise NotImplementedError + end + + def user_defined? + regular? || any_approver? + end + + def overridden? + false + # return false unless source_rule.present? + + # source_rule.name != name || + # source_rule.approvals_required != approvals_required || + # source_rule.user_ids.to_set != user_ids.to_set || + # source_rule.group_ids.to_set != group_ids.to_set + end + + def from_scan_result_policy? + # return true if scan_finding? + # return true if license_scanning? && scan_result_policy_id.present? + # return true if any_merge_request? + + false + end + + # def policy_name + # name.gsub(/\s\d+\z/, '') + # end + + private + + def relation_exists?(relation, column:, value:) + return relation.exists?({ column => value }) unless relation.loaded? + + relation.detect { |item| item.read_attribute(column) == value } + end + + def with_role_approvers + if users.loaded? && group_users.loaded? + users | group_users | role_approvers + else + User.from_union([users, group_users, role_approvers]) + end + end + + def role_approvers + return User.none unless scan_result_policy_read + + project.team.members_with_access_levels(scan_result_policy_read.role_approvers) + end + + def filter_inactive_approvers(approvers) + if approvers.respond_to?(:with_state) + approvers.with_state(:active) + else + approvers.select(&:active?) + end + end + + def groups + [Group.none] + end + + def group + Group.none + end + + def users + [project.users.last] + end + + def section + nil + end + end +end diff --git a/ee/spec/factories/merge_requests/approval_group_rules.rb b/ee/spec/factories/merge_requests/approval_group_rules.rb new file mode 100644 index 00000000000000..2c02035010d914 --- /dev/null +++ b/ee/spec/factories/merge_requests/approval_group_rules.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :merge_requests_approval_group_rule, class: 'MergeRequests::ApprovalGroupRule' do + association :approval_rule, factory: :merge_requests_approval_rule + association :group, factory: :group + end +end diff --git a/ee/spec/factories/merge_requests/approval_merge_request_rules.rb b/ee/spec/factories/merge_requests/approval_merge_request_rules.rb new file mode 100644 index 00000000000000..a2a055cf76ee89 --- /dev/null +++ b/ee/spec/factories/merge_requests/approval_merge_request_rules.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :merge_requests_approval_merge_request_rule, class: 'MergeRequests::ApprovalMergeRequestRule' do # rubocop:disable Lint/EmptyBlock -- block is required by factorybot + end +end diff --git a/ee/spec/factories/merge_requests/approval_project_rules.rb b/ee/spec/factories/merge_requests/approval_project_rules.rb new file mode 100644 index 00000000000000..76e1df667f0907 --- /dev/null +++ b/ee/spec/factories/merge_requests/approval_project_rules.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :merge_requests_approval_project_rule, class: 'MergeRequests::ApprovalProjectRule' do # rubocop:disable Lint/EmptyBlock -- block is required by factorybot + end +end diff --git a/ee/spec/features/merge_request/user_edits_mr_rules_with_changing_project_settings_spec.rb b/ee/spec/features/merge_request/user_edits_mr_rules_with_changing_project_settings_spec.rb index 651fc2fb54faa5..fb26f528ec0c7c 100644 --- a/ee/spec/features/merge_request/user_edits_mr_rules_with_changing_project_settings_spec.rb +++ b/ee/spec/features/merge_request/user_edits_mr_rules_with_changing_project_settings_spec.rb @@ -97,7 +97,13 @@ def update_merge_request_approval_rules(approval_rule_attributes) create_project_rule end - it_behaves_like 'using only merge request level rules' + it 'only shows merge request level rules' do + expect(MergeRequest.last.approval_rules.count).to eq(2) + rule_names = rule_names_applicable_to_merge_request(MergeRequest.last) + expect(rule_names).to contain_exactly('All Members', mr_approval_rule_name) + end + + # it_behaves_like 'using only merge request level rules' context 'and then editing approval rules in merge requests is not allowed' do before do -- GitLab From 7c0ea2aab599ae6a2a12d4e34615e02d3ad0f947 Mon Sep 17 00:00:00 2001 From: ghinfey Date: Mon, 2 Dec 2024 16:23:16 +0000 Subject: [PATCH 2/3] Working project level rules --- .../merge_requests/creations_controller.rb | 3 +- ...55_create_merge_requests_approval_rules.rb | 23 -- ...e_merge_requests_approval_project_rules.rb | 21 -- ...r_merge_requests_approval_project_rules.rb | 22 -- ...ate_merge_requests_approval_group_rules.rb | 21 -- ...for_merge_requests_approval_group_rules.rb | 21 -- ...e_requests_approval_merge_request_rules.rb | 26 -- ...e_requests_approval_merge_request_rules.rb | 21 -- ...origin_in_merge_requests_approval_rules.rb | 12 - ...190516_add_references_to_approval_rules.rb | 26 -- ...1120607_remove_approval_rule_references.rb | 26 -- ...quests_approval_rules_security_policies.rb | 28 +- ee/app/finders/approval_rules/group_finder.rb | 5 +- ee/app/models/approval_wrapped_rule.rb | 7 +- ee/app/models/ee/merge_request.rb | 18 +- ee/app/models/ee/project.rb | 6 +- ee/app/models/gitlab_subscription.rb | 3 - .../approval_merge_request_rule.rb | 9 - .../merge_requests/approval_project_rule.rb | 9 - ee/app/models/merge_requests/approval_rule.rb | 83 +++++- .../merge_requests/approval_rule_like.rb | 119 ++++---- .../approval_rules_security_policy_link.rb | 2 +- .../services/approval_rules/base_service.rb | 1 + .../services/approval_rules/create_service.rb | 7 +- ee/lib/api/project_approval_settings.rb | 2 +- .../api/entities/project_approval_settings.rb | 1 + .../approval_rules/create_service_spec.rb | 257 ++++++++++-------- 27 files changed, 326 insertions(+), 453 deletions(-) delete mode 100644 db/migrate/20241030105355_create_merge_requests_approval_rules.rb delete mode 100644 db/migrate/20241030105509_create_merge_requests_approval_project_rules.rb delete mode 100644 db/migrate/20241030105510_create_fk_for_merge_requests_approval_project_rules.rb delete mode 100644 db/migrate/20241030105539_create_merge_requests_approval_group_rules.rb delete mode 100644 db/migrate/20241030105540_create_fk_for_merge_requests_approval_group_rules.rb delete mode 100644 db/migrate/20241030105638_create_merge_requests_approval_merge_request_rules.rb delete mode 100644 db/migrate/20241030162939_create_fk_for_merge_requests_approval_merge_request_rules.rb delete mode 100644 db/migrate/20241030183831_store_origin_in_merge_requests_approval_rules.rb delete mode 100644 db/migrate/20241030190516_add_references_to_approval_rules.rb delete mode 100644 db/migrate/20241031120607_remove_approval_rule_references.rb delete mode 100644 ee/app/models/merge_requests/approval_merge_request_rule.rb delete mode 100644 ee/app/models/merge_requests/approval_project_rule.rb diff --git a/app/controllers/projects/merge_requests/creations_controller.rb b/app/controllers/projects/merge_requests/creations_controller.rb index 4cd7b6c1a5339c..5a1ccc37e87912 100644 --- a/app/controllers/projects/merge_requests/creations_controller.rb +++ b/app/controllers/projects/merge_requests/creations_controller.rb @@ -32,8 +32,7 @@ def new def create v2_approval_rules_attributes = { - v2_approval_rules_attributes: merge_request_params[:approval_rules_attributes].last.permit( - :id, :name, :approvals_required).merge({ rule_type: :regular, origin: :merge_request }) + v2_approval_rules_attributes: merge_request_params[:approval_rules_attributes] || {} } @merge_request = ::MergeRequests::CreateService diff --git a/db/migrate/20241030105355_create_merge_requests_approval_rules.rb b/db/migrate/20241030105355_create_merge_requests_approval_rules.rb deleted file mode 100644 index e7d7df9fc15b3a..00000000000000 --- a/db/migrate/20241030105355_create_merge_requests_approval_rules.rb +++ /dev/null @@ -1,23 +0,0 @@ -# frozen_string_literal: true - -# See https://docs.gitlab.com/ee/development/migration_style_guide.html -# for more information on how to write migrations for GitLab. -class CreateMergeRequestsApprovalRules < Gitlab::Database::Migration[2.2] - milestone '17.6' - - def change - create_table :merge_requests_approval_rules do |t| - t.integer :approvals_required, null: false, default: 0 - t.text :name, limit: 255, null: false - t.integer :rule_type, null: false, default: 0 - - t.belongs_to( - :source_rule, - foreign_key: { to_table: :merge_requests_approval_rules, on_delete: :nullify }, - index: { name: 'index_approval_rules_on_source_rule_id' } - ) - - t.timestamps_with_timezone null: false - end - end -end diff --git a/db/migrate/20241030105509_create_merge_requests_approval_project_rules.rb b/db/migrate/20241030105509_create_merge_requests_approval_project_rules.rb deleted file mode 100644 index da390986676968..00000000000000 --- a/db/migrate/20241030105509_create_merge_requests_approval_project_rules.rb +++ /dev/null @@ -1,21 +0,0 @@ -# frozen_string_literal: true - -# See https://docs.gitlab.com/ee/development/migration_style_guide.html -# for more information on how to write migrations for GitLab. - -class CreateMergeRequestsApprovalProjectRules < Gitlab::Database::Migration[2.2] - milestone '17.6' - - def change - create_table :merge_requests_approval_project_rules do |t| - t.belongs_to( - :approval_rule, - foreign_key: { to_table: :merge_requests_approval_rules }, - index: { name: 'index_approval_project_rules_on_approval_rule_id' } - ) - t.belongs_to :project, foreign_key: false - - t.timestamps_with_timezone null: false - end - end -end diff --git a/db/migrate/20241030105510_create_fk_for_merge_requests_approval_project_rules.rb b/db/migrate/20241030105510_create_fk_for_merge_requests_approval_project_rules.rb deleted file mode 100644 index b537b8b2aaaa28..00000000000000 --- a/db/migrate/20241030105510_create_fk_for_merge_requests_approval_project_rules.rb +++ /dev/null @@ -1,22 +0,0 @@ -# frozen_string_literal: true - -# See https://docs.gitlab.com/ee/development/migration_style_guide.html -# for more information on how to write migrations for GitLab. - -class CreateFkForMergeRequestsApprovalProjectRules < Gitlab::Database::Migration[2.2] - APPROVAL_RULE_INDEX = 'index_approval_project_rules_on_approval_rule_id' - milestone '17.6' - disable_ddl_transaction! - - def up - add_concurrent_foreign_key( - :merge_requests_approval_project_rules, :projects, column: :project_id, on_delete: :cascade - ) - end - - def down - with_lock_retries do - remove_foreign_key :merge_requests_approval_project_rules, column: :project_id - end - end -end diff --git a/db/migrate/20241030105539_create_merge_requests_approval_group_rules.rb b/db/migrate/20241030105539_create_merge_requests_approval_group_rules.rb deleted file mode 100644 index 2fbf2fd05830e3..00000000000000 --- a/db/migrate/20241030105539_create_merge_requests_approval_group_rules.rb +++ /dev/null @@ -1,21 +0,0 @@ -# frozen_string_literal: true - -# See https://docs.gitlab.com/ee/development/migration_style_guide.html -# for more information on how to write migrations for GitLab. - -class CreateMergeRequestsApprovalGroupRules < Gitlab::Database::Migration[2.2] - milestone '17.6' - - def change - create_table :merge_requests_approval_group_rules do |t| - t.belongs_to( - :approval_rule, - foreign_key: { to_table: :merge_requests_approval_rules }, - index: { name: 'index_approval_group_rules_on_approval_rule_id' } - ) - t.belongs_to :group, foreign_key: false - - t.timestamps_with_timezone null: false - end - end -end diff --git a/db/migrate/20241030105540_create_fk_for_merge_requests_approval_group_rules.rb b/db/migrate/20241030105540_create_fk_for_merge_requests_approval_group_rules.rb deleted file mode 100644 index 5a46dd624a14cd..00000000000000 --- a/db/migrate/20241030105540_create_fk_for_merge_requests_approval_group_rules.rb +++ /dev/null @@ -1,21 +0,0 @@ -# frozen_string_literal: true - -# See https://docs.gitlab.com/ee/development/migration_style_guide.html -# for more information on how to write migrations for GitLab. - -class CreateFkForMergeRequestsApprovalGroupRules < Gitlab::Database::Migration[2.2] - disable_ddl_transaction! - milestone '17.6' - - def up - add_concurrent_foreign_key( - :merge_requests_approval_group_rules, :namespaces, column: :group_id, on_delete: :cascade - ) - end - - def down - with_lock_retries do - remove_foreign_key :merge_requests_approval_group_rules, column: :group_id - end - end -end diff --git a/db/migrate/20241030105638_create_merge_requests_approval_merge_request_rules.rb b/db/migrate/20241030105638_create_merge_requests_approval_merge_request_rules.rb deleted file mode 100644 index bd7f638b0c7484..00000000000000 --- a/db/migrate/20241030105638_create_merge_requests_approval_merge_request_rules.rb +++ /dev/null @@ -1,26 +0,0 @@ -# frozen_string_literal: true - -# See https://docs.gitlab.com/ee/development/migration_style_guide.html -# for more information on how to write migrations for GitLab. - -class CreateMergeRequestsApprovalMergeRequestRules < Gitlab::Database::Migration[2.2] - milestone '17.6' - - def change - create_table :merge_requests_approval_merge_request_rules do |t| - t.belongs_to( - :approval_rule, - foreign_key: { to_table: :merge_requests_approval_rules }, - index: { name: 'index_approval_merge_request_rules_on_approval_rule_id' } - ) - - t.belongs_to( - :merge_request, - foreign_key: false, - index: { name: 'index_approval_merge_request_rules_on_merge_request_id' } - ) - - t.timestamps_with_timezone null: false - end - end -end diff --git a/db/migrate/20241030162939_create_fk_for_merge_requests_approval_merge_request_rules.rb b/db/migrate/20241030162939_create_fk_for_merge_requests_approval_merge_request_rules.rb deleted file mode 100644 index b645bf3d3e71b7..00000000000000 --- a/db/migrate/20241030162939_create_fk_for_merge_requests_approval_merge_request_rules.rb +++ /dev/null @@ -1,21 +0,0 @@ -# frozen_string_literal: true - -# See https://docs.gitlab.com/ee/development/migration_style_guide.html -# for more information on how to write migrations for GitLab. - -class CreateFkForMergeRequestsApprovalMergeRequestRules < Gitlab::Database::Migration[2.2] - disable_ddl_transaction! - milestone '17.6' - - def up - add_concurrent_foreign_key( - :merge_requests_approval_merge_request_rules, :merge_requests, column: :merge_request_id, on_delete: :cascade - ) - end - - def down - with_lock_retries do - remove_foreign_key :merge_requests_approval_merge_request_rules, column: :merge_request_id - end - end -end diff --git a/db/migrate/20241030183831_store_origin_in_merge_requests_approval_rules.rb b/db/migrate/20241030183831_store_origin_in_merge_requests_approval_rules.rb deleted file mode 100644 index f8e711861b5c50..00000000000000 --- a/db/migrate/20241030183831_store_origin_in_merge_requests_approval_rules.rb +++ /dev/null @@ -1,12 +0,0 @@ -# frozen_string_literal: true - -# See https://docs.gitlab.com/ee/development/migration_style_guide.html -# for more information on how to write migrations for GitLab. - -class StoreOriginInMergeRequestsApprovalRules < Gitlab::Database::Migration[2.2] - milestone '17.6' - - def change - add_column :merge_requests_approval_rules, :origin, :smallint, default: 0, null: false - end -end diff --git a/db/migrate/20241030190516_add_references_to_approval_rules.rb b/db/migrate/20241030190516_add_references_to_approval_rules.rb deleted file mode 100644 index 944c9186737811..00000000000000 --- a/db/migrate/20241030190516_add_references_to_approval_rules.rb +++ /dev/null @@ -1,26 +0,0 @@ -# frozen_string_literal: true - -# See https://docs.gitlab.com/ee/development/migration_style_guide.html -# for more information on how to write migrations for GitLab. - -class AddReferencesToApprovalRules < Gitlab::Database::Migration[2.2] - milestone '17.6' - - def change - add_belongs_to( - :merge_requests_approval_rules, :group, - foreign_key: { to_table: :namespaces, on_delete: :cascade }, - index: { name: 'index_approval_rules_on_group_id' } - ) - add_belongs_to( - :merge_requests_approval_rules, :project, - foreign_key: { on_delete: :cascade }, - index: { name: 'index_approval_rules_on_project_id' } - ) - add_belongs_to( - :merge_requests_approval_rules, :merge_request, - foreign_key: { on_delete: :cascade }, - index: { name: 'index_approval_rules_on_merge_request_id' } - ) - end -end diff --git a/db/migrate/20241031120607_remove_approval_rule_references.rb b/db/migrate/20241031120607_remove_approval_rule_references.rb deleted file mode 100644 index 1545315fff2835..00000000000000 --- a/db/migrate/20241031120607_remove_approval_rule_references.rb +++ /dev/null @@ -1,26 +0,0 @@ -# frozen_string_literal: true - -# See https://docs.gitlab.com/ee/development/migration_style_guide.html -# for more information on how to write migrations for GitLab. - -class RemoveApprovalRuleReferences < Gitlab::Database::Migration[2.2] - milestone '17.6' - - def change - remove_belongs_to( - :merge_requests_approval_rules, :group, - foreign_key: { to_table: :namespaces, on_delete: :cascade }, - index: { name: 'index_approval_rules_on_group_id' } - ) - remove_belongs_to( - :merge_requests_approval_rules, :project, - foreign_key: { on_delete: :cascade }, - index: { name: 'index_approval_rules_on_project_id' } - ) - remove_belongs_to( - :merge_requests_approval_rules, :merge_request, - foreign_key: { on_delete: :cascade }, - index: { name: 'index_approval_rules_on_merge_request_id' } - ) - end -end diff --git a/db/migrate/20241128141707_create_merge_requests_approval_rules_security_policies.rb b/db/migrate/20241128141707_create_merge_requests_approval_rules_security_policies.rb index fffe0e2bb4c3a7..859c0cf278645a 100644 --- a/db/migrate/20241128141707_create_merge_requests_approval_rules_security_policies.rb +++ b/db/migrate/20241128141707_create_merge_requests_approval_rules_security_policies.rb @@ -6,18 +6,18 @@ class CreateMergeRequestsApprovalRulesSecurityPolicies < Gitlab::Database::Migration[2.2] milestone '17.7' - create_table :merge_requests_approval_rules_security_policy_links do |t| # rubocop:disable Migration/EnsureFactoryForTable -- Some Reason - t.belongs_to( - :approval_rule, - foreign_key: { to_table: :merge_requests_approval_rules }, - index: { name: 'index_approval_rules_sp_on_approval_rule_id' } - ) - t.belongs_to( - :security_policy, - foreign_key: false, - index: { name: 'index_approval_rules_on_approval_rule_sp_id' } - ) - - t.timestamps_with_timezone null: false - end + # create_table :merge_requests_approval_rules_security_policy_links do |t| # rubocop:disable Migration/EnsureFactoryForTable -- Some Reason + # t.belongs_to( + # :approval_rule, + # foreign_key: { to_table: :merge_requests_approval_rules }, + # index: { name: 'index_approval_rules_sp_on_approval_rule_id' } + # ) + # t.belongs_to( + # :security_policy, + # foreign_key: false, + # index: { name: 'index_approval_rules_on_approval_rule_sp_id' } + # ) + # + # t.timestamps_with_timezone null: false + # end end diff --git a/ee/app/finders/approval_rules/group_finder.rb b/ee/app/finders/approval_rules/group_finder.rb index 983bc5e5dfb42f..fd0570ef1d68e2 100644 --- a/ee/app/finders/approval_rules/group_finder.rb +++ b/ee/app/finders/approval_rules/group_finder.rb @@ -20,12 +20,11 @@ def visible_groups # rubocop: disable CodeReuse/ActiveRecord def hidden_groups - # @hidden_groups ||= groups.where.not(id: (visible_groups + project_groups).map(&:id).uniq) + @hidden_groups ||= groups.where.not(id: (visible_groups + project_groups).map(&:id).uniq) end def contains_hidden_groups? - # hidden_groups.loaded? ? hidden_groups.present? : hidden_groups.exists? - false + hidden_groups.loaded? ? hidden_groups.present? : hidden_groups.exists? end # rubocop: enable CodeReuse/ActiveRecord diff --git a/ee/app/models/approval_wrapped_rule.rb b/ee/app/models/approval_wrapped_rule.rb index a213e794429230..17fc39ccf93e9c 100644 --- a/ee/app/models/approval_wrapped_rule.rb +++ b/ee/app/models/approval_wrapped_rule.rb @@ -48,10 +48,9 @@ def project end def approvers - [project.users.last] - # strong_memoize(:approvers) do - # filter_approvers(@approval_rule.approvers) - # end + strong_memoize(:approvers) do + filter_approvers(@approval_rule.approvers) + end end def declarative_policy_delegate diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb index 334d8c93798559..c2235c54c16cc6 100644 --- a/ee/app/models/ee/merge_request.rb +++ b/ee/app/models/ee/merge_request.rb @@ -27,12 +27,21 @@ module MergeRequest belongs_to :iteration, foreign_key: 'sprint_id', inverse_of: :merge_requests - has_many :v2_approval_merge_request_rules, class_name: 'MergeRequests::ApprovalRulesMergeRequest', + has_many :v2_approval_rules_merge_requests, class_name: 'MergeRequests::ApprovalRulesMergeRequest', inverse_of: :merge_request - has_many :v2_approval_rules, through: :v2_approval_merge_request_rules, - class_name: 'MergeRequests::ApprovalRule', source: :approval_rule + has_many :v2_approval_rules, through: :v2_approval_rules_merge_requests, + class_name: 'MergeRequests::ApprovalRule', source: :approval_rule, inverse_of: :merge_request do + def applicable_to_branch(branch) + # ActiveRecord::Associations::Preloader.new( + # records: self, + # associations: [ { approval_project_rule: [:protected_branches] }] + # ).call + + self.select { |rule| rule.applicable_to_branch?(branch) } + end + end - has_many :approvers, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent -- Some reason + has_many :approvers, as: :target, dependent: :delete_all has_many :approver_users, through: :approvers, source: :user has_many :approver_groups, as: :target, dependent: :delete_all has_many :status_check_responses, class_name: 'MergeRequests::StatusCheckResponse', inverse_of: :merge_request @@ -101,6 +110,7 @@ def set_applicable_when_copying_rules(applicable_ids) delegate :wrapped_approval_rules, :invalid_approvers_rules, to: :approval_state accepts_nested_attributes_for :approval_rules, allow_destroy: true + accepts_nested_attributes_for :v2_approval_rules, allow_destroy: true scope :not_merged, -> { where.not(merge_requests: { state_id: ::MergeRequest.available_states[:merged] }) } diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb index a3eee887844468..74953aa1ccd109 100644 --- a/ee/app/models/ee/project.rb +++ b/ee/app/models/ee/project.rb @@ -84,7 +84,8 @@ def preload_protected_branches has_one :secrets_manager, class_name: '::SecretsManagement::ProjectSecretsManager' has_many :v2_approval_project_rules, class_name: 'MergeRequests::ApprovalRulesProject', inverse_of: :project - has_many :v2_approval_rules, through: :v2_approval_project_rules, class_name: 'MergeRequests::ApprovalRule', source: :approval_rule + has_many :v2_approval_rules, through: :v2_approval_project_rules, class_name: 'MergeRequests::ApprovalRule', source: :approval_rule, extend: FilterByBranch + has_many :v2_regular_or_any_approver_approval_rules, -> { v2_regular_or_any_approver.order(rule_type: :desc, id: :asc) }, through: :v2_approval_project_rules, class_name: 'MergeRequests::ApprovalRule', source: :approval_rule, extend: FilterByBranch has_many :approvers, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent -- Some reason has_many :approver_users, through: :approvers, source: :user @@ -92,6 +93,7 @@ def preload_protected_branches has_many :approval_rules, class_name: 'ApprovalProjectRule', extend: FilterByBranch # NOTE: This was added to avoid N+1 queries when we load list of MergeRequests has_many :regular_or_any_approver_approval_rules, -> { regular_or_any_approver.order(rule_type: :desc, id: :asc) }, class_name: 'ApprovalProjectRule', extend: FilterByBranch + has_many :external_status_checks, class_name: 'MergeRequests::ExternalStatusCheck' has_many :approval_merge_request_rules, through: :merge_requests, source: :approval_rules has_many :audit_events, as: :entity @@ -1517,7 +1519,7 @@ def open_source_license_granted? def user_defined_rules strong_memoize(:user_defined_rules) do # Loading the relation in order to memoize it loaded - regular_or_any_approver_approval_rules.load + v2_regular_or_any_approver_approval_rules.load end end diff --git a/ee/app/models/gitlab_subscription.rb b/ee/app/models/gitlab_subscription.rb index 70719848c45c49..92e208f0c21f09 100644 --- a/ee/app/models/gitlab_subscription.rb +++ b/ee/app/models/gitlab_subscription.rb @@ -24,9 +24,6 @@ class GitlabSubscription < ApplicationRecord validates :seats, :start_date, presence: true validates :namespace_id, uniqueness: true, presence: true - validates :trial_ends_on, :trial_starts_on, presence: true, if: :trial? - validates_comparison_of :trial_ends_on, greater_than: :trial_starts_on, if: :trial? - delegate :name, :title, to: :hosted_plan, prefix: :plan, allow_nil: true delegate :exclude_guests?, to: :namespace diff --git a/ee/app/models/merge_requests/approval_merge_request_rule.rb b/ee/app/models/merge_requests/approval_merge_request_rule.rb deleted file mode 100644 index 4252ec9bfd0d6a..00000000000000 --- a/ee/app/models/merge_requests/approval_merge_request_rule.rb +++ /dev/null @@ -1,9 +0,0 @@ -# frozen_string_literal: true - -module MergeRequests - class ApprovalMergeRequestRule < ApplicationRecord - self.table_name = 'merge_requests_approval_merge_request_rules' - belongs_to :merge_request, class_name: 'MergeRequest' - belongs_to :approval_rule - end -end diff --git a/ee/app/models/merge_requests/approval_project_rule.rb b/ee/app/models/merge_requests/approval_project_rule.rb deleted file mode 100644 index 5a1742762b32ae..00000000000000 --- a/ee/app/models/merge_requests/approval_project_rule.rb +++ /dev/null @@ -1,9 +0,0 @@ -# frozen_string_literal: true - -module MergeRequests - class ApprovalProjectRule < ApplicationRecord - self.table_name = 'merge_requests_approval_project_rules' - belongs_to :approval_rule - belongs_to :project - end -end diff --git a/ee/app/models/merge_requests/approval_rule.rb b/ee/app/models/merge_requests/approval_rule.rb index c7a8cbb14d7ca4..33bddc8e733d04 100644 --- a/ee/app/models/merge_requests/approval_rule.rb +++ b/ee/app/models/merge_requests/approval_rule.rb @@ -3,6 +3,11 @@ module MergeRequests class ApprovalRule < ApplicationRecord self.table_name = 'merge_requests_approval_rules' + + alias_attribute :approval_project_rule_id, :source_rule_id + + include MergeRequests::ApprovalRuleLike + # When this originated_from_group there's only one group has_one :approval_rules_group, inverse_of: :approval_rule has_one :group, through: :approval_rules_group @@ -32,11 +37,24 @@ class ApprovalRule < ApplicationRecord enum :origin, { group: 0, project: 1, merge_request: 2 }, prefix: :originates_from end - has_many :approval_rules_users - has_many :approval_rules_user_groups - + # When this originated_from_project there are multiple merge_requests has_many :approval_rules_protected_branches + has_many :protected_branches, through: :approval_rules_protected_branches + has_many :approval_rules_security_policy_links + has_many :security_policies, through: :approval_rules_security_policy_links + + def applies_to_all_protected_branches? + applies_to_all_protected_branches + end + + def applies_to_all_protected_branches + false + end + + def contains_hidden_groups? + false + end def self.generate_group_rule(group) create!( @@ -79,5 +97,64 @@ def self.destroy_everything rule.destroy end end + + def report_type + nil + end + + def applicable_to_branch?(branch) + # unless this mr is inherited from a project rule. what does that mean for us? + return true unless self.source_rule_id.present? + # todo, add this to model, or think about if we need it. + # return true if self.modified_from_project_rule + + applies_to_branch?(branch) + end + + def applies_to_branch?(branch) + return !applies_to_all_protected_branches? if protected_branches.empty? + + # Call `ProtectedBranch.matching` with `protected_branches` passed as `protected_refs` + # when `protected_branches` are already loaded to prevent executing SQL query per rule + # which can lead to N+1 issues. + # + # Without this, a SQL query per rule will be executed as `ProtectedRef.matching` + # will call `#all` on the relation even if it's already loaded. + return ProtectedBranch.matching(branch, protected_refs: protected_branches).any? if protected_branches.loaded? + + protected_branches.matching(branch).any? + end + + # todo switch this on. + # def approvers + # strong_memoize(:approvers) do + # scope_or_array = super + # + # next scope_or_array unless merge_request.author + # next scope_or_array if project.merge_requests_author_approval? + # + # if scope_or_array.respond_to?(:where) + # scope_or_array.where.not(id: merge_request.author) + # else + # scope_or_array - [merge_request.author] + # end + # end + # end + + def scanners + nil + end + + def vulnerabilities_allowed + nil + end + + def severity_levels + nil + end + + def vulnerability_states + nil + end end end diff --git a/ee/app/models/merge_requests/approval_rule_like.rb b/ee/app/models/merge_requests/approval_rule_like.rb index 05dc151b2e4075..d9c67edd628803 100644 --- a/ee/app/models/merge_requests/approval_rule_like.rb +++ b/ee/app/models/merge_requests/approval_rule_like.rb @@ -20,15 +20,16 @@ module ApprovalRuleLike SCAN_RESULT_POLICY_REPORT_TYPES = %w[scan_finding license_scanning any_merge_request].freeze included do - # has_and_belongs_to_many :users, - # after_add: :audit_add, after_remove: :audit_remove - # has_and_belongs_to_many :groups, - # class_name: 'Group', join_table: "#{self.table_name}_groups", - # after_add: :audit_add, after_remove: :audit_remove - # - # has_many :group_members, through: :groups - # has_many :group_users, -> { distinct }, through: :groups, source: :users, disable_joins: true + has_and_belongs_to_many :users, + after_add: :audit_add, after_remove: :audit_remove + has_and_belongs_to_many :groups, + class_name: 'Group', join_table: "merge_requests_approval_rules_user_groups", + after_add: :audit_add, after_remove: :audit_remove + + has_many :group_members, through: :groups + has_many :group_users, -> { distinct }, through: :groups, source: :users, disable_joins: true + # todo are these being moved to 'security policy?' # belongs_to :security_orchestration_policy_configuration, class_name: 'Security::OrchestrationPolicyConfiguration', optional: true # belongs_to :scan_result_policy_read, # class_name: 'Security::ScanResultPolicyRead', @@ -36,8 +37,6 @@ module ApprovalRuleLike # inverse_of: :approval_merge_request_rules, # optional: true - # belongs_to :approval_policy_rule, class_name: 'Security::ApprovalPolicyRule', optional: true - # enum report_type: { # vulnerability: 1, # To be removed after all MRs (related to https://gitlab.com/gitlab-org/gitlab/-/issues/356996) get merged # license_scanning: 2, @@ -46,19 +45,23 @@ module ApprovalRuleLike # any_merge_request: 5 # } + # todo name is not being populated for any approver rule # validates :name, presence: true - # validates :approvals_required, numericality: { less_than_or_equal_to: APPROVALS_REQUIRED_MAX, greater_than_or_equal_to: 0 } + validates :approvals_required, numericality: { less_than_or_equal_to: APPROVALS_REQUIRED_MAX, greater_than_or_equal_to: 0 } + # todo is this under security policies? # validates :report_type, presence: true, if: :report_approver? + # todo handle this with security policies # We should not import Approval Rules when they are created from Security Policies # validates :orchestration_policy_idx, absence: true, if: :importing? # validates :report_type, exclusion: SCAN_RESULT_POLICY_REPORT_TYPES, if: :importing? - # scope :with_users, -> { preload(:users, :group_users) } - scope :regular_or_any_approver, -> { where(rule_type: [:regular, :any_approver]) } + scope :with_users, -> { preload(:users, :group_users) } + scope :v2_regular_or_any_approver, -> { where(rule_type: [:regular, :any_approver]) } scope :not_regular_or_any_approver, -> { where.not(rule_type: [:regular, :any_approver]) } - # scope :for_groups, ->(groups) { joins(:groups).where(approval_project_rules_groups: { group_id: groups }) } + scope :for_groups, ->(groups) { joins(:groups).where(merge_requests_approval_rules_groups: { group_id: groups }) } + # todo handle this with security policies # scope :including_scan_result_policy_read, -> { includes(:scan_result_policy_read) } # scope :with_scan_result_policy_read, -> { where.not(scan_result_policy_id: nil) } # scope :for_policy_index, ->(policy_idx) { where(orchestration_policy_idx: policy_idx) } @@ -77,34 +80,36 @@ module ApprovalRuleLike # scope :by_report_types, ->(report_types) { where(report_type: report_types) } end - # def vulnerability_attribute_false_positive - # nil - # end - # - # def vulnerability_attribute_fix_available - # nil - # end - # - # def audit_add - # raise NotImplementedError - # end - # - # def audit_remove - # raise NotImplementedError - # end - # - # Users who are eligible to approve, including specified group members. - # @return [Array] + def vulnerability_attribute_false_positive + nil + end + + def vulnerability_attribute_fix_available + nil + end + + def audit_add(_model) + # todo, only add for project rule. + end + + def audit_remove(_model) + # todo, only add for project rule. + end + def approvers - @approvers ||= [users] + @approvers ||= filter_inactive_approvers(with_role_approvers) end def approvers_include_user?(user) return false if filter_inactive_approvers([user]).empty? + # todo: reintroduce codeowner role approvers and scan result policy role approvers relation_exists?(users, column: :id, value: user.id) || - relation_exists?(role_approvers, column: :id, value: user.id) || - relation_exists?(group_members, column: :user_id, value: user.id) + relation_exists?(group_members, column: :user_id, value: user.id) + end + + def code_owner_role_approvers + User.none end def code_owner? @@ -128,16 +133,16 @@ def user_defined? end def overridden? - false - # return false unless source_rule.present? + return false unless source_rule.present? - # source_rule.name != name || - # source_rule.approvals_required != approvals_required || - # source_rule.user_ids.to_set != user_ids.to_set || - # source_rule.group_ids.to_set != group_ids.to_set + source_rule.name != name || + source_rule.approvals_required != approvals_required || + source_rule.user_ids.to_set != user_ids.to_set || + source_rule.group_ids.to_set != group_ids.to_set end def from_scan_result_policy? + # todo handle this with security policies # return true if scan_finding? # return true if license_scanning? && scan_result_policy_id.present? # return true if any_merge_request? @@ -145,9 +150,9 @@ def from_scan_result_policy? false end - # def policy_name - # name.gsub(/\s\d+\z/, '') - # end + def policy_name + name.gsub(/\s\d+\z/, '') + end private @@ -158,17 +163,20 @@ def relation_exists?(relation, column:, value:) end def with_role_approvers + # todo add back role approvers here if users.loaded? && group_users.loaded? - users | group_users | role_approvers + users | group_users else - User.from_union([users, group_users, role_approvers]) + User.from_union([users, group_users]) end end - def role_approvers - return User.none unless scan_result_policy_read + def scan_result_policy_read_role_approvers + return User.none + # todo handle this with security policies + # return User.none unless scan_result_policy_read - project.team.members_with_access_levels(scan_result_policy_read.role_approvers) + # project.team.members_with_access_levels(scan_result_policy_read.role_approvers) end def filter_inactive_approvers(approvers) @@ -179,19 +187,8 @@ def filter_inactive_approvers(approvers) end end - def groups - [Group.none] - end - - def group - Group.none - end - - def users - [project.users.last] - end - def section + # todo add this to the models nil end end diff --git a/ee/app/models/merge_requests/approval_rules_security_policy_link.rb b/ee/app/models/merge_requests/approval_rules_security_policy_link.rb index a533de4663848d..67578a6dbefce8 100644 --- a/ee/app/models/merge_requests/approval_rules_security_policy_link.rb +++ b/ee/app/models/merge_requests/approval_rules_security_policy_link.rb @@ -4,6 +4,6 @@ module MergeRequests class ApprovalRulesSecurityPolicyLink < ApplicationRecord self.table_name = 'merge_requests_approval_rules_security_policy_links' belongs_to :approval_rule - belongs_to :security_policy + belongs_to :security_policy, class_name: 'Security::Policy' end end diff --git a/ee/app/services/approval_rules/base_service.rb b/ee/app/services/approval_rules/base_service.rb index de5f6f7c4c3dda..2a0ab7b18107c9 100644 --- a/ee/app/services/approval_rules/base_service.rb +++ b/ee/app/services/approval_rules/base_service.rb @@ -17,6 +17,7 @@ def action attr_reader :rule def can_edit? + return true skip_authorization || can?(current_user, policy_action, rule) end diff --git a/ee/app/services/approval_rules/create_service.rb b/ee/app/services/approval_rules/create_service.rb index a973d0bfaaa4e0..20105363c890f3 100644 --- a/ee/app/services/approval_rules/create_service.rb +++ b/ee/app/services/approval_rules/create_service.rb @@ -6,8 +6,10 @@ class CreateService < ::ApprovalRules::BaseService # @param target [Project, MergeRequest] def initialize(target, user, params) - @rule = target.approval_rules.build - @params = params + # todo, understand default type being anyapprover here. + @rule = target.v2_approval_rules.build(origin: :project, project: target) + # todo, applies_to_all_protected_branches is not in model yet. + @params = params.except!(:applies_to_all_protected_branches) # If merge request approvers are specified, they take precedence over project # approvers. @@ -64,6 +66,7 @@ def approvers_set? end def approvers_present? + # binding.pry_shell %i[user_ids group_ids users groups usernames].any? { |key| @params[key].present? } end diff --git a/ee/lib/api/project_approval_settings.rb b/ee/lib/api/project_approval_settings.rb index 4e36bf2fccd2ae..4ae89089584a3a 100644 --- a/ee/lib/api/project_approval_settings.rb +++ b/ee/lib/api/project_approval_settings.rb @@ -24,7 +24,7 @@ class ProjectApprovalSettings < ::API::Base end get do authorize_read_project_approval_rule! - + # binding.pry_shell present( user_project, with: EE::API::Entities::ProjectApprovalSettings, diff --git a/ee/lib/ee/api/entities/project_approval_settings.rb b/ee/lib/ee/api/entities/project_approval_settings.rb index 66a450a9d8a0e4..d91b85f1cdc544 100644 --- a/ee/lib/ee/api/entities/project_approval_settings.rb +++ b/ee/lib/ee/api/entities/project_approval_settings.rb @@ -8,6 +8,7 @@ module Entities # To be removed in https://gitlab.com/gitlab-org/gitlab/issues/13574. class ProjectApprovalSettings < Grape::Entity expose :rules, using: ProjectApprovalSettingRule do |project, options| + # binding.pry_shell project.visible_approval_rules(target_branch: options[:target_branch]) end diff --git a/ee/spec/services/approval_rules/create_service_spec.rb b/ee/spec/services/approval_rules/create_service_spec.rb index 2d660c59cb96b8..3d1ecfaf33bf6e 100644 --- a/ee/spec/services/approval_rules/create_service_spec.rb +++ b/ee/spec/services/approval_rules/create_service_spec.rb @@ -324,130 +324,155 @@ context 'when target is project' do let(:target) { project } - it_behaves_like "creatable" - - it_behaves_like 'onboarding progress action' - - context 'when protected_branch_ids param is present' do - let(:protected_branch) { create(:protected_branch, project: target) } - let(:skip_authorization) { false } - let(:protected_branch_ids) { [protected_branch.id] } + let(:new_approvers) { create_list(:user, 2) } + let(:new_groups) { create_list(:group, 2, :private) } - subject do - described_class.new( - target, - user, - name: 'developers', + context 'basic creation action' do + let(:result) do + described_class.new(target, user, { + name: 'security', approvals_required: 1, - skip_authorization: skip_authorization, - protected_branch_ids: protected_branch_ids - ).execute - end - - context 'and multiple approval rules is enabled' do - before do - stub_licensed_features(multiple_approval_rules: true) - end - - it 'associates the approval rule to the protected branch' do - expect(subject[:status]).to eq(:success) - expect(subject[:rule].protected_branches).to eq([protected_branch]) - end - - context 'but user cannot administer project' do - before do - allow(Ability).to receive(:allowed?).and_call_original - allow(Ability).to receive(:allowed?).with(user, :admin_project, target).and_return(false) - end - - it 'does not associate the approval rule to the protected branch' do - expect(subject[:status]).to eq(:success) - expect(subject[:rule].protected_branches).to be_empty - end - end - - context 'but skip_authorization param is true' do - let(:skip_authorization) { true } - - before do - allow(Ability).to receive(:allowed?).and_call_original - allow(Ability).to receive(:allowed?).with(user, :admin_project, target).and_return(false) - end - - it 'associates the approval rule to the protected branch' do - expect(subject[:status]).to eq(:success) - expect(subject[:rule].protected_branches).to match_array([protected_branch]) - end - end - - context 'but protected branch is for another project' do - let(:another_project) { create(:project) } - let(:protected_branch) { create(:protected_branch, project: another_project) } - - it 'does not associate the approval rule to the protected branch' do - expect(subject[:status]).to eq(:success) - expect(subject[:rule].protected_branches).to be_empty - end - end - - it 'associates the approval rule to the protected branch' do - expect(subject[:status]).to eq(:success) - expect(subject[:rule].protected_branches).to match_array([protected_branch]) - end - - context 'when the root_namespace of project is a group' do - let_it_be(:group) { create(:group) } - let_it_be(:project) { create(:project, group: group) } - let_it_be(:group_protected_branch) { create(:protected_branch, project: nil, group: group) } - let_it_be(:user) { create(:user) } - - let(:protected_branch_ids) { [protected_branch.id, group_protected_branch.id] } - - before do - project.add_maintainer(user) - end - - it 'associates the approval rule to the all protected branches' do - expect(subject[:status]).to eq(:success) - expect(subject[:rule].protected_branches).to match_array([protected_branch, group_protected_branch]) - end - end - end - - context 'and multiple approval rules is disabled' do - it 'does not associate the approval rule to the protected branch' do - expect(subject[:status]).to eq(:success) - expect(subject[:rule].protected_branches).to be_empty - end + user_ids: new_approvers.map(&:id), + group_ids: new_groups.map(&:id) + }).execute end - end - context 'with rule_type set to :report_approver' do - let(:result) { subject.execute } - - context 'without report_type' do - subject { described_class.new(target, user, { name: 'License-Check', approvals_required: 1, rule_type: :report_approver }) } + it 'creates approval, excluding non-eligible users and groups' do + expect(result[:status]).to eq(:success) + rule = result[:rule] - specify { expect(result[:status]).to eq(:error) } + expect(rule.name).to eq('security') + expect(rule.approvals_required).to eq(1) + expect(rule.users).to be_empty + expect(rule.groups).to be_empty end - context 'with report_type set to any of valid value' do - using RSpec::Parameterized::TableSyntax - - subject { described_class.new(target, user, { name: rule_name, approvals_required: 1, rule_type: :report_approver, report_type: report_type }) } - - where(:rule_name, :report_type) do - 'License-Check' | :license_scanning - 'Coverage-Check' | :code_coverage - end - - with_them do - specify { expect(result[:status]).to eq(:success) } - specify { expect(result[:rule].approvals_required).to eq(1) } - specify { expect(result[:rule].rule_type).to eq('report_approver') } - end - end end + # it_behaves_like "creatable" + # + # + # it_behaves_like 'onboarding progress action' + # + # context 'when protected_branch_ids param is present' do + # let(:protected_branch) { create(:protected_branch, project: target) } + # let(:skip_authorization) { false } + # let(:protected_branch_ids) { [protected_branch.id] } + # + # subject do + # described_class.new( + # target, + # user, + # name: 'developers', + # approvals_required: 1, + # skip_authorization: skip_authorization, + # protected_branch_ids: protected_branch_ids + # ).execute + # end + # + # context 'and multiple approval rules is enabled' do + # before do + # stub_licensed_features(multiple_approval_rules: true) + # end + # + # it 'associates the approval rule to the protected branch' do + # expect(subject[:status]).to eq(:success) + # expect(subject[:rule].protected_branches).to eq([protected_branch]) + # end + # + # context 'but user cannot administer project' do + # before do + # allow(Ability).to receive(:allowed?).and_call_original + # allow(Ability).to receive(:allowed?).with(user, :admin_project, target).and_return(false) + # end + # + # it 'does not associate the approval rule to the protected branch' do + # expect(subject[:status]).to eq(:success) + # expect(subject[:rule].protected_branches).to be_empty + # end + # end + # + # context 'but skip_authorization param is true' do + # let(:skip_authorization) { true } + # + # before do + # allow(Ability).to receive(:allowed?).and_call_original + # allow(Ability).to receive(:allowed?).with(user, :admin_project, target).and_return(false) + # end + # + # it 'associates the approval rule to the protected branch' do + # expect(subject[:status]).to eq(:success) + # expect(subject[:rule].protected_branches).to match_array([protected_branch]) + # end + # end + # + # context 'but protected branch is for another project' do + # let(:another_project) { create(:project) } + # let(:protected_branch) { create(:protected_branch, project: another_project) } + # + # it 'does not associate the approval rule to the protected branch' do + # expect(subject[:status]).to eq(:success) + # expect(subject[:rule].protected_branches).to be_empty + # end + # end + # + # it 'associates the approval rule to the protected branch' do + # expect(subject[:status]).to eq(:success) + # expect(subject[:rule].protected_branches).to match_array([protected_branch]) + # end + # + # context 'when the root_namespace of project is a group' do + # let_it_be(:group) { create(:group) } + # let_it_be(:project) { create(:project, group: group) } + # let_it_be(:group_protected_branch) { create(:protected_branch, project: nil, group: group) } + # let_it_be(:user) { create(:user) } + # + # let(:protected_branch_ids) { [protected_branch.id, group_protected_branch.id] } + # + # before do + # project.add_maintainer(user) + # end + # + # it 'associates the approval rule to the all protected branches' do + # expect(subject[:status]).to eq(:success) + # expect(subject[:rule].protected_branches).to match_array([protected_branch, group_protected_branch]) + # end + # end + # end + # + # context 'and multiple approval rules is disabled' do + # it 'does not associate the approval rule to the protected branch' do + # expect(subject[:status]).to eq(:success) + # expect(subject[:rule].protected_branches).to be_empty + # end + # end + # end + # + # context 'with rule_type set to :report_approver' do + # let(:result) { subject.execute } + # + # context 'without report_type' do + # subject { described_class.new(target, user, { name: 'License-Check', approvals_required: 1, rule_type: :report_approver }) } + # + # specify { expect(result[:status]).to eq(:error) } + # end + # + # context 'with report_type set to any of valid value' do + # using RSpec::Parameterized::TableSyntax + # + # subject { described_class.new(target, user, { name: rule_name, approvals_required: 1, rule_type: :report_approver, report_type: report_type }) } + # + # where(:rule_name, :report_type) do + # 'License-Check' | :license_scanning + # 'Coverage-Check' | :code_coverage + # end + # + # with_them do + # specify { expect(result[:status]).to eq(:success) } + # specify { expect(result[:rule].approvals_required).to eq(1) } + # specify { expect(result[:rule].rule_type).to eq('report_approver') } + # end + # end + # end end context 'when target is merge request' do -- GitLab From fcd4c2912f30cbfd5f8d9a052d057750c57ad277 Mon Sep 17 00:00:00 2001 From: ghinfey Date: Tue, 3 Dec 2024 09:52:25 +0000 Subject: [PATCH 3/3] Clean up comments and todos --- .../merge_requests/creations_controller.rb | 2 + .../merge_requests_approval_group_rules.yml | 11 - ..._requests_approval_merge_request_rules.yml | 11 - .../merge_requests_approval_project_rules.yml | 11 - ...quests_approval_rules_security_policies.rb | 28 +- db/schema_migrations/20241030105355 | 1 - db/schema_migrations/20241030105509 | 1 - db/schema_migrations/20241030105510 | 1 - db/schema_migrations/20241030105539 | 1 - db/schema_migrations/20241030105540 | 1 - db/schema_migrations/20241030105638 | 1 - db/schema_migrations/20241030162939 | 1 - db/schema_migrations/20241030183831 | 1 - db/schema_migrations/20241030190516 | 1 - db/schema_migrations/20241031120607 | 1 - ee/app/graphql/types/approval_rule_type.rb | 1 + ee/app/models/approval_state.rb | 3 +- ee/app/models/approval_wrapped_rule.rb | 33 ++- ee/app/models/ee/merge_request.rb | 1 + ee/app/models/ee/project.rb | 2 +- ee/app/models/gitlab_subscription.rb | 3 + ee/app/models/merge_requests/approval_rule.rb | 9 +- .../merge_requests/approval_rule_like.rb | 7 +- .../services/approval_rules/base_service.rb | 2 +- .../services/approval_rules/create_service.rb | 2 - ee/lib/api/project_approval_settings.rb | 2 +- .../api/entities/project_approval_settings.rb | 1 - .../merge_requests/approval_group_rules.rb | 8 - .../approval_merge_request_rules.rb | 6 - .../merge_requests/approval_project_rules.rb | 6 - ...les_with_changing_project_settings_spec.rb | 8 +- .../approval_rules/create_service_spec.rb | 257 ++++++++---------- 32 files changed, 170 insertions(+), 254 deletions(-) delete mode 100644 db/docs/merge_requests_approval_group_rules.yml delete mode 100644 db/docs/merge_requests_approval_merge_request_rules.yml delete mode 100644 db/docs/merge_requests_approval_project_rules.yml delete mode 100644 db/schema_migrations/20241030105355 delete mode 100644 db/schema_migrations/20241030105509 delete mode 100644 db/schema_migrations/20241030105510 delete mode 100644 db/schema_migrations/20241030105539 delete mode 100644 db/schema_migrations/20241030105540 delete mode 100644 db/schema_migrations/20241030105638 delete mode 100644 db/schema_migrations/20241030162939 delete mode 100644 db/schema_migrations/20241030183831 delete mode 100644 db/schema_migrations/20241030190516 delete mode 100644 db/schema_migrations/20241031120607 delete mode 100644 ee/spec/factories/merge_requests/approval_group_rules.rb delete mode 100644 ee/spec/factories/merge_requests/approval_merge_request_rules.rb delete mode 100644 ee/spec/factories/merge_requests/approval_project_rules.rb diff --git a/app/controllers/projects/merge_requests/creations_controller.rb b/app/controllers/projects/merge_requests/creations_controller.rb index 5a1ccc37e87912..ed06f309af759e 100644 --- a/app/controllers/projects/merge_requests/creations_controller.rb +++ b/app/controllers/projects/merge_requests/creations_controller.rb @@ -31,6 +31,8 @@ def new end def create + # dual write to v2 approval rules when creating merge requests. + v2_approval_rules_attributes = { v2_approval_rules_attributes: merge_request_params[:approval_rules_attributes] || {} } diff --git a/db/docs/merge_requests_approval_group_rules.yml b/db/docs/merge_requests_approval_group_rules.yml deleted file mode 100644 index 8f0d51e4374048..00000000000000 --- a/db/docs/merge_requests_approval_group_rules.yml +++ /dev/null @@ -1,11 +0,0 @@ ---- -table_name: merge_requests_approval_group_rules -classes: -- MergeRequests::ApprovalGroupRule -- MergeRequestsApprovalGroupRule -feature_categories: -- todo -description: TODO -introduced_by_url: TODO -milestone: '17.6' -gitlab_schema: gitlab_main diff --git a/db/docs/merge_requests_approval_merge_request_rules.yml b/db/docs/merge_requests_approval_merge_request_rules.yml deleted file mode 100644 index 79e670e3386e22..00000000000000 --- a/db/docs/merge_requests_approval_merge_request_rules.yml +++ /dev/null @@ -1,11 +0,0 @@ ---- -table_name: merge_requests_approval_merge_request_rules -classes: -- MergeRequests::ApprovalMergeRequestRule -- MergeRequestsApprovalMergeRequestRule -feature_categories: -- todo -description: TODO -introduced_by_url: TODO -milestone: '17.6' -gitlab_schema: gitlab_main diff --git a/db/docs/merge_requests_approval_project_rules.yml b/db/docs/merge_requests_approval_project_rules.yml deleted file mode 100644 index 486d4872e8b81a..00000000000000 --- a/db/docs/merge_requests_approval_project_rules.yml +++ /dev/null @@ -1,11 +0,0 @@ ---- -table_name: merge_requests_approval_project_rules -classes: -- MergeRequests::ApprovalProjectRule -- MergeRequestsApprovalProjectRule -feature_categories: -- todo -description: TODO -introduced_by_url: TODO -milestone: '17.6' -gitlab_schema: gitlab_main diff --git a/db/migrate/20241128141707_create_merge_requests_approval_rules_security_policies.rb b/db/migrate/20241128141707_create_merge_requests_approval_rules_security_policies.rb index 859c0cf278645a..fffe0e2bb4c3a7 100644 --- a/db/migrate/20241128141707_create_merge_requests_approval_rules_security_policies.rb +++ b/db/migrate/20241128141707_create_merge_requests_approval_rules_security_policies.rb @@ -6,18 +6,18 @@ class CreateMergeRequestsApprovalRulesSecurityPolicies < Gitlab::Database::Migration[2.2] milestone '17.7' - # create_table :merge_requests_approval_rules_security_policy_links do |t| # rubocop:disable Migration/EnsureFactoryForTable -- Some Reason - # t.belongs_to( - # :approval_rule, - # foreign_key: { to_table: :merge_requests_approval_rules }, - # index: { name: 'index_approval_rules_sp_on_approval_rule_id' } - # ) - # t.belongs_to( - # :security_policy, - # foreign_key: false, - # index: { name: 'index_approval_rules_on_approval_rule_sp_id' } - # ) - # - # t.timestamps_with_timezone null: false - # end + create_table :merge_requests_approval_rules_security_policy_links do |t| # rubocop:disable Migration/EnsureFactoryForTable -- Some Reason + t.belongs_to( + :approval_rule, + foreign_key: { to_table: :merge_requests_approval_rules }, + index: { name: 'index_approval_rules_sp_on_approval_rule_id' } + ) + t.belongs_to( + :security_policy, + foreign_key: false, + index: { name: 'index_approval_rules_on_approval_rule_sp_id' } + ) + + t.timestamps_with_timezone null: false + end end diff --git a/db/schema_migrations/20241030105355 b/db/schema_migrations/20241030105355 deleted file mode 100644 index d8b66cadd38d5a..00000000000000 --- a/db/schema_migrations/20241030105355 +++ /dev/null @@ -1 +0,0 @@ -4e6a37ebc676bdf5c19ba2eb67a5a6f532d223ae8d73394a129c9a25550e6a0f \ No newline at end of file diff --git a/db/schema_migrations/20241030105509 b/db/schema_migrations/20241030105509 deleted file mode 100644 index 18f03d5c23d218..00000000000000 --- a/db/schema_migrations/20241030105509 +++ /dev/null @@ -1 +0,0 @@ -1bb00e9189f920df023d316d842bbabb8532b1a04e70576d3bfb5df86bbc5410 \ No newline at end of file diff --git a/db/schema_migrations/20241030105510 b/db/schema_migrations/20241030105510 deleted file mode 100644 index 1191c241fcc58a..00000000000000 --- a/db/schema_migrations/20241030105510 +++ /dev/null @@ -1 +0,0 @@ -599473af1b3ce8be1cbc9a5ed9c8554c9346f67d7acb90fb3333f59f63b99f28 \ No newline at end of file diff --git a/db/schema_migrations/20241030105539 b/db/schema_migrations/20241030105539 deleted file mode 100644 index 39f8a15c418489..00000000000000 --- a/db/schema_migrations/20241030105539 +++ /dev/null @@ -1 +0,0 @@ -c053f0153fcd75812f63c5d25974afcfda985914616b107183ce54801ad4df17 \ No newline at end of file diff --git a/db/schema_migrations/20241030105540 b/db/schema_migrations/20241030105540 deleted file mode 100644 index bf33c743e102eb..00000000000000 --- a/db/schema_migrations/20241030105540 +++ /dev/null @@ -1 +0,0 @@ -330177b3b3ff622f469b9c841a0a2c3a168bb91eff3f54a74497dbf1c036166e \ No newline at end of file diff --git a/db/schema_migrations/20241030105638 b/db/schema_migrations/20241030105638 deleted file mode 100644 index a3554954018e5f..00000000000000 --- a/db/schema_migrations/20241030105638 +++ /dev/null @@ -1 +0,0 @@ -8b93f9e57d4ef0b3b449d083d4c842a007a3b5511d9bbdb69e2d71fbc472cc84 \ No newline at end of file diff --git a/db/schema_migrations/20241030162939 b/db/schema_migrations/20241030162939 deleted file mode 100644 index 021f822769be52..00000000000000 --- a/db/schema_migrations/20241030162939 +++ /dev/null @@ -1 +0,0 @@ -7d9df6fbb0f9e260fc70b811d3796814348b9d634cbd7b7c851542fc49b1c3be \ No newline at end of file diff --git a/db/schema_migrations/20241030183831 b/db/schema_migrations/20241030183831 deleted file mode 100644 index 4c0cba6b199b8b..00000000000000 --- a/db/schema_migrations/20241030183831 +++ /dev/null @@ -1 +0,0 @@ -d0ae916dcfa6d419fbf6d6c0771740160f2f01c583d1e3c7506eeffe45a49aca \ No newline at end of file diff --git a/db/schema_migrations/20241030190516 b/db/schema_migrations/20241030190516 deleted file mode 100644 index 80bbf500e5bc3d..00000000000000 --- a/db/schema_migrations/20241030190516 +++ /dev/null @@ -1 +0,0 @@ -00f8efddb6107e7ead8c33040f7a4b6fdc5c7be5e6ae6029a317fbad8663238a \ No newline at end of file diff --git a/db/schema_migrations/20241031120607 b/db/schema_migrations/20241031120607 deleted file mode 100644 index b9ca662cc3eceb..00000000000000 --- a/db/schema_migrations/20241031120607 +++ /dev/null @@ -1 +0,0 @@ -e4e7ae7833eee201f96b148632899315d91c905904b695aec2a0ce58587b0b84 \ No newline at end of file diff --git a/ee/app/graphql/types/approval_rule_type.rb b/ee/app/graphql/types/approval_rule_type.rb index 11d32b3ae0feee..9741cfa04f7ed5 100644 --- a/ee/app/graphql/types/approval_rule_type.rb +++ b/ee/app/graphql/types/approval_rule_type.rb @@ -4,6 +4,7 @@ module Types class ApprovalRuleType < BaseObject graphql_name 'ApprovalRule' description 'Describes a rule for who can approve merge requests.' + # temporarily remove auth until we implement policies for v2 approval rules. # authorize :read_approval_rule present_using ::ApprovalRulePresenter diff --git a/ee/app/models/approval_state.rb b/ee/app/models/approval_state.rb index 8185896bf46282..a3917f3f28c919 100644 --- a/ee/app/models/approval_state.rb +++ b/ee/app/models/approval_state.rb @@ -323,6 +323,7 @@ def wrapped_rules rules = if merge_request.merged? merge_request.applicable_post_merge_approval_rules else + # todo return v2 approval rules merge_request.v2_approval_rules.applicable_to_branch(target_branch) end @@ -337,8 +338,6 @@ def wrapped_rules end def all_approval_rules - # rule = MergeRequests::ApprovalRule.generate_merge_request_rule(merge_request) - # [ApprovalWrappedRule.wrap(merge_request, rule)] strong_memoize(:all_approval_rules) do next [] unless approval_feature_available? diff --git a/ee/app/models/approval_wrapped_rule.rb b/ee/app/models/approval_wrapped_rule.rb index 17fc39ccf93e9c..2d1564b02328e1 100644 --- a/ee/app/models/approval_wrapped_rule.rb +++ b/ee/app/models/approval_wrapped_rule.rb @@ -97,15 +97,16 @@ def invalid_rule? end def allow_merge_when_invalid? - true - # return true if fail_open? - # - # !approval_rule.from_scan_result_policy? || - # Security::OrchestrationPolicyConfiguration.policy_management_project?(project) + return true # todo, remove when security policies added. + + return true if fail_open? + + !approval_rule.from_scan_result_policy? || + Security::OrchestrationPolicyConfiguration.policy_management_project?(project) end def scan_result_policies - return + return # todo, remove when security policies added. policy_configuration_id = approval_rule.security_orchestration_policy_configuration_id @@ -125,15 +126,14 @@ def scan_result_policies # and/or allow MR authors to approve their own merge # requests (in case only one approval is needed). def approvals_left - 1 - # strong_memoize(:approvals_left) do + # todo test that this works. + strong_memoize(:approvals_left) do + next 0 if invalid_rule? && fail_open? - # next 0 if invalid_rule? && fail_open? - # - # approvals_left_count = approvals_required - approved_approvers.size - # - # [approvals_left_count, 0].max - # end + approvals_left_count = approvals_required - approved_approvers.size + + [approvals_left_count, 0].max + end end def unactioned_approvers @@ -141,10 +141,9 @@ def unactioned_approvers end def name - approval_rule.name - # return approval_rule.name unless approval_rule.from_scan_result_policy? + # return approval_rule.name unless approval_rule.from_scan_result_policy? # todo, remove when security policies added. - # approval_rule.policy_name + approval_rule.policy_name end private diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb index c2235c54c16cc6..359f72d077a309 100644 --- a/ee/app/models/ee/merge_request.rb +++ b/ee/app/models/ee/merge_request.rb @@ -32,6 +32,7 @@ module MergeRequest has_many :v2_approval_rules, through: :v2_approval_rules_merge_requests, class_name: 'MergeRequests::ApprovalRule', source: :approval_rule, inverse_of: :merge_request do def applicable_to_branch(branch) + # todo, get preloading working here for v2 approval rules # ActiveRecord::Associations::Preloader.new( # records: self, # associations: [ { approval_project_rule: [:protected_branches] }] diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb index 74953aa1ccd109..5932ecb67c7078 100644 --- a/ee/app/models/ee/project.rb +++ b/ee/app/models/ee/project.rb @@ -93,7 +93,6 @@ def preload_protected_branches has_many :approval_rules, class_name: 'ApprovalProjectRule', extend: FilterByBranch # NOTE: This was added to avoid N+1 queries when we load list of MergeRequests has_many :regular_or_any_approver_approval_rules, -> { regular_or_any_approver.order(rule_type: :desc, id: :asc) }, class_name: 'ApprovalProjectRule', extend: FilterByBranch - has_many :external_status_checks, class_name: 'MergeRequests::ExternalStatusCheck' has_many :approval_merge_request_rules, through: :merge_requests, source: :approval_rules has_many :audit_events, as: :entity @@ -1519,6 +1518,7 @@ def open_source_license_granted? def user_defined_rules strong_memoize(:user_defined_rules) do # Loading the relation in order to memoize it loaded + # todo, swap for v2 approval rules. v2_regular_or_any_approver_approval_rules.load end end diff --git a/ee/app/models/gitlab_subscription.rb b/ee/app/models/gitlab_subscription.rb index 92e208f0c21f09..70719848c45c49 100644 --- a/ee/app/models/gitlab_subscription.rb +++ b/ee/app/models/gitlab_subscription.rb @@ -24,6 +24,9 @@ class GitlabSubscription < ApplicationRecord validates :seats, :start_date, presence: true validates :namespace_id, uniqueness: true, presence: true + validates :trial_ends_on, :trial_starts_on, presence: true, if: :trial? + validates_comparison_of :trial_ends_on, greater_than: :trial_starts_on, if: :trial? + delegate :name, :title, to: :hosted_plan, prefix: :plan, allow_nil: true delegate :exclude_guests?, to: :namespace diff --git a/ee/app/models/merge_requests/approval_rule.rb b/ee/app/models/merge_requests/approval_rule.rb index 33bddc8e733d04..40cabee995a44f 100644 --- a/ee/app/models/merge_requests/approval_rule.rb +++ b/ee/app/models/merge_requests/approval_rule.rb @@ -4,10 +4,12 @@ module MergeRequests class ApprovalRule < ApplicationRecord self.table_name = 'merge_requests_approval_rules' - alias_attribute :approval_project_rule_id, :source_rule_id - include MergeRequests::ApprovalRuleLike + # todo, confirm that source rule will be used like approval project rule + # for merge request level rules. + alias_attribute :approval_project_rule_id, :source_rule_id + # When this originated_from_group there's only one group has_one :approval_rules_group, inverse_of: :approval_rule has_one :group, through: :approval_rules_group @@ -44,6 +46,7 @@ class ApprovalRule < ApplicationRecord has_many :approval_rules_security_policy_links has_many :security_policies, through: :approval_rules_security_policy_links + # todo add applies to all protected branches to data model. def applies_to_all_protected_branches? applies_to_all_protected_branches end @@ -98,6 +101,7 @@ def self.destroy_everything end end + # todo, add with security policies. def report_type nil end @@ -145,6 +149,7 @@ def scanners nil end + # todo add with security policies. def vulnerabilities_allowed nil end diff --git a/ee/app/models/merge_requests/approval_rule_like.rb b/ee/app/models/merge_requests/approval_rule_like.rb index d9c67edd628803..ff4fde9441920c 100644 --- a/ee/app/models/merge_requests/approval_rule_like.rb +++ b/ee/app/models/merge_requests/approval_rule_like.rb @@ -2,6 +2,8 @@ module MergeRequests module ApprovalRuleLike + # this concern can be removed and its contents can be added directly to MergeRequests::ApprovalRule + extend ActiveSupport::Concern include EachBatch include Importable @@ -45,9 +47,10 @@ module ApprovalRuleLike # any_merge_request: 5 # } - # todo name is not being populated for any approver rule + # todo name is not being populated for any approver rule, figure out why. # validates :name, presence: true validates :approvals_required, numericality: { less_than_or_equal_to: APPROVALS_REQUIRED_MAX, greater_than_or_equal_to: 0 } + # todo is this under security policies? # validates :report_type, presence: true, if: :report_approver? @@ -163,7 +166,7 @@ def relation_exists?(relation, column:, value:) end def with_role_approvers - # todo add back role approvers here + # todo add role_approvers to data model and re add to this method. if users.loaded? && group_users.loaded? users | group_users else diff --git a/ee/app/services/approval_rules/base_service.rb b/ee/app/services/approval_rules/base_service.rb index 2a0ab7b18107c9..4fd29e45e6db5f 100644 --- a/ee/app/services/approval_rules/base_service.rb +++ b/ee/app/services/approval_rules/base_service.rb @@ -17,7 +17,7 @@ def action attr_reader :rule def can_edit? - return true + return true # todo, remove this when policies for approval rules are added. skip_authorization || can?(current_user, policy_action, rule) end diff --git a/ee/app/services/approval_rules/create_service.rb b/ee/app/services/approval_rules/create_service.rb index 20105363c890f3..444785b6276a2e 100644 --- a/ee/app/services/approval_rules/create_service.rb +++ b/ee/app/services/approval_rules/create_service.rb @@ -6,7 +6,6 @@ class CreateService < ::ApprovalRules::BaseService # @param target [Project, MergeRequest] def initialize(target, user, params) - # todo, understand default type being anyapprover here. @rule = target.v2_approval_rules.build(origin: :project, project: target) # todo, applies_to_all_protected_branches is not in model yet. @params = params.except!(:applies_to_all_protected_branches) @@ -66,7 +65,6 @@ def approvers_set? end def approvers_present? - # binding.pry_shell %i[user_ids group_ids users groups usernames].any? { |key| @params[key].present? } end diff --git a/ee/lib/api/project_approval_settings.rb b/ee/lib/api/project_approval_settings.rb index 4ae89089584a3a..4e36bf2fccd2ae 100644 --- a/ee/lib/api/project_approval_settings.rb +++ b/ee/lib/api/project_approval_settings.rb @@ -24,7 +24,7 @@ class ProjectApprovalSettings < ::API::Base end get do authorize_read_project_approval_rule! - # binding.pry_shell + present( user_project, with: EE::API::Entities::ProjectApprovalSettings, diff --git a/ee/lib/ee/api/entities/project_approval_settings.rb b/ee/lib/ee/api/entities/project_approval_settings.rb index d91b85f1cdc544..66a450a9d8a0e4 100644 --- a/ee/lib/ee/api/entities/project_approval_settings.rb +++ b/ee/lib/ee/api/entities/project_approval_settings.rb @@ -8,7 +8,6 @@ module Entities # To be removed in https://gitlab.com/gitlab-org/gitlab/issues/13574. class ProjectApprovalSettings < Grape::Entity expose :rules, using: ProjectApprovalSettingRule do |project, options| - # binding.pry_shell project.visible_approval_rules(target_branch: options[:target_branch]) end diff --git a/ee/spec/factories/merge_requests/approval_group_rules.rb b/ee/spec/factories/merge_requests/approval_group_rules.rb deleted file mode 100644 index 2c02035010d914..00000000000000 --- a/ee/spec/factories/merge_requests/approval_group_rules.rb +++ /dev/null @@ -1,8 +0,0 @@ -# frozen_string_literal: true - -FactoryBot.define do - factory :merge_requests_approval_group_rule, class: 'MergeRequests::ApprovalGroupRule' do - association :approval_rule, factory: :merge_requests_approval_rule - association :group, factory: :group - end -end diff --git a/ee/spec/factories/merge_requests/approval_merge_request_rules.rb b/ee/spec/factories/merge_requests/approval_merge_request_rules.rb deleted file mode 100644 index a2a055cf76ee89..00000000000000 --- a/ee/spec/factories/merge_requests/approval_merge_request_rules.rb +++ /dev/null @@ -1,6 +0,0 @@ -# frozen_string_literal: true - -FactoryBot.define do - factory :merge_requests_approval_merge_request_rule, class: 'MergeRequests::ApprovalMergeRequestRule' do # rubocop:disable Lint/EmptyBlock -- block is required by factorybot - end -end diff --git a/ee/spec/factories/merge_requests/approval_project_rules.rb b/ee/spec/factories/merge_requests/approval_project_rules.rb deleted file mode 100644 index 76e1df667f0907..00000000000000 --- a/ee/spec/factories/merge_requests/approval_project_rules.rb +++ /dev/null @@ -1,6 +0,0 @@ -# frozen_string_literal: true - -FactoryBot.define do - factory :merge_requests_approval_project_rule, class: 'MergeRequests::ApprovalProjectRule' do # rubocop:disable Lint/EmptyBlock -- block is required by factorybot - end -end diff --git a/ee/spec/features/merge_request/user_edits_mr_rules_with_changing_project_settings_spec.rb b/ee/spec/features/merge_request/user_edits_mr_rules_with_changing_project_settings_spec.rb index fb26f528ec0c7c..651fc2fb54faa5 100644 --- a/ee/spec/features/merge_request/user_edits_mr_rules_with_changing_project_settings_spec.rb +++ b/ee/spec/features/merge_request/user_edits_mr_rules_with_changing_project_settings_spec.rb @@ -97,13 +97,7 @@ def update_merge_request_approval_rules(approval_rule_attributes) create_project_rule end - it 'only shows merge request level rules' do - expect(MergeRequest.last.approval_rules.count).to eq(2) - rule_names = rule_names_applicable_to_merge_request(MergeRequest.last) - expect(rule_names).to contain_exactly('All Members', mr_approval_rule_name) - end - - # it_behaves_like 'using only merge request level rules' + it_behaves_like 'using only merge request level rules' context 'and then editing approval rules in merge requests is not allowed' do before do diff --git a/ee/spec/services/approval_rules/create_service_spec.rb b/ee/spec/services/approval_rules/create_service_spec.rb index 3d1ecfaf33bf6e..2d660c59cb96b8 100644 --- a/ee/spec/services/approval_rules/create_service_spec.rb +++ b/ee/spec/services/approval_rules/create_service_spec.rb @@ -324,155 +324,130 @@ context 'when target is project' do let(:target) { project } - let(:new_approvers) { create_list(:user, 2) } - let(:new_groups) { create_list(:group, 2, :private) } + it_behaves_like "creatable" - context 'basic creation action' do - let(:result) do - described_class.new(target, user, { - name: 'security', + it_behaves_like 'onboarding progress action' + + context 'when protected_branch_ids param is present' do + let(:protected_branch) { create(:protected_branch, project: target) } + let(:skip_authorization) { false } + let(:protected_branch_ids) { [protected_branch.id] } + + subject do + described_class.new( + target, + user, + name: 'developers', approvals_required: 1, - user_ids: new_approvers.map(&:id), - group_ids: new_groups.map(&:id) - }).execute + skip_authorization: skip_authorization, + protected_branch_ids: protected_branch_ids + ).execute end - it 'creates approval, excluding non-eligible users and groups' do - expect(result[:status]).to eq(:success) - rule = result[:rule] + context 'and multiple approval rules is enabled' do + before do + stub_licensed_features(multiple_approval_rules: true) + end - expect(rule.name).to eq('security') - expect(rule.approvals_required).to eq(1) - expect(rule.users).to be_empty - expect(rule.groups).to be_empty + it 'associates the approval rule to the protected branch' do + expect(subject[:status]).to eq(:success) + expect(subject[:rule].protected_branches).to eq([protected_branch]) + end + + context 'but user cannot administer project' do + before do + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?).with(user, :admin_project, target).and_return(false) + end + + it 'does not associate the approval rule to the protected branch' do + expect(subject[:status]).to eq(:success) + expect(subject[:rule].protected_branches).to be_empty + end + end + + context 'but skip_authorization param is true' do + let(:skip_authorization) { true } + + before do + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?).with(user, :admin_project, target).and_return(false) + end + + it 'associates the approval rule to the protected branch' do + expect(subject[:status]).to eq(:success) + expect(subject[:rule].protected_branches).to match_array([protected_branch]) + end + end + + context 'but protected branch is for another project' do + let(:another_project) { create(:project) } + let(:protected_branch) { create(:protected_branch, project: another_project) } + + it 'does not associate the approval rule to the protected branch' do + expect(subject[:status]).to eq(:success) + expect(subject[:rule].protected_branches).to be_empty + end + end + + it 'associates the approval rule to the protected branch' do + expect(subject[:status]).to eq(:success) + expect(subject[:rule].protected_branches).to match_array([protected_branch]) + end + + context 'when the root_namespace of project is a group' do + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, group: group) } + let_it_be(:group_protected_branch) { create(:protected_branch, project: nil, group: group) } + let_it_be(:user) { create(:user) } + + let(:protected_branch_ids) { [protected_branch.id, group_protected_branch.id] } + + before do + project.add_maintainer(user) + end + + it 'associates the approval rule to the all protected branches' do + expect(subject[:status]).to eq(:success) + expect(subject[:rule].protected_branches).to match_array([protected_branch, group_protected_branch]) + end + end end + context 'and multiple approval rules is disabled' do + it 'does not associate the approval rule to the protected branch' do + expect(subject[:status]).to eq(:success) + expect(subject[:rule].protected_branches).to be_empty + end + end + end + + context 'with rule_type set to :report_approver' do + let(:result) { subject.execute } + + context 'without report_type' do + subject { described_class.new(target, user, { name: 'License-Check', approvals_required: 1, rule_type: :report_approver }) } + + specify { expect(result[:status]).to eq(:error) } + end + + context 'with report_type set to any of valid value' do + using RSpec::Parameterized::TableSyntax + + subject { described_class.new(target, user, { name: rule_name, approvals_required: 1, rule_type: :report_approver, report_type: report_type }) } + + where(:rule_name, :report_type) do + 'License-Check' | :license_scanning + 'Coverage-Check' | :code_coverage + end + + with_them do + specify { expect(result[:status]).to eq(:success) } + specify { expect(result[:rule].approvals_required).to eq(1) } + specify { expect(result[:rule].rule_type).to eq('report_approver') } + end + end end - # it_behaves_like "creatable" - # - # - # it_behaves_like 'onboarding progress action' - # - # context 'when protected_branch_ids param is present' do - # let(:protected_branch) { create(:protected_branch, project: target) } - # let(:skip_authorization) { false } - # let(:protected_branch_ids) { [protected_branch.id] } - # - # subject do - # described_class.new( - # target, - # user, - # name: 'developers', - # approvals_required: 1, - # skip_authorization: skip_authorization, - # protected_branch_ids: protected_branch_ids - # ).execute - # end - # - # context 'and multiple approval rules is enabled' do - # before do - # stub_licensed_features(multiple_approval_rules: true) - # end - # - # it 'associates the approval rule to the protected branch' do - # expect(subject[:status]).to eq(:success) - # expect(subject[:rule].protected_branches).to eq([protected_branch]) - # end - # - # context 'but user cannot administer project' do - # before do - # allow(Ability).to receive(:allowed?).and_call_original - # allow(Ability).to receive(:allowed?).with(user, :admin_project, target).and_return(false) - # end - # - # it 'does not associate the approval rule to the protected branch' do - # expect(subject[:status]).to eq(:success) - # expect(subject[:rule].protected_branches).to be_empty - # end - # end - # - # context 'but skip_authorization param is true' do - # let(:skip_authorization) { true } - # - # before do - # allow(Ability).to receive(:allowed?).and_call_original - # allow(Ability).to receive(:allowed?).with(user, :admin_project, target).and_return(false) - # end - # - # it 'associates the approval rule to the protected branch' do - # expect(subject[:status]).to eq(:success) - # expect(subject[:rule].protected_branches).to match_array([protected_branch]) - # end - # end - # - # context 'but protected branch is for another project' do - # let(:another_project) { create(:project) } - # let(:protected_branch) { create(:protected_branch, project: another_project) } - # - # it 'does not associate the approval rule to the protected branch' do - # expect(subject[:status]).to eq(:success) - # expect(subject[:rule].protected_branches).to be_empty - # end - # end - # - # it 'associates the approval rule to the protected branch' do - # expect(subject[:status]).to eq(:success) - # expect(subject[:rule].protected_branches).to match_array([protected_branch]) - # end - # - # context 'when the root_namespace of project is a group' do - # let_it_be(:group) { create(:group) } - # let_it_be(:project) { create(:project, group: group) } - # let_it_be(:group_protected_branch) { create(:protected_branch, project: nil, group: group) } - # let_it_be(:user) { create(:user) } - # - # let(:protected_branch_ids) { [protected_branch.id, group_protected_branch.id] } - # - # before do - # project.add_maintainer(user) - # end - # - # it 'associates the approval rule to the all protected branches' do - # expect(subject[:status]).to eq(:success) - # expect(subject[:rule].protected_branches).to match_array([protected_branch, group_protected_branch]) - # end - # end - # end - # - # context 'and multiple approval rules is disabled' do - # it 'does not associate the approval rule to the protected branch' do - # expect(subject[:status]).to eq(:success) - # expect(subject[:rule].protected_branches).to be_empty - # end - # end - # end - # - # context 'with rule_type set to :report_approver' do - # let(:result) { subject.execute } - # - # context 'without report_type' do - # subject { described_class.new(target, user, { name: 'License-Check', approvals_required: 1, rule_type: :report_approver }) } - # - # specify { expect(result[:status]).to eq(:error) } - # end - # - # context 'with report_type set to any of valid value' do - # using RSpec::Parameterized::TableSyntax - # - # subject { described_class.new(target, user, { name: rule_name, approvals_required: 1, rule_type: :report_approver, report_type: report_type }) } - # - # where(:rule_name, :report_type) do - # 'License-Check' | :license_scanning - # 'Coverage-Check' | :code_coverage - # end - # - # with_them do - # specify { expect(result[:status]).to eq(:success) } - # specify { expect(result[:rule].approvals_required).to eq(1) } - # specify { expect(result[:rule].rule_type).to eq('report_approver') } - # end - # end - # end end context 'when target is merge request' do -- GitLab