diff --git a/ee/app/helpers/ee/saml_providers_helper.rb b/ee/app/helpers/ee/saml_providers_helper.rb
index 01b9c78461719d3d0509725b636b52681919771b..68e101b1a42712d8a88ab50405eacf2e2b1a40be 100644
--- a/ee/app/helpers/ee/saml_providers_helper.rb
+++ b/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
diff --git a/ee/app/policies/ee/group_policy.rb b/ee/app/policies/ee/group_policy.rb
index ef4dbc1716e402b4a25f27e929de469bc26a6634..96107d4da65fc60320291fe6e0b2c2d0a9d9afd0 100644
--- a/ee/app/policies/ee/group_policy.rb
+++ b/ee/app/policies/ee/group_policy.rb
@@ -69,8 +69,16 @@ module GroupPolicy
@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 GroupPolicy
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
diff --git a/ee/spec/helpers/ee/saml_providers_helper_spec.rb b/ee/spec/helpers/ee/saml_providers_helper_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..5328a2925ccfe3eb2921ab51af466983d463276c
--- /dev/null
+++ b/ee/spec/helpers/ee/saml_providers_helper_spec.rb
@@ -0,0 +1,56 @@
+# 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
diff --git a/ee/spec/policies/group_policy_spec.rb b/ee/spec/policies/group_policy_spec.rb
index f713bf47aa1923af5e682a93f9e8ffffd445c897..a9240ba1311ffad982da5ca20c82941eb48ee6a9 100644
--- a/ee/spec/policies/group_policy_spec.rb
+++ b/ee/spec/policies/group_policy_spec.rb
@@ -285,62 +285,130 @@
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