LG-226 Fix Double Render Error with Bad Saml Packet on Logout

app/controllers/concerns/saml_idp_logout_concern.rb

@@ -85,7 +85,6 @@ def prepare_saml_logout_response
   end
 
   def prepare_saml_logout_request
-    validate_saml_request
     return if slo_session[:logout_response]
     # store originating SP's logout response in the user session
     # for final step in SLO

app/controllers/saml_idp_controller.rb

@@ -11,6 +11,7 @@ class SamlIdpController < ApplicationController
   include VerifySPAttributesConcern
 
   skip_before_action :verify_authenticity_token
+  before_action :validate_saml_logout_request, only: :logout
 
   def auth
     return confirm_two_factor_authenticated(request_id) unless user_fully_authenticated?
@@ -26,7 +27,6 @@ def metadata
   end
 
   def logout
-    track_logout_event
     prepare_saml_logout_response_and_request
 
     return handle_saml_logout_response if slo.successful_saml_response?
@@ -38,6 +38,21 @@ def logout
 
   private
 
+  def validate_saml_logout_request(raw_saml_request = params[:SAMLRequest])
+    request_valid = saml_request_valid?(raw_saml_request)
+
+    track_logout_event(request_valid)
+    return unless raw_saml_request
+
+    head :bad_request unless request_valid
+  end
+
+  def saml_request_valid?(saml_request)
+    return false unless saml_request
+    decode_request(saml_request)
+    valid_saml_request?
+  end
+
   def saml_metadata
     if SamlCertRotationManager.use_new_secrets_for_request?(request)
       cert_rotation_saml_metadata
@@ -96,11 +111,14 @@ def render_template_for(message, action_url, type)
     )
   end
 
-  def track_logout_event
+  def track_logout_event(saml_request_valid)
+    saml_request = params[:SAMLRequest]
     result = {
-      sp_initiated: params[:SAMLRequest].present?,
+      sp_initiated: saml_request.present?,
       oidc: false,
     }
+    result[:saml_request_valid] = saml_request_valid
+
     analytics.track_event(Analytics::LOGOUT_INITIATED, result)
   end
 end

spec/controllers/saml_idp_controller_spec.rb

@@ -19,7 +19,7 @@
 
     it 'tracks the event when idp-initiated' do
       stub_analytics
-      result = { sp_initiated: false, oidc: false }
+      result = { sp_initiated: false, oidc: false, saml_request_valid: false }
 
       expect(@analytics).to receive(:track_event).with(Analytics::LOGOUT_INITIATED, result)
 
@@ -29,7 +29,16 @@
     it 'tracks the event when sp-initiated' do
       allow(controller).to receive(:saml_request).and_return(FakeSamlRequest.new)
       stub_analytics
-      result = { sp_initiated: true, oidc: false }
+      result = { sp_initiated: true, oidc: false, saml_request_valid: true }
+
+      expect(@analytics).to receive(:track_event).with(Analytics::LOGOUT_INITIATED, result)
+
+      delete :logout, params: { SAMLRequest: 'foo' }
+    end
+
+    it 'tracks the event when sp-initiated and the saml request is not valid' do
+      stub_analytics
+      result = { sp_initiated: true, oidc: false, saml_request_valid: false }
 
       expect(@analytics).to receive(:track_event).with(Analytics::LOGOUT_INITIATED, result)
 
@@ -38,6 +47,16 @@
   end
 
   describe 'POST /api/saml/logout' do
+    context 'when there is an invalid SAML packet' do
+      let(:user) { create(:user, :signed_up) }
+
+      it 'responds with "400 Bad Request"' do
+        sign_in user
+
+        post :logout, params: { SAMLRequest: 'foo' }
+        expect(response.status).to eq(400)
+      end
+    end
     context 'when SAML response is not successful' do
       let(:user) { create(:user, :signed_up) }