stop using a singleton for SAML::SettingsService (#447)

* stop using a singleton for SAML::SettingsService

* do the whole SSL thing

* fix spec

app/controllers/application_controller.rb

@@ -86,7 +86,7 @@ def render_unauthorized
   end
 
   def saml_settings
-    settings = SAML::SettingsService.instance.saml_settings
+    settings = SAML::SettingsService.new.saml_settings
     # TODO: 'level' should be its own class with proper validation
     level = LOA::MAPPING.invert[params[:level]&.to_i]
     settings.authn_context = level || LOA::MAPPING.invert[1]

lib/saml/settings_service.rb

@@ -3,14 +3,22 @@ module SAML
   # This class is responsible for putting together a complete ruby-saml
   # SETTINGS object, meaning, our static SP settings + the IDP settings
   # which must be fetched once and only once via IDP metadata.
-  require 'singleton'
   class SettingsService
-    include Singleton
-
     attr_reader :saml_settings
 
+    METADATA_URI = URI(SAML_CONFIG['metadata_url'])
+
     def initialize
-      @saml_settings ||= fetch_idp_metadata
+      @saml_settings = create_settings_from_metadata
+    end
+
+    def self.metadata
+      return @metadata if defined?(@metadata)
+      http = Net::HTTP.new(METADATA_URI.host, METADATA_URI.port)
+      http.use_ssl = true
+      http.verify_mode = OpenSSL::SSL::VERIFY_PEER
+      get = Net::HTTP::Get.new(METADATA_URI.request_uri)
+      @metadata = http.request(get).body
     end
 
     private
@@ -31,9 +39,9 @@ def settings
     end
 
     ## Makes an external web call to get IDP metadata and populates SETTINGS
-    def fetch_idp_metadata
+    def create_settings_from_metadata
       parser = OneLogin::RubySaml::IdpMetadataParser.new
-      parser.parse_remote(SAML_CONFIG['metadata_url'], true, settings: settings)
+      parser.parse(self.class.metadata, settings: settings)
     end
   end
 end

spec/controllers/v0/sessions_controller_spec.rb

@@ -24,8 +24,9 @@
   let(:query_token) { Rack::Utils.parse_query(URI.parse(response.location).query)['token'] }
 
   before(:each) do
+    stub_request(:get, SAML_CONFIG['metadata_url']).to_return(body: 'abc')
     allow_any_instance_of(Decorators::MviUserDecorator).to receive(:create).and_return(mvi_user)
-    allow(settings_service).to receive_message_chain(:instance, :saml_settings).and_return(settings_no_context)
+    allow_any_instance_of(SAML::SettingsService).to receive(:saml_settings).and_return(settings_no_context)
   end
 
   context 'when not logged in' do

spec/lib/saml/saml_settings_service_spec.rb

@@ -3,15 +3,11 @@
 require 'saml/settings_service'
 
 RSpec.describe SAML::SettingsService do
-  # testing singletons is tricky since it may be initialized in a previous spec example,
-  # use this this instance variable for testing instead
-  before(:each) { @service_instance = SAML::SettingsService.clone }
-
   it 'should only ever make 1 external web call' do
-    stub_request(:get, SAML_CONFIG['metadata_url'])
-    @service_instance.instance.saml_settings
-    @service_instance.instance.saml_settings
-    @service_instance.instance.saml_settings
-    expect(a_request(:get, SAML_CONFIG['metadata_url'])).to have_been_made.times(1)
+    stub_request(:get, SAML_CONFIG['metadata_url']).to_return(body: 'abc')
+    SAML::SettingsService.new.saml_settings
+    SAML::SettingsService.new.saml_settings
+    SAML::SettingsService.new.saml_settings
+    expect(a_request(:get, SAML_CONFIG['metadata_url'])).to have_been_made.at_most_once
   end
 end