Merge branch 'module-admin-accounts-enhancements' into 'dev'

Fix and refactor module Admin\Accounts part I

See merge request hubzilla/core!2173

5 files changed, 395 insertions, 165 deletions

diff --git a/Zotlabs/Module/Admin/Accounts.php b/Zotlabs/Module/Admin/Accounts.php
index 6f7cb0311..108231d7d 100644
--- a/Zotlabs/Module/Admin/Accounts.php
+++ b/Zotlabs/Module/Admin/Accounts.php
@@ -6,133 +6,33 @@ use Zotlabs\Lib\Config;
 
 class Accounts {
 
-	/**
-	 * @brief Handle POST actions on accounts admin page.
-	 *
-	 * This function is called when on the admin user/account page the form was
-	 * submitted to handle multiple operations at once. If one of the icons next
-	 * to an entry are pressed the function admin_page_accounts() will handle this.
-	 *
-	 */
 
 	const MYP = 	'ZAR';			// ZAR2x
 	const VERSION =	'2.0.0';
 
-	function post() {
+	/**
+	 * Handle POST actions on accounts admin page.
+	 */
+	public function post() {
 
-		$pending = ( x($_POST, 'pending') ? $_POST['pending'] : array() );
-		$users   = ( x($_POST, 'user')    ? $_POST['user']    : array() );
-		$blocked = ( x($_POST, 'blocked') ? $_POST['blocked'] : array() );
+		$pending = x($_POST, 'pending') ? $_POST['pending'] : array();
 
 		check_form_security_token_redirectOnErr('/admin/accounts', 'admin_accounts');
 
-		$isajax = is_ajax();
-		$rc = 0;
-
-		If (!is_site_admin()) {
-			if ($isajax) {
-				killme();
-				exit;
-			}
-			goaway(z_root() . '/');
-		}
-
-		if ($isajax) {
-			//$debug = print_r($_SESSION[self::MYP],true);
-			$zarop = (x($_POST['zardo']) && preg_match('/^[ad]{1,1}$/', $_POST['zardo']) )
-					 ? $_POST['zardo'] : '';
-			// zarat arrives with leading underscore _n
-			$zarat = (x($_POST['zarat']) && preg_match('/^_{1,1}[0-9]{1,6}$/', $_POST['zarat']) )
-					 ? substr($_POST['zarat'],1) : '';
-			$zarse = (x($_POST['zarse']) && preg_match('/^[0-9a-f]{8,8}$/', $_POST['zarse']) )
-					 ? hex2bin($_POST['zarse']) : '';
-
-			if ($zarop && $zarat >= 0 && $zarse && $zarse == $_SESSION[self::MYP]['h'][$zarat]) {
-
-				//
-				if ($zarop == 'd') {
-					$rd = q("UPDATE register SET reg_vital = 0 WHERE reg_id = %d AND SUBSTR(reg_hash,1,4) = '%s' ",
-						intval($_SESSION[self::MYP]['i'][$zarat]),
-						dbesc($_SESSION[self::MYP]['h'][$zarat])
-					);
-					$rc = '×';
-				}
-				elseif ($zarop == 'a') {
-					// approval, REGISTER_DENIED by user 0x0040, REGISTER_AGREED by user 0x0020 @Regate
-					$rd = q("UPDATE register SET reg_flags = (reg_flags & ~ 16), "
-						.	" reg_vital = (CASE (reg_flags & ~ 48) WHEN 0 THEN 0 ELSE 1 END) "
-						.	" WHERE reg_vital = 1 AND reg_id = %d AND SUBSTR(reg_hash,1,4) = '%s' ",
-						intval($_SESSION[self::MYP]['i'][$zarat]),
-						dbesc($_SESSION[self::MYP]['h'][$zarat])
-					);
-					$rc = 0;
-					$rs = q("SELECT * from register WHERE reg_id = %d ",
-							intval($_SESSION[self::MYP]['i'][$zarat])
-					);
-					if ($rs && ($rs[0]['reg_flags'] & ~ 48) == 0) {
-						// create account
-						$rc = 'ok'.$rs[0]['reg_id'];
-						$ac = create_account_from_register($rs[0]);
-						if ( $ac['success'] ) {
-							$rc .= '✔';
-
-							$auto_create  = Config::Get('system','auto_channel_create',1);
-
-							if($auto_create) {
-								$reonar = json_decode($rs[0]['reg_stuff'], true);
-								// prepare channel creation
-								if($reonar['chan.name'])
-									set_aconfig($ac['account']['account_id'], 'register', 'channel_name', $reonar['chan.name']);
-
-								if($reonar['chan.did1'])
-									set_aconfig($ac['account']['account_id'], 'register', 'channel_address', $reonar['chan.did1']);
-
-								$permissions_role  = Config::Get('system','default_permissions_role');
-								if($permissions_role)
-									set_aconfig($ac['account']['account_id'], 'register', 'permissions_role', $permissions_role);
-
-								// create channel
-								$new_channel = auto_channel_create($ac['account']['account_id']);
-
-								if($new_channel['success']) {
-									$rc .= ' c,ok' . $new_channel['channel']['channel_id'] . '✔';
-								}
-								else {
-									$rc .= ' c ×';
-								}
-							}
-
-
-						}
-					} else {
-						$rc='oh ×';
-					}
-				}
-				echo json_encode(array('re' => $zarop, 'at' => '_' . $zarat, 'rc' => $rc));
-			}
+		if (is_ajax()) {
+			$this->handle_ajax_request();
 			killme();
-			exit;
 		}
 
 		// change to switch structure?
 		// account block/unblock button was submitted
 		if (x($_POST, 'page_accounts_block')) {
-			for ($i = 0; $i < count($users); $i++) {
-				// if account is blocked remove blocked bit-flag, otherwise add blocked bit-flag
-				$op = ($blocked[$i]) ? '& ~' : '| ';
-				q("UPDATE account SET account_flags = (account_flags $op%d) WHERE account_id = %d",
-					intval(ACCOUNT_BLOCKED),
-					intval($users[$i])
-				);
-			}
-			notice( sprintf( tt("%s account blocked/unblocked", "%s account blocked/unblocked", count($users)), count($users)) );
+			$this->block_unblock_accounts();
 		}
+
 		// account delete button was submitted
 		if (x($_POST, 'page_accounts_delete')) {
-			foreach ($users as $uid){
-				account_remove($uid, true, false);
-			}
-			notice( sprintf( tt("%s account deleted", "%s accounts deleted", count($users)), count($users)) );
+			$this->delete_accounts();
 		}
 		// registration approved button was submitted
 		if (x($_POST, 'page_accounts_approve')) {
@@ -351,5 +251,143 @@ class Accounts {
 		return $o;
 	}
 
+	private function handle_ajax_request(): void {
+		//$debug = print_r($_SESSION[self::MYP],true);
+		$zarop = (x($_POST['zardo']) && preg_match('/^[ad]{1,1}$/', $_POST['zardo']) )
+			? $_POST['zardo'] : '';
+		// zarat arrives with leading underscore _n
+		$zarat = (x($_POST['zarat']) && preg_match('/^_{1,1}[0-9]{1,6}$/', $_POST['zarat']) )
+			? substr($_POST['zarat'],1) : '';
+		$zarse = (x($_POST['zarse']) && preg_match('/^[0-9a-f]{8,8}$/', $_POST['zarse']) )
+			? hex2bin($_POST['zarse']) : '';
+
+		if ($zarop && $zarat >= 0 && $zarse && $zarse == $_SESSION[self::MYP]['h'][$zarat]) {
+
+			//
+			if ($zarop == 'd') {
+				$rd = q("UPDATE register SET reg_vital = 0 WHERE reg_id = %d AND SUBSTR(reg_hash,1,4) = '%s' ",
+					intval($_SESSION[self::MYP]['i'][$zarat]),
+					dbesc($_SESSION[self::MYP]['h'][$zarat])
+				);
+				$rc = '×';
+			}
+			elseif ($zarop == 'a') {
+				// approval, REGISTER_DENIED by user 0x0040, REGISTER_AGREED by user 0x0020 @Regate
+				$rd = q("UPDATE register SET reg_flags = (reg_flags & ~ 16), "
+					.	" reg_vital = (CASE (reg_flags & ~ 48) WHEN 0 THEN 0 ELSE 1 END) "
+					.	" WHERE reg_vital = 1 AND reg_id = %d AND SUBSTR(reg_hash,1,4) = '%s' ",
+					intval($_SESSION[self::MYP]['i'][$zarat]),
+					dbesc($_SESSION[self::MYP]['h'][$zarat])
+				);
+				$rc = 0;
+				$rs = q("SELECT * from register WHERE reg_id = %d ",
+					intval($_SESSION[self::MYP]['i'][$zarat])
+				);
+				if ($rs && ($rs[0]['reg_flags'] & ~ 48) == 0) {
+					// create account
+					$rc = 'ok'.$rs[0]['reg_id'];
+					$ac = create_account_from_register($rs[0]);
+					if ( $ac['success'] ) {
+						$rc .= '✔';
+
+						$auto_create  = Config::Get('system','auto_channel_create',1);
+
+						if($auto_create) {
+							$reonar = json_decode($rs[0]['reg_stuff'], true);
+							// prepare channel creation
+							if($reonar['chan.name'])
+								set_aconfig($ac['account']['account_id'], 'register', 'channel_name', $reonar['chan.name']);
+
+							if($reonar['chan.did1'])
+								set_aconfig($ac['account']['account_id'], 'register', 'channel_address', $reonar['chan.did1']);
+
+							$permissions_role  = Config::Get('system','default_permissions_role');
+							if($permissions_role)
+								set_aconfig($ac['account']['account_id'], 'register', 'permissions_role', $permissions_role);
+
+							// create channel
+							$new_channel = auto_channel_create($ac['account']['account_id']);
+
+							if($new_channel['success']) {
+								$rc .= ' c,ok' . $new_channel['channel']['channel_id'] . '✔';
+							}
+							else {
+								$rc .= ' c ×';
+							}
+						}
+
+
+					}
+				} else {
+					$rc='oh ×';
+				}
+			}
+			echo json_encode(array('re' => $zarop, 'at' => '_' . $zarat, 'rc' => $rc));
+		}
+	}
+
+	/**
+	 * Block or unblock accounts given by the `user` and `blocked` POST params.
+	 *
+	 * The post params `user` and `blocked` must be present and arrays of equal
+	 * lengths. The `user` array should contain account id's or the accounts to
+	 * process, and the `blocked` array holds a corresponding boolean value to
+	 * indicate that the account at the same offset in the `user` array is or is
+	 * not blocked.
+	 *
+	 * An account that is _not_ blocked will be blocked, and accounts that _are_
+	 * blocked will be unblocked.
+	 *
+	 * @SuppressWarnings(PHPMD.ShortVariable)
+	 */
+	private function block_unblock_accounts(): void {
+		if (!isset($_POST['user']) || !isset($_POST['blocked'])) {
+			return;
+		}
+
+		$users = $_POST['user'];
+		$blocked = $_POST['blocked'];
+
+		if (!is_array($users) || !is_array($blocked)) {
+			return;
+		}
+
+		foreach($users as $i => $id) {
+			// if account is blocked remove blocked bit-flag, otherwise add blocked bit-flag
+			$op = $blocked[$i] ? '& ~' : '| ';
+
+			q("UPDATE account SET account_flags = (account_flags $op%d) WHERE account_id = %d",
+				intval(ACCOUNT_BLOCKED),
+				intval($id)
+			);
+		}
+
+		$count = count($users);
+		$fmt = tt("%s account blocked/unblocked", "%s account blocked/unblocked", $count);
+		notice(sprintf($fmt, $count));
+	}
+
+	/**
+	 * Delete multiple accounts given by the `user` POST param.
+	 */
+	private function delete_accounts(): void {
+		if (!isset($_POST['user'])) {
+			return;
+		}
+
+		$users = $_POST['user'];
+
+		if (!is_array($users)) {
+			return;
+		}
+
+		foreach ($users as $uid){
+			account_remove($uid, true, false);
+		}
+
+		$count = count($users);
+		$fmt = tt("%s account deleted", "%s accounts deleted", $count);
+		notice(sprintf($fmt, $count));
+	}
 }
 
diff --git a/include/account.php b/include/account.php
index 884c07389..615c802f4 100644
--- a/include/account.php
+++ b/include/account.php
@@ -613,59 +613,45 @@ function send_register_success_email($email,$password) {
 }
 
 /**
- * @brief Allows a user registration.
+ * Mark a pending registration as approved, and notify the account
+ * holder by email.
  *
- * @param string $hash
- * @return array|boolean
+ * @param string $hash		The registration hash of the entry to approve
+ *
+ * @return bool
  */
-function account_allow($hash) {
-
-	$ret = array('success' => false);
+function account_allow(string $hash): bool {
 
 	$register = q("SELECT * FROM register WHERE reg_hash = '%s' LIMIT 1",
 		dbesc($hash)
 	);
 
-	if(! $register)
-		return $ret;
+	if (! $register) {
+		logger(
+			"Entry with hash '{$hash}' was not found in the register table.",
+			LOGGER_NORMAL,
+			LOG_ERR
+		);
+		return false;
+	}
 
-	$account = q("SELECT * FROM account WHERE account_id = %d LIMIT 1",
-		intval($register[0]['reg_uid'])
-	);
+	$account = get_account_by_id($register[0]['reg_uid']);
 
-	// a register entry without account assigned to
-	if(! $account)
-		return $ret;
+	if (! $account) {
+		logger(
+			"Account '{$register[0]['reg_uid']}' mentioned by registration hash '{$hash}' was not found.",
+			LOGGER_NORMAL,
+			LOG_ERR
+		);
+		return false;
+	}
 
-	// [hilmar ->
+	$transaction = new DbaTransaction(DBA::$dba);
 
-	q("START TRANSACTION");
-	//q("DELETE FROM register WHERE reg_hash = '%s'",
-	//	dbesc($register[0]['reg_hash'])
-	//);
 	$r1 = q("UPDATE register SET reg_vital = 0 WHERE reg_hash = '%s'",
 		dbesc($register[0]['reg_hash'])
 	);
 
-	/* instead of ...
-
-	// unblock
-	q("UPDATE account SET    account_flags = (account_flags & ~%d) "
-		.			" WHERE (account_flags & %d)>0 AND account_id = %d",
-		intval(ACCOUNT_BLOCKED),
-		intval(ACCOUNT_BLOCKED),
-		intval($register[0]['reg_uid'])
-	);
-
-	// unpend
-	q("UPDATE account SET    account_flags = (account_flags & ~%d) "
-		. 			" WHERE (account_flags & %d)>0 AND account_id = %d",
-		intval(ACCOUNT_PENDING),
-		intval(ACCOUNT_PENDING),
-		intval($register[0]['reg_uid'])
-	);
-
-	*/
 	// together unblock and unpend
 	$r2 = q("UPDATE account SET account_flags = %d WHERE account_id = %d",
 		intval($account['account_flags']
@@ -674,9 +660,7 @@ function account_allow($hash) {
 	);
 
 	if($r1 && $r2) {
-		q("COMMIT");
-
-		// <- hilmar]
+		$transaction->commit();
 
 		push_lang($register[0]['reg_lang']);
 
@@ -684,35 +668,35 @@ function account_allow($hash) {
 		$email_msg = replace_macros($email_tpl, array(
 				'$sitename' => Config::Get('system','sitename'),
 				'$siteurl' =>  z_root(),
-				'$username' => $account[0]['account_email'],
-				'$email' => $account[0]['account_email'],
+				'$username' => $account['account_email'],
+				'$email' => $account['account_email'],
 				'$password' => '',
-				'$uid' => $account[0]['account_id']
+				'$uid' => $account['account_id']
 		));
 
 		$res = z_mail(
 			[
-			'toEmail' => $account[0]['account_email'],
+			'toEmail' => $account['account_email'],
 			'messageSubject' => sprintf( t('Registration details for %s'), Config::Get('system','sitename')),
 			'textVersion' => $email_msg,
 			]
 		);
 
-		pop_lang();
+		if (! $res) {
+			info(t("Sending account approval email to {$account['email']} failed..."));
+		}
 
-		if(Config::Get('system', 'auto_channel_create', 1))
-			auto_channel_create($register[0]['uid']);
+		pop_lang();
 
-		if ($res) {
-			info( t('Account approved.') . EOL );
-			return true;
+		if(Config::Get('system', 'auto_channel_create', 1)) {
+			auto_channel_create($register[0]['reg_uid']);
 		}
 
-	// [hilmar ->
-	} else {
-		q("ROLLBACK");
+		info( t('Account approved.') . EOL );
+		return true;
 	}
-	// <- hilmar]
+
+	return false;
 }
 
 
diff --git a/tests/unit/Module/AdminAccountsTest.php b/tests/unit/Module/AdminAccountsTest.php
new file mode 100644
index 000000000..2c76f2779
--- /dev/null
+++ b/tests/unit/Module/AdminAccountsTest.php
@@ -0,0 +1,173 @@
+<?php
+/*
+ * SPDX-FileCopyrightText: 2024 Hubzilla Community
+ * SPDX-FileContributor: Harald Eilertsen
+ *
+ * SPDX-License-Identifier: MIT
+ */
+
+namespace Zotlabs\Tests\Unit\Module;
+
+use PHPUnit\Framework\Attributes\Before;
+
+class AdminAccountsTest extends TestCase {
+
+	protected $stub_check_security;
+	protected $stub_is_site_admin;
+	protected $stub_goaway;
+	protected $stub_notice;
+
+	protected array $notice;
+
+	/**
+	 * Set up the stubs common for the tests.
+	 */
+	#[Before]
+	public function setup_stubs(): void {
+		$this->stub_check_form_security();
+		$this->stub_is_site_admin();
+		$this->stub_goaway();
+		$this->stub_notice();
+	}
+
+	public function test_blocking_accounts_marks_selected_accounts_as_blocked(): void {
+		$params = [
+			'user' => [ 42 ],
+			'blocked' => [ false ],
+			'page_accounts_block' => true,
+		];
+
+		try {
+			$this->post('admin/accounts', [], $params);
+		} catch (RedirectException $redirect) {
+			$this->assertEquals(z_root() . '/admin/accounts', $redirect->getMessage());
+		}
+
+		$account = get_account_by_id(42);
+		$this->assertEquals(ACCOUNT_BLOCKED, $account['account_flags'] & ACCOUNT_BLOCKED);
+
+		$this->assertEquals('1 account blocked/unblocked', $this->notice[0]);
+	}
+
+	public function test_unblocking_accounts_clears_the_blocked_flag(): void {
+		// Pass two users to the module, one that is not blocked,
+		// and one that is.
+		$params = [
+			'user' => [ 42, 44 ],
+			'blocked' => [ false, true ],
+			'page_accounts_block' => true,
+		];
+
+		try {
+			$this->post('admin/accounts', [], $params);
+		} catch (RedirectException $redirect) {
+			$this->assertEquals(z_root() . '/admin/accounts', $redirect->getMessage());
+		}
+
+		// We expect the previously unblocked account to be blocked.
+		$account = get_account_by_id(42);
+		$this->assertEquals(ACCOUNT_BLOCKED, $account['account_flags'] & ACCOUNT_BLOCKED);
+
+		// We expect the previously blocked account to be unblocked.
+		$blocked_account = get_account_by_id(44);
+		$this->assertEquals(0, $blocked_account['account_flags'] & ACCOUNT_BLOCKED);
+
+		$this->assertEquals('2 account blocked/unblocked', $this->notice[0]);
+	}
+
+	public function test_deleting_accouns_remove_them_from_db(): void {
+		$params = [
+			'user' => [ 42, 44 ],
+			'page_accounts_delete' => true,
+		];
+
+		try {
+			$this->post('admin/accounts', [], $params);
+		} catch (RedirectException $redirect) {
+			$this->assertEquals(z_root() . '/admin/accounts', $redirect->getMessage());
+		}
+
+		$this->assertEquals(null, get_account_by_id(42));
+		$this->assertEquals(null, get_account_by_id(44));
+	}
+
+	public function test_approving_pending_accounts_clears_pending_flag(): void {
+
+		// Catch calls to the php mail function
+		//
+		// This is just to get it out of the way, we don't care about
+		// how many times it's called, or with what args here.
+		$this->getFunctionMock('Zotlabs\Lib', 'mail')
+			->expects($this->any())
+			->willReturn(true);
+
+		$params = [
+			'pending' => [
+				$this->fixtures['register'][0]['reg_hash'],
+				$this->fixtures['register'][1]['reg_hash']
+		   	],
+			'page_accounts_approve' => true,
+		];
+
+		try {
+			$this->post('admin/accounts', [], $params);
+		} catch (RedirectException $redirect) {
+			$this->assertEquals(z_root() . '/admin/accounts', $redirect->getMessage());
+		}
+
+		foreach ([45, 46] as $id) {
+			$account = get_account_by_id($id);
+			$this->assertEquals(0, $account['account_flags'] & ACCOUNT_PENDING);
+		}
+	}
+
+	/**
+	 * Stub the check_form_security_token_ForbiddenOnErr.
+	 */
+	protected function stub_check_form_security(): void {
+		$this->stub_check_security =
+			$this->getFunctionMock('Zotlabs\Module\Admin', 'check_form_security_token_redirectOnErr')
+				->expects($this->once())
+				->with(
+					$this->identicalTo('/admin/accounts'),
+					$this->identicalTo('admin_accounts'))
+				->willReturn(true);
+	}
+
+	/**
+	 * Stub the call to is_site_admin in the Admin main module.
+	 */
+	protected function stub_is_site_admin(): void {
+		$this->stub_is_site_admin =
+			$this->getFunctionMock('Zotlabs\Module', 'is_site_admin')
+				->expects($this->once())
+				->willReturn(true);
+	}
+
+	/**
+	 * Stub the goaway function.
+	 *
+	 * Will throw an RedirectException with the URL being redirected to
+	 * as the exception message.
+	 *
+	 * @throws RedirectException
+	 */
+	protected function stub_goaway(): void {
+		$this->stub_goaway =
+			$this->getFunctionMock('Zotlabs\Module\Admin', 'goaway')
+				->expects($this->once())
+				->willReturnCallback(function (string $uri) {
+					throw new RedirectException($uri);
+				});
+	}
+
+	protected function stub_notice(): void {
+		$this->notice = [];
+		$this->stub_notice =
+			$this->getFunctionMock('Zotlabs\Module\Admin', 'notice')
+				->expects($this->any())
+				->willReturnCallback(function (string $arg) {
+					$this->notice[] = $arg;
+				});
+	}
+}
diff --git a/tests/unit/includes/dba/_files/account.yml b/tests/unit/includes/dba/_files/account.yml
index b7a49529e..9c3d00ec8 100644
--- a/tests/unit/includes/dba/_files/account.yml
+++ b/tests/unit/includes/dba/_files/account.yml
@@ -11,3 +11,18 @@ account:
     account_language: "de"
     account_level: 5
     account_flags: 1
+  -
+    account_id: 44
+    account_email: "blocked@example.org"
+    account_level: 5
+    account_flags: 2
+  -
+    account_id: 45
+    account_email: "pending@example.org"
+    account_level: 5
+    account_flags: 0x10
+  -
+    account_id: 46
+    account_email: "unverified@example.org"
+    account_level: 5
+    account_flags: 0x11
diff --git a/tests/unit/includes/dba/_files/register.yml b/tests/unit/includes/dba/_files/register.yml
new file mode 100644
index 000000000..2ef1a5365
--- /dev/null
+++ b/tests/unit/includes/dba/_files/register.yml
@@ -0,0 +1,20 @@
+---
+register:
+  -
+    reg_vital: 1
+    reg_flags: 0x10
+    reg_did2: 'verified@example.com'
+    reg_email: 'verified@example.com'
+    reg_hash: '123'
+    reg_uid: 45
+    reg_pass: 'verify'
+    reg_stuff: ''
+  -
+    reg_vital: 1
+    reg_flags: 0x11
+    reg_did2: 'unverified@example.com'
+    reg_email: 'unverified@example.com'
+    reg_hash: '666'
+    reg_uid: 46
+    reg_pass: 'verify'
+    reg_stuff: ''