[feat] Collect request validation errors (#18)

* wip add error collector

* Fix type and rewrite request with proper validation test cases

* Lead error render decision to gem user

* Validate the certificate's existence before verifying the signature.

---------

Co-authored-by: zogoo <[email protected]>

lib/saml_idp/controller.rb

@@ -34,10 +34,7 @@ def acs_url
 
     def validate_saml_request(raw_saml_request = params[:SAMLRequest])
       decode_request(raw_saml_request, params[:Signature], params[:SigAlg], params[:RelayState])
-      return true if valid_saml_request?
-
-      head :forbidden if defined?(::Rails)
-      false
+      valid_saml_request?
     end
 
     def decode_request(raw_saml_request, signature, sig_algorithm, relay_state)

lib/saml_idp/request.rb

@@ -3,6 +3,8 @@
 require 'logger'
 module SamlIdp
   class Request
+    attr_accessor :errors
+
     def self.from_deflated_request(raw, external_attributes = {})
       if raw
         decoded = Base64.decode64(raw)
@@ -34,6 +36,7 @@ def initialize(raw_xml = "", external_attributes = {})
       self.relay_state = external_attributes[:relay_state]
       self.sig_algorithm = external_attributes[:sig_algorithm]
       self.signature = external_attributes[:signature]
+      self.errors = []
     end
 
     def logout_request?
@@ -89,37 +92,53 @@ def log(msg)
       end
     end
 
+    def collect_errors(error_type)
+      errors.push(error_type)
+    end
+
     def valid?(external_attributes = {})
       unless service_provider?
         log "Unable to find service provider for issuer #{issuer}"
+        collect_errors(:sp_not_found)
         return false
       end
 
       unless (authn_request? ^ logout_request?)
         log "One and only one of authnrequest and logout request is required. authnrequest: #{authn_request?} logout_request: #{logout_request?} "
+        collect_errors(:unaccepted_request)
+        return false
+      end
+
+      if (logout_request? || validate_auth_request_signature?) && (service_provider.cert.to_s.empty? || !!service_provider.fingerprint.to_s.empty?)
+        log "Verifying request signature is required. But certificate and fingerprint was empty."
+        collect_errors(:empty_certificate)
         return false
       end
 
       # XML embedded signature
       if signature.nil? && !valid_signature?
         log "Requested document signature is invalid in #{raw_xml}"
+        collect_errors(:invalid_embedded_signature)
         return false
       end
 
       # URI query signature
       if signature.present? && !valid_external_signature?
         log "Requested URI signature is invalid in #{raw_xml}"
+        collect_errors(:invalid_external_signature)
         return false
       end
 
       if response_url.nil?
         log "Unable to find response url for #{issuer}: #{raw_xml}"
+        collect_errors(:empty_response_url)
         return false
       end
 
       if !service_provider.acceptable_response_hosts.include?(response_host)
         log "#{service_provider.acceptable_response_hosts} compare to #{response_host}"
         log "No acceptable AssertionConsumerServiceURL, either configure them via config.service_provider.response_hosts or match to your metadata_url host"
+        collect_errors(:not_allowed_host)
         return false
       end
 
@@ -152,6 +171,9 @@ def valid_external_signature?
       end
 
       cert.public_key.verify(signature_algorithm.new, raw_signature, query_request_string)
+    rescue OpenSSL::X509::CertificateError => e
+      log e.message
+      collect_errors(:cert_format_error)
     end
 
     def service_provider?

spec/lib/saml_idp/controller_spec.rb

@@ -81,16 +81,6 @@ def params
         expect(response.is_valid?).to be_truthy
       end
 
-      it "should create a SAML Logout Response" do
-        params[:SAMLRequest] = make_saml_logout_request
-        expect(validate_saml_request).to eq(true)
-        expect(saml_request.logout_request?).to eq true
-        saml_response = encode_response(principal)
-        response = OneLogin::RubySaml::Logoutresponse.new(saml_response, saml_settings)
-        expect(response.validate).to eq(true)
-        expect(response.issuer).to eq("http://example.com")
-      end
-
       it "should by default create a SAML Response with a signed assertion" do
         saml_response = encode_response(principal)
         response = OneLogin::RubySaml::Response.new(saml_response)
@@ -138,5 +128,18 @@ def params
     it 'should successfully validate signature' do
       expect(validate_saml_request).to eq(true)
     end
+
+    context "solicited Response" do
+      let(:principal) { double email_address: "[email protected]" }
+
+      it "should create a SAML Logout Response" do
+        expect(validate_saml_request).to eq(true)
+        expect(saml_request.logout_request?).to eq true
+        saml_response = encode_response(principal)
+        response = OneLogin::RubySaml::Logoutresponse.new(saml_response, saml_settings)
+        expect(response.validate).to eq(true)
+        expect(response.issuer).to eq("http://idp.com/saml/idp")
+      end
+    end
   end
 end