Commit 1760d820 authored by Marc Shaw's avatar Marc Shaw

Add allowed to push superseding codeowners for web ide

Issue: gitlab.com/gitlab-org/gitlab/-/issues/35097
MR: gitlab.com/gitlab-org/gitlab/-/merge_requests/50608
parent 298ebf6f
---
title: Fix codeowners superseding web ide and single file edit
merge_request: 50608
author:
type: fixed
...@@ -33,6 +33,7 @@ module EE ...@@ -33,6 +33,7 @@ module EE
def check_against_codeowners(project, branch, paths) def check_against_codeowners(project, branch, paths)
return if paths.empty? return if paths.empty?
return if ::Feature.enabled?(:push_rules_supersede_code_owners, project, default_enabled: true)
::Gitlab::CodeOwners::Validator.new(project, branch, paths).execute ::Gitlab::CodeOwners::Validator.new(project, branch, paths).execute
end end
......
...@@ -8,39 +8,51 @@ RSpec.describe Projects::BlobController do ...@@ -8,39 +8,51 @@ RSpec.describe Projects::BlobController do
let(:project) { create(:project, :public, :repository) } let(:project) { create(:project, :public, :repository) }
shared_examples "renders to the expected_view with a code owners error msg" do shared_examples_for "handling the codeowners interaction" do
let(:error_msg) do it "redirects to blob" do
"Pushes to protected branches that contain changes to files that match " \ default_params[:file_path] = "docs/EXAMPLE_FILE"
"patterns defined in `CODEOWNERS` are disabled for this project. " \
"Please submit these changes via a merge request. The following " \ subject
"pattern(s) from `CODEOWNERS` were matched: - docs/ "
expect(flash[:alert]).to eq(nil)
expect(response).to be_redirect
end end
before do context 'when push_rules_supersede_code_owners is false' do
allow(::ProtectedBranch).to receive(:branch_requires_code_owner_approval?) let(:error_msg) do
.and_return(true) "Pushes to protected branches that contain changes to files that match " \
"patterns defined in `CODEOWNERS` are disabled for this project. " \
expect_next_instance_of(Repository) do |repo| "Please submit these changes via a merge request. The following " \
allow(repo).to receive(:code_owners_blob) "pattern(s) from `CODEOWNERS` were matched: - docs/ "
.with(ref: "master")
.and_return(
fake_blob(
path: "CODEOWNERS",
data: "*.rb @#{user.username}\ndocs/ @#{user.username}"
)
)
end end
stub_licensed_features(code_owner_approval_required: true) before do
end allow(::ProtectedBranch).to receive(:branch_requires_code_owner_approval?)
.and_return(true)
expect_next_instance_of(Repository) do |repo|
allow(repo).to receive(:code_owners_blob)
.with(ref: "master")
.and_return(
fake_blob(
path: "CODEOWNERS",
data: "*.rb @#{user.username}\ndocs/ @#{user.username}"
)
)
end
it "renders to the edit page with an error msg" do stub_licensed_features(code_owner_approval_required: true)
default_params[:file_path] = "docs/EXAMPLE_FILE" stub_feature_flags(push_rules_supersede_code_owners: false)
end
subject it "renders to the edit page with an error msg" do
default_params[:file_path] = "docs/EXAMPLE_FILE"
subject
expect(flash[:alert]).to eq(error_msg) expect(flash[:alert]).to eq(error_msg)
expect(response).to render_template(expected_view) expect(response).to render_template(expected_view)
end
end end
end end
...@@ -70,7 +82,7 @@ RSpec.describe Projects::BlobController do ...@@ -70,7 +82,7 @@ RSpec.describe Projects::BlobController do
expect(response).to be_redirect expect(response).to be_redirect
end end
it_behaves_like "renders to the expected_view with a code owners error msg" do it_behaves_like "handling the codeowners interaction" do
subject { post :create, params: default_params } subject { post :create, params: default_params }
let(:expected_view) { :new } let(:expected_view) { :new }
...@@ -96,7 +108,7 @@ RSpec.describe Projects::BlobController do ...@@ -96,7 +108,7 @@ RSpec.describe Projects::BlobController do
sign_in(user) sign_in(user)
end end
it_behaves_like "renders to the expected_view with a code owners error msg" do it_behaves_like "handling the codeowners interaction" do
subject { put :update, params: default_params } subject { put :update, params: default_params }
let(:expected_view) { :edit } let(:expected_view) { :edit }
......
...@@ -24,11 +24,25 @@ RSpec.describe 'EE IDE user commits changes', :js do ...@@ -24,11 +24,25 @@ RSpec.describe 'EE IDE user commits changes', :js do
ide_visit(project) ide_visit(project)
end end
it 'shows error message' do it 'does not show an error message' do
ide_create_new_file('test.rb', content: '# A ruby file') ide_create_new_file('test.rb', content: '# A ruby file')
ide_commit ide_commit
expect(page).to have_content('CODEOWNERS rule violation') expect(page).not_to have_content('CODEOWNERS rule violation')
end
context 'when the push_rules_supersede_codeowners is false' do
before do
stub_feature_flags(push_rules_supersede_code_owners: false)
end
it 'shows error message' do
ide_create_new_file('test.rb', content: '# A ruby file')
ide_commit
expect(page).to have_content('CODEOWNERS rule violation')
end
end end
end end
...@@ -11,31 +11,42 @@ RSpec.describe API::Commits do ...@@ -11,31 +11,42 @@ RSpec.describe API::Commits do
project.add_maintainer(user) project.add_maintainer(user)
end end
shared_examples_for "returns a 400 from a codeowners violation" do shared_examples_for "handling the codeowners interaction" do
let(:error_msg) { "CodeOwners error msg" } it "does not create a new validator" do
before do
allow(ProtectedBranch)
.to receive(:branch_requires_code_owner_approval?)
.with(project, branch).and_return(code_owner_approval_required)
end
it "creates a new validator with expected parameters" do
expect(Gitlab::CodeOwners::Validator) expect(Gitlab::CodeOwners::Validator)
.to receive(:new).with(project, branch, Array(paths)).and_call_original .not_to receive(:new)
subject subject
end end
specify do context 'when push_rules_supersede_code_owners is false' do
expect_next_instance_of(Gitlab::CodeOwners::Validator) do |validator| let(:error_msg) { "CodeOwners error msg" }
expect(validator).to receive(:execute).and_return(error_msg)
before do
stub_feature_flags(push_rules_supersede_code_owners: false)
allow(ProtectedBranch)
.to receive(:branch_requires_code_owner_approval?)
.with(project, branch).and_return(code_owner_approval_required)
end end
subject it "creates a new validator with expected parameters" do
expect(Gitlab::CodeOwners::Validator)
.to receive(:new).with(project, branch, Array(paths)).and_call_original
expect(response).to have_gitlab_http_status(:bad_request) subject
expect(json_response["message"]).to include(error_msg) end
specify do
expect_next_instance_of(Gitlab::CodeOwners::Validator) do |validator|
expect(validator).to receive(:execute).and_return(error_msg)
end
subject
expect(response).to have_gitlab_http_status(:bad_request)
expect(json_response["message"]).to include(error_msg)
end
end end
end end
...@@ -80,7 +91,7 @@ RSpec.describe API::Commits do ...@@ -80,7 +91,7 @@ RSpec.describe API::Commits do
let(:branch) { valid_c_params[:branch] } let(:branch) { valid_c_params[:branch] }
let(:paths) { valid_c_params[:actions].first[:file_path] } let(:paths) { valid_c_params[:actions].first[:file_path] }
it_behaves_like "returns a 400 from a codeowners violation" it_behaves_like "handling the codeowners interaction"
end end
end end
end end
...@@ -115,7 +126,7 @@ RSpec.describe API::Commits do ...@@ -115,7 +126,7 @@ RSpec.describe API::Commits do
let(:branch) { valid_d_params[:branch] } let(:branch) { valid_d_params[:branch] }
let(:paths) { valid_d_params[:actions].first[:file_path] } let(:paths) { valid_d_params[:actions].first[:file_path] }
it_behaves_like "returns a 400 from a codeowners violation" it_behaves_like "handling the codeowners interaction"
end end
end end
...@@ -154,7 +165,7 @@ RSpec.describe API::Commits do ...@@ -154,7 +165,7 @@ RSpec.describe API::Commits do
[action[:file_path], action[:previous_path]] [action[:file_path], action[:previous_path]]
end end
it_behaves_like "returns a 400 from a codeowners violation" it_behaves_like "handling the codeowners interaction"
end end
end end
end end
...@@ -185,7 +196,7 @@ RSpec.describe API::Commits do ...@@ -185,7 +196,7 @@ RSpec.describe API::Commits do
let(:code_owner_approval_required) { true } let(:code_owner_approval_required) { true }
let(:paths) { commit.raw_deltas.flat_map { |diff| [diff.new_path, diff.old_path] }.uniq } let(:paths) { commit.raw_deltas.flat_map { |diff| [diff.new_path, diff.old_path] }.uniq }
it_behaves_like "returns a 400 from a codeowners violation" it_behaves_like "handling the codeowners interaction"
end end
end end
end end
...@@ -219,7 +230,7 @@ RSpec.describe API::Commits do ...@@ -219,7 +230,7 @@ RSpec.describe API::Commits do
let(:code_owner_approval_required) { true } let(:code_owner_approval_required) { true }
let(:paths) { commit.raw_deltas.flat_map { |diff| [diff.new_path, diff.old_path] }.uniq } let(:paths) { commit.raw_deltas.flat_map { |diff| [diff.new_path, diff.old_path] }.uniq }
it_behaves_like "returns a 400 from a codeowners violation" it_behaves_like "handling the codeowners interaction"
end end
end end
end end
......
...@@ -5,13 +5,15 @@ require 'spec_helper' ...@@ -5,13 +5,15 @@ require 'spec_helper'
RSpec.describe Commits::CreateService do RSpec.describe Commits::CreateService do
let(:project) { create(:project) } let(:project) { create(:project) }
let(:user) { create(:user) } let(:user) { create(:user) }
let(:branch_name) { 'master' }
let(:extra_params) { {} }
before do before do
project.add_maintainer(user) project.add_maintainer(user)
end end
subject(:service) do subject(:service) do
described_class.new(project, user, start_branch: 'master', branch_name: 'master') described_class.new(project, user, start_branch: branch_name, branch_name: branch_name, **extra_params)
end end
describe '#execute' do describe '#execute' do
...@@ -29,5 +31,30 @@ RSpec.describe Commits::CreateService do ...@@ -29,5 +31,30 @@ RSpec.describe Commits::CreateService do
expect(result[:status]).to be(:error) expect(result[:status]).to be(:error)
expect(result[:message]).to eq('Your changes could not be committed, because this repository has exceeded its size limit of 1 Byte by 1 Byte') expect(result[:message]).to eq('Your changes could not be committed, because this repository has exceeded its size limit of 1 Byte by 1 Byte')
end end
context 'when validating codeowners' do
let(:extra_params) { { file_path: 'path', actions: [{ file_path: 'a', previous_path: 'b' }] } }
context 'when the paths are empty' do
let(:extra_params) { {} }
it 'does not validate' do
expect(::Gitlab::CodeOwners::Validator).not_to receive(:new)
result
end
end
it 'does not validate when the push_rules_supersede_code_owners flag is true' do
expect(::Gitlab::CodeOwners::Validator).not_to receive(:new)
result
end
it 'validates the code owners file when the push_rules_supersede_code_owners flag is false' do
stub_feature_flags(push_rules_supersede_code_owners: false)
expect(::Gitlab::CodeOwners::Validator).to receive(:new).with(project, branch_name, %w[path a b]).and_call_original
result
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