Merge branch '328239-use-type-instead-of-name-to-detect-secret-integration-fields' into 'master'

Use type to detect password fields in integrations instead of name

See merge request gitlab-org/gitlab!68786

app/controllers/concerns/integrations/params.rb

@@ -77,18 +77,15 @@ module Integrations
       :webhook
     ].freeze
 
-    # Parameters to ignore if no value is specified
-    FILTER_BLANK_PARAMS = [:password].freeze
-
     def integration_params
-      dynamic_params = @integration.event_channel_names + @integration.event_names # rubocop:disable Gitlab/ModuleWithInstanceVariables
+      dynamic_params = integration.event_channel_names + integration.event_names
       allowed = allowed_integration_params + dynamic_params
       return_value = params.permit(:id, integration: allowed, service: allowed)
       return_value[:integration] ||= return_value.delete(:service)
       param_values = return_value[:integration]
 
       if param_values.is_a?(ActionController::Parameters)
-        FILTER_BLANK_PARAMS.each do |param|
+        integration.password_fields.each do |param|
           param_values.delete(param) if param_values[param].blank?
         end
       end

app/controllers/projects/services_controller.rb

@@ -26,15 +26,15 @@ class Projects::ServicesController < Projects::ApplicationController
     attributes = integration_params[:integration]
 
     if use_inherited_settings?(attributes)
-      @integration.inherit_from_id = default_integration.id
+      integration.inherit_from_id = default_integration.id
 
-      if saved = @integration.save(context: :manual_change)
-        BulkUpdateIntegrationService.new(default_integration, [@integration]).execute
+      if saved = integration.save(context: :manual_change)
+        BulkUpdateIntegrationService.new(default_integration, [integration]).execute
       end
     else
       attributes[:inherit_from_id] = nil
-      @integration.attributes = attributes
-      saved = @integration.save(context: :manual_change)
+      integration.attributes = attributes
+      saved = integration.save(context: :manual_change)
     end
 
     respond_to do |format|
@@ -65,15 +65,15 @@ class Projects::ServicesController < Projects::ApplicationController
   private
 
   def redirect_path
-    safe_redirect_path(params[:redirect_to]).presence || edit_project_service_path(@project, @integration)
+    safe_redirect_path(params[:redirect_to]).presence || edit_project_service_path(project, integration)
   end
 
   def service_test_response
-    unless @integration.update(integration_params[:integration])
-      return { error: true, message: _('Validations failed.'), service_response: @integration.errors.full_messages.join(','), test_failed: false }
+    unless integration.update(integration_params[:integration])
+      return { error: true, message: _('Validations failed.'), service_response: integration.errors.full_messages.join(','), test_failed: false }
     end
 
-    result = ::Integrations::Test::ProjectService.new(@integration, current_user, params[:event]).execute
+    result = ::Integrations::Test::ProjectService.new(integration, current_user, params[:event]).execute
 
     unless result[:success]
       return { error: true, message: s_('Integrations|Connection failed. Please check your settings.'), service_response: result[:message].to_s, test_failed: true }
@@ -93,7 +93,7 @@ class Projects::ServicesController < Projects::ApplicationController
   end
 
   def integration
-    @integration ||= @project.find_or_initialize_integration(params[:id])
+    @integration ||= project.find_or_initialize_integration(params[:id])
   end
   alias_method :service, :integration
 

app/models/integration.rb

@@ -357,6 +357,10 @@ class Integration < ApplicationRecord
     []
   end
 
+  def password_fields
+    fields.select { |f| f[:type] == 'password' }.pluck(:name)
+  end
+
   # Expose a list of fields in the JSON endpoint.
   #
   # This list is used in `Integration#as_json(only: json_fields)`.

spec/controllers/admin/integrations_controller_spec.rb

@@ -9,6 +9,14 @@ RSpec.describe Admin::IntegrationsController do
     sign_in(admin)
   end
 
+  it_behaves_like IntegrationsActions do
+    let(:integration_attributes) { { instance: true, project: nil } }
+
+    let(:routing_params) do
+      { id: integration.to_param }
+    end
+  end
+
   describe '#edit' do
     Integration.available_integration_names.each do |integration_name|
       context "#{integration_name}" do

spec/controllers/groups/settings/integrations_controller_spec.rb

@@ -10,6 +10,21 @@ RSpec.describe Groups::Settings::IntegrationsController do
     sign_in(user)
   end
 
+  it_behaves_like IntegrationsActions do
+    let(:integration_attributes) { { group: group, project: nil } }
+
+    let(:routing_params) do
+      {
+        group_id: group,
+        id: integration.to_param
+      }
+    end
+
+    before do
+      group.add_owner(user)
+    end
+  end
+
   describe '#index' do
     context 'when user is not owner' do
       it 'renders not_found' do

spec/controllers/projects/services_controller_spec.rb

@@ -18,6 +18,18 @@ RSpec.describe Projects::ServicesController do
     project.add_maintainer(user)
   end
 
+  it_behaves_like IntegrationsActions do
+    let(:integration_attributes) { { project: project } }
+
+    let(:routing_params) do
+      {
+        namespace_id: project.namespace,
+        project_id: project,
+        id: integration.to_param
+      }
+    end
+  end
+
   describe '#test' do
     context 'when the integration is not testable' do
       it 'renders 404' do

spec/factories/integrations.rb

@@ -12,6 +12,12 @@ FactoryBot.define do
     issue_tracker
   end
 
+  factory :datadog_integration, class: 'Integrations::Datadog' do
+    project
+    active { true }
+    api_key { 'secret' }
+  end
+
   factory :emails_on_push_integration, class: 'Integrations::EmailsOnPush' do
     project
     type { 'EmailsOnPushService' }

spec/models/integration_spec.rb

@@ -825,4 +825,20 @@ RSpec.describe Integration do
         .to include(*described_class::PROJECT_SPECIFIC_INTEGRATION_NAMES)
     end
   end
+
+  describe '#password_fields' do
+    it 'returns all fields with type `password`' do
+      allow(subject).to receive(:fields).and_return([
+        { name: 'password', type: 'password' },
+        { name: 'secret', type: 'password' },
+        { name: 'public', type: 'text' }
+      ])
+
+      expect(subject.password_fields).to match_array(%w[password secret])
+    end
+
+    it 'returns an empty array if no password fields exist' do
+      expect(subject.password_fields).to eq([])
+    end
+  end
 end

spec/support/shared_examples/controllers/concerns/integrations_actions_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples IntegrationsActions do
+  let(:integration) do
+    create(:datadog_integration,
+      integration_attributes.merge(
+        api_url: 'http://example.com',
+        api_key: 'secret'
+      )
+    )
+  end
+
+  describe 'GET #edit' do
+    before do
+      get :edit, params: routing_params
+    end
+
+    it 'assigns the integration' do
+      expect(response).to have_gitlab_http_status(:ok)
+      expect(assigns(:integration)).to eq(integration)
+    end
+  end
+
+  describe 'PUT #update' do
+    let(:params) do
+      {
+        datadog_env: 'env',
+        datadog_service: 'service'
+      }
+    end
+
+    before do
+      put :update, params: routing_params.merge(integration: params)
+    end
+
+    it 'updates the integration with the provided params and redirects to the form' do
+      expect(response).to redirect_to(routing_params.merge(action: :edit))
+      expect(integration.reload).to have_attributes(params)
+    end
+
+    context 'when sending a password field' do
+      let(:params) { super().merge(api_key: 'new') }
+
+      it 'updates the integration with the password and other params' do
+        expect(response).to be_redirect
+        expect(integration.reload).to have_attributes(params)
+      end
+    end
+
+    context 'when sending a blank password field' do
+      let(:params) { super().merge(api_key: '') }
+
+      it 'ignores the password field and saves the other params' do
+        expect(response).to be_redirect
+        expect(integration.reload).to have_attributes(params.merge(api_key: 'secret'))
+      end
+    end
+  end
+end