Revert "Merge pull request #2351 from 18F/mb-refactor-redirect-uri-va…

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

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

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

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

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

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

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