Merge branch 'use-unique-feature-flag-for-instance-security-dashboard' into 'master'

Use unique feature name for Instance Sec Dashboard

See merge request gitlab-org/gitlab!24505

db/migrate/20200212014653_rename_security_dashboard_feature_flag_to_instance_security_dashboard.rb

+# frozen_string_literal: true
+
+class RenameSecurityDashboardFeatureFlagToInstanceSecurityDashboard < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  class FeatureGate < ApplicationRecord
+    self.table_name = 'feature_gates'
+  end
+
+  def up
+    security_dashboard_feature = FeatureGate.find_by(feature_key: :security_dashboard, key: :boolean)
+
+    if security_dashboard_feature.present?
+      FeatureGate.safe_find_or_create_by!(
+        feature_key: :instance_security_dashboard,
+        key: :boolean,
+        value: security_dashboard_feature.value
+      )
+    end
+  end
+
+  def down
+    FeatureGate.find_by(feature_key: :instance_security_dashboard, key: :boolean)&.delete
+  end
+end

db/post_migrate/20200214034836_remove_security_dashboard_feature_flag.rb

+# frozen_string_literal: true
+
+class RemoveSecurityDashboardFeatureFlag < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  class FeatureGate < ApplicationRecord
+    self.table_name = 'feature_gates'
+  end
+
+  def up
+    FeatureGate.find_by(feature_key: :security_dashboard, key: :boolean)&.delete
+  end
+
+  def down
+    instance_security_dashboard_feature = FeatureGate.find_by(feature_key: :instance_security_dashboard, key: :boolean)
+
+    if instance_security_dashboard_feature.present?
+      FeatureGate.safe_find_or_create_by!(
+        feature_key: :security_dashboard,
+        key: instance_security_dashboard_feature.key,
+        value: instance_security_dashboard_feature.value
+      )
+    end
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2020_02_13_220211) do
+ActiveRecord::Schema.define(version: 2020_02_14_034836) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "pg_trgm"

ee/app/assets/javascripts/pages/security/dashboard/show/index.js

@@ -5,7 +5,7 @@ import syncWithRouter from 'ee/security_dashboard/store/plugins/sync_with_router
 import createStore from 'ee/security_dashboard/store';
 import InstanceSecurityDashboard from 'ee/security_dashboard/components/instance_security_dashboard.vue';
 
-if (gon.features && gon.features.securityDashboard) {
+if (gon.features && gon.features.instanceSecurityDashboard) {
   document.addEventListener('DOMContentLoaded', () => {
     const el = document.querySelector('#js-security');
     const {

ee/app/controllers/security/application_controller.rb

@@ -6,13 +6,13 @@ module Security
 
     before_action :check_feature_enabled!
     before_action do
-      push_frontend_feature_flag(:security_dashboard, default_enabled: true)
+      push_frontend_feature_flag(:instance_security_dashboard, default_enabled: true)
     end
 
     protected
 
     def check_feature_enabled!
-      render_404 unless Feature.enabled?(:security_dashboard, default_enabled: true)
+      render_404 unless Feature.enabled?(:instance_security_dashboard, default_enabled: true)
     end
 
     def vulnerable

ee/app/helpers/ee/dashboard_helper.rb

@@ -64,7 +64,7 @@ module EE
     def security_dashboard_available?
       security_dashboard = InstanceSecurityDashboard.new(current_user)
 
-      ::Feature.enabled?(:security_dashboard, default_enabled: true) &&
+      ::Feature.enabled?(:instance_security_dashboard, default_enabled: true) &&
         security_dashboard.feature_available?(:security_dashboard) &&
         can?(current_user, :read_instance_security_dashboard, security_dashboard)
     end

ee/spec/controllers/security/projects_controller_spec.rb

@@ -155,7 +155,7 @@ describe Security::ProjectsController do
 
       context 'and the security dashboard feature is disabled' do
         it '404s' do
-          stub_feature_flags(security_dashboard: false)
+          stub_feature_flags(instance_security_dashboard: false)
 
           subject
 

ee/spec/helpers/ee/dashboard_helper_spec.rb

@@ -13,13 +13,11 @@ describe DashboardHelper, type: :helper do
     end
 
     describe 'analytics' do
-      before do
-        allow(helper).to receive(:can?) { true }
-      end
-
       context 'when at least one analytics feature is enabled' do
         before do
           enable_only_one_analytics_feature_flag
+
+          stub_user_permissions_for(:analytics, false)
         end
 
         it 'includes analytics' do
@@ -34,7 +32,7 @@ describe DashboardHelper, type: :helper do
 
         context 'and the user has no access to instance statistics features' do
           before do
-            allow(helper).to receive(:can?) { false }
+            stub_user_permissions_for(:analytics, false)
           end
 
           it 'does not include analytics' do
@@ -43,6 +41,10 @@ describe DashboardHelper, type: :helper do
         end
 
         context 'and the user has access to instance statistics features' do
+          before do
+            stub_user_permissions_for(:analytics, true)
+          end
+
           it 'does include analytics' do
             expect(helper.dashboard_nav_links).to include(:analytics)
           end
@@ -50,96 +52,162 @@ describe DashboardHelper, type: :helper do
       end
     end
 
-    describe 'operations, environments and security' do
-      using RSpec::Parameterized::TableSyntax
+    describe 'operations dashboard link' do
+      context 'when the feature is available on the license' do
+        context 'and the user is authenticated' do
+          before do
+            stub_user_permissions_for(:operations, true)
+          end
 
-      before do
-        allow(helper).to receive(:can?).and_return(false)
+          it 'is included in the nav' do
+            expect(helper.dashboard_nav_links).to include(:operations)
+          end
+        end
+
+        context 'and the user is not authenticated' do
+          before do
+            stub_user_permissions_for(:operations, false)
+          end
+
+          it 'is not included in the nav' do
+            expect(helper.dashboard_nav_links).not_to include(:operations)
+          end
+        end
       end
 
-      where(:ability, :feature_flag, :nav_link) do
-        :read_operations_dashboard        | nil                     | :operations
-        :read_operations_dashboard        | :environments_dashboard | :environments
-        :read_instance_security_dashboard | :security_dashboard     | :security
+      context 'when the feature is not available on the license' do
+        before do
+          stub_user_permissions_for(:operations, false)
+        end
+
+        it 'is not included in the nav' do
+          expect(helper.dashboard_nav_links).not_to include(:operations)
+        end
       end
+    end
 
-      with_them do
-        describe 'when the feature is enabled' do
-          before do
-            stub_feature_flags(feature_flag => true) unless feature_flag.nil?
-          end
-
-          context 'and the feature is available on the license' do
-            context 'and the user is authenticated' do
-              before do
-                stub_resource_visibility(
-                  feature_flag,
-                  read_other_resources: true,
-                  read_security_dashboard: true,
-                  security_dashboard_available: true
-                )
-              end
-
-              it 'includes the nav link' do
-                expect(helper.dashboard_nav_links).to include(nav_link)
-              end
+    describe 'environments dashboard link' do
+      context 'when the feature is enabled' do
+        before do
+          stub_feature_flags(environments_dashboard: true)
+        end
+
+        context 'and the feature is available on the license' do
+          context 'and the user is authenticated' do
+            before do
+              stub_user_permissions_for(:operations, true)
+            end
+
+            it 'is included in the nav' do
+              expect(helper.dashboard_nav_links).to include(:environments)
+            end
+          end
+
+          context 'and the user is not authenticated' do
+            before do
+              stub_user_permissions_for(:operations, false)
             end
 
-            context 'and the user is not authenticated' do
-              let(:user) { nil }
-
-              before do
-                stub_resource_visibility(
-                  feature_flag,
-                  read_other_resources: false,
-                  read_security_dashboard: false,
-                  security_dashboard_available: true
-                )
-              end
-
-              it 'does not include the nav link' do
-                expect(helper.dashboard_nav_links).not_to include(nav_link)
-              end
+            it 'is not included in the nav' do
+              expect(helper.dashboard_nav_links).not_to include(:environments)
             end
           end
+        end
+
+        context 'and the feature is not available on the license' do
+          before do
+            stub_user_permissions_for(:operations, false)
+          end
+
+          it 'is not included in the nav' do
+            expect(helper.dashboard_nav_links).not_to include(:environments)
+          end
+        end
+      end
+
+      context 'when the feature is not enabled' do
+        before do
+          stub_feature_flags(environments_dashboard: false)
+          stub_user_permissions_for(:operations, false)
+        end
+
+        it 'is not included in the nav' do
+          expect(helper.dashboard_nav_links).not_to include(:environments)
+        end
+      end
+    end
+
+    describe 'security dashboard link' do
+      context 'when the feature is enabled' do
+        before do
+          stub_feature_flags(instance_security_dashboard: true)
+        end
 
-          context 'and the feature is not available on the license' do
+        context 'and the feature is available on the license' do
+          before do
+            stub_licensed_features(security_dashboard: true)
+          end
+
+          context 'and the user is authenticated' do
             before do
-              stub_resource_visibility(
-                feature_flag,
-                read_other_resources: false,
-                read_security_dashboard: true,
-                security_dashboard_available: false
-              )
+              stub_user_permissions_for(:security, true)
             end
 
-            it 'does not include the nav link' do
-              expect(helper.dashboard_nav_links).not_to include(nav_link)
+            it 'is included in the nav' do
+              expect(helper.dashboard_nav_links).to include(:security)
             end
           end
 
-          def stub_resource_visibility(feature_flag, read_other_resources:, read_security_dashboard:, security_dashboard_available:)
-            if feature_flag == :security_dashboard
-              stub_licensed_features(feature_flag => security_dashboard_available)
+          context 'and the user is not authenticated' do
+            before do
+              stub_user_permissions_for(:security, false)
+            end
 
-              allow(helper).to receive(:can?).and_return(read_security_dashboard)
-            else
-              allow(helper).to receive(:can?).with(user, ability).and_return(read_other_resources)
+            it 'is not included in the nav' do
+              expect(helper.dashboard_nav_links).not_to include(:security)
             end
           end
         end
 
-        describe 'when the feature is disabled' do
+        context 'when the feature is not available on the license' do
           before do
-            stub_feature_flags(feature_flag => false) unless feature_flag.nil?
-            allow(helper).to receive(:can?).and_return(false)
+            stub_licensed_features(security_dashboard: false)
+            stub_user_permissions_for(:security, true)
           end
 
-          it 'does not include the nav link' do
-            expect(helper.dashboard_nav_links).not_to include(nav_link)
+          it 'is not included in the nav' do
+            expect(helper.dashboard_nav_links).not_to include(:security)
           end
         end
       end
+
+      context 'when the feature is not enabled' do
+        before do
+          stub_feature_flags(instance_security_dashboard: false)
+          stub_licensed_features(security_dashboard: true)
+          stub_user_permissions_for(:security, true)
+        end
+
+        it 'is not included in the nav' do
+          expect(helper.dashboard_nav_links).not_to include(:security)
+        end
+      end
+    end
+
+    def stub_user_permissions_for(feature, enabled)
+      allow(helper).to receive(:can?).with(user, :read_cross_project).and_return(false)
+
+      can_read_instance_statistics = enabled && feature == :analytics
+      can_read_operations_dashboard = enabled && feature == :operations
+      can_read_instance_security_dashboard = enabled && feature == :security
+
+      allow(helper).to receive(:can?).with(user, :read_instance_statistics).and_return(can_read_instance_statistics)
+      allow(helper).to receive(:can?).with(user, :read_operations_dashboard).and_return(can_read_operations_dashboard)
+      allow_next_instance_of(InstanceSecurityDashboard) do |dashboard|
+        allow(helper).to(
+          receive(:can?).with(user, :read_instance_security_dashboard, dashboard).and_return(can_read_instance_security_dashboard)
+        )
+      end
     end
   end
 

ee/spec/support/shared_examples/controllers/security/application_controller_shared_examples.rb

@@ -27,7 +27,7 @@ RSpec.shared_examples Security::ApplicationController do
 
     context 'and the security dashboard feature is disabled' do
       it '404s' do
-        stub_feature_flags(security_dashboard: false)
+        stub_feature_flags(instance_security_dashboard: false)
 
         security_application_controller_child_action
 

ee/spec/support/shared_examples/requests/security/security_dashboard_json_endpoint_shared_examples.rb

@@ -28,7 +28,7 @@ shared_examples 'security dashboard JSON endpoint' do
 
     context 'and the security dashboard feature is disabled' do
       it '404s' do
-        stub_feature_flags(security_dashboard: false)
+        stub_feature_flags(instance_security_dashboard: false)
 
         security_dashboard_request
 

spec/migrations/remove_security_dashboard_feature_flag_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+require Rails.root.join('db', 'post_migrate', '20200214034836_remove_security_dashboard_feature_flag.rb')
+
+describe RemoveSecurityDashboardFeatureFlag, :migration do
+  let(:feature_gates) { table(:feature_gates) }
+
+  subject(:migration) { described_class.new }
+
+  describe '#up' do
+    it 'deletes the security_dashboard feature gate' do
+      security_dashboard_feature = feature_gates.create!(feature_key: :security_dashboard, key: :boolean, value: 'false')
+      actors_security_dashboard_feature = feature_gates.create!(feature_key: :security_dashboard, key: :actors, value: 'Project:1')
+
+      migration.up
+
+      expect { security_dashboard_feature.reload }.to raise_error(ActiveRecord::RecordNotFound)
+      expect(actors_security_dashboard_feature.reload).to be_present
+    end
+  end
+
+  describe '#down' do
+    it 'copies the instance_security_dashboard feature gate to a security_dashboard gate' do
+      feature_gates.create!(feature_key: :instance_security_dashboard, key: :actors, value: 'Project:1')
+      feature_gates.create!(feature_key: :instance_security_dashboard, key: 'boolean', value: 'false')
+
+      migration.down
+
+      security_dashboard_feature = feature_gates.find_by(feature_key: :security_dashboard, key: :boolean)
+      expect(security_dashboard_feature.value).to eq('false')
+    end
+
+    context 'when there is no instance_security_dashboard gate' do
+      it 'does nothing' do
+        migration.down
+
+        security_dashboard_feature = feature_gates.find_by(feature_key: :security_dashboard, key: :boolean)
+        expect(security_dashboard_feature).to be_nil
+      end
+    end
+
+    context 'when there already is a security_dashboard gate' do
+      it 'does nothing' do
+        feature_gates.create!(feature_key: :security_dashboard, key: 'boolean', value: 'false')
+        feature_gates.create!(feature_key: :instance_security_dashboard, key: 'boolean', value: 'false')
+
+        expect { migration.down }.not_to raise_error
+      end
+    end
+  end
+end

spec/migrations/rename_security_dashboard_feature_flag_to_instance_security_dashboard_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+require Rails.root.join('db', 'migrate', '20200212014653_rename_security_dashboard_feature_flag_to_instance_security_dashboard.rb')
+
+describe RenameSecurityDashboardFeatureFlagToInstanceSecurityDashboard, :migration do
+  let(:feature_gates) { table(:feature_gates) }
+
+  subject(:migration) { described_class.new }
+
+  describe '#up' do
+    it 'copies the security_dashboard feature gate to a new instance_security_dashboard gate' do
+      feature_gates.create!(feature_key: :security_dashboard, key: :actors, value: 'Project:1')
+      feature_gates.create!(feature_key: :security_dashboard, key: :boolean, value: 'false')
+
+      migration.up
+
+      instance_security_dashboard_feature = feature_gates.find_by(feature_key: :instance_security_dashboard, key: :boolean)
+      expect(instance_security_dashboard_feature.value).to eq('false')
+    end
+
+    context 'when there is no security_dashboard gate' do
+      it 'does nothing' do
+        migration.up
+
+        instance_security_dashboard_feature = feature_gates.find_by(feature_key: :instance_security_dashboard, key: :boolean)
+        expect(instance_security_dashboard_feature).to be_nil
+      end
+    end
+
+    context 'when there is already an instance_security_dashboard gate' do
+      it 'does nothing' do
+        feature_gates.create!(feature_key: :security_dashboard, key: 'boolean', value: 'false')
+        feature_gates.create!(feature_key: :instance_security_dashboard, key: 'boolean', value: 'false')
+
+        expect { migration.up }.not_to raise_error
+      end
+    end
+  end
+
+  describe '#down' do
+    it 'removes the instance_security_dashboard gate' do
+      actors_instance_security_dashboard_feature = feature_gates.create!(feature_key: :instance_security_dashboard, key: :actors, value: 'Project:1')
+      instance_security_dashboard_feature = feature_gates.create!(feature_key: :instance_security_dashboard, key: :boolean, value: 'false')
+
+      migration.down
+
+      expect { instance_security_dashboard_feature.reload }.to raise_error(ActiveRecord::RecordNotFound)
+      expect(actors_instance_security_dashboard_feature.reload).to be_present
+    end
+  end
+end