Commit bb7e95be authored by Peter Leitzen's avatar Peter Leitzen

Check user permissions when updating alert status

It's good practice to check user permissions in the service despite
this check has been performed somewhere else (here GraphQL mutation).
parent ffa0944d
...@@ -11,7 +11,6 @@ module Mutations ...@@ -11,7 +11,6 @@ module Mutations
def resolve(args) def resolve(args)
alert = authorized_find!(project_path: args[:project_path], iid: args[:iid]) alert = authorized_find!(project_path: args[:project_path], iid: args[:iid])
result = update_status(alert, args[:status]) result = update_status(alert, args[:status])
prepare_response(result) prepare_response(result)
...@@ -20,7 +19,9 @@ module Mutations ...@@ -20,7 +19,9 @@ module Mutations
private private
def update_status(alert, status) def update_status(alert, status)
::AlertManagement::UpdateAlertStatusService.new(alert, status).execute ::AlertManagement::UpdateAlertStatusService
.new(alert, current_user, status)
.execute
end end
def prepare_response(result) def prepare_response(result)
......
...@@ -5,14 +5,17 @@ module AlertManagement ...@@ -5,14 +5,17 @@ module AlertManagement
include Gitlab::Utils::StrongMemoize include Gitlab::Utils::StrongMemoize
# @param alert [AlertManagement::Alert] # @param alert [AlertManagement::Alert]
# @param user [User]
# @param status [Integer] Must match a value from AlertManagement::Alert::STATUSES # @param status [Integer] Must match a value from AlertManagement::Alert::STATUSES
def initialize(alert, status) def initialize(alert, user, status)
@alert = alert @alert = alert
@user = user
@status = status @status = status
end end
def execute def execute
return error('Invalid status') unless status_key return error_no_permissions unless allowed?
return error_invalid_status unless status_key
if alert.update(status_event: status_event) if alert.update(status_event: status_event)
success success
...@@ -23,7 +26,13 @@ module AlertManagement ...@@ -23,7 +26,13 @@ module AlertManagement
private private
attr_reader :alert, :status attr_reader :alert, :user, :status
delegate :project, to: :alert
def allowed?
user.can?(:update_alert_management_alert, project)
end
def status_key def status_key
strong_memoize(:status_key) do strong_memoize(:status_key) do
...@@ -39,6 +48,14 @@ module AlertManagement ...@@ -39,6 +48,14 @@ module AlertManagement
ServiceResponse.success(payload: { alert: alert }) ServiceResponse.success(payload: { alert: alert })
end end
def error_no_permissions
error(_('You have no permissions'))
end
def error_invalid_status
error(_('Invalid status'))
end
def error(message) def error(message)
ServiceResponse.error(payload: { alert: alert }, message: message) ServiceResponse.error(payload: { alert: alert }, message: message)
end end
......
...@@ -11612,6 +11612,9 @@ msgstr "" ...@@ -11612,6 +11612,9 @@ msgstr ""
msgid "Invalid start or end time format" msgid "Invalid start or end time format"
msgstr "" msgstr ""
msgid "Invalid status"
msgstr ""
msgid "Invalid two-factor code." msgid "Invalid two-factor code."
msgstr "" msgstr ""
......
...@@ -3,6 +3,7 @@ require 'ffaker' ...@@ -3,6 +3,7 @@ require 'ffaker'
FactoryBot.define do FactoryBot.define do
factory :alert_management_alert, class: 'AlertManagement::Alert' do factory :alert_management_alert, class: 'AlertManagement::Alert' do
triggered
project project
title { FFaker::Lorem.sentence } title { FFaker::Lorem.sentence }
started_at { Time.current } started_at { Time.current }
...@@ -35,6 +36,11 @@ FactoryBot.define do ...@@ -35,6 +36,11 @@ FactoryBot.define do
ended_at { nil } ended_at { nil }
end end
trait :triggered do
status { AlertManagement::Alert::STATUSES[:triggered] }
without_ended_at
end
trait :acknowledged do trait :acknowledged do
status { AlertManagement::Alert::STATUSES[:acknowledged] } status { AlertManagement::Alert::STATUSES[:acknowledged] }
without_ended_at without_ended_at
......
...@@ -4,7 +4,7 @@ require 'spec_helper' ...@@ -4,7 +4,7 @@ require 'spec_helper'
describe Mutations::AlertManagement::UpdateAlertStatus do describe Mutations::AlertManagement::UpdateAlertStatus do
let_it_be(:current_user) { create(:user) } let_it_be(:current_user) { create(:user) }
let_it_be(:alert) { create(:alert_management_alert, status: 'triggered') } let_it_be(:alert) { create(:alert_management_alert, :triggered) }
let_it_be(:project) { alert.project } let_it_be(:project) { alert.project }
let(:new_status) { Types::AlertManagement::StatusEnum.values['ACKNOWLEDGED'].value } let(:new_status) { Types::AlertManagement::StatusEnum.values['ACKNOWLEDGED'].value }
let(:args) { { status: new_status, project_path: project.full_path, iid: alert.iid } } let(:args) { { status: new_status, project_path: project.full_path, iid: alert.iid } }
...@@ -53,7 +53,7 @@ describe Mutations::AlertManagement::UpdateAlertStatus do ...@@ -53,7 +53,7 @@ describe Mutations::AlertManagement::UpdateAlertStatus do
it 'returns the alert with errors' do it 'returns the alert with errors' do
expect(resolve).to eq( expect(resolve).to eq(
alert: alert, alert: alert,
errors: ['Invalid status'] errors: [_('Invalid status')]
) )
end end
end end
......
...@@ -3,27 +3,64 @@ ...@@ -3,27 +3,64 @@
require 'spec_helper' require 'spec_helper'
describe AlertManagement::UpdateAlertStatusService do describe AlertManagement::UpdateAlertStatusService do
let_it_be(:alert) { create(:alert_management_alert, status: 'triggered') } let(:project) { alert.project }
let_it_be(:user) { build(:user) }
let_it_be(:alert, reload: true) do
create(:alert_management_alert, :triggered)
end
let(:service) { described_class.new(alert, user, new_status) }
describe '#execute' do describe '#execute' do
subject(:execute) { described_class.new(alert, new_status).execute } shared_examples 'update failure' do |error_message|
it 'returns an error' do
expect(response).to be_error
expect(response.message).to eq(error_message)
expect(response.payload[:alert]).to eq(alert)
end
it 'does not update the status' do
expect { response }.not_to change { alert.status }
end
end
let(:new_status) { Types::AlertManagement::StatusEnum.values['ACKNOWLEDGED'].value } let(:new_status) { Types::AlertManagement::StatusEnum.values['ACKNOWLEDGED'].value }
let(:can_update) { true }
subject(:response) { service.execute }
before do
allow(user).to receive(:can?)
.with(:update_alert_management_alert, project)
.and_return(can_update)
end
it 'returns success' do
expect(response).to be_success
expect(response.payload[:alert]).to eq(alert)
end
it 'updates the status' do it 'updates the status' do
expect { execute }.to change { alert.acknowledged? }.to(true) expect { response }.to change { alert.acknowledged? }.to(true)
end end
context 'with unknown status' do context 'when user has no permissions' do
let(:new_status) { 'random_status' } let(:can_update) { false }
it 'returns an error' do include_examples 'update failure', _('You have no permissions')
expect(execute.status).to eq(:error)
end end
it 'does not update the status' do context 'with no status' do
expect { execute }.not_to change { alert.status } let(:new_status) { nil }
include_examples 'update failure', _('Invalid status')
end end
context 'with unknown status' do
let(:new_status) { -1 }
include_examples 'update failure', _('Invalid status')
end end
end end
end end
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