SECURITY: when enabled_local_logins is false users could log in via API

thanks @Nicholas Blanco

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

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)

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

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