Refactor fetching of SP attributes

.reek

@@ -83,6 +83,7 @@ UtilityFunction:
     - SessionDecorator#registration_heading
     - SessionDecorator#registration_bullet_1
     - SessionDecorator#new_session_heading
+    - SessionDecorator#timeout_flash_text
     - WorkerHealthChecker::Middleware#call
     - UserEncryptedAttributeOverrides#create_fingerprint
 'app/controllers':

app/controllers/application_controller.rb

@@ -1,16 +1,15 @@
-class ApplicationController < ActionController::Base # rubocop:disable Metrics/ClassLength
-  include BrandedExperience
+class ApplicationController < ActionController::Base
   include UserSessionContext
 
   # Prevent CSRF attacks by raising an exception.
   # For APIs, you may want to use :null_session instead.
   protect_from_forgery with: :exception
 
-  rescue_from ActionController::InvalidAuthenticityToken,
-              with: :invalid_auth_token
+  rescue_from ActionController::InvalidAuthenticityToken, with: :invalid_auth_token
 
-  helper_method :decorated_user, :reauthn?, :user_fully_authenticated?
+  helper_method :decorated_session, :decorated_user, :reauthn?, :user_fully_authenticated?
 
+  before_action :create_branded_experience
   prepend_before_action :session_expires_at
   before_action :set_locale
 
@@ -44,36 +43,29 @@ def create_user_event(event_type, user = current_user)
     Event.create(user_id: user.id, event_type: event_type)
   end
 
+  def decorated_session
+    @_decorated_session ||= DecoratedSession.new(sp: current_sp, view_context: view_context).call
+  end
+
   private
 
   def redirect_on_timeout
-    params = request.query_parameters
     return unless params[:timeout]
 
-    params[:issuer].present? ? redirect_with_sp : redirect_without_sp
-  end
-
-  def sp_metadata
-    ServiceProvider.from_issuer(request.query_parameters[:issuer]).metadata
+    flash[:timeout] = decorated_session.timeout_flash_text
+    redirect_to url_for(params.except(:timeout))
   end
 
-  def sp_name
-    sp_metadata[:friendly_name] || sp_metadata[:agency]
+  def current_sp
+    @current_sp ||= sp_from_sp_session || sp_from_params
   end
 
-  def redirect_with_sp # rubocop:disable Metrics/AbcSize
-    flash[:timeout] = t(
-      'notices.session_cleared_with_sp',
-      link: view_context.link_to(sp_name, sp_metadata[:return_to_sp_url]),
-      minutes: Figaro.env.session_timeout_in_minutes,
-      sp: sp_name
-    )
-    redirect_to url_for(request.query_parameters.except(:timeout))
+  def sp_from_sp_session
+    ServiceProvider.from_issuer(sp_session[:issuer])
   end
 
-  def redirect_without_sp
-    flash[:timeout] = t('notices.session_cleared', minutes: Figaro.env.session_timeout_in_minutes)
-    redirect_to url_for(request.query_parameters.except(:issuer, :timeout))
+  def sp_from_params
+    ServiceProvider.from_issuer(params[:issuer])
   end
 
   def decorated_user
@@ -137,4 +129,12 @@ def set_locale
   def sp_session
     session.fetch(:sp, {})
   end
+
+  def create_branded_experience
+    return unless session[:sp]
+
+    @sp_logo = current_sp.logo
+    @sp_name = decorated_session.sp_name
+    @sp_return_url = current_sp.return_to_sp_url
+  end
 end

app/controllers/concerns/saml_idp_auth_concern.rb

@@ -22,13 +22,9 @@ def validate_service_provider_and_authn_context
     render nothing: true, status: :unauthorized
   end
 
-  def add_sp_metadata_to_session # rubocop:disable Metrics/AbcSize
+  def add_sp_metadata_to_session
     session[:sp] = { loa3: loa3_requested?,
-                     logo: current_sp_metadata[:logo],
-                     issuer: saml_request.service_provider.identifier,
-                     return_url: current_sp_metadata[:return_to_sp_url],
-                     name: current_sp_metadata[:friendly_name] ||
-                           current_sp_metadata[:agency],
+                     issuer: current_issuer,
                      request_url: request.original_url,
                      show_start_page: true }
   end
@@ -38,9 +34,7 @@ def requested_authn_context
   end
 
   def link_identity_from_session_data
-    provider = saml_request.service_provider.identifier
-
-    IdentityLinker.new(current_user, provider).link_identity
+    IdentityLinker.new(current_user, current_issuer).link_identity
   end
 
   def identity_needs_verification?
@@ -89,10 +83,10 @@ def saml_response
   end
 
   def current_service_provider
-    @_sp ||= ServiceProvider.from_issuer(saml_request.service_provider.identifier)
+    @_sp ||= ServiceProvider.from_issuer(current_issuer)
   end
 
-  def current_sp_metadata
-    current_service_provider.metadata
+  def current_issuer
+    @_issuer ||= saml_request.service_provider.identifier
   end
 end

app/controllers/openid_connect/authorization_controller.rb

@@ -2,7 +2,6 @@ module OpenidConnect
   class AuthorizationController < ApplicationController
     before_action :build_authorize_form_from_params, only: [:index]
     before_action :add_sp_metadata_to_session, only: [:index]
-
     before_action :confirm_two_factor_authenticated
     before_action :load_authorize_form_from_session, only: [:create, :destroy]
     before_action :apply_secure_headers_override, only: [:index]
@@ -65,8 +64,7 @@ def build_authorize_form_from_params
       @authorize_form = OpenidConnectAuthorizeForm.new(authorization_params)
 
       @authorize_decorator = OpenidConnectAuthorizeDecorator.new(
-        scopes: @authorize_form.scope,
-        service_provider: @authorize_form.service_provider
+        scopes: @authorize_form.scope
       )
     end
 
@@ -83,12 +81,8 @@ def session_params
     end
 
     def add_sp_metadata_to_session
-      current_sp_metadata = @authorize_form.service_provider.metadata
       session[:sp] = { loa3: @authorize_form.loa3_requested?,
-                       logo: current_sp_metadata[:logo],
                        issuer: @authorize_form.client_id,
-                       name: current_sp_metadata[:friendly_name] ||
-                             current_sp_metadata[:agency],
                        show_start_page: true }
     end
   end

app/controllers/sign_up/completions_controller.rb

@@ -32,7 +32,7 @@ def loa3?
     end
 
     def service_provider_attributes
-      { loa3: sp_session[:loa3], service_provider_name: sp_session[:friendly_name] }
+      { loa3: sp_session[:loa3], service_provider_name: decorated_session.sp_name }
     end
   end
 end

app/decorators/openid_connect_authorize_decorator.rb

@@ -1,22 +1,15 @@
 class OpenidConnectAuthorizeDecorator
-  attr_reader :scopes, :service_provider
+  attr_reader :requested_attributes
 
-  delegate :metadata, to: :service_provider
-
-  def initialize(scopes:, service_provider:)
+  def initialize(scopes:)
     @scopes = scopes
-    @service_provider = service_provider
-  end
-
-  def name
-    metadata[:friendly_name] || metadata[:agency]
   end
 
   def requested_attributes
     OpenidConnectAttributeScoper.new(scopes).requested_attributes
   end
 
-  def logo
-    metadata[:logo]
-  end
+  private
+
+  attr_reader :scopes
 end

app/decorators/service_provider_session_decorator.rb

@@ -1,7 +1,9 @@
 class ServiceProviderSessionDecorator
-  def initialize(sp_name:, sp_logo:)
-    @sp_name = sp_name
-    @sp_logo = sp_logo
+  attr_reader :sp_name
+
+  def initialize(sp:, view_context:)
+    @sp = sp
+    @view_context = view_context
   end
 
   def return_to_service_provider_partial
@@ -16,6 +18,10 @@ def new_session_heading
     I18n.t('headings.sign_in_with_sp', sp: sp_name)
   end
 
+  def sp_name
+    sp.friendly_name || sp.agency
+  end
+
   def registration_heading
     sp = ActionController::Base.helpers.content_tag(:strong, sp_name)
     ActionController::Base.helpers.safe_join(
@@ -28,12 +34,21 @@ def idv_hardfail4_partial
   end
 
   def logo_partial
-    return 'shared/nav_branded_logo' if sp_logo
+    return 'shared/nav_branded_logo' if sp.logo
 
     'shared/null'
   end
 
+  def timeout_flash_text
+    I18n.t(
+      'notices.session_cleared_with_sp',
+      link: view_context.link_to(sp_name, sp.return_to_sp_url),
+      minutes: Figaro.env.session_timeout_in_minutes,
+      sp: sp_name
+    )
+  end
+
   private
 
-  attr_reader :sp_name, :sp_logo
+  attr_reader :sp, :view_context
 end

app/decorators/session_decorator.rb

@@ -20,4 +20,12 @@ def idv_hardfail4_partial
   end
 
   def logo_partial; end
+
+  def timeout_flash_text
+    I18n.t('notices.session_cleared', minutes: Figaro.env.session_timeout_in_minutes)
+  end
+
+  def sp_name
+    nil
+  end
 end

app/forms/openid_connect_authorize_form.rb

@@ -145,7 +145,7 @@ def error_redirect_uri
   end
 
   def sp_redirect_uri
-    service_provider.metadata[:redirect_uri].to_s
+    service_provider.redirect_uri
   end
 end
 # rubocop:enable Metrics/ClassLength

app/helpers/application_helper.rb

@@ -25,20 +25,8 @@ def tooltip(text)
     )
   end
 
-  def decorated_session
-    @_decorated_session ||= begin
-      if @sp_name.present?
-        ServiceProviderSessionDecorator.new(
-          sp_name: @sp_name, sp_logo: @sp_logo
-        )
-      else
-        SessionDecorator.new
-      end
-    end
-  end
-
   def sp_session
-    session[:sp]
+    session.fetch(:sp, {})
   end
 
   def sign_up_init?

app/helpers/session_timeout_warning_helper.rb

@@ -15,14 +15,10 @@ def timeout_refresh_url
     URIService.add_params(
       request.original_url,
       timeout: true,
-      issuer: request.query_parameters[:issuer] || sp_issuer
+      issuer: request.query_parameters[:issuer] || sp_session[:issuer]
     ).html_safe # rubocop:disable Rails/OutputSafety
   end
 
-  def sp_issuer
-    sp_session[:issuer] if sp_session.present?
-  end
-
   def auto_session_timeout_js
     nonced_javascript_tag do
       render partial: 'session_timeout/ping',

app/models/null_service_provider.rb

@@ -21,11 +21,15 @@ def metadata
     {}
   end
 
-  def fingerprint
-    nil
-  end
+  def fingerprint; end
 
-  def ssl_cert
-    nil
-  end
+  def ssl_cert; end
+
+  def logo; end
+
+  def friendly_name; end
+
+  def return_to_sp_url; end
+
+  def redirect_uri; end
 end

app/services/decorated_session.rb

@@ -0,0 +1,18 @@
+class DecoratedSession
+  def initialize(sp:, view_context:)
+    @sp = sp
+    @view_context = view_context
+  end
+
+  def call
+    if sp.is_a? ServiceProvider
+      ServiceProviderSessionDecorator.new(sp: sp, view_context: view_context)
+    else
+      SessionDecorator.new
+    end
+  end
+
+  private
+
+  attr_reader :sp, :view_context
+end

app/views/openid_connect/authorization/index.html.slim

@@ -1,10 +1,10 @@
-- if @authorize_decorator.logo
-  = image_tag(asset_url("sp-logos/#{@authorize_decorator.logo}"), height: 50,
-    alt: @authorize_decorator.name, class: 'align-middle')
+- if @sp_logo
+  = image_tag(asset_url("sp-logos/#{@sp_logo}"), height: 50,
+    alt: @sp_name, class: 'align-middle')
 
-h3 = @authorize_decorator.name
+h3 = @sp_name
 
-p = t('openid_connect.authorization.index.allow_prompt', name: @authorize_decorator.name)
+p = t('openid_connect.authorization.index.allow_prompt', name: @sp_name)
 
 ul
   - @authorize_decorator.requested_attributes.each do |attribute|

spec/controllers/openid_connect/authorization_controller_spec.rb

@@ -105,9 +105,7 @@
 
         expect(session[:sp]).to eq(
           loa3: false,
-          logo: 'generic.svg',
           issuer: 'urn:gov:gsa:openidconnect:test',
-          name: 'Example iOS App',
           show_start_page: true
         )
       end