From ec448a1516721dd8b2086d966954bf626d99711a Mon Sep 17 00:00:00 2001 From: David Taylor Date: Wed, 17 Jun 2020 11:15:53 +0100 Subject: [PATCH] DEV: Refactor Auth::Result for readability, recreate during signup flow --- app/services/user_authenticator.rb | 16 +-- lib/auth/result.rb | 152 ++++++++++++++++++----------- 2 files changed, 102 insertions(+), 66 deletions(-) diff --git a/app/services/user_authenticator.rb b/app/services/user_authenticator.rb index d988300ceb8..1cc37613e67 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 0e715bd6758..3596017e298 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