diff --git a/.reek b/.reek index b0482a81af5..4cf387df550 100644 --- a/.reek +++ b/.reek @@ -3,7 +3,6 @@ Attribute: ControlParameter: exclude: - CustomDeviseFailureApp#i18n_message - - OpenidConnectRedirector#initialize - NoRetryJobs#call - PhoneFormatter#self.format - Users::TwoFactorAuthenticationController#invalid_phone_number @@ -88,7 +87,6 @@ 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 74a11e2078b..23da64ea5c3 100644 --- a/app/controllers/concerns/secure_headers_concern.rb +++ b/app/controllers/concerns/secure_headers_concern.rb @@ -5,17 +5,20 @@ 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'", authorize_form.sp_redirect_uri].compact, + form_action: ["'self'", 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 3d499416659..ad130f16596 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'", @authorize_form.sp_redirect_uri].compact, + form_action: ["'self'", authorization_params[: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 8e91dce7f04..1e31ad4321a 100644 --- a/app/decorators/service_provider_session_decorator.rb +++ b/app/decorators/service_provider_session_decorator.rb @@ -92,8 +92,12 @@ def sp_agency end def sp_return_url - if sp.redirect_uris.present? && openid_connect_redirector.valid? - openid_connect_redirector.decline_redirect_uri + if sp.redirect_uris.present? && valid_oidc_request? + URIService.add_params( + oidc_redirect_uri, + error: 'access_denied', + state: request_params[:state] + ) else sp.return_to_sp_url end @@ -127,7 +131,19 @@ def request_url sp_session[:request_url] || service_provider_request.url end - def openid_connect_redirector - @_openid_connect_redirector ||= OpenidConnectRedirector.from_request_url(request_url) + 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) end end diff --git a/app/forms/openid_connect_authorize_form.rb b/app/forms/openid_connect_authorize_form.rb index 4ee64191fd9..83596f7876b 100644 --- a/app/forms/openid_connect_authorize_form.rb +++ b/app/forms/openid_connect_authorize_form.rb @@ -2,6 +2,7 @@ class OpenidConnectAuthorizeForm include ActiveModel::Model include ActionView::Helpers::TranslationHelper + include RedirectUriValidator SIMPLE_ATTRS = %i[ client_id @@ -33,7 +34,6 @@ class OpenidConnectAuthorizeForm validate :validate_acr_values validate :validate_client_id - validate :validate_redirect_uri validate :validate_scope def initialize(params) @@ -43,10 +43,6 @@ 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 @@ -59,10 +55,6 @@ 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 @@ -79,13 +71,15 @@ 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 - openid_connect_redirector.success_redirect_uri(code: code) if code + + URIService.add_params(uri, code: code, state: state) if code end private - attr_reader :identity, :success, :openid_connect_redirector, :already_linked + attr_reader :identity, :success, :already_linked def requested_attributes @requested_attributes ||= @@ -107,10 +101,6 @@ 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')) @@ -141,7 +131,14 @@ def result_uri end def error_redirect_uri - openid_connect_redirector.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 + ) 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 1b21e5bd241..98bc7d2af4a 100644 --- a/app/forms/openid_connect_logout_form.rb +++ b/app/forms/openid_connect_logout_form.rb @@ -1,6 +1,7 @@ class OpenidConnectLogoutForm include ActiveModel::Model include ActionView::Helpers::TranslationHelper + include RedirectUriValidator ATTRS = %i[ id_token_hint @@ -16,7 +17,6 @@ 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,7 +25,6 @@ def initialize(params) end @identity = load_identity - @openid_connect_redirector = build_openid_connect_redirector end def submit @@ -39,7 +38,6 @@ def submit private attr_reader :identity, - :openid_connect_redirector, :success def load_identity @@ -58,20 +56,6 @@ 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 @@ -90,10 +74,23 @@ def extra_analytics_attributes end def redirect_uri - if success - openid_connect_redirector.logout_redirect_uri - else - openid_connect_redirector.error_redirect_uri - end + 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 + ) end end diff --git a/app/services/openid_connect_redirector.rb b/app/services/openid_connect_redirector.rb deleted file mode 100644 index 4e5d1b0ebbf..00000000000 --- a/app/services/openid_connect_redirector.rb +++ /dev/null @@ -1,97 +0,0 @@ -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 new file mode 100644 index 00000000000..a28c91816c8 --- /dev/null +++ b/app/validators/redirect_uri_validator.rb @@ -0,0 +1,32 @@ +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 be7ce1ed030..6fd3d7d1e0e 100644 --- a/config/service_providers.yml +++ b/config/service_providers.yml @@ -47,6 +47,7 @@ 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' @@ -57,7 +58,7 @@ test: 'urn:gov:gsa:openidconnect:sp:server': agency_id: 2 redirect_uris: - - 'http://localhost:7654/' + - 'http://localhost:7654/auth/result' - 'https://example.com' cert: 'saml_test_sp' friendly_name: 'Test SP' @@ -632,7 +633,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 38500e8c69f..67bda6e2382 100644 --- a/spec/controllers/openid_connect/logout_controller_spec.rb +++ b/spec/controllers/openid_connect/logout_controller_spec.rb @@ -80,11 +80,15 @@ 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: hash_including(:post_logout_redirect_uri), + errors: errors, 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 eeea3adcfa3..314a4ca0a65 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/auth', + redirect_uri: 'gov.gsa.openidconnect.test://result', 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 new file mode 100644 index 00000000000..2a3f4949044 --- /dev/null +++ b/spec/features/openid_connect/redirect_uri_validation_spec.rb @@ -0,0 +1,259 @@ +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 new file mode 100644 index 00000000000..8b1c5199428 --- /dev/null +++ b/spec/features/saml/redirect_uri_validation_spec.rb @@ -0,0 +1,27 @@ +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 6522593872d..4ff1fa7fd0e 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 adds on to the registered redirect_uri' do + context 'with a redirect_uri that only partially matches any registered redirect_uri' do let(:redirect_uri) { 'gov.gsa.openidconnect.test://result/more/extra' } - it { expect(valid?).to eq(true) } + it { expect(valid?).to eq(false) } end end diff --git a/spec/forms/openid_connect_logout_form_spec.rb b/spec/forms/openid_connect_logout_form_spec.rb index 1a385df42ed..09f9914e908 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[:post_logout_redirect_uri]).to be_present + expect(form.errors[:redirect_uri]).to be_present end end @@ -146,7 +146,7 @@ it 'is not valid' do expect(valid?).to eq(false) - expect(form.errors[:post_logout_redirect_uri]). + expect(form.errors[: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 deleted file mode 100644 index 0c1c621e956..00000000000 --- a/spec/services/openid_connect_redirector_spec.rb +++ /dev/null @@ -1,150 +0,0 @@ -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