Merge branch 'jej/prevent-enabling-group-managed-accounts-when-SSO-not-linked' into 'master'

Require group owner to have linked SAML before enabling Group Managed Accounts

Closes #38021 and #39183

See merge request gitlab-org/gitlab!21721

changelogs/unreleased/jej-prevent-enabling-group-managed-accounts-when-SSO-not-linked.yml

+---
+title: Require group owner to have linked SAML before enabling Group Managed Accounts
+merge_request: 21721
+author:
+type: fixed

ee/app/services/group_saml/saml_provider/base_service.rb

@@ -22,6 +22,7 @@ module GroupSaml
           updated = saml_provider.update(params)
 
           if updated && saml_provider.enforced_group_managed_accounts? && !group_managed_accounts_was_enforced
+            require_linked_saml_to_enable_group_managed!
             cleanup_members!
           end
         end
@@ -29,10 +30,21 @@ module GroupSaml
 
       private
 
+      def require_linked_saml_to_enable_group_managed!
+        return if saml_provider.identities.for_user(current_user).exists?
+
+        add_error!(_("Group Owner must have signed in with SAML before enabling Group Managed Accounts"))
+      end
+
       def cleanup_members!
         return if GroupManagedAccounts::CleanUpMembersService.new(current_user, group).execute
 
-        saml_provider.errors.add(:base, _("Can't remove group members without group managed account"))
+        add_error!(_("Can't remove group members without group managed account"))
+      end
+
+      def add_error!(message)
+        saml_provider.errors.add(:base, message)
+
         raise ActiveRecord::Rollback
       end
     end

ee/spec/controllers/groups/saml_providers_controller_spec.rb

@@ -116,7 +116,7 @@ describe Groups::SamlProvidersController do
     end
 
     describe 'PUT #update' do
-      subject { put :update, params: { group_id: group, saml_provider: { enforced_sso: 'true', enforced_group_managed_accounts: 'true' } } }
+      subject { put :update, params: { group_id: group, saml_provider: { enforced_sso: 'true' } } }
 
       before do
         group.add_owner(user)
@@ -148,6 +148,13 @@ describe Groups::SamlProvidersController do
         end
       end
 
+      context 'enabling group managed when owner has linked identity' do
+        subject { put :update, params: { group_id: group, saml_provider: { enforced_sso: 'true', enforced_group_managed_accounts: 'true' } } }
+
+        before do
+          create(:group_saml_identity, saml_provider: saml_provider, user: user)
+        end
+
         context 'group_managed_accounts feature flag enabled' do
           before do
             stub_feature_flags(group_managed_accounts: true)
@@ -174,5 +181,21 @@ describe Groups::SamlProvidersController do
           end
         end
       end
+
+      context 'enabling group managed when owner has not linked identity' do
+        subject { put :update, params: { group_id: group, saml_provider: { enforced_sso: 'true', enforced_group_managed_accounts: 'true' } } }
+
+        before do
+          stub_feature_flags(group_managed_accounts: true)
+        end
+
+        it 'does not update update the flags' do
+          expect do
+            subject
+            saml_provider.reload
+          end.not_to change { saml_provider.enforced_group_managed_accounts? }
+        end
+      end
+    end
   end
 end

ee/spec/features/groups/saml_providers_spec.rb

@@ -117,6 +117,10 @@ describe 'SAML provider settings' do
       end
 
       context 'enforced_group_managed_accounts enabled', :js do
+        before do
+          create(:group_saml_identity, saml_provider: saml_provider, user: user)
+        end
+
         it 'updates the flag' do
           stub_feature_flags(group_managed_accounts: true)
 

ee/spec/services/group_saml/saml_provider/create_service_spec.rb

@@ -3,7 +3,8 @@
 require 'spec_helper'
 
 describe GroupSaml::SamlProvider::CreateService do
-  subject(:service) { described_class.new(nil, group, params: params) }
+  let(:current_user) { build_stubbed(:user) }
+  subject(:service) { described_class.new(current_user, group, params: params) }
 
   let(:group) { create :group }
 

ee/spec/services/group_saml/saml_provider/update_service_spec.rb

@@ -3,12 +3,14 @@
 require 'spec_helper'
 
 describe GroupSaml::SamlProvider::UpdateService do
-  subject(:service) { described_class.new(nil, saml_provider, params: params) }
+  let(:current_user) { create(:user) }
+  subject(:service) { described_class.new(current_user, saml_provider, params: params) }
 
   let(:saml_provider) do
-    create :saml_provider, enabled: false, enforced_sso: false, enforced_group_managed_accounts: enforced_group_managed_accounts
+    create :saml_provider, enabled: false, enforced_sso: false
   end
   let(:group) { saml_provider.group }
 
   include_examples 'base SamlProvider service'
+  include_examples 'SamlProvider service toggles Group Managed Accounts'
 end

ee/spec/support/shared_examples/services/group_saml/saml_provider/base_service_shared_examples.rb

@@ -6,19 +6,11 @@ shared_examples 'base SamlProvider service' do
       sso_url: 'https://test',
       certificate_fingerprint: fingerprint,
       enabled: true,
-      enforced_sso: true,
-      enforced_group_managed_accounts: true
+      enforced_sso: true
     }
   end
 
-  let(:enforced_group_managed_accounts) { false }
   let(:fingerprint) { '11:22:33:44:55:66:77:88:99:11:22:33:44:55:66:77:88:99' }
-  let(:cleanup_members_service_spy) { spy('GroupSaml::GroupManagedAccounts::CleanUpMembersService') }
-
-  before do
-    allow(GroupSaml::GroupManagedAccounts::CleanUpMembersService)
-      .to receive(:new).with(nil, group).and_return(cleanup_members_service_spy)
-  end
 
   it 'updates SAML provider with given params' do
     expect do
@@ -28,10 +20,33 @@ shared_examples 'base SamlProvider service' do
              .and change { group.saml_provider&.certificate_fingerprint }.to(fingerprint)
              .and change { group.saml_provider&.enabled? }.to(true)
              .and change { group.saml_provider&.enforced_sso? }.to(true)
-             .and change { group.saml_provider&.enforced_group_managed_accounts? }.to(true)
+  end
+end
+
+shared_examples 'SamlProvider service toggles Group Managed Accounts' do
+  let(:cleanup_members_service_spy) { spy('GroupSaml::GroupManagedAccounts::CleanUpMembersService') }
+
+  before do
+    allow(GroupSaml::GroupManagedAccounts::CleanUpMembersService)
+      .to receive(:new).with(current_user, group).and_return(cleanup_members_service_spy)
+  end
+
+  context 'when enabling enforced_group_managed_accounts' do
+    let(:params) do
+      attributes_for(:saml_provider, :enforced_group_managed_accounts)
+    end
+
+    before do
+      create(:group_saml_identity, user: current_user, saml_provider: saml_provider)
+    end
+
+    it 'updates enforced_group_managed_accounts boolean' do
+      expect do
+        service.execute
+        group.reload
+      end.to change { group.saml_provider&.enforced_group_managed_accounts? }.to(true)
     end
 
-  context 'when enforced_group_managed_accounts is enabled' do
     it 'cleans up group members' do
       service.execute
 
@@ -66,11 +81,34 @@ shared_examples 'base SamlProvider service' do
         end.not_to change { group.saml_provider }
       end
     end
+
+    context 'when owner has not linked SAML yet' do
+      before do
+        Identity.delete_all
+      end
+
+      it 'adds an error warning that the owner must first link SAML' do
+        service.execute
+
+        expect(service.saml_provider.errors[:base]).to eq(["Group Owner must have signed in with SAML before enabling Group Managed Accounts"])
+      end
+
+      it 'does not attempt member cleanup' do
+        service.execute
+
+        expect(cleanup_members_service_spy).not_to have_received(:execute)
+      end
+    end
   end
 
   context 'when group managed accounts was enabled beforehand' do
-    let(:enforced_group_managed_accounts) { true }
-    let(:params) { { enforced_group_managed_accounts: true } }
+    let(:params) do
+      attributes_for(:saml_provider, :enforced_group_managed_accounts)
+    end
+
+    before do
+      saml_provider.update!(enforced_group_managed_accounts: true)
+    end
 
     it 'does not clean up group members' do
       service.execute
@@ -80,8 +118,13 @@ shared_examples 'base SamlProvider service' do
   end
 
   context 'when enforced_group_managed_accounts is disabled' do
-    let(:enforced_group_managed_accounts) { true }
-    let(:params) { { enforced_group_managed_accounts: false } }
+    before do
+      saml_provider.update!(enforced_group_managed_accounts: true)
+    end
+
+    let(:params) do
+      attributes_for(:saml_provider, enabled: true, enforced_sso: true, enforced_group_managed_accounts: false)
+    end
 
     it 'does not clean up group members' do
       service.execute

locale/gitlab.pot

@@ -9235,6 +9235,9 @@ msgstr ""
 msgid "Group ID: %{group_id}"
 msgstr ""
 
+msgid "Group Owner must have signed in with SAML before enabling Group Managed Accounts"
+msgstr ""
+
 msgid "Group Runners"
 msgstr ""
 

qa/qa/ee/page/group/saml_sso_sign_up.rb

@@ -9,6 +9,11 @@ module QA
             element :sign_out_and_register_button
             element :new_user_email_field
             element :new_user_username_field
+            element :new_user_register_button
+          end
+
+          def click_register_button
+            click_element :new_user_register_button
           end
 
           def click_signout_and_register_button

qa/qa/ee/runtime/saml.rb

@@ -5,15 +5,23 @@ module QA
       module Saml
         def self.idp_base_url
           host = QA::Runtime::Env.simple_saml_hostname || 'localhost'
-          "https://#{host}:8443/simplesaml/saml2/idp"
+          "https://#{host}:8443/simplesaml"
         end
 
         def self.idp_sso_url
-          "#{idp_base_url}/SSOService.php"
+          "#{idp_base_url}/saml2/idp/SSOService.php"
+        end
+
+        def self.idp_sign_out_url
+          "#{idp_base_url}/module.php/core/authenticate.php?as=example-userpass&logout"
+        end
+
+        def self.idp_signed_out_url
+          "#{idp_base_url}/logout.php"
         end
 
         def self.idp_metadata_url
-          "#{idp_base_url}/metadata.php"
+          "#{idp_base_url}/saml2/idp/metadata.php"
         end
 
         def self.idp_issuer

qa/qa/specs/features/ee/browser_ui/1_manage/group/group_saml_sso_spec.rb

@@ -184,6 +184,9 @@ module QA
             end
 
             @managed_group_url = setup_and_enable_group_managed_accounts
+
+            Page::Main::Menu.perform(&:sign_out_if_signed_in)
+            logout_from_idp
           end
 
           it 'removes existing users from the group, forces existing users to create a new account and allows to leave group' do
@@ -203,7 +206,7 @@ module QA
 
             new_username = EE::Page::Group::SamlSSOSignUp.perform(&:current_username)
 
-            EE::Page::Group::SamlSSOSignUp.perform(&:click_signout_and_register_button)
+            EE::Page::Group::SamlSSOSignUp.perform(&:click_register_button)
 
             expect(page).to have_text("Sign up was successful! Please confirm your email to sign in.")
 
@@ -298,20 +301,22 @@ module QA
     end
 
     def setup_and_enable_group_managed_accounts
-      Page::Main::Login.perform(&:sign_in_using_credentials) unless Page::Main::Menu.perform(&:signed_in?)
+      setup_and_enable_enforce_sso
 
       Support::Retrier.retry_on_exception do
-        @group.visit!
+        # We must sign in with SAML before enabling Group Managed Accounts
+        EE::Page::Group::Settings::SamlSSO.perform do |saml_sso|
+          saml_sso.click_user_login_url_link
+        end
+
+        EE::Page::Group::SamlSSOSignIn.perform(&:click_sign_in)
+        login_to_idp_if_required_and_expect_success('user1', 'user1pass')
 
         Page::Group::Menu.perform(&:go_to_saml_sso_group_settings)
 
         EE::Page::Group::Settings::SamlSSO.perform do |saml_sso|
-          saml_sso.enforce_sso
           saml_sso.enable_group_managed_accounts
 
-          saml_sso.set_id_provider_sso_url(EE::Runtime::Saml.idp_sso_url)
-          saml_sso.set_cert_fingerprint(EE::Runtime::Saml.idp_certificate_fingerprint)
-
           saml_sso.click_save_changes
 
           saml_sso.user_login_url_link_text
@@ -331,6 +336,11 @@ module QA
       Resource::User.fabricate_via_api!
     end
 
+    def logout_from_idp
+      page.visit EE::Runtime::Saml.idp_sign_out_url
+      Support::Waiter.wait { current_url == EE::Runtime::Saml.idp_signed_out_url }
+    end
+
     def reset_idp_session
       Runtime::Logger.debug(%Q[Visiting IDP url at "#{EE::Runtime::Saml.idp_sso_url}"])