diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 85cbcacd6a5..29591269ca0 100644 --- a/app/controllers/users_controller.rb +++ b/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) - user.ip_address = request.ip - auth = authenticate_user(user, params) - register_nickname(user) + authentication = UserAuthenticator.new(user, session) + authentication.start - 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 diff --git a/lib/user_activator.rb b/app/services/user_activator.rb similarity index 69% rename from lib/user_activator.rb rename to app/services/user_activator.rb index aa124d14eb5..fd76126d16c 100644 --- a/lib/user_activator.rb +++ b/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 - activator.activate + def start + 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 diff --git a/app/services/user_authenticator.rb b/app/services/user_authenticator.rb new file mode 100644 index 00000000000..e350dbbeb03 --- /dev/null +++ b/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 diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 4c66603b8c3..2363936e9e6 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/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"