Merge pull request #222 from stevecrozz/detect_metadata_parse_failure

No more silent failure fetching idp metadata. (OneLogin::RubySaml::HttpError raised)

Author: pitbulk

lib/onelogin/ruby-saml.rb

@@ -9,6 +9,7 @@
 require 'onelogin/ruby-saml/response'
 require 'onelogin/ruby-saml/settings'
 require 'onelogin/ruby-saml/attribute_service'
+require 'onelogin/ruby-saml/http_error'
 require 'onelogin/ruby-saml/validation_error'
 require 'onelogin/ruby-saml/metadata'
 require 'onelogin/ruby-saml/idp_metadata_parser'

lib/onelogin/ruby-saml/http_error.rb

@@ -0,0 +1,7 @@
+module OneLogin
+  module RubySaml
+    class HttpError < StandardError
+    end
+  end
+end
+

lib/onelogin/ruby-saml/idp_metadata_parser.rb

@@ -20,11 +20,14 @@ class IdpMetadataParser
       DSIG     = "http://www.w3.org/2000/09/xmldsig#"
 
       attr_reader :document
+      attr_reader :response
 
-      # Parse the Identity Provider metadata and update the settings with the IdP values
-      # @param url [String] Url where the XML of the Identity Provider Metadata is published.
-      # @param validate_cert [Boolean] If true and the URL is HTTPs, the cert of the domain is checked.
+      # Parse the Identity Provider metadata and update the settings with the
+      # IdP values
       #
+      # @param (see IdpMetadataParser#get_idp_metadata)
+      # @return (see IdpMetadataParser#get_idp_metadata)
+      # @raise (see IdpMetadataParser#get_idp_metadata)
       def parse_remote(url, validate_cert = true)
         idp_metadata = get_idp_metadata(url, validate_cert)
         parse(idp_metadata)
@@ -51,7 +54,7 @@ def parse(idp_metadata)
       # @param url [String] Url where the XML of the Identity Provider Metadata is published.
       # @param validate_cert [Boolean] If true and the URL is HTTPs, the cert of the domain is checked.
       # @return [REXML::document] Parsed XML IdP metadata
-      #
+      # @raise [HttpError] Failure to fetch remote IdP metadata
       def get_idp_metadata(url, validate_cert)
         uri = URI.parse(url)
         if uri.scheme == "http"
@@ -75,7 +78,14 @@ def get_idp_metadata(url, validate_cert)
           get = Net::HTTP::Get.new(uri.request_uri)
           response = http.request(get)
           meta_text = response.body
+        else
+          raise ArgumentError.new("url must begin with http or https")
         end
+
+        unless response.is_a? Net::HTTPSuccess
+          raise OneLogin::RubySaml::HttpError.new("Failed to fetch idp metadata")
+        end
+
         meta_text
       end
 

test/idp_metadata_parser_test.rb

@@ -3,8 +3,17 @@
 require 'onelogin/ruby-saml/idp_metadata_parser'
 
 class IdpMetadataParserTest < Minitest::Test
+  class MockSuccessResponse < Net::HTTPSuccess
+    # override parent's initialize
+    def initialize; end
+
+    attr_accessor :body
+  end
+
+  class MockFailureResponse < Net::HTTPNotFound
+    # override parent's initialize
+    def initialize; end
 
-  class MockResponse
     attr_accessor :body
   end
 
@@ -24,7 +33,7 @@ class MockResponse
 
   describe "download and parse IdP descriptor file" do
     before do
-      mock_response = MockResponse.new
+      mock_response = MockSuccessResponse.new
       mock_response.body = idp_metadata
       @url = "https://example.com"
       uri = URI(@url)
@@ -34,7 +43,6 @@ class MockResponse
       @http.expects(:request).returns(mock_response)
     end
 
-
     it "extract settings from remote xml" do
       idp_metadata_parser = OneLogin::RubySaml::IdpMetadataParser.new
       settings = idp_metadata_parser.parse_remote(@url)
@@ -49,10 +57,39 @@ class MockResponse
 
     it "accept self signed certificate if insturcted" do
       idp_metadata_parser = OneLogin::RubySaml::IdpMetadataParser.new
-      settings = idp_metadata_parser.parse_remote(@url, false)
+      idp_metadata_parser.parse_remote(@url, false)
 
       assert_equal OpenSSL::SSL::VERIFY_NONE, @http.verify_mode
     end
   end
 
+  describe "download failure cases" do
+    it "raises an exception when the url has no scheme" do
+      idp_metadata_parser = OneLogin::RubySaml::IdpMetadataParser.new
+
+      exception = assert_raises(ArgumentError) do
+        idp_metadata_parser.parse_remote("blahblah")
+      end
+
+      assert_equal("url must begin with http or https", exception.message)
+    end
+
+    it "raises an exception when unable to download metadata" do
+      mock_response = MockFailureResponse.new
+      @url = "https://example.com"
+      uri = URI(@url)
+
+      @http = Net::HTTP.new(uri.host, uri.port)
+      Net::HTTP.expects(:new).returns(@http)
+      @http.expects(:request).returns(mock_response)
+
+      idp_metadata_parser = OneLogin::RubySaml::IdpMetadataParser.new
+
+      exception = assert_raises(OneLogin::RubySaml::HttpError) do
+        idp_metadata_parser.parse_remote("https://example.hello.com/access/saml/idp.xml")
+      end
+
+      assert_equal("Failed to fetch idp metadata", exception.message)
+    end
+  end
 end