From 9c04aa593c180873f9158d3497274042be06de93 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Fri, 17 Jan 2020 11:25:31 +1000 Subject: [PATCH] Fix broken admin login fro SSO enabled sites (#8737) * When we refactored away the admin-login route we introduced a bug where admins could not log into an SSO enabled site, because of a check in the email_login route that disallowed this. * Allow admin to get around this check. --- app/controllers/session_controller.rb | 26 +++++----- spec/requests/session_controller_spec.rb | 61 ++++++++++++++++++++++-- 2 files changed, 70 insertions(+), 17 deletions(-) diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index 0338bd85a4b..ff6627eebf8 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -6,7 +6,7 @@ class SessionController < ApplicationController render body: nil, status: 500 end - before_action :check_local_login_allowed, only: %i(create forgot_password email_login email_login_info) + 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) skip_before_action :redirect_to_login_if_required @@ -303,11 +303,9 @@ class SessionController < ApplicationController def email_login_info token = params[:token] matched_token = EmailToken.confirmable(token) + user = matched_token&.user - if !SiteSetting.enable_local_logins_via_email && - !matched_token.user.admin? # admin-login uses this route, so allow them even if disabled - raise Discourse::NotFound - end + check_local_login_allowed(user: user, check_login_via_email: true) if matched_token response = { @@ -343,13 +341,10 @@ class SessionController < ApplicationController def email_login token = params[:token] matched_token = EmailToken.confirmable(token) - - if !SiteSetting.enable_local_logins_via_email && - !matched_token&.user&.admin? # admin-login uses this route, so allow them even if disabled - raise Discourse::NotFound - end - user = matched_token&.user + + check_local_login_allowed(user: user, check_login_via_email: true) + if user.present? && !authenticate_second_factor(user) return render(json: @second_factor_failure_payload) end @@ -436,8 +431,13 @@ class SessionController < ApplicationController protected - def check_local_login_allowed - if SiteSetting.enable_sso || !SiteSetting.enable_local_logins + def check_local_login_allowed(user: nil, check_login_via_email: false) + # admin-login can get around enabled SSO/disabled local logins + return if user&.admin? + + 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." end end diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb index 28afc72be68..18d7cbc9158 100644 --- a/spec/requests/session_controller_spec.rb +++ b/spec/requests/session_controller_spec.rb @@ -25,7 +25,23 @@ RSpec.describe SessionController do it "only works for admins" do get "/session/email-login/#{email_token.token}.json" - expect(response.status).to eq(404) + expect(response.status).to eq(500) + + user.update(admin: true) + get "/session/email-login/#{email_token.token}.json" + expect(response.status).to eq(200) + end + end + + context "when SSO enabled" do + before do + SiteSetting.sso_url = "https://www.example.com/sso" + SiteSetting.enable_sso = true + end + + it "only works for admins" do + get "/session/email-login/#{email_token.token}.json" + expect(response.status).to eq(500) user.update(admin: true) get "/session/email-login/#{email_token.token}.json" @@ -56,7 +72,7 @@ RSpec.describe SessionController do get "/session/email-login/#{email_token.token}.json" - expect(response.status).to eq(404) + expect(response.status).to eq(500) end it 'fails when local logins is disabled' do @@ -111,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(404) + expect(response.status).to eq(500) user.update(admin: true) post "/session/email-login/#{email_token.token}.json" @@ -165,7 +181,7 @@ RSpec.describe SessionController do post "/session/email-login/#{email_token.token}.json" - expect(response.status).to eq(404) + expect(response.status).to eq(500) expect(session[:current_user_id]).to eq(nil) end @@ -1118,6 +1134,21 @@ RSpec.describe SessionController do it_behaves_like "failed to continue local login" end + context 'local login via email is disabled' do + before do + SiteSetting.enable_local_logins_via_email = false + end + it 'doesnt matter, logs in correctly' do + events = DiscourseEvent.track_events do + post "/session.json", params: { + login: user.username, password: 'myawesomepassword' + } + end + + expect(response.status).to eq(200) + end + end + context 'when email is confirmed' do before do token = user.email_tokens.find_by(email: user.email) @@ -1767,6 +1798,28 @@ RSpec.describe SessionController do it_behaves_like "failed to continue local login" end + context "local logins are disabled" do + before do + SiteSetting.enable_local_logins = false + + post "/session.json", params: { + login: user.username, password: 'myawesomepassword' + } + end + it_behaves_like "failed to continue local login" + end + + context "local logins via email are disabled" do + before do + SiteSetting.enable_local_logins_via_email = false + end + it "does not matter, generates a new token for a made up username" do + expect do + post "/session/forgot_password.json", params: { login: user.username } + end.to change(EmailToken, :count) + end + end + it "generates a new token for a made up username" do expect do post "/session/forgot_password.json", params: { login: user.username }