Merge branch 'security-jej/restrict-group-saml-when-not-enabled' into 'master'

Obey GitLab.com group SAML enabled? setting

See merge request gitlab/gitlab-ee!826

ee/app/controllers/groups/sso_controller.rb

@@ -6,6 +6,7 @@ class Groups::SsoController < Groups::ApplicationController
   before_action :check_group_saml_configured
   before_action :check_group_saml_available!
   before_action :require_configured_provider
+  before_action :require_enabled_provider, except: [:unlink]
   before_action :authenticate_user!, only: [:unlink]
   before_action :check_user_can_sign_in_with_provider, only: [:saml]
   before_action :redirect_if_group_moved
@@ -50,6 +51,16 @@ class Groups::SsoController < Groups::ApplicationController
   def require_configured_provider
     return if @unauthenticated_group.saml_provider
 
+    redirect_settings_or_not_found
+  end
+
+  def require_enabled_provider
+    return if @unauthenticated_group.saml_provider&.enabled?
+
+    redirect_settings_or_not_found
+  end
+
+  def redirect_settings_or_not_found
     if can?(current_user, :admin_group_saml, @unauthenticated_group)
       flash[:notice] = 'SAML sign on has not been configured for this group'
 

ee/changelogs/unreleased/security-jej-restrict-group-saml-when-not-enabled.yml

+---
+title: Prevent SAML access when disabled by group admin on GitLab.com
+merge_request:
+author:
+type: security

ee/lib/gitlab/auth/group_saml/group_lookup.rb

@@ -21,7 +21,7 @@ module Gitlab
         end
 
         def group_saml_enabled?
-          saml_provider && group.feature_available?(:group_saml)
+          saml_provider&.enabled? && group.feature_available?(:group_saml)
         end
 
         def token_discoverable?

ee/spec/controllers/groups/sso_controller_spec.rb

@@ -25,6 +25,34 @@ describe Groups::SsoController do
       expect(assigns[:group_name]).to eq 'our-group'
     end
 
+    it 'allows account unlinking' do
+      create(:group_saml_identity, saml_provider: saml_provider, user: user)
+
+      expect do
+        delete :unlink, params: { group_id: group }
+      end.to change(Identity, :count).by(-1)
+    end
+
+    context 'when SAML is disabled for the group' do
+      before do
+        saml_provider.update!(enabled: false)
+      end
+
+      it 'renders 404' do
+        get :saml, params: { group_id: group }
+
+        expect(response).to have_gitlab_http_status(404)
+      end
+
+      it 'still allows account unlinking' do
+        create(:group_saml_identity, saml_provider: saml_provider, user: user)
+
+        expect do
+          delete :unlink, params: { group_id: group }
+        end.to change(Identity, :count).by(-1)
+      end
+    end
+
     context 'when user is not signed in' do
       it 'acts as route not found' do
         sign_out(user)

ee/spec/lib/omni_auth/strategies/group_saml_spec.rb

@@ -57,6 +57,14 @@ describe OmniAuth::Strategies::GroupSaml, type: :strategy do
         options = last_request.env['omniauth.strategy'].options
         expect(options['idp_cert_fingerprint']).to eq fingerprint
       end
+
+      it 'returns 404 when SAML is disabled for the group' do
+        saml_provider.update!(enabled: false)
+
+        expect do
+          post "/groups/my-group/-/saml/callback", SAMLResponse: saml_response
+        end.to raise_error(ActionController::RoutingError)
+      end
     end
 
     context 'with invalid SAMLResponse' do