Commit a001ed04 authored by Peter Leitzen's avatar Peter Leitzen

Add permission check to the main publish service

We want to make sure to check permission in PublishIncidentService
as well.
parent b7cf43b2
# frozen_string_literal: true # frozen_string_literal: true
module StatusPage module StatusPage
# Delegate work to more specific publishing services. # Publishes content to status page by delegating to specific
# publishing services.
# #
# Use this service for publishing an incident to CDN which calls: # Use this service for publishing an incident to CDN synchronously.
# To publish asynchronously use +StatusPage::TriggerPublishService+ instead.
#
# This services calls:
# * StatusPage::PublishDetailsService # * StatusPage::PublishDetailsService
# * StatusPage::PublishListService # * StatusPage::PublishListService
class PublishIncidentService class PublishIncidentService
include Gitlab::Utils::StrongMemoize include Gitlab::Utils::StrongMemoize
def initialize(project:, issue_id:) def initialize(user:, project:, issue_id:)
@user = user
@project = project @project = project
@issue_id = issue_id @issue_id = issue_id
end end
def execute def execute
return error_permission_denied unless can_publish?
return error_issue_not_found unless issue return error_issue_not_found unless issue
response = publish_details response = publish_details
...@@ -25,7 +31,7 @@ module StatusPage ...@@ -25,7 +31,7 @@ module StatusPage
private private
attr_reader :project, :issue_id attr_reader :user, :project, :issue_id
def publish_details def publish_details
PublishDetailsService.new(project: project).execute(issue, user_notes) PublishDetailsService.new(project: project).execute(issue, user_notes)
...@@ -55,8 +61,20 @@ module StatusPage ...@@ -55,8 +61,20 @@ module StatusPage
end end
end end
def can_publish?
user.can?(:publish_status_page, project)
end
def error_permission_denied
error('No publish permission')
end
def error_issue_not_found def error_issue_not_found
ServiceResponse.error(message: 'Issue not found') error('Issue not found')
end
def error(message)
ServiceResponse.error(message: message)
end end
end end
end end
...@@ -15,7 +15,8 @@ module StatusPage ...@@ -15,7 +15,8 @@ module StatusPage
return unless can_publish? return unless can_publish?
return unless status_page_enabled? return unless status_page_enabled?
StatusPage::PublishIncidentWorker.perform_async(project.id, issue_id) StatusPage::PublishIncidentWorker
.perform_async(user.id, project.id, issue_id)
end end
private private
......
...@@ -11,22 +11,23 @@ module StatusPage ...@@ -11,22 +11,23 @@ module StatusPage
worker_has_external_dependencies! worker_has_external_dependencies!
idempotent! idempotent!
def perform(project_id, issue_id) def perform(user_id, project_id, issue_id)
@user_id = user_id
@project_id = project_id @project_id = project_id
@issue_id = issue_id @issue_id = issue_id
@project = Project.find_by_id(project_id)
return if project.nil? return unless user && project
publish publish
end end
private private
attr_reader :project_id, :issue_id, :project attr_reader :user_id, :project_id, :issue_id
def publish def publish
result = PublishIncidentService result = PublishIncidentService
.new(project: project, issue_id: issue_id) .new(user: user, project: project, issue_id: issue_id)
.execute .execute
log_error(result.message) if result.error? log_error(result.message) if result.error?
...@@ -35,6 +36,14 @@ module StatusPage ...@@ -35,6 +36,14 @@ module StatusPage
raise raise
end end
def user
strong_memoize(:user) { User.find_by_id(user_id) }
end
def project
strong_memoize(:project) { Project.find_by_id(project_id) }
end
def log_error(message) def log_error(message)
preamble = "Failed to publish incident for project_id=#{project_id}, issue_id=#{issue_id}" preamble = "Failed to publish incident for project_id=#{project_id}, issue_id=#{issue_id}"
logger.error("#{preamble}: #{message}") logger.error("#{preamble}: #{message}")
......
...@@ -3,17 +3,24 @@ ...@@ -3,17 +3,24 @@
require 'spec_helper' require 'spec_helper'
describe StatusPage::PublishIncidentService do describe StatusPage::PublishIncidentService do
let_it_be(:user) { create(:user) }
let_it_be(:project, refind: true) { create(:project) } let_it_be(:project, refind: true) { create(:project) }
let_it_be(:issue) { create(:issue, project: project) } let_it_be(:issue) { create(:issue, project: project) }
let_it_be(:settings) { create(:status_page_setting, :enabled, project: project) } let_it_be(:settings) { create(:status_page_setting, :enabled, project: project) }
let(:user_can_publish) { true }
let(:service) { described_class.new(project: project, issue_id: issue.id) } let(:service) do
described_class.new(user: user, project: project, issue_id: issue.id)
end
subject(:result) { service.execute } subject(:result) { service.execute }
describe '#execute' do describe '#execute' do
before do before do
stub_licensed_features(status_page: true) stub_licensed_features(status_page: true)
allow(user).to receive(:can?).with(:publish_status_page, project)
.and_return(user_can_publish)
end end
context 'when publishing succeeds' do context 'when publishing succeeds' do
...@@ -50,6 +57,15 @@ describe StatusPage::PublishIncidentService do ...@@ -50,6 +57,15 @@ describe StatusPage::PublishIncidentService do
expect(result.message).to eq('Issue not found') expect(result.message).to eq('Issue not found')
end end
end end
context 'when user cannot publish' do
let(:user_can_publish) { false }
it 'returns error missing publish permission' do
expect(result).to be_error
expect(result.message).to eq('No publish permission')
end
end
end end
private private
......
...@@ -29,11 +29,13 @@ describe StatusPage::TriggerPublishService do ...@@ -29,11 +29,13 @@ describe StatusPage::TriggerPublishService do
stub_feature_flags(status_page: true) stub_feature_flags(status_page: true)
stub_licensed_features(status_page: true) stub_licensed_features(status_page: true)
allow(worker).to receive(:perform_async).with(project.id, issue.id) allow(worker).to receive(:perform_async)
.with(user.id, project.id, issue.id)
end end
it 'schedules a job' do it 'schedules a job' do
expect(worker).to receive(:perform_async).with(project.id, issue.id) expect(worker).to receive(:perform_async)
.with(user.id, project.id, issue.id)
subject subject
end end
......
...@@ -5,6 +5,7 @@ require 'spec_helper' ...@@ -5,6 +5,7 @@ require 'spec_helper'
describe StatusPage::PublishIncidentWorker do describe StatusPage::PublishIncidentWorker do
include ExclusiveLeaseHelpers include ExclusiveLeaseHelpers
let_it_be(:user) { create(:user) }
let_it_be(:project) { create(:project) } let_it_be(:project) { create(:project) }
let_it_be(:issue) { create(:issue, project: project) } let_it_be(:issue) { create(:issue, project: project) }
...@@ -15,17 +16,17 @@ describe StatusPage::PublishIncidentWorker do ...@@ -15,17 +16,17 @@ describe StatusPage::PublishIncidentWorker do
before do before do
allow(StatusPage::PublishIncidentService) allow(StatusPage::PublishIncidentService)
.to receive(:new).with(project: project, issue_id: issue.id) .to receive(:new).with(user: user, project: project, issue_id: issue.id)
.and_return(service) .and_return(service)
allow(service).to receive(:execute) allow(service).to receive(:execute)
.and_return(service_result) .and_return(service_result)
end end
describe '#perform' do describe '#perform' do
subject { worker.perform(project.id, issue.id) } subject { worker.perform(user.id, project.id, issue.id) }
it_behaves_like 'an idempotent worker' do it_behaves_like 'an idempotent worker' do
let(:job_args) { [project.id, issue.id] } let(:job_args) { [user.id, project.id, issue.id] }
context 'when service succeeds' do context 'when service succeeds' do
it 'execute the service' do it 'execute the service' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment