Commit d6d30254 authored by Igor Drozdov's avatar Igor Drozdov

Merge branch 'pks-lfs-pointers-batched-checks' into 'master'

Fix LFS pointer checks only verifying first reference

See merge request gitlab-org/gitlab!61355
parents 7a9043b6 b37691bc
......@@ -3,7 +3,7 @@
module EE
module Gitlab
module Checks
module BaseChecker
module BaseSingleChecker
extend ActiveSupport::Concern
include ::Gitlab::Utils::StrongMemoize
......
......@@ -3,7 +3,7 @@
module EE
module Gitlab
module Checks
class PushRuleCheck < ::Gitlab::Checks::BaseChecker
class PushRuleCheck < ::Gitlab::Checks::BaseSingleChecker
def validate!
return unless push_rule
......
......@@ -4,7 +4,7 @@ module EE
module Gitlab
module Checks
module PushRules
class BranchCheck < ::Gitlab::Checks::BaseChecker
class BranchCheck < ::Gitlab::Checks::BaseSingleChecker
ERROR_MESSAGE = "Branch name does not follow the pattern '%{branch_name_regex}'"
LOG_MESSAGE = "Checking if branch follows the naming patterns defined by the project..."
......
......@@ -4,7 +4,7 @@ module EE
module Gitlab
module Checks
module PushRules
class CommitCheck < ::Gitlab::Checks::BaseChecker
class CommitCheck < ::Gitlab::Checks::BaseSingleChecker
ERROR_MESSAGES = {
committer_not_verified: "Committer email '%{committer_email}' is not verified.",
committer_not_allowed: "You cannot push commits for '%{committer_email}'. You can only push commits that were committed with one of your own verified emails."
......
......@@ -4,7 +4,7 @@ module EE
module Gitlab
module Checks
module PushRules
class FileSizeCheck < ::Gitlab::Checks::BaseChecker
class FileSizeCheck < ::Gitlab::Checks::BaseSingleChecker
LOG_MESSAGE = "Checking if any files are larger than the allowed size..."
def validate!
......
......@@ -4,7 +4,7 @@ module EE
module Gitlab
module Checks
module PushRules
class TagCheck < ::Gitlab::Checks::BaseChecker
class TagCheck < ::Gitlab::Checks::BaseSingleChecker
def validate!
return unless push_rule
......
......@@ -3,7 +3,7 @@
module EE
module Gitlab
module Checks
module ChangeAccess
module SingleChangeAccess
extend ActiveSupport::Concern
extend ::Gitlab::Utils::Override
......
......@@ -2,7 +2,7 @@
require 'spec_helper'
RSpec.describe Gitlab::Checks::ChangeAccess do
RSpec.describe Gitlab::Checks::SingleChangeAccess do
describe '#validate!' do
include_context 'push rules checks context'
......
# frozen_string_literal: true
module Gitlab
module Checks
class BaseBulkChecker < BaseChecker
attr_reader :changes_access
delegate(*ChangesAccess::ATTRIBUTES, to: :changes_access)
def initialize(changes_access)
@changes_access = changes_access
end
def validate!
raise NotImplementedError
end
end
end
end
......@@ -5,39 +5,16 @@ module Gitlab
class BaseChecker
include Gitlab::Utils::StrongMemoize
attr_reader :change_access
delegate(*ChangeAccess::ATTRIBUTES, to: :change_access)
def initialize(change_access)
@change_access = change_access
end
def validate!
raise NotImplementedError
end
private
def creation?
Gitlab::Git.blank_ref?(oldrev)
end
def deletion?
Gitlab::Git.blank_ref?(newrev)
end
def update?
!creation? && !deletion?
end
def updated_from_web?
protocol == 'web'
end
def tag_exists?
project.repository.tag_exists?(tag_name)
end
def validate_once(resource)
Gitlab::SafeRequestStore.fetch(cache_key_for_resource(resource)) do
yield(resource)
......
# frozen_string_literal: true
module Gitlab
module Checks
class BaseSingleChecker < BaseChecker
attr_reader :change_access
delegate(*SingleChangeAccess::ATTRIBUTES, to: :change_access)
def initialize(change_access)
@change_access = change_access
end
private
def creation?
Gitlab::Git.blank_ref?(oldrev)
end
def deletion?
Gitlab::Git.blank_ref?(newrev)
end
def update?
!creation? && !deletion?
end
def tag_exists?
project.repository.tag_exists?(tag_name)
end
end
end
end
Gitlab::Checks::BaseSingleChecker.prepend_mod_with('Gitlab::Checks::BaseSingleChecker')
......@@ -2,7 +2,7 @@
module Gitlab
module Checks
class BranchCheck < BaseChecker
class BranchCheck < BaseSingleChecker
ERROR_MESSAGES = {
delete_default_branch: 'The default branch of a project cannot be deleted.',
force_push_protected_branch: 'You are not allowed to force push code to a protected branch on this project.',
......
# frozen_string_literal: true
module Gitlab
module Checks
class ChangesAccess
ATTRIBUTES = %i[user_access project protocol changes logger].freeze
attr_reader(*ATTRIBUTES)
def initialize(
changes, user_access:, project:, protocol:, logger:
)
@changes = changes
@user_access = user_access
@project = project
@protocol = protocol
@logger = logger
end
def validate!
return if changes.empty?
single_access_checks!
logger.log_timed("Running checks for #{changes.length} changes") do
bulk_access_checks!
end
true
end
protected
def single_access_checks!
# Iterate over all changes to find if user allowed all of them to be applied
changes.each do |change|
# If user does not have access to make at least one change, cancel all
# push by allowing the exception to bubble up
Checks::SingleChangeAccess.new(
change,
user_access: user_access,
project: project,
protocol: protocol,
logger: logger
).validate!
end
end
def bulk_access_checks!
Gitlab::Checks::LfsCheck.new(self).validate!
end
end
end
end
......@@ -2,7 +2,7 @@
module Gitlab
module Checks
class DiffCheck < BaseChecker
class DiffCheck < BaseSingleChecker
include Gitlab::Utils::StrongMemoize
LOG_MESSAGES = {
......
......@@ -2,7 +2,7 @@
module Gitlab
module Checks
class LfsCheck < BaseChecker
class LfsCheck < BaseBulkChecker
LOG_MESSAGE = 'Scanning repository for blobs stored in LFS and verifying their files have been uploaded to GitLab...'
ERROR_MESSAGE = 'LFS objects are missing. Ensure LFS is properly set up or try a manual "git lfs push --all".'
......@@ -12,11 +12,10 @@ module Gitlab
return unless Feature.enabled?(:lfs_check, default_enabled: true)
return unless project.lfs_enabled?
return if skip_lfs_integrity_check
return if deletion?
logger.log_timed(LOG_MESSAGE) do
lfs_check = Checks::LfsIntegrity.new(project, newrev, logger.time_left)
newrevs = changes.map { |change| change[:newrev] }
lfs_check = Checks::LfsIntegrity.new(project, newrevs, logger.time_left)
if lfs_check.objects_missing?
raise GitAccess::ForbiddenError, ERROR_MESSAGE
......
......@@ -3,16 +3,19 @@
module Gitlab
module Checks
class LfsIntegrity
def initialize(project, newrev, time_left)
def initialize(project, newrevs, time_left)
@project = project
@newrev = newrev
@newrevs = newrevs
@time_left = time_left
end
def objects_missing?
return false unless @newrev && @project.lfs_enabled?
return false unless @project.lfs_enabled?
new_lfs_pointers = Gitlab::Git::LfsChanges.new(@project.repository, @newrev)
newrevs = @newrevs.reject { |rev| rev.blank? || Gitlab::Git.blank_ref?(rev) }
return if newrevs.blank?
new_lfs_pointers = Gitlab::Git::LfsChanges.new(@project.repository, newrevs)
.new_pointers(object_limit: ::Gitlab::Git::Repository::REV_LIST_COMMIT_LIMIT, dynamic_timeout: @time_left)
return false unless new_lfs_pointers.present?
......
......@@ -2,7 +2,7 @@
module Gitlab
module Checks
class PushCheck < BaseChecker
class PushCheck < BaseSingleChecker
def validate!
logger.log_timed("Checking if you are allowed to push...") do
unless can_push?
......
......@@ -2,7 +2,7 @@
module Gitlab
module Checks
class PushFileCountCheck < BaseChecker
class PushFileCountCheck < BaseSingleChecker
attr_reader :repository, :newrev, :limit, :logger
LOG_MESSAGES = {
......
......@@ -2,23 +2,22 @@
module Gitlab
module Checks
class ChangeAccess
class SingleChangeAccess
ATTRIBUTES = %i[user_access project skip_authorization
skip_lfs_integrity_check protocol oldrev newrev ref
protocol oldrev newrev ref
branch_name tag_name logger commits].freeze
attr_reader(*ATTRIBUTES)
def initialize(
change, user_access:, project:,
skip_lfs_integrity_check: false, protocol:, logger:
protocol:, logger:
)
@oldrev, @newrev, @ref = change.values_at(:oldrev, :newrev, :ref)
@branch_name = Gitlab::Git.branch_name(@ref)
@tag_name = Gitlab::Git.tag_name(@ref)
@user_access = user_access
@project = project
@skip_lfs_integrity_check = skip_lfs_integrity_check
@protocol = protocol
@logger = logger
......@@ -44,7 +43,6 @@ module Gitlab
Gitlab::Checks::PushCheck.new(self).validate!
Gitlab::Checks::BranchCheck.new(self).validate!
Gitlab::Checks::TagCheck.new(self).validate!
Gitlab::Checks::LfsCheck.new(self).validate!
end
def commits_check
......@@ -54,4 +52,4 @@ module Gitlab
end
end
Gitlab::Checks::ChangeAccess.prepend_mod_with('Gitlab::Checks::ChangeAccess')
Gitlab::Checks::SingleChangeAccess.prepend_mod_with('Gitlab::Checks::SingleChangeAccess')
......@@ -2,7 +2,7 @@
module Gitlab
module Checks
class SnippetCheck < BaseChecker
class SnippetCheck < BaseSingleChecker
ERROR_MESSAGES = {
create_delete_branch: 'You can not create or delete branches.'
}.freeze
......
......@@ -2,7 +2,7 @@
module Gitlab
module Checks
class TagCheck < BaseChecker
class TagCheck < BaseSingleChecker
ERROR_MESSAGES = {
change_existing_tags: 'You are not allowed to change existing tags on this project.',
update_protected_tag: 'Protected tags cannot be updated.',
......
......@@ -3,13 +3,13 @@
module Gitlab
module Git
class LfsChanges
def initialize(repository, newrev = nil)
def initialize(repository, newrevs = nil)
@repository = repository
@newrev = newrev
@newrevs = newrevs
end
def new_pointers(object_limit: nil, not_in: nil, dynamic_timeout: nil)
@repository.gitaly_blob_client.get_new_lfs_pointers(@newrev, object_limit, not_in, dynamic_timeout)
@repository.gitaly_blob_client.get_new_lfs_pointers(@newrevs, object_limit, not_in, dynamic_timeout)
end
def all_pointers
......
......@@ -334,23 +334,15 @@ module Gitlab
# clear stale lock files.
project.repository.clean_stale_repository_files if project.present?
# Iterate over all changes to find if user allowed all of them to be applied
changes_list.each.with_index do |change, index|
first_change = index == 0
# If user does not have access to make at least one change, cancel all
# push by allowing the exception to bubble up
check_single_change_access(change, skip_lfs_integrity_check: !first_change)
end
check_access!
end
end
def check_single_change_access(change, skip_lfs_integrity_check: false)
Checks::ChangeAccess.new(
change,
def check_access!
Checks::ChangesAccess.new(
changes_list.changes,
user_access: user_access,
project: project,
skip_lfs_integrity_check: skip_lfs_integrity_check,
protocol: protocol,
logger: logger
).validate!
......
......@@ -109,20 +109,18 @@ module Gitlab
end
check_size_before_push!
check_access!
check_push_size!
end
override :check_access!
def check_access!
changes_list.each do |change|
# If user does not have access to make at least one change, cancel all
# push by allowing the exception to bubble up
check_single_change_access(change)
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!
end
check_push_size!
end
override :check_single_change_access
def check_single_change_access(change, _skip_lfs_integrity_check: false)
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!
rescue Checks::TimedLogger::TimeoutError
raise TimeoutError, logger.full_message
end
......
......@@ -77,8 +77,8 @@ module Gitlab
map_blob_types(response)
end
def get_new_lfs_pointers(revision, limit, not_in, dynamic_timeout = nil)
request, rpc = create_new_lfs_pointers_request(revision, limit, not_in)
def get_new_lfs_pointers(revisions, limit, not_in, dynamic_timeout = nil)
request, rpc = create_new_lfs_pointers_request(revisions, limit, not_in)
timeout =
if dynamic_timeout
......@@ -109,7 +109,7 @@ module Gitlab
private
def create_new_lfs_pointers_request(revision, limit, not_in)
def create_new_lfs_pointers_request(revisions, limit, not_in)
# If the check happens for a change which is using a quarantine
# environment for incoming objects, then we can avoid doing the
# necessary graph walk to detect only new LFS pointers and instead scan
......@@ -126,7 +126,7 @@ module Gitlab
[request, :list_all_lfs_pointers]
else
revisions = [revision]
revisions = Array.wrap(revisions)
revisions += if not_in.nil? || not_in == :all
["--not", "--all"]
else
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Gitlab::Checks::ChangesAccess do
describe '#validate!' do
include_context 'changes access checks context'
before do
allow(project).to receive(:lfs_enabled?).and_return(true)
end
subject { changes_access }
context 'without failed checks' do
it "doesn't raise an error" do
expect { subject.validate! }.not_to raise_error
end
it 'calls lfs checks' do
expect_next_instance_of(Gitlab::Checks::LfsCheck) do |instance|
expect(instance).to receive(:validate!)
end
subject.validate!
end
end
context 'when time limit was reached' do
it 'raises a TimeoutError' do
logger = Gitlab::Checks::TimedLogger.new(start_time: timeout.ago, timeout: timeout)
access = described_class.new(changes,
project: project,
user_access: user_access,
protocol: protocol,
logger: logger)
expect { access.validate! }.to raise_error(Gitlab::Checks::TimedLogger::TimeoutError)
end
end
end
end
......@@ -3,7 +3,7 @@
require 'spec_helper'
RSpec.describe Gitlab::Checks::LfsCheck do
include_context 'change access checks context'
include_context 'changes access checks context'
let(:blob_object) { project.repository.blob_at_branch('lfs', 'files/lfs/lfs_object.iso') }
......@@ -15,6 +15,10 @@ RSpec.describe Gitlab::Checks::LfsCheck do
describe '#validate!' do
context 'with LFS not enabled' do
before do
allow(project).to receive(:lfs_enabled?).and_return(false)
end
it 'skips integrity check' do
expect_any_instance_of(Gitlab::Git::LfsChanges).not_to receive(:new_pointers)
......@@ -51,13 +55,13 @@ RSpec.describe Gitlab::Checks::LfsCheck do
context 'with missing newrev' do
it_behaves_like 'a skipped integrity check' do
let(:changes) { { oldrev: oldrev, ref: ref } }
let(:changes) { [{ oldrev: oldrev, ref: ref }] }
end
end
context 'with blank newrev' do
it_behaves_like 'a skipped integrity check' do
let(:changes) { { oldrev: oldrev, newrev: Gitlab::Git::BLANK_SHA, ref: ref } }
let(:changes) { [{ oldrev: oldrev, newrev: Gitlab::Git::BLANK_SHA, ref: ref }] }
end
end
end
......
......@@ -18,12 +18,18 @@ RSpec.describe Gitlab::Checks::LfsIntegrity do
operations.commit_tree('8856a329dd38ca86dfb9ce5aa58a16d88cc119bd', "New LFS objects")
end
subject { described_class.new(project, newrev, time_left) }
let(:newrevs) { [newrev] }
subject { described_class.new(project, newrevs, time_left) }
describe '#objects_missing?' do
let(:blob_object) { repository.blob_at_branch('lfs', 'files/lfs/lfs_object.iso') }
context 'with LFS not enabled' do
before do
allow(project).to receive(:lfs_enabled?).and_return(false)
end
it 'skips integrity check' do
expect_any_instance_of(Gitlab::Git::LfsChanges).not_to receive(:new_pointers)
......@@ -36,8 +42,28 @@ RSpec.describe Gitlab::Checks::LfsIntegrity do
allow(project).to receive(:lfs_enabled?).and_return(true)
end
context 'nil rev' do
let(:newrevs) { [nil] }
it 'skips integrity check' do
expect_any_instance_of(Gitlab::Git::LfsChanges).not_to receive(:new_pointers)
expect(subject.objects_missing?).to be_falsey
end
end
context 'deletion' do
let(:newrev) { nil }
let(:newrevs) { [Gitlab::Git::BLANK_SHA] }
it 'skips integrity check' do
expect_any_instance_of(Gitlab::Git::LfsChanges).not_to receive(:new_pointers)
expect(subject.objects_missing?).to be_falsey
end
end
context 'no changes' do
let(:newrevs) { [] }
it 'skips integrity check' do
expect_any_instance_of(Gitlab::Git::LfsChanges).not_to receive(:new_pointers)
......
......@@ -2,7 +2,7 @@
require 'spec_helper'
RSpec.describe Gitlab::Checks::ChangeAccess do
RSpec.describe Gitlab::Checks::SingleChangeAccess do
describe '#validate!' do
include_context 'change access checks context'
......@@ -37,14 +37,6 @@ RSpec.describe Gitlab::Checks::ChangeAccess do
subject.validate!
end
it 'calls lfs checks' do
expect_next_instance_of(Gitlab::Checks::LfsCheck) do |instance|
expect(instance).to receive(:validate!)
end
subject.validate!
end
it 'calls diff checks' do
expect_next_instance_of(Gitlab::Checks::DiffCheck) do |instance|
expect(instance).to receive(:validate!)
......
......@@ -731,6 +731,8 @@ RSpec.describe Gitlab::GitAccess do
context 'when LFS is not enabled' do
it 'does not run LFSIntegrity check' do
allow(project).to receive(:lfs_enabled?).and_return(false)
expect(Gitlab::Checks::LfsIntegrity).not_to receive(:new)
push_access_check
......@@ -1004,10 +1006,10 @@ RSpec.describe Gitlab::GitAccess do
expect { access.check('git-receive-pack', changes) }.not_to exceed_query_limit(control_count).with_threshold(2)
end
it 'raises TimeoutError when #check_single_change_access raises a timeout error' do
it 'raises TimeoutError when #check_access! raises a timeout error' do
message = "Push operation timed out\n\nTiming information for debugging purposes:\nRunning checks for ref: wow"
expect_next_instance_of(Gitlab::Checks::ChangeAccess) do |check|
expect_next_instance_of(Gitlab::Checks::SingleChangeAccess) do |check|
expect(check).to receive(:validate!).and_raise(Gitlab::Checks::TimedLogger::TimeoutError)
end
......
# frozen_string_literal: true
RSpec.shared_context 'changes access checks context' do
let_it_be(:user) { create(:user) }
let_it_be(:project) { create(:project, :repository) }
let(:user_access) { Gitlab::UserAccess.new(user, container: project) }
let(:protocol) { 'ssh' }
let(:timeout) { Gitlab::GitAccess::INTERNAL_TIMEOUT }
let(:oldrev) { 'be93687618e4b132087f430a4d8fc3a609c9b77c' }
let(:newrev) { '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51' }
let(:ref) { 'refs/heads/master' }
let(:changes) do
[
# Update of existing branch
{ oldrev: oldrev, newrev: newrev, ref: ref },
# Creation of new branch
{ newrev: newrev, ref: 'refs/heads/something' },
# Deletion of branch
{ oldrev: oldrev, ref: 'refs/heads/deleteme' }
]
end
let(:logger) { Gitlab::Checks::TimedLogger.new(timeout: timeout) }
let(:changes_access) do
Gitlab::Checks::ChangesAccess.new(
changes,
project: project,
user_access: user_access,
protocol: protocol,
logger: logger
)
end
subject { described_class.new(changes_access) }
before do
project.add_developer(user)
end
end
......@@ -12,7 +12,7 @@ RSpec.shared_context 'change access checks context' do
let(:timeout) { Gitlab::GitAccess::INTERNAL_TIMEOUT }
let(:logger) { Gitlab::Checks::TimedLogger.new(timeout: timeout) }
let(:change_access) do
Gitlab::Checks::ChangeAccess.new(
Gitlab::Checks::SingleChangeAccess.new(
changes,
project: project,
user_access: user_access,
......
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