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

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

diff --git a/app/services/user_authenticator.rb b/app/services/user_authenticator.rb
index d988300..1cc3761 100644
--- a/app/services/user_authenticator.rb
+++ b/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
diff --git a/lib/auth/result.rb b/lib/auth/result.rb
index 0e715bd..3596017 100644
--- a/lib/auth/result.rb
+++ b/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,60 +57,64 @@ 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?
-        {
-          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 =
-          if omniauth_disallow_totp
-            {
-              omniauth_disallow_totp: !!omniauth_disallow_totp,
-              email: email
-            }
-          else
-            {
-              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
-      end
-    else
-      result = { email: email,
-                 username: UserNameSuggester.suggest(username || name || email),
-                 auth_provider: authenticator_name,
-                 email_valid: !!email_valid,
-                 omit_username: !!omit_username }
-
-      result[:destination_url] = destination_url if destination_url.present?
-
-      if SiteSetting.enable_names?
-        result[:name] = name.presence || User.suggest_name(username || email)
-      end
-
-      result
+      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)
+      }
+    end
+
+    if omniauth_disallow_totp
+      return {
+        omniauth_disallow_totp: !!omniauth_disallow_totp,
+        email: email
+      }
     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
+      }
+
+      result[:destination_url] = destination_url if authenticated && destination_url.present?
+
+      return result
+    end
+
+    result = {
+      email: email,
+      username: UserNameSuggester.suggest(username || name || email),
+      auth_provider: authenticator_name,
+      email_valid: !!email_valid,
+      omit_username: !!omit_username
+    }
+
+    result[:destination_url] = destination_url if destination_url.present?
+
+    if SiteSetting.enable_names?
+      result[:name] = name.presence || User.suggest_name(username || email)
+    end
+
+    result
   end
 end

GitHub sha: ec448a15

1 Like