From f0afa80e2d846c2e18733c0aeb0493bd9f921bfd Mon Sep 17 00:00:00 2001 From: Simon Leary Date: Sat, 7 Jun 2025 10:54:26 -0400 Subject: [PATCH 01/25] prevent access to user attributes when they might not be defined --- resources/lib/UnityUser.php | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/resources/lib/UnityUser.php b/resources/lib/UnityUser.php index 04c412b..6523095 100644 --- a/resources/lib/UnityUser.php +++ b/resources/lib/UnityUser.php @@ -176,6 +176,7 @@ public function exists() */ public function getUID() { + assert($this->exists()); return $this->uid; } @@ -189,6 +190,7 @@ public function setOrg($org) public function getOrg($ignorecache = false) { + assert($this->exists()); if (!$ignorecache) { $cached_val = $this->REDIS->getCache($this->getUID(), "org"); if (!is_null($cached_val)) { @@ -238,6 +240,7 @@ public function setFirstname($firstname, $operator = null) */ public function getFirstname($ignorecache = false) { + assert($this->exists()); if (!$ignorecache) { $cached_val = $this->REDIS->getCache($this->getUID(), "firstname"); if (!is_null($cached_val)) { @@ -287,6 +290,7 @@ public function setLastname($lastname, $operator = null) */ public function getLastname($ignorecache = false) { + assert($this->exists()); if (!$ignorecache) { $cached_val = $this->REDIS->getCache($this->getUID(), "lastname"); if (!is_null($cached_val)) { @@ -309,6 +313,7 @@ public function getLastname($ignorecache = false) public function getFullname() { + assert($this->exists()); return $this->getFirstname() . " " . $this->getLastname(); } @@ -341,6 +346,7 @@ public function setMail($email, $operator = null) */ public function getMail($ignorecache = false) { + assert($this->exists()); if (!$ignorecache) { $cached_val = $this->REDIS->getCache($this->getUID(), "mail"); if (!is_null($cached_val)) { @@ -404,6 +410,7 @@ public function setSSHKeys($keys, $operator = null, $send_mail = true) */ public function getSSHKeys($ignorecache = false) { + assert($this->exists()); if (!$ignorecache) { $cached_val = $this->REDIS->getCache($this->getUID(), "sshkeys"); if (!is_null($cached_val)) { @@ -480,6 +487,7 @@ public function setLoginShell($shell, $operator = null, $send_mail = true) */ public function getLoginShell($ignorecache = false) { + assert($this->exists()); if (!$ignorecache) { $cached_val = $this->REDIS->getCache($this->getUID(), "loginshell"); if (!is_null($cached_val)) { @@ -528,6 +536,7 @@ public function setHomeDir($home, $operator = null) */ public function getHomeDir($ignorecache = false) { + assert($this->exists()); if (!$ignorecache) { $cached_val = $this->REDIS->getCache($this->getUID(), "homedir"); if (!is_null($cached_val)) { From 39f18c98da385d6e87fd1ffe1b963ae50a8e1b49 Mon Sep 17 00:00:00 2001 From: Simon Leary Date: Sat, 7 Jun 2025 11:19:00 -0400 Subject: [PATCH 02/25] getuid without existing is OK --- resources/lib/UnityUser.php | 1 - 1 file changed, 1 deletion(-) diff --git a/resources/lib/UnityUser.php b/resources/lib/UnityUser.php index 6523095..ebe214b 100644 --- a/resources/lib/UnityUser.php +++ b/resources/lib/UnityUser.php @@ -176,7 +176,6 @@ public function exists() */ public function getUID() { - assert($this->exists()); return $this->uid; } From 5b966cbef46a31f641a1801d1a2b94c20177ffe5 Mon Sep 17 00:00:00 2001 From: Simon Leary Date: Sat, 7 Jun 2025 11:31:44 -0400 Subject: [PATCH 03/25] store attributes in request --- resources/lib/UnityGroup.php | 95 +++++++++++++++--------- resources/lib/UnitySQL.php | 28 +++++-- resources/lib/UnityUser.php | 19 ++--- tools/docker-dev/sql/bootstrap.sql | 4 + webroot/admin/ajax/get_group_members.php | 15 ++-- webroot/admin/pi-mgmt.php | 12 +-- webroot/panel/account.php | 22 +++++- webroot/panel/groups.php | 17 ++++- webroot/panel/new_account.php | 22 +++++- webroot/panel/pi.php | 18 +++-- 10 files changed, 174 insertions(+), 78 deletions(-) diff --git a/resources/lib/UnityGroup.php b/resources/lib/UnityGroup.php index 2acfc0a..23185e2 100644 --- a/resources/lib/UnityGroup.php +++ b/resources/lib/UnityGroup.php @@ -76,7 +76,7 @@ public function exists() // Portal-facing methods, these are the methods called by scripts in webroot // - public function requestGroup($send_mail_to_admins, $send_mail = true) + public function requestGroup($firstname, $lastname, $email, $org, $send_mail_to_admins, $send_mail = true) { // check for edge cases... if ($this->exists()) { @@ -88,7 +88,7 @@ public function requestGroup($send_mail_to_admins, $send_mail = true) return; } - $this->SQL->addRequest($this->getOwner()->getUID()); + $this->SQL->addRequest($this->getOwner()->getUID(), $firstname, $lastname, $email, $org); if ($send_mail) { // send email to requestor @@ -101,9 +101,9 @@ public function requestGroup($send_mail_to_admins, $send_mail = true) "group_request_admin", array( "user" => $this->getOwner()->getUID(), - "org" => $this->getOwner()->getOrg(), - "name" => $this->getOwner()->getFullname(), - "email" => $this->getOwner()->getMail() + "org" => $org, + "name" => "$firstname $lastname", + "email" => $email ) ); @@ -113,9 +113,9 @@ public function requestGroup($send_mail_to_admins, $send_mail = true) "group_request_admin", array( "user" => $this->getOwner()->getUID(), - "org" => $this->getOwner()->getOrg(), - "name" => $this->getOwner()->getFullname(), - "email" => $this->getOwner()->getMail() + "org" => $org, + "name" => "$firstname $lastname", + "email" => $email ) ); } @@ -125,9 +125,9 @@ public function requestGroup($send_mail_to_admins, $send_mail = true) "group_request_admin", array( "user" => $this->getOwner()->getUID(), - "org" => $this->getOwner()->getOrg(), - "name" => $this->getOwner()->getFullname(), - "email" => $this->getOwner()->getMail() + "org" => $org, + "name" => "$firstname $lastname", + "email" => $email ) ); } @@ -138,10 +138,11 @@ public function requestGroup($send_mail_to_admins, $send_mail = true) */ public function approveGroup($operator = null, $send_mail = true) { - if (!$this->SQL->requestExists($this->getOwner()->getUID())) { - throw new Exception( - "attempt to approve nonexistent request for group='{$this->getPIUID()}' uid='$new_user'" - ); + $uid = $this->getOwner()->getUID(); + $gid = $this->getPIUID(); + $request = $this->SQL->getRequest($uid, $gid); + if (is_null($request)) { + throw new Exception("uid '$uid' does not have a group request!"); } // check for edge cases... @@ -151,7 +152,13 @@ public function approveGroup($operator = null, $send_mail = true) // check if owner exists if (!$this->getOwner()->exists()) { - $this->getOwner()->init(); + $this->getOwner()->init( + $request["firstname"], + $request["lastname"], + $request["email"], + $request["org"], + $send_mail + ); } // initialize ldap objects, if this fails the script will crash, but nothing will persistently break @@ -288,15 +295,22 @@ public function cancelGroupJoinRequest($user, $send_mail = true) */ public function approveUser($new_user, $send_mail = true) { - if (!$this->requestExists($new_user)) { - throw new Exception( - "attempt to approve nonexistent request for group='{$this->getPIUID()}' uid='$new_user'" - ); + + $uid = $new_user->getUID(); + $gid = $this->getPIUID(); + $request = $this->SQL->getRequest($uid, $gid); + if (is_null($request)) { + throw new Exception("uid '$uid' does not have a request for group '$gid'!"); } // check if user exists if (!$new_user->exists()) { - $new_user->init(); + $new_user->init( + $request["firstname"], + $request["lastname"], + $request["email"], + $request["org"], + ); } // add user to the LDAP object @@ -320,9 +334,9 @@ public function approveUser($new_user, $send_mail = true) array( "group" => $this->pi_uid, "user" => $new_user->getUID(), - "name" => $new_user->getFullName(), - "email" => $new_user->getMail(), - "org" => $new_user->getOrg() + "name" => $request["firstname"] . " " . $request["lastname"], + "email" => $request["email"], + "org" => $request["org"], ) ); } @@ -330,8 +344,11 @@ public function approveUser($new_user, $send_mail = true) public function denyUser($new_user, $send_mail = true) { - if (!$this->requestExists($new_user)) { - return; + $uid = $new_user->getUID(); + $gid = $this->getPIUID(); + $request = $this->SQL->getRequest($uid, $gid); + if (is_null($request)) { + throw new Exception("uid '$uid' does not have a request for group '$gid'!"); } // remove request, this will fail silently if the request doesn't exist @@ -396,7 +413,7 @@ public function removeUser($new_user, $send_mail = true) } } - public function newUserRequest($new_user, $send_mail = true) + public function newUserRequest($new_user, $firstname, $lastname, $email, $org, $send_mail = true) { if ($this->userExists($new_user)) { UnitySite::errorLog("warning", "user '$new_user' already in group"); @@ -413,7 +430,7 @@ public function newUserRequest($new_user, $send_mail = true) return; } - $this->addRequest($new_user->getUID()); + $this->addRequest($new_user->getUID(), $firstname, $lastname, $email, $org); if ($send_mail) { // send email to user @@ -430,9 +447,9 @@ public function newUserRequest($new_user, $send_mail = true) array( "group" => $this->pi_uid, "user" => $new_user->getUID(), - "name" => $new_user->getFullName(), - "email" => $new_user->getMail(), - "org" => $new_user->getOrg() + "name" => "$firstname $lastname", + "email" => $email, + "org" => $org, ) ); } @@ -452,7 +469,17 @@ public function getRequests() $this->REDIS, $this->WEBHOOK ); - array_push($out, [$user, $request["timestamp"]]); + array_push( + $out, + [ + $user, + $request["timestamp"], + $request["firstname"], + $request["lastname"], + $request["email"], + $request["org"], + ] + ); } return $out; @@ -563,9 +590,9 @@ public function userExists($user) return in_array($user->getUID(), $this->getGroupMemberUIDs()); } - private function addRequest($uid) + private function addRequest($uid, $firstname, $lastname, $email, $org) { - $this->SQL->addRequest($uid, $this->pi_uid); + $this->SQL->addRequest($uid, $firstname, $lastname, $email, $org, $this->pi_uid); } // diff --git a/resources/lib/UnitySQL.php b/resources/lib/UnitySQL.php index d9be2ca..b94178f 100644 --- a/resources/lib/UnitySQL.php +++ b/resources/lib/UnitySQL.php @@ -38,17 +38,23 @@ public function getConn() // // requests table methods // - public function addRequest($requestor, $dest = self::REQUEST_BECOME_PI) + public function addRequest($requestor, $firstname, $lastname, $email, $org, $dest = self::REQUEST_BECOME_PI) { if ($this->requestExists($requestor, $dest)) { return; } $stmt = $this->conn->prepare( - "INSERT INTO " . self::TABLE_REQS . " (uid, request_for) VALUES (:uid, :request_for)" + "INSERT INTO " . self::TABLE_REQS . " " . + "(uid, firstname, lastname, email, org, request_for) VALUES " . + "(:uid, :firstname, :lastname, :email, :org, :request_for)" ); $stmt->bindParam(":uid", $requestor); $stmt->bindParam(":request_for", $dest); + $stmt->bindParam(":firstname", $firstname); + $stmt->bindParam(":lastname", $lastname); + $stmt->bindParam(":email", $email); + $stmt->bindParam(":org", $org); $stmt->execute(); } @@ -78,17 +84,27 @@ public function removeRequests($dest = self::REQUEST_BECOME_PI) $stmt->execute(); } - public function requestExists($requestor, $dest = self::REQUEST_BECOME_PI) + public function getRequest($user, $dest) { $stmt = $this->conn->prepare( "SELECT * FROM " . self::TABLE_REQS . " WHERE uid=:uid and request_for=:request_for" ); - $stmt->bindParam(":uid", $requestor); + $stmt->bindParam(":uid", $user); $stmt->bindParam(":request_for", $dest); - $stmt->execute(); + $result = $stmt->fetchAll(); + if (count($result) == 0) { + throw new Exception("no such request: uid='$user' request_for='$dest'"); + } + if (count($result) > 1) { + throw new Exception("too many requests for uid='$user' request_for='$dest'"); + } + return $result[0]; + } - return count($stmt->fetchAll()) > 0; + public function requestExists($requestor, $dest = self::REQUEST_BECOME_PI) + { + return (!is_null(self::getRequest($requestor, $dest))); } public function getRequests($dest = self::REQUEST_BECOME_PI) diff --git a/resources/lib/UnityUser.php b/resources/lib/UnityUser.php index ebe214b..32f752f 100644 --- a/resources/lib/UnityUser.php +++ b/resources/lib/UnityUser.php @@ -49,10 +49,11 @@ public function __toString() * @param string $firstname First name of new account * @param string $lastname Last name of new account * @param string $email email of new account + * @param string $org organization name of new account * @param bool $isPI boolean value for if the user checked the "I am a PI box" * @return void */ - public function init($send_mail = true) + public function init($firstname, $lastname, $email, $org, $send_mail = true) { // // Create LDAP group @@ -74,14 +75,14 @@ public function init($send_mail = true) if (!$ldapUserEntry->exists()) { $ldapUserEntry->setAttribute("objectclass", UnityLDAP::POSIX_ACCOUNT_CLASS); $ldapUserEntry->setAttribute("uid", $this->uid); - $ldapUserEntry->setAttribute("givenname", $this->getFirstname()); - $ldapUserEntry->setAttribute("sn", $this->getLastname()); + $ldapUserEntry->setAttribute("givenname", $firstname); + $ldapUserEntry->setAttribute("sn", $lastname); $ldapUserEntry->setAttribute( "gecos", \transliterator_transliterate("Latin-ASCII", "{$this->getFirstname()} {$this->getLastname()}") ); - $ldapUserEntry->setAttribute("mail", $this->getMail()); - $ldapUserEntry->setAttribute("o", $this->getOrg()); + $ldapUserEntry->setAttribute("mail", $email); + $ldapUserEntry->setAttribute("o", $org); $ldapUserEntry->setAttribute("homedirectory", self::HOME_DIR . $this->uid); $ldapUserEntry->setAttribute("loginshell", $this->LDAP->getDefUserShell()); $ldapUserEntry->setAttribute("uidnumber", strval($id)); @@ -90,10 +91,10 @@ public function init($send_mail = true) } // update cache - //$this->REDIS->setCache($this->uid, "firstname", $this->getFirstname()); - //$this->REDIS->setCache($this->uid, "lastname", $this->getLastname()); - //$this->REDIS->setCache($this->uid, "mail", $this->getMail()); - //$this->REDIS->setCache($this->uid, "org", $this->getOrg()); + $this->REDIS->setCache($this->uid, "firstname", $firstname); + $this->REDIS->setCache($this->uid, "lastname", $lastname); + $this->REDIS->setCache($this->uid, "mail", $email); + $this->REDIS->setCache($this->uid, "org", $org); $this->REDIS->setCache($this->uid, "homedir", self::HOME_DIR . $this->uid); $this->REDIS->setCache($this->uid, "loginshell", $this->LDAP->getDefUserShell()); $this->REDIS->setCache($this->uid, "sshkeys", array()); diff --git a/tools/docker-dev/sql/bootstrap.sql b/tools/docker-dev/sql/bootstrap.sql index ab0aab8..bfb5cbf 100644 --- a/tools/docker-dev/sql/bootstrap.sql +++ b/tools/docker-dev/sql/bootstrap.sql @@ -173,6 +173,10 @@ CREATE TABLE `requests` ( `id` int(11) NOT NULL, `request_for` varchar(131) NOT NULL, `uid` varchar(128) NOT NULL, + `firstname` varchar(768) NOT NULL, + `lastname` varchar(768) NOT NULL, + `email` varchar(768) NOT NULL, + `org` varchar(768) NOT NULL, `timestamp` timestamp NOT NULL DEFAULT current_timestamp() ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci; diff --git a/webroot/admin/ajax/get_group_members.php b/webroot/admin/ajax/get_group_members.php index ba126ce..5334d4a 100644 --- a/webroot/admin/ajax/get_group_members.php +++ b/webroot/admin/ajax/get_group_members.php @@ -48,23 +48,22 @@ $key++; } -foreach ($requests as $key => $request) { +foreach ($requests as $key => [$user, $timestamp, $firstname, $lastname, $email, $org]) { if ($key >= $count - 1) { echo ""; } else { echo ""; } - - [$request, $timestamp] = $request; - echo "" . $request->getFirstname() . " " . $request->getLastname() . ""; - echo "" . $request->getUID() . ""; - echo "" . $request->getMail() . ""; + $uid = $user->getUID(); + echo "" . $firstname . " " . $lastname . ""; + echo "" . $uid . ""; + echo "" . $email . ""; echo ""; echo "
+ onsubmit='return confirm(\"Are you sure you want to approve " . $uid . "?\");'> - +
"; diff --git a/webroot/admin/pi-mgmt.php b/webroot/admin/pi-mgmt.php index 24e09d6..f5ba919 100644 --- a/webroot/admin/pi-mgmt.php +++ b/webroot/admin/pi-mgmt.php @@ -77,19 +77,19 @@ $request_user = new UnityUser($request["uid"], $LDAP, $SQL, $MAILER, $REDIS, $WEBHOOK); echo ""; - echo "" . $request_user->getFirstname() . " " . $request_user->getLastname() . ""; - echo "" . $request_user->getUID() . ""; - echo "" . $request_user->getMail() . ""; + echo "" . $request["firstname"] . " " . $request["lastname"] . ""; + echo "" . $request["uid"] . ""; + echo "" . $request["mail"] . ""; echo "" . date("jS F, Y", strtotime($request['timestamp'])) . ""; echo ""; echo "
- + + onclick='return confirm(\"Are you sure you want to approve " . $request["uid"] . "?\");'> + onclick='return confirm(\"Are you sure you want to deny " . $request["uid"] . "?\");'>
"; echo ""; echo ""; diff --git a/webroot/panel/account.php b/webroot/panel/account.php index 14047f3..9c41437 100644 --- a/webroot/panel/account.php +++ b/webroot/panel/account.php @@ -54,11 +54,25 @@ $USER->setLoginShell($_POST["shellSelect"], $OPERATOR); break; case "pi_request": - if (!$USER->isPI()) { - if (!$SQL->requestExists($USER->getUID())) { - $USER->getPIGroup()->requestGroup($SEND_PIMESG_TO_ADMINS); - } + if ($USER->isPI()) { + UnitySite::badRequest("already a PI"); + } + if ($SQL->requestExists($USER->getUID())) { + UnitySite::badRequest("already requested to be PI"); + } + if ($USER->getUID() != $SSO["user"]) { + UnitySite::badRequest( + "cannot request due to uid mismatch: " . + "USER='{$USER->getUID()}' SSO[user]='$sso_user'" + ); } + $USER->getPIGroup()->requestGroup( + $SSO["firstname"], + $SSO["lastname"], + $SSO["email"], + $SSO["org"], + $SEND_PIMESG_TO_ADMINS + ); break; case "account_deletion_request": $hasGroups = count($USER->getGroups()) > 0; diff --git a/webroot/panel/groups.php b/webroot/panel/groups.php index 916acb9..069d61f 100644 --- a/webroot/panel/groups.php +++ b/webroot/panel/groups.php @@ -31,9 +31,23 @@ array_push($modalErrors, "You\'re already in this PI group"); } + if ($USER->getUID() != $SSO["user"]) { + $sso_user = $SSO["user"]; + UnitySite::errorLog( + "warning", + "cannot request due to uid mismatch: " . + "USER='{$USER->getUID()}' SSO[user]='$sso_user'" + ); + array_push( + $modalErrors, + "The requesting user must be the one signed in with SSO. " . + "Are you an admin viewing as another user?" + ); + } + // Add row to sql if (empty($modalErrors)) { - $pi_account->newUserRequest($USER); + $pi_account->newUserRequest($USER, $SSO["firstname"], $SSO["lastname"], $SSO["mail"], $SSO["org "]); } break; case "removePIForm": @@ -160,6 +174,7 @@ } ?> + // tables.js uses ajax_url to populate expandable tables var ajax_url = "/panel/ajax/get_group_members.php?pi_uid="; diff --git a/webroot/panel/new_account.php b/webroot/panel/new_account.php index 4c83102..0ab8f7d 100644 --- a/webroot/panel/new_account.php +++ b/webroot/panel/new_account.php @@ -17,18 +17,36 @@ if (!isset($_POST["eula"]) || $_POST["eula"] != "agree") { UnitySite::badRequest("user did not agree to EULA"); } + if ($USER->getUID() != $SSO["user"]) { + $sso_user = $SSO["user"]; + UnitySite::badRequest( + "cannot request due to uid mismatch: USER='{$USER->getUID()}' SSO[user]='$sso_user'" + ); + } if ($_POST["new_user_sel"] == "not_pi") { $form_group = new UnityGroup($_POST["pi"], $LDAP, $SQL, $MAILER, $REDIS, $WEBHOOK); if (!$form_group->exists()) { UnitySite::badRequest("The selected PI does not exist"); } - $form_group->newUserRequest($USER); + $form_group->newUserRequest( + $USER, + $SSO["firstname"], + $SSO["lastname"], + $SSO["mail"], + $SSO["org"] + ); } if ($_POST["new_user_sel"] == "pi") { if (!isset($_POST["confirm_pi"]) || $_POST["confirm_pi"] != "agree") { UnitySite::badRequest("user did not agree to account policy"); } - $USER->getPIGroup()->requestGroup($SEND_PIMESG_TO_ADMINS); + $USER->getPIGroup()->requestGroup( + $SSO["firstname"], + $SSO["lastname"], + $SSO["mail"], + $SSO["org"], + $SEND_PIMESG_TO_ADMINS + ); } } elseif (isset($_POST["cancel"])) { foreach ($pending_requests as $request) { diff --git a/webroot/panel/pi.php b/webroot/panel/pi.php index 8cafa81..f86b43f 100644 --- a/webroot/panel/pi.php +++ b/webroot/panel/pi.php @@ -53,21 +53,23 @@ echo "
Pending Requests
"; echo ""; - foreach ($requests as $request) { + foreach ($requests as [$user, $timestamp, $firstname, $lastname, $email, $org]) { + $uid = $user->getUID(); + $date = date("jS F, Y", strtotime($timestamp)); echo ""; - echo ""; - echo ""; - echo ""; - echo ""; + echo ""; + echo ""; + echo ""; + echo ""; echo ""; echo ""; From f6d5442ba09cee61f87fee5d53b6ffa961cc9364 Mon Sep 17 00:00:00 2001 From: Simon Leary Date: Sat, 7 Jun 2025 11:51:36 -0400 Subject: [PATCH 04/25] fix UnitySQL::requestExists --- resources/autoload.php | 2 ++ resources/lib/UnitySQL.php | 11 +++++++++-- .../lib/exceptions/UnitySQLNoSuchRequestException.php | 6 ++++++ 3 files changed, 17 insertions(+), 2 deletions(-) create mode 100644 resources/lib/exceptions/UnitySQLNoSuchRequestException.php diff --git a/resources/autoload.php b/resources/autoload.php index 5c0c25c..8875b20 100644 --- a/resources/autoload.php +++ b/resources/autoload.php @@ -24,6 +24,8 @@ require_once __DIR__ . "/lib/UnityWebhook.php"; require_once __DIR__ . "/lib/UnityRedis.php"; require_once __DIR__ . "/lib/UnityGithub.php"; +require_once __DIR__ . "/lib/exceptions/PhpUnitNoDieException.php"; +require_once __DIR__ . "/lib/exceptions/UnitySQLNoSuchRequestException.php"; // run init script require __DIR__ . "/init.php"; diff --git a/resources/lib/UnitySQL.php b/resources/lib/UnitySQL.php index b94178f..d07de03 100644 --- a/resources/lib/UnitySQL.php +++ b/resources/lib/UnitySQL.php @@ -3,6 +3,8 @@ namespace UnityWebPortal\lib; use PDO; +use Exception; +use UnityWebPortal\lib\exceptions\UnitySQLNoSuchRequestException; class UnitySQL { @@ -94,7 +96,7 @@ public function getRequest($user, $dest) $stmt->execute(); $result = $stmt->fetchAll(); if (count($result) == 0) { - throw new Exception("no such request: uid='$user' request_for='$dest'"); + throw new UnitySQLNoSuchRequestException("no such request: uid='$user' request_for='$dest'"); } if (count($result) > 1) { throw new Exception("too many requests for uid='$user' request_for='$dest'"); @@ -104,7 +106,12 @@ public function getRequest($user, $dest) public function requestExists($requestor, $dest = self::REQUEST_BECOME_PI) { - return (!is_null(self::getRequest($requestor, $dest))); + try { + self::getRequest($requestor, $dest); + return true; + } catch (UnitySQLNoSuchRequestException) { + return false; + } } public function getRequests($dest = self::REQUEST_BECOME_PI) diff --git a/resources/lib/exceptions/UnitySQLNoSuchRequestException.php b/resources/lib/exceptions/UnitySQLNoSuchRequestException.php new file mode 100644 index 0000000..45755c6 --- /dev/null +++ b/resources/lib/exceptions/UnitySQLNoSuchRequestException.php @@ -0,0 +1,6 @@ + Date: Sat, 7 Jun 2025 14:06:17 -0400 Subject: [PATCH 05/25] email -> mail --- webroot/panel/account.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/webroot/panel/account.php b/webroot/panel/account.php index 9c41437..6144983 100644 --- a/webroot/panel/account.php +++ b/webroot/panel/account.php @@ -69,7 +69,7 @@ $USER->getPIGroup()->requestGroup( $SSO["firstname"], $SSO["lastname"], - $SSO["email"], + $SSO["mail"], $SSO["org"], $SEND_PIMESG_TO_ADMINS ); From 10a60e8c0489ca508637cb9f4ff55f0e03d93b84 Mon Sep 17 00:00:00 2001 From: Simon Leary Date: Sat, 7 Jun 2025 14:06:22 -0400 Subject: [PATCH 06/25] fix addRequest --- resources/lib/UnitySQL.php | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/resources/lib/UnitySQL.php b/resources/lib/UnitySQL.php index 2dbf48e..2d0e452 100644 --- a/resources/lib/UnitySQL.php +++ b/resources/lib/UnitySQL.php @@ -106,12 +106,28 @@ private function update($table, $filters, $data) $stmt->execute(); } - public function addRequest($requestor, $firstname, $lastname, $email, $org, $dest = self::REQUEST_BECOME_PI) - { + public function addRequest( + $requestor, + $firstname, + $lastname, + $email, + $org, + $dest = self::REQUEST_BECOME_PI + ) { if ($this->requestExists($requestor, $dest)) { return; } - $this->insert(self::TABLE_REQS, ["uid" => $requestor, "request_for" => $dest]); + $this->insert( + self::TABLE_REQS, + [ + "uid" => $requestor, + "firstname" => $firstname, + "lastname" => $lastname, + "email" => $email, + "org" => $org, + "request_for" => $dest + ] + ); } public function removeRequest($requestor, $dest = self::REQUEST_BECOME_PI) From c6a4ad7e5b5b86f706148d79ccc18ed83b3cf731 Mon Sep 17 00:00:00 2001 From: Simon Leary Date: Sat, 7 Jun 2025 14:58:49 -0400 Subject: [PATCH 07/25] use new vars --- resources/lib/UnityGroup.php | 4 ++-- resources/lib/UnityUser.php | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/resources/lib/UnityGroup.php b/resources/lib/UnityGroup.php index 23185e2..1039253 100644 --- a/resources/lib/UnityGroup.php +++ b/resources/lib/UnityGroup.php @@ -93,7 +93,7 @@ public function requestGroup($firstname, $lastname, $email, $org, $send_mail_to_ if ($send_mail) { // send email to requestor $this->MAILER->sendMail( - $this->getOwner()->getMail(), + $email, "group_request" ); @@ -180,7 +180,7 @@ public function approveGroup($operator = null, $send_mail = true) // send email to the newly approved PI if ($send_mail) { $this->MAILER->sendMail( - $this->getOwner()->getMail(), + $request["email"], "group_created" ); } diff --git a/resources/lib/UnityUser.php b/resources/lib/UnityUser.php index 32f752f..a6d00c1 100644 --- a/resources/lib/UnityUser.php +++ b/resources/lib/UnityUser.php @@ -79,7 +79,7 @@ public function init($firstname, $lastname, $email, $org, $send_mail = true) $ldapUserEntry->setAttribute("sn", $lastname); $ldapUserEntry->setAttribute( "gecos", - \transliterator_transliterate("Latin-ASCII", "{$this->getFirstname()} {$this->getLastname()}") + \transliterator_transliterate("Latin-ASCII", "$firstname $lastname") ); $ldapUserEntry->setAttribute("mail", $email); $ldapUserEntry->setAttribute("o", $org); From 567d9d27c529e9dc5939461d7e5ff992bb277f50 Mon Sep 17 00:00:00 2001 From: Simon Leary Date: Sat, 7 Jun 2025 14:59:15 -0400 Subject: [PATCH 08/25] fix request fetch --- resources/lib/UnityGroup.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/resources/lib/UnityGroup.php b/resources/lib/UnityGroup.php index 1039253..4de6a01 100644 --- a/resources/lib/UnityGroup.php +++ b/resources/lib/UnityGroup.php @@ -139,8 +139,7 @@ public function requestGroup($firstname, $lastname, $email, $org, $send_mail_to_ public function approveGroup($operator = null, $send_mail = true) { $uid = $this->getOwner()->getUID(); - $gid = $this->getPIUID(); - $request = $this->SQL->getRequest($uid, $gid); + $request = $this->SQL->getRequest($uid, UnitySQL::REQUEST_BECOME_PI); if (is_null($request)) { throw new Exception("uid '$uid' does not have a group request!"); } From aa50390191871fda78eb79fd9a1095cf30cfd376 Mon Sep 17 00:00:00 2001 From: Simon Leary Date: Sat, 7 Jun 2025 15:09:04 -0400 Subject: [PATCH 09/25] make sql exception more generic --- resources/autoload.php | 2 +- resources/lib/UnitySQL.php | 9 ++++++--- .../lib/exceptions/UnitySQLNoSuchRequestException.php | 6 ------ resources/lib/exceptions/UnitySQLRecordNotFound.php | 6 ++++++ test/phpunit-bootstrap.php | 1 + 5 files changed, 14 insertions(+), 10 deletions(-) delete mode 100644 resources/lib/exceptions/UnitySQLNoSuchRequestException.php create mode 100644 resources/lib/exceptions/UnitySQLRecordNotFound.php diff --git a/resources/autoload.php b/resources/autoload.php index 8875b20..1838832 100644 --- a/resources/autoload.php +++ b/resources/autoload.php @@ -25,7 +25,7 @@ require_once __DIR__ . "/lib/UnityRedis.php"; require_once __DIR__ . "/lib/UnityGithub.php"; require_once __DIR__ . "/lib/exceptions/PhpUnitNoDieException.php"; -require_once __DIR__ . "/lib/exceptions/UnitySQLNoSuchRequestException.php"; +require_once __DIR__ . "/lib/exceptions/UnitySQLRecordNotFound.php"; // run init script require __DIR__ . "/init.php"; diff --git a/resources/lib/UnitySQL.php b/resources/lib/UnitySQL.php index 2d0e452..d58c368 100644 --- a/resources/lib/UnitySQL.php +++ b/resources/lib/UnitySQL.php @@ -4,7 +4,7 @@ use PDO; use PDOException; -use UnityWebPortal\lib\exceptions\UnitySQLNoSuchRequestException; +use UnityWebPortal\lib\exceptions\UnitySQLRecordNotFound; class UnitySQL { @@ -147,7 +147,7 @@ public function getRequest($user, $dest) { $results = $this->search(self::TABLE_REQS, ["request_for" => $dest]); if (count($results) == 0) { - throw new UnitySQLNoSuchRequestException("no such request: uid='$user' request_for='$dest'"); + throw new UnitySQLRecordNotFound("no such request: uid='$user' request_for='$dest'"); } if (count($results) > 1) { throw new Exception("too many requests for uid='$user' request_for='$dest'"); @@ -160,7 +160,7 @@ public function requestExists($requestor, $dest = self::REQUEST_BECOME_PI) try { self::getRequest($requestor, $dest); return true; - } catch (UnitySQLNoSuchRequestException) { + } catch (UnitySQLRecordNotFound) { return false; } } @@ -266,6 +266,9 @@ public function deleteAccountDeletionRequest($uid) public function getSiteVar($name): string { $results = $this->search(self::TABLE_SITEVARS, ["name" => $name]); + if (count($results) == 0) { + throw new UnitySQLRecordNotFound($name); + } assert(count($results) == 1); return $results[0]["value"]; } diff --git a/resources/lib/exceptions/UnitySQLNoSuchRequestException.php b/resources/lib/exceptions/UnitySQLNoSuchRequestException.php deleted file mode 100644 index 45755c6..0000000 --- a/resources/lib/exceptions/UnitySQLNoSuchRequestException.php +++ /dev/null @@ -1,6 +0,0 @@ - Date: Sat, 7 Jun 2025 15:02:29 -0400 Subject: [PATCH 10/25] fix tests --- test/functional/NewUserTest.php | 63 ++++++++++++++++++--------------- 1 file changed, 35 insertions(+), 28 deletions(-) diff --git a/test/functional/NewUserTest.php b/test/functional/NewUserTest.php index c5483be..fbac8dc 100644 --- a/test/functional/NewUserTest.php +++ b/test/functional/NewUserTest.php @@ -3,13 +3,18 @@ use PHPUnit\Framework\TestCase; use UnityWebPortal\lib\exceptions\PhpUnitNoDieException; use UnityWebPortal\lib\UnityGroup; +use UnityWebPortal\lib\UnityOrg; +use UnityWebPortal\lib\UnitySQL; class NewUserTest extends TestCase { - private function assertNumberGroupRequests(int $x) + private function assertRequestedPIGroup(bool $expected) { global $USER, $SQL; - $this->assertEquals($x, count($SQL->getRequestsByUser($USER->getUID()))); + $this->assertEquals( + $expected, + $SQL->requestExists($USER->getUID(), UnitySQL::REQUEST_BECOME_PI) + ); } private function requestGroupCreation() @@ -45,12 +50,12 @@ private function ensureUserDoesNotExist() { global $USER, $SQL, $LDAP; $SQL->deleteRequestsByUser($USER->getUID()); - $org = $USER->getOrgGroup(); - if ($org->exists() and $org->inOrg($USER)) { - $org->removeUser($USER); - assert(!$org->inOrg($USER)); - } if ($USER->exists()) { + $org = $USER->getOrgGroup(); + if ($org->exists() and $org->inOrg($USER)) { + $org->removeUser($USER); + assert(!$org->inOrg($USER)); + } $USER->getLDAPUser()->delete(); assert(!$USER->exists()); } @@ -69,8 +74,8 @@ private function ensureUserDoesNotExist() private function ensureOrgGroupDoesNotExist() { - global $USER; - $org_group = $USER->getOrgGroup(); + global $USER, $SSO, $LDAP, $SQL, $MAILER, $REDIS, $WEBHOOK; + $org_group = new UnityOrg($SSO["org"], $LDAP, $SQL, $MAILER, $REDIS, $WEBHOOK); if ($org_group->exists()) { $org_group->getLDAPOrgGroup()->delete(); assert(!$org_group->exists()); @@ -97,18 +102,19 @@ private function ensurePIGroupDoesNotExist() public function testCreateUserByJoinGoup() { - global $USER, $SQL, $LDAP; + global $USER, $SSO, $LDAP, $SQL, $MAILER, $REDIS, $WEBHOOK; switchUser(...getUserIsPIHasNoMembersNoMemberRequests()); $pi_group = $USER->getPIGroup(); switchUser(...getNonExistentUser()); $this->assertTrue(!$USER->exists()); - $this->assertTrue(!$USER->getOrgGroup()->exists()); + $newOrg = new UnityOrg($SSO["org"], $LDAP, $SQL, $MAILER, $REDIS, $WEBHOOK); + $this->assertTrue(!$newOrg->exists()); $this->assertTrue($pi_group->exists()); $this->assertTrue(!$pi_group->userExists($USER)); - $this->assertNumberGroupRequests(0); + $this->assertRequestedPIGroup(false); try { $this->requestGroupMembership($pi_group->getPIUID()); - $this->assertNumberGroupRequests(1); + $this->assertRequestedPIGroup(true); // $second_request_failed = false; // try { @@ -117,21 +123,21 @@ public function testCreateUserByJoinGoup() // $second_request_failed = true; // } // $this->assertTrue($second_request_failed); - $this->assertNumberGroupRequests(1); + $this->assertRequestedPIGroup(true); $this->cancelAllRequests(); - $this->assertNumberGroupRequests(0); + $this->assertRequestedPIGroup(false); $this->requestGroupMembership($pi_group->getPIUID()); $this->assertTrue($pi_group->requestExists($USER)); - $this->assertNumberGroupRequests(1); + $this->assertRequestedPIGroup(true); $pi_group->approveUser($USER); $this->assertTrue(!$pi_group->requestExists($USER)); - $this->assertNumberGroupRequests(0); + $this->assertRequestedPIGroup(false); $this->assertTrue($pi_group->userExists($USER)); $this->assertTrue($USER->exists()); - $this->assertTrue($USER->getOrgGroup()->exists()); + $this->assertTrue($newOrg->exists()); // $third_request_failed = false; // try { @@ -140,7 +146,7 @@ public function testCreateUserByJoinGoup() // $third_request_failed = true; // } // $this->assertTrue($third_request_failed); - $this->assertNumberGroupRequests(0); + $this->assertRequestedPIGroup(false); $this->assertTrue(!$pi_group->requestExists($USER)); } finally { $this->ensureUserNotInPIGroup($pi_group); @@ -151,15 +157,16 @@ public function testCreateUserByJoinGoup() public function testCreateUserByCreateGroup() { - global $USER, $SQL, $LDAP; + global $USER, $SSO, $LDAP, $SQL, $MAILER, $REDIS, $WEBHOOK; switchuser(...getNonExistentUser()); $pi_group = $USER->getPIGroup(); $this->assertTrue(!$USER->exists()); $this->assertTrue(!$pi_group->exists()); - $this->assertTrue(!$USER->getOrgGroup()->exists()); + $newOrg = new UnityOrg($SSO["org"], $LDAP, $SQL, $MAILER, $REDIS, $WEBHOOK); + $this->assertTrue(!$newOrg->exists()); try { $this->requestGroupCreation(); - $this->assertNumberGroupRequests(1); + $this->assertRequestedPIGroup(true); // $second_request_failed = false; // try { @@ -168,19 +175,19 @@ public function testCreateUserByCreateGroup() // $second_request_failed = true; // } // $this->assertTrue($second_request_failed); - $this->assertNumberGroupRequests(1); + $this->assertRequestedPIGroup(true); $this->cancelAllRequests(); - $this->assertNumberGroupRequests(0); + $this->assertRequestedPIGroup(false); $this->requestGroupCreation(); - $this->assertNumberGroupRequests(1); + $this->assertRequestedPIGroup(true); $pi_group->approveGroup(); - $this->assertNumberGroupRequests(0); + $this->assertRequestedPIGroup(false); $this->assertTrue($pi_group->exists()); $this->assertTrue($USER->exists()); - $this->assertTrue($USER->getOrgGroup()->exists()); + $this->assertTrue($newOrg->exists()); // $third_request_failed = false; // try { @@ -189,7 +196,7 @@ public function testCreateUserByCreateGroup() // $third_request_failed = true; // } // $this->assertTrue($third_request_failed); - $this->assertNumberGroupRequests(0); + $this->assertRequestedPIGroup(false); } finally { $this->ensurePIGroupDoesNotExist(); $this->ensureUserDoesNotExist(); From 0fc9c69dff36e91996b1b5c772bbeca376fd791d Mon Sep 17 00:00:00 2001 From: Simon Leary Date: Sat, 7 Jun 2025 15:44:09 -0400 Subject: [PATCH 11/25] use email from request --- resources/lib/UnityGroup.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/resources/lib/UnityGroup.php b/resources/lib/UnityGroup.php index 4de6a01..06911da 100644 --- a/resources/lib/UnityGroup.php +++ b/resources/lib/UnityGroup.php @@ -434,7 +434,7 @@ public function newUserRequest($new_user, $firstname, $lastname, $email, $org, $ if ($send_mail) { // send email to user $this->MAILER->sendMail( - $new_user->getMail(), + $email, "group_user_request", array("group" => $this->pi_uid) ); From b568d898880d7bbc0c6d997bb3e2e0e4311b9760 Mon Sep 17 00:00:00 2001 From: Simon Leary Date: Sat, 7 Jun 2025 15:44:19 -0400 Subject: [PATCH 12/25] fix tests --- test/functional/NewUserTest.php | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/test/functional/NewUserTest.php b/test/functional/NewUserTest.php index fbac8dc..474bce4 100644 --- a/test/functional/NewUserTest.php +++ b/test/functional/NewUserTest.php @@ -17,6 +17,15 @@ private function assertRequestedPIGroup(bool $expected) ); } + private function assertRequestedMembership(bool $expected, string $gid) + { + global $USER, $SQL; + $this->assertEquals( + $expected, + $SQL->requestExists($USER->getUID(), $gid) + ); + } + private function requestGroupCreation() { http_post( @@ -105,16 +114,17 @@ public function testCreateUserByJoinGoup() global $USER, $SSO, $LDAP, $SQL, $MAILER, $REDIS, $WEBHOOK; switchUser(...getUserIsPIHasNoMembersNoMemberRequests()); $pi_group = $USER->getPIGroup(); + $gid = $pi_group->getPIUID(); switchUser(...getNonExistentUser()); $this->assertTrue(!$USER->exists()); $newOrg = new UnityOrg($SSO["org"], $LDAP, $SQL, $MAILER, $REDIS, $WEBHOOK); $this->assertTrue(!$newOrg->exists()); $this->assertTrue($pi_group->exists()); $this->assertTrue(!$pi_group->userExists($USER)); - $this->assertRequestedPIGroup(false); + $this->assertRequestedMembership(false, $gid); try { $this->requestGroupMembership($pi_group->getPIUID()); - $this->assertRequestedPIGroup(true); + $this->assertRequestedMembership(true, $gid); // $second_request_failed = false; // try { @@ -123,18 +133,18 @@ public function testCreateUserByJoinGoup() // $second_request_failed = true; // } // $this->assertTrue($second_request_failed); - $this->assertRequestedPIGroup(true); + $this->assertRequestedMembership(true, $gid); $this->cancelAllRequests(); - $this->assertRequestedPIGroup(false); + $this->assertRequestedMembership(false, $gid); $this->requestGroupMembership($pi_group->getPIUID()); $this->assertTrue($pi_group->requestExists($USER)); - $this->assertRequestedPIGroup(true); + $this->assertRequestedMembership(true, $gid); $pi_group->approveUser($USER); $this->assertTrue(!$pi_group->requestExists($USER)); - $this->assertRequestedPIGroup(false); + $this->assertRequestedMembership(false, $gid); $this->assertTrue($pi_group->userExists($USER)); $this->assertTrue($USER->exists()); $this->assertTrue($newOrg->exists()); @@ -146,7 +156,7 @@ public function testCreateUserByJoinGoup() // $third_request_failed = true; // } // $this->assertTrue($third_request_failed); - $this->assertRequestedPIGroup(false); + $this->assertRequestedMembership(false, $gid); $this->assertTrue(!$pi_group->requestExists($USER)); } finally { $this->ensureUserNotInPIGroup($pi_group); From 2f3edabbd5b32a9b55a6ba6c6395631c9f03bcfb Mon Sep 17 00:00:00 2001 From: Simon Leary Date: Sat, 7 Jun 2025 16:14:46 -0400 Subject: [PATCH 13/25] remove extra space --- webroot/panel/groups.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/webroot/panel/groups.php b/webroot/panel/groups.php index 069d61f..1f11a39 100644 --- a/webroot/panel/groups.php +++ b/webroot/panel/groups.php @@ -47,7 +47,7 @@ // Add row to sql if (empty($modalErrors)) { - $pi_account->newUserRequest($USER, $SSO["firstname"], $SSO["lastname"], $SSO["mail"], $SSO["org "]); + $pi_account->newUserRequest($USER, $SSO["firstname"], $SSO["lastname"], $SSO["mail"], $SSO["org"]); } break; case "removePIForm": From 48905b066652aa0c63f2dcf5b48754fa10d201a7 Mon Sep 17 00:00:00 2001 From: Simon Leary Date: Sat, 7 Jun 2025 16:14:54 -0400 Subject: [PATCH 14/25] rewrite tests --- test/functional/PiMemberApproveTest.php | 113 ++++++++++++++---------- 1 file changed, 65 insertions(+), 48 deletions(-) diff --git a/test/functional/PiMemberApproveTest.php b/test/functional/PiMemberApproveTest.php index 539f1fc..228745b 100644 --- a/test/functional/PiMemberApproveTest.php +++ b/test/functional/PiMemberApproveTest.php @@ -3,87 +3,104 @@ use PHPUnit\Framework\TestCase; use PHPUnit\Framework\Attributes\DataProvider; use UnityWebPortal\lib\UnityUser; +use UnityWebPortal\lib\UnityGroup; +use UnityWebPortal\lib\UnitySSO; class PiMemberApproveTest extends TestCase { - static $requestUid; - static $noRequestUid; - - public static function setUpBeforeClass(): void{ - global $USER; - switchUser(...getNormalUser()); - self::$requestUid = $USER->getUID(); - switchUser(...getNormalUser2()); - self::$noRequestUid = $USER->getUID(); - } + static $userWithRequestSwitchArgs; + static $userWithoutRequestSwitchArgs; + static $piSwitchArgs; + static $pi; + static $userWithRequestUID; + static $userWithoutRequestUID; + static $piUID; + static $userWithRequest; + static $userWithoutRequest; + static $piGroup; + static $piGroupGID; private function approveUser(string $uid) { - post( + http_post( __DIR__ . "/../../webroot/panel/pi.php", ["form_type" => "userReq", "action" => "approve", "uid" => $uid] ); } - public function testApproveRequest() + private function requestJoinPI(string $gid) + { + http_post( + __DIR__ . "/../../webroot/panel/groups.php", + ["form_type" => "addPIform", "pi" => $gid] + ); + } + + private function assertGroupMembers(UnityGroup $group, array $members) { - global $USER, $LDAP, $SQL, $MAILER, $REDIS, $WEBHOOK; - switchUser(...getUserIsPIHasNoMembersNoMemberRequests()); - $pi = $USER; - $piGroup = $USER->getPIGroup(); - $this->assertTrue($piGroup->exists()); $this->assertTrue( arraysAreEqualUnOrdered( - [$pi->getUID()], - $piGroup->getGroupMemberUIDs() + $members, + $group->getGroupMemberUIDs() ) ); + } + + public function testApproveRequest() + { + global $USER; + $userSwitchArgs = getNormalUser(); + $piSwitchArgs = getUserIsPIHasNoMembersNoMemberRequests(); + switchUser(...$userSwitchArgs); + $user = $USER; + $uid = $USER->getUID(); + switchUser(...$piSwitchArgs); + $piUID = $USER->getUID(); + $piGroup = $USER->getPIGroup(); + + $this->assertTrue($piGroup->exists()); + $this->assertGroupMembers($piGroup, [$piUID]); $this->assertEmpty($piGroup->getRequests()); - $requestedUser = new UnityUser(self::$requestUid, $LDAP, $SQL, $MAILER, $REDIS, $WEBHOOK); try { - $piGroup->newUserRequest($requestedUser); - $this->assertFalse($piGroup->userExists($requestedUser)); + switchUser(...$userSwitchArgs); + $this->requestJoinPI($piGroup->getPIUID()); + $this->assertFalse($piGroup->userExists($user)); - $piGroup->approveUser($requestedUser); + switchUser(...$piSwitchArgs); + $this->approveUser($uid); $this->assertEmpty($piGroup->getRequests()); - - $this->assertTrue( - arraysAreEqualUnOrdered( - [$pi->getUID(), self::$requestUid], - $piGroup->getGroupMemberUIDs() - ) - ); - $this->assertTrue($piGroup->userExists($requestedUser)); + $this->assertGroupMembers($piGroup, [$piUID, $uid]); + $this->assertTrue($piGroup->userExists($user)); } finally { - $piGroup->removeUser($requestedUser); - $SQL->removeRequest(self::$requestUid, $piGroup->getPIUID()); + if ($piGroup->userExists($user)) { + $piGroup->removeUser($user); + } + if ($piGroup->requestExists($user)) { + $piGroup->cancelGroupJoinRequest($user); + } } } public function testApproveNonexistentRequest() { - global $USER, $LDAP, $SQL, $MAILER, $REDIS, $WEBHOOK; + global $USER; + switchUser(...getNormalUser2()); + $user = $USER; + $uid = $USER->getUID(); switchUser(...getUserIsPIHasNoMembersNoMemberRequests()); - $pi = $USER; + $piUID = $USER->getUID(); $piGroup = $USER->getPIGroup(); + $this->assertTrue($piGroup->exists()); - $this->assertTrue( - arraysAreEqualUnOrdered( - [$pi->getUID()], - $piGroup->getGroupMemberUIDs() - ) - ); + $this->assertGroupMembers($piGroup, [$piUID]); $this->assertEmpty($piGroup->getRequests()); - - $notRequestedUser = new UnityUser(self::$noRequestUid, $LDAP, $SQL, $MAILER, $REDIS, $WEBHOOK); - $this->assertFalse($piGroup->userExists($notRequestedUser)); + $this->assertFalse($piGroup->userExists($user)); $this->assertEmpty($piGroup->getRequests()); - try { $this->expectException(Exception::class); - $piGroup->approveUser($notRequestedUser); + $piGroup->approveUser($user); } finally { - if ($piGroup->userExists($notRequestedUser)) { - $piGroup->removeUser($notRequestedUser); + if ($piGroup->userExists($user)) { + $piGroup->removeUser($user); } } } From 6857f7123cba472b5099384a188627a52f73d6a7 Mon Sep 17 00:00:00 2001 From: Simon Leary Date: Sat, 7 Jun 2025 16:42:14 -0400 Subject: [PATCH 15/25] fix tests --- test/functional/PiMemberApproveTest.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/functional/PiMemberApproveTest.php b/test/functional/PiMemberApproveTest.php index 228745b..94ee6af 100644 --- a/test/functional/PiMemberApproveTest.php +++ b/test/functional/PiMemberApproveTest.php @@ -23,7 +23,7 @@ private function approveUser(string $uid) { http_post( __DIR__ . "/../../webroot/panel/pi.php", - ["form_type" => "userReq", "action" => "approve", "uid" => $uid] + ["form_type" => "userReq", "action" => "Approve", "uid" => $uid] ); } @@ -67,6 +67,7 @@ public function testApproveRequest() switchUser(...$piSwitchArgs); $this->approveUser($uid); + $this->assertTrue(!$piGroup->requestExists($user)); $this->assertEmpty($piGroup->getRequests()); $this->assertGroupMembers($piGroup, [$piUID, $uid]); $this->assertTrue($piGroup->userExists($user)); From a0a46481a232258e68998bebff9d4473ad587934 Mon Sep 17 00:00:00 2001 From: Simon Leary Date: Sat, 7 Jun 2025 16:46:31 -0400 Subject: [PATCH 16/25] fix tests --- test/functional/PiBecomeRequestTest.php | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/test/functional/PiBecomeRequestTest.php b/test/functional/PiBecomeRequestTest.php index d0c1edd..6cebd61 100644 --- a/test/functional/PiBecomeRequestTest.php +++ b/test/functional/PiBecomeRequestTest.php @@ -1,6 +1,7 @@ assertNumberPiBecomeRequests(1); } finally { - $SQL->removeRequest($USER->getUID()); + if ($SQL->requestExists($USER, UnitySQL::REQUEST_BECOME_PI)) { + $SQL->removeRequest($USER->getUID(), UnitySQL::REQUEST_BECOME_PI); + } } } @@ -67,7 +70,9 @@ public function testRequestBecomePiUserRequestedAccountDeletion() ); $this->assertNumberPiBecomeRequests(0); } finally { - $SQL->removeRequest($USER->getUID()); + if ($SQL->requestExists($USER, UnitySQL::REQUEST_BECOME_PI)) { + $SQL->removeRequest($USER->getUID(), UnitySQL::REQUEST_BECOME_PI); + } } } } From 2dcdbd2b6622bf55604e8ebb17c5771b3821109d Mon Sep 17 00:00:00 2001 From: Simon Leary Date: Sat, 7 Jun 2025 16:52:30 -0400 Subject: [PATCH 17/25] fix tests --- test/functional/PiRemoveUserTest.php | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/test/functional/PiRemoveUserTest.php b/test/functional/PiRemoveUserTest.php index 2acd46f..1d7dd72 100644 --- a/test/functional/PiRemoveUserTest.php +++ b/test/functional/PiRemoveUserTest.php @@ -43,7 +43,13 @@ public function testRemoveUser() $this->assertFalse($piGroup->userExists($memberToDelete)); } finally { if (!$piGroup->userExists($memberToDelete)) { - $piGroup->newUserRequest($memberToDelete); + $piGroup->newUserRequest( + $memberToDelete, + $memberToDelete->getFirstname(), + $memberToDelete->getLastname(), + $memberToDelete->getMail(), + $memberToDelete->getOrg(), + ); $piGroup->approveUser($memberToDelete); } } @@ -63,7 +69,13 @@ public function testRemovePIFromTheirOwnGroup() $this->assertTrue($piGroup->userExists($pi)); } finally { if (!$piGroup->userExists($pi)) { - $piGroup->newUserRequest($pi); + $piGroup->newUserRequest( + $pi, + $pi->getFirstname(), + $pi->getLastname(), + $pi->getMail(), + $pi->getOrg(), + ); $piGroup->approveUser($pi); } } From c6739adc6f631222864a3b72b68d02af1d999aa7 Mon Sep 17 00:00:00 2001 From: Simon Leary Date: Sat, 7 Jun 2025 16:53:39 -0400 Subject: [PATCH 18/25] fix test --- test/functional/PiMemberDenyTest.php | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/test/functional/PiMemberDenyTest.php b/test/functional/PiMemberDenyTest.php index dbe405f..4278a1d 100644 --- a/test/functional/PiMemberDenyTest.php +++ b/test/functional/PiMemberDenyTest.php @@ -37,7 +37,13 @@ public function testDenyRequest() $this->assertEmpty($piGroup->getRequests()); $requestedUser = new UnityUser(self::$requestUid, $LDAP, $SQL, $MAILER, $REDIS, $WEBHOOK); try { - $piGroup->newUserRequest($requestedUser); + $piGroup->newUserRequest( + $requestedUser, + $requestedUser->getFirstname(), + $requestedUser->getLastname(), + $requestedUser->getMail(), + $requestedUser->getOrg(), + ); $this->assertFalse($piGroup->userExists($requestedUser)); $piGroup->denyUser($requestedUser); From 193b9809e2a389b81f76aede5a7867c5d8a8422b Mon Sep 17 00:00:00 2001 From: Simon Leary Date: Sat, 7 Jun 2025 16:53:50 -0400 Subject: [PATCH 19/25] remove prune --- tools/docker-dev/build.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/docker-dev/build.sh b/tools/docker-dev/build.sh index 4b9dc59..91c5c9c 100755 --- a/tools/docker-dev/build.sh +++ b/tools/docker-dev/build.sh @@ -3,4 +3,4 @@ set -e cd "$(dirname "$0")" docker-compose down docker-compose build -docker image prune +# docker image prune From 1e787231688a6c76caf704b96270cc67cf80d354 Mon Sep 17 00:00:00 2001 From: Simon Leary Date: Sat, 7 Jun 2025 16:53:56 -0400 Subject: [PATCH 20/25] remove old comment --- resources/lib/UnityGroup.php | 1 - 1 file changed, 1 deletion(-) diff --git a/resources/lib/UnityGroup.php b/resources/lib/UnityGroup.php index 06911da..b025d49 100644 --- a/resources/lib/UnityGroup.php +++ b/resources/lib/UnityGroup.php @@ -315,7 +315,6 @@ public function approveUser($new_user, $send_mail = true) // add user to the LDAP object $this->addUserToGroup($new_user); - // remove request, this will fail silently if the request doesn't exist $this->SQL->removeRequest($new_user->getUID(), $this->pi_uid); // send email to the requestor From 2966ded9f548afb1581da738e2c2b9cec73138ff Mon Sep 17 00:00:00 2001 From: Simon Leary Date: Sat, 7 Jun 2025 16:59:27 -0400 Subject: [PATCH 21/25] more similar to previous revision --- resources/lib/UnityGroup.php | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/resources/lib/UnityGroup.php b/resources/lib/UnityGroup.php index b025d49..0d587e4 100644 --- a/resources/lib/UnityGroup.php +++ b/resources/lib/UnityGroup.php @@ -138,10 +138,10 @@ public function requestGroup($firstname, $lastname, $email, $org, $send_mail_to_ */ public function approveGroup($operator = null, $send_mail = true) { - $uid = $this->getOwner()->getUID(); - $request = $this->SQL->getRequest($uid, UnitySQL::REQUEST_BECOME_PI); - if (is_null($request)) { - throw new Exception("uid '$uid' does not have a group request!"); + if (!$this->SQL->requestExists($this->getOwner()->getUID())) { + throw new Exception( + "attempt to approve nonexistent request for group='{$this->getPIUID()}'" + ); } // check for edge cases... @@ -294,12 +294,10 @@ public function cancelGroupJoinRequest($user, $send_mail = true) */ public function approveUser($new_user, $send_mail = true) { - - $uid = $new_user->getUID(); - $gid = $this->getPIUID(); - $request = $this->SQL->getRequest($uid, $gid); - if (is_null($request)) { - throw new Exception("uid '$uid' does not have a request for group '$gid'!"); + if (!$this->requestExists($new_user)) { + throw new Exception( + "attempt to approve nonexistent request for group='{$this->getPIUID()}' uid='$new_user'" + ); } // check if user exists @@ -342,11 +340,10 @@ public function approveUser($new_user, $send_mail = true) public function denyUser($new_user, $send_mail = true) { - $uid = $new_user->getUID(); - $gid = $this->getPIUID(); - $request = $this->SQL->getRequest($uid, $gid); - if (is_null($request)) { - throw new Exception("uid '$uid' does not have a request for group '$gid'!"); + if (!$this->requestExists($new_user)) { + throw new Exception( + "attempt to deny nonexistent request for group='{$this->getPIUID()}' uid='$new_user'" + ); } // remove request, this will fail silently if the request doesn't exist From c1760ff82f5fe4f4d2da080726be2b7fa65910de Mon Sep 17 00:00:00 2001 From: Simon Leary Date: Sat, 7 Jun 2025 17:00:04 -0400 Subject: [PATCH 22/25] wording --- resources/lib/UnitySQL.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/resources/lib/UnitySQL.php b/resources/lib/UnitySQL.php index d58c368..cd1e2ff 100644 --- a/resources/lib/UnitySQL.php +++ b/resources/lib/UnitySQL.php @@ -150,7 +150,7 @@ public function getRequest($user, $dest) throw new UnitySQLRecordNotFound("no such request: uid='$user' request_for='$dest'"); } if (count($results) > 1) { - throw new Exception("too many requests for uid='$user' request_for='$dest'"); + throw new Exception("multiple requests for uid='$user' request_for='$dest'"); } return $results[0]; } From 45e0600c258d42f94382345110bfcdf2781cbff8 Mon Sep 17 00:00:00 2001 From: Simon Leary Date: Sat, 7 Jun 2025 17:06:05 -0400 Subject: [PATCH 23/25] consistent --- webroot/panel/groups.php | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/webroot/panel/groups.php b/webroot/panel/groups.php index 1f11a39..fc916e6 100644 --- a/webroot/panel/groups.php +++ b/webroot/panel/groups.php @@ -33,16 +33,10 @@ if ($USER->getUID() != $SSO["user"]) { $sso_user = $SSO["user"]; - UnitySite::errorLog( - "warning", + UnitySite::badRequest( "cannot request due to uid mismatch: " . "USER='{$USER->getUID()}' SSO[user]='$sso_user'" ); - array_push( - $modalErrors, - "The requesting user must be the one signed in with SSO. " . - "Are you an admin viewing as another user?" - ); } // Add row to sql From c523e2e76ef403af7523e25f15ab7e18703a2af6 Mon Sep 17 00:00:00 2001 From: Simon Leary Date: Sat, 7 Jun 2025 17:08:12 -0400 Subject: [PATCH 24/25] Revert "more similar to previous revision" This reverts commit 2966ded9f548afb1581da738e2c2b9cec73138ff. --- resources/lib/UnityGroup.php | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/resources/lib/UnityGroup.php b/resources/lib/UnityGroup.php index 0d587e4..b025d49 100644 --- a/resources/lib/UnityGroup.php +++ b/resources/lib/UnityGroup.php @@ -138,10 +138,10 @@ public function requestGroup($firstname, $lastname, $email, $org, $send_mail_to_ */ public function approveGroup($operator = null, $send_mail = true) { - if (!$this->SQL->requestExists($this->getOwner()->getUID())) { - throw new Exception( - "attempt to approve nonexistent request for group='{$this->getPIUID()}'" - ); + $uid = $this->getOwner()->getUID(); + $request = $this->SQL->getRequest($uid, UnitySQL::REQUEST_BECOME_PI); + if (is_null($request)) { + throw new Exception("uid '$uid' does not have a group request!"); } // check for edge cases... @@ -294,10 +294,12 @@ public function cancelGroupJoinRequest($user, $send_mail = true) */ public function approveUser($new_user, $send_mail = true) { - if (!$this->requestExists($new_user)) { - throw new Exception( - "attempt to approve nonexistent request for group='{$this->getPIUID()}' uid='$new_user'" - ); + + $uid = $new_user->getUID(); + $gid = $this->getPIUID(); + $request = $this->SQL->getRequest($uid, $gid); + if (is_null($request)) { + throw new Exception("uid '$uid' does not have a request for group '$gid'!"); } // check if user exists @@ -340,10 +342,11 @@ public function approveUser($new_user, $send_mail = true) public function denyUser($new_user, $send_mail = true) { - if (!$this->requestExists($new_user)) { - throw new Exception( - "attempt to deny nonexistent request for group='{$this->getPIUID()}' uid='$new_user'" - ); + $uid = $new_user->getUID(); + $gid = $this->getPIUID(); + $request = $this->SQL->getRequest($uid, $gid); + if (is_null($request)) { + throw new Exception("uid '$uid' does not have a request for group '$gid'!"); } // remove request, this will fail silently if the request doesn't exist From 48677b94206597da089d81032475b7ab93778d0d Mon Sep 17 00:00:00 2001 From: Simon Leary Date: Sat, 7 Jun 2025 17:54:31 -0400 Subject: [PATCH 25/25] delete org before user --- test/functional/NewUserTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/functional/NewUserTest.php b/test/functional/NewUserTest.php index 5a0bb99..4e72a04 100644 --- a/test/functional/NewUserTest.php +++ b/test/functional/NewUserTest.php @@ -162,9 +162,9 @@ public function testCreateUserByJoinGoup() $this->assertRequestedMembership(false, $gid); $this->assertTrue(!$pi_group->requestExists($USER)); } finally { + $this->ensureOrgGroupDoesNotExist(); $this->ensureUserNotInPIGroup($pi_group); $this->ensureUserDoesNotExist(); - $this->ensureOrgGroupDoesNotExist(); } } @@ -211,9 +211,9 @@ public function testCreateUserByCreateGroup() // $this->assertTrue($third_request_failed); $this->assertRequestedPIGroup(false); } finally { + $this->ensureOrgGroupDoesNotExist(); $this->ensurePIGroupDoesNotExist(); $this->ensureUserDoesNotExist(); - $this->ensureOrgGroupDoesNotExist(); } } }
" . $request[0]->getFirstname() . " " . $request[0]->getLastname() . "" . $request[0]->getUID() . "" . $request[0]->getMail() . "" . date("jS F, Y", strtotime($request[1])) . "" . $firstname . " " . $lastname . "" . $uid . "" . $email . "" . $date . ""; echo "
- + + onclick='return confirm(\"Are you sure you want to approve " . $uid . "?\")'> + onclick='return confirm(\"Are you sure you want to deny " . $uid . "?\")'>
"; echo "