FIX: Logout redirect should only be `/login` for login_required sites (#11466)

25563357 moved the logout redirect logic from the client-side to the server-side. Unfortunately the login_required check was lost during the refactoring which meant that non-login-required sites would redirect to `/login` after redirect, and immediately restart the login process. Depending on the SSO implementation, that can make it impossible for users to log out cleanly.

This commit restores the login_required check, and prevents the potential redirect loop.
This commit is contained in:
David Taylor 2020-12-11 09:44:16 +00:00 committed by GitHub
parent 55c00ba2dd
commit 36b4712349
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 7 additions and 2 deletions

View File

@ -446,7 +446,7 @@ class SessionController < ApplicationController
sso = SiteSetting.enable_sso
only_one_authenticator = !SiteSetting.enable_local_logins && Discourse.enabled_authenticators.length == 1
if sso || only_one_authenticator
if SiteSetting.login_required && (sso || only_one_authenticator)
# In this situation visiting most URLs will start the auth process again
# Go to the `/login` page to avoid an immediate redirect
redirect_url ||= path("/login")

View File

@ -1779,13 +1779,18 @@ RSpec.describe SessionController do
expect(response.parsed_body["redirect_url"]).to eq("/")
end
it 'redirects to /login for SSO' do
it 'redirects to /login when SSO and login_required' do
SiteSetting.sso_url = "https://example.com/sso"
SiteSetting.enable_sso = true
user = sign_in(Fabricate(:user))
delete "/session/#{user.username}.json", xhr: true
expect(response.status).to eq(200)
expect(response.parsed_body["redirect_url"]).to eq("/")
SiteSetting.login_required = true
user = sign_in(Fabricate(:user))
delete "/session/#{user.username}.json", xhr: true
expect(response.status).to eq(200)
expect(response.parsed_body["redirect_url"]).to eq("/login")
end