Use UUIDValidator in contentcontainer, file, space and user…

… models (#6587)
humhub · Dec 15, 2023 · 428c7a3 · 428c7a3
1 parent eac7f12
commit 428c7a3
Show file tree

Hide file tree

Showing 12 changed files with 129 additions and 39 deletions.
diff --git a/CHANGELOG-DEV.md b/CHANGELOG-DEV.md
@@ -7,6 +7,7 @@ HumHub Changelog
 - Fix #6693: `MigrateController::$migrationPathMap` stored rolling sum of migrations
 - Enh #6697: Make state badge customizable
 - Fix #6636: Module Manager test
+- Enh #6587: Apply UUID validator
 - Enh #6530: Small performance improvements
 - Fix #6511: Only test compatible modules in `onMarketplaceAfterFilterModules()`
 - Enh #6511: Backup folder path is now return from `removeModule()`

diff --git a/MIGRATE-DEV.md b/MIGRATE-DEV.md
@@ -15,6 +15,7 @@ Version 1.16 (Unreleased)
 - `\humhub\libs\BaseSettingsManager::isDatabaseInstalled()` use `Yii::$app->isDatabaseInstalled()` instead
 
 ### Type restrictions
+- `\humhub\components\behaviors\PolymorphicRelation` enforces types on fields, method parameters, & return types
 - `\humhub\commands\MigrateController` enforces types on fields, method parameters, & return types
 - `\humhub\modules\comment\models\Comment` on `canDelete()`
 - `\humhub\modules\content\components\ContentAddonActiveRecord` on `canDelete()`, `canWrite()`, `canEdit()`
@@ -37,6 +38,8 @@ Version 1.15
 - Use the verifying `Content->canArchive()` before run the methods `Content->archive()`
   and `Content->archive()`, because it was removed from within there.
 - Permission to configure modules is now restricted to users allowed to manage settings (was previously restricted to users allowed to manage modules). [More info here](https://github.com/humhub/humhub/issues/6174).
+- `$guid` properties in `contentcontainer`, `file`, `space`, and `user` models are now enforced to be valid UUIDs
+  (See `UUID::validate()`) and unique within the table.
 
 ### Type restrictions
 - `\humhub\libs\BaseSettingsManager` and its child classes on fields, method parameters, & return types

diff --git a/protected/humhub/components/behaviors/PolymorphicRelation.php b/protected/humhub/components/behaviors/PolymorphicRelation.php
@@ -9,16 +9,15 @@
 namespace humhub\components\behaviors;
 
 use Exception;
+use humhub\components\ActiveRecord;
 use humhub\libs\Helpers;
 use humhub\modules\content\components\ContentActiveRecord;
 use humhub\modules\content\components\ContentAddonActiveRecord;
 use ReflectionClass;
 use ReflectionException;
 use Yii;
 use yii\base\Behavior;
-use yii\base\Model;
-use yii\db\ActiveRecord;
-use yii\db\BaseActiveRecord;
+use yii\db\ActiveRecordInterface;
 use yii\db\IntegrityException;
 
 /**
@@ -33,53 +32,57 @@ class PolymorphicRelation extends Behavior
     /**
      * @var string the class name attribute
      */
-    public $classAttribute = 'object_model';
+    public string $classAttribute = 'object_model';
 
     /**
      * @var string the primary key attribute
      */
-    public $pkAttribute = 'object_id';
+    public string $pkAttribute = 'object_id';
 
     /**
      * @var boolean if set to true, an exception is thrown if `object_model` and `object_id` is set but does not exist
      */
-    public $strict = false;
+    public bool $strict = false;
 
     /**
      * @var array the related object needs to be an "instanceof" at least one of these given classnames
      */
-    public $mustBeInstanceOf = [];
+    public array $mustBeInstanceOf = [];
 
     /**
      * @var ActiveRecord|object|null the cached object
      */
-    private $cached = null;
+    private ?object $cached = null;
 
     /**
      * Returns the Underlying Object
      *
-     * @return ActiveRecord|object|null
+     * @return ActiveRecordInterface|ActiveRecord|object|null
      * @throws IntegrityException
      */
-    public function getPolymorphicRelation()
+    public function getPolymorphicRelation(): ?object
     {
         if ($this->cached !== null) {
             return $this->cached;
         }
 
-        $object = static::loadActiveRecord(
+        $record = static::loadActiveRecord(
             $this->owner->getAttribute($this->classAttribute),
             $this->owner->getAttribute($this->pkAttribute)
         );
 
-        if ($this->strict && !$object && !empty($this->classAttribute) && !empty($this->pkAttribute)) {
-            throw new IntegrityException('Call to an inconsistent polymorphic relation detected on ' . get_class($this->owner) . ' (' . $this->owner->getAttribute($this->classAttribute) . ':' . $this->owner->getAttribute($this->pkAttribute) . ')');
+        if ($this->strict && !$record && !empty($this->classAttribute) && !empty($this->pkAttribute)) {
+            throw new IntegrityException(
+                'Call to an inconsistent polymorphic relation detected on '
+                . ($this->owner === null ? 'NULL' : get_class($this->owner))
+                . ' (' . $this->owner->getAttribute($this->classAttribute) . ':' . $this->owner->getAttribute($this->pkAttribute) . ')'
+            );
         }
 
-        if ($object !== null && $this->validateUnderlyingObjectType($object)) {
-            $this->cached = $object;
+        if ($record !== null && $this->validateUnderlyingObjectType($record)) {
+            $this->cached = $record;
 
-            return $object;
+            return $record;
         }
 
         return null;
@@ -88,9 +91,9 @@ public function getPolymorphicRelation()
     /**
      * Sets the related object
      *
-     * @param mixed $object
+     * @param object|null $object
      */
-    public function setPolymorphicRelation($object)
+    public function setPolymorphicRelation(?object $object)
     {
         if ($this->cached === $object) {
             return;
@@ -100,7 +103,7 @@ public function setPolymorphicRelation($object)
             $cached = $this->cached;
             $this->cached = $object;
 
-            if ($object instanceof ActiveRecord) {
+            if ($object instanceof ActiveRecordInterface) {
                 $class = get_class($object);
                 if ($cached === null || get_class($cached) !== $class) {
                     $this->owner->setAttribute($this->classAttribute, $class);
@@ -114,7 +117,7 @@ public function setPolymorphicRelation($object)
         }
     }
 
-    public static function getObjectModel(Model $object): string
+    public static function getObjectModel(ActiveRecordInterface $object): string
     {
         return $object instanceof ContentActiveRecord || $object instanceof ContentAddonActiveRecord
             ? $object::getObjectModel()
@@ -129,14 +132,25 @@ public function resetPolymorphicRelation()
         $this->cached = null;
     }
 
+    /**
+     * Returns if the polymorphic relation is established
+     *
+     * @since 1.16
+     * @noinspection PhpUnused
+     */
+    public function isPolymorphicRelationLoaded(): bool
+    {
+        return $this->cached !== null;
+    }
+
     /**
      * Validates if given object is of an allowed type
      *
      * @param mixed $object
      *
      * @return boolean
      */
-    private function validateUnderlyingObjectType($object)
+    private function validateUnderlyingObjectType(?object $object)
     {
         if (empty($this->mustBeInstanceOf)) {
             return true;
@@ -155,12 +169,12 @@ private function validateUnderlyingObjectType($object)
     /**
      * Loads an active record based on classname and primary key.
      *
-     * @param $className
-     * @param $primaryKey
+     * @param string $className
+     * @param string|int $primaryKey
      *
-     * @return null|ActiveRecord
+     * @return null|ActiveRecord|ActiveRecordInterface
      */
-    public static function loadActiveRecord($className, $primaryKey)
+    public static function loadActiveRecord(string $className, $primaryKey): ?ActiveRecordInterface
     {
         if (empty($className) || empty($primaryKey)) {
             return null;
@@ -173,12 +187,13 @@ public static function loadActiveRecord($className, $primaryKey)
             return null;
         }
 
-        if (!$class->isSubclassOf(BaseActiveRecord::class)) {
-            Yii::error('Could not load polymorphic relation! Class (Class is no ActiveRecord: ' . $className . ')');
+        if (!$class->implementsInterface(ActiveRecordInterface::class)) {
+            Yii::error('Could not load polymorphic relation! Class (Class does not implement ActiveRecordInterface: ' . $className . ')');
             return null;
         }
 
         try {
+            /** @var ActiveRecordInterface $className */
             $primaryKeyNames = $className::primaryKey();
             if (count($primaryKeyNames) !== 1) {
                 Yii::error('Could not load polymorphic relation! Only one primary key is supported!');

diff --git a/protected/humhub/libs/UUIDValidator.php b/protected/humhub/libs/UUIDValidator.php
@@ -88,6 +88,7 @@ class UUIDValidator extends StringValidator
      * @see static::$allowNull
      */
     protected $autofillWith = true;
+    public $skipOnEmpty = false;
 
     /**
      * @var string|null user-defined error message used when the value is blank
@@ -313,7 +314,7 @@ public function getAutofillWith()
     public function setAutofillWith($autofillWith): UUIDValidator
     {
         if ($autofillWith !== null) {
-             $bool = filter_var($autofillWith, FILTER_VALIDATE_BOOLEAN, FILTER_NULL_ON_FAILURE);
+            $bool = filter_var($autofillWith, FILTER_VALIDATE_BOOLEAN, FILTER_NULL_ON_FAILURE);
 
             if ($bool !== null && $autofillWith !== '') {
                 $autofillWith = $bool;

diff --git a/protected/humhub/modules/content/components/ContentContainerActiveRecord.php b/protected/humhub/modules/content/components/ContentContainerActiveRecord.php
@@ -12,13 +12,15 @@
 use humhub\libs\BasePermission;
 use humhub\libs\ProfileBannerImage;
 use humhub\libs\ProfileImage;
+use humhub\libs\UUID;
 use humhub\modules\content\models\Content;
 use humhub\modules\content\models\ContentContainer;
 use humhub\modules\content\models\ContentContainerBlockedUsers;
 use humhub\modules\content\models\ContentContainerTagRelation;
 use humhub\modules\user\models\User;
 use humhub\modules\user\Module as UserModule;
 use Yii;
+use yii\base\Component;
 use yii\db\ActiveQuery;
 use yii\helpers\Url;
 use yii\web\IdentityInterface;
@@ -223,15 +225,16 @@ public function afterSave($insert, $changedAttributes)
         if ($insert) {
             $contentContainer = new ContentContainer();
             $contentContainer->guid = $this->guid;
-            $contentContainer->class = static::class;
-            $contentContainer->pk = $this->getPrimaryKey();
+            $contentContainer->setPolymorphicRelation($this);
+
             if ($this instanceof User) {
                 $contentContainer->owner_user_id = $this->id;
             } elseif ($this->hasAttribute('created_by')) {
                 $contentContainer->owner_user_id = $this->created_by;
             }
 
             $contentContainer->save();
+            $this->populateRelation('contentContainerRecord', $contentContainer);
 
             $this->contentcontainer_id = $contentContainer->id;
             $this->update(false, ['contentcontainer_id']);

diff --git a/protected/humhub/modules/content/models/Content.php b/protected/humhub/modules/content/models/Content.php
@@ -15,6 +15,7 @@
 use humhub\interfaces\ArchiveableInterface;
 use humhub\interfaces\EditableInterface;
 use humhub\interfaces\ViewableInterface;
+use humhub\libs\UUIDValidator;
 use humhub\modules\admin\permissions\ManageUsers;
 use humhub\modules\content\activities\ContentCreated as ActivitiesContentCreated;
 use humhub\modules\content\components\ContentActiveRecord;
@@ -180,10 +181,10 @@ public function rules()
         return [
             [['object_id', 'visibility', 'pinned'], 'integer'],
             [['archived'], 'safe'],
-            [['guid'], 'string', 'max' => 45],
+            [['guid'], UUIDValidator::class],
+            [['guid'], 'unique'],
             [['object_model'], 'string', 'max' => 100],
             [['object_model', 'object_id'], 'unique', 'targetAttribute' => ['object_model', 'object_id'], 'message' => 'The combination of Object Model and Object ID has already been taken.'],
-            [['guid'], 'unique']
         ];
     }
 

diff --git a/protected/humhub/modules/content/models/ContentContainer.php b/protected/humhub/modules/content/models/ContentContainer.php
@@ -10,6 +10,7 @@
 
 use humhub\components\ActiveRecord;
 use humhub\components\behaviors\PolymorphicRelation;
+use humhub\libs\UUIDValidator;
 use humhub\modules\content\components\ContentContainerActiveRecord;
 
 /**
@@ -41,10 +42,16 @@ public function rules()
     {
         return [
             [['pk', 'owner_user_id'], 'integer'],
-            [['class', 'pk', 'guid'], 'required'],
-            [['guid', 'class'], 'string', 'max' => 255],
+            [['class', 'pk'], 'required'],
+            [['class'], 'string', 'max' => 255],
             [['class', 'pk'], 'unique', 'targetAttribute' => ['class', 'pk'], 'message' => 'The combination of Class and Pk has already been taken.'],
-            [['guid'], 'unique']
+            [['guid'],
+                UUIDValidator::class,
+                'autofillWith' => false,
+                'allowNull' => false,
+                'messageOnForbiddenNull' => 'Cannot not create standalone ContentContainer instance. Instance will be automatically created on ContentContainerActiveRecord::afterSave()'
+            ],
+            [['guid'], 'unique'],
         ];
     }
 

diff --git a/protected/humhub/modules/content/tests/codeception/unit/models/ContentContainerTest.php b/protected/humhub/modules/content/tests/codeception/unit/models/ContentContainerTest.php
@@ -38,14 +38,33 @@ public function testUniqueModel()
         $this->assertNotEmpty($contentContainer->getErrors('pk'));
     }
 
-    public function testGuidRequired()
+    public function testGuid()
     {
         $user = User::findOne(['id' => 1]);
+
+        // make sure we have a fresh ID and GUID
+        $user->id = 9;
+        $user->guid = UUID::v4();
+
         $contentContainer = new ContentContainer();
         $contentContainer->setPolymorphicRelation($user);
 
         $this->assertFalse($contentContainer->save());
         $this->assertNotEmpty($contentContainer->getErrors('guid'));
+
+        $user = User::findOne(['id' => 1]);
+
+        // make user appear as new
+        $user->setOldAttributes(null);
+        $user->id = null;
+        $user->guid = null;
+        $user->username = "SomeNewUser";
+        $user->email = "SomeNewUser@example.com";
+        $user->populateRelation('contentContainerRecord', null);
+
+        $this->assertTrue($user->save());
+        $this->assertEmpty($user->getErrors('guid'));
+        $this->assertEmpty($user->contentContainerRecord->getErrors('guid'));
     }
 
     public function testModelRequired()

diff --git a/protected/humhub/modules/file/migrations/m230618_135512_file_add_guid_unique_index.php b/protected/humhub/modules/file/migrations/m230618_135512_file_add_guid_unique_index.php
@@ -0,0 +1,33 @@
+<?php
+
+/*
+ * @link      https://www.humhub.org/
+ * @copyright Copyright (c) 2023 HumHub GmbH & Co. KG
+ * @license   https://www.humhub.com/licences
+ */
+
+use humhub\components\Migration;
+use humhub\modules\file\models\File;
+
+/**
+ * Add and film GUID column
+ */
+class m230618_135512_file_add_guid_unique_index extends Migration
+{
+    // protected properties
+    protected string $table;
+
+    public function __construct($config = [])
+    {
+        $this->table = File::tableName();
+        parent::__construct($config);
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function safeUp(): void
+    {
+        $this->safeCreateIndex("ux-$this->table-guid", $this->table, ['guid'], true);
+    }
+}
diff --git a/protected/humhub/modules/file/models/File.php b/protected/humhub/modules/file/models/File.php
@@ -13,6 +13,7 @@
 use humhub\components\behaviors\PolymorphicRelation;
 use humhub\interfaces\ViewableInterface;
 use humhub\libs\StdClass;
+use humhub\libs\UUIDValidator;
 use humhub\modules\content\components\ContentActiveRecord;
 use humhub\modules\content\components\ContentAddonActiveRecord;
 use humhub\modules\file\components\StorageManager;
@@ -142,6 +143,8 @@ public function rules()
             ],
             [['category', 'size', 'state', 'sort_order'], 'integer'],
             [['file_name', 'title'], 'string', 'max' => 255],
+            [['guid'], UUIDValidator::class],
+            [['guid'], 'unique'],
         ];
     }