diff --git a/Zotlabs/Module/Admin/Account_edit.php b/Zotlabs/Module/Admin/Account_edit.php index 0300fb10c..35a15133f 100644 --- a/Zotlabs/Module/Admin/Account_edit.php +++ b/Zotlabs/Module/Admin/Account_edit.php @@ -8,6 +8,11 @@ class Account_edit { function post() { + // Validate CSRF token + // + // We terminate with a 403 Forbidden status if the check fails. + check_form_security_token_ForbiddenOnErr('admin_account_edit', 'security'); + $account_id = $_REQUEST['aid']; if(! $account_id) @@ -18,7 +23,7 @@ class Account_edit { if($pass1 && $pass2 && ($pass1 === $pass2)) { $salt = random_string(32); $password_encoded = hash('whirlpool', $salt . $pass1); - $r = q("update account set account_salt = '%s', account_password = '%s', + $r = q("update account set account_salt = '%s', account_password = '%s', account_password_changed = '%s' where account_id = %d", dbesc($salt), dbesc($password_encoded), @@ -34,7 +39,7 @@ class Account_edit { $account_level = 5; $account_language = trim($_REQUEST['account_language']); - $r = q("update account set account_service_class = '%s', account_level = %d, account_language = '%s' + $r = q("update account set account_service_class = '%s', account_level = %d, account_language = '%s' where account_id = %d", dbesc($service_class), intval($account_level), @@ -62,8 +67,8 @@ class Account_edit { return ''; } - $a = replace_macros(get_markup_template('admin_account_edit.tpl'), [ + '$security' => get_form_security_token('admin_account_edit'), '$account' => $x[0], '$title' => t('Account Edit'), '$pass1' => [ 'pass1', t('New Password'), ' ','' ], diff --git a/tests/unit/Module/AdminAccountEditTest.php b/tests/unit/Module/AdminAccountEditTest.php new file mode 100644 index 000000000..dab646a45 --- /dev/null +++ b/tests/unit/Module/AdminAccountEditTest.php @@ -0,0 +1,214 @@ +stub_is_site_admin = + $this->getFunctionMock('Zotlabs\Module', 'is_site_admin') + ->expects($this->once()) + ->willReturn(true); + + $this->info = []; + $this->stub_info = + $this->getFunctionMock('Zotlabs\Module\Admin', 'info') + ->expects($this->any()) + ->willReturnCallback(function (string $arg) { + $this->info[] = $arg; + }); + + $this->notice = []; + $this->stub_notice = + $this->getFunctionMock('Zotlabs\Module\Admin', 'notice') + ->expects($this->any()) + ->willReturnCallback(function (string $arg) { + $this->notice[] = $arg; + }); + + } + + #[After] + public function tear_down_mocks(): void { + $this->stub_is_site_admin = null; + $this->stub_info = null; + $this->stub_notice = null; + $this->stub_check_security = null; + $this->stub_get_form_security_token = null; + } + + public function test_rendering_admin_account_edit_page(): void { + $this->stub_get_form_security_token = + $this->getFunctionMock('Zotlabs\Module\Admin', 'get_form_security_token') + ->expects($this->once()) + ->willReturn('the-csrf-token'); + + $account = $this->fixtures['account'][0]; + + $this->get("admin/account_edit/{$account['account_id']}"); + + $this->assertPageContains("
assertPageContains($account['account_email']); + + // Check that we generate a CSRF token for the form + $this->assertPageContains("get("admin/account_edit/666"); + + $this->assertEquals('', \App::$page['content']); + } + + public function test_rendering_admin_account_edit_page_fails_if_id_is_not_numeric(): void { + $this->get("admin/account_edit/66invalid"); + + $this->assertEquals('', \App::$page['content']); + } + + public function test_post_empty_form_does_not_modify_account(): void { + $this->stub_goaway(); + $this->stub_check_form_security(true); + + $account = get_account_by_id($this->fixtures['account'][0]['account_id']); + + try { + $this->post( + "admin/account_edit/{$account['account_id']}", + [], + [ + 'aid' => $account['account_id'], + 'pass1' => '', + 'pass2' => '', + 'service_class' => $account['account_service_class'], + 'account_language' => $account['account_language'], + 'security' => 'The security token', + ] + ); + } catch (RedirectException $ex) { + $this->assertEquals(z_root() . '/admin/accounts', $ex->getMessage()); + } + + $reloaded = get_account_by_id($account['account_id']); + + $this->assertEquals($account, $reloaded); + + // Not sure if this is expected behaviour, but this is how it is today. + $this->assertContains('Account settings updated.' . EOL, $this->info); + } + + public function test_post_form_changes_account(): void { + $this->stub_goaway(); + $this->stub_check_form_security(true); + + // clone account from fixture, to ensure it's not replaced with + // the reloaded one below. + $account = get_account_by_id($this->fixtures['account'][0]['account_id']); + + try { + $this->post( + "admin/account_edit/{$account['account_id']}", + [], + [ + 'aid' => $account['account_id'], + 'pass1' => 'hunter2', + 'pass2' => 'hunter2', + 'service_class' => 'Some other class', + 'account_language' => 'nn', + 'security' => 'The security token', + ] + ); + } catch (RedirectException $ex) { + $this->assertEquals(z_root() . '/admin/accounts', $ex->getMessage()); + } + + $reloaded = get_account_by_id($account['account_id']); + + $this->assertNotEquals($account, $reloaded); + $this->assertEquals('Some other class', $reloaded['account_service_class']); + $this->assertEquals('nn', $reloaded['account_language']); + + $now = new DateTimeImmutable('now'); + $this->assertEquals($now->format('Y-m-d H:i:s'), $reloaded['account_password_changed']); + + $this->assertContains('Account settings updated.' . EOL, $this->info); + $this->assertContains("Password changed for account {$account['account_id']}." . EOL, $this->info); + } + + public function test_form_with_missing_or_incalid_csrf_token_is_rejected(): void { + $this->expectException(KillmeException::class); + + // Emulate a failed CSRF check + $this->stub_check_form_security(false); + + $account_id = $this->fixtures['account'][0]['account_id']; + + $this->post( + "admin/account_edit/{$account_id}", + [], + [ + 'aid' => $account_id, + 'pass1' => 'hunter2', + 'pass2' => 'hunter2', + 'service_class' => 'Some other class', + 'account_language' => 'nn', + 'security' => 'Invalid security token', + ] + ); + } + + /* + * Override the stub_goaway method because we need the stub to live in the + * Admin namespace. + */ + protected function stub_goaway(): void { + $this->goaway_stub = $this->getFunctionMock('Zotlabs\Module\Admin', 'goaway') + ->expects($this->once()) + ->willReturnCallback( + function (string $uri) { + throw new RedirectException($uri); + } + ); + } + + /** + * Stub the check_form_security_token_ForbiddenOnErr. + * + * In these tests we're not really interested in _how_ the form security + * tokens work, but that the code under test perform the checks. This stub + * allows us to do that without having to worry if everything is set up so + * that the real function would work or not. + * + * @param bool $valid true if emulating a valid token, false otherwise. + */ + protected function stub_check_form_security(bool $valid): void { + $this->stub_check_security = + $this->getFunctionMock('Zotlabs\Module\Admin', 'check_form_security_token_ForbiddenOnErr') + ->expects($this->once()) + ->with( + $this->identicalTo('admin_account_edit'), + $this->identicalTo('security')) + ->willReturnCallback(function () use ($valid) { + if (! $valid) { + throw new KillmeException(); + } + }); + } +} diff --git a/tests/unit/Module/TestCase.php b/tests/unit/Module/TestCase.php index e92bc7083..81d8e61fc 100644 --- a/tests/unit/Module/TestCase.php +++ b/tests/unit/Module/TestCase.php @@ -10,6 +10,7 @@ namespace Zotlabs\Tests\Unit\Module; +use PHPUnit\Framework\Attributes\After; use Zotlabs\Tests\Unit\UnitTestCase; use App; @@ -25,6 +26,31 @@ class TestCase extends UnitTestCase { // Import PHPMock methods into this class use \phpmock\phpunit\PHPMock; + #[After] + public function cleanup_stubs(): void { + $this->killme_stub = null; + $this->goaway_stub = null; + } + + protected function do_request(string $method, string $uri, array $query = [], array $params = []): void { + $_GET['q'] = $uri; + $_GET = array_merge($_GET, $query); + $_POST = $params; + + $_SERVER['REQUEST_METHOD'] = $method; + $_SERVER['SERVER_PROTOCOL'] = 'HTTP/1.1'; + $_SERVER['QUERY_STRING'] = "q={$uri}"; + // phpcs:disable Generic.PHP.DisallowRequestSuperglobal.Found + $_REQUEST = array_merge($_GET, $_POST); + // phpcs::enable + + \App::init(); + \App::$page['content'] = ''; + + $router = new \Zotlabs\Web\Router(); + $router->Dispatch(); + } + /** * Emulate a GET request. * @@ -34,24 +60,21 @@ class TestCase extends UnitTestCase { * as keys. */ protected function get(string $uri, array $query = []): void { - $_GET['q'] = $uri; + $this->do_request('GET', $uri, $query); + } - if (!empty($query)) { - $_GET = array_merge($_GET, $query); - } - - $_SERVER['REQUEST_METHOD'] = 'GET'; - $_SERVER['SERVER_PROTOCOL'] = 'HTTP/1.1'; - $_SERVER['QUERY_STRING'] = "q={$uri}"; - // phpcs:disable Generic.PHP.DisallowRequestSuperglobal.Found - $_REQUEST = $_GET; - // phpcs::enable - - \App::init(); - \App::$page['content'] = ''; - - $router = new \Zotlabs\Web\Router(); - $router->Dispatch(); + /** + * Emulate a POST request. + * + * @param string $uri The URI to request. Typically this will be the module + * name, followed by any req args separated by slashes. + * @param array $query Associative array of query args, with the parameters + * as keys. + * @param array $params Associative array of POST params, with the param names + * as keys. + */ + protected function post(string $uri, array $query = [], array $params = []): void { + $this->do_request('POST', $uri, $query, $params); } /** @@ -100,8 +123,7 @@ class TestCase extends UnitTestCase { * @throws KillmeException */ protected function stub_killme(): void { - $killme_stub = $this->getFunctionMock('Zotlabs\Module', 'killme'); - $killme_stub + $this->killme_stub = $this->getFunctionMock('Zotlabs\Module', 'killme') ->expects($this->once()) ->willReturnCallback( function () { @@ -147,8 +169,7 @@ class TestCase extends UnitTestCase { * @throws RedirectException */ protected function stub_goaway(): void { - $goaway_stub = $this->getFunctionMock('Zotlabs\Module', 'goaway'); - $goaway_stub + $this->goaway_stub = $this->getFunctionMock('Zotlabs\Module', 'goaway') ->expects($this->once()) ->willReturnCallback( function (string $uri) { diff --git a/tests/unit/includes/dba/_files/account.yml b/tests/unit/includes/dba/_files/account.yml index 88e59056e..b7a49529e 100644 --- a/tests/unit/includes/dba/_files/account.yml +++ b/tests/unit/includes/dba/_files/account.yml @@ -3,9 +3,11 @@ account: account_id: 42 account_email: "hubzilla@example.com" account_language: "no" + account_level: 5 account_flags: 0 - account_id: 43 account_email: "hubzilla@example.org" account_language: "de" + account_level: 5 account_flags: 1 diff --git a/view/tpl/admin_account_edit.tpl b/view/tpl/admin_account_edit.tpl index 1cbb9af0b..6cc8872ba 100644 --- a/view/tpl/admin_account_edit.tpl +++ b/view/tpl/admin_account_edit.tpl @@ -5,6 +5,7 @@ + {{include file="field_password.tpl" field=$pass1}} {{include file="field_password.tpl" field=$pass2}} diff --git a/view/tpl/field_input.tpl b/view/tpl/field_input.tpl index 1b548221f..c22b6fc29 100644 --- a/view/tpl/field_input.tpl +++ b/view/tpl/field_input.tpl @@ -1,6 +1,6 @@
{{$field.3}} diff --git a/view/tpl/field_password.tpl b/view/tpl/field_password.tpl index 7baad7d48..00cb9f9e1 100644 --- a/view/tpl/field_password.tpl +++ b/view/tpl/field_password.tpl @@ -1,5 +1,5 @@
- {{if $field.4}} {{$field.4}} {{/if}} + {{if isset($field.4)}} {{$field.4}} {{/if}} {{$field.3}}
diff --git a/view/tpl/field_select.tpl b/view/tpl/field_select.tpl index a98a26799..e1ad18b60 100644 --- a/view/tpl/field_select.tpl +++ b/view/tpl/field_select.tpl @@ -1,11 +1,11 @@
- + {{$field.3}}
-{{* +{{* COMMENTS for this template: @author hilmar runge, 2020.01 $field array index: