diff --git a/app/controllers/projects/pages_controller.rb b/app/controllers/projects/pages_controller.rb index d0e35bee986..73e629ab7c3 100644 --- a/app/controllers/projects/pages_controller.rb +++ b/app/controllers/projects/pages_controller.rb @@ -5,7 +5,8 @@ class Projects::PagesController < Projects::ApplicationController before_action :require_pages_enabled! before_action :authorize_read_pages!, only: [:show] - before_action :authorize_update_pages!, except: [:show] + before_action :authorize_update_pages!, except: [:show, :destroy] + before_action :authorize_remove_pages!, only: [:destroy] # rubocop: disable CodeReuse/ActiveRecord def show diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index cadbc5ae009..95dd8b2795e 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -152,7 +152,6 @@ class ProjectPolicy < BasePolicy enable :remove_fork_project enable :destroy_merge_request enable :destroy_issue - enable :remove_pages enable :set_issue_iid enable :set_issue_created_at @@ -271,6 +270,7 @@ class ProjectPolicy < BasePolicy enable :admin_pages enable :read_pages enable :update_pages + enable :remove_pages enable :read_cluster enable :add_cluster enable :create_cluster diff --git a/app/views/projects/pages/_destroy.haml b/app/views/projects/pages/_destroy.haml index ae8c801b705..138e2864bad 100644 --- a/app/views/projects/pages/_destroy.haml +++ b/app/views/projects/pages/_destroy.haml @@ -9,4 +9,4 @@ .form-actions = link_to 'Remove pages', project_pages_path(@project), data: { confirm: 'Are you sure?'}, method: :delete, class: "btn btn-remove" - else - .nothing-here-block Only the project owner can remove pages + .nothing-here-block Only project maintainers can remove pages diff --git a/changelogs/unreleased/allow-maintainers-to-remove-pages.yml b/changelogs/unreleased/allow-maintainers-to-remove-pages.yml new file mode 100644 index 00000000000..6e344dbe0e9 --- /dev/null +++ b/changelogs/unreleased/allow-maintainers-to-remove-pages.yml @@ -0,0 +1,5 @@ +--- +title: Allow maintainers to remove pages +merge_request: +author: +type: fixed diff --git a/doc/user/permissions.md b/doc/user/permissions.md index 74a966f3a17..dff77acd71b 100644 --- a/doc/user/permissions.md +++ b/doc/user/permissions.md @@ -97,7 +97,7 @@ The following table depicts the various user permission levels in a project. | Manage variables | | | | ✓ | ✓ | | Manage GitLab Pages | | | | ✓ | ✓ | | Manage GitLab Pages domains and certificates | | | | ✓ | ✓ | -| Remove GitLab Pages | | | | | ✓ | +| Remove GitLab Pages | | | | ✓ | ✓ | | View GitLab Pages protected by [access control](project/pages/introduction.md#gitlab-pages-access-control-core-only) | ✓ | ✓ | ✓ | ✓ | ✓ | | Manage clusters | | | | ✓ | ✓ | | Manage license policy **[ULTIMATE]** | | | | ✓ | ✓ | @@ -107,7 +107,6 @@ The following table depicts the various user permission levels in a project. | Transfer project to another namespace | | | | | ✓ | | Remove project | | | | | ✓ | | Delete issues | | | | | ✓ | -| Remove pages | | | | | ✓ | | Force push to protected branches [^4] | | | | | | | Remove protected branches [^4] | | | | | | | View project Audit Events | | | | ✓ | ✓ | diff --git a/spec/controllers/projects/pages_controller_spec.rb b/spec/controllers/projects/pages_controller_spec.rb index 4b742a5d427..d6eece47804 100644 --- a/spec/controllers/projects/pages_controller_spec.rb +++ b/spec/controllers/projects/pages_controller_spec.rb @@ -42,6 +42,18 @@ describe Projects::PagesController do expect(response).to have_gitlab_http_status(302) end + + context 'when user is developer' do + before do + project.add_developer(user) + end + + it 'returns 404 status' do + delete :destroy, params: request_params + + expect(response).to have_gitlab_http_status(404) + end + end end context 'pages disabled' do diff --git a/spec/features/projects/pages_spec.rb b/spec/features/projects/pages_spec.rb index 435fb229b69..72faeba92ee 100644 --- a/spec/features/projects/pages_spec.rb +++ b/spec/features/projects/pages_spec.rb @@ -13,16 +13,6 @@ describe 'Pages' do sign_in(user) end - shared_examples 'no pages deployed' do - it 'does not see anything to destroy' do - visit project_pages_path(project) - - expect(page).to have_content('Configure pages') - expect(page).not_to have_link('Remove pages') - expect(page).not_to have_text('Only the project owner can remove pages') - end - end - context 'when user is the owner' do before do project.namespace.update(owner: user) @@ -181,7 +171,12 @@ describe 'Pages' do end end - it_behaves_like 'no pages deployed' + it 'does not see anything to destroy' do + visit project_pages_path(project) + + expect(page).to have_content('Configure pages') + expect(page).not_to have_link('Remove pages') + end describe 'project settings page' do it 'renders "Pages" tab' do @@ -208,22 +203,6 @@ describe 'Pages' do end end - context 'when the user is not the owner' do - context 'when pages deployed' do - before do - allow_any_instance_of(Project).to receive(:pages_deployed?) { true } - end - - it 'sees "Only the project owner can remove pages" text' do - visit project_pages_path(project) - - expect(page).to have_text('Only the project owner can remove pages') - end - end - - it_behaves_like 'no pages deployed' - end - describe 'HTTPS settings', :js, :https_pages_enabled do before do project.namespace.update(owner: user) @@ -289,51 +268,45 @@ describe 'Pages' do end describe 'Remove page' do - context 'when user is the owner' do - let(:project) { create :project, :repository } + let(:project) { create :project, :repository } - before do - project.namespace.update(owner: user) + context 'when pages are deployed' do + let(:pipeline) do + commit_sha = project.commit('HEAD').sha + + project.ci_pipelines.create( + ref: 'HEAD', + sha: commit_sha, + source: :push, + protected: false + ) end - context 'when pages are deployed' do - let(:pipeline) do - commit_sha = project.commit('HEAD').sha + let(:ci_build) do + create( + :ci_build, + project: project, + pipeline: pipeline, + ref: 'HEAD', + legacy_artifacts_file: fixture_file_upload(File.join('spec/fixtures/pages.zip')), + legacy_artifacts_metadata: fixture_file_upload(File.join('spec/fixtures/pages.zip.meta')) + ) + end - project.ci_pipelines.create( - ref: 'HEAD', - sha: commit_sha, - source: :push, - protected: false - ) - end + before do + result = Projects::UpdatePagesService.new(project, ci_build).execute + expect(result[:status]).to eq(:success) + expect(project).to be_pages_deployed + end - let(:ci_build) do - create( - :ci_build, - project: project, - pipeline: pipeline, - ref: 'HEAD', - legacy_artifacts_file: fixture_file_upload(File.join('spec/fixtures/pages.zip')), - legacy_artifacts_metadata: fixture_file_upload(File.join('spec/fixtures/pages.zip.meta')) - ) - end + it 'removes the pages' do + visit project_pages_path(project) - before do - result = Projects::UpdatePagesService.new(project, ci_build).execute - expect(result[:status]).to eq(:success) - expect(project).to be_pages_deployed - end + expect(page).to have_link('Remove pages') - it 'removes the pages' do - visit project_pages_path(project) + click_link 'Remove pages' - expect(page).to have_link('Remove pages') - - click_link 'Remove pages' - - expect(project.pages_deployed?).to be_falsey - end + expect(project.pages_deployed?).to be_falsey end end end