summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/models/concerns/protected_ref.rb2
-rw-r--r--app/models/project.rb10
-rw-r--r--app/models/protected_branch.rb9
-rw-r--r--changelogs/unreleased/bvl-fix-maintainer-push-error.yml5
-rw-r--r--lib/gitlab/user_access.rb8
-rw-r--r--spec/factories/projects.rb7
-rw-r--r--spec/features/projects/blobs/user_creates_new_blob_in_new_project_spec.rb52
-rw-r--r--spec/lib/gitlab/ci/status/build/play_spec.rb6
-rw-r--r--spec/lib/gitlab/user_access_spec.rb12
-rw-r--r--spec/models/environment_spec.rb4
-rw-r--r--spec/models/project_spec.rb46
-rw-r--r--spec/services/ci/retry_pipeline_service_spec.rb2
12 files changed, 102 insertions, 61 deletions
diff --git a/app/models/concerns/protected_ref.rb b/app/models/concerns/protected_ref.rb
index 454374121f3..94eef4ff7cd 100644
--- a/app/models/concerns/protected_ref.rb
+++ b/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
diff --git a/app/models/project.rb b/app/models/project.rb
index c293b0b8cf4..645141ddbe5 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1041,13 +1041,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
@@ -2008,10 +2001,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
diff --git a/app/models/protected_branch.rb b/app/models/protected_branch.rb
index 609780c5587..cb361a66591 100644
--- a/app/models/protected_branch.rb
+++ b/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?
diff --git a/changelogs/unreleased/bvl-fix-maintainer-push-error.yml b/changelogs/unreleased/bvl-fix-maintainer-push-error.yml
new file mode 100644
index 00000000000..66ab8fbf884
--- /dev/null
+++ b/changelogs/unreleased/bvl-fix-maintainer-push-error.yml
@@ -0,0 +1,5 @@
+---
+title: Fix errors on pushing to an empty repository
+merge_request: 18462
+author:
+type: fixed
diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb
index 69952cbb47c..8cf5d636743 100644
--- a/lib/gitlab/user_access.rb
+++ b/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
diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb
index 1ae6152a1f0..803498d3b19 100644
--- a/spec/factories/projects.rb
+++ b/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
diff --git a/spec/features/projects/blobs/user_creates_new_blob_in_new_project_spec.rb b/spec/features/projects/blobs/user_creates_new_blob_in_new_project_spec.rb
new file mode 100644
index 00000000000..b7d063596c1
--- /dev/null
+++ b/spec/features/projects/blobs/user_creates_new_blob_in_new_project_spec.rb
@@ -0,0 +1,52 @@
+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
diff --git a/spec/lib/gitlab/ci/status/build/play_spec.rb b/spec/lib/gitlab/ci/status/build/play_spec.rb
index f128c1d4ca4..e2bb378f663 100644
--- a/spec/lib/gitlab/ci/status/build/play_spec.rb
+++ b/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 }
diff --git a/spec/lib/gitlab/user_access_spec.rb b/spec/lib/gitlab/user_access_spec.rb
index 40c8286b1b9..97b6069f64d 100644
--- a/spec/lib/gitlab/user_access_spec.rb
+++ b/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)
diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb
index 56161bfcc28..25d6597084c 100644
--- a/spec/models/environment_spec.rb
+++ b/spec/models/environment_spec.rb
@@ -1,7 +1,7 @@
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) }
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index f00cebba364..acff4427cb7 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -1483,52 +1483,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) }
diff --git a/spec/services/ci/retry_pipeline_service_spec.rb b/spec/services/ci/retry_pipeline_service_spec.rb
index 6ce75c65c8c..f1acfc48468 100644
--- a/spec/services/ci/retry_pipeline_service_spec.rb
+++ b/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
View Lorry Controller Status