Merge branch 'jej/ensure-can-unlink-group-saml-when-disabled' into 'master'

Ensure users can unlink Group SAML when the group has SAML disabled

Closes #217817

See merge request gitlab-org/gitlab!32655

ee/app/controllers/groups/sso_controller.rb

@@ -5,7 +5,9 @@ class Groups::SsoController < Groups::ApplicationController
   skip_before_action :group
 
   before_action :authenticate_user!, only: [:unlink]
-  before_action :require_configured_provider!
+  before_action :require_group_saml_instance!
+  before_action :require_licensed_group!, except: [:unlink]
+  before_action :require_saml_provider!
   before_action :require_enabled_provider!, except: [:unlink]
   before_action :check_user_can_sign_in_with_provider, only: [:saml]
   before_action :redirect_if_group_moved
@@ -112,20 +114,20 @@ class Groups::SsoController < Groups::ApplicationController
     Gitlab::Auth::GroupSaml::SsoEnforcer.new(unauthenticated_group.saml_provider).update_session
   end
 
-  def require_configured_provider!
-    unless unauthenticated_group&.feature_available?(:group_saml) && Gitlab::Auth::GroupSaml::Config.enabled?
-      return route_not_found
-    end
+  def require_group_saml_instance!
+    route_not_found unless Gitlab::Auth::GroupSaml::Config.enabled?
+  end
 
-    return if unauthenticated_group.saml_provider
+  def require_licensed_group!
+    route_not_found unless unauthenticated_group&.feature_available?(:group_saml)
+  end
 
-    redirect_settings_or_not_found
+  def require_saml_provider!
+    redirect_settings_or_not_found unless unauthenticated_group.saml_provider
   end
 
   def require_enabled_provider!
-    return if unauthenticated_group.saml_provider&.enabled?
-
-    redirect_settings_or_not_found
+    redirect_settings_or_not_found unless unauthenticated_group.saml_provider&.enabled?
   end
 
   def redirect_settings_or_not_found

ee/changelogs/unreleased/jej-ensure-can-unlink-group-saml-when-disabled.yml

+---
+title: Ensure users can unlink Group SAML when the group has SAML disabled
+merge_request: 32655
+author:
+type: fixed

ee/spec/controllers/groups/sso_controller_spec.rb

@@ -62,6 +62,25 @@ describe Groups::SsoController do
       end
     end
 
+    context 'when SAML trial has expired' do
+      before do
+        create(:group_saml_identity, saml_provider: saml_provider, user: user)
+        stub_licensed_features(group_saml: false)
+      end
+
+      it 'DELETE /unlink still allows account unlinking' do
+        expect do
+          delete :unlink, params: { group_id: group }
+        end.to change(Identity, :count).by(-1)
+      end
+
+      it 'GET /saml renders 404' do
+        get :saml, params: { group_id: group }
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+
     context 'when user is not signed in' do
       it 'acts as route not found' do
         sign_out(user)

ee/spec/features/profiles/account_spec.rb

@@ -10,8 +10,9 @@ describe 'Profile > Account' do
   end
 
   describe "Disconnect Group SAML", :js do
-    let(:group) { create(:group, :private, name: 'Test Group') }
-    let(:saml_provider) { create(:saml_provider, group: group) }
+    let_it_be(:group) { create(:group, :private, name: 'Test Group') }
+    let_it_be(:saml_provider) { create(:saml_provider, group: group) }
+    let_it_be(:unlink_label) { "SAML for Test Group" }
 
     def enable_group_saml
       stub_licensed_features(group_saml: true)
@@ -33,16 +34,52 @@ describe 'Profile > Account' do
     it 'unlinks account' do
       visit profile_account_path
 
-      unlink_label = "SAML for Test Group"
-
       expect(page).to have_content unlink_label
       click_link "Disconnect"
 
       expect(current_path).to eq profile_account_path
       expect(page).not_to have_content(unlink_label)
+    end
+
+    it 'removes access to the group' do
+      visit profile_account_path
+
+      click_link "Disconnect"
 
       visit group_path(group)
       expect(page).to have_content('Page Not Found')
     end
+
+    context 'group has disabled SAML' do
+      before do
+        saml_provider.update!(enabled: false)
+      end
+
+      it 'lets members distrust and unlink authentication' do
+        visit profile_account_path
+
+        expect(page).to have_content unlink_label
+        click_link "Disconnect"
+
+        expect(current_path).to eq profile_account_path
+        expect(page).not_to have_content(unlink_label)
+      end
+    end
+
+    context 'group trial has expired' do
+      before do
+        stub_licensed_features(group_saml: false)
+      end
+
+      it 'lets members distrust and unlink authentication' do
+        visit profile_account_path
+
+        expect(page).to have_content unlink_label
+        click_link "Disconnect"
+
+        expect(current_path).to eq profile_account_path
+        expect(page).not_to have_content(unlink_label)
+      end
+    end
   end
 end