Check instance cluster feature at policy level

Try to simplify feature flag checks by using policies

10 files changed, 107 insertions, 36 deletions

diff --git a/app/controllers/admin/application_controller.rb b/app/controllers/admin/application_controller.rb
index ef182b981f1..b742b7e19cf 100644
--- a/app/controllers/admin/application_controller.rb
+++ b/app/controllers/admin/application_controller.rb
@@ -4,10 +4,7 @@
 #
 # Automatically sets the layout and ensures an administrator is logged in
 class Admin::ApplicationController < ApplicationController
-  before_action :authenticate_admin!
-  layout 'admin'
+  include EnforcesAdminAuthentication
 
-  def authenticate_admin!
-    render_404 unless current_user.admin?
-  end
+  layout 'admin'
 end
diff --git a/app/controllers/admin/clusters/applications_controller.rb b/app/controllers/admin/clusters/applications_controller.rb
index 3351d3ff825..7400cc16175 100644
--- a/app/controllers/admin/clusters/applications_controller.rb
+++ b/app/controllers/admin/clusters/applications_controller.rb
@@ -1,6 +1,8 @@
 # frozen_string_literal: true
 
 class Admin::Clusters::ApplicationsController < Clusters::ApplicationsController
+  include EnforcesAdminAuthentication
+
   private
 
   def clusterable
diff --git a/app/controllers/admin/clusters_controller.rb b/app/controllers/admin/clusters_controller.rb
index f2b54125d48..f54933de10f 100644
--- a/app/controllers/admin/clusters_controller.rb
+++ b/app/controllers/admin/clusters_controller.rb
@@ -1,25 +1,13 @@
 # frozen_string_literal: true
 
 class Admin::ClustersController < Clusters::ClustersController
-  prepend_before_action :check_instance_clusters_feature_flag!
+  include EnforcesAdminAuthentication
 
   layout 'admin'
 
   private
 
   def clusterable
-    @clusterable ||= InstanceClusterablePresenter.fabricate(cluster_instance, current_user: current_user)
-  end
-
-  def cluster_instance
-    @cluster_instance ||= Clusters::Instance.new
-  end
-
-  def check_instance_clusters_feature_flag!
-    render_404 unless instance_clusters_enabled?
-  end
-
-  def instance_clusters_enabled?
-    cluster_instance.instance_clusters_enabled?
+    @clusterable ||= InstanceClusterablePresenter.fabricate(Clusters::Instance.new, current_user: current_user)
   end
 end
diff --git a/app/controllers/concerns/enforces_admin_authentication.rb b/app/controllers/concerns/enforces_admin_authentication.rb
new file mode 100644
index 00000000000..3ef92730df6
--- /dev/null
+++ b/app/controllers/concerns/enforces_admin_authentication.rb
@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+# == EnforcesAdminAuthentication
+#
+# Controller concern to enforce that users are authenticated as admins
+#
+# Upon inclusion, adds `authenticate_admin!` as a before_action
+#
+module EnforcesAdminAuthentication
+  extend ActiveSupport::Concern
+
+  included do
+    before_action :authenticate_admin!
+  end
+
+  def authenticate_admin!
+    render_404 unless current_user.admin?
+  end
+end
diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb
index 5995ef57e26..971d1052824 100644
--- a/app/helpers/application_settings_helper.rb
+++ b/app/helpers/application_settings_helper.rb
@@ -286,4 +286,8 @@ module ApplicationSettingsHelper
   def expanded_by_default?
     Rails.env.test?
   end
+
+  def instance_clusters_enabled?
+    can?(current_user, :read_cluster, Clusters::Instance.new)
+  end
 end
diff --git a/app/policies/clusters/instance_policy.rb b/app/policies/clusters/instance_policy.rb
index f72096e8fc6..0818a5b2604 100644
--- a/app/policies/clusters/instance_policy.rb
+++ b/app/policies/clusters/instance_policy.rb
@@ -6,8 +6,9 @@ module Clusters
 
     condition(:has_clusters, scope: :subject) { clusterable_has_clusters? }
     condition(:can_have_multiple_clusters) { multiple_clusters_available? }
+    condition(:instance_clusters_enabled, scope: :subject) { @subject.instance_clusters_enabled? }
 
-    rule { admin }.policy do
+    rule { admin & instance_clusters_enabled }.policy do
       enable :read_cluster
       enable :add_cluster
       enable :create_cluster
diff --git a/app/views/layouts/nav/sidebar/_admin.html.haml b/app/views/layouts/nav/sidebar/_admin.html.haml
index 4e96b904ad1..04d67e024ba 100644
--- a/app/views/layouts/nav/sidebar/_admin.html.haml
+++ b/app/views/layouts/nav/sidebar/_admin.html.haml
@@ -132,17 +132,18 @@
                 = _('Abuse Reports')
               %span.badge.badge-pill.count.merge_counter.js-merge-counter.fly-out-badge= number_with_delimiter(AbuseReport.count(:all))
 
-      = nav_link(controller: :clusters) do
-        = link_to admin_clusters_path do
-          .nav-icon-container
-            = sprite_icon('cloud-gear')
-          %span.nav-item-name
-            = _('Kubernetes')
-        %ul.sidebar-sub-level-items.is-fly-out-only
-          = nav_link(controller: :clusters, html_options: { class: "fly-out-top-item" } ) do
-            = link_to admin_clusters_path do
-              %strong.fly-out-top-item-name
-                = _('Kubernetes')
+      - if instance_clusters_enabled?
+        = nav_link(controller: :clusters) do
+          = link_to admin_clusters_path do
+            .nav-icon-container
+              = sprite_icon('cloud-gear')
+            %span.nav-item-name
+              = _('Kubernetes')
+          %ul.sidebar-sub-level-items.is-fly-out-only
+            = nav_link(controller: :clusters, html_options: { class: "fly-out-top-item" } ) do
+              = link_to admin_clusters_path do
+                %strong.fly-out-top-item-name
+                  = _('Kubernetes')
 
       - if akismet_enabled?
         = nav_link(controller: :spam_logs) do
diff --git a/spec/controllers/admin/clusters/applications_controller_spec.rb b/spec/controllers/admin/clusters/applications_controller_spec.rb
index cf202d88acc..76f261e7d3f 100644
--- a/spec/controllers/admin/clusters/applications_controller_spec.rb
+++ b/spec/controllers/admin/clusters/applications_controller_spec.rb
@@ -13,6 +13,16 @@ describe Admin::Clusters::ApplicationsController do
     it { expect { subject }.to be_allowed_for(:admin) }
     it { expect { subject }.to be_denied_for(:user) }
     it { expect { subject }.to be_denied_for(:external) }
+
+    context 'when instance clusters are disabled' do
+      before do
+        stub_feature_flags(instance_clusters: false)
+      end
+
+      it 'returns 404' do
+        is_expected.to have_http_status(:not_found)
+      end
+    end
   end
 
   let(:cluster) { create(:cluster, :instance, :provided_by_gcp) }
diff --git a/spec/controllers/concerns/enforces_admin_authentication_spec.rb b/spec/controllers/concerns/enforces_admin_authentication_spec.rb
new file mode 100644
index 00000000000..9025293f9ea
--- /dev/null
+++ b/spec/controllers/concerns/enforces_admin_authentication_spec.rb
@@ -0,0 +1,38 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe EnforcesAdminAuthentication do
+  let(:user) { create(:user) }
+
+  before do
+    sign_in(user)
+  end
+
+  controller(ApplicationController) do
+    # `described_class` is not available in this context
+    include EnforcesAdminAuthentication # rubocop:disable RSpec/DescribedClass
+
+    def index
+      head :ok
+    end
+  end
+
+  describe 'authenticate_admin!' do
+    context 'as an admin' do
+      let(:user) { create(:admin) }
+
+      it 'renders ok' do
+        get :index
+        expect(response).to have_gitlab_http_status(200)
+      end
+    end
+
+    context 'as a user' do
+      it 'renders a 404' do
+        get :index
+        expect(response).to have_gitlab_http_status(404)
+      end
+    end
+  end
+end
diff --git a/spec/policies/clusters/instance_policy_spec.rb b/spec/policies/clusters/instance_policy_spec.rb
index f4652c2ad00..9d755c6d29d 100644
--- a/spec/policies/clusters/instance_policy_spec.rb
+++ b/spec/policies/clusters/instance_policy_spec.rb
@@ -3,9 +3,8 @@
 require 'spec_helper'
 
 describe Clusters::InstancePolicy do
-  let(:cluster) { create(:cluster, :instance) }
   let(:user) { create(:user) }
-  let(:policy) { described_class.new(user, cluster) }
+  let(:policy) { described_class.new(user, Clusters::Instance.new) }
 
   describe 'rules' do
     context 'when user' do
@@ -17,9 +16,21 @@ describe Clusters::InstancePolicy do
     context 'when admin' do
       let(:user) { create(:admin) }
 
-      it { expect(policy).to be_allowed :read_cluster }
-      it { expect(policy).to be_allowed :update_cluster }
-      it { expect(policy).to be_allowed :admin_cluster }
+      context 'with instance_level_clusters enabled' do
+        it { expect(policy).to be_allowed :read_cluster }
+        it { expect(policy).to be_allowed :update_cluster }
+        it { expect(policy).to be_allowed :admin_cluster }
+      end
+
+      context 'with instance_level_clusters disabled' do
+        before do
+          stub_feature_flags(instance_clusters: false)
+        end
+
+        it { expect(policy).to be_disallowed :read_cluster }
+        it { expect(policy).to be_disallowed :update_cluster }
+        it { expect(policy).to be_disallowed :admin_cluster }
+      end
     end
   end
 end