diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 0198d937820..479839a7e86 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -140,83 +140,83 @@ class UsersController < ApplicationController def create - if params[:password_confirmation] != honeypot_value || params[:challenge] != challenge_value.try(:reverse) + if honeypot_or_challenge_fails?(params) # Don't give any indication that we caught you in the honeypot - return render(json: {success: true, active: false, message: I18n.t("login.activate_email", email: params[:email]) }) + honey_pot_response = { + success: true, + active: false, + message: I18n.t("login.activate_email", email: params[:email]) + } + return render(json: honey_pot_response) end - user = User.new - user.name = params[:name] - user.email = params[:email] - user.password = params[:password] - user.username = params[:username] + user = User.new_from_params(params) auth = session[:authentication] - if auth && auth[:email] == params[:email] && auth[:email_valid] + if valid_session_authentication?(auth, params[:email]) user.active = true end user.password_required! unless auth - DiscourseHub.register_nickname( user.username, user.email ) if user.valid? && SiteSetting.call_discourse_hub? + if user.valid? && SiteSetting.call_discourse_hub? + DiscourseHub.register_nickname(user.username, user.email) + end if user.save - msg = nil - active_result = user.active? - if active_result + active_user = user.active? + if active_user # If the user is active (remote authorized email) if SiteSetting.must_approve_users? msg = I18n.t("login.wait_approval") - active_result = false + active_user = false else log_on_user(user) user.enqueue_welcome_message('welcome_user') msg = I18n.t("login.active") end - else msg = I18n.t("login.activate_email", email: user.email) - Jobs.enqueue(:user_email, type: :signup, user_id: user.id, email_token: user.email_tokens.first.token) - end - - # Create auth records - if auth.present? - if auth[:twitter_user_id] && auth[:twitter_screen_name] && TwitterUserInfo.find_by_twitter_user_id(auth[:twitter_user_id]).nil? - TwitterUserInfo.create(user_id: user.id, screen_name: auth[:twitter_screen_name], twitter_user_id: auth[:twitter_user_id]) - end - - if auth[:facebook].present? && FacebookUserInfo.find_by_facebook_user_id(auth[:facebook][:facebook_user_id]).nil? - FacebookUserInfo.create!(auth[:facebook].merge(user_id: user.id)) - end - - if auth[:github_user_id] && auth[:github_screen_name] && GithubUserInfo.find_by_github_user_id(auth[:github_user_id]).nil? - GithubUserInfo.create(user_id: user.id, screen_name: auth[:github_screen_name], github_user_id: auth[:github_user_id]) - end + Jobs.enqueue( + :user_email, type: :signup, user_id: user.id, + email_token: user.email_tokens.first.token + ) end + # Create 3rd party auth records (Twitter, Facebook, GitHub) + create_third_party_auth_records(user, auth) if auth.present? # Clear authentication session. session[:authentication] = nil # JSON result - render json: {success: true, active: active_result, message: msg } + render json: { success: true, active: active_user, message: msg } else - render json: {success: false, message: I18n.t("login.errors", errors: user.errors.full_messages.join("\n"))} + render json: { + success: false, + message: I18n.t("login.errors", errors: user.errors.full_messages.join("\n")) + } 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 - render json: {success: false, message: I18n.t("login.errors", errors:I18n.t("login.not_available", suggestion: User.suggest_username(params[:username])) )} + render json: { success: false, + message: I18n.t( + "login.errors", + errors:I18n.t( + "login.not_available", suggestion: User.suggest_username(params[:username]) + ) + ) + } rescue RestClient::Forbidden - render json: {errors: [I18n.t("discourse_hub.access_token_problem")]} + render json: { errors: [I18n.t("discourse_hub.access_token_problem")] } end def get_honeypot_value render json: {value: honeypot_value, challenge: challenge_value} end - # all avatars are funneled through here def avatar @@ -224,13 +224,10 @@ class UsersController < ApplicationController # raise ActiveRecord::RecordNotFound user = User.select(:email).where(username_lower: params[:username].downcase).first - if user - # for now we only support gravatar in square (redirect cached for a day), later we can use x-sendfile and/or a cdn to serve local - size = params[:size].to_i - size = 64 if size == 0 - size = 10 if size < 10 - size = 128 if size > 128 - + if user.present? + # for now we only support gravatar in square (redirect cached for a day), + # later we can use x-sendfile and/or a cdn to serve local + size = determine_avatar_size(params[:size]) url = user.avatar_template.gsub("{size}", size.to_s) expires_in 1.day redirect_to url @@ -270,14 +267,18 @@ class UsersController < ApplicationController guardian.ensure_can_edit!(user) # Raise an error if the email is already in use - raise Discourse::InvalidParameters.new(:email) if User.where("lower(email) = ?", params[:email].downcase).exists? + if User.where("lower(email) = ?", params[:email].downcase).exists? + raise Discourse::InvalidParameters.new(:email) + end email_token = user.email_tokens.create(email: params[:email]) - Jobs.enqueue(:user_email, - to_address: params[:email], - type: :authorize_email, - user_id: user.id, - email_token: email_token.token) + Jobs.enqueue( + :user_email, + to_address: params[:email], + type: :authorize_email, + user_id: user.id, + email_token: email_token.token + ) render nothing: true end @@ -327,8 +328,8 @@ class UsersController < ApplicationController results = UserSearch.search term, topic_id - render json: { users: results.as_json( only: [ :username, :name ], - methods: :avatar_template ) } + render json: { users: results.as_json(only: [ :username, :name ], + methods: :avatar_template) } end private @@ -351,4 +352,58 @@ class UsersController < ApplicationController guardian.ensure_can_see!(user) user 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) + if twitter_auth?(auth) + TwitterUserInfo.create( + user_id: user.id, + screen_name: auth[:twitter_screen_name], + twitter_user_id: auth[:twitter_user_id] + ) + end + + if facebook_auth?(auth) + FacebookUserInfo.create!(auth[:facebook].merge(user_id: user.id)) + end + + if github_auth?(auth) + GithubUserInfo.create( + user_id: user.id, + screen_name: auth[:github_screen_name], + github_user_id: auth[:github_user_id] + ) + end + end + + def twitter_auth?(auth) + auth[:twitter_user_id] && auth[:twitter_screen_name] && + TwitterUserInfo.find_by_twitter_user_id(auth[:twitter_user_id]).nil? + end + + def facebook_auth?(auth) + auth[:facebook].present? && + FacebookUserInfo.find_by_facebook_user_id(auth[:facebook][:facebook_user_id]).nil? + end + + def github_auth?(auth) + auth[:github_user_id] && auth[:github_screen_name] && + GithubUserInfo.find_by_github_user_id(auth[:github_user_id]).nil? + end + + def determine_avatar_size(size) + size = size.to_i + size = 64 if size == 0 + size = 10 if size < 10 + size = 128 if size > 128 + size + end end diff --git a/app/models/user.rb b/app/models/user.rb index 760167e8dbb..600b6f28036 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -102,6 +102,15 @@ class User < ActiveRecord::Base find_available_username_based_on(name) end + def self.new_from_params(params) + user = User.new + user.name = params[:name] + user.email = params[:email] + user.password = params[:password] + user.username = params[:username] + user + end + def self.create_for_email(email, opts={}) username = suggest_username(email) diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index f3676dc0a88..b99926fc735 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -277,12 +277,14 @@ describe UsersController do context 'when creating a non active user (unconfirmed email)' do it 'should enqueue a signup email' do Jobs.expects(:enqueue).with(:user_email, has_entries(type: :signup)) - xhr :post, :create, name: @user.name, username: @user.username, password: "strongpassword", email: @user.email + xhr :post, :create, name: @user.name, username: @user.username, + password: "strongpassword", email: @user.email end it "doesn't send a welcome email" do User.any_instance.expects(:enqueue_welcome_message).with('welcome_user').never - xhr :post, :create, name: @user.name, username: @user.username, password: "strongpassword", email: @user.email + xhr :post, :create, name: @user.name, username: @user.username, + password: "strongpassword", email: @user.email end end @@ -294,7 +296,8 @@ describe UsersController do it 'should enqueue a signup email' do User.any_instance.expects(:enqueue_welcome_message).with('welcome_user') - xhr :post, :create, name: @user.name, username: @user.username, password: "strongpassword", email: @user.email + xhr :post, :create, name: @user.name, username: @user.username, + password: "strongpassword", email: @user.email end it "should be logged in" do @@ -309,6 +312,7 @@ describe UsersController do ::JSON.parse(response.body)['active'].should == true end + context 'when approving of users is required' do before do SiteSetting.expects(:must_approve_users).returns(true) @@ -322,14 +326,51 @@ describe UsersController do it "doesn't return active in the JSON" do ::JSON.parse(response.body)['active'].should == false end - end + context 'authentication records for' do + + before do + SiteSetting.expects(:must_approve_users).returns(true) + end + + it 'should create twitter user info if none exists' do + twitter_auth = { twitter_user_id: 42, twitter_screen_name: "bruce" } + session[:authentication] = twitter_auth + TwitterUserInfo.expects(:find_by_twitter_user_id).returns(nil) + TwitterUserInfo.expects(:create) + + xhr :post, :create, name: @user.name, username: @user.username, + password: "strongpassword", email: @user.email + end + + it 'should create facebook user info if none exists' do + fb_auth = { facebook: { facebook_user_id: 42} } + session[:authentication] = fb_auth + FacebookUserInfo.expects(:find_by_facebook_user_id).returns(nil) + FacebookUserInfo.expects(:create!) + + xhr :post, :create, name: @user.name, username: @user.username, + password: "strongpassword", email: @user.email + end + + it 'should create github user info if none exists' do + gh_auth = { github_user_id: 2, github_screen_name: "bruce" } + session[:authentication] = gh_auth + GithubUserInfo.expects(:find_by_github_user_id).returns(nil) + GithubUserInfo.expects(:create) + + xhr :post, :create, name: @user.name, username: @user.username, + password: "strongpassword", email: @user.email + end + + end end context 'after success' do before do - xhr :post, :create, name: @user.name, username: @user.username, password: "strongpassword", email: @user.email + xhr :post, :create, name: @user.name, username: @user.username, + password: "strongpassword", email: @user.email end it 'should succeed' do @@ -403,13 +444,22 @@ describe UsersController do it_should_behave_like 'failed signup' end - context 'when InvalidStatement is raised' do - before do - User.any_instance.stubs(:save).raises(ActiveRecord::StatementInvalid) + context 'when an Exception is raised' do + + [ ActiveRecord::StatementInvalid, + DiscourseHub::NicknameUnavailable, + RestClient::Forbidden ].each do |exception| + before { User.any_instance.stubs(:save).raises(exception) } + + let(:create_params) { + { name: @user.name, username: @user.username, + password: "strongpassword", email: @user.email} + } + + it_should_behave_like 'failed signup' end - let(:create_params) { {name: @user.name, username: @user.username, password: "strongpassword", email: @user.email} } - it_should_behave_like 'failed signup' end + end context '.username' do @@ -704,7 +754,9 @@ describe UsersController do context 'not logged in' do it 'raises an error when not logged in' do - lambda { xhr :put, :update, username: 'somename' }.should raise_error(Discourse::NotLoggedIn) + expect do + xhr :put, :update, username: 'somename' + end.to raise_error(Discourse::NotLoggedIn) end end @@ -735,7 +787,6 @@ describe UsersController do end end end - end describe "search_users" do