diff options
| -rw-r--r-- | Zotlabs/Module/Admin/Account_edit.php | 11 | ||||
| -rw-r--r-- | tests/unit/Module/AdminAccountEditTest.php | 214 | ||||
| -rw-r--r-- | tests/unit/Module/TestCase.php | 59 | ||||
| -rw-r--r-- | tests/unit/includes/dba/_files/account.yml | 2 | ||||
| -rw-r--r-- | view/tpl/admin_account_edit.tpl | 1 | ||||
| -rw-r--r-- | view/tpl/field_input.tpl | 4 | ||||
| -rw-r--r-- | view/tpl/field_password.tpl | 2 | ||||
| -rw-r--r-- | view/tpl/field_select.tpl | 4 |
8 files changed, 270 insertions, 27 deletions
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 @@ +<?php +/* Tests for Account_edit module + * + * SPDX-FileCopyrightText: 2024 Hubzilla Community + * SPDX-FileContributor: Harald Eilertsen + * + * SPDX-License-Identifier: MIT + */ + +namespace Zotlabs\Tests\Unit\Module; + +use DateTimeImmutable; +use PHPUnit\Framework\Attributes\{Before, After}; +use Zotlabs\Model\Account; + +class AdminAccountEditTest extends TestCase { + + #[Before] + public function setup_mocks(): void { + /* + * As we're testing pages that should only be reachable by the + * site admin, it makes no sense to have it return anything else + * than true. + */ + $this->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("<form action=\"admin/account_edit/{$account['account_id']}\" method=\"post\""); + $this->assertPageContains($account['account_email']); + + // Check that we generate a CSRF token for the form + $this->assertPageContains("<input type=\"hidden\" name=\"security\" value=\"the-csrf-token\""); + } + + public function test_rendering_admin_account_edit_page_fails_if_id_is_not_found(): void { + $this->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,26 +26,22 @@ class TestCase extends UnitTestCase { // Import PHPMock methods into this class use \phpmock\phpunit\PHPMock; - /** - * Emulate a GET 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 Assciative array of query args, with the parameters - * as keys. - */ - protected function get(string $uri, array $query = []): void { - $_GET['q'] = $uri; + #[After] + public function cleanup_stubs(): void { + $this->killme_stub = null; + $this->goaway_stub = null; + } - if (!empty($query)) { - $_GET = array_merge($_GET, $query); - } + 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'] = 'GET'; + $_SERVER['REQUEST_METHOD'] = $method; $_SERVER['SERVER_PROTOCOL'] = 'HTTP/1.1'; $_SERVER['QUERY_STRING'] = "q={$uri}"; // phpcs:disable Generic.PHP.DisallowRequestSuperglobal.Found - $_REQUEST = $_GET; + $_REQUEST = array_merge($_GET, $_POST); // phpcs::enable \App::init(); @@ -55,6 +52,32 @@ class TestCase extends UnitTestCase { } /** + * Emulate a GET 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 Assciative array of query args, with the parameters + * as keys. + */ + protected function get(string $uri, array $query = []): void { + $this->do_request('GET', $uri, $query); + } + + /** + * 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); + } + + /** * Helper to simplify asserting contents in the rendered page. * * @param string $needle The expected string to find. @@ -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 @@ <form action="admin/account_edit/{{$account.account_id}}" method="post" > <input type="hidden" name="aid" value="{{$account.account_id}}" /> +<input type="hidden" name="security" value="{{$security}}"> {{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 @@ <div id="id_{{$field.0}}_wrapper" class="mb-3"> <label for="id_{{$field.0}}" id="label_{{$field.0}}"> - {{$field.1}}{{if $field.4}}<sup class="required zuiqmid"> {{$field.4}}</sup>{{/if}} + {{$field.1}}{{if isset($field.4)}}<sup class="required zuiqmid"> {{$field.4}}</sup>{{/if}} </label> <input class="form-control" @@ -8,7 +8,7 @@ id="id_{{$field.0}}" type="text" value="{{$field.2|escape:'html':'UTF-8':FALSE}}" - {{if $field.5}}{{$field.5}}{{/if}} + {{if isset($field.5)}}{{$field.5}}{{/if}} > <small id="help_{{$field.0}}" class="form-text text-muted"> {{$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 @@ <div class="mb-3"> <label for="id_{{$field.0}}">{{$field.1}}</label> - <input class="form-control" type="password" name="{{$field.0}}" id="id_{{$field.0}}" value="{{$field.2}}"{{if $field.5}} {{$field.5}}{{/if}}>{{if $field.4}} <span class="required">{{$field.4}}</span> {{/if}} + <input class="form-control" type="password" name="{{$field.0}}" id="id_{{$field.0}}" value="{{$field.2}}"{{if isset($field.5)}} {{$field.5}}{{/if}}>{{if isset($field.4)}} <span class="required">{{$field.4}}</span> {{/if}} <small id="help_{{$field.0}}" class="form-text text-muted">{{$field.3}}</small> </div> 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 @@ <div id="id_{{$field.0}}_wrapper" class="mb-3"> - <label for="id_{{$field.0}}">{{$field.1}}{{if $field.5}}<sup class="required zuiqmid"> {{$field.5}}</sup>{{/if}}</label> + <label for="id_{{$field.0}}">{{$field.1}}{{if isset($field.5)}}<sup class="required zuiqmid"> {{$field.5}}</sup>{{/if}}</label> <select class="form-control" name="{{$field.0}}" id="id_{{$field.0}}"> {{foreach $field.4 as $opt=>$val}}<option value="{{$opt}}" {{if $opt==$field.2}}selected="selected"{{/if}}>{{$val}}</option>{{/foreach}} </select> <small class="form-text text-muted">{{$field.3}}</small > </div> -{{* +{{* COMMENTS for this template: @author hilmar runge, 2020.01 $field array index: |