From a6671e32bdc866a410d5bd72933af74c8e34da76 Mon Sep 17 00:00:00 2001 From: Tom Elliott Date: Tue, 18 Aug 2026 07:33:22 -0500 Subject: [PATCH] OIDC: after a successful single logout, land on the login page that redirects Reported from the lab: with single logout and forced redirect both on, signing out left you on management/login.php -- the break-glass page -- rather than on FOG's ordinary login page. That was wrong, and the reasoning behind it was wrong in a specific way. #18 pointed post_logout_redirect_uri at login.php to avoid a loop: index.php on a forced-redirect install sends the browser back to the provider, and if the provider session were still alive that is a silent re-login. But single logout is exactly the case where the provider session is NOT still alive. It was just ended, by the request that is carrying this parameter. So the browser goes back to index.php, index.php sends it to the provider, the provider has no session, and it asks who you are. That is signing out and signing in as somebody else in one continuous journey, which is the thing single logout exists to make possible -- and it was being cut short one page early. So the two landings are now separate, because they answer different questions: OIDC::postLogoutUri() -> management/index.php. The provider session HAS been ended, so bouncing back to it is correct. OIDC::localLoginUrl() -> management/login.php. Used where bouncing back really would loop or silently sign the person in again: - OIDCFlow::_fail(), where the provider just refused, so returning to it is a loop - the USER_LOGGING_OUT fallback when single logout is OFF and forced redirect is on, where the SSO session is untouched and for the URL printed beside the forced-redirect setting, which is the escape hatch and must never be the redirecting page. ADMINS MUST RE-REGISTER: the post-logout redirect URI to register at the provider changes from .../management/login.php to .../management/index.php. The management page prints the current value; providers that follow the spec refuse an unregistered one and show their own error page instead of returning, so an install upgrading from v1.6.9 has to update it there. Both gates updated. The single-logout gate now pins the two URLs separately, pins that they differ (collapsing them re-creates whichever bug the other was avoiding), and pins post_logout_redirect_uri as a KEY and value pair rather than merely mentioning the method -- a mutation that rebound the value to another parameter name survived the first draft. 7/7 mutations caught; sh tests/run-all.sh -> 6 passed, 0 failed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017aBSWrDArXHTpKWkkN27LR --- oidc/class/oidc.class.php | 39 +++++++++++++++---- oidc/hooks/oidclogout.hook.php | 7 +++- oidc/pages/oidcmanagement.page.php | 2 +- tests/oidc-auto-redirect.test.php | 4 +- tests/oidc-single-logout.test.php | 62 ++++++++++++++++++++++-------- 5 files changed, 88 insertions(+), 26 deletions(-) diff --git a/oidc/class/oidc.class.php b/oidc/class/oidc.class.php index 411a423..00572c3 100644 --- a/oidc/class/oidc.class.php +++ b/oidc/class/oidc.class.php @@ -284,13 +284,19 @@ public static function redirectUri() /** * Where a provider sends the browser after ending its own session. * - * management/login.php rather than management/index.php, and the - * difference is the whole point: index.php is the page an install with - * forced redirect on (#17) bounces straight back to the provider. A - * signed-out user landing there would be silently signed back in by the - * SSO session that was just ended -- or, if it really was ended, sent - * around the loop again. login.php always renders FOG's own form - * (fogproject#1175). + * FOG's ordinary login page, which is the whole point of having ended + * the provider session: with forced redirect on (#17) that page sends + * the browser back to the provider, the provider now has no session, + * and it asks who you are. Signing out and then signing in as somebody + * else is one continuous journey rather than a dead end. + * + * There is no loop here BECAUSE single logout ran. That is the + * distinction against the two places that do point at login.php: + * + * OIDCFlow::_fail() -- the provider refused, so + * bouncing back to it is a loop + * OIDCLogout, single logout off -- the SSO session is still alive, + * so index.php signs you back in * * This URL has to be registered at the provider as a post-logout * redirect URI, the same way the callback does. Providers that follow @@ -302,6 +308,25 @@ public static function redirectUri() * @return string */ public static function postLogoutUri() + { + return self::absoluteUrl('management/index.php'); + } + /** + * The login page no provider setting can redirect away from. + * + * fogproject#1175. Core offers the LOGIN_PAGE_REDIRECT hook only when + * FOG_LOCAL_LOGIN is undefined, and management/login.php defines it -- + * so on that URL this plugin is never asked where to send anybody, and + * a provider that is unreachable or misconfigured cannot take the page + * down with it. + * + * Used for the two landings that must NOT bounce back to a provider, + * and printed beside the forced-redirect setting so the admin turning + * it on is told the way back before they need it. + * + * @return string + */ + public static function localLoginUrl() { return self::absoluteUrl('management/login.php'); } diff --git a/oidc/hooks/oidclogout.hook.php b/oidc/hooks/oidclogout.hook.php index 93336ce..a7359ab 100644 --- a/oidc/hooks/oidclogout.hook.php +++ b/oidc/hooks/oidclogout.hook.php @@ -97,8 +97,13 @@ public function providerLogout($arguments) * It does not end the provider session -- only single logout * does -- but it leaves somebody looking at a form instead of * back where they started. + * + * Deliberately NOT postLogoutUri(), which is the ordinary login + * page: that is the right landing when single logout HAS run, + * because the provider will then ask who you are instead of + * waving you through. Here it has not. */ - $url = OIDC::postLogoutUri(); + $url = OIDC::localLoginUrl(); } if ('' === $url) { return; diff --git a/oidc/pages/oidcmanagement.page.php b/oidc/pages/oidcmanagement.page.php index 33a7a95..c197836 100644 --- a/oidc/pages/oidcmanagement.page.php +++ b/oidc/pages/oidcmanagement.page.php @@ -511,7 +511,7 @@ public function oidcGeneral() // locked out of their own server -- and the URL is not // something they could guess at that point. _('the local login form stays available at %s'), - '' . Initiator::e(OIDC::postLogoutUri()) . '' + '' . Initiator::e(OIDC::localLoginUrl()) . '' ) . ')' ) => self::makeInput( diff --git a/tests/oidc-auto-redirect.test.php b/tests/oidc-auto-redirect.test.php index 75625f9..b54c5d6 100644 --- a/tests/oidc-auto-redirect.test.php +++ b/tests/oidc-auto-redirect.test.php @@ -308,7 +308,7 @@ function methodBody($s, $needle) bad('OIDCLogout::providerLogout() is missing'); } else { if (false !== strpos($logoutListener, 'OIDCFlow::forcedProvider()') - && false !== strpos($logoutListener, 'OIDC::postLogoutUri()') + && false !== strpos($logoutListener, 'OIDC::localLoginUrl()') ) { ok('logging out of a forced-redirect install lands on the form'); } else { @@ -355,7 +355,7 @@ function methodBody($s, $needle) : substr($page, $labelAt, $inputAt - $labelAt); if ('' === $label) { bad('could not find the autoRedirect field on the management page'); -} elseif (false !== strpos($label, 'OIDC::postLogoutUri()')) { +} elseif (false !== strpos($label, 'OIDC::localLoginUrl()')) { ok('the management page prints the local login URL beside the setting'); } else { bad('the autoRedirect label no longer names the local login URL; the' diff --git a/tests/oidc-single-logout.test.php b/tests/oidc-single-logout.test.php index be131ac..2b33c59 100644 --- a/tests/oidc-single-logout.test.php +++ b/tests/oidc-single-logout.test.php @@ -279,16 +279,20 @@ function methodBody($s, $needle) . ' which session to end and prompts instead'); } /* - * login.php, not index.php. This is property 5 and it is the one that - * only bites once #17 exists -- at which point index.php is exactly the - * page that redirects back to the provider. + * The key as well as the value. Asserting only that postLogoutUri() is + * mentioned passes when it is bound to some other parameter name, and + * the provider then ends its session and leaves the person on its own + * page -- a logout that worked and looks broken. */ - if (false !== strpos($logoutUrl, 'OIDC::postLogoutUri()')) { - ok('it returns the browser to the local login page'); + if (false !== strpos( + $logoutUrl, + "'post_logout_redirect_uri'=>OIDC::postLogoutUri()," + )) { + ok('it returns the browser to FOG\'s login page'); } else { - bad('logoutUrl() no longer uses OIDC::postLogoutUri(); returning to' - . ' index.php on a forced-redirect install signs the user' - . ' straight back in or loops'); + bad('logoutUrl() no longer sends a post_logout_redirect_uri built' + . ' by OIDC::postLogoutUri(); the person ends the journey on' + . ' the provider\'s own page with no way back to FOG'); } /* * Single use. Left in place, a second pass through logout would build @@ -316,18 +320,46 @@ function methodBody($s, $needle) } } -if (false !== strpos($model, 'publicstaticfunctionpostLogoutUri()')) { - ok('OIDC::postLogoutUri() is defined'); -} else { +/* + * The two landings are different pages and the difference is the whole + * point, so pin both. + * + * postLogoutUri() -> the ORDINARY login page. Single logout has just run, + * so a forced-redirect install sending the browser back to the provider is + * correct: the provider now has no session and asks who you are, which is + * how you sign out and back in as somebody else. Pointing this at + * login.php instead strands the person on the break-glass page after a + * perfectly successful logout. + * + * localLoginUrl() -> the page no provider setting can redirect away from, + * used by _fail() and by the single-logout-off fallback, where bouncing to + * the provider really would loop or silently sign the person back in. + */ +$postLogout = methodBody($model, 'publicstaticfunctionpostLogoutUri()'); +if ('' === $postLogout) { bad('OIDC::postLogoutUri() is missing'); +} elseif (false !== strpos($postLogout, "'management/index.php'")) { + ok('postLogoutUri() names the ordinary login page'); +} else { + bad('postLogoutUri() no longer points at management/index.php; after a' + . ' successful single logout the person is stranded on whatever it' + . ' does name instead of being able to sign in again'); } -$postLogout = methodBody($model, 'publicstaticfunctionpostLogoutUri()'); -if (false !== strpos($postLogout, "'management/login.php'")) { - ok('postLogoutUri() names management/login.php'); + +$localLogin = methodBody($model, 'publicstaticfunctionlocalLoginUrl()'); +if ('' === $localLogin) { + bad('OIDC::localLoginUrl() is missing'); +} elseif (false !== strpos($localLogin, "'management/login.php'")) { + ok('localLoginUrl() names management/login.php'); } else { - bad('postLogoutUri() no longer points at management/login.php, the one' + bad('localLoginUrl() no longer points at management/login.php, the one' . ' page a forced-redirect install cannot bounce to the provider'); } +if ($postLogout === $localLogin && '' !== $postLogout) { + bad('postLogoutUri() and localLoginUrl() return the same thing; they' + . ' answer different questions and collapsing them re-creates' + . ' whichever bug the other one was avoiding'); +} /* * The callback URI must not move. It is registered at every provider by