Merge branch 'zotlabs/improve-access-list-type-safety' into 'dev'

Zotlabs: Improve type safety for AccessList class.

See merge request hubzilla/core!2052

2 files changed, 71 insertions, 21 deletions

diff --git a/Zotlabs/Access/AccessList.php b/Zotlabs/Access/AccessList.php
index 790ef4745..3f5271e87 100644
--- a/Zotlabs/Access/AccessList.php
+++ b/Zotlabs/Access/AccessList.php
@@ -2,6 +2,7 @@
 
 namespace Zotlabs\Access;
 
+
 /**
  * @brief AccessList class which represents individual content ACLs.
  *
@@ -17,29 +18,48 @@ class AccessList {
 	 * @brief Allow contacts
 	 * @var string
 	 */
-	private $allow_cid;
+	private ?string $allow_cid;
 	/**
 	 * @brief Allow groups
 	 * @var string
 	 */
-	private $allow_gid;
+	private ?string $allow_gid;
 	/**
 	 * @brief Deny contacts
 	 * @var string
 	 */
-	private $deny_cid;
+	private ?string $deny_cid;
 	/**
 	 * @brief Deny groups
 	 * @var string
 	 */
-	private $deny_gid;
+	private ?string $deny_gid;
 	/**
 	 * @brief Indicates if we are using the default constructor values or
 	 * values that have been set explicitly.
 	 * @var boolean
 	 */
-	private $explicit;
+	private bool $explicit;
+
+	/**
+	 * @brief Keys required by the constructor if the channel array is given.
+	 */
+	private const REQUIRED_KEYS_CONSTRUCTOR = [
+		'channel_allow_cid',
+		'channel_allow_gid',
+		'channel_deny_cid',
+		'channel_deny_gid'
+	];
 
+	/**
+	 * @brief Keys required by the set method.
+	 */
+	private const REQUIRED_KEYS_SET = [
+		'allow_cid',
+		'allow_gid',
+		'deny_cid',
+		'deny_gid'
+	];
 
 	/**
 	 * @brief Constructor for AccessList class.
@@ -53,8 +73,9 @@ class AccessList {
 	 *   * \e string \b channel_deny_cid => string of denied cids
 	 *   * \e string \b channel_deny_gid => string of denied gids
 	 */
-	function __construct($channel) {
+	function __construct(array $channel) {
 		if ($channel) {
+			$this->validate_input_array($channel, self::REQUIRED_KEYS_CONSTRUCTOR);
 			$this->allow_cid = $channel['channel_allow_cid'];
 			$this->allow_gid = $channel['channel_allow_gid'];
 			$this->deny_cid  = $channel['channel_deny_cid'];
@@ -70,13 +91,24 @@ class AccessList {
 		$this->explicit = false;
 	}
 
+	private function validate_input_array(array $arr, array $required_keys) : void {
+		$missing_keys = array_diff($required_keys, array_keys($arr));
+
+		if (!empty($missing_keys)) {
+			throw new \Exception(
+				'Invalid AccessList object: Expected array with keys: '
+				. implode(', ', $missing_keys)
+			);
+		}
+	}
+
 	/**
 	 * @brief Get if we are using the default constructor values
 	 * or values that have been set explicitly.
 	 *
 	 * @return boolean
 	 */
-	function get_explicit() {
+	function get_explicit() : bool {
 		return $this->explicit;
 	}
 
@@ -94,7 +126,9 @@ class AccessList {
 	 *   * \e string \b deny_gid  => string of denied gids
 	 * @param boolean $explicit (optional) default true
 	 */
-	function set($arr, $explicit = true) {
+	function set(array $arr, bool $explicit = true) : void {
+		$this->validate_input_array($arr, self::REQUIRED_KEYS_SET);
+
 		$this->allow_cid = $arr['allow_cid'];
 		$this->allow_gid = $arr['allow_gid'];
 		$this->deny_cid  = $arr['deny_cid'];
@@ -112,7 +146,7 @@ class AccessList {
 	 *   * \e string \b deny_cid  => string of denied cids
 	 *   * \e string \b deny_gid  => string of denied gids
 	 */
-	function get() {
+	function get() : array {
 		return [
 			'allow_cid' => $this->allow_cid,
 			'allow_gid' => $this->allow_gid,
@@ -138,7 +172,7 @@ class AccessList {
 	 *   * \e array|string \b group_deny    => array with gids or comma-seperated string
 	 * @param boolean $explicit (optional) default true
 	 */
-	function set_from_array($arr, $explicit = true) {
+	function set_from_array(array $arr, bool $explicit = true) : void {
 		$arr['contact_allow'] = $arr['contact_allow'] ?? [];
 		$arr['group_allow'] = $arr['group_allow'] ?? [];
 		$arr['contact_deny'] = $arr['contact_deny'] ?? [];
@@ -161,7 +195,7 @@ class AccessList {
 	 *
 	 * @return boolean Return true if any of allow_* deny_* values is set.
 	 */
-	function is_private() {
+	function is_private() : bool {
 		return (($this->allow_cid || $this->allow_gid || $this->deny_cid || $this->deny_gid) ? true : false);
 	}
 
diff --git a/tests/unit/Access/AccessListTest.php b/tests/unit/Access/AccessListTest.php
index 2f185db17..635f09930 100644
--- a/tests/unit/Access/AccessListTest.php
+++ b/tests/unit/Access/AccessListTest.php
@@ -60,14 +60,24 @@ class AccessListTest extends UnitTestCase {
 	}
 
 	/**
-	 * @expectedException PHPUnit\Framework\Error\Error
+	 * AccessList constructor should throw an exception if input is not
+	 * an array.
 	 */
-/*
-	public function testPHPErrorOnInvalidConstructor() {
+	public function testConstructorThrowsOnInvalidInputType() {
+		$this->expectException("TypeError");
 		$accessList = new AccessList('invalid');
-		// Causes: "Illegal string offset 'channel_allow_cid'"
 	}
-*/
+
+	/**
+	 * AccessList constructor should throw an exception on
+	 * invalid input.
+	 */
+	public function testConstructorThrowsOnMissingKeysInArray() {
+		$this->expectException("Exception");
+		$this->expectExceptionMessage("Invalid AccessList object");
+		$accessList = new AccessList(['something_else' => 'should_this_fail?']);
+	}
+
 	/**
 	 * Test that the defaults are as expected when constructed with
 	 * an empty array.
@@ -111,16 +121,22 @@ class AccessListTest extends UnitTestCase {
 	}
 
 	/**
-	 * @expectedException PHPUnit\Framework\Error\Error
+	 * The set method should throw an exception if input is not an array.
 	 */
-/*
-	public function testPHPErrorOnInvalidSet() {
+	public function testSetThrowsOnInvalidInputType() {
+		$this->expectException('TypeError');
 		$accessList = new AccessList([]);
 
 		$accessList->set('invalid');
-		// Causes: "Illegal string offset 'allow_cid'"
 	}
-*/
+
+	public function testSetThrowsOnMissingKeysInArray() {
+		$this->expectException('Exception');
+		$this->expectExceptionMessage('Invalid AccessList object');
+
+		$accessList = new AccessList([]);
+		$accessList->set(['something_else' => 'should_this_fail?']);
+	}
 
 	/**
 	 * The set_from_array() function calls some other functions, too which are