diff --git a/.reek b/.reek index 12bda872d89..3276cc4e715 100644 --- a/.reek +++ b/.reek @@ -3,6 +3,7 @@ Attribute: ControlParameter: exclude: - CustomDeviseFailureApp#i18n_message + - OpenidConnectRedirector#initialize - NoRetryJobs#call - PhoneFormatter#self.format - Users::TwoFactorAuthenticationController#invalid_phone_number @@ -88,6 +89,7 @@ TooManyConstants: TooManyInstanceVariables: exclude: - OpenidConnectAuthorizeForm + - OpenidConnectRedirector - Idv::VendorResult - CloudhsmKeyGenerator TooManyStatements: diff --git a/app/controllers/concerns/secure_headers_concern.rb b/app/controllers/concerns/secure_headers_concern.rb index 23da64ea5c3..74a11e2078b 100644 --- a/app/controllers/concerns/secure_headers_concern.rb +++ b/app/controllers/concerns/secure_headers_concern.rb @@ -5,20 +5,17 @@ def apply_secure_headers_override return if stored_url_for_user.blank? authorize_params = URIService.params(stored_url_for_user) + authorize_form = OpenidConnectAuthorizeForm.new(authorize_params) return unless authorize_form.valid? - redirect_uri = authorize_params[:redirect_uri] - override_content_security_policy_directives( - form_action: ["'self'", redirect_uri].compact, + form_action: ["'self'", authorize_form.sp_redirect_uri].compact, preserve_schemes: true ) end - private - def stored_url_for_user sp_session[:request_url] end diff --git a/app/controllers/openid_connect/authorization_controller.rb b/app/controllers/openid_connect/authorization_controller.rb index ad130f16596..3d499416659 100644 --- a/app/controllers/openid_connect/authorization_controller.rb +++ b/app/controllers/openid_connect/authorization_controller.rb @@ -52,7 +52,7 @@ def track_authorize_analytics(result) def apply_secure_headers_override override_content_security_policy_directives( - form_action: ["'self'", authorization_params[:redirect_uri]].compact, + form_action: ["'self'", @authorize_form.sp_redirect_uri].compact, preserve_schemes: true ) end diff --git a/app/decorators/service_provider_session_decorator.rb b/app/decorators/service_provider_session_decorator.rb index 1e31ad4321a..8e91dce7f04 100644 --- a/app/decorators/service_provider_session_decorator.rb +++ b/app/decorators/service_provider_session_decorator.rb @@ -92,12 +92,8 @@ def sp_agency end def sp_return_url - if sp.redirect_uris.present? && valid_oidc_request? - URIService.add_params( - oidc_redirect_uri, - error: 'access_denied', - state: request_params[:state] - ) + if sp.redirect_uris.present? && openid_connect_redirector.valid? + openid_connect_redirector.decline_redirect_uri else sp.return_to_sp_url end @@ -131,19 +127,7 @@ def request_url sp_session[:request_url] || service_provider_request.url end - def valid_oidc_request? - authorize_form .valid? - end - - def authorize_form - OpenidConnectAuthorizeForm.new(request_params) - end - - def oidc_redirect_uri - request_params[:redirect_uri] - end - - def request_params - @request_params ||= URIService.params(request_url) + def openid_connect_redirector + @_openid_connect_redirector ||= OpenidConnectRedirector.from_request_url(request_url) end end diff --git a/app/forms/openid_connect_authorize_form.rb b/app/forms/openid_connect_authorize_form.rb index 83596f7876b..4ee64191fd9 100644 --- a/app/forms/openid_connect_authorize_form.rb +++ b/app/forms/openid_connect_authorize_form.rb @@ -2,7 +2,6 @@ class OpenidConnectAuthorizeForm include ActiveModel::Model include ActionView::Helpers::TranslationHelper - include RedirectUriValidator SIMPLE_ATTRS = %i[ client_id @@ -34,6 +33,7 @@ class OpenidConnectAuthorizeForm validate :validate_acr_values validate :validate_client_id + validate :validate_redirect_uri validate :validate_scope def initialize(params) @@ -43,6 +43,10 @@ def initialize(params) instance_variable_set(:"@#{key}", params[key]) end @prompt ||= 'select_account' + + @openid_connect_redirector = OpenidConnectRedirector.new( + redirect_uri: redirect_uri, service_provider: service_provider, state: state, errors: errors + ) end def submit @@ -55,6 +59,10 @@ def loa3_requested? loa == 3 end + def sp_redirect_uri + openid_connect_redirector.validated_input_redirect_uri + end + def service_provider @_service_provider ||= ServiceProvider.from_issuer(client_id) end @@ -71,15 +79,13 @@ def link_identity_to_service_provider(current_user, rails_session_id) end def success_redirect_uri - uri = redirect_uri unless errors.include?(:redirect_uri) code = identity&.session_uuid - - URIService.add_params(uri, code: code, state: state) if code + openid_connect_redirector.success_redirect_uri(code: code) if code end private - attr_reader :identity, :success, :already_linked + attr_reader :identity, :success, :openid_connect_redirector, :already_linked def requested_attributes @requested_attributes ||= @@ -101,6 +107,10 @@ def validate_client_id errors.add(:client_id, t('openid_connect.authorization.errors.bad_client_id')) end + def validate_redirect_uri + openid_connect_redirector.validate + end + def validate_scope return if scope.present? errors.add(:scope, t('openid_connect.authorization.errors.no_valid_scope')) @@ -131,14 +141,7 @@ def result_uri end def error_redirect_uri - uri = redirect_uri unless errors.include?(:redirect_uri) - - URIService.add_params( - uri, - error: 'invalid_request', - error_description: errors.full_messages.join(' '), - state: state - ) + openid_connect_redirector.error_redirect_uri end end # rubocop:enable Metrics/ClassLength diff --git a/app/forms/openid_connect_logout_form.rb b/app/forms/openid_connect_logout_form.rb index 98bc7d2af4a..1b21e5bd241 100644 --- a/app/forms/openid_connect_logout_form.rb +++ b/app/forms/openid_connect_logout_form.rb @@ -1,7 +1,6 @@ class OpenidConnectLogoutForm include ActiveModel::Model include ActionView::Helpers::TranslationHelper - include RedirectUriValidator ATTRS = %i[ id_token_hint @@ -17,6 +16,7 @@ class OpenidConnectLogoutForm validates :post_logout_redirect_uri, presence: true validates :state, presence: true, length: { minimum: RANDOM_VALUE_MINIMUM_LENGTH } + validate :validate_redirect_uri validate :validate_identity def initialize(params) @@ -25,6 +25,7 @@ def initialize(params) end @identity = load_identity + @openid_connect_redirector = build_openid_connect_redirector end def submit @@ -38,6 +39,7 @@ def submit private attr_reader :identity, + :openid_connect_redirector, :success def load_identity @@ -56,6 +58,20 @@ def identity_from_payload(payload) AgencyIdentityLinker.sp_identity_from_uuid_and_sp(uuid, sp) end + def build_openid_connect_redirector + OpenidConnectRedirector.new( + redirect_uri: post_logout_redirect_uri, + service_provider: service_provider, + state: state, + errors: errors, + error_attr: :post_logout_redirect_uri + ) + end + + def validate_redirect_uri + openid_connect_redirector.validate + end + def validate_identity errors.add(:id_token_hint, t('openid_connect.logout.errors.id_token_hint')) unless identity end @@ -74,23 +90,10 @@ def extra_analytics_attributes end def redirect_uri - success ? logout_redirect_uri : error_redirect_uri - end - - def logout_redirect_uri - uri = post_logout_redirect_uri unless errors.include?(:redirect_uri) - - URIService.add_params(uri, state: state) - end - - def error_redirect_uri - uri = post_logout_redirect_uri unless errors.include?(:redirect_uri) - - URIService.add_params( - uri, - error: 'invalid_request', - error_description: errors.full_messages.join(' '), - state: state - ) + if success + openid_connect_redirector.logout_redirect_uri + else + openid_connect_redirector.error_redirect_uri + end end end diff --git a/app/services/openid_connect_redirector.rb b/app/services/openid_connect_redirector.rb new file mode 100644 index 00000000000..4e5d1b0ebbf --- /dev/null +++ b/app/services/openid_connect_redirector.rb @@ -0,0 +1,97 @@ +class OpenidConnectRedirector + include ActionView::Helpers::TranslationHelper + + def self.from_request_url(request_url) + params = URIService.params(request_url) + + new( + redirect_uri: params[:redirect_uri], + service_provider: ServiceProvider.from_issuer(params[:client_id]), + state: params[:state] + ) + end + + def initialize(redirect_uri:, service_provider:, state:, errors: nil, error_attr: :redirect_uri) + @redirect_uri = redirect_uri + @service_provider = service_provider + @state = state + @errors = errors || ActiveModel::Errors.new(self) + @error_attr = error_attr + end + + def valid? + validate + errors.blank? + end + + def validate + validate_redirect_uri + validate_redirect_uri_matches_sp_redirect_uri + end + + def success_redirect_uri(code:) + URIService.add_params(validated_input_redirect_uri, code: code, state: state) + end + + def decline_redirect_uri + URIService.add_params( + validated_input_redirect_uri, + error: 'access_denied', + state: state + ) + end + + def error_redirect_uri + URIService.add_params( + validated_input_redirect_uri, + error: 'invalid_request', + error_description: errors.full_messages.join(' '), + state: state + ) + end + + def logout_redirect_uri + URIService.add_params(validated_input_redirect_uri, state: state) + end + + def validated_input_redirect_uri + redirect_uri if redirect_uri_matches_sp_redirect_uri? + end + + private + + attr_reader :redirect_uri, :service_provider, :state, :errors, :error_attr + + def validate_redirect_uri + _uri = URI(redirect_uri) + rescue ArgumentError, URI::InvalidURIError + errors.add(error_attr, t('openid_connect.authorization.errors.redirect_uri_invalid')) + end + + def validate_redirect_uri_matches_sp_redirect_uri + return if redirect_uri_matches_sp_redirect_uri? + errors.add(error_attr, t('openid_connect.authorization.errors.redirect_uri_no_match')) + end + + # rubocop:disable Metrics/AbcSize + # rubocop:disable Metrics/CyclomaticComplexity + # rubocop:disable Metrics/MethodLength + def redirect_uri_matches_sp_redirect_uri? + return unless redirect_uri.present? && service_provider.active? + parsed_redirect_uri = URI(redirect_uri) + service_provider.redirect_uris.any? do |sp_redirect_uri| + parsed_sp_redirect_uri = URI(sp_redirect_uri) + + parsed_redirect_uri.scheme == parsed_sp_redirect_uri.scheme && + parsed_redirect_uri.port == parsed_sp_redirect_uri.port && + parsed_redirect_uri.host == parsed_sp_redirect_uri.host && + parsed_redirect_uri.path.start_with?(parsed_sp_redirect_uri.path) + end + rescue URI::Error + false + end + + # rubocop:enable Metrics/AbcSize + # rubocop:enable Metrics/CyclomaticComplexity + # rubocop:enable Metrics/MethodLength +end diff --git a/app/validators/redirect_uri_validator.rb b/app/validators/redirect_uri_validator.rb deleted file mode 100644 index a28c91816c8..00000000000 --- a/app/validators/redirect_uri_validator.rb +++ /dev/null @@ -1,32 +0,0 @@ -module RedirectUriValidator - extend ActiveSupport::Concern - - included do - attr_reader :redirect_uri, :post_logout_redirect_uri, :service_provider - - validate :allowed_redirect_uri - end - - private - - def allowed_redirect_uri - return if any_registered_sp_redirect_uris_identical_to_the_requested_uri? - - errors.add(:redirect_uri, t('openid_connect.authorization.errors.redirect_uri_no_match')) - end - - def any_registered_sp_redirect_uris_identical_to_the_requested_uri? - service_provider.redirect_uris.any? do |sp_redirect_uri| - parsed_sp_redirect_uri = URI.parse(sp_redirect_uri) - - parsed_sp_redirect_uri == parsed_redirect_uri - end - rescue ArgumentError, URI::InvalidURIError - errors.add(:redirect_uri, t('openid_connect.authorization.errors.redirect_uri_invalid')) - end - - def parsed_redirect_uri - requested_uri = post_logout_redirect_uri || redirect_uri - URI.parse(requested_uri) - end -end diff --git a/config/service_providers.yml b/config/service_providers.yml index 451b8f3a9c2..d853dc94075 100644 --- a/config/service_providers.yml +++ b/config/service_providers.yml @@ -47,7 +47,6 @@ test: 'urn:gov:gsa:openidconnect:test': redirect_uris: - 'gov.gsa.openidconnect.test://result' - - 'gov.gsa.openidconnect.test://result/logout' cert: 'saml_test_sp' friendly_name: 'Example iOS App' agency: '18F' @@ -58,7 +57,7 @@ test: 'urn:gov:gsa:openidconnect:sp:server': agency_id: 2 redirect_uris: - - 'http://localhost:7654/auth/result' + - 'http://localhost:7654/' - 'https://example.com' cert: 'saml_test_sp' friendly_name: 'Test SP' @@ -633,7 +632,7 @@ production: attribute_bundle: - x509_subject - x509_presented - + # My Move.mil 'urn:gov:gsa:openidconnect.profiles:sp:sso:dod:mymovemilprod': agency_id: 8 diff --git a/spec/controllers/openid_connect/logout_controller_spec.rb b/spec/controllers/openid_connect/logout_controller_spec.rb index 67bda6e2382..38500e8c69f 100644 --- a/spec/controllers/openid_connect/logout_controller_spec.rb +++ b/spec/controllers/openid_connect/logout_controller_spec.rb @@ -80,15 +80,11 @@ it 'tracks analytics' do stub_analytics - - errors = { - redirect_uri: [t('openid_connect.authorization.errors.redirect_uri_no_match')], - } expect(@analytics).to receive(:track_event). with(Analytics::LOGOUT_INITIATED, success: false, client_id: service_provider, - errors: errors, + errors: hash_including(:post_logout_redirect_uri), sp_initiated: true, oidc: true) diff --git a/spec/features/openid_connect/openid_connect_spec.rb b/spec/features/openid_connect/openid_connect_spec.rb index 314a4ca0a65..eeea3adcfa3 100644 --- a/spec/features/openid_connect/openid_connect_spec.rb +++ b/spec/features/openid_connect/openid_connect_spec.rb @@ -437,7 +437,7 @@ def sign_in_get_id_token response_type: 'code', acr_values: Saml::Idp::Constants::LOA1_AUTHN_CONTEXT_CLASSREF, scope: 'openid email', - redirect_uri: 'gov.gsa.openidconnect.test://result', + redirect_uri: 'gov.gsa.openidconnect.test://result/auth', state: state, prompt: 'select_account', nonce: nonce, diff --git a/spec/features/openid_connect/redirect_uri_validation_spec.rb b/spec/features/openid_connect/redirect_uri_validation_spec.rb deleted file mode 100644 index 2a3f4949044..00000000000 --- a/spec/features/openid_connect/redirect_uri_validation_spec.rb +++ /dev/null @@ -1,259 +0,0 @@ -require 'rails_helper' - -describe 'redirect_uri validation' do - context 'when the redirect_uri in the request does not match one that is registered' do - it 'displays error instead of branded landing page' do - visit_idp_from_sp_with_loa1_with_disallowed_redirect_uri - current_host = URI.parse(page.current_url).host - - expect(current_host).to eq 'www.example.com' - expect(page). - to have_content t('openid_connect.authorization.errors.redirect_uri_no_match') - end - end - - context 'when the redirect_uri is not a valid URI' do - it 'displays error instead of branded landing page' do - visit_idp_from_sp_with_loa1_with_invalid_redirect_uri - current_host = URI.parse(page.current_url).host - - expect(current_host).to eq 'www.example.com' - expect(page). - to have_content t('openid_connect.authorization.errors.redirect_uri_invalid') - end - end - - context 'when the service_provider is not active' do - it 'displays error instead of branded landing page' do - visit_idp_from_inactive_sp - current_host = URI.parse(page.current_url).host - - expect(current_host).to eq 'www.example.com' - expect(page). - to have_content t('openid_connect.authorization.errors.bad_client_id') - end - end - - context 'when redirect_uri is present in params but the request is not from an SP' do - it 'does not provide a link to the redirect_uri' do - visit sign_up_start_path(request_id: '123', redirect_uri: 'evil.com') - - expect(page).to_not have_link t('links.back_to_sp') - - visit new_user_session_path(request_id: '123', redirect_uri: 'evil.com') - - expect(page).to_not have_link t('links.back_to_sp') - end - end - - context 'when new non-SP request with redirect_uri is made after initial SP request' do - it 'does not provide a link to the new redirect_uri' do - state = SecureRandom.hex - visit_idp_from_sp_with_loa1_with_valid_redirect_uri(state: state) - visit sign_up_start_path(request_id: '123', redirect_uri: 'evil.com') - sp_redirect_uri = "http://localhost:7654/auth/result?error=access_denied&state=#{state}" - - expect(page). - to have_link(t('links.back_to_sp', sp: 'Test SP'), href: sp_redirect_uri) - - visit new_user_session_path(request_id: '123', redirect_uri: 'evil.com') - - expect(page). - to have_link(t('links.back_to_sp', sp: 'Test SP'), href: sp_redirect_uri) - end - end - - context 'when the user is already signed in directly' do - it 'displays error instead of redirecting' do - sign_in_and_2fa_user - - visit_idp_from_inactive_sp - current_host = URI.parse(page.current_url).host - - expect(current_host).to eq 'www.example.com' - expect(page). - to have_content t('openid_connect.authorization.errors.bad_client_id') - - visit_idp_from_sp_with_loa1_with_invalid_redirect_uri - current_host = URI.parse(page.current_url).host - - expect(current_host).to eq 'www.example.com' - expect(page). - to have_content t('openid_connect.authorization.errors.redirect_uri_invalid') - - visit_idp_from_sp_with_loa1_with_disallowed_redirect_uri - current_host = URI.parse(page.current_url).host - - expect(current_host).to eq 'www.example.com' - expect(page). - to have_content t('openid_connect.authorization.errors.redirect_uri_no_match') - end - end - - context 'when the user is already signed in via an SP' do - it 'displays error instead of redirecting' do - allow(FeatureManagement).to receive(:prefill_otp_codes?).and_return(true) - user = create(:user, :signed_up) - visit_idp_from_sp_with_loa1_with_valid_redirect_uri - click_link t('links.sign_in') - fill_in_credentials_and_submit(user.email, user.password) - click_submit_default - click_continue - - visit_idp_from_inactive_sp - current_host = URI.parse(page.current_url).host - - expect(current_host).to eq 'www.example.com' - expect(page). - to have_content t('openid_connect.authorization.errors.bad_client_id') - - visit_idp_from_sp_with_loa1_with_invalid_redirect_uri - current_host = URI.parse(page.current_url).host - - expect(current_host).to eq 'www.example.com' - expect(page). - to have_content t('openid_connect.authorization.errors.redirect_uri_invalid') - - visit_idp_from_sp_with_loa1_with_disallowed_redirect_uri - current_host = URI.parse(page.current_url).host - - expect(current_host).to eq 'www.example.com' - expect(page). - to have_content t('openid_connect.authorization.errors.redirect_uri_no_match') - end - end - - context 'when the SP has multiple registered redirect_uris and the second one is requested' do - it 'considers the request valid and redirects to the one requested' do - allow(FeatureManagement).to receive(:prefill_otp_codes?).and_return(true) - user = create(:user, :signed_up) - visit_idp_from_sp_with_loa1_with_second_valid_redirect_uri - click_link t('links.sign_in') - fill_in_credentials_and_submit(user.email, user.password) - click_submit_default - click_continue - - redirect_host = URI.parse(current_url).host - redirect_scheme = URI.parse(current_url).scheme - - expect(redirect_host).to eq('example.com') - expect(redirect_scheme).to eq('https') - end - end - - context 'when the SP does not have any registered redirect_uris' do - it 'considers the request invalid and does not redirect if the user signs in' do - allow(FeatureManagement).to receive(:prefill_otp_codes?).and_return(true) - user = create(:user, :signed_up) - visit_idp_from_sp_that_does_not_have_redirect_uris - current_host = URI.parse(page.current_url).host - - expect(current_host).to eq 'www.example.com' - expect(page). - to have_content t('openid_connect.authorization.errors.redirect_uri_no_match') - - visit new_user_session_path - fill_in_credentials_and_submit(user.email, user.password) - click_submit_default - click_continue - - expect(page).to have_current_path account_path - end - end - - def visit_idp_from_sp_with_loa1_with_disallowed_redirect_uri(state: SecureRandom.hex) - client_id = 'urn:gov:gsa:openidconnect:sp:server' - nonce = SecureRandom.hex - - visit openid_connect_authorize_path( - client_id: client_id, - response_type: 'code', - acr_values: Saml::Idp::Constants::LOA1_AUTHN_CONTEXT_CLASSREF, - scope: 'openid email', - redirect_uri: 'https://example.com.evil.com/auth/result', - state: state, - prompt: 'select_account', - nonce: nonce - ) - end - - def visit_idp_from_sp_with_loa1_with_invalid_redirect_uri(state: SecureRandom.hex) - client_id = 'urn:gov:gsa:openidconnect:sp:server' - nonce = SecureRandom.hex - - visit openid_connect_authorize_path( - client_id: client_id, - response_type: 'code', - acr_values: Saml::Idp::Constants::LOA1_AUTHN_CONTEXT_CLASSREF, - scope: 'openid email', - redirect_uri: ':aaaa', - state: state, - prompt: 'select_account', - nonce: nonce - ) - end - - def visit_idp_from_inactive_sp(state: SecureRandom.hex) - client_id = 'inactive' - nonce = SecureRandom.hex - - visit openid_connect_authorize_path( - client_id: client_id, - response_type: 'code', - acr_values: Saml::Idp::Constants::LOA1_AUTHN_CONTEXT_CLASSREF, - scope: 'openid email', - redirect_uri: 'http://localhost:7654/auth/result', - state: state, - prompt: 'select_account', - nonce: nonce - ) - end - - def visit_idp_from_sp_with_loa1_with_valid_redirect_uri(state: SecureRandom.hex) - client_id = 'urn:gov:gsa:openidconnect:sp:server' - nonce = SecureRandom.hex - - visit openid_connect_authorize_path( - client_id: client_id, - response_type: 'code', - acr_values: Saml::Idp::Constants::LOA1_AUTHN_CONTEXT_CLASSREF, - scope: 'openid email', - redirect_uri: 'http://localhost:7654/auth/result', - state: state, - prompt: 'select_account', - nonce: nonce - ) - end - - def visit_idp_from_sp_with_loa1_with_second_valid_redirect_uri(state: SecureRandom.hex) - client_id = 'urn:gov:gsa:openidconnect:sp:server' - nonce = SecureRandom.hex - - visit openid_connect_authorize_path( - client_id: client_id, - response_type: 'code', - acr_values: Saml::Idp::Constants::LOA1_AUTHN_CONTEXT_CLASSREF, - scope: 'openid email', - redirect_uri: 'https://example.com', - state: state, - prompt: 'select_account', - nonce: nonce - ) - end - - def visit_idp_from_sp_that_does_not_have_redirect_uris(state: SecureRandom.hex) - client_id = 'http://test.host' - nonce = SecureRandom.hex - - visit openid_connect_authorize_path( - client_id: client_id, - response_type: 'code', - acr_values: Saml::Idp::Constants::LOA1_AUTHN_CONTEXT_CLASSREF, - scope: 'openid email', - redirect_uri: 'http://test.host', - state: state, - prompt: 'select_account', - nonce: nonce - ) - end -end diff --git a/spec/features/saml/redirect_uri_validation_spec.rb b/spec/features/saml/redirect_uri_validation_spec.rb deleted file mode 100644 index 8b1c5199428..00000000000 --- a/spec/features/saml/redirect_uri_validation_spec.rb +++ /dev/null @@ -1,27 +0,0 @@ -require 'rails_helper' - -describe 'redirect_uri validation' do - include SamlAuthHelper - - context 'when redirect_uri param is included in SAML request' do - it 'uses the return_to_sp_url URL and not the redirect_uri' do - allow(FeatureManagement).to receive(:prefill_otp_codes?).and_return(true) - user = create(:user, :signed_up) - visit api_saml_auth_path( - SAMLRequest: CGI.unescape(saml_request(saml_settings)), redirect_uri: 'http://evil.com' - ) - sp = ServiceProvider.find_by(issuer: 'http://localhost:3000') - - expect(page). - to have_link t('links.back_to_sp', sp: sp.friendly_name), href: sp.return_to_sp_url - - click_link t('links.sign_in') - fill_in_credentials_and_submit(user.email, user.password) - click_submit_default - click_continue - click_submit_default - - expect(current_url).to eq sp.acs_url - end - end -end diff --git a/spec/forms/openid_connect_authorize_form_spec.rb b/spec/forms/openid_connect_authorize_form_spec.rb index 4ff1fa7fd0e..6522593872d 100644 --- a/spec/forms/openid_connect_authorize_form_spec.rb +++ b/spec/forms/openid_connect_authorize_form_spec.rb @@ -180,9 +180,9 @@ end end - context 'with a redirect_uri that only partially matches any registered redirect_uri' do + context 'with a redirect_uri that adds on to the registered redirect_uri' do let(:redirect_uri) { 'gov.gsa.openidconnect.test://result/more/extra' } - it { expect(valid?).to eq(false) } + it { expect(valid?).to eq(true) } end end diff --git a/spec/forms/openid_connect_logout_form_spec.rb b/spec/forms/openid_connect_logout_form_spec.rb index 09f9914e908..1a385df42ed 100644 --- a/spec/forms/openid_connect_logout_form_spec.rb +++ b/spec/forms/openid_connect_logout_form_spec.rb @@ -137,7 +137,7 @@ it 'is not valid' do expect(valid?).to eq(false) - expect(form.errors[:redirect_uri]).to be_present + expect(form.errors[:post_logout_redirect_uri]).to be_present end end @@ -146,7 +146,7 @@ it 'is not valid' do expect(valid?).to eq(false) - expect(form.errors[:redirect_uri]). + expect(form.errors[:post_logout_redirect_uri]). to include(t('openid_connect.authorization.errors.redirect_uri_no_match')) end end diff --git a/spec/services/openid_connect_redirector_spec.rb b/spec/services/openid_connect_redirector_spec.rb new file mode 100644 index 00000000000..0c1c621e956 --- /dev/null +++ b/spec/services/openid_connect_redirector_spec.rb @@ -0,0 +1,150 @@ +require 'rails_helper' + +RSpec.describe OpenidConnectRedirector do + include Rails.application.routes.url_helpers + + let(:redirect_uri) { 'http://localhost:7654/' } + let(:state) { SecureRandom.hex } + let(:service_provider) { ServiceProvider.from_issuer('urn:gov:gsa:openidconnect:sp:server') } + let(:errors) { ActiveModel::Errors.new(nil) } + + subject(:redirector) do + OpenidConnectRedirector.new( + redirect_uri: redirect_uri, + service_provider: service_provider, + state: state, + errors: errors + ) + end + + describe '.from_request_url' do + it 'builds a redirector from an OpenID request_url' do + request_url = openid_connect_authorize_url( + client_id: service_provider.issuer, + redirect_uri: redirect_uri, + state: state + ) + + result = OpenidConnectRedirector.from_request_url(request_url) + + expect(result).to be_a(OpenidConnectRedirector) + expect(result.send(:redirect_uri)).to eq(redirect_uri) + expect(result.send(:service_provider)).to eq(service_provider) + expect(result.send(:state)).to eq(state) + end + end + + describe '#validate' do + context 'with a redirect_uri that spoofs a hostname' do + let(:redirect_uri) { 'https://example.com.evilish.com/' } + + it 'is invalid' do + redirector.validate + expect(errors[:redirect_uri]). + to include(t('openid_connect.authorization.errors.redirect_uri_no_match')) + end + end + + context 'with a valid redirect_uri' do + let(:redirect_uri) { 'http://localhost:7654/result/more/extra' } + it 'is valid' do + redirector.validate + expect(errors).to be_empty + end + end + + context 'with a malformed redirect_uri' do + let(:redirect_uri) { ':aaaa' } + it 'has errors' do + redirector.validate + expect(errors[:redirect_uri]). + to include(t('openid_connect.authorization.errors.redirect_uri_invalid')) + end + end + + context 'with a redirect_uri not registered to the service provider' do + let(:redirect_uri) { 'http://localhost:3000/test' } + it 'has errors' do + redirector.validate + expect(errors[:redirect_uri]). + to include(t('openid_connect.authorization.errors.redirect_uri_no_match')) + end + end + end + + describe '#success_redirect_uri' do + it 'adds the code and state to the URL' do + code = SecureRandom.hex + expect(redirector.success_redirect_uri(code: code)). + to eq(URIService.add_params(redirect_uri, code: code, state: state)) + end + end + + describe '#decline_redirect_uri' do + it 'adds the state and access_denied to the URL' do + expect(redirector.decline_redirect_uri). + to eq(URIService.add_params(redirect_uri, state: state, error: 'access_denied')) + end + end + + describe '#error_redirect_uri' do + before { expect(errors).to receive(:full_messages).and_return(['some attribute is missing']) } + + it 'adds the errors to the URL' do + expect(redirector.error_redirect_uri). + to eq(URIService.add_params(redirect_uri, + state: state, + error: 'invalid_request', + error_description: 'some attribute is missing')) + end + end + + describe '#logout_redirect_uri' do + it 'adds the state to the URL' do + expect(redirector.logout_redirect_uri). + to eq(URIService.add_params(redirect_uri, state: state)) + end + end + + describe '#validated_input_redirect_uri' do + let(:service_provider) { ServiceProvider.new(redirect_uris: redirect_uris, active: true) } + + subject(:validated_input_redirect_uri) { redirector.validated_input_redirect_uri } + + context 'when the service provider has no redirect URIs' do + let(:redirect_uris) { [] } + + it 'is nil' do + expect(validated_input_redirect_uri).to be_nil + end + end + + context 'when the service provider has 2 redirect URIs' do + let(:redirect_uris) { %w[http://localhost:1234/result my-app://result] } + + context 'when a URL matching the first redirect_uri is passed in' do + let(:redirect_uri) { 'http://localhost:1234/result/more' } + + it 'is that URL' do + expect(validated_input_redirect_uri).to eq(redirect_uri) + end + end + + context 'when a URL matching the second redirect_uri is passed in' do + let(:redirect_uri) { 'my-app://result/more' } + + it 'is that URL' do + expect(validated_input_redirect_uri).to eq(redirect_uri) + end + end + + context 'when a URL matching the neither redirect_uri is passed in' do + let(:redirect_uri) { 'https://example.com' } + + it 'is nil' do + expect(validated_input_redirect_uri).to be_nil + end + end + end + end +end