Merge branch 'security-132-remove-eks-details-from-admin-form' into 'master'

Hide EKS secret key in admin integrations settings

Closes #132

See merge request gitlab-org/security/gitlab!527

app/controllers/admin/application_settings_controller.rb

@@ -191,8 +191,10 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
 
     params[:application_setting][:import_sources]&.delete("")
     params[:application_setting][:restricted_visibility_levels]&.delete("")
-    params[:application_setting].delete(:elasticsearch_aws_secret_access_key) if params[:application_setting][:elasticsearch_aws_secret_access_key].blank?
     params[:application_setting][:required_instance_ci_template] = nil if params[:application_setting][:required_instance_ci_template].blank?
+
+    remove_blank_params_for!(:elasticsearch_aws_secret_access_key, :eks_secret_access_key)
+
     # TODO Remove domain_blacklist_raw in APIv5 (See https://gitlab.com/gitlab-org/gitlab-foss/issues/67204)
     params.delete(:domain_blacklist_raw) if params[:domain_blacklist_file]
     params.delete(:domain_blacklist_raw) if params[:domain_blacklist]
@@ -262,6 +264,10 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
     render action
   end
 
+  def remove_blank_params_for!(*keys)
+    params[:application_setting].delete_if { |setting, value| setting.to_sym.in?(keys) && value.blank? }
+  end
+
   # overridden in EE
   def valid_setting_panels
     VALID_SETTING_PANELS

app/views/admin/application_settings/_eks.html.haml

@@ -26,6 +26,6 @@
           = f.text_field :eks_access_key_id, class: 'form-control'
         .form-group
           = f.label :eks_secret_access_key, 'Secret access key', class: 'label-bold'
-          = f.password_field :eks_secret_access_key, value: @application_setting.eks_secret_access_key, class: 'form-control'
+          = f.password_field :eks_secret_access_key, autocomplete: 'off', class: 'form-control'
 
       = f.submit 'Save changes', class: "btn btn-success"

changelogs/unreleased/security-132-remove-eks-details-from-admin-form.yml

+---
+title: Hide EKS secret key in admin integrations settings
+merge_request:
+author:
+type: security

spec/controllers/admin/application_settings_controller_spec.rb

@@ -162,6 +162,46 @@ describe Admin::ApplicationSettingsController do
     end
   end
 
+  describe 'PATCH #integrations' do
+    before do
+      stub_feature_flags(instance_level_integrations: false)
+      sign_in(admin)
+    end
+
+    describe 'EKS integration' do
+      let(:application_setting) { ApplicationSetting.current }
+      let(:settings_params) do
+        {
+          eks_integration_enabled: '1',
+          eks_account_id: '123456789012',
+          eks_access_key_id: 'dummy access key',
+          eks_secret_access_key: 'dummy secret key'
+        }
+      end
+
+      it 'updates EKS settings' do
+        patch :integrations, params: { application_setting: settings_params }
+
+        expect(application_setting.eks_integration_enabled).to be_truthy
+        expect(application_setting.eks_account_id).to eq '123456789012'
+        expect(application_setting.eks_access_key_id).to eq 'dummy access key'
+        expect(application_setting.eks_secret_access_key).to eq 'dummy secret key'
+      end
+
+      context 'secret access key is blank' do
+        let(:settings_params) { { eks_secret_access_key: '' } }
+
+        it 'does not update the secret key' do
+          application_setting.update!(eks_secret_access_key: 'dummy secret key')
+
+          patch :integrations, params: { application_setting: settings_params }
+
+          expect(application_setting.reload.eks_secret_access_key).to eq 'dummy secret key'
+        end
+      end
+    end
+  end
+
   describe 'PUT #reset_registration_token' do
     before do
       sign_in(admin)

spec/views/admin/application_settings/_eks.html.haml_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'admin/application_settings/_eks' do
+  let_it_be(:admin) { create(:admin) }
+  let(:page) { Capybara::Node::Simple.new(rendered) }
+
+  before do
+    assign(:application_setting, application_setting)
+    allow(view).to receive(:current_user) { admin }
+    allow(view).to receive(:expanded) { true }
+  end
+
+  shared_examples 'EKS secret access key input' do
+    it 'renders an empty password field' do
+      render
+      expect(rendered).to have_field('Secret access key', type: 'password')
+      expect(page.find_field('Secret access key').value).to be_blank
+    end
+  end
+
+  context 'when eks_secret_access_key is not set' do
+    let(:application_setting) { build(:application_setting) }
+
+    include_examples 'EKS secret access key input'
+  end
+
+  context 'when eks_secret_access_key is set' do
+    let(:application_setting) { build(:application_setting, eks_secret_access_key: 'eks_secret_access_key') }
+
+    include_examples 'EKS secret access key input'
+  end
+end