diff --git a/app/models/ci/group_variable.rb b/app/models/ci/group_variable.rb index 323ff560564..492d1d0329e 100644 --- a/app/models/ci/group_variable.rb +++ b/app/models/ci/group_variable.rb @@ -5,7 +5,6 @@ module Ci extend Gitlab::Ci::Model include HasVariable include Presentable - include Maskable belongs_to :group, class_name: "::Group" diff --git a/app/models/ci/variable.rb b/app/models/ci/variable.rb index 64836ea4fa4..524d79014f8 100644 --- a/app/models/ci/variable.rb +++ b/app/models/ci/variable.rb @@ -5,7 +5,6 @@ module Ci extend Gitlab::Ci::Model include HasVariable include Presentable - include Maskable belongs_to :project diff --git a/app/models/concerns/has_variable.rb b/app/models/concerns/has_variable.rb index 2ec42a1029b..dfbe413a878 100644 --- a/app/models/concerns/has_variable.rb +++ b/app/models/concerns/has_variable.rb @@ -21,9 +21,9 @@ module HasVariable def key=(new_key) super(new_key.to_s.strip) end - end - def to_runner_variable - { key: key, value: value, public: false } + def to_runner_variable + { key: key, value: value, public: false } + end end end diff --git a/app/models/concerns/maskable.rb b/app/models/concerns/maskable.rb deleted file mode 100644 index 8793f0ec965..00000000000 --- a/app/models/concerns/maskable.rb +++ /dev/null @@ -1,22 +0,0 @@ -# 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 diff --git a/changelogs/unreleased/13784-simple-masking-of-protected-variables-in-logs.yml b/changelogs/unreleased/13784-simple-masking-of-protected-variables-in-logs.yml deleted file mode 100644 index 5c3b6833235..00000000000 --- a/changelogs/unreleased/13784-simple-masking-of-protected-variables-in-logs.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Add support for masking CI variables. -merge_request: 25293 -author: -type: added diff --git a/db/migrate/20190218134158_add_masked_to_ci_variables.rb b/db/migrate/20190218134158_add_masked_to_ci_variables.rb deleted file mode 100644 index b4999d5b4a9..00000000000 --- a/db/migrate/20190218134158_add_masked_to_ci_variables.rb +++ /dev/null @@ -1,21 +0,0 @@ -# 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 diff --git a/db/migrate/20190218134209_add_masked_to_ci_group_variables.rb b/db/migrate/20190218134209_add_masked_to_ci_group_variables.rb deleted file mode 100644 index 8633875b341..00000000000 --- a/db/migrate/20190218134209_add_masked_to_ci_group_variables.rb +++ /dev/null @@ -1,21 +0,0 @@ -# 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 diff --git a/db/schema.rb b/db/schema.rb index f7bf6fb9cf7..1bdc9682cc9 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # 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 enable_extension "plpgsql" @@ -405,7 +405,6 @@ ActiveRecord::Schema.define(version: 20190218134209) do t.boolean "protected", default: false, null: false t.datetime_with_timezone "created_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 end @@ -601,7 +600,6 @@ ActiveRecord::Schema.define(version: 20190218134209) do t.integer "project_id", null: false t.boolean "protected", default: false, 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 end diff --git a/lib/gitlab/ci/variables/collection/item.rb b/lib/gitlab/ci/variables/collection/item.rb index 833aa75adb5..e3e4e62cc02 100644 --- a/lib/gitlab/ci/variables/collection/item.rb +++ b/lib/gitlab/ci/variables/collection/item.rb @@ -5,12 +5,12 @@ module Gitlab module Variables class Collection 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 value.is_a?(String) || value.nil? @variable = { - key: key, value: value, public: public, file: file, masked: masked + key: key, value: value, public: public, file: file } end @@ -27,13 +27,9 @@ module Gitlab # don't expose `file` attribute at all (stems from what the runner # expects). # - # If the `variable_masking` feature is enabled we expose the `masked` - # attribute, otherwise it's not exposed. - # def to_runner_variable @variable.reject do |hash_key, hash_value| - (hash_key == :file && hash_value == false) || - (hash_key == :masked && !Feature.enabled?(:variable_masking)) + hash_key == :file && hash_value == false end end diff --git a/spec/factories/ci/group_variables.rb b/spec/factories/ci/group_variables.rb index 9bf520a2c0a..64716842b12 100644 --- a/spec/factories/ci/group_variables.rb +++ b/spec/factories/ci/group_variables.rb @@ -2,7 +2,6 @@ FactoryBot.define do factory :ci_group_variable, class: Ci::GroupVariable do sequence(:key) { |n| "VARIABLE_#{n}" } value 'VARIABLE_VALUE' - masked false trait(:protected) do protected true diff --git a/spec/factories/ci/variables.rb b/spec/factories/ci/variables.rb index 97a7c9ba252..3d014b9b54f 100644 --- a/spec/factories/ci/variables.rb +++ b/spec/factories/ci/variables.rb @@ -2,7 +2,6 @@ FactoryBot.define do factory :ci_variable, class: Ci::Variable do sequence(:key) { |n| "VARIABLE_#{n}" } value 'VARIABLE_VALUE' - masked false trait(:protected) do protected true diff --git a/spec/features/group_variables_spec.rb b/spec/features/group_variables_spec.rb index 1a53e7c9512..57e3ddfb39c 100644 --- a/spec/features/group_variables_spec.rb +++ b/spec/features/group_variables_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe 'Group variables', :js do let(:user) { create(:user) } 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) } before do diff --git a/spec/features/project_variables_spec.rb b/spec/features/project_variables_spec.rb index 6bdf5df1036..a93df3696d2 100644 --- a/spec/features/project_variables_spec.rb +++ b/spec/features/project_variables_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe 'Project variables', :js do let(:user) { create(:user) } 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) } before do diff --git a/spec/lib/gitlab/ci/variables/collection/item_spec.rb b/spec/lib/gitlab/ci/variables/collection/item_spec.rb index 3ff2fe18c15..8bf44acb228 100644 --- a/spec/lib/gitlab/ci/variables/collection/item_spec.rb +++ b/spec/lib/gitlab/ci/variables/collection/item_spec.rb @@ -6,7 +6,7 @@ describe Gitlab::Ci::Variables::Collection::Item do let(:expected_value) { variable_value } let(:variable) do - { key: variable_key, value: variable_value, public: true, masked: false } + { key: variable_key, value: variable_value, public: true } end describe '.new' do @@ -88,7 +88,7 @@ describe Gitlab::Ci::Variables::Collection::Item do resource = described_class.fabricate(variable) 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 it 'supports using another collection item' do @@ -134,21 +134,7 @@ describe Gitlab::Ci::Variables::Collection::Item do .to_runner_variable expect(runner_variable) - .to eq(key: 'VAR', value: 'value', public: true, file: true, masked: false) - 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) + .to eq(key: 'VAR', value: 'value', public: true, file: true) end end end diff --git a/spec/lib/gitlab/ci/variables/collection_spec.rb b/spec/lib/gitlab/ci/variables/collection_spec.rb index edb209c0cf4..5c91816a586 100644 --- a/spec/lib/gitlab/ci/variables/collection_spec.rb +++ b/spec/lib/gitlab/ci/variables/collection_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe Gitlab::Ci::Variables::Collection do describe '.new' 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]) @@ -93,7 +93,7 @@ describe Gitlab::Ci::Variables::Collection do collection = described_class.new([{ key: 'TEST', value: '1' }]) 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 diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 81ff727b458..17540443688 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -2114,55 +2114,55 @@ describe Ci::Build do context 'returns variables' do let(:predefined_variables) do [ - { key: 'CI_PIPELINE_ID', value: pipeline.id.to_s, public: true, masked: false }, - { key: 'CI_PIPELINE_URL', value: project.web_url + "/pipelines/#{pipeline.id}", public: true, masked: false }, - { key: 'CI_JOB_ID', value: build.id.to_s, public: true, masked: false }, - { key: 'CI_JOB_URL', value: project.web_url + "/-/jobs/#{build.id}", public: true, masked: false }, - { key: 'CI_JOB_TOKEN', value: 'my-token', public: false, masked: false }, - { key: 'CI_BUILD_ID', value: build.id.to_s, public: true, masked: false }, - { key: 'CI_BUILD_TOKEN', value: 'my-token', public: false, masked: false }, - { key: 'CI_REGISTRY_USER', value: 'gitlab-ci-token', public: true, masked: false }, - { key: 'CI_REGISTRY_PASSWORD', value: 'my-token', public: false, masked: false }, - { key: 'CI_REPOSITORY_URL', value: build.repo_url, public: false, masked: false }, - { key: 'CI', value: 'true', public: true, masked: false }, - { key: 'GITLAB_CI', value: 'true', public: true, masked: false }, - { key: 'GITLAB_FEATURES', value: project.licensed_features.join(','), public: true, masked: false }, - { key: 'CI_SERVER_NAME', value: 'GitLab', public: true, masked: false }, - { key: 'CI_SERVER_VERSION', value: Gitlab::VERSION, public: true, masked: false }, - { key: 'CI_SERVER_VERSION_MAJOR', value: Gitlab.version_info.major.to_s, public: true, masked: false }, - { key: 'CI_SERVER_VERSION_MINOR', value: Gitlab.version_info.minor.to_s, public: true, masked: false }, - { key: 'CI_SERVER_VERSION_PATCH', value: Gitlab.version_info.patch.to_s, public: true, masked: false }, - { key: 'CI_SERVER_REVISION', value: Gitlab.revision, public: true, masked: false }, - { key: 'CI_JOB_NAME', value: 'test', public: true, masked: false }, - { key: 'CI_JOB_STAGE', value: 'test', public: true, masked: false }, - { key: 'CI_COMMIT_SHA', value: build.sha, public: true, masked: false }, - { key: 'CI_COMMIT_SHORT_SHA', value: build.short_sha, public: true, masked: false }, - { key: 'CI_COMMIT_BEFORE_SHA', value: build.before_sha, public: true, masked: false }, - { key: 'CI_COMMIT_REF_NAME', value: build.ref, public: true, masked: false }, - { key: 'CI_COMMIT_REF_SLUG', value: build.ref_slug, public: true, masked: false }, - { key: 'CI_NODE_TOTAL', value: '1', public: true, masked: false }, - { key: 'CI_BUILD_REF', value: build.sha, public: true, masked: false }, - { key: 'CI_BUILD_BEFORE_SHA', value: build.before_sha, public: true, masked: false }, - { key: 'CI_BUILD_REF_NAME', value: build.ref, public: true, masked: false }, - { key: 'CI_BUILD_REF_SLUG', value: build.ref_slug, public: true, masked: false }, - { key: 'CI_BUILD_NAME', value: 'test', public: true, masked: false }, - { key: 'CI_BUILD_STAGE', value: 'test', public: true, masked: false }, - { key: 'CI_PROJECT_ID', value: project.id.to_s, public: true, masked: false }, - { key: 'CI_PROJECT_NAME', value: project.path, public: true, masked: false }, - { key: 'CI_PROJECT_PATH', value: project.full_path, public: true, masked: false }, - { key: 'CI_PROJECT_PATH_SLUG', value: project.full_path_slug, public: true, masked: false }, - { key: 'CI_PROJECT_NAMESPACE', value: project.namespace.full_path, public: true, masked: false }, - { key: 'CI_PROJECT_URL', value: project.web_url, public: true, masked: false }, - { key: 'CI_PROJECT_VISIBILITY', value: 'private', public: true, masked: false }, - { key: 'CI_PAGES_DOMAIN', value: Gitlab.config.pages.host, public: true, masked: false }, - { key: 'CI_PAGES_URL', value: project.pages_url, public: true, masked: false }, - { key: 'CI_API_V4_URL', value: 'http://localhost/api/v4', public: true, masked: false }, - { key: 'CI_PIPELINE_IID', value: pipeline.iid.to_s, public: true, masked: false }, - { key: 'CI_CONFIG_PATH', value: pipeline.ci_yaml_file_path, public: true, masked: false }, - { key: 'CI_PIPELINE_SOURCE', value: pipeline.source, public: true, masked: false }, - { key: 'CI_COMMIT_MESSAGE', value: pipeline.git_commit_message, public: true, masked: false }, - { key: 'CI_COMMIT_TITLE', value: pipeline.git_commit_title, public: true, masked: false }, - { key: 'CI_COMMIT_DESCRIPTION', value: pipeline.git_commit_description, public: true, masked: false } + { key: 'CI_PIPELINE_ID', value: pipeline.id.to_s, public: true }, + { key: 'CI_PIPELINE_URL', value: project.web_url + "/pipelines/#{pipeline.id}", public: true }, + { key: 'CI_JOB_ID', value: build.id.to_s, public: true }, + { key: 'CI_JOB_URL', value: project.web_url + "/-/jobs/#{build.id}", public: true }, + { key: 'CI_JOB_TOKEN', value: 'my-token', public: false }, + { key: 'CI_BUILD_ID', value: build.id.to_s, public: true }, + { key: 'CI_BUILD_TOKEN', value: 'my-token', public: false }, + { key: 'CI_REGISTRY_USER', value: 'gitlab-ci-token', public: true }, + { key: 'CI_REGISTRY_PASSWORD', value: 'my-token', public: false }, + { key: 'CI_REPOSITORY_URL', value: build.repo_url, public: false }, + { key: 'CI', value: 'true', public: true }, + { key: 'GITLAB_CI', value: 'true', public: true }, + { key: 'GITLAB_FEATURES', value: project.licensed_features.join(','), public: true }, + { key: 'CI_SERVER_NAME', value: 'GitLab', public: true }, + { key: 'CI_SERVER_VERSION', value: Gitlab::VERSION, public: true }, + { key: 'CI_SERVER_VERSION_MAJOR', value: Gitlab.version_info.major.to_s, public: true }, + { key: 'CI_SERVER_VERSION_MINOR', value: Gitlab.version_info.minor.to_s, public: true }, + { key: 'CI_SERVER_VERSION_PATCH', value: Gitlab.version_info.patch.to_s, public: true }, + { key: 'CI_SERVER_REVISION', value: Gitlab.revision, public: true }, + { key: 'CI_JOB_NAME', value: 'test', public: true }, + { key: 'CI_JOB_STAGE', value: 'test', public: true }, + { key: 'CI_COMMIT_SHA', value: build.sha, public: true }, + { key: 'CI_COMMIT_SHORT_SHA', value: build.short_sha, public: true }, + { key: 'CI_COMMIT_BEFORE_SHA', value: build.before_sha, public: true }, + { key: 'CI_COMMIT_REF_NAME', value: build.ref, public: true }, + { key: 'CI_COMMIT_REF_SLUG', value: build.ref_slug, public: true }, + { key: 'CI_NODE_TOTAL', value: '1', public: true }, + { key: 'CI_BUILD_REF', value: build.sha, public: true }, + { key: 'CI_BUILD_BEFORE_SHA', value: build.before_sha, public: true }, + { key: 'CI_BUILD_REF_NAME', value: build.ref, public: true }, + { key: 'CI_BUILD_REF_SLUG', value: build.ref_slug, public: true }, + { key: 'CI_BUILD_NAME', value: 'test', public: true }, + { key: 'CI_BUILD_STAGE', value: 'test', public: true }, + { key: 'CI_PROJECT_ID', value: project.id.to_s, public: true }, + { key: 'CI_PROJECT_NAME', value: project.path, public: true }, + { key: 'CI_PROJECT_PATH', value: project.full_path, public: true }, + { key: 'CI_PROJECT_PATH_SLUG', value: project.full_path_slug, public: true }, + { key: 'CI_PROJECT_NAMESPACE', value: project.namespace.full_path, public: true }, + { key: 'CI_PROJECT_URL', value: project.web_url, public: true }, + { key: 'CI_PROJECT_VISIBILITY', value: 'private', public: true }, + { key: 'CI_PAGES_DOMAIN', value: Gitlab.config.pages.host, public: true }, + { key: 'CI_PAGES_URL', value: project.pages_url, public: true }, + { key: 'CI_API_V4_URL', value: 'http://localhost/api/v4', public: true }, + { key: 'CI_PIPELINE_IID', value: pipeline.iid.to_s, public: true }, + { key: 'CI_CONFIG_PATH', value: pipeline.ci_yaml_file_path, public: true }, + { key: 'CI_PIPELINE_SOURCE', value: pipeline.source, public: true }, + { key: 'CI_COMMIT_MESSAGE', value: pipeline.git_commit_message, public: true }, + { key: 'CI_COMMIT_TITLE', value: pipeline.git_commit_title, public: true }, + { key: 'CI_COMMIT_DESCRIPTION', value: pipeline.git_commit_description, public: true } ] end @@ -2175,10 +2175,10 @@ describe Ci::Build do describe 'variables ordering' do context 'when variables hierarchy is stubbed' do - let(:build_pre_var) { { key: 'build', value: 'value', public: true, masked: false } } - let(:project_pre_var) { { key: 'project', value: 'value', public: true, masked: false } } - let(:pipeline_pre_var) { { key: 'pipeline', value: 'value', public: true, masked: false } } - let(:build_yaml_var) { { key: 'yaml', value: 'value', public: true, masked: false } } + let(:build_pre_var) { { key: 'build', value: 'value', public: true } } + let(:project_pre_var) { { key: 'project', value: 'value', public: true } } + let(:pipeline_pre_var) { { key: 'pipeline', value: 'value', public: true } } + let(:build_yaml_var) { { key: 'yaml', value: 'value', public: true } } before do allow(build).to receive(:predefined_variables) { [build_pre_var] } @@ -2200,7 +2200,7 @@ describe Ci::Build do project_pre_var, pipeline_pre_var, build_yaml_var, - { key: 'secret', value: 'value', public: false, masked: false }]) + { key: 'secret', value: 'value', public: false }]) end end @@ -2233,10 +2233,10 @@ describe Ci::Build do context 'when build has user' do let(:user_variables) do [ - { key: 'GITLAB_USER_ID', value: user.id.to_s, public: true, masked: false }, - { key: 'GITLAB_USER_EMAIL', value: user.email, public: true, masked: false }, - { key: 'GITLAB_USER_LOGIN', value: user.username, public: true, masked: false }, - { key: 'GITLAB_USER_NAME', value: user.name, public: true, masked: false } + { key: 'GITLAB_USER_ID', value: user.id.to_s, public: true }, + { key: 'GITLAB_USER_EMAIL', value: user.email, public: true }, + { key: 'GITLAB_USER_LOGIN', value: user.username, public: true }, + { key: 'GITLAB_USER_NAME', value: user.name, public: true } ] end @@ -2250,8 +2250,8 @@ describe Ci::Build do context 'when build has an environment' do let(:environment_variables) do [ - { key: 'CI_ENVIRONMENT_NAME', value: 'production', public: true, masked: false }, - { key: 'CI_ENVIRONMENT_SLUG', value: 'prod-slug', public: true, masked: false } + { key: 'CI_ENVIRONMENT_NAME', value: 'production', public: true }, + { key: 'CI_ENVIRONMENT_SLUG', value: 'prod-slug', public: true } ] end @@ -2286,7 +2286,7 @@ describe Ci::Build do before do environment_variables << - { key: 'CI_ENVIRONMENT_URL', value: url, public: true, masked: false } + { key: 'CI_ENVIRONMENT_URL', value: url, public: true } end context 'when the URL was set from the job' do @@ -2323,7 +2323,7 @@ describe Ci::Build do end let(:manual_variable) do - { key: 'CI_JOB_MANUAL', value: 'true', public: true, masked: false } + { key: 'CI_JOB_MANUAL', value: 'true', public: true } end it { is_expected.to include(manual_variable) } @@ -2331,7 +2331,7 @@ describe Ci::Build do context 'when build is for tag' do let(:tag_variable) do - { key: 'CI_COMMIT_TAG', value: 'master', public: true, masked: false } + { key: 'CI_COMMIT_TAG', value: 'master', public: true } end before do @@ -2343,7 +2343,7 @@ describe Ci::Build do context 'when CI variable is defined' do let(:ci_variable) do - { key: 'SECRET_KEY', value: 'secret_value', public: false, masked: false } + { key: 'SECRET_KEY', value: 'secret_value', public: false } end before do @@ -2358,7 +2358,7 @@ describe Ci::Build do let(:ref) { Gitlab::Git::BRANCH_REF_PREFIX + build.ref } let(:protected_variable) do - { key: 'PROTECTED_KEY', value: 'protected_value', public: false, masked: false } + { key: 'PROTECTED_KEY', value: 'protected_value', public: false } end before do @@ -2390,7 +2390,7 @@ describe Ci::Build do context 'when group CI variable is defined' do let(:ci_variable) do - { key: 'SECRET_KEY', value: 'secret_value', public: false, masked: false } + { key: 'SECRET_KEY', value: 'secret_value', public: false } end before do @@ -2405,7 +2405,7 @@ describe Ci::Build do let(:ref) { Gitlab::Git::BRANCH_REF_PREFIX + build.ref } let(:protected_variable) do - { key: 'PROTECTED_KEY', value: 'protected_value', public: false, masked: false } + { key: 'PROTECTED_KEY', value: 'protected_value', public: false } end before do @@ -2444,11 +2444,11 @@ describe Ci::Build do let(:trigger_request) { create(:ci_trigger_request, pipeline: pipeline, trigger: trigger) } let(:user_trigger_variable) do - { key: 'TRIGGER_KEY_1', value: 'TRIGGER_VALUE_1', public: false, masked: false } + { key: 'TRIGGER_KEY_1', value: 'TRIGGER_VALUE_1', public: false } end let(:predefined_trigger_variable) do - { key: 'CI_PIPELINE_TRIGGERED', value: 'true', public: true, masked: false } + { key: 'CI_PIPELINE_TRIGGERED', value: 'true', public: true } end before do @@ -2480,7 +2480,7 @@ describe Ci::Build do context 'when pipeline has a variable' do let!(:pipeline_variable) { create(:ci_pipeline_variable, pipeline: pipeline) } - it { is_expected.to include(key: pipeline_variable.key, value: pipeline_variable.value, public: false, masked: false) } + it { is_expected.to include(pipeline_variable.to_runner_variable) } end context 'when a job was triggered by a pipeline schedule' do @@ -2497,16 +2497,16 @@ describe Ci::Build do pipeline_schedule.reload end - it { is_expected.to include(key: pipeline_schedule_variable.key, value: pipeline_schedule_variable.value, public: false, masked: false) } + it { is_expected.to include(pipeline_schedule_variable.to_runner_variable) } end context 'when container registry is enabled' do let(:container_registry_enabled) { true } let(:ci_registry) do - { key: 'CI_REGISTRY', value: 'registry.example.com', public: true, masked: false } + { key: 'CI_REGISTRY', value: 'registry.example.com', public: true } end let(:ci_registry_image) do - { key: 'CI_REGISTRY_IMAGE', value: project.container_registry_url, public: true, masked: false } + { key: 'CI_REGISTRY_IMAGE', value: project.container_registry_url, public: true } end context 'and is disabled for project' do @@ -2535,13 +2535,13 @@ describe Ci::Build do build.update(runner: runner) end - it { is_expected.to include({ key: 'CI_RUNNER_ID', value: runner.id.to_s, public: true, masked: false }) } - it { is_expected.to include({ key: 'CI_RUNNER_DESCRIPTION', value: 'description', public: true, masked: false }) } - it { is_expected.to include({ key: 'CI_RUNNER_TAGS', value: 'docker, linux', public: true, masked: false }) } + it { is_expected.to include({ key: 'CI_RUNNER_ID', value: runner.id.to_s, public: true }) } + it { is_expected.to include({ key: 'CI_RUNNER_DESCRIPTION', value: 'description', public: true }) } + it { is_expected.to include({ key: 'CI_RUNNER_TAGS', value: 'docker, linux', public: true }) } end context 'when build is for a deployment' do - let(:deployment_variable) { { key: 'KUBERNETES_TOKEN', value: 'TOKEN', public: false, masked: false } } + let(:deployment_variable) { { key: 'KUBERNETES_TOKEN', value: 'TOKEN', public: false } } before do build.environment = 'production' @@ -2555,7 +2555,7 @@ describe Ci::Build do end context 'when project has custom CI config path' do - let(:ci_config_path) { { key: 'CI_CONFIG_PATH', value: 'custom', public: true, masked: false } } + let(:ci_config_path) { { key: 'CI_CONFIG_PATH', value: 'custom', public: true } } before do project.update(ci_config_path: 'custom') @@ -2572,7 +2572,7 @@ describe Ci::Build do it "includes AUTO_DEVOPS_DOMAIN" do is_expected.to include( - { key: 'AUTO_DEVOPS_DOMAIN', value: 'example.com', public: true, masked: false }) + { key: 'AUTO_DEVOPS_DOMAIN', value: 'example.com', public: true }) end end @@ -2583,7 +2583,7 @@ describe Ci::Build do it "includes AUTO_DEVOPS_DOMAIN" do is_expected.not_to include( - { key: 'AUTO_DEVOPS_DOMAIN', value: 'example.com', public: true, masked: false }) + { key: 'AUTO_DEVOPS_DOMAIN', value: 'example.com', public: true }) end end end @@ -2598,9 +2598,9 @@ describe Ci::Build do variables = subject.reverse.uniq { |variable| variable[:key] }.reverse expect(variables) - .not_to include(key: 'MYVAR', value: 'myvar', public: true, masked: false) + .not_to include(key: 'MYVAR', value: 'myvar', public: true) expect(variables) - .to include(key: 'MYVAR', value: 'pipeline value', public: false, masked: false) + .to include(key: 'MYVAR', value: 'pipeline value', public: false) end end @@ -2616,13 +2616,13 @@ describe Ci::Build do it 'includes CI_NODE_INDEX' do is_expected.to include( - { key: 'CI_NODE_INDEX', value: index.to_s, public: true, masked: false } + { key: 'CI_NODE_INDEX', value: index.to_s, public: true } ) end it 'includes correct CI_NODE_TOTAL' do is_expected.to include( - { key: 'CI_NODE_TOTAL', value: total.to_s, public: true, masked: false } + { key: 'CI_NODE_TOTAL', value: total.to_s, public: true } ) end end @@ -2641,7 +2641,7 @@ describe Ci::Build do it 'returns static predefined variables' do expect(build.variables.size).to be >= 28 expect(build.variables) - .to include(key: 'CI_COMMIT_REF_NAME', value: 'feature', public: true, masked: false) + .to include(key: 'CI_COMMIT_REF_NAME', value: 'feature', public: true) expect(build).not_to be_persisted end end @@ -2651,8 +2651,8 @@ describe Ci::Build do let(:deploy_token_variables) do [ - { key: 'CI_DEPLOY_USER', value: deploy_token.username, public: true, masked: false }, - { key: 'CI_DEPLOY_PASSWORD', value: deploy_token.token, public: false, masked: false } + { key: 'CI_DEPLOY_USER', value: deploy_token.username, public: true }, + { key: 'CI_DEPLOY_PASSWORD', value: deploy_token.token, public: false } ] end @@ -2711,7 +2711,7 @@ describe Ci::Build do end expect(variables) - .to include(key: 'CI_COMMIT_REF_NAME', value: 'feature', public: true, masked: false) + .to include(key: 'CI_COMMIT_REF_NAME', value: 'feature', public: true) end it 'does not return prohibited variables' do diff --git a/spec/models/ci/group_variable_spec.rb b/spec/models/ci/group_variable_spec.rb index 21d96bf3454..1b10501701c 100644 --- a/spec/models/ci/group_variable_spec.rb +++ b/spec/models/ci/group_variable_spec.rb @@ -5,7 +5,6 @@ describe Ci::GroupVariable do it { is_expected.to include_module(HasVariable) } 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/) } describe '.unprotected' do diff --git a/spec/models/ci/variable_spec.rb b/spec/models/ci/variable_spec.rb index 02c07a2bd83..875e8b2b682 100644 --- a/spec/models/ci/variable_spec.rb +++ b/spec/models/ci/variable_spec.rb @@ -6,7 +6,6 @@ describe Ci::Variable do describe 'validations' do it { is_expected.to include_module(HasVariable) } 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/) } end diff --git a/spec/models/concerns/has_variable_spec.rb b/spec/models/concerns/has_variable_spec.rb index bff96e12ffa..3fbe86c5b56 100644 --- a/spec/models/concerns/has_variable_spec.rb +++ b/spec/models/concerns/has_variable_spec.rb @@ -57,7 +57,7 @@ describe HasVariable do describe '#to_runner_variable' do it 'returns a hash for the runner' do 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 diff --git a/spec/models/concerns/maskable_spec.rb b/spec/models/concerns/maskable_spec.rb deleted file mode 100644 index aeba7ad862f..00000000000 --- a/spec/models/concerns/maskable_spec.rb +++ /dev/null @@ -1,76 +0,0 @@ -# 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 diff --git a/spec/requests/api/group_variables_spec.rb b/spec/requests/api/group_variables_spec.rb index 66b9aae4b58..e52f4c70407 100644 --- a/spec/requests/api/group_variables_spec.rb +++ b/spec/requests/api/group_variables_spec.rb @@ -87,12 +87,12 @@ describe API::GroupVariables do it 'creates variable' 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) expect(response).to have_gitlab_http_status(201) 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 end diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index 43c06f7c973..e6c235ca26e 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -436,9 +436,9 @@ describe API::Runner, :clean_gitlab_redis_shared_state do end let(:expected_variables) do - [{ 'key' => 'CI_JOB_NAME', 'value' => 'spinach', 'public' => true, 'masked' => false }, - { 'key' => 'CI_JOB_STAGE', 'value' => 'test', 'public' => true, 'masked' => false }, - { 'key' => 'DB_NAME', 'value' => 'postgres', 'public' => true, 'masked' => false }] + [{ 'key' => 'CI_JOB_NAME', 'value' => 'spinach', 'public' => true }, + { 'key' => 'CI_JOB_STAGE', 'value' => 'test', 'public' => true }, + { 'key' => 'DB_NAME', 'value' => 'postgres', 'public' => true }] end let(:expected_artifacts) do @@ -740,12 +740,12 @@ describe API::Runner, :clean_gitlab_redis_shared_state do context 'when triggered job is available' do let(:expected_variables) do - [{ 'key' => 'CI_JOB_NAME', 'value' => 'spinach', 'public' => true, 'masked' => false }, - { 'key' => 'CI_JOB_STAGE', 'value' => 'test', 'public' => true, 'masked' => false }, - { 'key' => 'CI_PIPELINE_TRIGGERED', 'value' => 'true', 'public' => true, 'masked' => false }, - { 'key' => 'DB_NAME', 'value' => 'postgres', 'public' => true, 'masked' => false }, - { 'key' => 'SECRET_KEY', 'value' => 'secret_value', 'public' => false, 'masked' => false }, - { 'key' => 'TRIGGER_KEY_1', 'value' => 'TRIGGER_VALUE_1', 'public' => false, 'masked' => false }] + [{ 'key' => 'CI_JOB_NAME', 'value' => 'spinach', 'public' => true }, + { 'key' => 'CI_JOB_STAGE', 'value' => 'test', 'public' => true }, + { 'key' => 'CI_PIPELINE_TRIGGERED', 'value' => 'true', 'public' => true }, + { 'key' => 'DB_NAME', 'value' => 'postgres', 'public' => true }, + { 'key' => 'SECRET_KEY', 'value' => 'secret_value', 'public' => false }, + { 'key' => 'TRIGGER_KEY_1', 'value' => 'TRIGGER_VALUE_1', 'public' => false }] end let(:trigger) { create(:ci_trigger, project: project) } diff --git a/spec/requests/api/variables_spec.rb b/spec/requests/api/variables_spec.rb index 5df6baf0ddf..cdac5b2f400 100644 --- a/spec/requests/api/variables_spec.rb +++ b/spec/requests/api/variables_spec.rb @@ -73,12 +73,12 @@ describe API::Variables do context 'authorized user with proper permissions' do it 'creates variable' 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) expect(response).to have_gitlab_http_status(201) 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 end diff --git a/spec/support/features/variable_list_shared_examples.rb b/spec/support/features/variable_list_shared_examples.rb index 73156d18c1b..0a464d77cb7 100644 --- a/spec/support/features/variable_list_shared_examples.rb +++ b/spec/support/features/variable_list_shared_examples.rb @@ -8,7 +8,7 @@ shared_examples 'variable list' do it 'adds new CI variable' 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-value').set('key_value') + find('.js-ci-variable-input-value').set('key value') end click_button('Save variables') @@ -19,7 +19,7 @@ shared_examples 'variable list' do # 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 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 @@ -44,7 +44,7 @@ shared_examples 'variable list' do it 'adds new protected variable' 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-value').set('key_value') + find('.js-ci-variable-input-value').set('key value') find('.ci-variable-protected-item .js-project-feature-toggle').click expect(find('.js-ci-variable-input-protected', visible: false).value).to eq('true') @@ -58,7 +58,7 @@ shared_examples 'variable list' do # 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 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') end end