Fix recursion in ACL model (#114)

* fix recursion in ACL model * move normalization to getRules() and make it protected * fix phpdoc types * try only after assign is working * improve value normalize/split * improve ternary coverage * fix non-empty array after empty string explode * field value is never array * remove not implemented conditions field * fix ui add new rule --------- Co-authored-by: Michael Voříšek <[email protected]>
atk4 · Apr 6, 2024 · 24f3b51 · 24f3b51
1 parent edec0f8
commit 24f3b51
Show file tree

Hide file tree

Showing 12 changed files with 63 additions and 69 deletions.
diff --git a/.php-cs-fixer.dist.php b/.php-cs-fixer.dist.php
@@ -51,9 +51,6 @@
         'general_phpdoc_annotation_remove' => [
             'annotations' => ['author', 'copyright', 'throws'],
         ],
-        'nullable_type_declaration_for_default_null_value' => [
-            'use_nullable_type_declaration' => false,
-        ],
 
         // fn => without curly brackets is less readable,
         // also prevent bounding of unwanted variables for GC

diff --git a/demos/_demo-data/create-db.php b/demos/_demo-data/create-db.php
@@ -44,18 +44,17 @@
 $model->addField('role_id', ['type' => 'integer']);
 $model->addField('model', ['type' => 'string']);
 $model->addField('all_visible', ['type' => 'boolean']);
-$model->addField('visible_fields', ['type' => 'boolean']);
+$model->addField('visible_fields', ['type' => 'string']);
 $model->addField('all_editable', ['type' => 'boolean']);
-$model->addField('editable_fields', ['type' => 'boolean']);
+$model->addField('editable_fields', ['type' => 'string']);
 $model->addField('all_actions', ['type' => 'boolean']);
-$model->addField('actions', ['type' => 'boolean']);
-$model->addField('conditions', ['type' => 'boolean']);
+$model->addField('actions', ['type' => 'string']);
 
 (new Migrator($model))->create();
 $model->import([
-    ['id' => 1, 'role_id' => 1, 'model' => User::class, 'all_visible' => 1, 'visible_fields' => null, 'all_editable' => 0, 'editable_fields' => null, 'all_actions' => 1, 'actions' => null, 'conditions' => null],
-    ['id' => 2, 'role_id' => 2, 'model' => User::class, 'all_visible' => 1, 'visible_fields' => null, 'all_editable' => 1, 'editable_fields' => null, 'all_actions' => 1, 'actions' => null, 'conditions' => null],
-    ['id' => 3, 'role_id' => 2, 'model' => Role::class, 'all_visible' => 1, 'visible_fields' => null, 'all_editable' => 1, 'editable_fields' => null, 'all_actions' => 1, 'actions' => null, 'conditions' => null],
+    ['id' => 1, 'role_id' => 1, 'model' => User::class, 'all_visible' => true, 'visible_fields' => null, 'all_editable' => false, 'editable_fields' => null, 'all_actions' => true, 'actions' => null],
+    ['id' => 2, 'role_id' => 2, 'model' => User::class, 'all_visible' => true, 'visible_fields' => null, 'all_editable' => true, 'editable_fields' => null, 'all_actions' => true, 'actions' => null],
+    ['id' => 3, 'role_id' => 2, 'model' => Role::class, 'all_visible' => true, 'visible_fields' => null, 'all_editable' => true, 'editable_fields' => null, 'all_actions' => true, 'actions' => null],
 ]);
 
 $model = new Model($db, ['table' => 'demo_client']);

diff --git a/demos/acl-clients.php b/demos/acl-clients.php
@@ -22,4 +22,7 @@
 Message::addTo($app, ['type' => 'info'])
     ->set('This is how an ACL managed app will look like based on logged in user and his role and permissions.');
 
+Message::addTo($app, ['type' => 'info'])
+    ->set('Currently logged-in user: ' . $app->auth->user->getTitle());
+
 Crud::addTo($app)->setModel(new Model\Client($app->db));
diff --git a/src/Acl.php b/src/Acl.php
@@ -6,7 +6,6 @@
 
 use Atk4\Data\Exception;
 use Atk4\Data\Model;
-use Atk4\Login\Model\AccessRule;
 use Atk4\Login\Model\User;
 
 /**
@@ -22,12 +21,14 @@ class Acl
      */
     public $auth;
 
+    private bool $skipApplyRestrictionsForRulesExport = false;
+
     /**
-     * Returns AccessRules model for logged in user and in model scope.
+     * Returns array of AccessRules records for logged in user and in particular model scope.
      *
-     * @return AccessRule
+     * @return list<array{model: class-string<Model>, all_visible: bool, visible_fields: list<string>, all_editable: bool, editable_fields: list<string>, all_actions: bool, actions: list<string>}>
      */
-    public function getRules(Model $model)
+    protected function getRules(Model $model): array
     {
         /** @var User */
         $user = $this->auth->user;
@@ -45,9 +46,33 @@ public function getRules(Model $model)
                 $modelClasses[] = $class;
             }
         } while (($class = get_parent_class($class)) !== false);
-        $res = $user->ref('AccessRules')->addCondition('model', 'in', $modelClasses);
 
-        return $res; // @phpstan-ignore-line
+        $this->skipApplyRestrictionsForRulesExport = true;
+        try {
+            $rules = $user->ref('AccessRules')
+                ->addCondition('model', 'in', $modelClasses)
+                ->export(['model', 'all_visible', 'visible_fields', 'all_editable', 'editable_fields', 'all_actions', 'actions']);
+
+            foreach ($rules as $k => $rule) {
+                $rules[$k]['visible_fields'] = $rule['all_visible'] ? [] : $this->explodeValue($rule['visible_fields']);
+                $rules[$k]['editable_fields'] = $rule['all_editable'] ? [] : $this->explodeValue($rule['editable_fields']);
+                $rules[$k]['actions'] = $rule['all_actions'] ? [] : $this->explodeValue($rule['actions']);
+            }
+        } finally {
+            $this->skipApplyRestrictionsForRulesExport = false;
+        }
+
+        return $rules;
+    }
+
+    /**
+     * @return list<string>
+     */
+    private function explodeValue(?string $v): array
+    {
+        return ($v ?? '') === ''
+            ? []
+            : explode(',', $v);
     }
 
     /**
@@ -57,58 +82,28 @@ public function getRules(Model $model)
      */
     public function applyRestrictions(Model $m): void
     {
-        foreach ($this->getRules($m) as $rule) {
-            // extract as arrays
-            $visible = is_array($rule->get('visible_fields')) ? $rule->get('visible_fields') : explode(',', $rule->get('visible_fields') ?? '');
-            $editable = is_array($rule->get('editable_fields')) ? $rule->get('editable_fields') : explode(',', $rule->get('editable_fields') ?? '');
-            $actions = is_array($rule->get('actions')) ? $rule->get('actions') : explode(',', $rule->get('actions') ?? '');
+        if ($this->skipApplyRestrictionsForRulesExport) {
+            return;
+        }
 
+        foreach ($this->getRules($m) as $rule) {
             // set visible and editable fields
             foreach ($m->getFields() as $name => $field) {
-                if (!$rule->get('all_visible') && $visible) {
-                    $field->ui['visible'] = array_search($name, $visible, true) !== false;
+                if ($rule['visible_fields'] !== [] && array_search($name, $rule['visible_fields'], true) === false) {
+                    $field->ui['visible'] = false;
                 }
-                if (!$rule->get('all_editable') && $editable) {
-                    $field->ui['editable'] = array_search($name, $editable, true) !== false;
+                if ($rule['editable_fields'] !== [] && array_search($name, $rule['editable_fields'], true) === false) {
+                    $field->ui['editable'] = false;
                 }
             }
 
             // remove not allowed actions
-            if (!$rule->get('all_actions') && $actions) {
-                $actions_to_remove = array_diff(array_keys($m->getUserActions()), $actions);
+            if ($rule['actions'] !== []) {
+                $actions_to_remove = array_diff(array_keys($m->getUserActions()), $rule['actions']);
                 foreach ($actions_to_remove as $action) {
                     $m->getUserAction($action)->enabled = false;
                 }
             }
-
-            // add conditions on model
-            /* this will work in future when we will have json encoded condition structure stored in here
-            if ($rule['conditions']) {
-                $this->applyConditions($m, $rule['conditions']);
-            }
-            */
         }
     }
-
-    /**
-     * Apply conditions on model.
-     *
-     * @param mixed $conditions
-     */
-    public function applyConditions(Model $m, $conditions): void
-    {
-        $m->addCondition($conditions);
-    }
-
-    // Call $app->acl->can('admin'); for example to find out if user is allowed to admin things.
-    /*
-    public function can($feature)
-    {
-        if (!$this->permissions) {
-            $this->cachePermissions();
-        }
-
-        return $this->permissions[$feature] ?? false;
-    }
-    */
 }
diff --git a/src/Auth.php b/src/Auth.php
@@ -117,7 +117,7 @@ public function __construct(App $app, $options = [])
      *
      * @return $this
      */
-    public function setModel(Model $model, string $fieldLogin = null, string $fieldPassword = null)
+    public function setModel(Model $model, ?string $fieldLogin = null, ?string $fieldPassword = null)
     {
         if ($this->user !== null) {
             throw new Exception('Model is already set');
@@ -231,7 +231,7 @@ public function logout(): void
      *
      * @return $this
      */
-    public function setAcl(Acl $acl, Persistence $persistence = null)
+    public function setAcl(Acl $acl, ?Persistence $persistence = null)
     {
         $persistence ??= $this->user->getModel()->getPersistence();
         $acl->auth = $this;

diff --git a/src/Feature/SetupAccessRuleModelTrait.php b/src/Feature/SetupAccessRuleModelTrait.php
@@ -35,10 +35,10 @@ protected function init()
         $this->getField('visible_fields')->ui['form'] = [Control\Fields::class];
         $this->getField('editable_fields')->ui['form'] = [Control\Fields::class];
         $this->getField('actions')->ui['form'] = [Control\Actions::class];
-        $this->getField('conditions')->type = 'text';
 
         // cleanup data
         $this->onHook(Model::HOOK_BEFORE_SAVE, static function (self $m) {
+            // @todo this is not always right way to normalize data
             if ($m->get('all_visible')) {
                 $m->setNull('visible_fields');
             }

diff --git a/src/Form/Control/Actions.php b/src/Form/Control/Actions.php
@@ -30,6 +30,8 @@ protected function renderView(): void
         if ($model) {
             $actions = array_keys($model->getUserActions());
             $this->values = array_combine($actions, $actions);
+        } else {
+            $this->values = [];
         }
 
         parent::renderView();

diff --git a/src/Form/Control/Fields.php b/src/Form/Control/Fields.php
@@ -30,6 +30,8 @@ protected function renderView(): void
         if ($model) {
             $fields = array_keys($model->getFields());
             $this->values = array_combine($fields, $fields);
+        } else {
+            $this->values = [];
         }
 
         parent::renderView();

diff --git a/src/Form/Register.php b/src/Form/Register.php
@@ -38,7 +38,7 @@ protected function init(): void
     }
 
     #[\Override]
-    public function setModel(Model $user, array $fields = null): void
+    public function setModel(Model $user, ?array $fields = null): void
     {
         parent::setModel($user, []);
 

diff --git a/src/Model/AccessRule.php b/src/Model/AccessRule.php
@@ -35,7 +35,7 @@ protected function init(): void
         $this->addField('model'); // model class name
 
         /*
-         * @TODO maybe all_visible and visible_fields can be replaced with just on field visible:
+         * @TODO maybe all_visible and visible_fields can be replaced with just one field visible:
          *      '*' - equals all_fields=true
          *      'foo,bar' - equals visible_fields='foo,bar' or visible_fields=['foo', 'bar']
          *
@@ -62,10 +62,6 @@ protected function init(): void
         $this->addField('all_actions', ['type' => 'boolean']);
         $this->addField('actions'); // used if all_actions is false
 
-        // Specify which conditions will be applied on the model, e.g. "status=DRAFT AND sent=true OR status=SENT"
-        // @TODO this will be replaced by JSON structure when Alain will develop such JS widget
-        $this->addField('conditions');
-
         $this->setupAccessRuleModel();
     }
 }
diff --git a/src/RoleAdmin.php b/src/RoleAdmin.php
@@ -23,7 +23,7 @@ class RoleAdmin extends Crud
      * Initialize User Admin and add all the UI pieces.
      */
     #[\Override]
-    public function setModel(Model $role, array $fields = null): void
+    public function setModel(Model $role, ?array $fields = null): void
     {
         parent::setModel($role);
 

diff --git a/tests/GenericTestCase.php b/tests/GenericTestCase.php
@@ -78,9 +78,9 @@ protected function setupDefaultDb(): void
                 ['id' => 2, 'name' => 'Administrator', 'email' => 'admin', 'password' => '$2y$10$p34ciRcg9GZyxukkLIaEnenGBao79fTFa4tFSrl7FvqrxnmEGlD4O' /* admin */, 'role_id' => 2, 'last_login' => null],
             ],
             self::getTableByStandardModelClass(AccessRule::class) => [
-                ['id' => 1, 'role_id' => 1, 'model' => User::class, 'all_visible' => true, 'visible_fields' => null, 'all_editable' => false, 'editable_fields' => 'vat_number,active', 'all_actions' => true, 'actions' => null, 'conditions' => null],
-                ['id' => 2, 'role_id' => 2, 'model' => User::class, 'all_visible' => true, 'visible_fields' => null, 'all_editable' => false, 'editable_fields' => null, 'all_actions' => true, 'actions' => null, 'conditions' => null],
-                ['id' => 3, 'role_id' => 2, 'model' => Role::class, 'all_visible' => true, 'visible_fields' => null, 'all_editable' => true, 'editable_fields' => null, 'all_actions' => true, 'actions' => null, 'conditions' => null],
+                ['id' => 1, 'role_id' => 1, 'model' => User::class, 'all_visible' => true, 'visible_fields' => null, 'all_editable' => false, 'editable_fields' => 'vat_number,active', 'all_actions' => true, 'actions' => null],
+                ['id' => 2, 'role_id' => 2, 'model' => User::class, 'all_visible' => true, 'visible_fields' => null, 'all_editable' => false, 'editable_fields' => null, 'all_actions' => true, 'actions' => null],
+                ['id' => 3, 'role_id' => 2, 'model' => Role::class, 'all_visible' => true, 'visible_fields' => null, 'all_editable' => true, 'editable_fields' => null, 'all_actions' => true, 'actions' => null],
             ],
         ]);
     }