From a515e0295375cd9d126f7ef02ada2867449c4ed8 Mon Sep 17 00:00:00 2001
From: Thomas Gelf <thomas@gelf.net>
Date: Tue, 20 Feb 2018 13:29:33 +0100
Subject: [PATCH] FilterByNameRestriction: test and unify behavior

fixes #1392
---
 .../Restriction/FilterByNameRestriction.php   | 30 ++--------
 .../Director/Restriction/MatchingFilter.php   | 40 +++++++++++++
 .../Web/Form/Validate/NamePattern.php         | 40 ++-----------
 .../Restriction/MatchingFilterTest.php        | 56 +++++++++++++++++++
 4 files changed, 108 insertions(+), 58 deletions(-)
 create mode 100644 library/Director/Restriction/MatchingFilter.php
 create mode 100644 test/php/library/Director/Restriction/MatchingFilterTest.php

diff --git a/library/Director/Restriction/FilterByNameRestriction.php b/library/Director/Restriction/FilterByNameRestriction.php
index f03022c9..fcf18022 100644
--- a/library/Director/Restriction/FilterByNameRestriction.php
+++ b/library/Director/Restriction/FilterByNameRestriction.php
@@ -39,7 +39,7 @@ class FilterByNameRestriction extends ObjectRestriction
             return true;
         }
 
-        return $this->prepareFilter()->matches([
+        return $this->getFilter()->matches([
             (object) ['object_name' => $object->getObjectName()]
         ]);
     }
@@ -47,7 +47,11 @@ class FilterByNameRestriction extends ObjectRestriction
     public function getFilter()
     {
         if ($this->filter === null) {
-            $this->filter = $this->prepareFilter();
+            $this->filter = MatchingFilter::forUser(
+                $this->auth->getUser(),
+                $this->name,
+                'object_name'
+            );
         }
 
         return $this->filter;
@@ -57,26 +61,4 @@ class FilterByNameRestriction extends ObjectRestriction
     {
         FilterRenderer::applyToQuery($this->getFilter(), $query);
     }
-
-    protected function prepareFilter()
-    {
-        $filter = Filter::matchAll();
-        foreach ($this->auth->getRestrictions($this->name) as $restriction) {
-            $filter->addFilter(Filter::expression('object_name', '=', $restriction));
-        }
-
-        return $filter;
-    }
-
-    protected function preparePrefixedFilter($prefix)
-    {
-        $filter = Filter::matchAll();
-        foreach ($this->auth->getRestrictions($this->name) as $restriction) {
-            $filter->addFilter(
-                Filter::expression("$prefix.object_name", '=', $restriction)
-            );
-        }
-
-        return $filter;
-    }
 }
diff --git a/library/Director/Restriction/MatchingFilter.php b/library/Director/Restriction/MatchingFilter.php
new file mode 100644
index 00000000..162840ce
--- /dev/null
+++ b/library/Director/Restriction/MatchingFilter.php
@@ -0,0 +1,40 @@
+<?php
+
+namespace Icinga\Module\Director\Restriction;
+
+use Icinga\Data\Filter\Filter;
+use Icinga\User;
+
+class MatchingFilter
+{
+    public static function forPatterns(array $restrictions, $columnName)
+    {
+        $filters = [];
+        foreach ($restrictions as $restriction) {
+            foreach (preg_split('/\|/', $restriction) as $pattern) {
+                $filters[] = Filter::expression(
+                    $columnName,
+                    '=',
+                    $pattern
+                );
+            }
+        }
+
+        if (count($filters) === 1) {
+            return $filters[0];
+        } else {
+            return Filter::matchAny($filters);
+        }
+    }
+
+    public static function forUser(
+        User $user,
+        $restrictionName,
+        $columnName
+    ) {
+        return static::forPatterns(
+            $user->getRestrictions($restrictionName),
+            $columnName
+        );
+    }
+}
diff --git a/library/Director/Web/Form/Validate/NamePattern.php b/library/Director/Web/Form/Validate/NamePattern.php
index ffe0d3c1..fac44d97 100644
--- a/library/Director/Web/Form/Validate/NamePattern.php
+++ b/library/Director/Web/Form/Validate/NamePattern.php
@@ -2,60 +2,32 @@
 
 namespace Icinga\Module\Director\Web\Form\Validate;
 
-use Icinga\Data\Filter\FilterMatch;
-use Icinga\Data\Filter\FilterOr;
+use Icinga\Module\Director\Restriction\MatchingFilter;
 use Zend_Validate_Abstract;
 
 class NamePattern extends Zend_Validate_Abstract
 {
     const INVALID = 'intInvalid';
 
-    private $pattern;
-
     private $filter;
 
     public function __construct($pattern)
     {
-        if (is_array($pattern) && count($pattern) === 1) {
-            $this->pattern = current($pattern);
-        } else {
-            $this->pattern = $pattern;
+        if (! is_array($pattern)) {
+            $pattern = [$pattern];
         }
 
-        if (is_array($this->pattern)) {
-            $msg = implode(' | ', $this->pattern);
-        } else {
-            $msg = $this->pattern;
-        }
+        $this->filter = MatchingFilter::forPatterns($pattern, 'value');
 
         $this->_messageTemplates[self::INVALID] = sprintf(
             'Does not match %s',
-            $msg
+            (string) $this->filter
         );
     }
 
-    protected function matches($value)
-    {
-        if ($this->filter === null) {
-            if (is_array($this->pattern)) {
-                $this->filter = new FilterOr();
-                foreach ($this->pattern as $pattern) {
-                    $filter = new FilterMatch('prop', '=', $pattern);
-                    $filter->setCaseSensitive(false);
-                    $this->filter->addFilter($filter);
-                }
-            } else {
-                $this->filter = new FilterMatch('prop', '=', $this->pattern);
-                $this->filter->setCaseSensitive(false);
-            }
-        }
-
-        return $this->filter->matches($value);
-    }
-
     public function isValid($value)
     {
-        if ($this->matches((object) ['prop' => $value])) {
+        if ($this->filter->matches((object) ['value' => $value])) {
             return true;
         } else {
             $this->_error(self::INVALID, $value);
diff --git a/test/php/library/Director/Restriction/MatchingFilterTest.php b/test/php/library/Director/Restriction/MatchingFilterTest.php
new file mode 100644
index 00000000..cd1b57eb
--- /dev/null
+++ b/test/php/library/Director/Restriction/MatchingFilterTest.php
@@ -0,0 +1,56 @@
+<?php
+
+namespace Tests\Icinga\Module\Director\Restriction;
+
+use Icinga\Module\Director\Restriction\MatchingFilter;
+use Icinga\Module\Director\Test\BaseTestCase;
+use Icinga\User;
+
+class MatchingFilterTest extends BaseTestCase
+{
+    public function testUserWithNoRestrictionHasNoFilter()
+    {
+        $user = new User('dummy');
+        $this->assertEquals(
+            '',
+            (string) MatchingFilter::forUser($user, 'some/name', 'prop')
+        );
+    }
+
+    public function testSimpleRestrictionRendersCorrectly()
+    {
+        $this->assertEquals(
+            'prop = a*',
+            (string) MatchingFilter::forPatterns(['a*'], 'prop')
+        );
+    }
+
+    public function testMultipleRestrictionsAreCombinedWithOr()
+    {
+        $this->assertEquals(
+            'prop = a* | prop = *b',
+            (string) MatchingFilter::forPatterns(['a*', '*b'], 'prop')
+        );
+    }
+
+    public function testUserWithMultipleRestrictionsWorksFine()
+    {
+        $user = new User('dummy');
+        $user->setRestrictions([
+            'some/name' => ['a*', '*b'],
+            'some/thing' => ['else']
+        ]);
+        $this->assertEquals(
+            'prop = a* | prop = *b',
+            (string) MatchingFilter::forUser($user, 'some/name', 'prop')
+        );
+    }
+
+    public function testSingleRestrictionAllowsForPipes()
+    {
+        $this->assertEquals(
+            'prop = a* | prop = *b',
+            (string) MatchingFilter::forPatterns(['a*|*b'], 'prop')
+        );
+    }
+}