Merge branch 'bvl-fix-maintainer-push-error' into 'master'

Fix errors on pushing/editing files on empty repositories

Closes #44618 and #42583

See merge request gitlab-org/gitlab-ce!18462

app/models/concerns/protected_ref.rb

@@ -31,7 +31,7 @@ module ProtectedRef
       end
     end
 
-    def protected_ref_accessible_to?(ref, user, action:, protected_refs: nil)
+    def protected_ref_accessible_to?(ref, user, project:, action:, protected_refs: nil)
       access_levels_for_ref(ref, action: action, protected_refs: protected_refs).any? do |access_level|
         access_level.check_access(user)
       end

app/models/project.rb

@@ -1047,13 +1047,6 @@ class Project < ActiveRecord::Base
     "#{web_url}.git"
   end
 
-  def user_can_push_to_empty_repo?(user)
-    return false unless empty_repo?
-    return false unless Ability.allowed?(user, :push_code, self)
-
-    !ProtectedBranch.default_branch_protected? || team.max_member_access(user.id) > Gitlab::Access::DEVELOPER
-  end
-
   def forked?
     return true if fork_network && fork_network.root_project != self
 
@@ -2014,10 +2007,11 @@ class Project < ActiveRecord::Base
 
   def fetch_branch_allows_maintainer_push?(user, branch_name)
     check_access = -> do
+      next false if empty_repo?
+
       merge_request = source_of_merge_requests.opened
                         .where(allow_maintainer_to_push: true)
                         .find_by(source_branch: branch_name)
-
       merge_request&.can_be_merged_by?(user)
     end
 

app/models/protected_branch.rb

@@ -4,6 +4,15 @@ class ProtectedBranch < ActiveRecord::Base
 
   protected_ref_access_levels :merge, :push
 
+  def self.protected_ref_accessible_to?(ref, user, project:, action:, protected_refs: nil)
+    # Masters, owners and admins are allowed to create the default branch
+    if default_branch_protected? && project.empty_repo?
+      return true if user.admin? || project.team.max_member_access(user.id) > Gitlab::Access::DEVELOPER
+    end
+
+    super
+  end
+
   # Check if branch name is marked as protected in the system
   def self.protected?(project, ref_name)
     return true if project.empty_repo? && default_branch_protected?

app/presenters/project_presenter.rb

@@ -4,6 +4,7 @@ class ProjectPresenter < Gitlab::View::Presenter::Delegated
   include GitlabRoutingHelper
   include StorageHelper
   include TreeHelper
+  include ChecksCollaboration
   include Gitlab::Utils::StrongMemoize
 
   presents :project
@@ -170,9 +171,11 @@ class ProjectPresenter < Gitlab::View::Presenter::Delegated
   end
 
   def can_current_user_push_to_branch?(branch)
-    return false unless repository.branch_exists?(branch)
+    user_access(project).can_push_to_branch?(branch)
+  end
 
-    ::Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(branch)
+  def can_current_user_push_to_default_branch?
+    can_current_user_push_to_branch?(default_branch)
   end
 
   def files_anchor_data
@@ -200,7 +203,7 @@ class ProjectPresenter < Gitlab::View::Presenter::Delegated
   end
 
   def new_file_anchor_data
-    if current_user && can_current_user_push_code?
+    if current_user && can_current_user_push_to_default_branch?
       OpenStruct.new(enabled: false,
                      label: _('New file'),
                      link: project_new_blob_path(project, default_branch || 'master'),
@@ -209,7 +212,7 @@ class ProjectPresenter < Gitlab::View::Presenter::Delegated
   end
 
   def readme_anchor_data
-    if current_user && can_current_user_push_code? && repository.readme.blank?
+    if current_user && can_current_user_push_to_default_branch? && repository.readme.blank?
       OpenStruct.new(enabled: false,
                      label: _('Add Readme'),
                      link: add_readme_path)
@@ -221,7 +224,7 @@ class ProjectPresenter < Gitlab::View::Presenter::Delegated
   end
 
   def changelog_anchor_data
-    if current_user && can_current_user_push_code? && repository.changelog.blank?
+    if current_user && can_current_user_push_to_default_branch? && repository.changelog.blank?
       OpenStruct.new(enabled: false,
                      label: _('Add Changelog'),
                      link: add_changelog_path)
@@ -233,7 +236,7 @@ class ProjectPresenter < Gitlab::View::Presenter::Delegated
   end
 
   def license_anchor_data
-    if current_user && can_current_user_push_code? && repository.license_blob.blank?
+    if current_user && can_current_user_push_to_default_branch? && repository.license_blob.blank?
       OpenStruct.new(enabled: false,
                      label: _('Add License'),
                      link: add_license_path)
@@ -245,7 +248,7 @@ class ProjectPresenter < Gitlab::View::Presenter::Delegated
   end
 
   def contribution_guide_anchor_data
-    if current_user && can_current_user_push_code? && repository.contribution_guide.blank?
+    if current_user && can_current_user_push_to_default_branch? && repository.contribution_guide.blank?
       OpenStruct.new(enabled: false,
                      label: _('Add Contribution guide'),
                      link: add_contribution_guide_path)

app/views/projects/empty.html.haml

@@ -58,7 +58,9 @@
               touch README.md
               git add README.md
               git commit -m "add README"
-              git push -u origin master
+            - if @project.can_current_user_push_to_default_branch?
+              %span><
+                git push -u origin master
 
         %fieldset
           %h5 Existing folder
@@ -69,7 +71,9 @@
               git remote add origin #{ content_tag(:span, default_url_to_repo, class: 'clone')}
               git add .
               git commit -m "Initial commit"
-              git push -u origin master
+            - if @project.can_current_user_push_to_default_branch?
+              %span><
+                git push -u origin master
 
         %fieldset
           %h5 Existing Git repository
@@ -78,8 +82,10 @@
               cd existing_repo
               git remote rename origin old-origin
               git remote add origin #{ content_tag(:span, default_url_to_repo, class: 'clone')}
-              git push -u origin --all
-              git push -u origin --tags
+            - if @project.can_current_user_push_to_default_branch?
+              %span><
+                git push -u origin --all
+                git push -u origin --tags
 
           - if can? current_user, :remove_project, @project
             .prepend-top-20

changelogs/unreleased/bvl-fix-maintainer-push-error.yml

+---
+title: Fix errors on pushing to an empty repository
+merge_request: 18462
+author:
+type: fixed

lib/gitlab/user_access.rb

@@ -63,10 +63,12 @@ module Gitlab
 
     request_cache def can_push_to_branch?(ref)
       return false unless can_access_git?
-      return false unless user.can?(:push_code, project) || project.branch_allows_maintainer_push?(user, ref)
+      return false unless project
+
+      return false if !user.can?(:push_code, project) && !project.branch_allows_maintainer_push?(user, ref)
 
       if protected?(ProtectedBranch, project, ref)
-        project.user_can_push_to_empty_repo?(user) || protected_branch_accessible_to?(ref, action: :push)
+        protected_branch_accessible_to?(ref, action: :push)
       else
         true
       end
@@ -101,6 +103,7 @@ module Gitlab
     def protected_branch_accessible_to?(ref, action:)
       ProtectedBranch.protected_ref_accessible_to?(
         ref, user,
+        project: project,
         action: action,
         protected_refs: project.protected_branches)
     end
@@ -108,6 +111,7 @@ module Gitlab
     def protected_tag_accessible_to?(ref, action:)
       ProtectedTag.protected_ref_accessible_to?(
         ref, user,
+        project: project,
         action: action,
         protected_refs: project.protected_tags)
     end

spec/factories/projects.rb

@@ -151,6 +151,13 @@ FactoryBot.define do
       end
     end
 
+    trait :stubbed_repository do
+      after(:build) do |project|
+        allow(project).to receive(:empty_repo?).and_return(false)
+        allow(project.repository).to receive(:empty?).and_return(false)
+      end
+    end
+
     trait :wiki_repo do
       after(:create) do |project|
         raise 'Failed to create wiki repository!' unless project.create_wiki

spec/features/projects/blobs/user_creates_new_blob_in_new_project_spec.rb

+require 'spec_helper'
+
+feature 'User creates blob in new project', :js do
+  let(:user) { create(:user) }
+  let(:project) { create(:project, :empty_repo) }
+
+  shared_examples 'creating a file' do
+    before do
+      sign_in(user)
+      visit project_path(project)
+    end
+
+    it 'allows the user to add a new file' do
+      click_link 'New file'
+
+      find('#editor')
+      execute_script('ace.edit("editor").setValue("Hello world")')
+
+      fill_in(:file_name, with: 'dummy-file')
+
+      click_button('Commit changes')
+
+      expect(page).to have_content('The file has been successfully created')
+    end
+  end
+
+  describe 'as a master' do
+    before do
+      project.add_master(user)
+    end
+
+    it_behaves_like 'creating a file'
+  end
+
+  describe 'as an admin' do
+    let(:user) { create(:user, :admin) }
+
+    it_behaves_like 'creating a file'
+  end
+
+  describe 'as a developer' do
+    before do
+      project.add_developer(user)
+      sign_in(user)
+      visit project_path(project)
+    end
+
+    it 'does not allow pushing to the default branch' do
+      expect(page).not_to have_content('New file')
+    end
+  end
+end

spec/features/projects/user_views_empty_project_spec.rb

+require 'spec_helper'
+
+describe 'User views an empty project' do
+  let(:project) { create(:project, :empty_repo) }
+  let(:user) { create(:user) }
+
+  shared_examples 'allowing push to default branch' do
+    before do
+      sign_in(user)
+      visit project_path(project)
+    end
+
+    it 'shows push-to-master instructions' do
+      expect(page).to have_content('git push -u origin master')
+    end
+  end
+
+  describe 'as a master' do
+    before do
+      project.add_master(user)
+    end
+
+    it_behaves_like 'allowing push to default branch'
+  end
+
+  describe 'as an admin' do
+    let(:user) { create(:user, :admin) }
+
+    it_behaves_like 'allowing push to default branch'
+  end
+
+  describe 'as a developer' do
+    before do
+      project.add_developer(user)
+      sign_in(user)
+      visit project_path(project)
+    end
+
+    it 'does not show push-to-master instructions' do
+      expect(page).not_to have_content('git push -u origin master')
+    end
+  end
+end

spec/lib/gitlab/ci/status/build/play_spec.rb

@@ -2,8 +2,8 @@ require 'spec_helper'
 
 describe Gitlab::Ci::Status::Build::Play do
   let(:user) { create(:user) }
-  let(:project) { build.project }
-  let(:build) { create(:ci_build, :manual) }
+  let(:project) { create(:project, :stubbed_repository) }
+  let(:build) { create(:ci_build, :manual, project: project) }
   let(:status) { Gitlab::Ci::Status::Core.new(build, user) }
 
   subject { described_class.new(status) }
@@ -46,6 +46,8 @@ describe Gitlab::Ci::Status::Build::Play do
       context 'when user can not push to the branch' do
         before do
           build.project.add_developer(user)
+          create(:protected_branch, :masters_can_push,
+                 name: build.ref, project: project)
         end
 
         it { is_expected.not_to have_action }

spec/lib/gitlab/user_access_spec.rb

@@ -32,6 +32,12 @@ describe Gitlab::UserAccess do
       let(:empty_project) { create(:project_empty_repo) }
       let(:project_access) { described_class.new(user, project: empty_project) }
 
+      it 'returns true for admins' do
+        user.update!(admin: true)
+
+        expect(access.can_push_to_branch?('master')).to be_truthy
+      end
+
       it 'returns true if user is master' do
         empty_project.add_master(user)
 
@@ -71,6 +77,12 @@ describe Gitlab::UserAccess do
       let(:branch) { create :protected_branch, project: project, name: "test" }
       let(:not_existing_branch) { create :protected_branch, :developers_can_merge, project: project }
 
+      it 'returns true for admins' do
+        user.update!(admin: true)
+
+        expect(access.can_push_to_branch?(branch.name)).to be_truthy
+      end
+
       it 'returns true if user is a master' do
         project.add_master(user)
 

spec/models/environment_spec.rb

 require 'spec_helper'
 
 describe Environment do
-  let(:project) { create(:project) }
+  let(:project) { create(:project, :stubbed_repository) }
   subject(:environment) { create(:environment, project: project) }
 
   it { is_expected.to belong_to(:project) }
@@ -201,7 +201,7 @@ describe Environment do
   end
 
   describe '#stop_with_action!' do
-    let(:user) { create(:admin) }
+    let(:user) { create(:user) }
 
     subject { environment.stop_with_action!(user) }
 

spec/models/project_spec.rb

@@ -1492,52 +1492,6 @@ describe Project do
     end
   end
 
-  describe '#user_can_push_to_empty_repo?' do
-    let(:project) { create(:project) }
-    let(:user)    { create(:user) }
-
-    it 'returns false when default_branch_protection is in full protection and user is developer' do
-      project.add_developer(user)
-      stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_FULL)
-
-      expect(project.user_can_push_to_empty_repo?(user)).to be_falsey
-    end
-
-    it 'returns false when default_branch_protection only lets devs merge and user is dev' do
-      project.add_developer(user)
-      stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_MERGE)
-
-      expect(project.user_can_push_to_empty_repo?(user)).to be_falsey
-    end
-
-    it 'returns true when default_branch_protection lets devs push and user is developer' do
-      project.add_developer(user)
-      stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_PUSH)
-
-      expect(project.user_can_push_to_empty_repo?(user)).to be_truthy
-    end
-
-    it 'returns true when default_branch_protection is unprotected and user is developer' do
-      project.add_developer(user)
-      stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_NONE)
-
-      expect(project.user_can_push_to_empty_repo?(user)).to be_truthy
-    end
-
-    it 'returns true when user is master' do
-      project.add_master(user)
-
-      expect(project.user_can_push_to_empty_repo?(user)).to be_truthy
-    end
-
-    it 'returns false when the repo is not empty' do
-      project.add_master(user)
-      expect(project).to receive(:empty_repo?).and_return(false)
-
-      expect(project.user_can_push_to_empty_repo?(user)).to be_falsey
-    end
-  end
-
   describe '#container_registry_url' do
     let(:project) { create(:project) }
 

spec/presenters/project_presenter_spec.rb

@@ -208,6 +208,17 @@ describe ProjectPresenter do
       it 'returns nil if user cannot push' do
         expect(presenter.new_file_anchor_data).to be_nil
       end
+
+      context 'when the project is empty' do
+        let(:project) { create(:project, :empty_repo) }
+
+        # Since we protect the default branch for empty repos
+        it 'is empty for a developer' do
+          project.add_developer(user)
+
+          expect(presenter.new_file_anchor_data).to be_nil
+        end
+      end
     end
 
     describe '#readme_anchor_data' do

spec/services/ci/retry_pipeline_service_spec.rb

@@ -235,6 +235,8 @@ describe Ci::RetryPipelineService, '#execute' do
   context 'when user is not allowed to trigger manual action' do
     before do
       project.add_developer(user)
+      create(:protected_branch, :masters_can_push,
+             name: pipeline.ref, project: project)
     end
 
     context 'when there is a failed manual action present' do