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