Commit 0b01f700 authored by Timothy Andrew's avatar Timothy Andrew

Implement review comments from @dbalexandre.

- Don't define "allowed environment variables" in two places.
- Dispatch to different arities of `Popen.open` without an if/else block.
- Use `described_class` instead of explicitly stating the class name within a
- spec.
- Remove `git_environment_variables_validator_spec` and keep the validation inline.
parent f6ad0a94
class GitEnvironmentVariablesValidator < ActiveModel::EachValidator
def validate_each(record, attribute, env)
variables_to_validate = %w(GIT_OBJECT_DIRECTORY GIT_ALTERNATE_OBJECT_DIRECTORIES)
variables_to_validate.each do |variable_name|
variable_value = env[variable_name]
if variable_value.present? && !(variable_value =~ /^#{record.project.repository.path_to_repo}/)
record.errors.add(attribute, "The #{variable_name} variable must start with the project repo path")
end
end
end
end
......@@ -3,12 +3,10 @@
module Gitlab
module Git
class RevList
include ActiveModel::Validations
validates :env, git_environment_variables: true
attr_reader :project, :env
ALLOWED_VARIABLES = %w(GIT_OBJECT_DIRECTORY GIT_ALTERNATE_OBJECT_DIRECTORIES).freeze
def initialize(oldrev, newrev, project:, env: nil)
@project = project
@env = env.presence || {}
......@@ -21,17 +19,21 @@ module Gitlab
end
def execute
if self.valid?
Gitlab::Popen.popen(@args, nil, @env.slice(*allowed_environment_variables))
else
Gitlab::Popen.popen(@args)
Gitlab::Popen.popen(@args, nil, parse_environment_variables)
end
def valid?
env.slice(*ALLOWED_VARIABLES).all? do |(name, value)|
value =~ /^#{project.repository.path_to_repo}/
end
end
private
def allowed_environment_variables
%w(GIT_ALTERNATE_OBJECT_DIRECTORIES GIT_OBJECT_DIRECTORY)
def parse_environment_variables
return {} unless valid?
env.slice(*ALLOWED_VARIABLES)
end
end
end
......
require 'spec_helper'
require 'validators/git_environment_variables_validator_spec'
describe Gitlab::Git::RevList, lib: true do
let(:project) { create(:project) }
context "validations" do
it_behaves_like(
"validated git environment variables",
->(env, project) { Gitlab::Git::RevList.new('oldrev', 'newrev', project: project, env: env) }
)
context "GIT_OBJECT_DIRECTORY" do
it "accepts values starting with the project repo path" do
env = { "GIT_OBJECT_DIRECTORY" => "#{project.repository.path_to_repo}/objects" }
rev_list = described_class.new('oldrev', 'newrev', project: project, env: env)
expect(rev_list).to be_valid
end
it "rejects values starting not with the project repo path" do
env = { "GIT_OBJECT_DIRECTORY" => "/some/other/path" }
rev_list = described_class.new('oldrev', 'newrev', project: project, env: env)
expect(rev_list).not_to be_valid
end
it "rejects values containing the project repo path but not starting with it" do
env = { "GIT_OBJECT_DIRECTORY" => "/some/other/path/#{project.repository.path_to_repo}" }
rev_list = described_class.new('oldrev', 'newrev', project: project, env: env)
expect(rev_list).not_to be_valid
end
end
context "GIT_ALTERNATE_OBJECT_DIRECTORIES" do
it "accepts values starting with the project repo path" do
env = { "GIT_ALTERNATE_OBJECT_DIRECTORIES" => project.repository.path_to_repo }
rev_list = described_class.new('oldrev', 'newrev', project: project, env: env)
expect(rev_list).to be_valid
end
it "rejects values starting not with the project repo path" do
env = { "GIT_ALTERNATE_OBJECT_DIRECTORIES" => "/some/other/path" }
rev_list = described_class.new('oldrev', 'newrev', project: project, env: env)
expect(rev_list).not_to be_valid
end
it "rejects values containing the project repo path but not starting with it" do
env = { "GIT_ALTERNATE_OBJECT_DIRECTORIES" => "/some/other/path/#{project.repository.path_to_repo}" }
rev_list = described_class.new('oldrev', 'newrev', project: project, env: env)
expect(rev_list).not_to be_valid
end
end
end
context "#execute" do
......
require 'spec_helper'
shared_examples_for "validated git environment variables" do |record_fn|
subject { GitEnvironmentVariablesValidator.new(attributes: ['env']) }
let(:project) { create(:project) }
context "GIT_OBJECT_DIRECTORY" do
it "accepts values starting with the project repo path" do
env = { "GIT_OBJECT_DIRECTORY" => "#{project.repository.path_to_repo}/objects" }
record = record_fn[env, project]
subject.validate_each(record, 'env', env)
expect(record).to be_valid, "expected #{project.repository.path_to_repo}"
end
it "rejects values starting not with the project repo path" do
env = { "GIT_OBJECT_DIRECTORY" => "/some/other/path" }
record = record_fn[env, project]
subject.validate_each(record, 'env', env)
expect(record).to be_invalid
end
it "rejects values containing the project repo path but not starting with it" do
env = { "GIT_OBJECT_DIRECTORY" => "/some/other/path/#{project.repository.path_to_repo}" }
record = record_fn[env, project]
subject.validate_each(record, 'env', env)
expect(record).to be_invalid
end
end
context "GIT_ALTERNATE_OBJECT_DIRECTORIES" do
it "accepts values starting with the project repo path" do
env = { "GIT_ALTERNATE_OBJECT_DIRECTORIES" => project.repository.path_to_repo }
record = record_fn[env, project]
subject.validate_each(record, 'env', env)
expect(record).to be_valid, "expected #{project.repository.path_to_repo}"
end
it "rejects values starting not with the project repo path" do
env = { "GIT_ALTERNATE_OBJECT_DIRECTORIES" => "/some/other/path" }
record = record_fn[env, project]
subject.validate_each(record, 'env', env)
expect(record).to be_invalid
end
it "rejects values containing the project repo path but not starting with it" do
env = { "GIT_ALTERNATE_OBJECT_DIRECTORIES" => "/some/other/path/#{project.repository.path_to_repo}" }
record = record_fn[env, project]
subject.validate_each(record, 'env', env)
expect(record).to be_invalid
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