Avoid branch name checking when creating a new snippet

There a time window between the moment where we start the
snippet creation commit and when the git hooks are called.

In those git hooks we check that the user can only create
the default branch in the repository. Nevertheless, in that
window time, the default branch might change, resulting
in an invalid git operation.
parent 51e5f433
---
title: Avoid branch name checking when creating a new snippet
merge_request: 48995
author:
type: fixed
...@@ -10,12 +10,13 @@ module Gitlab ...@@ -10,12 +10,13 @@ module Gitlab
ATTRIBUTES = %i[oldrev newrev ref branch_name tag_name logger].freeze ATTRIBUTES = %i[oldrev newrev ref branch_name tag_name logger].freeze
attr_reader(*ATTRIBUTES) attr_reader(*ATTRIBUTES)
def initialize(change, default_branch:, logger:) def initialize(change, default_branch:, root_ref:, logger:)
@oldrev, @newrev, @ref = change.values_at(:oldrev, :newrev, :ref) @oldrev, @newrev, @ref = change.values_at(:oldrev, :newrev, :ref)
@branch_name = Gitlab::Git.branch_name(@ref) @branch_name = Gitlab::Git.branch_name(@ref)
@tag_name = Gitlab::Git.tag_name(@ref) @tag_name = Gitlab::Git.tag_name(@ref)
@default_branch = default_branch @default_branch = default_branch
@root_ref = root_ref
@logger = logger @logger = logger
@logger.append_message("Running checks for ref: #{@branch_name || @tag_name}") @logger.append_message("Running checks for ref: #{@branch_name || @tag_name}")
end end
...@@ -34,8 +35,13 @@ module Gitlab ...@@ -34,8 +35,13 @@ module Gitlab
private private
# If the `root_ref` is not present means that this is the first commit to the
# repository and when the default branch is going to be created.
# We allow the first branch creation no matter the name because
# it can be even an imported snippet from an instance with a different
# default branch.
def creation? def creation?
@branch_name != @default_branch && super super && @root_ref && (@branch_name != @default_branch)
end end
end end
end end
......
...@@ -114,7 +114,7 @@ module Gitlab ...@@ -114,7 +114,7 @@ module Gitlab
override :check_single_change_access override :check_single_change_access
def check_single_change_access(change, _skip_lfs_integrity_check: false) def check_single_change_access(change, _skip_lfs_integrity_check: false)
Checks::SnippetCheck.new(change, default_branch: snippet.default_branch, logger: logger).validate! Checks::SnippetCheck.new(change, default_branch: snippet.default_branch, root_ref: snippet.repository.root_ref, logger: logger).validate!
Checks::PushFileCountCheck.new(change, repository: repository, limit: Snippet.max_file_limit, logger: logger).validate! Checks::PushFileCountCheck.new(change, repository: repository, limit: Snippet.max_file_limit, logger: logger).validate!
rescue Checks::TimedLogger::TimeoutError rescue Checks::TimedLogger::TimeoutError
raise TimeoutError, logger.full_message raise TimeoutError, logger.full_message
......
...@@ -13,7 +13,7 @@ RSpec.describe Gitlab::Checks::SnippetCheck do ...@@ -13,7 +13,7 @@ RSpec.describe Gitlab::Checks::SnippetCheck do
let(:creation) { false } let(:creation) { false }
let(:deletion) { false } let(:deletion) { false }
subject { Gitlab::Checks::SnippetCheck.new(changes, default_branch: default_branch, logger: logger) } subject { Gitlab::Checks::SnippetCheck.new(changes, default_branch: default_branch, root_ref: snippet.repository.root_ref, logger: logger) }
describe '#validate!' do describe '#validate!' do
it 'does not raise any error' do it 'does not raise any error' do
...@@ -45,10 +45,18 @@ RSpec.describe Gitlab::Checks::SnippetCheck do ...@@ -45,10 +45,18 @@ RSpec.describe Gitlab::Checks::SnippetCheck do
let(:branch_name) { 'feature' } let(:branch_name) { 'feature' }
end end
context "when branch is 'master'" do context 'when branch is the same as the default branch' do
let(:ref) { 'refs/heads/master' } let(:ref) { "refs/heads/#{default_branch}" }
it "allows the operation" do it 'allows the operation' do
expect { subject.validate! }.not_to raise_error
end
end
context 'when snippet has an empty repo' do
let_it_be(:snippet) { create(:personal_snippet, :empty_repo) }
it 'allows the operation' do
expect { subject.validate! }.not_to raise_error expect { subject.validate! }.not_to raise_error
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