Enabling group-managed-accounts needs linked SAML

Avoid bug where owner ends up unable to log in due to SSO enforcement,
by first requiring SAML to have been linked before allowing group
managed accounts to be enabled.

Long term we will offer to convert the user to a Group Managed one, so
would be able to remove this workaround.

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,29 +148,52 @@ describe Groups::SamlProvidersController do
         end
       end
 
-      context 'group_managed_accounts feature flag enabled' do
+      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
-          stub_feature_flags(group_managed_accounts: true)
+          create(:group_saml_identity, saml_provider: saml_provider, user: user)
         end
 
-        it 'updates the flags' do
-          expect do
-            subject
-            saml_provider.reload
-          end.to change { saml_provider.enforced_group_managed_accounts? }.to(true)
+        context 'group_managed_accounts feature flag enabled' do
+          before do
+            stub_feature_flags(group_managed_accounts: true)
+          end
+
+          it 'updates the flags' do
+            expect do
+              subject
+              saml_provider.reload
+            end.to change { saml_provider.enforced_group_managed_accounts? }.to(true)
+          end
+        end
+
+        context 'group_managed_accounts feature flag disabled' do
+          before do
+            stub_feature_flags(group_managed_accounts: false)
+          end
+
+          it 'does not update the setting' do
+            expect do
+              subject
+              saml_provider.reload
+            end.not_to change { saml_provider.enforced_group_managed_accounts? }.from(false)
+          end
         end
       end
 
-      context 'group_managed_accounts feature flag disabled' do
+      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: false)
+          stub_feature_flags(group_managed_accounts: true)
         end
 
-        it 'does not update the setting' do
+        it 'does not update update the flags' do
           expect do
             subject
             saml_provider.reload
-          end.not_to change { saml_provider.enforced_group_managed_accounts? }.from(false)
+          end.not_to change { saml_provider.enforced_group_managed_accounts? }
         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,10 +118,15 @@ 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
+    it 'does not clean up group members' do
       service.execute
 
       expect(cleanup_members_service_spy).not_to have_received(: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}"])