diff --git a/.rubocop_todo/gitlab/bounded_contexts.yml b/.rubocop_todo/gitlab/bounded_contexts.yml index 5c9ae67889a1085f8aa0db25c794fea03ccaedaa..47ac030b97584749f569757156d98c09042fe074 100644 --- a/.rubocop_todo/gitlab/bounded_contexts.yml +++ b/.rubocop_todo/gitlab/bounded_contexts.yml @@ -1713,6 +1713,7 @@ Gitlab/BoundedContexts: - 'app/services/jira_connect_installations/update_service.rb' - 'app/services/jira_connect_subscriptions/base_service.rb' - 'app/services/jira_connect_subscriptions/create_service.rb' + - 'app/services/jira_connect_subscriptions/destroy_service.rb' - 'app/services/jira_import/cloud_users_mapper_service.rb' - 'app/services/jira_import/server_users_mapper_service.rb' - 'app/services/jira_import/start_import_service.rb' diff --git a/app/controllers/jira_connect/subscriptions_controller.rb b/app/controllers/jira_connect/subscriptions_controller.rb index 17a79f83a7864e3db7df37581f1602dfaa297929..6885e5560a5a8364ef4883431c8669b83f6cd999 100644 --- a/app/controllers/jira_connect/subscriptions_controller.rb +++ b/app/controllers/jira_connect/subscriptions_controller.rb @@ -46,14 +46,12 @@ def create end def destroy - subscription = current_jira_installation.subscriptions.find(params[:id]) + result = destroy_service.execute - if !jira_user&.jira_admin? - render json: { error: 'forbidden' }, status: :forbidden - elsif subscription.destroy + if result.success? render json: { success: true } else - render json: { error: subscription.errors.full_messages.join(', ') }, status: :unprocessable_entity + render json: { error: result.message }, status: result[:reason] end end @@ -70,6 +68,12 @@ def create_service JiraConnectSubscriptions::CreateService.new(current_jira_installation, current_user, namespace_path: params['namespace_path'], jira_user: jira_user) end + def destroy_service + subscription = current_jira_installation.subscriptions.find(params[:id]) + + JiraConnectSubscriptions::DestroyService.new(subscription, jira_user) + end + def allow_rendering_in_iframe response.headers.delete('X-Frame-Options') end diff --git a/app/services/jira_connect_subscriptions/create_service.rb b/app/services/jira_connect_subscriptions/create_service.rb index 5ae4c5ffd1cfd849e9d083937531a6dcada1ebb1..1e22bf825876787999fcb631dfd1e77ae016d4cd 100644 --- a/app/services/jira_connect_subscriptions/create_service.rb +++ b/app/services/jira_connect_subscriptions/create_service.rb @@ -5,6 +5,7 @@ class CreateService < ::JiraConnectSubscriptions::BaseService include Gitlab::Utils::StrongMemoize MERGE_REQUEST_SYNC_BATCH_SIZE = 20 MERGE_REQUEST_SYNC_BATCH_DELAY = 1.minute.freeze + BATCH_SIZE = 1_000 def execute if !params[:jira_user] @@ -32,6 +33,7 @@ def create_subscription subscription = JiraConnectSubscription.new(installation: jira_connect_installation, namespace: namespace) if subscription.save + create_jira_cloud_integration! schedule_sync_project_jobs success @@ -40,6 +42,38 @@ def create_subscription end end + # We must make all GitLab for Jira app integrations active (or inactive in the DestroyService) + # regardless of whether those integration inherit, or have defined their own custom settings. + # Unless the group namespace is linked in Jira, + # the project integrations do not work, even if they are non-inheriting. + # + # Using Integration.descendants_from_self_or_ancestors_from we update + # all integrations of all subgroups and sub projects to be active. + # + # We keep their inherit_from_id in tact, as they might have custom service_ids fields. + # We also still queue a PropagateIntegrationWorker in order to create integrations + # (the Integration.descendants_from_self_or_ancestors_from only updates existing ones). + def create_jira_cloud_integration! + return unless Feature.enabled?(:enable_jira_connect_configuration) # rubocop:disable Gitlab/FeatureFlagWithoutActor -- flag must be global + + integration = Integration.find_or_initialize_non_project_specific_integration( + 'jira_cloud_app', + group_id: namespace.id + ) + + Integrations::JiraCloudApp.transaction do + integration.inherit_from_id = nil + integration.activate! + + Integration.descendants_from_self_or_ancestors_from(integration).each_batch(of: BATCH_SIZE) do |records| + records.update!(active: true) + end + end + + # This worker must be queued outside of the PostgreSQL transaction. + PropagateIntegrationWorker.perform_async(integration.id) if integration.persisted? + end + def namespace strong_memoize(:namespace) do Namespace.find_by_full_path(params[:namespace_path]) diff --git a/app/services/jira_connect_subscriptions/destroy_service.rb b/app/services/jira_connect_subscriptions/destroy_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..6dd61ef3ccba31d7696cfa49f0995ba532f65f49 --- /dev/null +++ b/app/services/jira_connect_subscriptions/destroy_service.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +module JiraConnectSubscriptions + class DestroyService + BATCH_SIZE = 1_000 + + attr_accessor :subscription, :jira_user + + def initialize(subscription, jira_user) + @subscription = subscription + @jira_user = jira_user + end + + def execute + unless subscription + return ServiceResponse.error(message: _('Invalid JiraConnectSubscriptions'), + reason: :unprocessable_entity) + end + + return ServiceResponse.error(message: _('Forbidden'), reason: :forbidden) unless can_administer_jira? + + deactivate_jira_cloud_app_integrations! + + return ServiceResponse.success if subscription.destroy + + ServiceResponse.error( + message: subscription.errors.full_messages.to_sentence, + reason: :unprocessable_entity + ) + end + + private + + def can_administer_jira? + jira_user&.jira_admin? + end + + def deactivate_jira_cloud_app_integrations! + return unless Feature.enabled?(:enable_jira_connect_configuration) # rubocop:disable Gitlab/FeatureFlagWithoutActor -- flag must be global + + integration = Integrations::JiraCloudApp.for_group(@subscription.namespace_id).first + + return unless integration + + Integrations::JiraCloudApp.transaction do + integration.inherit_from_id = nil + integration.deactivate! + Integration.descendants_from_self_or_ancestors_from(integration).each_batch(of: BATCH_SIZE) do |records| + records.update!(active: false) + end + end + end + end +end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 8655c2c9c327a67e5911d5b215e668a36e32606c..cb6ade9409d30394916fa17d5bc8498614960897 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -28325,6 +28325,9 @@ msgstr "" msgid "Invalid Insights config file detected" msgstr "" +msgid "Invalid JiraConnectSubscriptions" +msgstr "" + msgid "Invalid OS" msgstr "" diff --git a/spec/controllers/jira_connect/subscriptions_controller_spec.rb b/spec/controllers/jira_connect/subscriptions_controller_spec.rb index a05f18f1a16499a850f56406f66a6a3df4422283..335ee3f84ca7dc391da58d557926bfd9ecd00dbf 100644 --- a/spec/controllers/jira_connect/subscriptions_controller_spec.rb +++ b/spec/controllers/jira_connect/subscriptions_controller_spec.rb @@ -77,6 +77,7 @@ before do group.add_maintainer(user) + allow(PropagateIntegrationWorker).to receive(:perform_async) end subject { post :create, params: { jwt: jwt, namespace_path: group.path, format: :json } } diff --git a/spec/requests/jira_connect/subscriptions_controller_spec.rb b/spec/requests/jira_connect/subscriptions_controller_spec.rb index 8b019970b61ed13e14efbd556cbe47960dd6ef50..af11c49b36d1413d15b7e6c0bdade044c7cf0481 100644 --- a/spec/requests/jira_connect/subscriptions_controller_spec.rb +++ b/spec/requests/jira_connect/subscriptions_controller_spec.rb @@ -95,6 +95,7 @@ describe 'DELETE /-/jira_connect/subscriptions/:id' do let_it_be(:installation) { create(:jira_connect_installation, instance_url: 'http://self-managed-gitlab.com') } let_it_be(:subscription) { create(:jira_connect_subscription, installation: installation) } + let(:stub_service_response) { ::ServiceResponse.success } let(:qsh) do Atlassian::Jwt.create_query_string_hash('https://gitlab.test/subscriptions', 'GET', 'https://gitlab.test') @@ -106,6 +107,9 @@ before do stub_application_setting(jira_connect_proxy_url: 'https://gitlab.com') + allow_next_instance_of(JiraConnectSubscriptions::DestroyService) do |service| + allow(service).to receive(:execute).and_return(stub_service_response) + end end it 'allows cross-origin requests', :aggregate_failures do @@ -114,6 +118,21 @@ expect(response.headers['Access-Control-Allow-Origin']).to eq 'https://gitlab.com' expect(response.headers['Access-Control-Allow-Methods']).to eq 'DELETE, OPTIONS' expect(response.headers['Access-Control-Allow-Credentials']).to be_nil + expect(response).to have_gitlab_http_status(:ok) + end + + context 'when the service responds with an error' do + let(:stub_service_response) { ::ServiceResponse.error(message: 'some error', reason: :unprocessable_entity) } + + it 'rejects request with status-code', :aggregate_failures do + delete "/-/jira_connect/subscriptions/#{subscription.id}", params: params, headers: cors_request_headers + + expect(response.headers['Access-Control-Allow-Origin']).to eq 'https://gitlab.com' + expect(response.headers['Access-Control-Allow-Methods']).to eq 'DELETE, OPTIONS' + expect(response.headers['Access-Control-Allow-Credentials']).to be_nil + expect(response.body).to eq "{\"error\":\"some error\"}" + expect(response).to have_gitlab_http_status(:unprocessable_entity) + end end end end diff --git a/spec/services/jira_connect_subscriptions/create_service_spec.rb b/spec/services/jira_connect_subscriptions/create_service_spec.rb index 7f3ff975a647b496d6b784f9ea3081d61737caf3..b029e57944f81aa2f23dd69ebb4348435751b62b 100644 --- a/spec/services/jira_connect_subscriptions/create_service_spec.rb +++ b/spec/services/jira_connect_subscriptions/create_service_spec.rb @@ -22,6 +22,10 @@ expect(subject[:status]).to eq(:error) expect(subject).to include(status_attributes) end + + it 'does not create jira cloud app integration' do + expect { subject }.not_to change { Integration.count } + end end context 'remote user does not have access' do @@ -41,12 +45,38 @@ end context 'when user does have access' do + before do + allow(PropagateIntegrationWorker).to receive(:perform_async) + end + it 'creates a subscription' do expect { subject }.to change { installation.subscriptions.count }.from(0).to(1) + expect(subject[:status]).to eq(:success) + end + + it 'creates an active jira cloud app integration for the group and returns success' do + expect { subject }.to change { Integrations::JiraCloudApp.for_group(group).count }.from(0).to(1) + expect(subject[:status]).to eq(:success) + + expect(Integrations::JiraCloudApp.for_group(group).first).to be_active + expect(PropagateIntegrationWorker).to have_received(:perform_async) end - it 'returns success' do + it 'does not create integration when feature flag is disabled and returns success' do + stub_feature_flags(enable_jira_connect_configuration: false) + expect(subject[:status]).to eq(:success) + + expect { subject }.not_to change { Integrations::JiraCloudApp.count } + expect(PropagateIntegrationWorker).not_to have_received(:perform_async) + end + + context 'when group has an existing inactive integration' do + let_it_be(:integration) { create(:jira_cloud_app_integration, :group, :inactive, group: group) } + + it 'activates the integration' do + expect { subject }.to change { integration.reload.active }.to eq(true) + end end context 'namespace has projects' do @@ -66,6 +96,50 @@ subject end end + + context 'when project has non-inheriting inactive jira cloud app integration' do + let_it_be(:project_1) { create(:project, group: group) } + + let_it_be(:project_integration) do + create(:jira_cloud_app_integration, :inactive, project: project_1, inherit_from_id: nil) + end + + it 'activates the integration, but keeps it as non-inheriting' do + expect { subject }.to change { project_integration.reload.active }.to eq(true) + expect(project_integration.inherit_from_id).to be_nil + end + end + end + + context 'when group has inheriting inactive jira cloud app integrations' do + let_it_be(:subgroup_1) { create(:group, parent: group) } + let_it_be(:subgroup_2) { create(:group, parent: group) } + let_it_be(:project_1) { create(:project, group: subgroup_1) } + let_it_be(:group_integration) { create(:jira_cloud_app_integration, :group, :inactive, group: group) } + let_it_be(:jira_integration) do + create(:jira_integration, :group, :inactive, group: group, inherit_from_id: group_integration.id) + end + + let_it_be(:subgroup_integration_1) do + create(:jira_cloud_app_integration, :group, :inactive, group: subgroup_1, inherit_from_id: group_integration.id) + end + + let_it_be(:project_integration_1) do + create(:jira_cloud_app_integration, :inactive, project: project_1, inherit_from_id: group_integration.id) + end + + before do + allow(PropagateIntegrationWorker).to receive(:perform_async) + end + + it 'activate existing jira cloud app integrations if subscription saved successfully' do + expect(subject[:status]).to eq(:success) + + expect(group_integration.reload).to be_active + expect(subgroup_integration_1.reload).to be_active + expect(project_integration_1.reload).to be_active + expect(jira_integration.reload).not_to be_active + end end context 'when path is invalid' do diff --git a/spec/services/jira_connect_subscriptions/destroy_service_spec.rb b/spec/services/jira_connect_subscriptions/destroy_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..7a407af2ad529ded7dd7dfdd7cf9dd06786bd275 --- /dev/null +++ b/spec/services/jira_connect_subscriptions/destroy_service_spec.rb @@ -0,0 +1,97 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe JiraConnectSubscriptions::DestroyService, feature_category: :integrations do + describe '#execute' do + let_it_be(:group) { create(:group, :public, path: 'group') } + + let_it_be(:installation) { create(:jira_connect_installation) } + let_it_be_with_refind(:subscription) do + create(:jira_connect_subscription, installation: installation, namespace: group) + end + + let(:jira_user_is_admin) { true } + let(:jira_user) { instance_double(Atlassian::JiraConnect::JiraUser, jira_admin?: jira_user_is_admin) } + + subject(:result) { described_class.new(subscription, jira_user).execute } + + context 'when subscription namespace has descendants with inheriting Jira Cloud app integration' do + let_it_be(:subgroup_1) { create(:group, parent: group) } + let_it_be(:subgroup_2) { create(:group, parent: group) } + let_it_be(:project_1) { create(:project, group: subgroup_1) } + + let_it_be(:group_integration) { create(:jira_cloud_app_integration, :group, group: group) } + let_it_be(:non_inheriting_asana_integration) do + create(:asana_integration, :group, group: subgroup_2, inherit_from_id: nil) + end + + let_it_be(:non_inheriting_jira_cloud_app_integration) do + create(:jira_cloud_app_integration, :group, group: subgroup_2, inherit_from_id: nil) + end + + let_it_be(:inheriting_jira_cloud_app_integration) do + create(:jira_cloud_app_integration, :group, group: subgroup_1, inherit_from_id: group_integration.id) + end + + let_it_be(:inheriting_jira_cloud_app_project_integration) do + create(:jira_cloud_app_integration, project: project_1, inherit_from_id: group_integration.id) + end + + it 'destroys the subscription, deactivates the integration, and schedules PropagateIntegrationWorker' do + expect { result }.to change { JiraConnectSubscription.count }.by(-1) + expect(inheriting_jira_cloud_app_integration.reload).not_to be_active + expect(inheriting_jira_cloud_app_project_integration.reload).not_to be_active + expect(non_inheriting_jira_cloud_app_integration.reload).not_to be_active + expect(non_inheriting_asana_integration.reload).to be_active + expect(result).to be_success + end + end + + context 'when subscription namespace does not have a Jira Cloud app integration' do + let_it_be(:other_integration) { create(:jira_cloud_app_integration) } + + it 'destroys the subscription but does not schedule PropagateIntegrationWorker' do + expect { result }.to change { JiraConnectSubscription.count }.by(-1) + expect(other_integration.reload).to be_active + expect(result).to be_success + end + end + + context 'when destroy fails' do + before do + allow(subscription).to receive(:destroy).and_return(false) + end + + it 'returns an error' do + expect(Integration).not_to receive(:descendants_from_self_or_ancestors_from) + + expect { result }.not_to change { JiraConnectSubscription.count } + expect(result).to be_error + end + end + + context 'when subscription is nil' do + subject(:result) { described_class.new(nil, jira_user).execute } + + it 'returns an error' do + expect(Integration).not_to receive(:descendants_from_self_or_ancestors_from) + + expect { result }.not_to change { JiraConnectSubscription.count } + expect(result).to be_error + end + end + + context 'when the Jira user is not an admin' do + let(:jira_user_is_admin) { false } + + it 'returns an error with a forbidden message' do + expect(Integration).not_to receive(:descendants_from_self_or_ancestors_from) + + expect { result }.not_to change { JiraConnectSubscription.count } + expect(result).to be_error + expect(result.message).to eq('Forbidden') + end + end + end +end