Skip to content

G
gitlab-ce
Commit f3caeb39 authored by Fabio Pitino's avatar Fabio Pitino
Browse files
Options

Merge branch 'revert-1042adb2' into 'master' 

Revert "Merge branch 'mc/feature/audit-log-variable-changes' into 'master'"

See merge request gitlab-org/gitlab!37941
parents 6ccc837f 8f22bbe3
Show whitespace changes
Inline Side-by-side
Showing with 14 additions and 639 deletions
app/controllers/groups/variables_controller.rb
View file @ f3caeb39
...@@ -15,12 +15,7 @@ module Groups ...@@ -15,12 +15,7 @@ module Groups
end end
def update def update
update_result = Ci::ChangeVariablesService.new( if @group.update(group_variables_params)
container: @group, current_user: current_user,
params: group_variables_params
).execute
if update_result
respond_to do |format| respond_to do |format|
format.json { render_group_variables } format.json { render_group_variables }
end end
......
app/services/ci/change_variable_service.rb deleted 100644 → 0
View file @ 6ccc837f
# frozen_string_literal: true
module Ci
class ChangeVariableService < BaseContainerService
def execute
case params[:action]
when :create
container.variables.create(params[:variable_params])
when :update
variable.tap do |target_variable|
target_variable.update(params[:variable_params].except(:key))
end
when :destroy
variable.tap do |target_variable|
target_variable.destroy
end
end
end
private
def variable
container.variables.find_by!(params[:variable_params].slice(:key)) # rubocop:disable CodeReuse/ActiveRecord
end
end
end
::Ci::ChangeVariableService.prepend_if_ee('EE::Ci::ChangeVariableService')
app/services/ci/change_variables_service.rb deleted 100644 → 0
View file @ 6ccc837f
# frozen_string_literal: true
module Ci
class ChangeVariablesService < BaseContainerService
def execute
container.update(params)
end
end
end
::Ci::ChangeVariablesService.prepend_if_ee('EE::Ci::ChangeVariablesService')
ee/app/services/ci/audit_variable_change_service.rb deleted 100644 → 0
View file @ 6ccc837f
# frozen_string_literal: true
module Ci
class AuditVariableChangeService < ::BaseContainerService
include ::Audit::Changes
def execute
return unless container.feature_available?(:extended_audit_events)
case params[:action]
when :create, :destroy
log_audit_event(params[:action], params[:variable])
when :update
audit_changes(
:protected,
as: 'variable protection', entity: container,
model: params[:variable], target_details: params[:variable].key
)
end
end
private
def log_audit_event(action, variable)
case variable.class.to_s
when ::Ci::GroupVariable.to_s
::AuditEventService.new(
current_user,
container,
action: action
).for_group_variable(variable.key).security_event
end
end
end
end
ee/app/services/ee/audit_event_service.rb
View file @ f3caeb39
...@@ -179,15 +179,6 @@ module EE ...@@ -179,15 +179,6 @@ module EE
for_custom_model('group', @entity.full_path) for_custom_model('group', @entity.full_path)
end end
# Builds the @details attribute for group variable
#
# This uses the [Ci::GroupVariable] @entity as the target object being audited
#
# @return [AuditEventService]
def for_group_variable(group_variable_key)
for_custom_model('ci_group_variable', group_variable_key)
end
def enabled? def enabled?
admin_audit_log_enabled? || admin_audit_log_enabled? ||
audit_events_enabled? || audit_events_enabled? ||
......
ee/app/services/ee/ci/change_variable_service.rb deleted 100644 → 0
View file @ 6ccc837f
# frozen_string_literal: true
module EE
module Ci
module ChangeVariableService
extend ::Gitlab::Utils::Override
override :execute
def execute
super.tap do |target_variable|
if target_variable.valid?
::Ci::AuditVariableChangeService.new(
container: container,
current_user: current_user,
params: { action: params[:action], variable: target_variable }
).execute
end
end
end
end
end
end
ee/app/services/ee/ci/change_variables_service.rb deleted 100644 → 0
View file @ 6ccc837f
# frozen_string_literal: true
module EE
module Ci
module ChangeVariablesService
extend ::Gitlab::Utils::Override
override :execute
def execute
super.tap do |result|
log_audit_events if result
end
end
private
def log_audit_events
params[:variables_attributes].each do |variable_params|
action = variable_action(variable_params)
target = target_variable(action, variable_params)
::Ci::AuditVariableChangeService.new(
container: container,
current_user: current_user,
params: { action: action, variable: target }
).execute
end
end
def find_variable_by_id(target_id)
container.variables.find { |variable| variable.id.to_s == target_id.to_s }
end
def find_variable_by_key(target_key)
container.variables.find { |variable| variable.key == target_key }
end
def variable_class
container.class.reflect_on_association(:variables).klass
end
def variable_action(variable_params)
if variable_params[:_destroy]
:destroy
elsif variable_params[:id].nil?
:create
else
:update
end
end
def target_variable(action, variable_params)
case action
when :create
find_variable_by_key(variable_params[:key])
when :update
find_variable_by_id(variable_params[:id])
when :destroy
variable_class.new(variable_params.except(:_destroy))
end
end
end
end
end
ee/changelogs/unreleased/mc-feature-audit-log-variable-changes.yml deleted 100644 → 0
View file @ 6ccc837f
---
title: Audit log group variable updates.
merge_request: 35977
author:
type: changed
ee/spec/services/ci/audit_variable_change_service_spec.rb deleted 100644 → 0
View file @ 6ccc837f
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Ci::AuditVariableChangeService do
subject(:execute) { service.execute }
let_it_be(:user) { create(:user) }
let(:group) { create(:group) }
let(:variable) { create(:ci_group_variable) }
let(:service) do
described_class.new(
container: group, current_user: user,
params: { action: action, variable: variable }
)
end
before do
group.variables << variable
end
context 'when extended audits are available' do
before do
stub_licensed_features(extended_audit_events: true)
end
context 'when creating variable' do
let(:action) { :create }
it 'logs audit event' do
expect { execute }.to change(AuditEvent, :count).from(0).to(1)
end
it 'logs variable creation' do
execute
audit_event = AuditEvent.last.present
expect(audit_event.action).to eq('Added ci group variable')
expect(audit_event.target).to eq(variable.key)
end
end
context 'when updating variable protection' do
let(:action) { :update }
before do
variable.update!(protected: true)
end
it 'logs audit event' do
expect { execute }.to change(AuditEvent, :count).from(0).to(1)
end
it 'logs variable protection update' do
execute
audit_event = AuditEvent.last.present
expect(audit_event.action).to eq('Changed variable protection from false to true')
expect(audit_event.target).to eq(variable.key)
end
end
context 'when destroying variable' do
let(:action) { :destroy }
it 'logs audit event' do
expect { execute }.to change(AuditEvent, :count).from(0).to(1)
end
it 'logs variable destruction' do
execute
audit_event = AuditEvent.last.present
expect(audit_event.action).to eq('Removed ci group variable')
expect(audit_event.target).to eq(variable.key)
end
end
end
context 'when extended audits are not available' do
before do
stub_licensed_features(extended_audit_events: false)
end
context 'when creating variable' do
let(:action) { :create }
it 'does not log an audit event' do
expect { execute }.not_to change(AuditEvent, :count).from(0)
end
end
context 'when updating variable protection' do
let(:action) { :update }
before do
variable.update!(protected: true)
end
it 'does not log an audit event' do
expect { execute }.not_to change(AuditEvent, :count).from(0)
end
end
context 'when destroying variable' do
let(:action) { :destroy }
it 'does not log an audit event' do
expect { execute }.not_to change(AuditEvent, :count).from(0)
end
end
end
end
ee/spec/services/ee/ci/change_variable_service_spec.rb deleted 100644 → 0
View file @ 6ccc837f
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Ci::ChangeVariableService do
subject(:execute) { service.execute }
let_it_be(:user) { create(:user) }
let(:group) { create(:group) }
let(:audit_service_spy) { class_spy(::Ci::AuditVariableChangeService, new: spy) }
let(:service) do
described_class.new(
container: group, current_user: user,
params: variable_params
)
end
before do
stub_const('::Ci::AuditVariableChangeService', audit_service_spy)
end
context 'when creating a variable' do
let(:variable_params) { { variable_params: { key: 'new_variable', value: 'new_value' }, action: :create } }
it 'calls AuditVariableChangeService with create' do
execute
expect(audit_service_spy).to have_received(:new).with(
hash_including(
container: group, current_user: user,
params: hash_including(action: :create, variable: instance_of(::Ci::GroupVariable))
)
)
end
end
context 'when updating a variable' do
let(:variable) { create(:ci_group_variable) }
let(:variable_params) { { variable_params: { key: variable.key, protected: 'true' }, action: :update } }
before do
group.variables << variable
end
it 'calls AuditVariableChangeService with update' do
execute
expect(audit_service_spy).to have_received(:new).with(
hash_including(
container: group, current_user: user,
params: hash_including(action: :update, variable: instance_of(::Ci::GroupVariable))
)
)
end
end
context 'when destroying a variable' do
let(:variable) { create(:ci_group_variable, key: 'old_variable') }
let(:variable_params) { { variable_params: { key: variable.key }, action: :destroy } }
before do
group.variables << variable
end
it 'calls AuditVariableChangeService with destroy' do
execute
expect(audit_service_spy).to have_received(:new).with(
hash_including(
container: group, current_user: user,
params: hash_including(action: :destroy, variable: instance_of(::Ci::GroupVariable))
)
)
end
end
end
ee/spec/services/ee/ci/change_variables_service_spec.rb deleted 100644 → 0
View file @ 6ccc837f
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Ci::ChangeVariablesService do
subject(:execute) { service.execute }
let_it_be(:user) { create(:user) }
let(:group) { create(:group) }
let(:audit_service_spy) { class_spy(Ci::AuditVariableChangeService, new: spy) }
let(:service) do
described_class.new(
container: group, current_user: user,
params: variable_params
)
end
before do
stub_const('Ci::AuditVariableChangeService', audit_service_spy)
end
context 'when creating a variable' do
let(:variable_params) { { variables_attributes: [{ key: 'new_variable', value: 'new_value' }] } }
it 'calls AuditVariableChangeService with create' do
execute
expect(audit_service_spy).to have_received(:new).with(
hash_including(
container: group, current_user: user,
params: hash_including(action: :create, variable: instance_of(::Ci::GroupVariable))
)
)
end
end
context 'when updating a variable' do
let(:variable) { create(:ci_group_variable) }
let(:variable_params) { { variables_attributes: [{ id: variable.id }.merge(variable_changes)] } }
before do
group.variables << variable
end
context 'when update succeeds' do
let(:variable_changes) { { protected: 'true' } }
it 'calls AuditVariableChangeService with update' do
execute
expect(audit_service_spy).to have_received(:new).with(
hash_including(
container: group, current_user: user,
params: hash_including(action: :update, variable: instance_of(::Ci::GroupVariable))
)
)
end
end
context 'when update fails' do
let(:variable_changes) { { value: 'shrt', masked: 'true' } }
it 'does not call AuditVariableChangeService' do
execute
expect(audit_service_spy).not_to have_received(:new)
end
end
end
context 'when destroying a variable' do
let(:variable) { create(:ci_group_variable, key: 'old_variable') }
let(:variable_params) { { variables_attributes: [{ id: variable.id, key: variable.key, _destroy: 'true' }] } }
before do
group.variables << variable
end
it 'calls AuditVariableChangeService with destroy' do
execute
expect(audit_service_spy).to have_received(:new).with(
hash_including(
container: group, current_user: user,
params: hash_including(action: :destroy, variable: instance_of(::Ci::GroupVariable))
)
)
end
end
context 'when making multiple changes' do
let(:update_variable) { create(:ci_group_variable) }
let(:delete_variable) { create(:ci_group_variable, key: 'old_variable') }
let(:variable_params) do
{
variables_attributes: [
{ key: 'new_variable', value: 'new_value' },
{ id: update_variable.id, protected: 'true' },
{ id: delete_variable.id, key: delete_variable.key, _destroy: 'true' }
]
}
end
before do
group.variables << update_variable
group.variables << delete_variable
end
it 'calls AuditVariableChangeService with create' do
execute
expect(audit_service_spy).to have_received(:new).with(
hash_including(
container: group, current_user: user,
params: hash_including(action: :create, variable: instance_of(::Ci::GroupVariable))
)
)
end
it 'calls AuditVariableChangeService with update' do
execute
expect(audit_service_spy).to have_received(:new).with(
hash_including(
container: group, current_user: user,
params: hash_including(action: :update, variable: instance_of(::Ci::GroupVariable))
)
)
end
it 'calls AuditVariableChangeService with destroy' do
execute
expect(audit_service_spy).to have_received(:new).with(
hash_including(
container: group, current_user: user,
params: hash_including(action: :destroy, variable: instance_of(::Ci::GroupVariable))
)
)
end
end
end
lib/api/group_variables.rb
View file @ f3caeb39
...@@ -51,11 +51,9 @@ module API ...@@ -51,11 +51,9 @@ module API
optional :variable_type, type: String, values: ::Ci::GroupVariable.variable_types.keys, desc: 'The type of variable, must be one of env_var or file. Defaults to env_var' optional :variable_type, type: String, values: ::Ci::GroupVariable.variable_types.keys, desc: 'The type of variable, must be one of env_var or file. Defaults to env_var'
end end
post ':id/variables' do post ':id/variables' do
variable = ::Ci::ChangeVariableService.new( variable_params = declared_params(include_missing: false)
container: user_group,
current_user: current_user, variable = user_group.variables.create(variable_params)
params: { action: :create, variable_params: declared_params(include_missing: false) }
).execute
if variable.valid? if variable.valid?
present variable, with: Entities::Variable present variable, with: Entities::Variable
...@@ -76,19 +74,17 @@ module API ...@@ -76,19 +74,17 @@ module API
end end
# rubocop: disable CodeReuse/ActiveRecord # rubocop: disable CodeReuse/ActiveRecord
put ':id/variables/:key' do put ':id/variables/:key' do
variable = ::Ci::ChangeVariableService.new( variable = user_group.variables.find_by(key: params[:key])
container: user_group,
current_user: current_user,
params: { action: :update, variable_params: declared_params(include_missing: false) }
).execute
if variable.valid? break not_found!('GroupVariable') unless variable
variable_params = declared_params(include_missing: false).except(:key)
if variable.update(variable_params)
present variable, with: Entities::Variable present variable, with: Entities::Variable
else else
render_validation_error!(variable) render_validation_error!(variable)
end end
rescue ::ActiveRecord::RecordNotFound
not_found!('GroupVariable')
end end
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
...@@ -100,17 +96,10 @@ module API ...@@ -100,17 +96,10 @@ module API
end end
# rubocop: disable CodeReuse/ActiveRecord # rubocop: disable CodeReuse/ActiveRecord
delete ':id/variables/:key' do delete ':id/variables/:key' do
variable = user_group.variables.find_by!(key: params[:key]) variable = user_group.variables.find_by(key: params[:key])
not_found!('GroupVariable') unless variable
destroy_conditionally!(variable) do |target_variable| destroy_conditionally!(variable)
::Ci::ChangeVariableService.new(
container: user_group,
current_user: current_user,
params: { action: :destroy, variable_params: declared_params(include_missing: false) }
).execute
end
rescue ::ActiveRecord::RecordNotFound
not_found!('GroupVariable')
end end
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
end end
......
spec/requests/api/group_variables_spec.rb
View file @ f3caeb39
...@@ -169,14 +169,6 @@ RSpec.describe API::GroupVariables do ...@@ -169,14 +169,6 @@ RSpec.describe API::GroupVariables do
expect(response).to have_gitlab_http_status(:not_found) expect(response).to have_gitlab_http_status(:not_found)
end end
it 'responds with 400 if the update fails' do
put api("/groups/#{group.id}/variables/#{variable.key}", user), params: { value: 'shrt', masked: true }
expect(response).to have_gitlab_http_status(:bad_request)
expect(variable.reload.masked).to eq(false)
expect(json_response['message']).to eq('value' => ['is invalid'])
end
end end
context 'authorized user with invalid permissions' do context 'authorized user with invalid permissions' do
......
spec/services/ci/change_variable_service_spec.rb deleted 100644 → 0
View file @ 6ccc837f
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Ci::ChangeVariableService do
let(:service) { described_class.new(container: group, current_user: user, params: params) }
let_it_be(:user) { create(:user) }
let(:group) { create(:group) }
describe '#execute' do
subject(:execute) { service.execute }
context 'when creating a variable' do
let(:params) { { variable_params: { key: 'new_variable', value: 'variable_value' }, action: :create } }
it 'persists a variable' do
expect { execute }.to change(Ci::GroupVariable, :count).from(0).to(1)
end
end
context 'when updating a variable' do
let!(:variable) { create(:ci_group_variable, value: 'old_value') }
let(:params) { { variable_params: { key: variable.key, value: 'new_value' }, action: :update } }
before do
group.variables << variable
end
it 'updates a variable' do
expect { execute }.to change { variable.reload.value }.from('old_value').to('new_value')
end
context 'when the variable does not exist' do
before do
variable.destroy!
end
it 'raises a record not found error' do
expect { execute }.to raise_error(::ActiveRecord::RecordNotFound)
end
end
end
context 'when destroying a variable' do
let!(:variable) { create(:ci_group_variable) }
let(:params) { { variable_params: { key: variable.key }, action: :destroy } }
before do
group.variables << variable
end
it 'destroys a variable' do
expect { execute }.to change { Ci::GroupVariable.exists?(variable.id) }.from(true).to(false)
end
context 'when the variable does not exist' do
before do
variable.destroy!
end
it 'raises a record not found error' do
expect { execute }.to raise_error(::ActiveRecord::RecordNotFound)
end
end
end
end
end
spec/services/ci/change_variables_service_spec.rb deleted 100644 → 0
View file @ 6ccc837f
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Ci::ChangeVariablesService do
let(:service) { described_class.new(container: group, current_user: user, params: params) }
let_it_be(:user) { create(:user) }
let(:group) { spy(:group, variables: []) }
let(:params) { { variables_attributes: [{ key: 'new_variable', value: 'variable_value' }] } }
describe '#execute' do
subject(:execute) { service.execute }
it 'delegates to ActiveRecord update' do
execute
expect(group).to have_received(:update).with(params)
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
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7