Use type to detect password fields in integrations instead of name

When updating integration settings, password fields only need to be
filled in if the user wants to change them, otherwise the previous
values would be preserved.

This relied on clearing blank params with the name `password`, but some
integrations use a different name, such as `api_key` in the Datadog
integration. So instead we check for fields with `type: 'password'`,
which is also what the frontend does to decide how to render these
fields in the `DynamicField` component.

Changelog: fixed

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