Commit 02bc5650 authored by Tan Le's avatar Tan Le Committed by Michael Kozono

Audit project MR approval permission changes

There is a small gotcha around the flipping logic of the from/to value
for `merge_request_author_approval`. This is to align with user
perception on the UI (i.e. the value `false` means disallowing approval
from authors and effectively maps to the ticked checkbox on the UI)
parent 7285e02f
...@@ -79,6 +79,8 @@ From there, you can see the following actions: ...@@ -79,6 +79,8 @@ From there, you can see the following actions:
- Release was added to a project - Release was added to a project
- Release was updated - Release was updated
- Release milestone associations changed - Release milestone associations changed
- Permission to approve merge requests by committers was updated ([introduced](https://gitlab.com/gitlab-org/gitlab/issues/7531) in GitLab 12.9)
- Permission to approve merge requests by authors was updated ([introduced](https://gitlab.com/gitlab-org/gitlab/issues/7531) in GitLab 12.9)
### Instance events **(PREMIUM ONLY)** ### Instance events **(PREMIUM ONLY)**
......
---
title: Audit project MR approval permission changes
merge_request: 25959
author:
type: added
...@@ -11,9 +11,14 @@ module EE ...@@ -11,9 +11,14 @@ module EE
audit_changes(:repository_size_limit, as: 'repository_size_limit', model: model) audit_changes(:repository_size_limit, as: 'repository_size_limit', model: model)
audit_changes(:packages_enabled, as: 'packages_enabled', model: model) audit_changes(:packages_enabled, as: 'packages_enabled', model: model)
audit_changes(:merge_requests_author_approval, as: 'prevent merge request approval from authors', model: model)
audit_changes(:merge_requests_disable_committers_approval, as: 'prevent merge request approval from reviewers', model: model)
audit_project_feature_changes audit_project_feature_changes
end end
private
def audit_project_feature_changes def audit_project_feature_changes
::EE::Audit::ProjectFeatureChangesAuditor.new(@current_user, model.project_feature, model).execute ::EE::Audit::ProjectFeatureChangesAuditor.new(@current_user, model.project_feature, model).execute
end end
...@@ -40,6 +45,11 @@ module EE ...@@ -40,6 +45,11 @@ module EE
from: model.old_path_with_namespace, from: model.old_path_with_namespace,
to: model.full_path to: model.full_path
} }
when :merge_requests_author_approval
{
from: !model.previous_changes[column].first,
to: !model.previous_changes[column].last
}
else else
{ {
from: model.previous_changes[column].first, from: model.previous_changes[column].first,
......
...@@ -129,6 +129,40 @@ describe 'Projects > Audit Events', :js do ...@@ -129,6 +129,40 @@ describe 'Projects > Audit Events', :js do
end end
end end
describe 'changing merge request approval permission for authors and reviewers' do
before do
project.add_developer(pete)
end
it "appears in the project's audit events" do
visit edit_project_path(project)
page.within('#js-merge-request-approval-settings') do
uncheck 'project_merge_requests_author_approval'
check 'project_merge_requests_disable_committers_approval'
click_button 'Save changes'
end
wait_for('Save is completed') do
page.has_content?('was successfully updated', wait: 0)
end
page.within('.qa-project-sidebar') do
find(:link, text: 'Settings').click
click_link 'Audit Events'
end
wait_for_all_requests
page.within('#audits') do
expect(page).to have_content(project.owner.name)
expect(page).to have_content('Change prevent merge request approval from authors')
expect(page).to have_content('Change prevent merge request approval from reviewers')
expect(page).to have_content(project.name)
end
end
end
it_behaves_like 'audit event contains custom message' do it_behaves_like 'audit event contains custom message' do
let(:audit_events_url) { project_audit_events_path(project) } let(:audit_events_url) { project_audit_events_path(project) }
end end
......
...@@ -4,9 +4,21 @@ require 'spec_helper' ...@@ -4,9 +4,21 @@ require 'spec_helper'
describe EE::Audit::ProjectChangesAuditor do describe EE::Audit::ProjectChangesAuditor do
describe '.audit_changes' do describe '.audit_changes' do
let!(:user) { create(:user) } let(:user) { create(:user) }
let!(:project) { create(:project, visibility_level: 0) } let(:project) do
let(:foo_instance) { described_class.new(user, project) } create(
:project,
visibility_level: 0,
name: 'interesting name',
path: 'interesting-path',
repository_size_limit: 10,
packages_enabled: true,
merge_requests_author_approval: false,
merge_requests_disable_committers_approval: true
)
end
subject(:foo_instance) { described_class.new(user, project) }
before do before do
project.reload project.reload
...@@ -65,6 +77,32 @@ describe EE::Audit::ProjectChangesAuditor do ...@@ -65,6 +77,32 @@ describe EE::Audit::ProjectChangesAuditor do
expect { foo_instance.execute }.to change { SecurityEvent.count }.by(1) expect { foo_instance.execute }.to change { SecurityEvent.count }.by(1)
expect(SecurityEvent.last.details[:change]).to eq 'packages_enabled' expect(SecurityEvent.last.details[:change]).to eq 'packages_enabled'
end end
it 'creates an event when the merge requests author approval changes' do
project.update!(merge_requests_author_approval: true)
aggregate_failures do
expect { foo_instance.execute }.to change { SecurityEvent.count }.by(1)
expect(SecurityEvent.last.details).to include(
change: 'prevent merge request approval from authors',
from: true,
to: false
)
end
end
it 'creates an event when the merge requests committers approval changes' do
project.update!(merge_requests_disable_committers_approval: false)
aggregate_failures do
expect { foo_instance.execute }.to change { SecurityEvent.count }.by(1)
expect(SecurityEvent.last.details).to include(
change: 'prevent merge request approval from reviewers',
from: true,
to: false
)
end
end
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