Protected branch is now created for default branch on import3968-protected-branch-is-not-set-for-default-branch-on-import

7 files changed, 89 insertions, 20 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index 9c0bbf697e2..ff9418ea70c 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1459,6 +1459,7 @@ class Project < ActiveRecord::Base
     import_finish
     remove_import_jid
     update_project_counter_caches
+    after_create_default_branch
   end
 
   def update_project_counter_caches
@@ -1472,6 +1473,27 @@ class Project < ActiveRecord::Base
     end
   end
 
+  def after_create_default_branch
+    return unless default_branch
+
+    # Ensure HEAD points to the default branch in case it is not master
+    change_head(default_branch)
+
+    if current_application_settings.default_branch_protection != Gitlab::Access::PROTECTION_NONE && !ProtectedBranch.protected?(self, default_branch)
+      params = {
+        name: default_branch,
+        push_access_levels_attributes: [{
+          access_level: current_application_settings.default_branch_protection == Gitlab::Access::PROTECTION_DEV_CAN_PUSH ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER
+        }],
+        merge_access_levels_attributes: [{
+          access_level: current_application_settings.default_branch_protection == Gitlab::Access::PROTECTION_DEV_CAN_MERGE ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER
+        }]
+      }
+
+      ProtectedBranches::CreateService.new(self, creator, params).execute(skip_authorization: true)
+    end
+  end
+
   def remove_import_jid
     return unless import_jid
 
diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb
index bb61136e33b..e6fd193ffb3 100644
--- a/app/services/git_push_service.rb
+++ b/app/services/git_push_service.rb
@@ -154,24 +154,7 @@ class GitPushService < BaseService
     offset = [@push_commits_count - PROCESS_COMMIT_LIMIT, 0].max
     @push_commits = project.repository.commits(params[:newrev], offset: offset, limit: PROCESS_COMMIT_LIMIT)
 
-    # Ensure HEAD points to the default branch in case it is not master
-    project.change_head(branch_name)
-
-    # Set protection on the default branch if configured
-    if current_application_settings.default_branch_protection != PROTECTION_NONE && !ProtectedBranch.protected?(@project, @project.default_branch)
-
-      params = {
-        name: @project.default_branch,
-        push_access_levels_attributes: [{
-          access_level: current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_PUSH ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER
-        }],
-        merge_access_levels_attributes: [{
-          access_level: current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_MERGE ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER
-        }]
-      }
-
-      ProtectedBranches::CreateService.new(@project, current_user, params).execute
-    end
+    @project.after_create_default_branch
   end
 
   def build_push_data
diff --git a/app/services/protected_branches/create_service.rb b/app/services/protected_branches/create_service.rb
index a84e335340d..6212fd69077 100644
--- a/app/services/protected_branches/create_service.rb
+++ b/app/services/protected_branches/create_service.rb
@@ -2,8 +2,8 @@ module ProtectedBranches
   class CreateService < BaseService
     attr_reader :protected_branch
 
-    def execute
-      raise Gitlab::Access::AccessDeniedError unless can?(current_user, :admin_project, project)
+    def execute(skip_authorization: false)
+      raise Gitlab::Access::AccessDeniedError unless skip_authorization || can?(current_user, :admin_project, project)
 
       project.protected_branches.create(params)
     end
diff --git a/changelogs/unreleased/3968-protected-branch-is-not-set-for-default-branch-on-import.yml b/changelogs/unreleased/3968-protected-branch-is-not-set-for-default-branch-on-import.yml
new file mode 100644
index 00000000000..e972ac6d54a
--- /dev/null
+++ b/changelogs/unreleased/3968-protected-branch-is-not-set-for-default-branch-on-import.yml
@@ -0,0 +1,5 @@
+---
+title: Protected branch is now created for default branch on import
+merge_request: 16198
+author:
+type: fixed
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index cea22bbd184..9b6646dcae1 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -3072,9 +3072,51 @@ describe Project do
       expect(project).to receive(:import_finish)
       expect(project).to receive(:update_project_counter_caches)
       expect(project).to receive(:remove_import_jid)
+      expect(project).to receive(:after_create_default_branch)
 
       project.after_import
     end
+
+    context 'branch protection' do
+      let(:project) { create(:project, :repository) }
+
+      it 'does not protect when branch protection is disabled' do
+        stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_NONE)
+
+        project.after_import
+
+        expect(project.protected_branches).to be_empty
+      end
+
+      it "gives developer access to push when branch protection is set to 'developers can push'" do
+        stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_PUSH)
+
+        project.after_import
+
+        expect(project.protected_branches).not_to be_empty
+        expect(project.default_branch).to eq(project.protected_branches.first.name)
+        expect(project.protected_branches.first.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::DEVELOPER])
+      end
+
+      it "gives developer access to merge when branch protection is set to 'developers can merge'" do
+        stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_MERGE)
+
+        project.after_import
+
+        expect(project.protected_branches).not_to be_empty
+        expect(project.default_branch).to eq(project.protected_branches.first.name)
+        expect(project.protected_branches.first.merge_access_levels.map(&:access_level)).to eq([Gitlab::Access::DEVELOPER])
+      end
+
+      it 'protects default branch' do
+        project.after_import
+
+        expect(project.protected_branches).not_to be_empty
+        expect(project.default_branch).to eq(project.protected_branches.first.name)
+        expect(project.protected_branches.first.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::MASTER])
+        expect(project.protected_branches.first.merge_access_levels.map(&:access_level)).to eq([Gitlab::Access::MASTER])
+      end
+    end
   end
 
   describe '#update_project_counter_caches' do
diff --git a/spec/services/protected_branches/create_service_spec.rb b/spec/services/protected_branches/create_service_spec.rb
index 835e83d6dba..53b3e5e365d 100644
--- a/spec/services/protected_branches/create_service_spec.rb
+++ b/spec/services/protected_branches/create_service_spec.rb
@@ -19,5 +19,21 @@ describe ProtectedBranches::CreateService do
       expect(project.protected_branches.last.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::MASTER])
       expect(project.protected_branches.last.merge_access_levels.map(&:access_level)).to eq([Gitlab::Access::MASTER])
     end
+
+    context 'when user does not have permission' do
+      let(:user) { create(:user) }
+
+      before do
+        project.add_developer(user)
+      end
+
+      it 'creates a new protected branch if we skip authorization step' do
+        expect { service.execute(skip_authorization: true) }.to change(ProtectedBranch, :count).by(1)
+      end
+
+      it 'raises Gitlab::Access:AccessDeniedError' do
+        expect { service.execute }.to raise_error(Gitlab::Access::AccessDeniedError)
+      end
+    end
   end
 end
diff --git a/spec/workers/repository_import_worker_spec.rb b/spec/workers/repository_import_worker_spec.rb
index 85ac14eb347..7274a9f00f9 100644
--- a/spec/workers/repository_import_worker_spec.rb
+++ b/spec/workers/repository_import_worker_spec.rb
@@ -32,6 +32,7 @@ describe RepositoryImportWorker do
         expect_any_instance_of(Projects::ImportService).to receive(:execute)
           .and_return({ status: :ok })
 
+        expect_any_instance_of(Project).to receive(:after_import).and_call_original
         expect_any_instance_of(Repository).to receive(:expire_emptiness_caches)
         expect_any_instance_of(Project).to receive(:import_finish)