Commit f5201a81 authored by Sean McGivern's avatar Sean McGivern

Merge branch 'revert-8baf9e5f' into 'master'

Revert "Merge branch '13784-simple-masking-of-protected-variables-in-logs' into 'master'"

See merge request gitlab-org/gitlab-ce!25566
parents 48e6db0d 7b445f9b
...@@ -5,7 +5,6 @@ module Ci ...@@ -5,7 +5,6 @@ module Ci
extend Gitlab::Ci::Model extend Gitlab::Ci::Model
include HasVariable include HasVariable
include Presentable include Presentable
include Maskable
belongs_to :group, class_name: "::Group" belongs_to :group, class_name: "::Group"
......
...@@ -5,7 +5,6 @@ module Ci ...@@ -5,7 +5,6 @@ module Ci
extend Gitlab::Ci::Model extend Gitlab::Ci::Model
include HasVariable include HasVariable
include Presentable include Presentable
include Maskable
belongs_to :project belongs_to :project
......
...@@ -21,9 +21,9 @@ module HasVariable ...@@ -21,9 +21,9 @@ module HasVariable
def key=(new_key) def key=(new_key)
super(new_key.to_s.strip) super(new_key.to_s.strip)
end end
end
def to_runner_variable def to_runner_variable
{ key: key, value: value, public: false } { key: key, value: value, public: false }
end
end end
end end
# frozen_string_literal: true
module Maskable
extend ActiveSupport::Concern
# * Single line
# * No escape characters
# * No variables
# * No spaces
# * Minimal length of 8 characters
# * Absolutely no fun is allowed
REGEX = /\A\w{8,}\z/
included do
validates :masked, inclusion: { in: [true, false] }
validates :value, format: { with: REGEX }, if: :masked?
end
def to_runner_variable
super.merge(masked: masked?)
end
end
---
title: Add support for masking CI variables.
merge_request: 25293
author:
type: added
# frozen_string_literal: true
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class AddMaskedToCiVariables < ActiveRecord::Migration[5.0]
include Gitlab::Database::MigrationHelpers
# Set this constant to true if this migration requires downtime.
DOWNTIME = false
disable_ddl_transaction!
def up
add_column_with_default :ci_variables, :masked, :boolean, default: false, allow_null: false
end
def down
remove_column :ci_variables, :masked
end
end
# frozen_string_literal: true
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class AddMaskedToCiGroupVariables < ActiveRecord::Migration[5.0]
include Gitlab::Database::MigrationHelpers
# Set this constant to true if this migration requires downtime.
DOWNTIME = false
disable_ddl_transaction!
def up
add_column_with_default :ci_group_variables, :masked, :boolean, default: false, allow_null: false
end
def down
remove_column :ci_group_variables, :masked
end
end
...@@ -10,7 +10,7 @@ ...@@ -10,7 +10,7 @@
# #
# It's strongly recommended that you check this file into your version control system. # It's strongly recommended that you check this file into your version control system.
ActiveRecord::Schema.define(version: 20190218134209) do ActiveRecord::Schema.define(version: 20190204115450) do
# These are extensions that must be enabled in order to support this database # These are extensions that must be enabled in order to support this database
enable_extension "plpgsql" enable_extension "plpgsql"
...@@ -405,7 +405,6 @@ ActiveRecord::Schema.define(version: 20190218134209) do ...@@ -405,7 +405,6 @@ ActiveRecord::Schema.define(version: 20190218134209) do
t.boolean "protected", default: false, null: false t.boolean "protected", default: false, null: false
t.datetime_with_timezone "created_at", null: false t.datetime_with_timezone "created_at", null: false
t.datetime_with_timezone "updated_at", null: false t.datetime_with_timezone "updated_at", null: false
t.boolean "masked", default: false, null: false
t.index ["group_id", "key"], name: "index_ci_group_variables_on_group_id_and_key", unique: true, using: :btree t.index ["group_id", "key"], name: "index_ci_group_variables_on_group_id_and_key", unique: true, using: :btree
end end
...@@ -601,7 +600,6 @@ ActiveRecord::Schema.define(version: 20190218134209) do ...@@ -601,7 +600,6 @@ ActiveRecord::Schema.define(version: 20190218134209) do
t.integer "project_id", null: false t.integer "project_id", null: false
t.boolean "protected", default: false, null: false t.boolean "protected", default: false, null: false
t.string "environment_scope", default: "*", null: false t.string "environment_scope", default: "*", null: false
t.boolean "masked", default: false, null: false
t.index ["project_id", "key", "environment_scope"], name: "index_ci_variables_on_project_id_and_key_and_environment_scope", unique: true, using: :btree t.index ["project_id", "key", "environment_scope"], name: "index_ci_variables_on_project_id_and_key_and_environment_scope", unique: true, using: :btree
end end
......
...@@ -5,12 +5,12 @@ module Gitlab ...@@ -5,12 +5,12 @@ module Gitlab
module Variables module Variables
class Collection class Collection
class Item class Item
def initialize(key:, value:, public: true, file: false, masked: false) def initialize(key:, value:, public: true, file: false)
raise ArgumentError, "`#{key}` must be of type String or nil value, while it was: #{value.class}" unless raise ArgumentError, "`#{key}` must be of type String or nil value, while it was: #{value.class}" unless
value.is_a?(String) || value.nil? value.is_a?(String) || value.nil?
@variable = { @variable = {
key: key, value: value, public: public, file: file, masked: masked key: key, value: value, public: public, file: file
} }
end end
...@@ -27,13 +27,9 @@ module Gitlab ...@@ -27,13 +27,9 @@ module Gitlab
# don't expose `file` attribute at all (stems from what the runner # don't expose `file` attribute at all (stems from what the runner
# expects). # expects).
# #
# If the `variable_masking` feature is enabled we expose the `masked`
# attribute, otherwise it's not exposed.
#
def to_runner_variable def to_runner_variable
@variable.reject do |hash_key, hash_value| @variable.reject do |hash_key, hash_value|
(hash_key == :file && hash_value == false) || hash_key == :file && hash_value == false
(hash_key == :masked && !Feature.enabled?(:variable_masking))
end end
end end
......
...@@ -2,7 +2,6 @@ FactoryBot.define do ...@@ -2,7 +2,6 @@ FactoryBot.define do
factory :ci_group_variable, class: Ci::GroupVariable do factory :ci_group_variable, class: Ci::GroupVariable do
sequence(:key) { |n| "VARIABLE_#{n}" } sequence(:key) { |n| "VARIABLE_#{n}" }
value 'VARIABLE_VALUE' value 'VARIABLE_VALUE'
masked false
trait(:protected) do trait(:protected) do
protected true protected true
......
...@@ -2,7 +2,6 @@ FactoryBot.define do ...@@ -2,7 +2,6 @@ FactoryBot.define do
factory :ci_variable, class: Ci::Variable do factory :ci_variable, class: Ci::Variable do
sequence(:key) { |n| "VARIABLE_#{n}" } sequence(:key) { |n| "VARIABLE_#{n}" }
value 'VARIABLE_VALUE' value 'VARIABLE_VALUE'
masked false
trait(:protected) do trait(:protected) do
protected true protected true
......
...@@ -3,7 +3,7 @@ require 'spec_helper' ...@@ -3,7 +3,7 @@ require 'spec_helper'
describe 'Group variables', :js do describe 'Group variables', :js do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:group) { create(:group) } let(:group) { create(:group) }
let!(:variable) { create(:ci_group_variable, key: 'test_key', value: 'test_value', group: group) } let!(:variable) { create(:ci_group_variable, key: 'test_key', value: 'test value', group: group) }
let(:page_path) { group_settings_ci_cd_path(group) } let(:page_path) { group_settings_ci_cd_path(group) }
before do before do
......
...@@ -3,7 +3,7 @@ require 'spec_helper' ...@@ -3,7 +3,7 @@ require 'spec_helper'
describe 'Project variables', :js do describe 'Project variables', :js do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:project) { create(:project) } let(:project) { create(:project) }
let(:variable) { create(:ci_variable, key: 'test_key', value: 'test_value') } let(:variable) { create(:ci_variable, key: 'test_key', value: 'test value') }
let(:page_path) { project_settings_ci_cd_path(project) } let(:page_path) { project_settings_ci_cd_path(project) }
before do before do
......
...@@ -6,7 +6,7 @@ describe Gitlab::Ci::Variables::Collection::Item do ...@@ -6,7 +6,7 @@ describe Gitlab::Ci::Variables::Collection::Item do
let(:expected_value) { variable_value } let(:expected_value) { variable_value }
let(:variable) do let(:variable) do
{ key: variable_key, value: variable_value, public: true, masked: false } { key: variable_key, value: variable_value, public: true }
end end
describe '.new' do describe '.new' do
...@@ -88,7 +88,7 @@ describe Gitlab::Ci::Variables::Collection::Item do ...@@ -88,7 +88,7 @@ describe Gitlab::Ci::Variables::Collection::Item do
resource = described_class.fabricate(variable) resource = described_class.fabricate(variable)
expect(resource).to be_a(described_class) expect(resource).to be_a(described_class)
expect(resource).to eq(key: 'CI_VAR', value: '123', public: false, masked: false) expect(resource).to eq(key: 'CI_VAR', value: '123', public: false)
end end
it 'supports using another collection item' do it 'supports using another collection item' do
...@@ -134,21 +134,7 @@ describe Gitlab::Ci::Variables::Collection::Item do ...@@ -134,21 +134,7 @@ describe Gitlab::Ci::Variables::Collection::Item do
.to_runner_variable .to_runner_variable
expect(runner_variable) expect(runner_variable)
.to eq(key: 'VAR', value: 'value', public: true, file: true, masked: false) .to eq(key: 'VAR', value: 'value', public: true, file: true)
end
end
context 'when variable masking is disabled' do
before do
stub_feature_flags(variable_masking: false)
end
it 'does not expose the masked field to the runner' do
runner_variable = described_class
.new(key: 'VAR', value: 'value', masked: true)
.to_runner_variable
expect(runner_variable).to eq(key: 'VAR', value: 'value', public: true)
end end
end end
end end
......
...@@ -3,7 +3,7 @@ require 'spec_helper' ...@@ -3,7 +3,7 @@ require 'spec_helper'
describe Gitlab::Ci::Variables::Collection do describe Gitlab::Ci::Variables::Collection do
describe '.new' do describe '.new' do
it 'can be initialized with an array' do it 'can be initialized with an array' do
variable = { key: 'VAR', value: 'value', public: true, masked: false } variable = { key: 'VAR', value: 'value', public: true }
collection = described_class.new([variable]) collection = described_class.new([variable])
...@@ -93,7 +93,7 @@ describe Gitlab::Ci::Variables::Collection do ...@@ -93,7 +93,7 @@ describe Gitlab::Ci::Variables::Collection do
collection = described_class.new([{ key: 'TEST', value: '1' }]) collection = described_class.new([{ key: 'TEST', value: '1' }])
expect(collection.to_runner_variables) expect(collection.to_runner_variables)
.to eq [{ key: 'TEST', value: '1', public: true, masked: false }] .to eq [{ key: 'TEST', value: '1', public: true }]
end end
end end
......
This diff is collapsed.
...@@ -5,7 +5,6 @@ describe Ci::GroupVariable do ...@@ -5,7 +5,6 @@ describe Ci::GroupVariable do
it { is_expected.to include_module(HasVariable) } it { is_expected.to include_module(HasVariable) }
it { is_expected.to include_module(Presentable) } it { is_expected.to include_module(Presentable) }
it { is_expected.to include_module(Maskable) }
it { is_expected.to validate_uniqueness_of(:key).scoped_to(:group_id).with_message(/\(\w+\) has already been taken/) } it { is_expected.to validate_uniqueness_of(:key).scoped_to(:group_id).with_message(/\(\w+\) has already been taken/) }
describe '.unprotected' do describe '.unprotected' do
......
...@@ -6,7 +6,6 @@ describe Ci::Variable do ...@@ -6,7 +6,6 @@ describe Ci::Variable do
describe 'validations' do describe 'validations' do
it { is_expected.to include_module(HasVariable) } it { is_expected.to include_module(HasVariable) }
it { is_expected.to include_module(Presentable) } it { is_expected.to include_module(Presentable) }
it { is_expected.to include_module(Maskable) }
it { is_expected.to validate_uniqueness_of(:key).scoped_to(:project_id, :environment_scope).with_message(/\(\w+\) has already been taken/) } it { is_expected.to validate_uniqueness_of(:key).scoped_to(:project_id, :environment_scope).with_message(/\(\w+\) has already been taken/) }
end end
......
...@@ -57,7 +57,7 @@ describe HasVariable do ...@@ -57,7 +57,7 @@ describe HasVariable do
describe '#to_runner_variable' do describe '#to_runner_variable' do
it 'returns a hash for the runner' do it 'returns a hash for the runner' do
expect(subject.to_runner_variable) expect(subject.to_runner_variable)
.to include(key: subject.key, value: subject.value, public: false) .to eq(key: subject.key, value: subject.value, public: false)
end end
end end
end end
# frozen_string_literal: true
require 'spec_helper'
describe Maskable do
let(:variable) { build(:ci_variable) }
describe 'masked value validations' do
subject { variable }
context 'when variable is masked' do
before do
subject.masked = true
end
it { is_expected.not_to allow_value('hello').for(:value) }
it { is_expected.not_to allow_value('hello world').for(:value) }
it { is_expected.not_to allow_value('hello$VARIABLEworld').for(:value) }
it { is_expected.not_to allow_value('hello\rworld').for(:value) }
it { is_expected.to allow_value('helloworld').for(:value) }
end
context 'when variable is not masked' do
before do
subject.masked = false
end
it { is_expected.to allow_value('hello').for(:value) }
it { is_expected.to allow_value('hello world').for(:value) }
it { is_expected.to allow_value('hello$VARIABLEworld').for(:value) }
it { is_expected.to allow_value('hello\rworld').for(:value) }
it { is_expected.to allow_value('helloworld').for(:value) }
end
end
describe 'REGEX' do
subject { Maskable::REGEX }
it 'does not match strings shorter than 8 letters' do
expect(subject.match?('hello')).to eq(false)
end
it 'does not match strings with spaces' do
expect(subject.match?('hello world')).to eq(false)
end
it 'does not match strings with shell variables' do
expect(subject.match?('hello$VARIABLEworld')).to eq(false)
end
it 'does not match strings with escape characters' do
expect(subject.match?('hello\rworld')).to eq(false)
end
it 'does not match strings that span more than one line' do
string = <<~EOS
hello
world
EOS
expect(subject.match?(string)).to eq(false)
end
it 'matches valid strings' do
expect(subject.match?('helloworld')).to eq(true)
end
end
describe '#to_runner_variable' do
subject { variable.to_runner_variable }
it 'exposes the masked attribute' do
expect(subject).to include(:masked)
end
end
end
...@@ -87,12 +87,12 @@ describe API::GroupVariables do ...@@ -87,12 +87,12 @@ describe API::GroupVariables do
it 'creates variable' do it 'creates variable' do
expect do expect do
post api("/groups/#{group.id}/variables", user), params: { key: 'TEST_VARIABLE_2', value: 'PROTECTED_VALUE_2', protected: true } post api("/groups/#{group.id}/variables", user), params: { key: 'TEST_VARIABLE_2', value: 'VALUE_2', protected: true }
end.to change {group.variables.count}.by(1) end.to change {group.variables.count}.by(1)
expect(response).to have_gitlab_http_status(201) expect(response).to have_gitlab_http_status(201)
expect(json_response['key']).to eq('TEST_VARIABLE_2') expect(json_response['key']).to eq('TEST_VARIABLE_2')
expect(json_response['value']).to eq('PROTECTED_VALUE_2') expect(json_response['value']).to eq('VALUE_2')
expect(json_response['protected']).to be_truthy expect(json_response['protected']).to be_truthy
end end
......
...@@ -436,9 +436,9 @@ describe API::Runner, :clean_gitlab_redis_shared_state do ...@@ -436,9 +436,9 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
end end
let(:expected_variables) do let(:expected_variables) do
[{ 'key' => 'CI_JOB_NAME', 'value' => 'spinach', 'public' => true, 'masked' => false }, [{ 'key' => 'CI_JOB_NAME', 'value' => 'spinach', 'public' => true },
{ 'key' => 'CI_JOB_STAGE', 'value' => 'test', 'public' => true, 'masked' => false }, { 'key' => 'CI_JOB_STAGE', 'value' => 'test', 'public' => true },
{ 'key' => 'DB_NAME', 'value' => 'postgres', 'public' => true, 'masked' => false }] { 'key' => 'DB_NAME', 'value' => 'postgres', 'public' => true }]
end end
let(:expected_artifacts) do let(:expected_artifacts) do
...@@ -740,12 +740,12 @@ describe API::Runner, :clean_gitlab_redis_shared_state do ...@@ -740,12 +740,12 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
context 'when triggered job is available' do context 'when triggered job is available' do
let(:expected_variables) do let(:expected_variables) do
[{ 'key' => 'CI_JOB_NAME', 'value' => 'spinach', 'public' => true, 'masked' => false }, [{ 'key' => 'CI_JOB_NAME', 'value' => 'spinach', 'public' => true },
{ 'key' => 'CI_JOB_STAGE', 'value' => 'test', 'public' => true, 'masked' => false }, { 'key' => 'CI_JOB_STAGE', 'value' => 'test', 'public' => true },
{ 'key' => 'CI_PIPELINE_TRIGGERED', 'value' => 'true', 'public' => true, 'masked' => false }, { 'key' => 'CI_PIPELINE_TRIGGERED', 'value' => 'true', 'public' => true },
{ 'key' => 'DB_NAME', 'value' => 'postgres', 'public' => true, 'masked' => false }, { 'key' => 'DB_NAME', 'value' => 'postgres', 'public' => true },
{ 'key' => 'SECRET_KEY', 'value' => 'secret_value', 'public' => false, 'masked' => false }, { 'key' => 'SECRET_KEY', 'value' => 'secret_value', 'public' => false },
{ 'key' => 'TRIGGER_KEY_1', 'value' => 'TRIGGER_VALUE_1', 'public' => false, 'masked' => false }] { 'key' => 'TRIGGER_KEY_1', 'value' => 'TRIGGER_VALUE_1', 'public' => false }]
end end
let(:trigger) { create(:ci_trigger, project: project) } let(:trigger) { create(:ci_trigger, project: project) }
......
...@@ -73,12 +73,12 @@ describe API::Variables do ...@@ -73,12 +73,12 @@ describe API::Variables do
context 'authorized user with proper permissions' do context 'authorized user with proper permissions' do
it 'creates variable' do it 'creates variable' do
expect do expect do
post api("/projects/#{project.id}/variables", user), params: { key: 'TEST_VARIABLE_2', value: 'PROTECTED_VALUE_2', protected: true } post api("/projects/#{project.id}/variables", user), params: { key: 'TEST_VARIABLE_2', value: 'VALUE_2', protected: true }
end.to change {project.variables.count}.by(1) end.to change {project.variables.count}.by(1)
expect(response).to have_gitlab_http_status(201) expect(response).to have_gitlab_http_status(201)
expect(json_response['key']).to eq('TEST_VARIABLE_2') expect(json_response['key']).to eq('TEST_VARIABLE_2')
expect(json_response['value']).to eq('PROTECTED_VALUE_2') expect(json_response['value']).to eq('VALUE_2')
expect(json_response['protected']).to be_truthy expect(json_response['protected']).to be_truthy
end end
......
...@@ -8,7 +8,7 @@ shared_examples 'variable list' do ...@@ -8,7 +8,7 @@ shared_examples 'variable list' do
it 'adds new CI variable' do it 'adds new CI variable' do
page.within('.js-ci-variable-list-section .js-row:last-child') do page.within('.js-ci-variable-list-section .js-row:last-child') do
find('.js-ci-variable-input-key').set('key') find('.js-ci-variable-input-key').set('key')
find('.js-ci-variable-input-value').set('key_value') find('.js-ci-variable-input-value').set('key value')
end end
click_button('Save variables') click_button('Save variables')
...@@ -19,7 +19,7 @@ shared_examples 'variable list' do ...@@ -19,7 +19,7 @@ shared_examples 'variable list' do
# We check the first row because it re-sorts to alphabetical order on refresh # We check the first row because it re-sorts to alphabetical order on refresh
page.within('.js-ci-variable-list-section .js-row:nth-child(1)') do page.within('.js-ci-variable-list-section .js-row:nth-child(1)') do
expect(find('.js-ci-variable-input-key').value).to eq('key') expect(find('.js-ci-variable-input-key').value).to eq('key')
expect(find('.js-ci-variable-input-value', visible: false).value).to eq('key_value') expect(find('.js-ci-variable-input-value', visible: false).value).to eq('key value')
end end
end end
...@@ -44,7 +44,7 @@ shared_examples 'variable list' do ...@@ -44,7 +44,7 @@ shared_examples 'variable list' do
it 'adds new protected variable' do it 'adds new protected variable' do
page.within('.js-ci-variable-list-section .js-row:last-child') do page.within('.js-ci-variable-list-section .js-row:last-child') do
find('.js-ci-variable-input-key').set('key') find('.js-ci-variable-input-key').set('key')
find('.js-ci-variable-input-value').set('key_value') find('.js-ci-variable-input-value').set('key value')
find('.ci-variable-protected-item .js-project-feature-toggle').click find('.ci-variable-protected-item .js-project-feature-toggle').click
expect(find('.js-ci-variable-input-protected', visible: false).value).to eq('true') expect(find('.js-ci-variable-input-protected', visible: false).value).to eq('true')
...@@ -58,7 +58,7 @@ shared_examples 'variable list' do ...@@ -58,7 +58,7 @@ shared_examples 'variable list' do
# We check the first row because it re-sorts to alphabetical order on refresh # We check the first row because it re-sorts to alphabetical order on refresh
page.within('.js-ci-variable-list-section .js-row:nth-child(1)') do page.within('.js-ci-variable-list-section .js-row:nth-child(1)') do
expect(find('.js-ci-variable-input-key').value).to eq('key') expect(find('.js-ci-variable-input-key').value).to eq('key')
expect(find('.js-ci-variable-input-value', visible: false).value).to eq('key_value') expect(find('.js-ci-variable-input-value', visible: false).value).to eq('key value')
expect(find('.js-ci-variable-input-protected', visible: false).value).to eq('true') expect(find('.js-ci-variable-input-protected', visible: false).value).to eq('true')
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