Merge branch 'dblessing_saml_group_links_policies' into 'master'

SAML Group Links permission policies

See merge request gitlab-org/gitlab!45581

ee/app/helpers/ee/saml_providers_helper.rb

@@ -2,18 +2,14 @@
 
 module EE
   module SamlProvidersHelper
-    def group_saml_configured?
-      ::Gitlab::Auth::GroupSaml::Config.enabled?
-    end
-
     def show_saml_in_sidebar?(group)
-      return false unless group_saml_configured?
-      return false unless group.feature_available?(:group_saml)
-      return false if group.subgroup?
-
       can?(current_user, :admin_group_saml, group)
     end
 
+    def show_saml_group_links_in_sidebar?(group)
+      can?(current_user, :admin_saml_group_links, group)
+    end
+
     def saml_link_for_provider(text, provider, **args)
       saml_link(text, provider.group.full_path, **args)
     end

ee/app/policies/ee/group_policy.rb

@@ -69,8 +69,16 @@ module EE
         @subject.feature_available?(:cluster_deployments)
       end
 
-      condition(:group_saml_enabled) do
-        @subject.saml_provider&.enabled?
+      condition(:group_saml_config_enabled, scope: :global) do
+        ::Gitlab::Auth::GroupSaml::Config.enabled?
+      end
+
+      condition(:group_saml_available, scope: :subject) do
+        !@subject.subgroup? && @subject.feature_available?(:group_saml)
+      end
+
+      condition(:group_saml_enabled, scope: :subject) do
+        @subject.saml_enabled?
       end
 
       condition(:group_timelogs_available) do
@@ -198,7 +206,9 @@ module EE
         enable :read_group_security_dashboard
       end
 
-      rule { admin | owner }.enable :admin_group_saml
+      rule { group_saml_config_enabled & group_saml_available & (admin | owner) }.enable :admin_group_saml
+
+      rule { group_saml_enabled & can?(:admin_group_saml) }.enable :admin_saml_group_links
 
       rule { admin | (can_owners_manage_ldap & owner) }.policy do
         enable :admin_ldap_group_links

ee/spec/helpers/ee/saml_providers_helper_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe EE::SamlProvidersHelper do
+  def stub_can(permission, value)
+    allow(helper).to receive(:can?).with(user, permission, group).and_return(value)
+  end
+
+  let_it_be(:group) { create(:group) }
+  let_it_be(:user) { create(:user) }
+
+  before do
+    allow(helper).to receive(:current_user).and_return(user)
+  end
+
+  describe '#show_saml_in_sidebar?' do
+    subject { helper.show_saml_in_sidebar?(group) }
+
+    context 'when the user can admin group saml' do
+      before do
+        stub_can(:admin_group_saml, true)
+      end
+
+      it { is_expected.to eq(true) }
+    end
+
+    context 'when the user cannot admin group saml' do
+      before do
+        stub_can(:admin_group_saml, false)
+      end
+
+      it { is_expected.to eq(false) }
+    end
+  end
+
+  describe '#show_saml_group_links_in_sidebar?' do
+    subject { helper.show_saml_group_links_in_sidebar?(group) }
+
+    context 'when the user can admin saml group links' do
+      before do
+        stub_can(:admin_saml_group_links, true)
+      end
+
+      it { is_expected.to eq(true) }
+    end
+
+    context 'when the user cannot admin saml group links' do
+      before do
+        stub_can(:admin_saml_group_links, false)
+      end
+
+      it { is_expected.to eq(false) }
+    end
+  end
+end

ee/spec/policies/group_policy_spec.rb

@@ -285,62 +285,130 @@ RSpec.describe GroupPolicy do
   end
 
   describe 'per group SAML' do
-    let(:current_user) { maintainer }
-
-    it { is_expected.to be_disallowed(:admin_group_saml) }
+    context 'when group_saml is unavailable' do
+      def stub_group_saml_config(enabled)
+        allow(::Gitlab::Auth::GroupSaml::Config).to receive_messages(enabled?: enabled)
+      end
 
-    context 'owner' do
       let(:current_user) { owner }
 
-      it { is_expected.to be_allowed(:admin_group_saml) }
-    end
+      context 'when group saml config is disabled' do
+        before do
+          stub_group_saml_config(false)
+        end
 
-    context 'admin' do
-      let(:current_user) { admin }
+        it { is_expected.to be_disallowed(:admin_group_saml) }
+      end
 
-      it { is_expected.to be_allowed(:admin_group_saml) }
-    end
+      context 'when the group is a subgroup' do
+        let_it_be(:subgroup) { create(:group, :private, parent: group) }
 
-    context 'with sso enforcement enabled' do
-      let(:current_user) { guest }
+        before do
+          stub_group_saml_config(true)
+        end
 
-      let_it_be(:saml_provider) { create(:saml_provider, group: group, enforced_sso: true) }
+        subject { described_class.new(current_user, subgroup) }
 
+        it { is_expected.to be_disallowed(:admin_group_saml) }
+      end
+
+      context 'when the feature is not licensed' do
+        before do
+          stub_group_saml_config(true)
+          stub_licensed_features(group_saml: false)
+        end
+
+        it { is_expected.to be_disallowed(:admin_group_saml) }
+      end
+    end
+
+    context 'when group_saml is available' do
       before do
         stub_licensed_features(group_saml: true)
       end
 
-      context 'when the session has been set globally' do
-        around do |example|
-          Gitlab::Session.with_session({}) do
-            example.run
-          end
+      context 'without an enabled SAML provider' do
+        context 'maintainer' do
+          let(:current_user) { maintainer }
+
+          it { is_expected.to be_disallowed(:admin_group_saml) }
+          it { is_expected.to be_disallowed(:admin_saml_group_links) }
         end
 
-        it 'prevents access without a SAML session' do
-          is_expected.not_to be_allowed(:read_group)
+        context 'owner' do
+          let(:current_user) { owner }
+
+          it { is_expected.to be_allowed(:admin_group_saml) }
+          it { is_expected.to be_disallowed(:admin_saml_group_links) }
         end
 
-        context 'as a group owner' do
-          before do
-            group.add_owner(current_user)
+        context 'admin' do
+          let(:current_user) { admin }
+
+          it { is_expected.to be_allowed(:admin_group_saml) }
+          it { is_expected.to be_disallowed(:admin_saml_group_links) }
+        end
+      end
+
+      context 'with an enabled SAML provider' do
+        let_it_be(:saml_provider) { create(:saml_provider, group: group, enabled: true) }
+
+        context 'maintainer' do
+          let(:current_user) { maintainer }
+
+          it { is_expected.to be_disallowed(:admin_saml_group_links) }
+        end
+
+        context 'owner' do
+          let(:current_user) { owner }
+
+          it { is_expected.to be_allowed(:admin_saml_group_links) }
+        end
+
+        context 'admin' do
+          let(:current_user) { admin }
+
+          it { is_expected.to be_allowed(:admin_saml_group_links) }
+        end
+      end
+
+      context 'with sso enforcement enabled' do
+        let(:current_user) { guest }
+
+        let_it_be(:saml_provider) { create(:saml_provider, group: group, enforced_sso: true) }
+
+        context 'when the session has been set globally' do
+          around do |example|
+            Gitlab::Session.with_session({}) do
+              example.run
+            end
           end
 
           it 'prevents access without a SAML session' do
-            is_expected.not_to allow_action(:read_group)
+            is_expected.not_to be_allowed(:read_group)
+          end
+
+          context 'as a group owner' do
+            before do
+              group.add_owner(current_user)
+            end
+
+            it 'prevents access without a SAML session' do
+              is_expected.not_to allow_action(:read_group)
+            end
           end
-        end
 
-        it 'allows access with a SAML session' do
-          Gitlab::Auth::GroupSaml::SsoEnforcer.new(saml_provider).update_session
+          it 'allows access with a SAML session' do
+            Gitlab::Auth::GroupSaml::SsoEnforcer.new(saml_provider).update_session
 
-          is_expected.to be_allowed(:read_group)
+            is_expected.to be_allowed(:read_group)
+          end
         end
-      end
 
-      context 'when there is no global session or sso state' do
-        it "allows access because we haven't yet restricted all use cases" do
-          is_expected.to be_allowed(:read_group)
+        context 'when there is no global session or sso state' do
+          it "allows access because we haven't yet restricted all use cases" do
+            is_expected.to be_allowed(:read_group)
+          end
         end
       end
     end