From 1014e56e804049bcfb0b38029408f73313ec6290 Mon Sep 17 00:00:00 2001
From: Martin Brennan <mjrbrennan@gmail.com>
Date: Mon, 20 Jan 2020 16:11:58 +1000
Subject: [PATCH] DEV: Respond with 403 instead of 500 for disabled local login
 via email

Previously if local login via email was disabled because of the site setting or because SSO was enabled, we were raising a 500 error. We now raise a 403 error instead; we shouldn't raise 500 errors on purpose, instead keeping that code for unhandled errors. It doesn't make sense in the context of what we are validating either to raise a 500.
---
 app/controllers/session_controller.rb    |  7 +------
 spec/requests/session_controller_spec.rb | 16 ++++++++--------
 2 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb
index ff6627eebf8..6103d012205 100644
--- a/app/controllers/session_controller.rb
+++ b/app/controllers/session_controller.rb
@@ -1,11 +1,6 @@
 # frozen_string_literal: true
 
 class SessionController < ApplicationController
-  class LocalLoginNotAllowed < StandardError; end
-  rescue_from LocalLoginNotAllowed do
-    render body: nil, status: 500
-  end
-
   before_action :check_local_login_allowed, only: %i(create forgot_password)
   before_action :rate_limit_login, only: %i(create email_login)
   before_action :rate_limit_second_factor_totp, only: %i(create email_login)
@@ -438,7 +433,7 @@ class SessionController < ApplicationController
     if (check_login_via_email && !SiteSetting.enable_local_logins_via_email) ||
         SiteSetting.enable_sso ||
         !SiteSetting.enable_local_logins
-      raise LocalLoginNotAllowed, "SSO takes over local login or the local login is disallowed."
+      raise Discourse::InvalidAccess, "SSO takes over local login or the local login is disallowed."
     end
   end
 
diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb
index 18d7cbc9158..e7e54946f19 100644
--- a/spec/requests/session_controller_spec.rb
+++ b/spec/requests/session_controller_spec.rb
@@ -11,7 +11,7 @@ RSpec.describe SessionController do
   shared_examples 'failed to continue local login' do
     it 'should return the right response' do
       expect(response).not_to be_successful
-      expect(response.status).to eq(500)
+      expect(response.status).to eq(403)
     end
   end
 
@@ -25,7 +25,7 @@ RSpec.describe SessionController do
 
       it "only works for admins" do
         get "/session/email-login/#{email_token.token}.json"
-        expect(response.status).to eq(500)
+        expect(response.status).to eq(403)
 
         user.update(admin: true)
         get "/session/email-login/#{email_token.token}.json"
@@ -41,7 +41,7 @@ RSpec.describe SessionController do
 
       it "only works for admins" do
         get "/session/email-login/#{email_token.token}.json"
-        expect(response.status).to eq(500)
+        expect(response.status).to eq(403)
 
         user.update(admin: true)
         get "/session/email-login/#{email_token.token}.json"
@@ -72,7 +72,7 @@ RSpec.describe SessionController do
 
         get "/session/email-login/#{email_token.token}.json"
 
-        expect(response.status).to eq(500)
+        expect(response.status).to eq(403)
       end
 
       it 'fails when local logins is disabled' do
@@ -80,7 +80,7 @@ RSpec.describe SessionController do
 
         get "/session/email-login/#{email_token.token}.json"
 
-        expect(response.status).to eq(500)
+        expect(response.status).to eq(403)
       end
 
       context 'user has 2-factor logins' do
@@ -127,7 +127,7 @@ RSpec.describe SessionController do
 
       it "only works for admins" do
         post "/session/email-login/#{email_token.token}.json"
-        expect(response.status).to eq(500)
+        expect(response.status).to eq(403)
 
         user.update(admin: true)
         post "/session/email-login/#{email_token.token}.json"
@@ -181,7 +181,7 @@ RSpec.describe SessionController do
 
         post "/session/email-login/#{email_token.token}.json"
 
-        expect(response.status).to eq(500)
+        expect(response.status).to eq(403)
         expect(session[:current_user_id]).to eq(nil)
       end
 
@@ -190,7 +190,7 @@ RSpec.describe SessionController do
 
         post "/session/email-login/#{email_token.token}.json"
 
-        expect(response.status).to eq(500)
+        expect(response.status).to eq(403)
         expect(session[:current_user_id]).to eq(nil)
       end