diff --git a/app/services/pages/delete_service.rb b/app/services/pages/delete_service.rb
index 96b451aeba42ea09251e3c7a4f801d4cfc666896..ba0889d1a902f56377979c92b32e3a84eb4b5a7d 100644
--- a/app/services/pages/delete_service.rb
+++ b/app/services/pages/delete_service.rb
@@ -3,6 +3,11 @@
module Pages
class DeleteService < BaseService
def execute
+ if current_user.is_a?(User) && !can_remove_pages?
+ return ServiceResponse.error(message: _('The current user is not authorized to remove the Pages deployment'),
+ reason: :forbidden)
+ end
+
PagesDeployment.deactivate_all(project)
# project.pages_domains.delete_all will just nullify project_id:
@@ -27,5 +32,9 @@ def publish_deleted_event
Gitlab::EventStore.publish(event)
end
+
+ def can_remove_pages?
+ can?(current_user, :remove_pages, project)
+ end
end
end
diff --git a/app/services/pages/update_service.rb b/app/services/pages/update_service.rb
index 01113da037b27d015af54a5579a18e2dc7a07871..bfe5d11b41f71b7975e2459f73e7c16a5fdab9ad 100644
--- a/app/services/pages/update_service.rb
+++ b/app/services/pages/update_service.rb
@@ -34,7 +34,7 @@ def update_pages_https_only!
end
def can_update_page_settings?
- current_user&.can_read_all_resources? && can?(current_user, :update_pages, project)
+ can?(current_user, :update_pages, project)
end
end
end
diff --git a/app/workers/pages/delete_pages_deployment_worker.rb b/app/workers/pages/delete_pages_deployment_worker.rb
index 2737be7d7bac45c52e577bd8c831d4c878aee920..ff516edf8f1269f831aef7a78d3caa89907f3f2f 100644
--- a/app/workers/pages/delete_pages_deployment_worker.rb
+++ b/app/workers/pages/delete_pages_deployment_worker.rb
@@ -12,7 +12,10 @@ def handle_event(event)
project = Project.find_by_id(event.data['project_id'])
return unless project
- ::Pages::DeleteService.new(project).execute
+ user = project.owner
+ return unless user
+
+ ::Pages::DeleteService.new(project, user).execute
end
end
end
diff --git a/doc/api/pages.md b/doc/api/pages.md
index 7dbdabade203dbdf9b5b6921ef740face4e52e3d..6a8c062fc623fa4d26c2dfb37b3829460eb58d1e 100644
--- a/doc/api/pages.md
+++ b/doc/api/pages.md
@@ -15,9 +15,11 @@ The GitLab Pages feature must be enabled to use these endpoints. Find out more a
## Unpublish Pages
+> - [Changed](https://gitlab.com/gitlab-org/gitlab/-/issues/498658) the minimum required role from administrator access to the Maintainer role in GitLab 17.7
+
Prerequisites:
-- You must have administrator access to the instance.
+- You must have at least the Maintainer role for the project.
Remove Pages.
@@ -105,10 +107,11 @@ Example response:
## Update Pages settings for a project
> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/147227) in GitLab 17.0.
+> - [Changed](https://gitlab.com/gitlab-org/gitlab/-/issues/498658) the minimum required role from administrator access to the Maintainer role in GitLab 17.7
Prerequisites:
-- You must have administrator access to the instance.
+- You must have at least the Maintainer role for the project.
Update Pages settings for the project.
diff --git a/lib/api/pages.rb b/lib/api/pages.rb
index 8f3b944b60bf2325bcc86fb2ff6cadb329bf758b..aa3d8ac410061ec6d049a414a41dfb3b6e235b75 100644
--- a/lib/api/pages.rb
+++ b/lib/api/pages.rb
@@ -23,7 +23,6 @@ class Pages < ::API::Base
tags %w[pages]
end
delete ':id/pages' do
- authenticated_with_can_read_all_resources!
authorize! :remove_pages, user_project
::Pages::DeleteService.new(user_project, current_user).execute
@@ -46,7 +45,6 @@ class Pages < ::API::Base
optional :pages_primary_domain, type: String, desc: 'Set pages primary domain'
end
patch ':id/pages' do
- authenticated_with_can_read_all_resources!
authorize! :update_pages, user_project
break not_found! unless user_project.pages_enabled?
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 767d31d47272e000ae638ef674cf46b048f951c8..888a21f00e092991c4b555d0259c7f4509221dd4 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -57323,6 +57323,9 @@ msgstr ""
msgid "The current user is not authorized to manage the pipeline trigger token"
msgstr ""
+msgid "The current user is not authorized to remove the Pages deployment"
+msgstr ""
+
msgid "The current user is not authorized to set pipeline schedule variables"
msgstr ""
diff --git a/spec/requests/api/pages/pages_spec.rb b/spec/requests/api/pages/pages_spec.rb
index e701238701ddabcc8fbd1577d7293abc3a26fd6e..b3755da09e61cd3d5f05f77ad411e1996cd6388c 100644
--- a/spec/requests/api/pages/pages_spec.rb
+++ b/spec/requests/api/pages/pages_spec.rb
@@ -15,7 +15,8 @@
stub_pages_setting(enabled: true)
end
- let(:succes_status_code) { :no_content }
+ let(:success_status_code) { :no_content }
+ let(:failed_status_code) { :not_found }
end
context 'when Pages is disabled' do
diff --git a/spec/requests/api/pages_spec.rb b/spec/requests/api/pages_spec.rb
index 5c7c997709b9e8d8567ccdada83cb16d6ae12633..3226663160e4496cebcc2e2db1fc666eb32ae700 100644
--- a/spec/requests/api/pages_spec.rb
+++ b/spec/requests/api/pages_spec.rb
@@ -100,7 +100,8 @@
context 'when the user is authorized' do
context 'and the update succeeds' do
it 'updates the pages settings and returns 200' do
- patch api(path, admin, admin_mode: true), params: params
+ project.add_maintainer(user)
+ patch api(path, user), params: params
expect(response).to have_gitlab_http_status(:ok)
expect(json_response['force_https']).to eq(true)
@@ -136,7 +137,8 @@
end
it 'returns 403 forbidden with the passed along error message' do
- patch api(path, admin, admin_mode: true), params: params
+ project.add_maintainer(user)
+ patch api(path, user), params: params
expect(response).to have_gitlab_http_status(:forbidden)
expect(response.body).to include(service_error_message)
@@ -146,6 +148,7 @@
context 'when the user is unauthorized' do
it 'returns a 403 forbidden' do
+ project.add_developer(user)
patch api(path, user), params: params
expect(response).to have_gitlab_http_status(:forbidden)
@@ -158,7 +161,7 @@
end
it 'returns a 404 not found' do
- patch api(path, admin, admin_mode: true), params: params
+ patch api(path, user), params: params
expect(response).to have_gitlab_http_status(:not_found)
end
diff --git a/spec/services/pages/delete_service_spec.rb b/spec/services/pages/delete_service_spec.rb
index 86b1bd5bde2012d50b2ba67fcce549ee61d7c9d8..5f045066e121373b24deea146eff1e10478745c9 100644
--- a/spec/services/pages/delete_service_spec.rb
+++ b/spec/services/pages/delete_service_spec.rb
@@ -3,50 +3,69 @@
require 'spec_helper'
RSpec.describe Pages::DeleteService, feature_category: :pages do
- let_it_be(:admin) { create(:admin) }
-
+ let_it_be(:user) { create(:user) }
let(:project) { create(:project, path: "my.project") }
- let(:service) { described_class.new(project, admin) }
-
- it 'marks pages as not deployed' do
- create(:pages_deployment, project: project)
+ let(:service) { described_class.new(project, user) }
- expect { service.execute }
- .to change { project.reload.pages_deployed? }
- .from(true).to(false)
+ before do
+ project.add_maintainer(user)
end
- it 'deletes all domains' do
- domain = create(:pages_domain, project: project)
- unrelated_domain = create(:pages_domain)
+ describe '#execute' do
+ context 'when user has permission' do
+ it 'marks pages as not deployed' do
+ create(:pages_deployment, project: project)
- service.execute
+ expect { service.execute }.to change { project.pages_deployed? }.from(true).to(false)
+ end
- expect(PagesDomain.find_by_id(domain.id)).to eq(nil)
- expect(PagesDomain.find_by_id(unrelated_domain.id)).to be_present
- end
+ it 'deletes domains' do
+ domain = create(:pages_domain, project: project)
+ other_domain = create(:pages_domain)
- it 'schedules a destruction of pages deployments' do
- expect(DestroyPagesDeploymentsWorker)
- .to(receive(:perform_async).with(project.id))
+ expect { service.execute }.to change { PagesDomain.count }.by(-1)
+ expect { domain.reload }.to raise_error(ActiveRecord::RecordNotFound)
+ expect { other_domain.reload }.not_to raise_error
+ end
- service.execute
- end
+ it 'schedules destruction of pages deployments' do
+ expect(DestroyPagesDeploymentsWorker).to receive(:perform_async).with(project.id)
- it 'removes pages deployments', :sidekiq_inline do
- create(:pages_deployment, project: project)
+ service.execute
+ end
- expect { service.execute }
- .to change { PagesDeployment.count }.by(-1)
- end
+ it 'removes pages deployments', :sidekiq_inline do
+ create(:pages_deployment, project: project)
+
+ expect { service.execute }.to change { PagesDeployment.count }.by(-1)
+ end
+
+ it 'publishes a ProjectDeleted event with project id and namespace id' do
+ expected_data = {
+ project_id: project.id,
+ namespace_id: project.namespace_id,
+ root_namespace_id: project.root_namespace.id
+ }
+
+ expect { service.execute }.to publish_event(Pages::PageDeletedEvent).with(expected_data)
+ end
+ end
+
+ context 'when user does not have permission' do
+ before do
+ project.add_developer(user)
+ end
+
+ it 'does not delete pages' do
+ expect { service.execute }.not_to change { project.pages_deployed? }
+ end
- it 'publishes a ProjectDeleted event with project id and namespace id' do
- expected_data = {
- project_id: project.id,
- namespace_id: project.namespace_id,
- root_namespace_id: project.root_namespace.id
- }
+ it 'returns an error message' do
+ result = service.execute
- expect { service.execute }.to publish_event(Pages::PageDeletedEvent).with(expected_data)
+ expect(result).to be_error
+ expect(result.message).to eq('The current user is not authorized to remove the Pages deployment')
+ end
+ end
end
end
diff --git a/spec/services/pages/update_service_spec.rb b/spec/services/pages/update_service_spec.rb
index c2a83c82dce16814d8a2e3bca72eaa28577d76e5..8de12184041f022f4cfe6b81ee98f3d32a68ca73 100644
--- a/spec/services/pages/update_service_spec.rb
+++ b/spec/services/pages/update_service_spec.rb
@@ -3,7 +3,6 @@
require 'spec_helper'
RSpec.describe Pages::UpdateService, feature_category: :pages do
- let_it_be(:admin) { create(:admin) }
let_it_be(:user) { create(:user) }
let_it_be_with_reload(:project) { create(:project) }
let(:domain) { 'my.domain.com' }
@@ -22,11 +21,10 @@
describe '#execute' do
context 'with sufficient permissions' do
- let(:service) { described_class.new(project, admin, params) }
+ let(:service) { described_class.new(project, user, params) }
before do
- allow(admin).to receive(:can_read_all_resources?).and_return(true)
- allow(service).to receive(:can?).with(admin, :update_pages, project).and_return(true)
+ allow(service).to receive(:can?).with(user, :update_pages, project).and_return(true)
end
context 'when updating page setting succeeds' do