DEV: Refactor Auth::Result for readability, recreate during signup flow

app/services/user_authenticator.rb

@@ -5,7 +5,9 @@ class UserAuthenticator
   def initialize(user, session, authenticator_finder = Users::OmniauthCallbacksController)
     @user = user
     @session = session
-    @auth_session = session[:authentication]
+    if session[:authentication] && session[:authentication].is_a?(Hash)
+      @auth_result = Auth::Result.from_session_data(session[:authentication])
+    end
     @authenticator_finder = authenticator_finder
   end
 
@@ -16,7 +18,7 @@ class UserAuthenticator
       @user.password_required!
     end
 
-    @user.skip_email_validation = true if @auth_session && @auth_session[:skip_email_validation].present?
+    @user.skip_email_validation = true if @auth_result && @auth_result.skip_email_validation
   end
 
   def has_authenticator?
@@ -25,18 +27,18 @@ class UserAuthenticator
 
   def finish
     if authenticator
-      authenticator.after_create_account(@user, @auth_session)
+      authenticator.after_create_account(@user, @auth_result)
       confirm_email
     end
-    @session[:authentication] = @auth_session = nil if @auth_session
+    @session[:authentication] = @auth_result = nil if @session[:authentication]
   end
 
   def email_valid?
-    @auth_session && @auth_session[:email_valid]
+    @auth_result&.email_valid
   end
 
   def authenticated?
-    @auth_session && @auth_session[:email]&.downcase == @user.email.downcase && @auth_session[:email_valid].to_s == "true"
+    @auth_result && @auth_result.email.downcase == @user.email.downcase && @auth_result.email_valid.to_s == "true"
   end
 
   private
@@ -55,7 +57,7 @@ class UserAuthenticator
   end
 
   def authenticator_name
-    @auth_session && @auth_session[:authenticator_name]
+    @auth_result&.authenticator_name
   end
 
 end

lib/auth/result.rb

@@ -1,18 +1,48 @@
 # frozen_string_literal: true
 
 class Auth::Result
-  attr_accessor :user, :name, :username, :email,
-                :email_valid, :extra_data, :awaiting_activation,
-                :awaiting_approval, :authenticated, :authenticator_name,
-                :requires_invite, :not_allowed_from_ip_address,
-                :admin_not_allowed_from_ip_address, :omit_username,
-                :skip_email_validation, :destination_url, :omniauth_disallow_totp
-
-  attr_accessor(
+  ATTRIBUTES = [
+    :user,
+    :name,
+    :username,
+    :email,
+    :email_valid,
+    :extra_data,
+    :awaiting_activation,
+    :awaiting_approval,
+    :authenticated,
+    :authenticator_name,
+    :requires_invite,
+    :not_allowed_from_ip_address,
+    :admin_not_allowed_from_ip_address,
+    :omit_username,
+    :skip_email_validation,
+    :destination_url,
+    :omniauth_disallow_totp,
     :failed,
     :failed_reason,
     :failed_code
-  )
+  ]
+
+  attr_accessor *ATTRIBUTES
+
+  # These are stored in the session during
+  # account creation. The user cannot read or modify them
+  SESSION_ATTRIBUTES = [
+    :email,
+    :username,
+    :email_valid,
+    :omit_username,
+    :name,
+    :authenticator_name,
+    :extra_data,
+    :skip_email_validation
+  ]
+
+  def [](key)
+    key = key.to_sym
+    public_send(key) if ATTRIBUTES.include?(key)
+  end
 
   def initialize
     @failed = false
@@ -27,52 +57,57 @@ class Auth::Result
   end
 
   def session_data
-    { email: email,
-      username: username,
-      email_valid: email_valid,
-      omit_username: omit_username,
-      name: name,
-      authenticator_name: authenticator_name,
-      extra_data: extra_data,
-      skip_email_validation: !!skip_email_validation }
+    SESSION_ATTRIBUTES.map { |att| [att, public_send(att)] }.to_h
+  end
+
+  def self.from_session_data(data)
+    result = new
+    data = data.symbolize_keys
+    SESSION_ATTRIBUTES.each { |att| result.public_send("#{att}=", data[att]) }
+    result
   end
 
   def to_client_hash
     if requires_invite
-      { requires_invite: true }
-    elsif user
-      if user.suspended?
-        {
+      return { requires_invite: true }
+    end
+
+    if user&.suspended?
+      return {
         suspended: true,
         suspended_message: I18n.t(user.suspend_reason ? "login.suspended_with_reason" : "login.suspended",
                                    date: I18n.l(user.suspended_till, format: :date_only), reason: user.suspend_reason)
       }
-      else
-        result =
+    end
+
     if omniauth_disallow_totp
-            {
+      return {
         omniauth_disallow_totp: !!omniauth_disallow_totp,
         email: email
       }
-          else
-            {
+    end
+
+    if user
+      result = {
         authenticated: !!authenticated,
         awaiting_activation: !!awaiting_activation,
         awaiting_approval: !!awaiting_approval,
         not_allowed_from_ip_address: !!not_allowed_from_ip_address,
         admin_not_allowed_from_ip_address: !!admin_not_allowed_from_ip_address
       }
-          end
 
       result[:destination_url] = destination_url if authenticated && destination_url.present?
-        result
+
+      return result
     end
-    else
-      result = { email: email,
+
+    result = {
+      email: email,
       username: UserNameSuggester.suggest(username || name || email),
       auth_provider: authenticator_name,
       email_valid: !!email_valid,
-                 omit_username: !!omit_username }
+      omit_username: !!omit_username
+    }
 
     result[:destination_url] = destination_url if destination_url.present?
 
@@ -83,4 +118,3 @@ class Auth::Result
     result
   end
 end
-end