Merge branch '29594-remove-plaintext-ff-tokens' into 'master'

Remove plaintext token values for feature flags clients

See merge request gitlab-org/gitlab!18923

db/post_migrate/20191022113635_nullify_feature_flag_plaintext_tokens.rb

+# frozen_string_literal: true
+
+class NullifyFeatureFlagPlaintextTokens < ActiveRecord::Migration[5.2]
+  DOWNTIME = false
+
+  class FeatureFlagsClient < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'operations_feature_flags_clients'
+
+    scope :with_encrypted_token, -> { where.not(token_encrypted: nil) }
+    scope :with_plaintext_token, -> { where.not(token: nil) }
+    scope :without_plaintext_token, -> { where(token: nil) }
+  end
+
+  disable_ddl_transaction!
+
+  def up
+    return unless Gitlab.ee?
+
+    # 7357 records to be updated on GitLab.com
+    FeatureFlagsClient.with_encrypted_token.with_plaintext_token.each_batch do |relation|
+      relation.update_all(token: nil)
+    end
+  end
+
+  def down
+    return unless Gitlab.ee?
+
+    # There is no way to restore only the tokens that were NULLifyed in the `up`
+    # but we can do is to restore _all_ of them in case it is needed.
+    say_with_time('Decrypting tokens from operations_feature_flags_clients') do
+      FeatureFlagsClient.with_encrypted_token.without_plaintext_token.find_each do |feature_flags_client|
+        token = Gitlab::CryptoHelper.aes256_gcm_decrypt(feature_flags_client.token_encrypted)
+        feature_flags_client.update_column(:token, token)
+      end
+    end
+  end
+end

ee/app/models/operations/feature_flags_client.rb

@@ -5,13 +5,14 @@ module Operations
     include TokenAuthenticatable
 
     self.table_name = 'operations_feature_flags_clients'
+    self.ignored_columns += %i[token]
 
     belongs_to :project
 
     validates :project, presence: true
     validates :token, presence: true
 
-    add_authentication_token_field :token, encrypted: :optional
+    add_authentication_token_field :token, encrypted: :required
 
     before_validation :ensure_token!
 

ee/changelogs/unreleased/29594-remove-plaintext-ff-tokens.yml

+---
+title: Remove plaintext tokens for feature flags clients
+merge_request: 18923
+author:
+type: other

ee/spec/migrations/nullify_feature_flag_plaintext_tokens_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20191022113635_nullify_feature_flag_plaintext_tokens.rb')
+
+describe NullifyFeatureFlagPlaintextTokens, :migration do
+  let(:namespaces) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+  let(:feature_flags_clients) { table(:operations_feature_flags_clients) }
+
+  let!(:namespace) { namespaces.create!(id: 11, name: 'gitlab', path: 'gitlab-org') }
+  let!(:project1) { projects.create!(namespace_id: namespace.id, name: 'Project 1') }
+  let!(:project2) { projects.create!(namespace_id: namespace.id, name: 'Project 2') }
+
+  let(:secret1_encrypted) { Gitlab::CryptoHelper.aes256_gcm_encrypt('secret1') }
+  let(:secret2_encrypted) { Gitlab::CryptoHelper.aes256_gcm_encrypt('secret2') }
+
+  before do
+    feature_flags_clients.create!(token: 'secret1', token_encrypted: secret1_encrypted, project_id: project1.id)
+    feature_flags_clients.create!(token: nil, token_encrypted: secret2_encrypted, project_id: project2.id)
+  end
+
+  it 'correctly migrates up and down' do
+    feature_flag1 = feature_flags_clients.find_by_project_id(project1.id)
+    feature_flag2 = feature_flags_clients.find_by_project_id(project2.id)
+
+    reversible_migration do |migration|
+      migration.before -> {
+        feature_flag1.reload
+        expect(feature_flag1.token).to eq('secret1')
+        expect(feature_flag1.token_encrypted).to eq(secret1_encrypted)
+
+        feature_flag2.reload
+        expect(feature_flag2.token_encrypted).to eq(secret2_encrypted)
+      }
+
+      migration.after -> {
+        expect(feature_flags_clients.where('token IS NOT NULL').count).to eq(0)
+
+        feature_flag1.reload
+        expect(feature_flag1.token).to be_nil
+        expect(feature_flag1.token_encrypted).to eq(secret1_encrypted)
+
+        feature_flag2.reload
+        expect(feature_flag2.token).to be_nil
+        expect(feature_flag2.token_encrypted).to eq(secret2_encrypted)
+      }
+    end
+  end
+end

ee/spec/models/project_spec.rb

@@ -1704,7 +1704,8 @@ describe Project do
     end
 
     context 'when there is access token' do
-      let!(:instance) { create(:operations_feature_flags_client, project: project, token: 'token') }
+      let(:token_encrypted) { Gitlab::CryptoHelper.aes256_gcm_encrypt('token') }
+      let!(:instance) { create(:operations_feature_flags_client, project: project, token_encrypted: token_encrypted) }
 
       it "provides an existing one" do
         is_expected.to eq('token')