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

This reverts merge request !25293

app/models/ci/group_variable.rb

@@ -5,7 +5,6 @@ class GroupVariable < ActiveRecord::Base
     extend Gitlab::Ci::Model
     include HasVariable
     include Presentable
-    include Maskable
 
     belongs_to :group, class_name: "::Group"
 

app/models/ci/variable.rb

@@ -5,7 +5,6 @@ class Variable < ActiveRecord::Base
     extend Gitlab::Ci::Model
     include HasVariable
     include Presentable
-    include Maskable
 
     belongs_to :project
 

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

app/models/concerns/maskable.rb

-# 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

changelogs/unreleased/13784-simple-masking-of-protected-variables-in-logs.yml

----
-title: Add support for masking CI variables.
-merge_request: 25293
-author:
-type: added

db/migrate/20190218134158_add_masked_to_ci_variables.rb

-# 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

db/migrate/20190218134209_add_masked_to_ci_group_variables.rb

-# 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

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 @@
     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 @@
     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
 

lib/gitlab/ci/variables/collection/item.rb

@@ -5,12 +5,12 @@ module Ci
     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 @@ def ==(other)
           # 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
 

spec/factories/ci/group_variables.rb

@@ -2,7 +2,6 @@
   factory :ci_group_variable, class: Ci::GroupVariable do
     sequence(:key) { |n| "VARIABLE_#{n}" }
     value 'VARIABLE_VALUE'
-    masked false
 
     trait(:protected) do
       protected true

spec/factories/ci/variables.rb

@@ -2,7 +2,6 @@
   factory :ci_variable, class: Ci::Variable do
     sequence(:key) { |n| "VARIABLE_#{n}" }
     value 'VARIABLE_VALUE'
-    masked false
 
     trait(:protected) do
       protected true

spec/features/group_variables_spec.rb

@@ -3,7 +3,7 @@
 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

spec/features/project_variables_spec.rb

@@ -3,7 +3,7 @@
 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

spec/lib/gitlab/ci/variables/collection/item_spec.rb

@@ -6,7 +6,7 @@
   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 @@
       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 @@
           .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

spec/lib/gitlab/ci/variables/collection_spec.rb

@@ -3,7 +3,7 @@
 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 @@
       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
 

spec/models/ci/group_variable_spec.rb

@@ -5,7 +5,6 @@
 
   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

spec/models/ci/variable_spec.rb

@@ -6,7 +6,6 @@
   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
 

spec/models/concerns/has_variable_spec.rb

@@ -57,7 +57,7 @@
   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

spec/models/concerns/maskable_spec.rb

-# 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