Automatic merge of gitlab-org/gitlab-ce master

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

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

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

changelogs/unreleased/allow-maintainers-to-remove-pages.yml

+---
+title: Allow maintainers to remove pages
+merge_request:
+author:
+type: fixed

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             |         |            |             | ✓        | ✓      |

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

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 }
-
-      before do
-        project.namespace.update(owner: user)
+    let(:project) { create :project, :repository }
+
+    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
-
-          project.ci_pipelines.create(
-            ref: 'HEAD',
-            sha: commit_sha,
-            source: :push,
-            protected: false
-          )
-        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
+      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
 
-        before do
-          result = Projects::UpdatePagesService.new(project, ci_build).execute
-          expect(result[:status]).to eq(:success)
-          expect(project).to be_pages_deployed
-        end
+      before do
+        result = Projects::UpdatePagesService.new(project, ci_build).execute
+        expect(result[:status]).to eq(:success)
+        expect(project).to be_pages_deployed
+      end
 
-        it 'removes the pages' do
-          visit project_pages_path(project)
+      it 'removes the pages' do
+        visit project_pages_path(project)
 
-          expect(page).to have_link('Remove pages')
+        expect(page).to have_link('Remove pages')
 
-          click_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