From be06156629c38d550bc8bc5137add1fdd7aeebdf Mon Sep 17 00:00:00 2001 From: Sam Date: Wed, 26 Mar 2014 15:39:44 +1100 Subject: [PATCH] SECURITY: when enabled_local_logins is false users could log in via API thanks @Nicholas Blanco --- app/controllers/session_controller.rb | 8 ++- app/controllers/users_controller.rb | 6 ++ spec/controllers/session_controller_spec.rb | 61 +++++++++++---------- spec/controllers/users_controller_spec.rb | 35 ++++++------ 4 files changed, 60 insertions(+), 50 deletions(-) diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index b276578c61c..0533ac4ae2d 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -44,7 +44,7 @@ class SessionController < ApplicationController def create - if SiteSetting.enable_sso + unless allow_local_auth? render nothing: true, status: 500 return end @@ -88,7 +88,7 @@ class SessionController < ApplicationController def forgot_password params.require(:login) - if SiteSetting.enable_sso + unless allow_local_auth? render nothing: true, status: 500 return end @@ -118,6 +118,10 @@ class SessionController < ApplicationController private + def allow_local_auth? + !SiteSetting.enable_sso && SiteSetting.enable_local_logins + end + def login_not_approved_for?(user) SiteSetting.must_approve_users? && !user.approved? && !user.admin? end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index b0f3dd54e98..104938562fd 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -123,6 +123,12 @@ class UsersController < ApplicationController user = User.new(user_params) authentication = UserAuthenticator.new(user, session) + + if !authentication.has_authenticator? && !SiteSetting.enable_local_logins + render nothing: true, status: 500 + return + end + authentication.start activation = UserActivator.new(user, request, session, cookies) diff --git a/spec/controllers/session_controller_spec.rb b/spec/controllers/session_controller_spec.rb index 35f256355cd..8813417909c 100644 --- a/spec/controllers/session_controller_spec.rb +++ b/spec/controllers/session_controller_spec.rb @@ -90,78 +90,72 @@ describe SessionController do get :sso_login, Rack::Utils.parse_query(sso.payload) response.code.should == '500' end - - describe 'local attribute ovveride from SSO payload' do + + describe 'local attribute override from SSO payload' do before do SiteSetting.stubs("sso_overrides_email").returns(true) SiteSetting.stubs("sso_overrides_username").returns(true) SiteSetting.stubs("sso_overrides_name").returns(true) - + @user = Fabricate(:user) - + @sso = get_sso('/hello/world') @sso.external_id = '997' - + @reversed_username = @user.username.reverse @sso.username = @reversed_username - @sso.email = "#{@reversed_username}@garbage.org" @reversed_name = @user.name.reverse - @sso.name = @reversed_name - + @suggested_username = UserNameSuggester.suggest(@sso.username || @sso.name || @sso.email) @suggested_name = User.suggest_name(@sso.name || @sso.username || @sso.email) - @user.create_single_sign_on_record(external_id: '997', last_payload: '') end - + it 'stores the external attributes' do get :sso_login, Rack::Utils.parse_query(@sso.payload) - @user.single_sign_on_record.reload @user.single_sign_on_record.external_username.should == @sso.username @user.single_sign_on_record.external_email.should == @sso.email @user.single_sign_on_record.external_name.should == @sso.name end - + it 'overrides attributes' do get :sso_login, Rack::Utils.parse_query(@sso.payload) - + logged_on_user = Discourse.current_user_provider.new(request.env).current_user - logged_on_user.username.should == @suggested_username logged_on_user.email.should == "#{@reversed_username}@garbage.org" logged_on_user.name.should == @suggested_name end - + it 'does not change matching attributes for an existing account' do @sso.username = @user.username @sso.name = @user.name @sso.email = @user.email - + get :sso_login, Rack::Utils.parse_query(@sso.payload) - + logged_on_user = Discourse.current_user_provider.new(request.env).current_user logged_on_user.username.should == @user.username logged_on_user.name.should == @user.name logged_on_user.email.should == @user.email end - + it 'does not change attributes for unchanged external attributes' do @user.single_sign_on_record.external_username = @sso.username @user.single_sign_on_record.external_email = @sso.email @user.single_sign_on_record.external_name = @sso.name @user.single_sign_on_record.save - + get :sso_login, Rack::Utils.parse_query(@sso.payload) - logged_on_user = Discourse.current_user_provider.new(request.env).current_user logged_on_user.username.should == @user.username logged_on_user.email.should == @user.email logged_on_user.name.should == @user.name end - end + end end describe '.create' do @@ -195,24 +189,25 @@ describe SessionController do end describe 'success by username' do - before do + it 'logs in correctly' do xhr :post, :create, login: user.username, password: 'myawesomepassword' + user.reload - end - it 'sets a session id' do session[:current_user_id].should == user.id - end - - it 'gives the user an auth token' do user.auth_token.should be_present - end - - it 'sets a cookie with the auth token' do cookies[:_t].should == user.auth_token end end + describe 'local logins disabled' do + it 'fails' do + SiteSetting.stubs(:enable_local_logins).returns(false) + xhr :post, :create, login: user.username, password: 'myawesomepassword' + response.status.to_i.should == 500 + end + end + describe 'strips leading @ symbol' do before do xhr :post, :create, login: "@" + user.username, password: 'myawesomepassword' @@ -349,6 +344,12 @@ describe SessionController do context 'for an existing username' do let(:user) { Fabricate(:user) } + it "returns a 500 if local logins are disabled" do + SiteSetting.stubs(:enable_local_logins).returns(false) + xhr :post, :forgot_password, login: user.username + response.code.to_i.should == 500 + end + it "generates a new token for a made up username" do lambda { xhr :post, :forgot_password, login: user.username}.should change(EmailToken, :count) end diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index ab299849aa4..0b401e9800c 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -115,7 +115,7 @@ describe UsersController do end - context 'reponse' do + context 'response' do before do Guardian.any_instance.expects(:can_access_forum?).returns(true) EmailToken.expects(:confirm).with('asdfasdf').returns(user) @@ -295,18 +295,20 @@ describe UsersController do end context 'when creating a non active user (unconfirmed email)' do - it 'enqueues a signup email' do + + it 'returns a 500 when local logins are disabled' do + SiteSetting.expects(:enable_local_logins).returns(false) + post_user + + expect(response.status).to eq(500) + end + + it 'creates a user correctly' do Jobs.expects(:enqueue).with(:user_email, has_entries(type: :signup)) - post_user - end - - it 'does not enqueue a welcome email' do User.any_instance.expects(:enqueue_welcome_message).with('welcome_user').never - post_user - end - it 'indicates the user is not active in the response' do post_user + expect(JSON.parse(response.body)['active']).to be_false end @@ -1199,7 +1201,7 @@ describe UsersController do xhr :post, :upload_user_image, username: user.username, file: user_image_url, user_image_type: "profile_background" response.status.should eq 422 end - + it 'rejects requests with unknown user_image_type' do xhr :post, :upload_user_image, username: user.username, file: user_image_url, user_image_type: "asdf" response.status.should eq 422 @@ -1224,22 +1226,20 @@ describe UsersController do json['width'].should == 100 json['height'].should == 200 end - + it 'is successful for profile backgrounds' do upload = Fabricate(:upload) Upload.expects(:create_for).returns(upload) xhr :post, :upload_user_image, username: user.username, file: user_image_url, user_image_type: "profile_background" user.reload - user.profile_background.should == "/uploads/default/1/1234567890123456.jpg" - + # returns the url, width and height of the uploaded image json = JSON.parse(response.body) json['url'].should == "/uploads/default/1/1234567890123456.jpg" json['width'].should == 100 json['height'].should == 200 end - end it "should handle malformed urls" do @@ -1282,13 +1282,13 @@ describe UsersController do end end - + describe '.clear_profile_background' do - + it 'raises an error when not logged in' do lambda { xhr :put, :clear_profile_background, username: 'asdf' }.should raise_error(Discourse::NotLoggedIn) end - + context 'while logged in' do let!(:user) { log_in } @@ -1306,7 +1306,6 @@ describe UsersController do end end - end describe '.destroy' do