diff --git a/oidc/class/oidc.class.php b/oidc/class/oidc.class.php index c5181cf..411a423 100644 --- a/oidc/class/oidc.class.php +++ b/oidc/class/oidc.class.php @@ -94,6 +94,10 @@ class OIDC extends FOGController // application behind that provider. See the note on the column in // OIDCManager::createSql(). 'singleLogout' => 'opSingleLogout', + // Send the login page straight to this provider instead of showing + // FOG's own form (#17). Off by default; see the note on the column + // in OIDCManager::createSql() for why this one in particular. + 'autoRedirect' => 'opAutoRedirect', 'icon' => 'opIcon' ]; /** @@ -301,6 +305,24 @@ public static function postLogoutUri() { return self::absoluteUrl('management/login.php'); } + /** + * The URL that begins a sign-in with one provider. + * + * Absolute, because core's LOGIN_PAGE_REDIRECT seam refuses anything + * that is not an absolute http(s) URL -- what a hook left in a variable + * is not a good enough answer for a Location header. The login-page + * button uses a relative form of the same path and does not need this. + * + * @param int $id the provider id + * + * @return string + */ + public static function startUrl($id) + { + return self::absoluteUrl( + sprintf('ext/oidc/start?provider=%d', (int)$id) + ); + } /** * An absolute https URL for a path inside this FOG install. * diff --git a/oidc/class/oidcflow.class.php b/oidc/class/oidcflow.class.php index 2ec2d3c..c6805aa 100644 --- a/oidc/class/oidcflow.class.php +++ b/oidc/class/oidcflow.class.php @@ -304,6 +304,79 @@ private static function _rememberLogout($provider, array $config, array $token) 'idToken' => (string)$token['id_token'] ]; } + /** + * The one provider the login page must redirect to, or 0 for none. + * + * Shared by the login-page listener and the logout listener, because + * both have to know the same thing: whether landing on + * management/index.php would bounce the visitor straight to a provider. + * + * TWO providers flagged is refused rather than resolved. The login page + * cannot redirect to both, and silently picking one -- lowest id, first + * row, whatever -- hides a misconfiguration on the single page an admin + * is least able to debug, while sending everybody to a provider half of + * them may not have an account at. Refusing renders FOG's own form, + * which is a working login for everyone and visibly not what was asked + * for. + * + * The complaint goes to the error log and NOT to the page. This runs + * for an anonymous visitor, and "this server has two misconfigured + * identity providers" is not something to tell one. + * + * @return int the provider id, or 0 + */ + public static function forcedProvider() + { + $ids = (array)Route::getIds( + 'oidc', + ['enabled' => [1], 'autoRedirect' => [1]] + ); + if (count($ids) < 1) { + return 0; + } + if (count($ids) > 1) { + error_log( + sprintf( + 'FOG OIDC: providers %s all have automatic redirect' + . ' enabled; the login page cannot redirect to more than' + . ' one, so it is showing the local form instead', + implode(', ', array_map('intval', $ids)) + ) + ); + return 0; + } + return (int)reset($ids); + } + /** + * Where the login page should send an anonymous visitor, or ''. + * + * Consumed by core's LOGIN_PAGE_REDIRECT seam (fogproject#1175), which + * fires only for a visitor who is NOT signed in and only on the form + * render -- so this can neither bounce a working session nor interrupt + * the callback coming back from the provider. + * + * The row is re-read and re-checked rather than trusted from the id, + * for the same reason _enabledProvider() re-checks at the start of every + * flow: a provider disabled a moment ago must not still be receiving + * people. + * + * @return string + */ + public static function loginRedirectUrl() + { + $id = self::forcedProvider(); + if ($id < 1) { + return ''; + } + $provider = self::getClass('OIDC', $id); + if (!$provider->isValid() + || '1' !== (string)$provider->get('enabled') + || '1' !== (string)$provider->get('autoRedirect') + ) { + return ''; + } + return OIDC::startUrl($id); + } /** * The provider logout URL for this session, or '' for none. * @@ -1224,6 +1297,15 @@ private static function _fail($message) { self::_session(); self::setMessage($message, _('Sign-in failed'), 'error'); - self::_redirect(OIDC::webrootBase() . 'management/index.php'); + /* + * login.php, not index.php. On an install with automatic redirect + * on (#17), index.php sends the visitor straight back to the + * provider that just refused them -- which is an infinite redirect + * for a provider that is down, and an unreadable flash message even + * when it is not, because nothing renders between the two hops. + * login.php always renders FOG's own form (fogproject#1175), so the + * explanation is attached to a page that stays put. + */ + self::_redirect(OIDC::webrootBase() . 'management/login.php'); } } diff --git a/oidc/class/oidcmanager.class.php b/oidc/class/oidcmanager.class.php index 62cf054..89f3036 100644 --- a/oidc/class/oidcmanager.class.php +++ b/oidc/class/oidcmanager.class.php @@ -53,6 +53,7 @@ public function createSql() 'opJITProvision', 'opAllowAPI', 'opSingleLogout', + 'opAutoRedirect', 'opIcon' ], [ @@ -71,6 +72,7 @@ public function createSql() "ENUM('0', '1')", "ENUM('0', '1')", "ENUM('0', '1')", + "ENUM('0', '1')", 'VARCHAR(255)' ], [ @@ -89,6 +91,7 @@ public function createSql() false, false, false, + false, false ], [ @@ -133,6 +136,14 @@ public function createSql() // session because somebody left FOG is a surprise that // reaches applications FOG has nothing to do with. "'0'", + // Sending everyone straight to this provider ships off, and + // it is the most dangerous switch in this plugin: an + // unconditional redirect to a provider that is unreachable, + // whose certificate expired, or whose issuer was mistyped + // takes the login form away from every administrator at + // once. management/login.php (fogproject#1175) is the way + // back, and the management page names it next to the box. + "'0'", "'fa fa-id-badge'" ], [ @@ -157,6 +168,7 @@ public function createSql() false, false, false, + false, false ], 'InnoDB', @@ -216,6 +228,14 @@ function () { // the column -- runs this harmlessly too. "ALTER TABLE `OIDCProviders` ADD COLUMN `opSingleLogout` " . "ENUM('0', '1') NOT NULL DEFAULT '0'", + // 7 - send the login page straight to this provider (#17). + // Appended for the same reason as step 6, and defaulting off for + // a sharper one: an install that upgraded into this switched ON + // would find its login form replaced by a redirect nobody asked + // for, and the only URL that still shows the form is one nobody + // has been told about yet. + "ALTER TABLE `OIDCProviders` ADD COLUMN `opAutoRedirect` " + . "ENUM('0', '1') NOT NULL DEFAULT '0'", ]; } /** diff --git a/oidc/hooks/oidcloginredirect.hook.php b/oidc/hooks/oidcloginredirect.hook.php new file mode 100644 index 0000000..03da966 --- /dev/null +++ b/oidc/hooks/oidcloginredirect.hook.php @@ -0,0 +1,95 @@ + + * @license http://opensource.org/licenses/gpl-3.0 GPLv3 + * @link https://fogproject.org + */ +/** + * Sends the login page straight to the identity provider. + * + * @category OIDCLoginRedirect + * @package FOGProject + * @author Tom Elliott + * @license http://opensource.org/licenses/gpl-3.0 GPLv3 + * @link https://fogproject.org + */ +class OIDCLoginRedirect extends Hook +{ + /** + * The name of this hook. + * + * @var string + */ + public $name = 'OIDCLoginRedirect'; + /** + * The description. + * + * @var string + */ + public $description = 'Send the login page to the identity provider.'; + /** + * For posterity. + * + * @var bool + */ + public $active = true; + /** + * The node to work with. + * + * @var string + */ + public $node = 'oidc'; + /** + * Initialize object. + * + * @return void + */ + public function __construct() + { + parent::__construct(); + $this->registerInstalled([ + ['LOGIN_PAGE_REDIRECT', 'loginRedirect'] + ]); + } + /** + * Where an anonymous visitor goes instead of FOG's login form. + * + * For an install where everyone signs in through one provider, landing + * on a username and password box is a dead end: the accounts are at the + * provider and the box cannot accept them. This is the setting that + * removes the extra click. + * + * It is also the most dangerous setting in this plugin, and the design + * of the seam is what contains it. Core only offers LOGIN_PAGE_REDIRECT + * when FOG_LOCAL_LOGIN is undefined, so on management/login.php this + * method is never reached -- not consulted and overruled, never asked. + * That is what makes the escape hatch survive a provider whose + * certificate expired, whose issuer was mistyped, or which is simply + * switched off; and it also means a bug in this method cannot take that + * page down, because the page does not run it. + * + * https:///fog/management/login.php + * + * A provider that refuses a sign-in sends the browser to that same page + * rather than back to index.php (OIDCFlow::_fail()), so a provider that + * is down produces one error message rather than a redirect loop. + * + * @param mixed $arguments where to send the browser instead + * + * @return void + */ + public function loginRedirect($arguments) + { + $url = OIDCFlow::loginRedirectUrl(); + if ('' === $url) { + return; + } + $arguments['redirect'] = $url; + } +} diff --git a/oidc/hooks/oidclogout.hook.php b/oidc/hooks/oidclogout.hook.php index f98be56..93336ce 100644 --- a/oidc/hooks/oidclogout.hook.php +++ b/oidc/hooks/oidclogout.hook.php @@ -84,6 +84,22 @@ public function __construct() public function providerLogout($arguments) { $url = OIDCFlow::logoutUrl(); + if ('' === $url && OIDCFlow::forcedProvider() > 0) { + /* + * No provider logout to do, but this install sends its login + * page straight to a provider (#17) -- so core's default + * landing spot, management/index.php, would bounce the person + * who just signed out back to a provider whose SSO session is + * still alive, and sign them silently back in. "Log out" that + * leaves you logged in is worse than no logout at all. + * + * management/login.php is the one page that cannot do that. + * 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. + */ + $url = OIDC::postLogoutUri(); + } if ('' === $url) { return; } diff --git a/oidc/js/fog.oidc.export.js b/oidc/js/fog.oidc.export.js index 3cd1054..eba5db3 100644 --- a/oidc/js/fog.oidc.export.js +++ b/oidc/js/fog.oidc.export.js @@ -22,6 +22,7 @@ {data: 'jitProvision', visible: false}, {data: 'allowapi', visible: false}, {data: 'singleLogout', visible: false}, + {data: 'autoRedirect', visible: false}, {data: 'icon', visible: false} ]); })(jQuery); diff --git a/oidc/pages/oidcmanagement.page.php b/oidc/pages/oidcmanagement.page.php index 59971e0..33a7a95 100644 --- a/oidc/pages/oidcmanagement.page.php +++ b/oidc/pages/oidcmanagement.page.php @@ -275,7 +275,8 @@ function (&$serverFault) { ->set('enabled', '0') ->set('jitProvision', '0') ->set('allowapi', '0') - ->set('singleLogout', '0'); + ->set('singleLogout', '0') + ->set('autoRedirect', '0'); if (!$OIDC->save()) { $serverFault = true; throw new \Exception(_('Add provider failed!')); @@ -498,6 +499,34 @@ public function oidcGeneral() // the provider's error page instead of back at FOG. That looks // like this plugin is broken, and the fix is a value an admin // has to copy from somewhere. + self::makeLabel( + $this->_labelClass, + 'autoRedirect', + _('Redirect Login To This Provider') + . '
(' + . sprintf( + // The escape hatch is named right here, on purpose. An + // admin who ticks this without knowing about login.php + // has one bad certificate between themselves and being + // 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()) . '' + ) + . ')' + ) => self::makeInput( + '', + 'autoRedirect', + '', + 'checkbox', + 'autoRedirect', + '', + false, + false, + -1, + -1, + $checked('autoRedirect') + ), self::makeLabel( $this->_labelClass, 'postLogoutUri', @@ -602,6 +631,10 @@ public function oidcGeneralPost() ->set( 'singleLogout', isset($_POST['singleLogout']) ? '1' : '0' + ) + ->set( + 'autoRedirect', + isset($_POST['autoRedirect']) ? '1' : '0' ); // The secret is only written when the admin actually typed one. An diff --git a/tests/oidc-auto-redirect.test.php b/tests/oidc-auto-redirect.test.php new file mode 100644 index 0000000..75625f9 --- /dev/null +++ b/tests/oidc-auto-redirect.test.php @@ -0,0 +1,373 @@ +'opAutoRedirect'")) { + ok('OIDC maps autoRedirect to opAutoRedirect'); +} else { + bad('OIDC no longer maps the autoRedirect field'); +} +if (false !== strpos($mgr, "'opAutoRedirect',")) { + ok('createSql() declares opAutoRedirect (fresh installs)'); +} else { + bad('createSql() no longer declares opAutoRedirect'); +} +if (false !== strpos( + $mgr, + '"ALTERTABLE`OIDCProviders`ADDCOLUMN`opAutoRedirect`"' +)) { + ok('a schema step adds opAutoRedirect (existing installs)'); +} else { + bad('no ALTER TABLE step adds opAutoRedirect; every install that' + . ' already passed the earlier steps keeps a table without it'); +} +/* + * Appended after the single-logout step, not before it and not inserted + * earlier. installdb() skips by COUNT, so reordering silently skips a step + * on every install that has already run some of them. + */ +$autoAt = strpos($mgr, 'ADDCOLUMN`opAutoRedirect`'); +$sloAt = strpos($mgr, 'ADDCOLUMN`opSingleLogout`'); +if (false !== $autoAt && false !== $sloAt && $autoAt > $sloAt) { + ok('the step is appended after the single-logout step'); +} else { + bad('the opAutoRedirect step is not last; installdb() skips by count,' + . ' so an inserted step silently skips a later one'); +} +/* + * Default off, and this is the one that matters most. On is a login form + * replaced by a redirect nobody asked for, on every install that upgrades. + */ +$alterAuto = substr($mgr, (int)$autoAt, 120); +if (false !== strpos($alterAuto, "DEFAULT'0'")) { + ok("the ALTER defaults to '0'"); +} else { + bad('the ALTER does not default opAutoRedirect off; upgrading would' + . ' redirect the login page on installs that never asked for it'); +} +/* + * And the create page must not hand a new provider the setting either. A + * provider is created switched off entirely. + */ +if (false !== strpos($page, "->set('autoRedirect','0')")) { + ok('a newly created provider has it off'); +} else { + bad('the create path no longer forces autoRedirect off; a provider' + . ' would be born redirecting the login page'); +} + +echo "\n2. two flagged providers refuse, and say so where a visitor cannot see\n"; + +$forced = methodBody($flow, 'publicstaticfunctionforcedProvider()'); +if ('' === $forced) { + bad('OIDCFlow::forcedProvider() is missing'); +} else { + if (false !== strpos($forced, "'autoRedirect'=>[1]") + && false !== strpos($forced, "'enabled'=>[1]") + ) { + ok('it selects on enabled AND autoRedirect'); + } else { + bad('forcedProvider() no longer requires both enabled and' + . ' autoRedirect; a provider an admin is still configuring' + . ' would receive every visitor'); + } + /* + * The refusal. Returning an id when more than one is flagged is the + * "silently pick one" behaviour that was argued down in #17, and it + * fails invisibly: the login page works, it just sends everybody to a + * provider that was never chosen. + */ + if (false !== strpos($forced, 'if(count($ids)>1){') + && false !== strpos($forced, 'return0;') + ) { + ok('more than one flagged provider refuses to redirect'); + } else { + bad('forcedProvider() no longer refuses when several providers are' + . ' flagged; it would silently pick one and hide the' + . ' misconfiguration'); + } + if (false !== strpos($forced, 'error_log(')) { + ok('the ambiguity is logged'); + } else { + bad('forcedProvider() no longer logs the ambiguity; the only' + . ' symptom would be a checkbox that appears not to work'); + } + /* + * Logged, NOT flashed. setMessage() here would tell an anonymous + * visitor how this server's identity providers are configured. + */ + if (false === strpos($forced, 'setMessage(')) { + ok('it does not tell the anonymous visitor'); + } else { + bad('forcedProvider() puts the misconfiguration on the page; this' + . ' runs for visitors who have not signed in'); + } +} + +echo "\n3. the seam is consumed, and the row re-checked\n"; + +$redirectUrl = methodBody($flow, 'publicstaticfunctionloginRedirectUrl()'); +if ('' === $redirectUrl) { + bad('OIDCFlow::loginRedirectUrl() is missing'); +} else { + if (false !== strpos($redirectUrl, "get('enabled')") + && false !== strpos($redirectUrl, "get('autoRedirect')") + ) { + ok('it re-reads the provider row before redirecting'); + } else { + bad('loginRedirectUrl() trusts the id without re-checking the row;' + . ' a provider disabled a moment ago would still be receiving' + . ' every visitor'); + } + if (false !== strpos($redirectUrl, 'OIDC::startUrl(')) { + ok('it redirects to the provider start URL'); + } else { + bad('loginRedirectUrl() no longer builds the start URL'); + } +} +/* + * Absolute. Core's seam refuses anything that is not an absolute http(s) + * URL, so a relative start path here means the setting silently does + * nothing at all -- the listener runs, sets a value, and core drops it. + */ +$startUrl = methodBody($model, 'publicstaticfunctionstartUrl('); +if (false !== strpos($startUrl, 'self::absoluteUrl(')) { + ok('startUrl() is absolute'); +} else { + bad('OIDC::startUrl() is not built through absoluteUrl(); core refuses' + . ' a relative LOGIN_PAGE_REDIRECT value and the setting would' + . ' silently do nothing'); +} + +if (false !== strpos($hook, "['LOGIN_PAGE_REDIRECT','loginRedirect']")) { + ok('OIDCLoginRedirect registers on LOGIN_PAGE_REDIRECT'); +} else { + bad('OIDCLoginRedirect no longer registers on LOGIN_PAGE_REDIRECT'); +} +if (false !== strpos($hook, 'registerInstalled(')) { + ok('it registers through registerInstalled()'); +} else { + bad('OIDCLoginRedirect registers unconditionally; a plugin present but' + . ' not installed must not redirect the login page'); +} +$listener = methodBody($hook, 'publicfunctionloginRedirect('); +$guardAt = strpos($listener, "if(''===\$url){return;}"); +$writeAt = strpos($listener, "\$arguments['redirect']="); +if (false !== $guardAt && false !== $writeAt && $guardAt < $writeAt) { + ok('it writes the redirect only when there is one'); +} else { + bad('OIDCLoginRedirect::loginRedirect() writes to $arguments even with' + . ' no URL, so it can clobber another listener'); +} + +echo "\n4. nothing bounces back into the redirect\n"; + +/* + * The two loops. A refused sign-in and a completed logout both used to + * land on management/index.php, which is precisely the page this feature + * redirects -- so on a forced-redirect install one is an infinite loop and + * the other signs the user straight back in. + */ +$failBody = methodBody($flow, 'privatestaticfunction_fail('); +if (false !== strpos($failBody, "'management/login.php'")) { + ok('a refused sign-in lands on the local login form'); +} else { + bad('_fail() still sends the browser to management/index.php; on a' + . ' forced-redirect install that is a redirect loop when the' + . ' provider is down, and an unread flash message when it is not'); +} + +$logoutListener = methodBody($out, 'publicfunctionproviderLogout('); +if ('' === $logoutListener) { + bad('OIDCLogout::providerLogout() is missing'); +} else { + if (false !== strpos($logoutListener, 'OIDCFlow::forcedProvider()') + && false !== strpos($logoutListener, 'OIDC::postLogoutUri()') + ) { + ok('logging out of a forced-redirect install lands on the form'); + } else { + bad('logging out with automatic redirect on but single logout off' + . ' still lands on management/index.php, which redirects to a' + . ' provider whose session is still alive and signs the user' + . ' straight back in'); + } + /* + * And only as a fallback. Overriding a real provider-logout URL with + * the local form would quietly disable single logout for exactly the + * installs most likely to want it. + */ + $sloAt2 = strpos($logoutListener, 'OIDCFlow::logoutUrl()'); + $fallbackAt = strpos($logoutListener, 'OIDCFlow::forcedProvider()'); + if (false !== $sloAt2 && false !== $fallbackAt && $sloAt2 < $fallbackAt) { + ok('the provider logout still wins when there is one'); + } else { + bad('the local-form fallback runs ahead of the provider logout URL,' + . ' disabling single logout'); + } +} + +echo "\n5. the escape hatch is named where the setting is turned on\n"; + +/* + * Not decoration. An admin who ticks this box without knowing login.php + * exists has one expired certificate between themselves and being locked + * out of their own server -- and at that point the URL is not something + * they could guess. + */ +/* + * Scoped to the autoRedirect label and nothing else. The page prints the + * same URL a second time in its own read-only Post-Logout Redirect URI + * field, so a whole-file search passes with the URL removed from exactly + * the place an admin reads it -- next to the box they are about to tick. + */ +$labelAt = strpos($page, "'autoRedirect',_("); +$inputAt = false === $labelAt + ? false + : strpos($page, '=>self::makeInput(', $labelAt); +$label = (false === $labelAt || false === $inputAt) + ? '' + : substr($page, $labelAt, $inputAt - $labelAt); +if ('' === $label) { + bad('could not find the autoRedirect field on the management page'); +} elseif (false !== strpos($label, 'OIDC::postLogoutUri()')) { + 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' + . ' escape hatch exists but the admin turning this on is not told' + . ' about it, and cannot guess it once locked out'); +} + +echo "\n"; +if ($fail > 0) { + echo "FAIL: $fail problem(s), $pass ok\n"; + exit(1); +} +echo "ok: $pass checks passed -- the login page can be sent to a provider," + . " and there is still a way back\n"; +exit(0);