From c60cd20834d38f70601e55829f6dee27872f069d Mon Sep 17 00:00:00 2001 From: Rez Date: Tue, 2 Sep 2025 23:25:32 +0200 Subject: [PATCH 1/2] Fix SlackIntegration duplicate bug when inheriting from parent When creating project integrations from group/instance-level GitLab Slack Application integrations, the SlackIntegration alias was not being set correctly because the dup method doesn't duplicate associated records (slack_integration), leaving the new integration without the necessary association. This adds a post_duplicate_setup hook that allows integrations to configure their associations after the parent relationship is established. Changelog: fixed --- .../concerns/integrations/base/integration.rb | 9 +++++ .../integrations/gitlab_slack_application.rb | 9 +++++ app/models/slack_integration.rb | 1 + .../slack_installation/instance_service.rb | 2 +- spec/models/integration_spec.rb | 8 ++++ .../gitlab_slack_application_spec.rb | 40 +++++++++++++++++++ spec/models/slack_integration_spec.rb | 6 +++ 7 files changed, 74 insertions(+), 1 deletion(-) diff --git a/app/models/concerns/integrations/base/integration.rb b/app/models/concerns/integrations/base/integration.rb index 40c823b529d51a..4a2256d05dedf3 100644 --- a/app/models/concerns/integrations/base/integration.rb +++ b/app/models/concerns/integrations/base/integration.rb @@ -308,6 +308,9 @@ def build_from_integration(integration, project_id: nil, group_id: nil) new_integration.project_id = project_id new_integration.group_id = group_id new_integration.inherit_from_id = integration.id if integration.inheritable? + + integration.after_build_from_integration(new_integration) + new_integration end @@ -576,6 +579,12 @@ def fields self.class.fields.dup end + # Hook for integrations to configure associations after duplication. + # Override in subclasses when associations need the parent context. + def after_build_from_integration(new_integration) + # no-op + end + # Duplicating an integration also duplicates the data fields. Duped records have different ciphertexts. override :dup def dup diff --git a/app/models/integrations/gitlab_slack_application.rb b/app/models/integrations/gitlab_slack_application.rb index 2ed5a38333a458..de411ddbe680d6 100644 --- a/app/models/integrations/gitlab_slack_application.rb +++ b/app/models/integrations/gitlab_slack_application.rb @@ -79,6 +79,15 @@ def configurable_events super end + def after_build_from_integration(new_integration) + return unless slack_integration + + new_integration.slack_integration = slack_integration.dup.tap do |entity| + entity.alias = new_integration.parent&.full_path || SlackIntegration::INSTANCE_ALIAS + entity.authorized_scope_names = slack_integration.authorized_scope_names + end + end + override :requires_webhook? def self.requires_webhook? false diff --git a/app/models/slack_integration.rb b/app/models/slack_integration.rb index 7900203b993dda..aae5721388d486 100644 --- a/app/models/slack_integration.rb +++ b/app/models/slack_integration.rb @@ -9,6 +9,7 @@ class SlackIntegration < ApplicationRecord SCOPE_COMMANDS = 'commands' SCOPE_CHAT_WRITE = 'chat:write' SCOPE_CHAT_WRITE_PUBLIC = 'chat:write.public' + INSTANCE_ALIAS = '_gitlab-instance' # These scopes are requested when installing the app, additional scopes # will need reauthorization. diff --git a/app/services/integrations/slack_installation/instance_service.rb b/app/services/integrations/slack_installation/instance_service.rb index 2d53547cd3cd37..8128fc58a08100 100644 --- a/app/services/integrations/slack_installation/instance_service.rb +++ b/app/services/integrations/slack_installation/instance_service.rb @@ -10,7 +10,7 @@ def redirect_uri end def installation_alias - '_gitlab-instance' + SlackIntegration::INSTANCE_ALIAS end def authorized? diff --git a/spec/models/integration_spec.rb b/spec/models/integration_spec.rb index b29f65e5a9ecad..e239fa025d0e87 100644 --- a/spec/models/integration_spec.rb +++ b/spec/models/integration_spec.rb @@ -480,6 +480,14 @@ def integration_hash(type) end describe '.build_from_integration' do + it 'calls after_build_from_integration hook' do + integration = create(:integration, :instance) + + expect(integration).to receive(:after_build_from_integration).with(kind_of(described_class)) + + described_class.build_from_integration(integration, project_id: project.id) + end + context 'when integration is an instance-level integration' do let(:instance_integration) { create(:jira_integration, :instance) } diff --git a/spec/models/integrations/gitlab_slack_application_spec.rb b/spec/models/integrations/gitlab_slack_application_spec.rb index 8d6366f61f6648..acc12357256450 100644 --- a/spec/models/integrations/gitlab_slack_application_spec.rb +++ b/spec/models/integrations/gitlab_slack_application_spec.rb @@ -334,4 +334,44 @@ def stub_slack_request(channel:, success:) end end end + + describe '#after_build_from_integration' do + let(:slack_integration) { create(:slack_integration) } + let(:initial_integration) do + create(:gitlab_slack_application_integration, slack_integration:) + end + + let(:project) { create(:project) } + let(:new_integration) { build(:gitlab_slack_application_integration, project: project) } + + it 'duplicates slack_integration with correct alias' do + initial_integration.after_build_from_integration(new_integration) + + new_slack_integration = new_integration.slack_integration + expect(new_slack_integration.slack_api_scopes.ids).to match_array(slack_integration.slack_api_scopes.ids) + expect(new_slack_integration).to have_attributes( + alias: project.full_path, + team_id: slack_integration.team_id, + team_name: slack_integration.team_name, + user_id: slack_integration.user_id, + bot_user_id: slack_integration.bot_user_id, + bot_access_token: slack_integration.bot_access_token + ) + end + + context 'when parent integration has no slack_integration' do + let(:initial_integration) do + create(:gitlab_slack_application_integration, slack_integration: nil, active: false) + end + + it 'does not duplicate slack_integration and does not activate the new integration on save' do + new_integration = described_class.build_from_integration(initial_integration, project_id: project.id) + + expect(new_integration.slack_integration).to be_nil + expect(new_integration).not_to be_active + expect { new_integration.save! }.not_to change { SlackIntegration.count } + expect(new_integration).not_to be_active + end + end + end end diff --git a/spec/models/slack_integration_spec.rb b/spec/models/slack_integration_spec.rb index 4766f07270ad6e..f96b4188b07ae2 100644 --- a/spec/models/slack_integration_spec.rb +++ b/spec/models/slack_integration_spec.rb @@ -9,6 +9,12 @@ it { is_expected.to belong_to(:integration) } end + describe 'constant' do + it 'is expected that DEFAULT_ALIAS is defined with the correct value' do + expect(SlackIntegration::INSTANCE_ALIAS).to eq('_gitlab-instance') + end + end + describe 'authorized_scope_names' do subject(:slack_integration) { integration } -- GitLab From b1c38e691076438a4080bfcd7e29e2d641c34fa4 Mon Sep 17 00:00:00 2001 From: Rez Date: Mon, 8 Sep 2025 11:41:37 +0200 Subject: [PATCH 2/2] Apply review comments --- spec/models/integrations/gitlab_slack_application_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/integrations/gitlab_slack_application_spec.rb b/spec/models/integrations/gitlab_slack_application_spec.rb index acc12357256450..fb3247a1f72467 100644 --- a/spec/models/integrations/gitlab_slack_application_spec.rb +++ b/spec/models/integrations/gitlab_slack_application_spec.rb @@ -338,7 +338,7 @@ def stub_slack_request(channel:, success:) describe '#after_build_from_integration' do let(:slack_integration) { create(:slack_integration) } let(:initial_integration) do - create(:gitlab_slack_application_integration, slack_integration:) + create(:gitlab_slack_application_integration, :instance, slack_integration: slack_integration) end let(:project) { create(:project) } -- GitLab