Cleanup attributes code, change Role constructor signature to variadi…

…c parameter
nepada · May 15, 2022 · 899e002 · 899e002
1 parent 637e851
commit 899e002
Show file tree

Hide file tree

Showing 10 changed files with 75 additions and 33 deletions.
diff --git a/README.md b/README.md
@@ -52,7 +52,7 @@ class SecuredPresenter extends Nette\Application\UI\Presenter
 
     use Nepada\SecurityAnnotations\SecurityAnnotations;
 
-    #[Role(["admin", "superadmin"])]
+    #[Role("admin", "superadmin")]
     public function actionForAdmins(): void
     {
         // Only users with role admin or superadmin are allowed here.

diff --git a/composer.json b/composer.json
@@ -20,7 +20,7 @@
         "nette/component-model": "^3.0.2@dev",
         "nette/application": "^3.1.4@dev",
         "nette/security": "^3.0@dev",
-        "doctrine/annotations": "^1.11.0@dev"
+        "doctrine/annotations": "^1.12.0@dev"
     },
     "require-dev": {
         "nette/tester": "2.4.2",

diff --git a/src/SecurityAnnotations/Annotations/Allowed.php b/src/SecurityAnnotations/Annotations/Allowed.php
@@ -17,17 +17,9 @@ final class Allowed implements NamedArgumentConstructorAnnotation
 
     use Nette\SmartObject;
 
-    /**
-     * @internal use getter instead
-     * @var string
-     */
-    public ?string $resource;
-
-    /**
-     * @internal use getter instead
-     * @var string
-     */
-    public ?string $privilege;
+    private ?string $resource;
+
+    private ?string $privilege;
 
     public function __construct(?string $resource = Nette\Security\IAuthorizator::ALL, ?string $privilege = Nette\Security\IAuthorizator::ALL)
     {

diff --git a/src/SecurityAnnotations/Annotations/Role.php b/src/SecurityAnnotations/Annotations/Role.php
@@ -12,28 +12,42 @@
  * @Target({"CLASS","METHOD"})
  */
 #[Attribute(Attribute::TARGET_CLASS | Attribute::TARGET_METHOD | Attribute::IS_REPEATABLE)]
-final class Role implements NamedArgumentConstructorAnnotation
+final class Role
 {
 
     use Nette\SmartObject;
 
     /**
-     * @Required
-     * @internal use getter instead
-     * @var array<string>
+     * @var non-empty-list<string>
      */
-    public array $roles;
+    private array $roles;
 
     /**
-     * @param string|array<string> $value
+     * @param array{value: string|non-empty-list<string>}|non-empty-list<string>|string ...$roles
      */
-    public function __construct(string|array $value)
+    public function __construct(string|array ...$roles)
     {
-        $this->roles = is_string($value) ? [$value] : $value;
+        if (isset($roles[0]['value'])) { // Compatibility with Doctrine annotations
+            $roles = is_string($roles[0]['value']) ? [$roles[0]['value']] : $roles[0]['value'];
+        } elseif (func_num_args() === 1 && is_array(func_get_arg(0))) { // BC with passing roles as an array
+            trigger_error('Passing roles as a single array argument is deprecated, use variadic argument instead', E_USER_DEPRECATED);
+            $roles = func_get_arg(0);
+        }
+        if ($roles === []) {
+            throw new \InvalidArgumentException('At least one role name must be specified');
+        }
+        foreach ($roles as $role) {
+            if (! is_string($role)) {
+                $type = gettype($role);
+                throw new \InvalidArgumentException("Expected string with role name, got {$type}");
+            }
+        }
+
+        $this->roles = $roles;
     }
 
     /**
-     * @return string[]
+     * @return non-empty-list<string>
      */
     public function getRoles(): array
     {

diff --git a/tests/SecurityAnnotations/AccessValidators/RoleValidatorTest.phpt b/tests/SecurityAnnotations/AccessValidators/RoleValidatorTest.phpt
@@ -51,12 +51,12 @@ class RoleValidatorTest extends TestCase
                 'rolesInheritance' => null,
             ],
             [
-                'annotation' => new Role(['xyz', 'bar', 'abc']),
+                'annotation' => new Role('xyz', 'bar', 'abc'),
                 'userRoles' => ['foo', 'bar', 'baz'],
                 'rolesInheritance' => null,
             ],
             [
-                'annotation' => new Role(['abc', 'cde']),
+                'annotation' => new Role('abc', 'cde'),
                 'userRoles' => ['foo', 'bar', 'baz'],
                 'rolesInheritance' => ['foo' => ['xyz'], 'bar' => ['abc']],
             ],
@@ -92,12 +92,12 @@ class RoleValidatorTest extends TestCase
                 'rolesInheritance' => null,
             ],
             [
-                'annotation' => new Role(['required', 'another']),
+                'annotation' => new Role('required', 'another'),
                 'userRoles' => ['baz'],
                 'rolesInheritance' => null,
             ],
             [
-                'annotation' => new Role(['required', 'another']),
+                'annotation' => new Role('required', 'another'),
                 'userRoles' => ['foo', 'bar', 'baz'],
                 'rolesInheritance' => ['foo' => ['xyz'], 'bar' => ['abc']],
             ],

diff --git a/tests/SecurityAnnotations/AnnotationReaders/AttributesReaderTest.phpt b/tests/SecurityAnnotations/AnnotationReaders/AttributesReaderTest.phpt
@@ -24,14 +24,31 @@ class AttributesReaderTest extends TestCase
         $expected = [
             new SecurityAnnotations\Annotations\LoggedIn(),
             new SecurityAnnotations\Annotations\Role('lorem'),
-            new SecurityAnnotations\Annotations\Role(['foo', 'bar']),
+            new SecurityAnnotations\Annotations\Role('foo', 'bar'),
             new SecurityAnnotations\Annotations\Allowed('foo', 'bar'),
             new SecurityAnnotations\Annotations\Allowed(null, 'shiny'),
         ];
         $actual = $reader->getAll(new \ReflectionClass(TestAnnotationsPresenter::class));
         Assert::equal($expected, $actual);
     }
 
+    public function testDeprecatedSyntax(): void
+    {
+        $reader = new SecurityAnnotations\AnnotationReaders\AttributesReader();
+
+        $expected = [
+            new SecurityAnnotations\Annotations\Role('foo', 'bar'),
+        ];
+        Assert::error(
+            function () use ($reader, $expected): void {
+                $actual = $reader->getAll(new \ReflectionMethod(TestAnnotationsPresenter::class, 'deprecated'));
+                Assert::equal($expected, $actual);
+            },
+            E_USER_DEPRECATED,
+            'Passing roles as a single array argument is deprecated, use variadic argument instead',
+        );
+    }
+
 }
 
 

diff --git a/tests/SecurityAnnotations/AnnotationReaders/DoctrineAnnotationsReaderTest.phpt b/tests/SecurityAnnotations/AnnotationReaders/DoctrineAnnotationsReaderTest.phpt
@@ -25,7 +25,7 @@ class DoctrineAnnotationsReaderTest extends TestCase
 
         $expected = [
             new SecurityAnnotations\Annotations\LoggedIn(),
-            new SecurityAnnotations\Annotations\Role(['a', 'b', 'c']),
+            new SecurityAnnotations\Annotations\Role('a', 'b', 'c'),
             new SecurityAnnotations\Annotations\Role('d'),
             new SecurityAnnotations\Annotations\Allowed('foo', 'bar'),
             new SecurityAnnotations\Annotations\Allowed(null, 'shiny'),
@@ -34,6 +34,17 @@ class DoctrineAnnotationsReaderTest extends TestCase
         Assert::equal($expected, $actual);
     }
 
+    public function testDeprecatedSyntax(): void
+    {
+        $reader = new SecurityAnnotations\AnnotationReaders\DoctrineAnnotationsReader(new AnnotationReader(new DocParser()));
+
+        $expected = [
+            new SecurityAnnotations\Annotations\Role('a', 'b', 'c'),
+        ];
+        $actual = $reader->getAll(new \ReflectionMethod(TestAnnotationsPresenter::class, 'deprecated'));
+        Assert::equal($expected, $actual);
+    }
+
 }
 
 

diff --git a/tests/SecurityAnnotations/AnnotationReaders/UnionReaderTest.phpt b/tests/SecurityAnnotations/AnnotationReaders/UnionReaderTest.phpt
@@ -27,13 +27,13 @@ class UnionReaderTest extends TestCase
         $expected = [
             new SecurityAnnotations\Annotations\LoggedIn(),
             new SecurityAnnotations\Annotations\Role('lorem'),
-            new SecurityAnnotations\Annotations\Role(['foo', 'bar']),
+            new SecurityAnnotations\Annotations\Role('foo', 'bar'),
             new SecurityAnnotations\Annotations\Allowed('foo', 'bar'),
             new SecurityAnnotations\Annotations\Allowed(null, 'shiny'),
 
             new SecurityAnnotations\Annotations\LoggedIn(),
             new SecurityAnnotations\Annotations\Role('lorem'),
-            new SecurityAnnotations\Annotations\Role(['foo', 'bar']),
+            new SecurityAnnotations\Annotations\Role('foo', 'bar'),
             new SecurityAnnotations\Annotations\Allowed('foo', 'bar'),
             new SecurityAnnotations\Annotations\Allowed(null, 'shiny'),
         ];

diff --git a/tests/SecurityAnnotations/Fixtures/TestAnnotationsPresenter.php b/tests/SecurityAnnotations/Fixtures/TestAnnotationsPresenter.php
@@ -10,18 +10,26 @@
 
 /**
  * @LoggedIn
- * @Role({"a", "b", "c"})
+ * @Role("a", "b", "c")
  * @Role("d")
  * @Allowed(resource="foo", privilege="bar")
  * @Allowed(privilege="shiny")
  * @author Foo Bar
  */
 #[LoggedIn()]
 #[Role('lorem')]
-#[Role(['foo', 'bar'])]
+#[Role('foo', 'bar')]
 #[Allowed('foo', 'bar')]
 #[Allowed(privilege: 'shiny')]
 class TestAnnotationsPresenter extends Nette\Application\UI\Presenter
 {
 
+    /**
+     * @Role({"a", "b", "c"})
+     */
+    #[Role(['foo', 'bar'])]
+    public function deprecated(): void
+    {
+    }
+
 }
diff --git a/tests/SecurityAnnotations/RequirementsCheckerTest.phpt b/tests/SecurityAnnotations/RequirementsCheckerTest.phpt
@@ -44,7 +44,7 @@ class RequirementsCheckerTest extends TestCase
     {
         $loggedIn = new LoggedIn();
         $role1 = new Role('foo');
-        $role2 = new Role(['a', 'b']);
+        $role2 = new Role('a', 'b');
         $allowed = new Allowed();
         $annotationsReader = new DummyAnnotationReader([$loggedIn, $role1, $role2, $allowed]);