Skip to content

Commit c21d9c3

Browse files
simonLeary42simonLeary42
authored
"view as user" test, rework uses of die() (#212)
* replace die with exception * use if/then instead of die * set unique session cache location for each phpunit run * viewAsUser test * UnitySite not static * remove pointless file * ajax php don't initialize entire stack * every switchUser has a fresh session by default * get post always discard output * Revert "UnitySite not static" This reverts commit 01e7581. * add UnitySite::die * Revert "replace die with exception" This reverts commit fa5d021. * add functions errorLog, badRequest * update ViewAsUserTest * get test working * add exceptions * delete comments * unused import * rename post to http_post, get to http_get * fix die * add pre commit check for usage of die * no color in die checker * dont print to stderr * ignore phpunit result cache * don't allow exit() either * refactor * show both die and exit * get -> http_get * revert conditional * revert conditional 2 * add message * remove unused exception class * exclude UnitySite from die check * use unauthorized function, don't print anything to use * allow empty die(), rename unauthorized to forbidden * don't explode trace * remove broken magic response code reason * httpResponseCode private * more tests * allow undefined server protocol * more tests * clear view before redirect nonexistent user * remove comment
1 parent 8814abd commit c21d9c3

26 files changed

+308
-66
lines changed

‎.gitignore

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,3 +16,5 @@ composer.lock
1616
deployment/*
1717
!deployment/**/README.md
1818
!deployment/deploy.sh
19+
20+
.phpunit.result.cache

‎.pre-commit-config.yaml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,3 +48,9 @@ repos:
4848
language: system
4949
files: \.php$
5050
args: [-l]
51+
- id: assert-no-die-exit
52+
name: Assert no die()/exit()
53+
entry: ./test/assert-no-die-exit.bash
54+
language: system
55+
files: \.php$
56+
exclude: resources/lib/UnitySite\.php$

‎resources/lib/UnitySite.php

Lines changed: 54 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,17 +3,70 @@
33
namespace UnityWebPortal\lib;
44

55
use phpseclib3\Crypt\PublicKeyLoader;
6+
use UnityWebPortal\lib\exceptions\PhpUnitNoDieException;
67

78
class UnitySite
89
{
10+
public static function die($x = null)
11+
{
12+
if (@$GLOBALS["PHPUNIT_NO_DIE_PLEASE"] == true) {
13+
if (is_null($x)) {
14+
throw new PhpUnitNoDieException();
15+
} else {
16+
throw new PhpUnitNoDieException($x);
17+
}
18+
} else {
19+
if (is_null($x)) {
20+
die();
21+
} else {
22+
die($x);
23+
}
24+
}
25+
}
26+
927
public static function redirect($destination)
1028
{
1129
if ($_SERVER["PHP_SELF"] != $destination) {
1230
header("Location: $destination");
13-
die("Redirect failed, click <a href='$destination'>here</a> to continue.");
31+
self::die("Redirect failed, click <a href='$destination'>here</a> to continue.");
1432
}
1533
}
1634

35+
private static function headerResponseCode(int $code, string $reason)
36+
{
37+
$protocol = @$_SERVER["SERVER_PROTOCOL"] ?? "HTTP/1.1";
38+
$msg = $protocol . " " . strval($code) . " " . $reason;
39+
header($msg, true, $code);
40+
}
41+
42+
public static function errorLog(string $title, string $message)
43+
{
44+
error_log(
45+
"$title: " . json_encode(
46+
[
47+
"message" => $message,
48+
"REMOTE_USER" => @$_SERVER["REMOTE_USER"],
49+
"REMOTE_ADDR" => @$_SERVER["REMOTE_ADDR"],
50+
"trace" => (new \Exception())->getTraceAsString()
51+
]
52+
)
53+
);
54+
}
55+
56+
public static function badRequest($message)
57+
{
58+
self::headerResponseCode(400, "bad request");
59+
self::errorLog("bad request", $message);
60+
self::die();
61+
}
62+
63+
public static function forbidden($message)
64+
{
65+
self::headerResponseCode(403, "forbidden");
66+
self::errorLog("forbidden", $message);
67+
self::die();
68+
}
69+
1770
public static function removeTrailingWhitespace($arr)
1871
{
1972
$out = array();

‎resources/lib/exceptions/PhpUnitNoDieException.php

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
<?php
2+
namespace UnityWebPortal\lib\exceptions;
3+
4+
class PhpUnitNoDieException extends \Exception
5+
{
6+
}

‎resources/templates/header.php

Lines changed: 24 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,14 @@
22

33
use UnityWebPortal\lib\UnitySite;
44

5+
if ((@$_SESSION["is_admin"] ?? false) == true
6+
&& $_SERVER["REQUEST_METHOD"] == "POST"
7+
&& (@$_POST["form_name"] ?? null) == "clearView"
8+
) {
9+
unset($_SESSION["viewUser"]);
10+
UnitySite::redirect($CONFIG["site"]["prefix"] . "/admin/user-mgmt.php");
11+
}
12+
513
if (isset($SSO)) {
614
if (!$_SESSION["user_exists"]) {
715
UnitySite::redirect($CONFIG["site"]["prefix"] . "/panel/new_account.php");
@@ -116,23 +124,20 @@
116124
<main>
117125

118126
<?php
119-
if (isset($_SESSION["is_admin"]) && $_SESSION["is_admin"]) {
120-
if ($_SERVER["REQUEST_METHOD"] == "POST" && isset($_POST["form_name"]) && $_POST["form_name"] == "clearView") {
121-
unset($_SESSION["viewUser"]);
122-
UnitySite::redirect($CONFIG["site"]["prefix"] . "/admin/user-mgmt.php");
123-
}
124-
125-
if (isset($_SESSION["viewUser"])) {
126-
echo "<div id='viewAsBar'>";
127-
echo "<span>You are accessing the web portal as the user <strong>" .
128-
$_SESSION["viewUser"] . "</strong></span>";
129-
echo
130-
"<form method='POST' action=''>
131-
<input type='hidden' name='form_name' value='clearView'>
132-
<input type='hidden' name='uid' value='" . $_SESSION["viewUser"] . "'>
133-
<input type='submit' value='Return to My User'>
134-
</form>";
135-
echo "</div>";
136-
}
127+
if (isset($_SESSION["is_admin"])
128+
&& $_SESSION["is_admin"]
129+
&& isset($_SESSION["viewUser"])
130+
) {
131+
$viewUser = $_SESSION["viewUser"];
132+
echo "
133+
<div id='viewAsBar'>
134+
<span>You are accessing the web portal as the user <strong>$viewUser</strong></span>
135+
<form method='POST' action=''>
136+
<input type='hidden' name='form_name' value='clearView'>
137+
<input type='hidden' name='uid' value='$viewUser'>
138+
<input type='submit' value='Return to My User'>
139+
</form>
140+
</div>
141+
";
137142
}
138-
?>
143+
?>

‎test/assert-no-die-exit.bash

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
#!/bin/bash
2+
set -euo pipefail
3+
if [[ $# -lt 1 ]]; then
4+
echo "at least one argument required"
5+
exit 1
6+
fi
7+
8+
rc=0
9+
10+
# --color=never because magit git output log doesn't support it
11+
die_occurrences="$(
12+
grep -H --color=never --line-number -P '\bdie\s*[\(;]' "$@" | grep -v -P 'UnitySite::die'
13+
)" || true
14+
if [ -n "$die_occurrences" ]; then
15+
echo "die is not allowed! use UnitySite::die() instead."
16+
echo "$die_occurrences"
17+
rc=1
18+
fi
19+
20+
# --color=never because magit git output log doesn't support it
21+
exit_occurrences="$(grep -H --color=never --line-number -P '\bexit\s*[\(;]' "$@")" || true
22+
if [ -n "$exit_occurrences" ]; then
23+
echo "exit is not allowed! use UnitySite::die() instead."
24+
echo "$exit_occurrences"
25+
rc=1
26+
fi
27+
28+
exit "$rc"

‎test/functional/AccountDeletionRequestTest.php

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,12 +38,12 @@ public function testRequestAccountDeletionUserHasNoGroups()
3838
$this->assertEmpty($USER->getGroups());
3939
$this->assertNumberAccountDeletionRequests(0);
4040
try {
41-
post(
41+
http_post(
4242
__DIR__ . "/../../webroot/panel/account.php",
4343
["form_type" => "account_deletion_request"]
4444
);
4545
$this->assertNumberAccountDeletionRequests(1);
46-
post(
46+
http_post(
4747
__DIR__ . "/../../webroot/panel/account.php",
4848
["form_type" => "account_deletion_request"]
4949
);
@@ -62,7 +62,7 @@ public function testRequestAccountDeletionUserHasGroup()
6262
$this->assertNotEmpty($USER->getGroups());
6363
$this->assertNumberAccountDeletionRequests(0);
6464
try {
65-
post(
65+
http_post(
6666
__DIR__ . "/../../webroot/panel/account.php",
6767
["form_type" => "account_deletion_request"]
6868
);

‎test/functional/LoginShellSetTest.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ public function testSetLoginShell(string $shell): void
4444
if (!$this->isShellValid($shell)) {
4545
$this->expectException("Exception");
4646
}
47-
post(
47+
http_post(
4848
__DIR__ . "/../../webroot/panel/account.php",
4949
["form_type" => "loginshell", "shellSelect" => $shell]
5050
);

‎test/functional/PiBecomeRequestTest.php

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,12 +38,12 @@ public function testRequestBecomePi()
3838
$this->assertFalse($USER->isPI());
3939
$this->assertNumberPiBecomeRequests(0);
4040
try {
41-
post(
41+
http_post(
4242
__DIR__ . "/../../webroot/panel/account.php",
4343
["form_type" => "pi_request"]
4444
);
4545
$this->assertNumberPiBecomeRequests(1);
46-
post(
46+
http_post(
4747
__DIR__ . "/../../webroot/panel/account.php",
4848
["form_type" => "pi_request"]
4949
);
@@ -61,7 +61,7 @@ public function testRequestBecomePiUserRequestedAccountDeletion()
6161
$this->assertNumberPiBecomeRequests(0);
6262
$this->assertTrue($SQL->accDeletionRequestExists($USER->getUID()));
6363
try {
64-
post(
64+
http_post(
6565
__DIR__ . "/../../webroot/panel/account.php",
6666
["form_type" => "pi_request"]
6767
);

‎test/functional/SSHKeyAddTest.php

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ class SSHKeyAddTest extends TestCase
99
{
1010
private function addSshKeysPaste(array $keys): void {
1111
foreach ($keys as $key) {
12-
post(
12+
http_post(
1313
__DIR__ . "/../../webroot/panel/account.php",
1414
[
1515
"form_type" => "addKey",
@@ -27,7 +27,7 @@ private function addSshKeysImport(array $keys): void {
2727
fwrite($tmp, $key);
2828
$_FILES["keyfile"] = ["tmp_name" => $tmp_path];
2929
try {
30-
post(
30+
http_post(
3131
__DIR__ . "/../../webroot/panel/account.php",
3232
["form_type" => "addKey", "add_type" => "import"]
3333
);
@@ -40,7 +40,7 @@ private function addSshKeysImport(array $keys): void {
4040

4141
private function addSshKeysGenerate(array $keys): void {
4242
foreach ($keys as $key) {
43-
post(
43+
http_post(
4444
__DIR__ . "/../../webroot/panel/account.php",
4545
[
4646
"form_type" => "addKey",
@@ -57,7 +57,7 @@ private function addSshKeysGithub(array $keys): void {
5757
$GITHUB = $this->createMock(UnityGithub::class);
5858
$GITHUB->method("getSshPublicKeys")->willReturn($keys);
5959
try {
60-
post(
60+
http_post(
6161
__DIR__ . "/../../webroot/panel/account.php",
6262
[
6363
"form_type" => "addKey",

‎test/functional/SSHKeyDeleteTest.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ public static function setUpBeforeClass(): void{
1313
}
1414

1515
private function deleteKey(string $index): void {
16-
post(
16+
http_post(
1717
__DIR__ . "/../../webroot/panel/account.php",
1818
["form_type" => "delKey", "delIndex" => $index]
1919
);

‎test/functional/ViewAsUserTest.php

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
<?php
2+
3+
use UnityWebPortal\lib\UnitySite;
4+
use UnityWebPortal\lib\exceptions\PhpUnitNoDieException;
5+
use PHPUnit\Framework\TestCase;
6+
7+
class ViewAsUserTest extends TestCase
8+
{
9+
public function _testViewAsUser(array $beforeUser, array $afterUser)
10+
{
11+
global $USER;
12+
switchUser(...$afterUser);
13+
$afterUid = $USER->getUID();
14+
switchUser(...$beforeUser);
15+
// $this->assertTrue($USER->isAdmin());
16+
$beforeUid = $USER->getUID();
17+
// $this->assertNotEquals($afterUid, $beforeUid);
18+
try {
19+
http_post(
20+
__DIR__ . "/../../webroot/admin/user-mgmt.php",
21+
[
22+
"form_name" => "viewAsUser",
23+
"uid" => $afterUid,
24+
],
25+
);
26+
} catch (PhpUnitNoDieException) {}
27+
$this->assertArrayHasKey("viewUser", $_SESSION);
28+
// redirect means that php process dies and user's browser will initiate a new one
29+
// this makes `require_once autoload.php` run again and init.php changes $USER
30+
session_write_close();
31+
http_get(__DIR__ . "/../../resources/init.php");
32+
// now we should be new user
33+
$this->assertEquals($afterUid, $USER->getUID());
34+
// $this->assertTrue($_SESSION["user_exists"]);
35+
try {
36+
http_post(
37+
__DIR__ . "/../../resources/templates/header.php",
38+
["form_name" => "clearView"],
39+
);
40+
} catch (PhpUnitNoDieException) {}
41+
$this->assertArrayNotHasKey("viewUser", $_SESSION);
42+
// redirect means that php process dies and user's browser will initiate a new one
43+
// this makes `require_once autoload.php` run again and init.php changes $USER
44+
session_write_close();
45+
http_get(__DIR__ . "/../../resources/init.php");
46+
// now we should be back to original user
47+
$this->assertEquals($beforeUid, $USER->getUID());
48+
}
49+
50+
public function testViewAsUser()
51+
{
52+
$this->_testViewAsUser(getAdminUser(), getNormalUser());
53+
}
54+
55+
public function testViewAsNonExistentUser()
56+
{
57+
$this->_testViewAsUser(getAdminUser(), getNonExistentUser());
58+
}
59+
60+
public function testViewAsSelf()
61+
{
62+
$this->_testViewAsUser(getAdminUser(), getAdminUser());
63+
}
64+
65+
public function testNonAdminViewAsAdmin()
66+
{
67+
global $USER;
68+
switchUser(...getAdminUser());
69+
$adminUid = $USER->getUID();
70+
$this->assertTrue($USER->isAdmin());
71+
switchUser(...getNormalUser());
72+
try {
73+
http_post(
74+
__DIR__ . "/../../webroot/admin/user-mgmt.php",
75+
[
76+
"form_name" => "viewAsUser",
77+
"uid" => $adminUid,
78+
],
79+
);
80+
} catch (PhpUnitNoDieException) {}
81+
$this->assertArrayNotHasKey("viewUser", $_SESSION);
82+
}
83+
}

0 commit comments

Comments
 (0)