Map 'Require pull request before merging' GitHub rule with access_levels

Select Developers + Maintainers in the Allowed to merge list, and select
'No one' in the 'Allowed to push list' in GitLab protected branch
settings when importing a GitHub project to GitLab if the "Require
a pull request before merging" GitHub protected rule is enabled.

Details: https://gitlab.com/gitlab-org/gitlab/-/issues/370951

Changelog: added

app/models/protected_branch/merge_access_level.rb

@@ -2,4 +2,6 @@
 
 class ProtectedBranch::MergeAccessLevel < ApplicationRecord
   include ProtectedBranchAccess
+  # default value for the access_level column
+  GITLAB_DEFAULT_ACCESS_LEVEL = Gitlab::Access::MAINTAINER
 end

app/models/protected_branch/push_access_level.rb

@@ -2,6 +2,8 @@
 
 class ProtectedBranch::PushAccessLevel < ApplicationRecord
   include ProtectedBranchAccess
+  # default value for the access_level column
+  GITLAB_DEFAULT_ACCESS_LEVEL = Gitlab::Access::MAINTAINER
 
   belongs_to :deploy_key
 

doc/user/project/import/github.md

@@ -204,7 +204,7 @@ defaults to the default project visibility.
 Supported GitHub branch protection rules are mapped to GitLab branch protection rules or project-wide GitLab settings when they are imported:
 
 - GitHub rule **Require conversation resolution before merging** for the project's default branch is mapped to the [**All threads must be resolved** GitLab setting](../../discussions/index.md#prevent-merge-unless-all-threads-are-resolved). [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/371110) in GitLab 15.5.
-- Support for GitHub rule **Require a pull request before merging** is proposed in issue [370951](https://gitlab.com/gitlab-org/gitlab/-/issues/370951).
+- GitHub rule **Require a pull request before merging** is mapped to the **No one** option in the **Allowed to push** list of the branch protection rule. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/370951) in GitLab 15.5.
 - GitHub rule **Require signed commits** for the project's default branch is mapped to the **Reject unsigned commits** GitLab setting. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/370949) in GitLab 15.5.
 - Support for GitHub rule **Require status checks to pass before merging** was proposed in issue [370948](https://gitlab.com/gitlab-org/gitlab/-/issues/370948). However, this rule cannot be translated during project import into GitLab due to technical difficulties.
 You can still create [status checks](../merge_requests/status_checks.md) in GitLab yourself.

lib/gitlab/github_import/importer/protected_branch_importer.rb

@@ -6,6 +6,10 @@ module Importer
       class ProtectedBranchImporter
         attr_reader :protected_branch, :project, :client
 
+        # By default on GitHub, both developers and maintainers can merge
+        # a PR into the protected branch
+        GITHUB_DEFAULT_MERGE_ACCESS_LEVEL = Gitlab::Access::DEVELOPER
+
         # protected_branch - An instance of
         #   `Gitlab::GithubImport::Representation::ProtectedBranch`.
         # project - An instance of `Project`
@@ -31,8 +35,8 @@ def execute
         def params
           {
             name: protected_branch.id,
-            push_access_levels_attributes: [{ access_level: Gitlab::Access::MAINTAINER }],
-            merge_access_levels_attributes: [{ access_level: Gitlab::Access::MAINTAINER }],
+            push_access_levels_attributes: [{ access_level: push_access_level }],
+            merge_access_levels_attributes: [{ access_level: merge_access_level }],
             allow_force_push: allow_force_push?
           }
         end
@@ -68,6 +72,71 @@ def update_project_push_rule
           push_rule.update!(reject_unsigned_commits: true)
           project.project_setting.update!(push_rule_id: push_rule.id)
         end
+
+        def push_access_level
+          if protected_branch.required_pull_request_reviews
+            Gitlab::Access::NO_ACCESS
+          else
+            gitlab_access_level_for(:push)
+          end
+        end
+
+        # Gets the strictest merge_access_level between GitHub and GitLab
+        def merge_access_level
+          gitlab_access = gitlab_access_level_for(:merge)
+
+          return gitlab_access if gitlab_access == Gitlab::Access::NO_ACCESS
+
+          [gitlab_access, GITHUB_DEFAULT_MERGE_ACCESS_LEVEL].max
+        end
+
+        # action - :push/:merge
+        def gitlab_access_level_for(action)
+          if default_branch?
+            action == :push ? default_branch_push_access_level : default_branch_merge_access_level
+          elsif protected_on_gitlab?
+            non_default_branch_access_level_for(action)
+          else
+            gitlab_default_access_level_for(action)
+          end
+        end
+
+        def default_branch_push_access_level
+          if default_branch_protection.developer_can_push?
+            Gitlab::Access::DEVELOPER
+          else
+            gitlab_default_access_level_for(:push)
+          end
+        end
+
+        def default_branch_merge_access_level
+          if default_branch_protection.developer_can_merge?
+            Gitlab::Access::DEVELOPER
+          else
+            gitlab_default_access_level_for(:merge)
+          end
+        end
+
+        def default_branch_protection
+          Gitlab::Access::BranchProtection.new(project.namespace.default_branch_protection)
+        end
+
+        def protected_on_gitlab?
+          ProtectedBranch.protected?(project, protected_branch.id)
+        end
+
+        def non_default_branch_access_level_for(action)
+          access_level = ProtectedBranch.access_levels_for_ref(protected_branch.id, action: action)
+            .find(&:role?)&.access_level
+
+          access_level || gitlab_default_access_level_for(action)
+        end
+
+        def gitlab_default_access_level_for(action)
+          return ProtectedBranch::PushAccessLevel::GITLAB_DEFAULT_ACCESS_LEVEL if action == :push
+
+          ProtectedBranch::MergeAccessLevel::GITLAB_DEFAULT_ACCESS_LEVEL
+        end
       end
     end
   end

lib/gitlab/github_import/representation/protected_branch.rb

@@ -9,12 +9,13 @@ class ProtectedBranch
 
         attr_reader :attributes
 
-        expose_attribute :id, :allow_force_pushes, :required_conversation_resolution, :required_signatures
+        expose_attribute :id, :allow_force_pushes, :required_conversation_resolution, :required_signatures,
+                         :required_pull_request_reviews
 
         # Builds a Branch Protection info from a GitHub API response.
         # Resource structure details:
         # https://docs.github.com/en/rest/branches/branch-protection#get-branch-protection
-        # branch_protection - An instance of `Sawyer::Resource` containing the protection details.
+        # branch_protection - An instance of `Hash` containing the protection details.
         def self.from_api_response(branch_protection, _additional_object_data = {})
           branch_name = branch_protection[:url].match(%r{/branches/(\S{1,255})/protection$})[1]
 
@@ -22,7 +23,8 @@ def self.from_api_response(branch_protection, _additional_object_data = {})
             id: branch_name,
             allow_force_pushes: branch_protection.dig(:allow_force_pushes, :enabled),
             required_conversation_resolution: branch_protection.dig(:required_conversation_resolution, :enabled),
-            required_signatures: branch_protection.dig(:required_signatures, :enabled)
+            required_signatures: branch_protection.dig(:required_signatures, :enabled),
+            required_pull_request_reviews: branch_protection[:required_pull_request_reviews].present?
           }
 
           new(hash)

spec/lib/gitlab/github_import/importer/protected_branch_importer_spec.rb

@@ -9,12 +9,17 @@
   let(:allow_force_pushes_on_github) { true }
   let(:required_conversation_resolution) { false }
   let(:required_signatures) { false }
+  let(:required_pull_request_reviews) { false }
+  let(:expected_push_access_level) { Gitlab::Access::MAINTAINER }
+  let(:expected_merge_access_level) { Gitlab::Access::MAINTAINER }
+  let(:expected_allow_force_push) { true }
   let(:github_protected_branch) do
     Gitlab::GithubImport::Representation::ProtectedBranch.new(
       id: branch_name,
       allow_force_pushes: allow_force_pushes_on_github,
       required_conversation_resolution: required_conversation_resolution,
-      required_signatures: required_signatures
+      required_signatures: required_signatures,
+      required_pull_request_reviews: required_pull_request_reviews
     )
   end
 
@@ -28,8 +33,8 @@
       let(:expected_ruleset) do
         {
           name: 'protection',
-          push_access_levels_attributes: [{ access_level: Gitlab::Access::MAINTAINER }],
-          merge_access_levels_attributes: [{ access_level: Gitlab::Access::MAINTAINER }],
+          push_access_levels_attributes: [{ access_level: expected_push_access_level }],
+          merge_access_levels_attributes: [{ access_level: expected_merge_access_level }],
           allow_force_push: expected_allow_force_push
         }
       end
@@ -165,7 +170,7 @@
       end
     end
 
-    context "when branch is not default" do
+    context 'when branch is not default' do
       context 'when required_conversation_resolution rule is enabled' do
         let(:required_conversation_resolution) { true }
 
@@ -190,5 +195,107 @@
         it_behaves_like 'does not change project attributes'
       end
     end
+
+    context 'when required_pull_request_reviews rule is enabled on GitHub' do
+      let(:required_pull_request_reviews) { true }
+      let(:expected_push_access_level) { Gitlab::Access::NO_ACCESS }
+      let(:expected_merge_access_level) { Gitlab::Access::MAINTAINER }
+
+      it_behaves_like 'create branch protection by the strictest ruleset'
+    end
+
+    context 'when required_pull_request_reviews rule is disabled on GitHub' do
+      let(:required_pull_request_reviews) { false }
+
+      context 'when branch is default' do
+        before do
+          allow(project).to receive(:default_branch).and_return(branch_name)
+        end
+
+        context 'when default branch protection = Gitlab::Access::PROTECTION_DEV_CAN_PUSH' do
+          before do
+            stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_PUSH)
+          end
+
+          let(:expected_push_access_level) { Gitlab::Access::DEVELOPER }
+          let(:expected_merge_access_level) { Gitlab::Access::MAINTAINER }
+
+          it_behaves_like 'create branch protection by the strictest ruleset'
+        end
+
+        context 'when default branch protection = Gitlab::Access::PROTECTION_DEV_CAN_MERGE' do
+          before do
+            stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_MERGE)
+          end
+
+          let(:expected_push_access_level) { Gitlab::Access::MAINTAINER }
+          let(:expected_merge_access_level) { Gitlab::Access::DEVELOPER }
+
+          it_behaves_like 'create branch protection by the strictest ruleset'
+        end
+      end
+
+      context 'when branch is protected on GitLab' do
+        let(:protected_branch) do
+          create(
+            :protected_branch,
+            project: project,
+            name: 'protect*',
+            allow_force_push: true
+          )
+        end
+
+        let(:push_access_level) { protected_branch.push_access_levels.first }
+        let(:merge_access_level) { protected_branch.merge_access_levels.first }
+
+        context 'when there is branch protection rule for the role' do
+          context 'when No one can merge' do
+            before do
+              merge_access_level.update_column(:access_level, Gitlab::Access::NO_ACCESS)
+            end
+
+            let(:expected_push_access_level) { push_access_level.access_level }
+            let(:expected_merge_access_level) { Gitlab::Access::NO_ACCESS }
+
+            it_behaves_like 'create branch protection by the strictest ruleset'
+          end
+
+          context 'when Maintainers and Developers can merge' do
+            before do
+              merge_access_level.update_column(:access_level, Gitlab::Access::DEVELOPER)
+            end
+
+            let(:gitlab_push_access_level) { push_access_level.access_level }
+            let(:gitlab_merge_access_level) { merge_access_level.access_level }
+            let(:expected_push_access_level) { gitlab_push_access_level }
+            let(:expected_merge_access_level) { [gitlab_merge_access_level, github_default_merge_access_level].max }
+            let(:github_default_merge_access_level) do
+              Gitlab::GithubImport::Importer::ProtectedBranchImporter::GITHUB_DEFAULT_MERGE_ACCESS_LEVEL
+            end
+
+            it_behaves_like 'create branch protection by the strictest ruleset'
+          end
+        end
+
+        context 'when there is no branch protection rule for the role' do
+          before do
+            push_access_level.update_column(:user_id, project.owner.id)
+            merge_access_level.update_column(:user_id, project.owner.id)
+          end
+
+          let(:expected_push_access_level) { ProtectedBranch::PushAccessLevel::GITLAB_DEFAULT_ACCESS_LEVEL }
+          let(:expected_merge_access_level) { Gitlab::Access::MAINTAINER }
+
+          it_behaves_like 'create branch protection by the strictest ruleset'
+        end
+      end
+
+      context 'when branch is neither default nor protected on GitLab' do
+        let(:expected_push_access_level) { ProtectedBranch::PushAccessLevel::GITLAB_DEFAULT_ACCESS_LEVEL }
+        let(:expected_merge_access_level) { ProtectedBranch::MergeAccessLevel::GITLAB_DEFAULT_ACCESS_LEVEL }
+
+        it_behaves_like 'create branch protection by the strictest ruleset'
+      end
+    end
   end
 end

spec/lib/gitlab/github_import/importer/protected_branches_importer_spec.rb

@@ -23,11 +23,13 @@
   let(:github_protection_rule) do
     response = Struct.new(:name, :url, :required_signatures, :enforce_admins, :required_linear_history,
       :allow_force_pushes, :allow_deletion, :block_creations, :required_conversation_resolution,
+      :required_pull_request_reviews,
       keyword_init: true
     )
     required_signatures = Struct.new(:url, :enabled, keyword_init: true)
     enforce_admins = Struct.new(:url, :enabled, keyword_init: true)
     allow_option = Struct.new(:enabled, keyword_init: true)
+    required_pull_request_reviews = Struct.new(:url, :dismissal_restrictions, keyword_init: true)
     response.new(
       name: 'main',
       url: 'https://example.com/branches/main/protection',
@@ -53,6 +55,10 @@
       ),
       required_conversation_resolution: allow_option.new(
         enabled: false
+      ),
+      required_pull_request_reviews: required_pull_request_reviews.new(
+        url: 'https://example.com/branches/main/protection/required_pull_request_reviews',
+        dismissal_restrictions: {}
       )
     )
   end

spec/lib/gitlab/github_import/representation/protected_branch_spec.rb

@@ -20,6 +20,10 @@
       it 'includes the protected branch required_conversation_resolution attribute' do
         expect(protected_branch.required_conversation_resolution).to eq true
       end
+
+      it 'includes the protected branch required_pull_request_reviews' do
+        expect(protected_branch.required_pull_request_reviews).to eq true
+      end
     end
   end
 
@@ -27,9 +31,11 @@
     let(:response) do
       response = Struct.new(
         :url, :allow_force_pushes, :required_conversation_resolution, :required_signatures,
+        :required_pull_request_reviews,
         keyword_init: true
       )
       enabled_setting = Struct.new(:enabled, keyword_init: true)
+      required_pull_request_reviews = Struct.new(:url, :dismissal_restrictions, keyword_init: true)
       response.new(
         url: 'https://example.com/branches/main/protection',
         allow_force_pushes: enabled_setting.new(
@@ -40,6 +46,10 @@
         ),
         required_signatures: enabled_setting.new(
           enabled: true
+        ),
+        required_pull_request_reviews: required_pull_request_reviews.new(
+          url: 'https://example.com/branches/main/protection/required_pull_request_reviews',
+          dismissal_restrictions: {}
         )
       )
     end
@@ -56,7 +66,8 @@
           'id' => 'main',
           'allow_force_pushes' => true,
           'required_conversation_resolution' => true,
-          'required_signatures' => true
+          'required_signatures' => true,
+          'required_pull_request_reviews' => true
         }
       end