Merge branch '273265-github-and-jenkins-only-at-project-level' into 'master'

GitHub and Jenkins only available at project-level

See merge request gitlab-org/gitlab!46583

app/controllers/admin/application_settings_controller.rb

@@ -53,7 +53,7 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
   def integrations
     return not_found unless instance_level_integrations?
 
-    @integrations = Service.find_or_initialize_all(Service.for_instance).sort_by(&:title)
+    @integrations = Service.find_or_initialize_all_non_project_specific(Service.for_instance).sort_by(&:title)
   end
 
   def update

app/controllers/admin/integrations_controller.rb

@@ -8,8 +8,8 @@ class Admin::IntegrationsController < Admin::ApplicationController
 
   private
 
-  def find_or_initialize_integration(name)
-    Service.find_or_initialize_integration(name, instance: true)
+  def find_or_initialize_non_project_specific_integration(name)
+    Service.find_or_initialize_non_project_specific_integration(name, instance: true)
   end
 
   def integrations_enabled?

app/controllers/concerns/integrations_actions.rb

@@ -52,7 +52,7 @@ module IntegrationsActions
   def integration
     # Using instance variable `@service` still required as it's used in ServiceParams.
     # Should be removed once that is refactored to use `@integration`.
-    @integration = @service ||= find_or_initialize_integration(params[:id]) # rubocop:disable Gitlab/ModuleWithInstanceVariables
+    @integration = @service ||= find_or_initialize_non_project_specific_integration(params[:id]) # rubocop:disable Gitlab/ModuleWithInstanceVariables
   end
 
   def success_message

app/controllers/groups/settings/integrations_controller.rb

@@ -10,7 +10,7 @@ module Groups
       feature_category :integrations
 
       def index
-        @integrations = Service.find_or_initialize_all(Service.for_group(group)).sort_by(&:title)
+        @integrations = Service.find_or_initialize_all_non_project_specific(Service.for_group(group)).sort_by(&:title)
       end
 
       def edit
@@ -21,8 +21,8 @@ module Groups
 
       private
 
-      def find_or_initialize_integration(name)
-        Service.find_or_initialize_integration(name, group_id: group.id)
+      def find_or_initialize_non_project_specific_integration(name)
+        Service.find_or_initialize_non_project_specific_integration(name, group_id: group.id)
       end
 
       def integrations_enabled?

app/graphql/types/projects/service_type_enum.rb

@@ -5,7 +5,7 @@ module Types
     class ServiceTypeEnum < BaseEnum
       graphql_name 'ServiceType'
 
-      ::Service.services_types.each do |service_type|
+      ::Service.available_services_types(include_dev: false).each do |service_type|
         value service_type.underscore.upcase, value: service_type
       end
     end

app/models/project.rb

@@ -1341,8 +1341,7 @@ class Project < ApplicationRecord
   end
 
   def find_or_initialize_services
-    available_services_names =
-      Service.available_services_names + Service.project_specific_services_names - disabled_services
+    available_services_names = Service.available_services_names - disabled_services
 
     available_services_names.map do |service_name|
       find_or_initialize_service(service_name)

app/models/service.rb

@@ -17,6 +17,7 @@ class Service < ApplicationRecord
     pivotaltracker prometheus pushover redmine slack slack_slash_commands teamcity unify_circuit webex_teams youtrack
   ].freeze
 
+  # Fake services to help with local development.
   DEV_SERVICE_NAMES = %w[
     mock_ci mock_deployment mock_monitoring
   ].freeze
@@ -65,9 +66,9 @@ class Service < ApplicationRecord
   scope :by_type, -> (type) { where(type: type) }
   scope :by_active_flag, -> (flag) { where(active: flag) }
   scope :inherit_from_id, -> (id) { where(inherit_from_id: id) }
-  scope :for_group, -> (group) { where(group_id: group, type: available_services_types) }
-  scope :for_template, -> { where(template: true, type: available_services_types) }
-  scope :for_instance, -> { where(instance: true, type: available_services_types) }
+  scope :for_group, -> (group) { where(group_id: group, type: available_services_types(include_project_specific: false)) }
+  scope :for_template, -> { where(template: true, type: available_services_types(include_project_specific: false)) }
+  scope :for_instance, -> { where(instance: true, type: available_services_types(include_project_specific: false)) }
 
   scope :push_hooks, -> { where(push_events: true, active: true) }
   scope :tag_push_hooks, -> { where(tag_push_events: true, active: true) }
@@ -168,13 +169,13 @@ class Service < ApplicationRecord
   end
   private_class_method :create_nonexistent_templates
 
-  def self.find_or_initialize_integration(name, instance: false, group_id: nil)
-    if name.in?(available_services_names)
+  def self.find_or_initialize_non_project_specific_integration(name, instance: false, group_id: nil)
+    if name.in?(available_services_names(include_project_specific: false))
       "#{name}_service".camelize.constantize.find_or_initialize_by(instance: instance, group_id: group_id)
     end
   end
 
-  def self.find_or_initialize_all(scope)
+  def self.find_or_initialize_all_non_project_specific(scope)
     scope + build_nonexistent_services_for(scope)
   end
 
@@ -188,13 +189,14 @@ class Service < ApplicationRecord
   def self.list_nonexistent_services_for(scope)
     # Using #map instead of #pluck to save one query count. This is because
     # ActiveRecord loaded the object here, so we don't need to query again later.
-    available_services_types - scope.map(&:type)
+    available_services_types(include_project_specific: false) - scope.map(&:type)
   end
   private_class_method :list_nonexistent_services_for
 
-  def self.available_services_names
+  def self.available_services_names(include_project_specific: true, include_dev: true)
     service_names = services_names
-    service_names += dev_services_names
+    service_names += project_specific_services_names if include_project_specific
+    service_names += dev_services_names if include_dev
 
     service_names.sort_by(&:downcase)
   end
@@ -213,12 +215,10 @@ class Service < ApplicationRecord
     []
   end
 
-  def self.available_services_types
-    available_services_names.map { |service_name| "#{service_name}_service".camelize }
-  end
-
-  def self.services_types
-    services_names.map { |service_name| "#{service_name}_service".camelize }
+  def self.available_services_types(include_project_specific: true, include_dev: true)
+    available_services_names(include_project_specific: include_project_specific, include_dev: include_dev).map do |service_name|
+      "#{service_name}_service".camelize
+    end
   end
 
   def self.build_from_integration(integration, project_id: nil, group_id: nil)

doc/api/graphql/reference/gitlab_schema.json

@@ -58259,6 +58259,12 @@
               "isDeprecated": false,
               "deprecationReason": null
             },
+            {
+              "name": "GITHUB_SERVICE",
+              "description": null,
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
             {
               "name": "HANGOUTS_CHAT_SERVICE",
               "description": null,
@@ -58277,6 +58283,12 @@
               "isDeprecated": false,
               "deprecationReason": null
             },
+            {
+              "name": "JENKINS_SERVICE",
+              "description": null,
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
             {
               "name": "JIRA_SERVICE",
               "description": null,
@@ -58372,18 +58384,6 @@
               "description": null,
               "isDeprecated": false,
               "deprecationReason": null
-            },
-            {
-              "name": "GITHUB_SERVICE",
-              "description": null,
-              "isDeprecated": false,
-              "deprecationReason": null
-            },
-            {
-              "name": "JENKINS_SERVICE",
-              "description": null,
-              "isDeprecated": false,
-              "deprecationReason": null
             }
           ],
           "possibleTypes": null

ee/app/models/ee/service.rb

@@ -4,28 +4,27 @@ module EE
   module Service
     extend ActiveSupport::Concern
 
-    EE_SERVICE_NAMES = %w[
-      github
-      jenkins
+    EE_COM_PROJECT_SPECIFIC_SERVICE_NAMES = %w[
+      gitlab_slack_application
     ].freeze
 
     EE_PROJECT_SPECIFIC_SERVICE_NAMES = %w[
-      gitlab_slack_application
+      github
+      jenkins
     ].freeze
 
     class_methods do
       extend ::Gitlab::Utils::Override
 
-      override :services_names
-      def services_names
-        super + EE_SERVICE_NAMES
-      end
-
       override :project_specific_services_names
       def project_specific_services_names
-        return super unless ::Gitlab.dev_env_or_com?
+        services = super + EE_PROJECT_SPECIFIC_SERVICE_NAMES
 
-        super + EE_PROJECT_SPECIFIC_SERVICE_NAMES
+        if ::Gitlab.com?
+          services + EE_COM_PROJECT_SPECIFIC_SERVICE_NAMES
+        else
+          services
+        end
       end
     end
   end

ee/spec/models/service_spec.rb

@@ -3,32 +3,32 @@
 require 'spec_helper'
 
 RSpec.describe Service do
-  describe 'Available services' do
-    let(:ee_services) do
-      %w[
-        github
-        jenkins
-      ]
-    end
+  let(:ee_project_services) do
+    %w[
+      github
+      jenkins
+    ]
+  end
 
-    it { expect(described_class.available_services_names).to include(*ee_services) }
+  describe '.available_services_names' do
+    it { expect(described_class.available_services_names).to include(*ee_project_services) }
   end
 
   describe '.project_specific_services_names' do
     before do
-      allow(::Gitlab).to receive(:dev_env_or_com?).and_return(dev_env_or_com)
+      allow(::Gitlab).to receive(:com?).and_return(com)
     end
 
-    context 'when not on gitlab.com and not in development environment' do
-      let(:dev_env_or_com) { false }
+    context 'when not on gitlab.com' do
+      let(:com) { false }
 
-      it { expect(described_class.project_specific_services_names).to eq([]) }
+      it { expect(described_class.project_specific_services_names).to match_array(ee_project_services) }
     end
 
-    context 'when on gitlab.com or in dev environment' do
-      let(:dev_env_or_com) { true }
+    context 'when on gitlab.com' do
+      let(:com) { true }
 
-      it { expect(described_class.project_specific_services_names).to eq(%w[gitlab_slack_application]) }
+      it { expect(described_class.project_specific_services_names).to match_array(ee_project_services + ['gitlab_slack_application']) }
     end
   end
 end

lib/gitlab/auth.rb

@@ -158,7 +158,7 @@ module Gitlab
 
         if Service.available_services_names.include?(underscored_service)
           # We treat underscored_service as a trusted input because it is included
-          # in the Service.available_services_names whitelist.
+          # in the Service.available_services_names allowlist.
           service = project.public_send("#{underscored_service}_service") # rubocop:disable GitlabSecurity/PublicSend
 
           if service && service.activated? && service.valid_token?(password)

spec/controllers/groups/settings/integrations_controller_spec.rb

@@ -46,7 +46,7 @@ RSpec.describe Groups::Settings::IntegrationsController do
   describe '#edit' do
     context 'when user is not owner' do
       it 'renders not_found' do
-        get :edit, params: { group_id: group, id: Service.available_services_names.sample }
+        get :edit, params: { group_id: group, id: Service.available_services_names(include_project_specific: false).sample }
 
         expect(response).to have_gitlab_http_status(:not_found)
       end
@@ -61,13 +61,13 @@ RSpec.describe Groups::Settings::IntegrationsController do
         it 'returns not_found' do
           stub_feature_flags(group_level_integrations: false)
 
-          get :edit, params: { group_id: group, id: Service.available_services_names.sample }
+          get :edit, params: { group_id: group, id: Service.available_services_names(include_project_specific: false).sample }
 
           expect(response).to have_gitlab_http_status(:not_found)
         end
       end
 
-      Service.available_services_names.each do |integration_name|
+      Service.available_services_names(include_project_specific: false).each do |integration_name|
         context "#{integration_name}" do
           it 'successfully displays the template' do
             get :edit, params: { group_id: group, id: integration_name }

spec/graphql/types/projects/services_enum_spec.rb

@@ -11,5 +11,5 @@ RSpec.describe GitlabSchema.types['ServiceType'] do
 end
 
 def available_services_enum
-  ::Service.services_types.map(&:underscore).map(&:upcase)
+  ::Service.available_services_types(include_dev: false).map(&:underscore).map(&:upcase)
 end

spec/models/project_spec.rb

@@ -5551,15 +5551,16 @@ RSpec.describe Project, factory_default: :keep do
   end
 
   describe '#find_or_initialize_services' do
-    it 'returns only enabled services' do
+    before do
       allow(Service).to receive(:available_services_names).and_return(%w[prometheus pushover teamcity])
-      allow(Service).to receive(:project_specific_services_names).and_return(%w[asana])
       allow(subject).to receive(:disabled_services).and_return(%w[prometheus])
+    end
 
+    it 'returns only enabled services' do
       services = subject.find_or_initialize_services
 
-      expect(services.count).to eq(3)
-      expect(services.map(&:title)).to eq(['Asana', 'JetBrains TeamCity CI', 'Pushover'])
+      expect(services.count).to eq(2)
+      expect(services.map(&:title)).to eq(['JetBrains TeamCity CI', 'Pushover'])
     end
   end
 

spec/models/service_spec.rb

@@ -208,27 +208,27 @@ RSpec.describe Service do
     end
   end
 
-  describe '.find_or_initialize_integration' do
+  describe '.find_or_initialize_non_project_specific_integration' do
     let!(:service1) { create(:jira_service, project_id: nil, group_id: group.id) }
     let!(:service2) { create(:jira_service) }
 
     it 'returns the right service' do
-      expect(Service.find_or_initialize_integration('jira', group_id: group)).to eq(service1)
+      expect(Service.find_or_initialize_non_project_specific_integration('jira', group_id: group)).to eq(service1)
     end
 
     it 'does not create a new service' do
-      expect { Service.find_or_initialize_integration('redmine', group_id: group) }.not_to change { Service.count }
+      expect { Service.find_or_initialize_non_project_specific_integration('redmine', group_id: group) }.not_to change { Service.count }
     end
   end
 
-  describe '.find_or_initialize_all' do
+  describe '.find_or_initialize_all_non_project_specific' do
     shared_examples 'service instances' do
       it 'returns the available service instances' do
-        expect(Service.find_or_initialize_all(Service.for_instance).pluck(:type)).to match_array(Service.available_services_types)
+        expect(Service.find_or_initialize_all_non_project_specific(Service.for_instance).pluck(:type)).to match_array(Service.available_services_types(include_project_specific: false))
       end
 
       it 'does not create service instances' do
-        expect { Service.find_or_initialize_all(Service.for_instance) }.not_to change { Service.count }
+        expect { Service.find_or_initialize_all_non_project_specific(Service.for_instance) }.not_to change { Service.count }
       end
     end
 
@@ -237,7 +237,7 @@ RSpec.describe Service do
     context 'with all existing instances' do
       before do
         Service.insert_all(
-          Service.available_services_types.map { |type| { instance: true, type: type } }
+          Service.available_services_types(include_project_specific: false).map { |type| { instance: true, type: type } }
         )
       end
 
@@ -265,13 +265,13 @@ RSpec.describe Service do
   describe 'template' do
     shared_examples 'retrieves service templates' do
       it 'returns the available service templates' do
-        expect(Service.find_or_create_templates.pluck(:type)).to match_array(Service.available_services_types)
+        expect(Service.find_or_create_templates.pluck(:type)).to match_array(Service.available_services_types(include_project_specific: false))
       end
     end
 
     describe '.find_or_create_templates' do
       it 'creates service templates' do
-        expect { Service.find_or_create_templates }.to change { Service.count }.from(0).to(Service.available_services_names.size)
+        expect { Service.find_or_create_templates }.to change { Service.count }.from(0).to(Service.available_services_names(include_project_specific: false).size)
       end
 
       it_behaves_like 'retrieves service templates'
@@ -279,7 +279,7 @@ RSpec.describe Service do
       context 'with all existing templates' do
         before do
           Service.insert_all(
-            Service.available_services_types.map { |type| { template: true, type: type } }
+            Service.available_services_types(include_project_specific: false).map { |type| { template: true, type: type } }
           )
         end
 
@@ -305,7 +305,7 @@ RSpec.describe Service do
         end
 
         it 'creates the rest of the service templates' do
-          expect { Service.find_or_create_templates }.to change { Service.count }.from(1).to(Service.available_services_names.size)
+          expect { Service.find_or_create_templates }.to change { Service.count }.from(1).to(Service.available_services_names(include_project_specific: false).size)
         end
 
         it_behaves_like 'retrieves service templates'
@@ -891,4 +891,30 @@ RSpec.describe Service do
       end
     end
   end
+
+  describe '.available_services_names' do
+    it 'calls the right methods' do
+      expect(described_class).to receive(:services_names).and_call_original
+      expect(described_class).to receive(:dev_services_names).and_call_original
+      expect(described_class).to receive(:project_specific_services_names).and_call_original
+
+      described_class.available_services_names
+    end
+
+    it 'does not call project_specific_services_names with include_project_specific false' do
+      expect(described_class).to receive(:services_names).and_call_original
+      expect(described_class).to receive(:dev_services_names).and_call_original
+      expect(described_class).not_to receive(:project_specific_services_names)
+
+      described_class.available_services_names(include_project_specific: false)
+    end
+
+    it 'does not call dev_services_names with include_dev false' do
+      expect(described_class).to receive(:services_names).and_call_original
+      expect(described_class).not_to receive(:dev_services_names)
+      expect(described_class).to receive(:project_specific_services_names).and_call_original
+
+      described_class.available_services_names(include_dev: false)
+    end
+  end
 end