diff options
author | Bob Van Landuyt <bob@vanlanduyt.co> | 2018-04-06 13:47:52 +0300 |
---|---|---|
committer | Bob Van Landuyt <bob@vanlanduyt.co> | 2018-04-11 11:51:14 +0300 |
commit | ec43e3644436e0ae90f6fb6a64256c38ceb68cc9 (patch) | |
tree | ae0ad64114b9708cd04c43a3f398e9983aa62119 | |
parent | 267dba0ae56c6236b49907ad77fd572578c7b30e (diff) |
Prevent new merge requests for archived projects
This prevents creating merge requests targeting archived projects.
This could happen when a project was already forked, but then the
source was archived.
-rw-r--r-- | app/controllers/projects/application_controller.rb | 6 | ||||
-rw-r--r-- | app/finders/merge_request_target_project_finder.rb | 1 | ||||
-rw-r--r-- | app/helpers/blob_helper.rb | 4 | ||||
-rw-r--r-- | app/policies/project_policy.rb | 4 | ||||
-rw-r--r-- | app/presenters/merge_request_presenter.rb | 6 | ||||
-rw-r--r-- | app/services/merge_requests/create_service.rb | 2 | ||||
-rw-r--r-- | spec/features/merge_request/user_cherry_picks_spec.rb | 8 | ||||
-rw-r--r-- | spec/features/projects/files/user_edits_files_spec.rb | 27 | ||||
-rw-r--r-- | spec/features/projects/merge_request_button_spec.rb | 12 | ||||
-rw-r--r-- | spec/features/projects/merge_requests/user_reverts_merge_request_spec.rb | 8 | ||||
-rw-r--r-- | spec/finders/merge_request_target_project_finder_spec.rb | 6 | ||||
-rw-r--r-- | spec/policies/project_policy_spec.rb | 17 | ||||
-rw-r--r-- | spec/services/merge_requests/create_service_spec.rb | 24 |
13 files changed, 118 insertions, 7 deletions
diff --git a/app/controllers/projects/application_controller.rb b/app/controllers/projects/application_controller.rb index 6d9b42a2c04..cbb14b55399 100644 --- a/app/controllers/projects/application_controller.rb +++ b/app/controllers/projects/application_controller.rb @@ -34,8 +34,12 @@ class Projects::ApplicationController < ApplicationController def can_collaborate_with_project?(project = nil, ref: nil) project ||= @project + can_create_merge_request = + can?(current_user, :create_merge_request_in_project, project) && + current_user.already_forked?(project) + can?(current_user, :push_code, project) || - (current_user && current_user.already_forked?(project)) || + can_create_merge_request || user_access(project).can_push_to_branch?(ref) end diff --git a/app/finders/merge_request_target_project_finder.rb b/app/finders/merge_request_target_project_finder.rb index f358938344e..188ec447a94 100644 --- a/app/finders/merge_request_target_project_finder.rb +++ b/app/finders/merge_request_target_project_finder.rb @@ -12,6 +12,7 @@ class MergeRequestTargetProjectFinder if @source_project.fork_network @source_project.fork_network.projects .public_or_visible_to_user(current_user) + .non_archived .with_feature_available_for_user(:merge_requests, current_user) else Project.where(id: source_project) diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb index 2b440e4d584..ac7b8d6672e 100644 --- a/app/helpers/blob_helper.rb +++ b/app/helpers/blob_helper.rb @@ -59,7 +59,7 @@ module BlobHelper button_tag label, class: "#{common_classes} disabled has-tooltip", title: "It is not possible to #{action} files that are stored in LFS using the web interface", data: { container: 'body' } elsif can_modify_blob?(blob, project, ref) button_tag label, class: "#{common_classes}", 'data-target' => "#modal-#{modal_type}-blob", 'data-toggle' => 'modal' - elsif can?(current_user, :fork_project, project) + elsif can?(current_user, :create_merge_request_in_project, project) edit_fork_button_tag(common_classes, project, label, edit_modify_file_fork_params(action), action) end end @@ -334,7 +334,7 @@ module BlobHelper # Web IDE (Beta) requires the user to have this feature enabled elsif !current_user || (current_user && can_modify_blob?(blob, project, ref)) edit_link_tag(text, edit_path, common_classes) - elsif current_user && can?(current_user, :fork_project, project) + elsif can?(current_user, :create_merge_request_in_project, project) edit_fork_button_tag(common_classes, project, text, edit_blob_fork_params(edit_path)) end end diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index b4970b605ca..72e30c932a9 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -140,6 +140,7 @@ class ProjectPolicy < BasePolicy rule { can?(:guest_access) }.policy do enable :read_project + enable :create_merge_request_in_project enable :read_board enable :read_list enable :read_wiki @@ -250,6 +251,7 @@ class ProjectPolicy < BasePolicy prevent :request_access prevent :upload_file prevent :resolve_note + prevent :create_merge_request_in_project READONLY_FEATURES_WHEN_ARCHIVED.each do |feature| prevent(*create_update_admin_destroy(feature)) @@ -261,6 +263,7 @@ class ProjectPolicy < BasePolicy end rule { merge_requests_disabled | repository_disabled }.policy do + prevent :create_merge_request_in_project prevent(*create_read_update_admin_destroy(:merge_request)) end @@ -306,6 +309,7 @@ class ProjectPolicy < BasePolicy rule { can?(:public_access) }.policy do enable :read_project + enable :create_merge_request_in_project enable :read_board enable :read_list enable :read_wiki diff --git a/app/presenters/merge_request_presenter.rb b/app/presenters/merge_request_presenter.rb index 9f3f2637183..950d3fde2ea 100644 --- a/app/presenters/merge_request_presenter.rb +++ b/app/presenters/merge_request_presenter.rb @@ -196,8 +196,12 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated end def user_can_collaborate_with_project? + can_create_merge_request = + can?(current_user, :create_merge_request_in_project, project) && + current_user.already_forked?(project) + can?(current_user, :push_code, project) || - (current_user && current_user.already_forked?(project)) || + can_create_merge_request || can_push_to_source_branch? end diff --git a/app/services/merge_requests/create_service.rb b/app/services/merge_requests/create_service.rb index c57a2445341..4c2c8398461 100644 --- a/app/services/merge_requests/create_service.rb +++ b/app/services/merge_requests/create_service.rb @@ -72,7 +72,7 @@ module MergeRequests params.delete(:target_project_id) unless can?(current_user, :read_project, @source_project) && - can?(current_user, :read_project, @project) + can?(current_user, :create_merge_request_in_project, @project) raise Gitlab::Access::AccessDeniedError end diff --git a/spec/features/merge_request/user_cherry_picks_spec.rb b/spec/features/merge_request/user_cherry_picks_spec.rb index 494096b21c0..61d1bdaa95a 100644 --- a/spec/features/merge_request/user_cherry_picks_spec.rb +++ b/spec/features/merge_request/user_cherry_picks_spec.rb @@ -40,6 +40,14 @@ describe 'Merge request > User cherry-picks', :js do expect(page).to have_link 'Cherry-pick' end + + it 'hides the cherry pick button for an archived project' do + project.update!(archived: true) + + visit project_merge_request_path(project, merge_request) + + expect(page).not_to have_link 'Cherry-pick' + end end end end diff --git a/spec/features/projects/files/user_edits_files_spec.rb b/spec/features/projects/files/user_edits_files_spec.rb index 523a9f3f4fe..ee9deb30890 100644 --- a/spec/features/projects/files/user_edits_files_spec.rb +++ b/spec/features/projects/files/user_edits_files_spec.rb @@ -12,6 +12,27 @@ describe 'Projects > Files > User edits files' do sign_in(user) end + shared_examples 'unavailable for an archived project' do + it 'does not show the edit link for an archived project', :js do + project.update!(archived: true) + visit project_tree_path(project, project.repository.root_ref) + + click_link('.gitignore') + + aggregate_failures 'available edit buttons' do + # We're showing a link, if the user can edit directly, this is becomes a + # button when the user can fork the project. + expect(page).not_to have_link('Edit') + expect(page).not_to have_button('Edit') + expect(page).not_to have_link('Web IDE') + expect(page).not_to have_button('Web IDE') + + expect(page).not_to have_button('Replace') + expect(page).not_to have_button('Delete') + end + end + end + context 'when an user has write access' do before do project.add_master(user) @@ -85,6 +106,8 @@ describe 'Projects > Files > User edits files' do expect(page).to have_css('.line_holder.new') end + + it_behaves_like 'unavailable for an archived project' end context 'when an user does not have write access' do @@ -168,6 +191,10 @@ describe 'Projects > Files > User edits files' do expect(page).to have_content("From #{forked_project.full_path}") expect(page).to have_content("into #{project2.full_path}") end + + it_behaves_like 'unavailable for an archived project' do + let(:project) { project2 } + end end end end diff --git a/spec/features/projects/merge_request_button_spec.rb b/spec/features/projects/merge_request_button_spec.rb index 40689964b91..b571d5a0e26 100644 --- a/spec/features/projects/merge_request_button_spec.rb +++ b/spec/features/projects/merge_request_button_spec.rb @@ -45,6 +45,18 @@ feature 'Merge Request button' do end end end + + context 'when the project is archived' do + it 'hides the link' do + project.update!(archived: true) + + visit url + + within("#content-body") do + expect(page).not_to have_link(label) + end + end + end end context 'logged in as non-member' do diff --git a/spec/features/projects/merge_requests/user_reverts_merge_request_spec.rb b/spec/features/projects/merge_requests/user_reverts_merge_request_spec.rb index a41d683dbbb..f3e97bc9eb2 100644 --- a/spec/features/projects/merge_requests/user_reverts_merge_request_spec.rb +++ b/spec/features/projects/merge_requests/user_reverts_merge_request_spec.rb @@ -56,4 +56,12 @@ describe 'User reverts a merge request', :js do expect(page).to have_content('The merge request has been successfully reverted. You can now submit a merge request to get this change into the original branch.') end + + it 'cannot revert a merge requests for an archived project' do + project.update!(archived: true) + + visit(merge_request_path(merge_request)) + + expect(page).not_to have_link('Revert') + end end diff --git a/spec/finders/merge_request_target_project_finder_spec.rb b/spec/finders/merge_request_target_project_finder_spec.rb index c81bfd7932c..f302cf80ce8 100644 --- a/spec/finders/merge_request_target_project_finder_spec.rb +++ b/spec/finders/merge_request_target_project_finder_spec.rb @@ -19,6 +19,12 @@ describe MergeRequestTargetProjectFinder do expect(finder.execute).to contain_exactly(forked_project) end + + it 'does not contain archived projects' do + base_project.update!(archived: true) + + expect(finder.execute).to contain_exactly(other_fork, forked_project) + end end context 'public projects' do diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb index f20e4a021da..4c01c320841 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -14,7 +14,7 @@ describe ProjectPolicy do read_project read_board read_list read_wiki read_issue read_project_for_iids read_issue_iid read_merge_request_iid read_label read_milestone read_project_snippet read_project_member read_note - create_project create_issue create_note upload_file + create_project create_issue create_note upload_file create_merge_request_in_project ] end @@ -136,6 +136,20 @@ describe ProjectPolicy do end end + context 'merge requests feature' do + subject { described_class.new(owner, project) } + + it 'disallows all permissions when the feature is disabled' do + project.project_feature.update(merge_requests_access_level: ProjectFeature::DISABLED) + + mr_permissions = [:create_merge_request, :read_merge_request, + :update_merge_request, :admin_merge_request, + :create_merge_request_in_project] + + expect_disallowed(*mr_permissions) + end + end + shared_examples 'archived project policies' do let(:feature_write_abilities) do described_class::READONLY_FEATURES_WHEN_ARCHIVED.flat_map do |feature| @@ -145,6 +159,7 @@ describe ProjectPolicy do let(:other_write_abilities) do %i[ + create_merge_request_in_project push_to_delete_protected_branch push_code request_access diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index 44a83c436cb..736a50b2c15 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -1,6 +1,8 @@ require 'spec_helper' describe MergeRequests::CreateService do + include ProjectForksHelper + let(:project) { create(:project, :repository) } let(:user) { create(:user) } let(:assignee) { create(:user) } @@ -300,7 +302,7 @@ describe MergeRequests::CreateService do end context 'when source and target projects are different' do - let(:target_project) { create(:project) } + let(:target_project) { fork_project(project, nil, repository: true) } let(:opts) do { @@ -334,6 +336,26 @@ describe MergeRequests::CreateService do .to raise_error Gitlab::Access::AccessDeniedError end end + + context 'when the user has access to both projects' do + before do + target_project.add_developer(user) + project.add_developer(user) + end + + it 'creates the merge request' do + merge_request = described_class.new(project, user, opts).execute + + expect(merge_request).to be_persisted + end + + it 'does not create the merge request when the target project is archived' do + target_project.update!(archived: true) + + expect { described_class.new(project, user, opts).execute } + .to raise_error Gitlab::Access::AccessDeniedError + end + end end context 'when user sets source project id' do |