Merge pull request #1658 from salbertson/sa-refactor-users-controller-create

Refactor UsersController#create

app/controllers/users_controller.rb

@@ -1,6 +1,5 @@
 require_dependency 'discourse_hub'
 require_dependency 'user_name_suggester'
-require_dependency 'user_activator'
 require_dependency 'avatar_upload_service'
 
 class UsersController < ApplicationController
@@ -9,6 +8,7 @@ class UsersController < ApplicationController
   skip_before_filter :check_xhr, only: [:show, :password_reset, :update, :activate_account, :authorize_email, :user_preferences_redirect, :avatar]
 
   before_filter :ensure_logged_in, only: [:username, :update, :change_email, :user_preferences_redirect, :upload_avatar, :toggle_avatar]
+  before_filter :respond_to_suspicious_request, only: [:create]
 
   # we need to allow account creation with bad CSRF tokens, if people are caching, the CSRF token on the
   #  page is going to be empty, this means that server will see an invalid CSRF and blow the session
@@ -120,17 +120,39 @@ class UsersController < ApplicationController
   end
 
   def create
-    return fake_success_response if suspicious? params
+    user = User.new(user_params)
 
-    user = User.new_from_params(params)
+    authentication = UserAuthenticator.new(user, session)
-    user.ip_address = request.ip
+    authentication.start
-    auth = authenticate_user(user, params)
-    register_nickname(user)
 
-    user.save ? user_create_successful(user, auth) : user_create_failed(user)
+    activation = UserActivator.new(user, request, session, cookies)
+    activation.start
 
+    if user.save
+      authentication.finish
+      activation.finish
+
+      render json: {
+        success: true,
+        active: user.active?,
+        message: activation.message
+      }
+    else
+      render json: {
+        success: false,
+        message: I18n.t(
+          'login.errors',
+          errors: user.errors.full_messages.join("\n")
+        ),
+        errors: user.errors.to_hash,
+        values: user.attributes.slice('name', 'username', 'email')
+      }
+    end
   rescue ActiveRecord::StatementInvalid
-    render json: { success: false, message: I18n.t("login.something_already_taken") }
+    render json: {
+      success: false,
+      message: I18n.t("login.something_already_taken")
+    }
   rescue DiscourseHub::NicknameUnavailable => e
     render json: e.response_message
   rescue RestClient::Forbidden
@@ -325,67 +347,6 @@ class UsersController < ApplicationController
       challenge
     end
 
-    def suspicious?(params)
-      honeypot_or_challenge_fails?(params) || SiteSetting.invite_only?
-    end
-
-    def fake_success_response
-      render(
-        json: {
-          success: true,
-          active: false,
-          message: I18n.t("login.activate_email", email: params[:email])
-        }
-      )
-    end
-
-    def honeypot_or_challenge_fails?(params)
-      params[:password_confirmation] != honeypot_value ||
-      params[:challenge] != challenge_value.try(:reverse)
-    end
-
-    def valid_session_authentication?(auth, email)
-      auth && auth[:email] == email && auth[:email_valid]
-    end
-
-    def create_third_party_auth_records(user, auth)
-      return unless auth && auth[:authenticator_name]
-
-      authenticator = Users::OmniauthCallbacksController.find_authenticator(auth[:authenticator_name])
-      authenticator.after_create_account(user, auth)
-    end
-
-    def register_nickname(user)
-      if user.valid? && SiteSetting.call_discourse_hub?
-        DiscourseHub.register_nickname(user.username, user.email)
-      end
-    end
-
-    def user_create_successful(user, auth)
-      activator = UserActivator.new(user, request, session, cookies)
-      create_third_party_auth_records(user, auth)
-
-      # Clear authentication session.
-      session[:authentication] = nil
-      render json: { success: true, active: user.active?, message: activator.activation_message }
-    end
-
-    def user_create_failed(user)
-      render json: {
-        success: false,
-        message: I18n.t("login.errors", errors: user.errors.full_messages.join("\n")),
-        errors: user.errors.to_hash,
-        values: user.attributes.slice("name", "username", "email")
-      }
-    end
-
-    def authenticate_user(user, params)
-      auth = session[:authentication]
-      user.active = true if valid_session_authentication?(auth, params[:email])
-      user.password_required! unless auth
-      auth
-    end
-
     def build_avatar_from(file)
       source = if file.is_a?(String)
                  is_api? ? :url : (raise FastImage::UnknownImageType)
@@ -403,4 +364,33 @@ class UsersController < ApplicationController
       render json: { url: upload.url, width: upload.width, height: upload.height }
     end
 
+    def respond_to_suspicious_request
+      if suspicious?(params)
+        render(
+          json: {
+            success: true,
+            active: false,
+            message: I18n.t("login.activate_email", email: params[:email])
+          }
+        )
+      end
+    end
+
+    def suspicious?(params)
+      honeypot_or_challenge_fails?(params) || SiteSetting.invite_only?
+    end
+
+    def honeypot_or_challenge_fails?(params)
+      params[:password_confirmation] != honeypot_value ||
+        params[:challenge] != challenge_value.try(:reverse)
+    end
+
+    def user_params
+      params.permit(
+        :name,
+        :email,
+        :password,
+        :username
+      ).merge(ip_address: request.ip)
+    end
 end

app/services/user_activator.rb

@@ -1,16 +1,22 @@
-# Responsible for dealing with different activation processes when a user is created
 class UserActivator
-  attr_reader :user, :request, :session, :cookies
+  attr_reader :user, :request, :session, :cookies, :message
 
   def initialize(user, request, session, cookies)
     @user = user
     @session = session
     @cookies = cookies
     @request = request
+    @settings = SiteSetting
+    @hub = DiscourseHub
+    @message = nil
   end
 
-  def activation_message
+  def start
-    activator.activate
+    register_nickname
+  end
+
+  def finish
+    @message = activator.activate
   end
 
   private
@@ -20,14 +26,19 @@ class UserActivator
   end
 
   def factory
-    if SiteSetting.must_approve_users?
+    if @settings.must_approve_users?
       ApprovalActivator
     elsif !user.active?
       EmailActivator
     else
       LoginActivator
     end
+  end
 
+  def register_nickname
+    if user.valid? && @settings.call_discourse_hub?
+      @hub.register_nickname(user.username, user.email)
+    end
   end
 end
 

app/services/user_authenticator.rb

@@ -0,0 +1,39 @@
+class UserAuthenticator
+  def initialize(user, session, authenticator_finder = Users::OmniauthCallbacksController)
+    @user = user
+    @session = session[:authentication]
+    @authenticator_finder = authenticator_finder
+  end
+
+  def start
+    if authenticated?
+      @user.active = true
+    else
+      @user.password_required!
+    end
+  end
+
+  def finish
+    if authenticator
+      authenticator.after_create_account(@user, @session)
+    end
+
+    @session = nil
+  end
+
+  private
+
+  def authenticated?
+    @session && @session[:email] == @user.email && @session[:email_valid]
+  end
+
+  def authenticator
+    if authenticator_name
+      @authenticator ||= @authenticator_finder.find_authenticator(authenticator_name)
+    end
+  end
+
+  def authenticator_name
+    @session && @session[:authenticator_name]
+  end
+end

spec/controllers/users_controller_spec.rb

@@ -277,12 +277,9 @@ describe UsersController do
         session[:current_user_id].should be_blank
       end
     end
-
-
   end
 
-
+  describe '#create' do
-  describe '.create' do
     before do
       @user = Fabricate.build(:user)
       @user.password = "strongpassword"