Commit 4be19816 authored by Georges Racinet's avatar Georges Racinet 🦑
Browse files

RSpec tests for hg_access: split in two

The tests involving protected branches are likely to have different
setup needs that the other. The bad news is that it's unbalanced:
tests for protected branches take 10mn on my system, the others
`hg_access_main_spec.rb` amount for just 3mn.

--HG--
branch : heptapod-stable
parent 663dfccf451f
......@@ -656,270 +656,6 @@ def disable_protocol(protocol)
end
end
describe '#check_push_access!' do
let(:unprotected_branch) { 'branch/unprotected' }
# yes, the protected branch is 'feature'. That probably means it's the
# *potentially* protected branch
# TODO we'll actually need a method to spit out a topic name
# for a protected branch name (either branch/feature or branch/exact
let(:topic_protected_branch) { 'topic/feature/follow-up' }
before do
mergeable_into_protected_branch
end
let(:mr_iid) { nil }
# last element of each list is the accept_mr_iid
let(:changes) do
{ any: [Gitlab::GitAccess::ANY, nil],
push_new_branch: ["#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/heads/wow",
nil],
push_master: ['6f6d7e7ed 570e7b2ab refs/heads/branch/default', nil],
push_protected_branch: [
'6f6d7e7ed 570e7b2ab refs/heads/branch/feature', nil
],
push_remove_protected_branch: ["570e7b2ab #{Gitlab::Git::BLANK_SHA} "\
'refs/heads/branch/feature', nil],
push_tag: ['6f6d7e7ed 570e7b2ab refs/tags/v1.0.0', nil],
push_new_tag: [
"#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/tags/v7.8.9", nil],
push_all: ['6f6d7e7ed 570e7b2ab refs/heads/master',
'6f6d7e7ed 570e7b2ab refs/heads/branch/feature',
nil],
merge_into_protected_branch: [
"0b4bc9a cafe1234 refs/heads/branch/feature", mr_iid
] }
end
# GR rename as prepare_mergeable_into_protected_branch ?
def mergeable_into_protected_branch
# This is obviously to avoid repeated setup. The examples don't
# actually perform actual pushes, so we can indeed just keep it as is
@mergeable_into_protected_branch_sha ||=
begin
project.repository.create_file(
project.creator,
'feature', # file name
'start of "feature" branch', # file content
branch_name: 'branch/feature',
start_branch_name: 'branch/default',
message: "potentially protected branch creation"
)
project.repository.create_file(
user,
'filename',
'This is the file content',
message: 'This is a good commit message',
start_branch_name: 'branch/feature',
branch_name: topic_protected_branch
)
project.repository.find_branch(topic_protected_branch).target
end
end
def self.run_permission_checks(permissions_matrix)
permissions_matrix.each_pair do |role, matrix|
# Run through the entire matrix for this role in one test to avoid
# repeated setup.
#
# Expectations are given a custom failure message proc so that it's
# easier to identify which check(s) failed.
it "has the correct permissions for #{role}s" do
if role == :admin
user.update_attribute(:admin, true)
project.add_guest(user)
else
project.add_role(user, role)
end
protected_branch.save!
aggregate_failures do
matrix.each do |action, allowed|
check = -> { push_changes(changes[action]) }
if allowed
expect(&check).not_to raise_error,
-> { "expected #{action} to be allowed" }
else
expect(&check).to raise_error(Gitlab::GitAccess::ForbiddenError),
-> { "expected #{action} to be disallowed" }
end
end
end
end
end
end
permissions_matrix = {
admin: {
any: true,
push_new_branch: true,
push_master: true,
push_protected_branch: true,
push_remove_protected_branch: false,
push_tag: true,
push_new_tag: true,
push_all: true,
merge_into_protected_branch: true
},
maintainer: {
any: true,
push_new_branch: true,
push_master: true,
push_protected_branch: true,
push_remove_protected_branch: false,
push_tag: true,
push_new_tag: true,
push_all: true,
merge_into_protected_branch: true
},
developer: {
any: true,
push_new_branch: true,
push_master: true,
push_protected_branch: false,
push_remove_protected_branch: false,
push_tag: true,
push_new_tag: true,
push_all: false,
merge_into_protected_branch: false
},
reporter: {
any: false,
push_new_branch: false,
push_master: false,
push_protected_branch: false,
push_remove_protected_branch: false,
push_tag: false,
push_new_tag: false,
push_all: false,
merge_into_protected_branch: false
},
guest: {
any: false,
push_new_branch: false,
push_master: false,
push_protected_branch: false,
push_remove_protected_branch: false,
push_tag: false,
push_new_tag: false,
push_all: false,
merge_into_protected_branch: false
}
}
[%w(branch/feature exact), ['branch/feat*', 'wildcard']].each do |protected_branch_name, protected_branch_type|
context do
let(:protected_branch) { create(:protected_branch, :maintainers_can_push, name: protected_branch_name, project: project) }
run_permission_checks(permissions_matrix)
end
context "when developers are allowed to push into the #{protected_branch_type} protected branch" do
let(:protected_branch) { create(:protected_branch, :developers_can_push, name: protected_branch_name, project: project) }
run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true }))
end
context "developers are allowed to merge into the #{protected_branch_type} protected branch" do
let(:protected_branch) { create(:protected_branch, :developers_can_merge, name: protected_branch_name, project: project) }
context "when a merge request exists for the given source/target branch" do
context "when the merge request is in progress" do
let(:mr_iid) { create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'branch/feature').iid }
run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: true }))
end
context "when the merge request is not in progress" do
before do
create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'branch/feature', in_progress_merge_commit_sha: nil)
end
run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: false }))
end
context "when a merge request does not exist for the given source/target branch" do
run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: false }))
end
end
end
context "when developers are allowed to push and merge into the #{protected_branch_type} protected branch" do
let(:protected_branch) { create(:protected_branch, :developers_can_merge, :developers_can_push, name: protected_branch_name, project: project) }
run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true }))
end
context "when no one is allowed to push to the #{protected_branch_name} protected branch" do
let(:protected_branch) { build(:protected_branch, :no_one_can_push, name: protected_branch_name, project: project) }
run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false },
maintainer: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false },
admin: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false }))
end
end
context 'when pushing to a project' do
let(:project) { create(:hg_git_project, :public, :hg_repository) }
let(:changes) { "#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/heads/topic/default/wow" }
before do
project.add_developer(user)
end
it 'does not allow deactivated users to push' do
user.deactivate!
expect { push_access_check }.to raise_forbidden("Your account has been deactivated by your administrator. Please log back in from a web browser to reactivate your account at #{Gitlab.config.gitlab.url}")
end
it 'cleans up the files' do
expect(project.repository).to receive(:clean_stale_repository_files).and_call_original
expect { push_access_check }.not_to raise_error
end
it 'avoids N+1 queries' do
# the `config.around` (it 'blabla', :request_store') in the
# corresponding GitAccess test wreaks havoc with our setup, with
# even the first "baseline" call being refused (not allowed to push)
# and that's really hard to debug.
# But putting it inside the example as we're doing here solves
# this issue:
Gitlab::WithRequestStore.with_request_store do
# Run this once to establish a baseline. Cached queries should get
# cached, so that when we introduce another change we shouldn't see
# additional queries.
access.check('unbundle', changes)
control_count = ActiveRecord::QueryRecorder.new do
access.check('unbundle', changes)
end
changes = ['6f6d7e7ed 570e7b2ab refs/heads/branch/default', '6f6d7e7ed 570e7b2ab refs/heads/feature']
# There is still an N+1 query with protected branches
expect { access.check('unbundle', changes) }.not_to exceed_query_limit(control_count).with_threshold(2)
end
end
it 'raises TimeoutError when #check_single_change_access raises a timeout error' do
message = "Push operation timed out\n\nTiming information for debugging purposes:\nRunning checks for ref: topic/default/wow"
expect_next_instance_of(Gitlab::Checks::ChangeAccess) do |check|
expect(check).to receive(:validate!).and_raise(Gitlab::Checks::TimedLogger::TimeoutError)
end
expect { access.check('unbundle', changes) }.to raise_error(described_class::TimeoutError, message)
end
end
end
describe 'build authentication abilities' do
let(:authentication_abilities) { build_authentication_abilities }
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Gitlab::HgAccess do
include TermsHelper
include GitHelpers
let(:user) { create(:user) }
let(:actor) { user }
let(:project) { create(:hg_git_project, :hg_repository) }
let(:project_path) { project&.path }
let(:namespace_path) { project&.namespace&.path }
let(:protocol) { 'ssh' }
let(:authentication_abilities) { %i[read_project download_code push_code] }
let(:redirected_path) { nil }
let(:auth_result_type) { nil }
let(:changes) { Gitlab::GitAccess::ANY }
let(:push_access_check) { access.check('unbundle', changes) }
let(:pull_access_check) { access.check('getbundle', changes) }
describe '#check_push_access!' do
let(:unprotected_branch) { 'branch/unprotected' }
# yes, the protected branch is 'feature'. That probably means it's the
# *potentially* protected branch
# TODO we'll actually need a method to spit out a topic name
# for a protected branch name (either branch/feature or branch/exact
let(:topic_protected_branch) { 'topic/feature/follow-up' }
before do
mergeable_into_protected_branch
end
let(:mr_iid) { nil }
# last element of each list is the accept_mr_iid
let(:changes) do
{ any: [Gitlab::GitAccess::ANY, nil],
push_new_branch: ["#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/heads/wow",
nil],
push_master: ['6f6d7e7ed 570e7b2ab refs/heads/branch/default', nil],
push_protected_branch: [
'6f6d7e7ed 570e7b2ab refs/heads/branch/feature', nil
],
push_remove_protected_branch: ["570e7b2ab #{Gitlab::Git::BLANK_SHA} "\
'refs/heads/branch/feature', nil],
push_tag: ['6f6d7e7ed 570e7b2ab refs/tags/v1.0.0', nil],
push_new_tag: [
"#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/tags/v7.8.9", nil],
push_all: ['6f6d7e7ed 570e7b2ab refs/heads/master',
'6f6d7e7ed 570e7b2ab refs/heads/branch/feature',
nil],
merge_into_protected_branch: [
"0b4bc9a cafe1234 refs/heads/branch/feature", mr_iid
] }
end
# GR rename as prepare_mergeable_into_protected_branch ?
def mergeable_into_protected_branch
# This is obviously to avoid repeated setup. The examples don't
# actually perform actual pushes, so we can indeed just keep it as is
@mergeable_into_protected_branch_sha ||=
begin
project.repository.create_file(
project.creator,
'feature', # file name
'start of "feature" branch', # file content
branch_name: 'branch/feature',
start_branch_name: 'branch/default',
message: "potentially protected branch creation"
)
project.repository.create_file(
user,
'filename',
'This is the file content',
message: 'This is a good commit message',
start_branch_name: 'branch/feature',
branch_name: topic_protected_branch
)
project.repository.find_branch(topic_protected_branch).target
end
end
def self.run_permission_checks(permissions_matrix)
permissions_matrix.each_pair do |role, matrix|
# Run through the entire matrix for this role in one test to avoid
# repeated setup.
#
# Expectations are given a custom failure message proc so that it's
# easier to identify which check(s) failed.
it "has the correct permissions for #{role}s" do
if role == :admin
user.update_attribute(:admin, true)
project.add_guest(user)
else
project.add_role(user, role)
end
protected_branch.save!
aggregate_failures do
matrix.each do |action, allowed|
check = -> { push_changes(changes[action]) }
if allowed
expect(&check).not_to raise_error,
-> { "expected #{action} to be allowed" }
else
expect(&check).to raise_error(Gitlab::GitAccess::ForbiddenError),
-> { "expected #{action} to be disallowed" }
end
end
end
end
end
end
permissions_matrix = {
admin: {
any: true,
push_new_branch: true,
push_master: true,
push_protected_branch: true,
push_remove_protected_branch: false,
push_tag: true,
push_new_tag: true,
push_all: true,
merge_into_protected_branch: true
},
maintainer: {
any: true,
push_new_branch: true,
push_master: true,
push_protected_branch: true,
push_remove_protected_branch: false,
push_tag: true,
push_new_tag: true,
push_all: true,
merge_into_protected_branch: true
},
developer: {
any: true,
push_new_branch: true,
push_master: true,
push_protected_branch: false,
push_remove_protected_branch: false,
push_tag: true,
push_new_tag: true,
push_all: false,
merge_into_protected_branch: false
},
reporter: {
any: false,
push_new_branch: false,
push_master: false,
push_protected_branch: false,
push_remove_protected_branch: false,
push_tag: false,
push_new_tag: false,
push_all: false,
merge_into_protected_branch: false
},
guest: {
any: false,
push_new_branch: false,
push_master: false,
push_protected_branch: false,
push_remove_protected_branch: false,
push_tag: false,
push_new_tag: false,
push_all: false,
merge_into_protected_branch: false
}
}
[%w(branch/feature exact), ['branch/feat*', 'wildcard']].each do |protected_branch_name, protected_branch_type|
context do
let(:protected_branch) { create(:protected_branch, :maintainers_can_push, name: protected_branch_name, project: project) }
run_permission_checks(permissions_matrix)
end
context "when developers are allowed to push into the #{protected_branch_type} protected branch" do
let(:protected_branch) { create(:protected_branch, :developers_can_push, name: protected_branch_name, project: project) }
run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true }))
end
context "developers are allowed to merge into the #{protected_branch_type} protected branch" do
let(:protected_branch) { create(:protected_branch, :developers_can_merge, name: protected_branch_name, project: project) }
context "when a merge request exists for the given source/target branch" do
context "when the merge request is in progress" do
let(:mr_iid) { create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'branch/feature').iid }
run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: true }))
end
context "when the merge request is not in progress" do
before do
create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'branch/feature', in_progress_merge_commit_sha: nil)
end
run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: false }))
end
context "when a merge request does not exist for the given source/target branch" do
run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: false }))
end
end
end
context "when developers are allowed to push and merge into the #{protected_branch_type} protected branch" do
let(:protected_branch) { create(:protected_branch, :developers_can_merge, :developers_can_push, name: protected_branch_name, project: project) }
run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true }))
end
context "when no one is allowed to push to the #{protected_branch_name} protected branch" do
let(:protected_branch) { build(:protected_branch, :no_one_can_push, name: protected_branch_name, project: project) }
run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false },
maintainer: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false },
admin: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false }))
end
end
context 'when pushing to a project' do
let(:project) { create(:hg_git_project, :public, :hg_repository) }
let(:changes) { "#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/heads/topic/default/wow" }
before do
project.add_developer(user)
end
it 'does not allow deactivated users to push' do
user.deactivate!
expect { push_access_check }.to raise_forbidden("Your account has been deactivated by your administrator. Please log back in from a web browser to reactivate your account at #{Gitlab.config.gitlab.url}")
end
it 'cleans up the files' do
expect(project.repository).to receive(:clean_stale_repository_files).and_call_original
expect { push_access_check }.not_to raise_error
end
it 'avoids N+1 queries' do
# the `config.around` (it 'blabla', :request_store') in the
# corresponding GitAccess test wreaks havoc with our setup, with
# even the first "baseline" call being refused (not allowed to push)
# and that's really hard to debug.
# But putting it inside the example as we're doing here solves
# this issue:
Gitlab::WithRequestStore.with_request_store do
# Run this once to establish a baseline. Cached queries should get
# cached, so that when we introduce another change we shouldn't see
# additional queries.
access.check('unbundle', changes)
control_count = ActiveRecord::QueryRecorder.new do
access.check('unbundle', changes)
end
changes = ['6f6d7e7ed 570e7b2ab refs/heads/branch/default', '6f6d7e7ed 570e7b2ab refs/heads/feature']
# There is still an N+1 query with protected branches
expect { access.check('unbundle', changes) }.not_to exceed_query_limit(control_count).with_threshold(2)
end
end
it 'raises TimeoutError when #check_single_change_access raises a timeout error' do
message = "Push operation timed out\n\nTiming information for debugging purposes:\nRunning checks for ref: topic/default/wow"
expect_next_instance_of(Gitlab::Checks::ChangeAccess) do |check|
expect(check).to receive(:validate!).and_raise(Gitlab::Checks::TimedLogger::TimeoutError)
end
expect { access.check('unbundle', changes) }.to raise_error(described_class::TimeoutError, message)
end
end
end
private
def access(accept_mr_iid: nil)
described_class.new(actor, project, protocol,
authentication_abilities: authentication_abilities,
namespace_path: namespace_path, repository_path: project_path,
redirected_path: redirected_path, auth_result_type: auth_result_type, accept_mr_iid: accept_mr_iid)
end
def push_changes(changes)
mr_iid = changes.pop
access(accept_mr_iid: mr_iid).check('unbundle', changes)
end
def raise_forbidden(message)
raise_error(Gitlab::HgAccess::ForbiddenError, message)
end
def raise_not_found
raise_error(Gitlab::GitAccess::NotFoundError, Gitlab::GitAccess::ERROR_MESSAGES[:project_not_found])
end
def raise_namespace_not_found
raise_error(Gitlab::GitAccess::NotFoundError, Gitlab::GitAccess::ERROR_MESSAGES[:namespace_not_found])
end
def build_authentication_abilities
[
:read_project,
:build_download_code
]
end
def full_authentication_abilities
[
:read_project,
:download_code,
:push_code
]
end
end
Markdown is supported
0%