diff options
author | Douwe Maan <douwe@gitlab.com> | 2018-04-24 14:49:38 +0000 |
---|---|---|
committer | James Lopez <james@jameslopez.es> | 2018-05-02 12:34:06 +0200 |
commit | 03bec777ccd81834042658b47d8ed4c62da1c24a (patch) | |
tree | 4f78e5b448548f96b84f22192406a4ab40a57e0c | |
parent | 2d4a5879e8e151a7093ee91d931015b1385f810a (diff) | |
download | gitlab-ce-03bec777ccd81834042658b47d8ed4c62da1c24a.tar.gz |
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
-rw-r--r-- | app/models/concerns/protected_ref.rb | 2 | ||||
-rw-r--r-- | app/models/project.rb | 10 | ||||
-rw-r--r-- | app/models/protected_branch.rb | 9 | ||||
-rw-r--r-- | app/presenters/project_presenter.rb | 17 | ||||
-rw-r--r-- | app/views/projects/empty.html.haml | 14 | ||||
-rw-r--r-- | changelogs/unreleased/bvl-fix-maintainer-push-error.yml | 5 | ||||
-rw-r--r-- | lib/gitlab/user_access.rb | 8 | ||||
-rw-r--r-- | spec/factories/projects.rb | 7 | ||||
-rw-r--r-- | spec/features/projects/blobs/user_creates_new_blob_in_new_project_spec.rb | 52 | ||||
-rw-r--r-- | spec/features/projects/user_views_empty_project_spec.rb | 43 | ||||
-rw-r--r-- | spec/lib/gitlab/ci/status/build/play_spec.rb | 6 | ||||
-rw-r--r-- | spec/lib/gitlab/user_access_spec.rb | 12 | ||||
-rw-r--r-- | spec/models/environment_spec.rb | 4 | ||||
-rw-r--r-- | spec/models/project_spec.rb | 46 | ||||
-rw-r--r-- | spec/presenters/project_presenter_spec.rb | 11 | ||||
-rw-r--r-- | spec/services/ci/retry_pipeline_service_spec.rb | 2 |
16 files changed, 176 insertions, 72 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 3f805dd1fc9..b64189ce7ee 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 @@ -2000,10 +1993,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/app/presenters/project_presenter.rb b/app/presenters/project_presenter.rb index 484ac64580d..e8fe2bfe292 100644 --- a/app/presenters/project_presenter.rb +++ b/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) diff --git a/app/views/projects/empty.html.haml b/app/views/projects/empty.html.haml index b15fe514a08..182cb01f250 100644 --- a/app/views/projects/empty.html.haml +++ b/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 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 1761b6e2a3b..da7255d8ad9 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/features/projects/user_views_empty_project_spec.rb b/spec/features/projects/user_views_empty_project_spec.rb new file mode 100644 index 00000000000..7b982301ffc --- /dev/null +++ b/spec/features/projects/user_views_empty_project_spec.rb @@ -0,0 +1,43 @@ +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 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 2675c2f52c1..e3053a2b86e 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/presenters/project_presenter_spec.rb b/spec/presenters/project_presenter_spec.rb index 55962f345d4..f6e24d5c96c 100644 --- a/spec/presenters/project_presenter_spec.rb +++ b/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 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 |