Merge branch '267519-merge-request-approvals-external' into 'master'

Adds ability to externally respond to status checks

See merge request gitlab-org/gitlab!61135

changelogs/unreleased/267519-merge-request-approvals-external.yml

+---
+title: Add external status check responses
+merge_request: 61135
+author:
+type: added

db/migrate/20210323125809_create_status_check_responses_table.rb

+# frozen_string_literal: true
+
+class CreateStatusCheckResponsesTable < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  def up
+    create_table :status_check_responses do |t|
+      t.bigint :merge_request_id, null: false
+      t.bigint :external_approval_rule_id, null: false
+    end
+
+    add_index :status_check_responses, :merge_request_id
+    add_index :status_check_responses, :external_approval_rule_id
+  end
+
+  def down
+    drop_table :status_check_responses
+  end
+end

db/migrate/20210323130831_add_merge_request_foreign_key_to_status_check_responses.rb

+# frozen_string_literal: true
+
+class AddMergeRequestForeignKeyToStatusCheckResponses < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_foreign_key :status_check_responses, :merge_requests, column: :merge_request_id, on_delete: :cascade
+  end
+
+  def down
+    with_lock_retries do
+      remove_foreign_key :status_check_responses, column: :merge_request_id
+    end
+  end
+end

db/migrate/20210323131543_add_external_approval_rule_foreign_key_to_status_check_responses.rb

+# frozen_string_literal: true
+
+class AddExternalApprovalRuleForeignKeyToStatusCheckResponses < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_foreign_key :status_check_responses, :external_approval_rules, column: :external_approval_rule_id, on_delete: :cascade
+  end
+
+  def down
+    with_lock_retries do
+      remove_foreign_key :status_check_responses, column: :external_approval_rule_id
+    end
+  end
+end

db/schema_migrations/20210323125809

+f8c4a3da0931ee04654050e3b172814e7ea1238bac501794e39d0d68592da8fa
\ No newline at end of file

db/schema_migrations/20210323130831

+ca53c3d2bf58aeb803f942ce122a84d7ce587fcceb06c5800c44fd5aac1fd6ac
\ No newline at end of file

db/schema_migrations/20210323131543

+a4cac229cdd249feef18a39e845158952bef2f67fa2784713db47ab9a06495bd
\ No newline at end of file

db/structure.sql

@@ -17973,6 +17973,21 @@ CREATE SEQUENCE sprints_id_seq
 
 ALTER SEQUENCE sprints_id_seq OWNED BY sprints.id;
 
+CREATE TABLE status_check_responses (
+    id bigint NOT NULL,
+    merge_request_id bigint NOT NULL,
+    external_approval_rule_id bigint NOT NULL
+);
+
+CREATE SEQUENCE status_check_responses_id_seq
+    START WITH 1
+    INCREMENT BY 1
+    NO MINVALUE
+    NO MAXVALUE
+    CACHE 1;
+
+ALTER SEQUENCE status_check_responses_id_seq OWNED BY status_check_responses.id;
+
 CREATE TABLE status_page_published_incidents (
     id bigint NOT NULL,
     created_at timestamp with time zone NOT NULL,
@@ -20082,6 +20097,8 @@ ALTER TABLE ONLY spam_logs ALTER COLUMN id SET DEFAULT nextval('spam_logs_id_seq
 
 ALTER TABLE ONLY sprints ALTER COLUMN id SET DEFAULT nextval('sprints_id_seq'::regclass);
 
+ALTER TABLE ONLY status_check_responses ALTER COLUMN id SET DEFAULT nextval('status_check_responses_id_seq'::regclass);
+
 ALTER TABLE ONLY status_page_published_incidents ALTER COLUMN id SET DEFAULT nextval('status_page_published_incidents_id_seq'::regclass);
 
 ALTER TABLE ONLY status_page_settings ALTER COLUMN project_id SET DEFAULT nextval('status_page_settings_project_id_seq'::regclass);
@@ -21695,6 +21712,9 @@ ALTER TABLE ONLY spam_logs
 ALTER TABLE ONLY sprints
     ADD CONSTRAINT sprints_pkey PRIMARY KEY (id);
 
+ALTER TABLE ONLY status_check_responses
+    ADD CONSTRAINT status_check_responses_pkey PRIMARY KEY (id);
+
 ALTER TABLE ONLY status_page_published_incidents
     ADD CONSTRAINT status_page_published_incidents_pkey PRIMARY KEY (id);
 
@@ -24359,6 +24379,10 @@ CREATE INDEX index_sprints_on_title ON sprints USING btree (title);
 
 CREATE INDEX index_sprints_on_title_trigram ON sprints USING gin (title gin_trgm_ops);
 
+CREATE INDEX index_status_check_responses_on_external_approval_rule_id ON status_check_responses USING btree (external_approval_rule_id);
+
+CREATE INDEX index_status_check_responses_on_merge_request_id ON status_check_responses USING btree (merge_request_id);
+
 CREATE UNIQUE INDEX index_status_page_published_incidents_on_issue_id ON status_page_published_incidents USING btree (issue_id);
 
 CREATE INDEX index_status_page_settings_on_project_id ON status_page_settings USING btree (project_id);
@@ -25124,6 +25148,9 @@ ALTER TABLE ONLY ci_unit_test_failures
 ALTER TABLE ONLY project_pages_metadata
     ADD CONSTRAINT fk_0fd5b22688 FOREIGN KEY (pages_deployment_id) REFERENCES pages_deployments(id) ON DELETE SET NULL;
 
+ALTER TABLE ONLY status_check_responses
+    ADD CONSTRAINT fk_116e7e7369 FOREIGN KEY (external_approval_rule_id) REFERENCES external_approval_rules(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY group_deletion_schedules
     ADD CONSTRAINT fk_11e3ebfcdd FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE;
 
@@ -25769,6 +25796,9 @@ ALTER TABLE ONLY boards
 ALTER TABLE ONLY ci_pipeline_variables
     ADD CONSTRAINT fk_f29c5f4380 FOREIGN KEY (pipeline_id) REFERENCES ci_pipelines(id) ON DELETE CASCADE;
 
+ALTER TABLE ONLY status_check_responses
+    ADD CONSTRAINT fk_f3953d86c6 FOREIGN KEY (merge_request_id) REFERENCES merge_requests(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY design_management_designs_versions
     ADD CONSTRAINT fk_f4d25ba00c FOREIGN KEY (version_id) REFERENCES design_management_versions(id) ON DELETE CASCADE;
 

ee/app/models/ee/merge_request.rb

@@ -19,6 +19,7 @@ module EE
       has_many :approvers, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
       has_many :approver_users, through: :approvers, source: :user
       has_many :approver_groups, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
+      has_many :status_check_responses, class_name: 'MergeRequests::StatusCheckResponse', inverse_of: :merge_request
       has_many :approval_rules, class_name: 'ApprovalMergeRequestRule', inverse_of: :merge_request do
         def applicable_to_branch(branch)
           ActiveRecord::Associations::Preloader.new.preload(

ee/app/models/merge_requests/status_check_response.rb

+# frozen_string_literal: true
+
+module MergeRequests
+  class StatusCheckResponse < ApplicationRecord
+    self.table_name = 'status_check_responses'
+
+    belongs_to :merge_request
+    belongs_to :external_approval_rule, class_name: 'ApprovalRules::ExternalApprovalRule'
+
+    validates :merge_request, presence: true
+    validates :external_approval_rule, presence: true
+  end
+end

ee/lib/api/entities/merge_requests/status_check_response.rb

+# frozen_string_literal: true
+
+module API
+  module Entities
+    module MergeRequests
+      class StatusCheckResponse < Grape::Entity
+        expose :id
+        expose :merge_request, using: Entities::MergeRequest
+        expose :external_approval_rule, using: Entities::ExternalApprovalRule
+      end
+    end
+  end
+end

ee/lib/api/status_checks.rb

+# frozen_string_literal: true
+
+module API
+  class StatusChecks < ::API::Base
+    feature_category :compliance_management
+
+    before { authenticate! }
+
+    resource :projects, requirements: ::API::API::NAMESPACE_OR_PROJECT_REQUIREMENTS do
+      segment ':id/merge_requests/:merge_request_iid/status_check_responses' do
+        desc 'Externally approve a merge request' do
+          detail 'This feature was introduced in 13.12 and is gated behind the :ff_compliance_approval_gates feature flag.'
+          success Entities::MergeRequests::StatusCheckResponse
+        end
+        params do
+          requires :id, type: String, desc: 'The ID of a project'
+          requires :merge_request_iid, type: Integer, desc: 'The IID of a merge request'
+          requires :external_approval_rule_id, type: Integer, desc: 'The ID of a merge request rule'
+          requires :sha, type: String, desc: 'The current SHA at HEAD of the merge request.'
+        end
+        post do
+          not_found! unless ::Feature.enabled?(:ff_compliance_approval_gates, user_project)
+
+          merge_request = find_merge_request_with_access(params[:merge_request_iid], :approve_merge_request)
+
+          check_sha_param!(params, merge_request)
+
+          approval = merge_request.status_check_responses.create(external_approval_rule_id: params[:external_approval_rule_id])
+
+          present(approval, with: Entities::MergeRequests::StatusCheckResponse)
+        end
+      end
+    end
+  end
+end

ee/lib/ee/api/api.rb

@@ -13,6 +13,7 @@ module EE
         mount ::API::AuditEvents
         mount ::API::ProjectApprovalRules
         mount ::API::ExternalApprovalRules
+        mount ::API::StatusChecks
         mount ::API::ProjectApprovalSettings
         mount ::API::Dora::Metrics
         mount ::API::EpicIssues

ee/spec/factories/status_check_responses.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :status_check_response, class: 'MergeRequests::StatusCheckResponse' do
+    merge_request
+    external_approval_rule
+  end
+end

ee/spec/models/merge_request_spec.rb

@@ -27,6 +27,7 @@ RSpec.describe MergeRequest do
     it { is_expected.to have_many(:approval_rules) }
     it { is_expected.to have_many(:approval_merge_request_rule_sources).through(:approval_rules) }
     it { is_expected.to have_many(:approval_project_rules).through(:approval_merge_request_rule_sources) }
+    it { is_expected.to have_many(:status_check_responses).class_name('MergeRequests::StatusCheckResponse').inverse_of(:merge_request) }
 
     describe 'approval_rules association' do
       describe '#applicable_to_branch' do

ee/spec/models/merge_requests/status_check_response_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequests::StatusCheckResponse, type: :model do
+  subject { build(:status_check_response) }
+
+  it { is_expected.to belong_to(:merge_request) }
+  it { is_expected.to belong_to(:external_approval_rule).class_name('ApprovalRules::ExternalApprovalRule') }
+
+  it { is_expected.to validate_presence_of(:merge_request) }
+  it { is_expected.to validate_presence_of(:external_approval_rule) }
+end

ee/spec/requests/api/status_checks_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe API::StatusChecks do
+  include AccessMatchersForRequest
+
+  describe 'POST :id/:merge_requests/:merge_request_iid/status_check_responses' do
+    let_it_be(:project) { create(:project, :repository) }
+    let_it_be(:rule) { create(:external_approval_rule, project: project) }
+    let_it_be(:merge_request) { create(:merge_request, source_project: project) }
+    let_it_be(:project_maintainer) { create(:user) }
+
+    let(:sha) { merge_request.source_branch_sha }
+    let(:user) { project_maintainer }
+
+    subject { post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/status_check_responses", user), params: { external_approval_rule_id: rule.id, sha: sha } }
+
+    describe 'permissions' do
+      it { expect { subject }.to be_allowed_for(:maintainer).of(project) }
+      it { expect { subject }.to be_allowed_for(:developer).of(project) }
+      it { expect { subject }.to be_denied_for(:reporter).of(project) }
+    end
+
+    context 'feature flag is disabled' do
+      before do
+        stub_feature_flags(ff_compliance_approval_gates: false)
+      end
+
+      it 'returns a not found error' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+
+    context 'when user has access' do
+      before do
+        project.add_user(project_maintainer, :maintainer)
+      end
+
+      it 'returns a 201' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:created)
+      end
+
+      it 'returns the status checks as JSON' do
+        subject
+
+        expect(json_response.keys).to contain_exactly('id', 'merge_request', 'external_approval_rule')
+      end
+
+      it 'creates new StatusCheckResponse with correct attributes' do
+        expect { subject }.to change { MergeRequests::StatusCheckResponse.count }.by 1
+      end
+
+      context 'when sha is not the source branch HEAD' do
+        let(:sha) { 'notarealsha' }
+
+        it 'does not create a new approval' do
+          expect { subject }.not_to change { MergeRequests::StatusCheckResponse.count }
+        end
+
+        it 'returns a conflict error' do
+          subject
+
+          expect(response).to have_gitlab_http_status(:conflict)
+        end
+      end
+
+      context 'when user is not authenticated' do
+        let(:user) { nil }
+
+        it 'returns an unauthorized status' do
+          subject
+
+          expect(response).to have_gitlab_http_status(:unauthorized)
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/import_export/all_models.yml

@@ -125,6 +125,7 @@ project_members:
 - source
 - project
 merge_requests:
+- status_check_responses
 - subscriptions
 - award_emoji
 - author