Merge branch 'dblessing_saml_ff_policies' into 'master'

Sync groups on sign-in for GitLab.com Group SAML

See merge request gitlab-org/gitlab!47445

config/feature_flags/development/saml_group_links.yml

@@ -5,4 +5,4 @@ rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/267020
 milestone: '13.6'
 type: development
 group: group::access
-default_enabled: false
+default_enabled: true

doc/user/group/saml_sso/index.md

@@ -242,6 +242,42 @@ For example, to unlink the `MyOrg` account, the following **Disconnect** button
 
 ![Unlink Group SAML](img/unlink_group_saml.png)
 
+## Group Sync
+
+When the SAML response includes a user and their group memberships from the SAML identity provider,
+GitLab uses that information to automatically manage that user's GitLab group memberships.
+
+Ensure your SAML identity provider sends an attribute statement named `Groups` or `groups` like the following:
+
+```xml
+<saml:AttributeStatement>
+  <saml:Attribute Name="Groups">
+    <saml:AttributeValue xsi:type="xs:string">Developers</saml:AttributeValue>
+    <saml:AttributeValue xsi:type="xs:string">Product Managers</saml:AttributeValue>
+  </saml:Attribute>
+</saml:AttributeStatement>
+```
+
+When SAML SSO is enabled for the top-level group, `Maintainer` and `Owner` level users
+see a new menu item in group **Settings -> SAML Group Links**. Each group can specify
+one or more group links to map a SAML identity provider group name to a GitLab access level.
+
+![SAML Group Links navigation](img/saml_group_links_nav_v13_6.png)
+
+To link the SAML `Freelancers` group in the attribute statement example above:
+
+1. Enter `Freelancers` in the `SAML Group Name` field.
+1. Choose the desired `Access Level`.
+1. **Save** the group link. 
+1. Repeat to add additional group links if desired. 
+
+![SAML Group Links](img/saml_group_links_v13_6.png)
+
+If a user is a member of multiple SAML groups mapped to the same GitLab group, 
+the user gets the highest access level from the groups. For example, if one group
+is linked as `Guest` and another `Maintainer`, a user in both groups gets `Maintainer` 
+access.
+
 ## Glossary
 
 | Term | Description |

ee/app/controllers/groups/saml_group_links_controller.rb

@@ -30,7 +30,7 @@ module Groups
     private
 
     def require_saml_group_links_enabled
-      render_404 unless ::Feature.enabled?(:saml_group_links, group)
+      render_404 unless ::Feature.enabled?(:saml_group_links, group, default_enabled: true)
     end
 
     def authorize_admin_saml_group_links!

ee/app/helpers/ee/saml_providers_helper.rb

@@ -7,7 +7,7 @@ module EE
     end
 
     def show_saml_group_links_in_sidebar?(group)
-      ::Feature.enabled?(:saml_group_links, group) && can?(current_user, :admin_saml_group_links, group)
+      can?(current_user, :admin_saml_group_links, group)
     end
 
     def saml_link_for_provider(text, provider, **args)

ee/app/models/ee/group.rb

@@ -227,7 +227,7 @@ module EE
     end
 
     def saml_group_sync_available?
-      ::Feature.enabled?(:saml_group_links, self) &&
+      ::Feature.enabled?(:saml_group_links, self, default_enabled: true) &&
         feature_available?(:group_saml_group_sync) && root_ancestor.saml_enabled?
     end
 

ee/app/models/license.rb

@@ -91,6 +91,7 @@ class License < ApplicationRecord
     group_project_templates
     group_repository_analytics
     group_saml
+    group_saml_group_sync
     group_wikis
     incident_sla
     incident_metric_upload
@@ -147,7 +148,6 @@ class License < ApplicationRecord
     license_scanning
     personal_access_token_expiration_policy
     enforce_pat_expiration
-    group_saml_group_sync
     prometheus_alerts
     pseudonymizer
     release_evidence_test_artifacts

ee/app/policies/ee/group_policy.rb

@@ -78,7 +78,7 @@ module EE
       end
 
       condition(:group_saml_group_sync_available, scope: :subject) do
-        @subject.feature_available?(:group_saml_group_sync)
+        @subject.saml_group_sync_available?
       end
 
       condition(:group_timelogs_available) do
@@ -205,7 +205,7 @@ module EE
 
       rule { group_saml_config_enabled & group_saml_available & (admin | owner) }.enable :admin_group_saml
 
-      rule { group_saml_group_sync_available & group_saml_enabled & can?(:admin_group_saml) }.policy do
+      rule { group_saml_config_enabled & group_saml_group_sync_available & (admin | owner) }.policy do
         enable :admin_saml_group_links
       end
 

ee/changelogs/unreleased/dblessing_trigger_saml_group_sync.yml

+---
+title: Sync groups on sign-in for GitLab.com Group SAML
+merge_request: 47445
+author:
+type: added

ee/spec/helpers/ee/saml_providers_helper_spec.rb

@@ -37,20 +37,6 @@ RSpec.describe EE::SamlProvidersHelper do
   describe '#show_saml_group_links_in_sidebar?' do
     subject { helper.show_saml_group_links_in_sidebar?(group) }
 
-    context 'when the feature is disabled' do
-      before do
-        stub_feature_flags(saml_group_links: false)
-        stub_can(:admin_saml_group_links, true)
-      end
-
-      it { is_expected.to eq(false) }
-    end
-
-    context 'when the feature is enabled' do
-      before do
-        stub_feature_flags(saml_group_links: true)
-      end
-
     context 'when the user can admin saml group links' do
       before do
         stub_can(:admin_saml_group_links, true)
@@ -67,5 +53,4 @@ RSpec.describe EE::SamlProvidersHelper do
       it { is_expected.to eq(false) }
     end
   end
-  end
 end

ee/spec/policies/group_policy_spec.rb

@@ -285,11 +285,11 @@ RSpec.describe GroupPolicy do
   end
 
   describe 'per group SAML' do
-    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 'when group_saml is unavailable' do
       let(:current_user) { owner }
 
       context 'when group saml config is disabled' do
@@ -347,12 +347,13 @@ RSpec.describe GroupPolicy do
 
       context 'when group_saml_group_sync is licensed' do
         before do
+          stub_group_saml_config(true)
           stub_application_setting(check_namespace_plan: true)
         end
 
         before_all do
           create(:license, plan: License::ULTIMATE_PLAN)
-          create(:gitlab_subscription, :gold, namespace: group)
+          create(:gitlab_subscription, :silver, namespace: group)
         end
 
         context 'without an enabled SAML provider' do
@@ -398,6 +399,15 @@ RSpec.describe GroupPolicy do
 
             it { is_expected.to be_allowed(:admin_saml_group_links) }
           end
+
+          context 'when the group is a subgroup' do
+            let_it_be(:subgroup) { create(:group, :private, parent: group) }
+            let(:current_user) { owner }
+
+            subject { described_class.new(current_user, subgroup) }
+
+            it { is_expected.to be_allowed(:admin_saml_group_links) }
+          end
         end
       end