From ba41a9e30154f5234a45062443548e909d60cd70 Mon Sep 17 00:00:00 2001 From: Kevin Bryan Date: Wed, 18 Jun 2025 11:15:04 -0400 Subject: [PATCH 1/4] Show an error message to the user when eppn is invalid --- resources/init.php | 11 ++++++++++- resources/lib/UnitySSO.php | 3 ++- resources/lib/exceptions/SSOException.php | 6 ++++++ 3 files changed, 18 insertions(+), 2 deletions(-) create mode 100644 resources/lib/exceptions/SSOException.php diff --git a/resources/init.php b/resources/init.php index f04625e3..b5f6521e 100644 --- a/resources/init.php +++ b/resources/init.php @@ -13,6 +13,8 @@ use UnityWebPortal\lib\UnityRedis; use UnityWebPortal\lib\UnityWebhook; use UnityWebPortal\lib\UnityGithub; +use UnityWebPortal\lib\UnitySite; +use UnityWebPortal\lib\exceptions\SSOException; // // Initialize Session @@ -92,7 +94,14 @@ // SSO Init // -$SSO = UnitySSO::getSSO(); +try { + $SSO = UnitySSO::getSSO(); +} catch (SSOException $e) { + $errorid = uniqid("sso-"); + $eppn = $_SERVER["REMOTE_USER"]; + UnitySite::errorLog("SSO Failure", "{$e} ($errorid)"); + UnitySite::die("Invalid eppn: '$eppn'. Please contact {$CONFIG["mail"]["support"]} (id: $errorid)", true); +} if (!is_null($SSO)) { // SSO is available $_SESSION["SSO"] = $SSO; diff --git a/resources/lib/UnitySSO.php b/resources/lib/UnitySSO.php index d2f71585..a5aac2b8 100644 --- a/resources/lib/UnitySSO.php +++ b/resources/lib/UnitySSO.php @@ -3,6 +3,7 @@ namespace UnityWebPortal\lib; use Exception; +use UnityWebPortal\lib\exceptions\SSOException; class UnitySSO { @@ -17,7 +18,7 @@ private static function eppnToOrg($eppn) { $parts = explode("@", $eppn); if (count($parts) != 2) { - throw new Exception("Malformed remote user detected: '$eppn'"); + throw new SSOException("Malformed remote user detected: '$eppn'"); } $org = $parts[1]; diff --git a/resources/lib/exceptions/SSOException.php b/resources/lib/exceptions/SSOException.php new file mode 100644 index 00000000..83b90e77 --- /dev/null +++ b/resources/lib/exceptions/SSOException.php @@ -0,0 +1,6 @@ + Date: Wed, 18 Jun 2025 16:23:49 -0400 Subject: [PATCH 2/4] refactor sso conditional --- resources/init.php | 19 +++++++++---------- resources/lib/UnitySSO.php | 22 ++++++++-------------- 2 files changed, 17 insertions(+), 24 deletions(-) diff --git a/resources/init.php b/resources/init.php index b5f6521e..b34df57a 100644 --- a/resources/init.php +++ b/resources/init.php @@ -94,16 +94,15 @@ // SSO Init // -try { - $SSO = UnitySSO::getSSO(); -} catch (SSOException $e) { - $errorid = uniqid("sso-"); - $eppn = $_SERVER["REMOTE_USER"]; - UnitySite::errorLog("SSO Failure", "{$e} ($errorid)"); - UnitySite::die("Invalid eppn: '$eppn'. Please contact {$CONFIG["mail"]["support"]} (id: $errorid)", true); -} -if (!is_null($SSO)) { - // SSO is available +if (isset($_SERVER["REMOTE_USER"])) { // Check if SSO is enabled on this page + try { + $SSO = UnitySSO::getSSO(); + } catch (SSOException $e) { + $errorid = uniqid("sso-"); + $eppn = $_SERVER["REMOTE_USER"]; + UnitySite::errorLog("SSO Failure", "{$e} ($errorid)"); + UnitySite::die("Invalid eppn: '$eppn'. Please contact {$CONFIG["mail"]["support"]} (id: $errorid)", true); + } $_SESSION["SSO"] = $SSO; $OPERATOR = new UnityUser($SSO["user"], $LDAP, $SQL, $MAILER, $REDIS, $WEBHOOK); diff --git a/resources/lib/UnitySSO.php b/resources/lib/UnitySSO.php index a5aac2b8..58a48405 100644 --- a/resources/lib/UnitySSO.php +++ b/resources/lib/UnitySSO.php @@ -28,19 +28,13 @@ private static function eppnToOrg($eppn) public static function getSSO() { - if (isset($_SERVER["REMOTE_USER"])) { // Check if SSO is enabled on this page - $SSO = array( - "user" => self::eppnToUID($_SERVER["REMOTE_USER"]), - "org" => self::eppnToOrg($_SERVER["REMOTE_USER"]), - "firstname" => $_SERVER["givenName"], - "lastname" => $_SERVER["sn"], - "name" => $_SERVER["givenName"] . " " . $_SERVER["sn"], - "mail" => isset($_SERVER["mail"]) ? $_SERVER["mail"] : $_SERVER["eppn"] - ); - - return $SSO; - } - - return null; + return array( + "user" => self::eppnToUID($_SERVER["REMOTE_USER"]), + "org" => self::eppnToOrg($_SERVER["REMOTE_USER"]), + "firstname" => $_SERVER["givenName"], + "lastname" => $_SERVER["sn"], + "name" => $_SERVER["givenName"] . " " . $_SERVER["sn"], + "mail" => isset($_SERVER["mail"]) ? $_SERVER["mail"] : $_SERVER["eppn"] + ); } } From 21a143d4e1ee2395426289353b9290fd7ecc0d96 Mon Sep 17 00:00:00 2001 From: Simon Leary Date: Wed, 18 Jun 2025 17:09:40 -0400 Subject: [PATCH 3/4] add tests --- test/functional/InvalidEPPNTest.php | 45 +++++++++++++++++++++++++++++ test/phpunit-bootstrap.php | 1 + 2 files changed, 46 insertions(+) create mode 100644 test/functional/InvalidEPPNTest.php diff --git a/test/functional/InvalidEPPNTest.php b/test/functional/InvalidEPPNTest.php new file mode 100644 index 00000000..7813b927 --- /dev/null +++ b/test/functional/InvalidEPPNTest.php @@ -0,0 +1,45 @@ +expectException(PhpUnitNoDieException::class); + $this->expectExceptionMessageMatches("/.*Invalid eppn.*/"); + } + try { + $_SERVER["REMOTE_USER"] = $eppn; + $_SERVER["REMOTE_ADDR"] = "127.0.0.1"; + $_SERVER["eppn"] = $eppn; + $_SERVER["givenName"] = "foo"; + $_SERVER["sn"] = "bar"; + // can't use http_get because it does `require_once` + // won't use phpunit --process-isolation because when I try that argument all tests fail with a blank error message + include __DIR__ . "/../../resources/init.php"; + } finally { + session_destroy(); // autoload does session_start() + $_SERVER = $original_server; + $SSO = $original_sso; + } + $this->assertTrue(true); // if $is_valid, there are no other assertions + } +} diff --git a/test/phpunit-bootstrap.php b/test/phpunit-bootstrap.php index 2240c7f6..be7fdd81 100644 --- a/test/phpunit-bootstrap.php +++ b/test/phpunit-bootstrap.php @@ -19,6 +19,7 @@ require_once __DIR__ . "/../resources/lib/UnityRedis.php"; require_once __DIR__ . "/../resources/lib/UnityGithub.php"; require_once __DIR__ . "/../resources/lib/exceptions/PhpUnitNoDieException.php"; +require_once __DIR__ . "/../resources/lib/exceptions/SSOException.php"; $GLOBALS["PHPUNIT_NO_DIE_PLEASE"] = true; From d13227f09e7e3c46bcba1ae8d91f78481c8029e6 Mon Sep 17 00:00:00 2001 From: Simon Leary Date: Wed, 18 Jun 2025 17:29:35 -0400 Subject: [PATCH 4/4] be gentle with sessions --- test/functional/InvalidEPPNTest.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/functional/InvalidEPPNTest.php b/test/functional/InvalidEPPNTest.php index 7813b927..658a88e8 100644 --- a/test/functional/InvalidEPPNTest.php +++ b/test/functional/InvalidEPPNTest.php @@ -22,6 +22,10 @@ public function testInitGetSSO(string $eppn, bool $is_valid): void global $SSO; $original_server = $_SERVER; $original_sso = $SSO; + if (session_status() == PHP_SESSION_ACTIVE) { + session_write_close(); + session_id(uniqid()); + } if (!$is_valid) { $this->expectException(PhpUnitNoDieException::class); $this->expectExceptionMessageMatches("/.*Invalid eppn.*/"); @@ -36,7 +40,6 @@ public function testInitGetSSO(string $eppn, bool $is_valid): void // won't use phpunit --process-isolation because when I try that argument all tests fail with a blank error message include __DIR__ . "/../../resources/init.php"; } finally { - session_destroy(); // autoload does session_start() $_SERVER = $original_server; $SSO = $original_sso; }