Refactor SP redirect URI validation

.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:

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

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

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

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

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

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