diff --git a/app/controllers/concerns/saml_idp_auth_concern.rb b/app/controllers/concerns/saml_idp_auth_concern.rb index 7e19ba70df3..db3f2015c02 100644 --- a/app/controllers/concerns/saml_idp_auth_concern.rb +++ b/app/controllers/concerns/saml_idp_auth_concern.rb @@ -7,6 +7,7 @@ module SamlIdpAuthConcern included do # rubocop:disable Rails/LexicallyScopedActionFilter + before_action :validate_year before_action :validate_and_create_saml_request_object, only: :auth before_action :validate_service_provider_and_authn_context, only: :auth before_action :check_sp_active, only: :auth @@ -21,6 +22,12 @@ module SamlIdpAuthConcern private + def validate_year + if !SamlEndpoint.valid_year?(params[:path_year]) + render plain: 'Invalid Year', status: :bad_request + end + end + def sign_out_if_forceauthn_is_true_and_user_is_signed_in if !saml_request.force_authn? set_issuer_forced_reauthentication( diff --git a/app/services/saml_endpoint.rb b/app/services/saml_endpoint.rb index a3feebdc86c..bd235e52464 100644 --- a/app/services/saml_endpoint.rb +++ b/app/services/saml_endpoint.rb @@ -1,12 +1,22 @@ # frozen_string_literal: true class SamlEndpoint + SAML_YEARS = AppArtifacts.store.members.map(&:to_s).map do |key| + regex = /saml_(?\d{4})_(?key|cert)/ + matches = regex.match(key) + matches && matches[:year] + end.compact.uniq.freeze + attr_reader :year def initialize(year) @year = year end + def self.valid_year?(year) + SAML_YEARS.include?(year) + end + def self.suffixes endpoint_configs.pluck(:suffix) end diff --git a/spec/controllers/saml_idp_controller_spec.rb b/spec/controllers/saml_idp_controller_spec.rb index 7d5a2b5a49d..c01bfe99204 100644 --- a/spec/controllers/saml_idp_controller_spec.rb +++ b/spec/controllers/saml_idp_controller_spec.rb @@ -8,6 +8,36 @@ let(:path_year) { SamlAuthHelper::PATH_YEAR } describe '/api/saml/logout' do + let(:service_provider) do + create( + :service_provider, + certs: ['sp_sinatra_demo', 'saml_test_sp'], + active: true, + assertion_consumer_logout_service_url: 'https://example.com', + ) + end + + let(:right_cert_settings) do + saml_settings( + overrides: { + issuer: service_provider.issuer, + assertion_consumer_logout_service_url: 'https://example.com', + }, + ) + end + + let(:wrong_cert_settings) do + saml_settings( + overrides: { + issuer: service_provider.issuer, + certificate: File.read(Rails.root.join('certs', 'sp', 'saml_test_sp2.crt')), + private_key: OpenSSL::PKey::RSA.new( + File.read(Rails.root + 'keys/saml_test_sp2.key'), + ).to_pem, + }, + ) + end + it 'assigns devise session limited failure redirect url' do delete :logout, params: { path_year: path_year } @@ -25,6 +55,17 @@ ) end + context 'with invalid year' do + let(:path_year) { 1999 } + + it 'returns bad request error' do + delete :logout, params: { SAMLRequest: 'foo', path_year: path_year } + + expect(response).to be_bad_request + expect(response.body).to eq('Invalid Year') + end + end + it 'tracks the event when sp initiated' do allow(controller).to receive(:saml_request).and_return(FakeSamlLogoutRequest.new) stub_analytics @@ -55,36 +96,6 @@ ) end - let(:service_provider) do - create( - :service_provider, - certs: ['sp_sinatra_demo', 'saml_test_sp'], - active: true, - assertion_consumer_logout_service_url: 'https://example.com', - ) - end - - let(:right_cert_settings) do - saml_settings( - overrides: { - issuer: service_provider.issuer, - assertion_consumer_logout_service_url: 'https://example.com', - }, - ) - end - - let(:wrong_cert_settings) do - saml_settings( - overrides: { - issuer: service_provider.issuer, - certificate: File.read(Rails.root.join('certs', 'sp', 'saml_test_sp2.crt')), - private_key: OpenSSL::PKey::RSA.new( - File.read(Rails.root + 'keys/saml_test_sp2.key'), - ).to_pem, - }, - ) - end - it 'accepts requests from a correct cert' do saml_request = UriService.params( OneLogin::RubySaml::Logoutrequest.new.create(right_cert_settings), @@ -207,21 +218,6 @@ end describe '/api/saml/remotelogout' do - it 'tracks the event when the saml request is invalid' do - stub_analytics - - post :remotelogout, params: { SAMLRequest: 'foo', path_year: path_year } - - expect(@analytics).to have_logged_event('Remote Logout initiated', saml_request_valid: false) - expect(@analytics).to have_logged_event( - :sp_integration_errors_present, - error_details: [:issuer_missing_or_invald, :no_auth_or_logout_request, :invalid_signature], - error_types: { saml_request_errors: true }, - event: :saml_remote_logout_request, - integration_exists: false, - ) - end - let(:agency) { create(:agency) } let(:service_provider) do create( @@ -318,6 +314,21 @@ ) end + it 'tracks the event when the saml request is invalid' do + stub_analytics + + post :remotelogout, params: { SAMLRequest: 'foo', path_year: path_year } + + expect(@analytics).to have_logged_event('Remote Logout initiated', saml_request_valid: false) + expect(@analytics).to have_logged_event( + :sp_integration_errors_present, + error_details: [:issuer_missing_or_invald, :no_auth_or_logout_request, :invalid_signature], + error_types: { saml_request_errors: true }, + event: :saml_remote_logout_request, + integration_exists: false, + ) + end + it 'accepts requests with correct cert and correct session index and renders logout response' do REDIS_POOL.with { |client| client.flushdb } session_accessor = OutOfBandSessionAccessor.new(session_id) @@ -521,6 +532,15 @@ expect(response.media_type).to eq 'text/xml' end + context 'with invalid year' do + let(:path_year) { 1999 } + + it 'returns bad request error' do + expect(response).to be_bad_request + expect(response.body).to eq('Invalid Year') + end + end + it 'contains an EntityDescriptor nodeset' do expect(xmldoc.metadata_nodeset.length).to eq(1) end @@ -2139,7 +2159,7 @@ def name_id_version(format_urn) context 'with missing SAMLRequest params' do it 'responds with "403 Forbidden"' do - get :auth + get :auth, params: { path_year: path_year } expect(response.status).to eq(403) end @@ -2147,7 +2167,7 @@ def name_id_version(format_urn) context 'with invalid SAMLRequest param' do it 'responds with "403 Forbidden"' do - get :auth + get :auth, params: { path_year: path_year } expect(response.status).to eq(403) end