From 6737cd0e227a8790c6044aa5b51b798086f13af1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20Mor=C3=A1vek?= Date: Sun, 15 May 2022 00:21:16 +0200 Subject: [PATCH] Deprecate Doctrine annotations --- README.md | 56 +++++-------------- .../SecurityAnnotationsExtension.php | 4 ++ .../config.doctrine-annotations-enabled.neon | 2 + .../Fixtures/config.neon | 3 + .../SecurityAnnotationsExtensionTest.phpt | 4 +- .../LoggedInValidatorTest.phpt | 18 +----- .../PermissionValidatorTest.phpt | 45 ++++----------- .../AccessValidators/RoleValidatorTest.phpt | 41 ++++---------- 8 files changed, 51 insertions(+), 122 deletions(-) create mode 100644 tests/Bridges/SecurityAnnotationsDI/Fixtures/config.doctrine-annotations-enabled.neon diff --git a/README.md b/README.md index 61203b2..c3443ae 100644 --- a/README.md +++ b/README.md @@ -23,7 +23,9 @@ extensions: securityAnnotations: Nepada\Bridges\SecurityAnnotationsDI\SecurityAnnotationsExtension ``` -The package relies on [doctrine/annotations](https://packagist.org/packages/doctrine/annotations) for parsing annotations. It's up to you to choose and set up the integration, the recommended package to do the job is [nettrine/annotations](https://packagist.org/packages/nettrine/annotations). +For parsing phpdoc annotation this package relies on [doctrine/annotations](https://packagist.org/packages/doctrine/annotations) for parsing annotations. It's up to you to choose and set up the integration, the recommended package to do the job is [nettrine/annotations](https://packagist.org/packages/nettrine/annotations). + +**Note: annotations driver is deprecated and will be removed in next major release. Migrate all your annotations to native PHP8 attributes and set `enableDoctrineAnnotations: false` in your config.** Usage @@ -32,7 +34,7 @@ Usage This package builds on top of the standard access authorization of Nette components, namely `Nette\Application\UI\Component::checkRequirements()` method. This method is called before invoking any of component/presenter signal handlers, and before presenter `startup`, `action<>` and `render<>` methods. -With this package you can specify the access rules via annotations on any of the mentioned methods, or on presenter class. +With this package you can specify the access rules via attributes on any of the mentioned methods, or on presenter class. To enable this feature simple use `SecurityAnnotations` trait in any presenter or component and make sure `RequirementsChecker` service gets injected via `injectRequirementsChecker()` - with default Nette configuration this should work on presenters out of the box, but you need to take care of components, e.g. by enabling inject calls. **Example:** @@ -42,25 +44,21 @@ use Nepada\SecurityAnnotations\Annotations\LoggedIn; use Nepada\SecurityAnnotations\Annotations\Role; /** - * @LoggedIn * To access this presenter the user must be logged in. */ + #[LoggedIn] 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. } - /** - * @Allowed(resource="world", privilege="destroy") - */ + #[Allowed(resource: "world", privilege: "destroy")] public function handleDestroyWorld(): void { // Only users with specific permission are allowed to call this signal. @@ -69,12 +67,12 @@ class SecuredPresenter extends Nette\Application\UI\Presenter } ``` -The annotations and rules they enforce are completely customizable (see below), however the default setup comes with three predefined rules: +The attributes and rules they enforce are completely customizable (see below), however the default setup comes with three predefined rules: -- **@LoggedIn** - checks whether the user is logged in. -- **@Role({"admin", "superadmin"})** - checks whether the user has at least one of the specified roles. +- **LoggedIn** - checks whether the user is logged in. +- **Role(["admin", "superadmin"])** - checks whether the user has at least one of the specified roles. If you use `Nette\Security\Permission` as your authorizator, then role inheritance is taken into account, i.e. users that have at least one role that inherits from at least one of the specified roles are allowed as well. -- **@Allowed(resource="world", privilege="destroy")** - checks whether the user has at least one role that is granted the specified privilege on the specified resource. +- **Allowed(resource: "world", privilege: "destroy")** - checks whether the user has at least one role that is granted the specified privilege on the specified resource. ### Securing components @@ -89,9 +87,7 @@ class SecuredPresenter extends Nette\Application\UI\Presenter use Nepada\SecurityAnnotations\SecurityAnnotations; - /** - * @LoggedIn - */ + #[LoggedIn] public function actionDefault(): void { // ... @@ -112,7 +108,7 @@ class SecuredPresenter extends Nette\Application\UI\Presenter Securing presenter `action<>` (or `render<>`) methods is not sufficient! All it takes is a one general route in your router, e.g. a very common `Route('/')`, and anyone may successfully submit the form by sending POST request to `/secured/foo` URL. -You should always check user's permissions when creating the component. To make your life easier there is `SecuredComponents` trait that calls the standard `Nette\Application\UI\Component::checkRequirements()` method before calling the component factory. Combining it with `SecurityAnnotations` it allows you to control access to components via annotations on `createComponent<>` methods. +You should always check user's permissions when creating the component. To make your life easier there is `SecuredComponents` trait that calls the standard `Nette\Application\UI\Component::checkRequirements()` method before calling the component factory. Combining it with `SecurityAnnotations` it allows you to control access to components via attributes on `createComponent<>` methods. ### Customizing access validators @@ -133,28 +129,6 @@ services: #### How do access validators work? -Every access validator implements `Nepada\SecurityAnnotations\AccessValidators\AccessValidator` interface. The access validator specifies which annotation type it supports via its public API. - -When checking the requirements PHP attributes and all annotations parsed using `doctrine/annotations` are passed one by one to associated access validator for inspection. Based on the annotation value the validator decides either to deny access (throws `Nette\Application\BadRequestException`), or grant access (no exception is thrown). - - -### PHP attributes support - -Security annotations may be defined also via standard PHP attributes (introduced in PHP 8.0), e.g. -```php -use Nepada\SecurityAnnotations\Annotations\LoggedIn; - -#[LoggedIn()] -class SecuredPresenter extends Nette\Application\UI\Presenter -{ - // ... -} -``` +Every access validator implements `Nepada\SecurityAnnotations\AccessValidators\AccessValidator` interface. The access validator specifies which attribute type it supports via its public API. -PHP attributes are the preferred way of specifying your security metadata. The support for legacy PHP DocBlock annotations will be removed in the future major version. - -If you do not use legacy PHP DocBlock annotations, consider completely disabling doctrine annotations reader: -```yaml -securityAnnotations: - enableDoctrineAnnotations: false -``` +When checking the requirements PHP attributes and all annotations parsed using `doctrine/annotations` are passed one by one to associated access validator for inspection. Based on the attribute value the validator decides either to deny access (throws `Nette\Application\BadRequestException`), or grant access (no exception is thrown). diff --git a/src/Bridges/SecurityAnnotationsDI/SecurityAnnotationsExtension.php b/src/Bridges/SecurityAnnotationsDI/SecurityAnnotationsExtension.php index 43ab45c..7c505a1 100644 --- a/src/Bridges/SecurityAnnotationsDI/SecurityAnnotationsExtension.php +++ b/src/Bridges/SecurityAnnotationsDI/SecurityAnnotationsExtension.php @@ -47,6 +47,10 @@ public function loadConfiguration(): void ->setType(AttributesReader::class) ->setAutowired(AttributesReader::class); if ($this->config->enableDoctrineAnnotations) { + trigger_error( + 'Using Doctrine annotations is deprecated, migrate to native PHP8 attributes and set enableDoctrineAnnotations: false in your config', + E_USER_DEPRECATED, + ); $readers[] = $container->addDefinition($this->prefix('doctrineAnnotationsReader'), new ServiceDefinition()) ->setType(DoctrineAnnotationsReader::class) ->setAutowired(DoctrineAnnotationsReader::class); diff --git a/tests/Bridges/SecurityAnnotationsDI/Fixtures/config.doctrine-annotations-enabled.neon b/tests/Bridges/SecurityAnnotationsDI/Fixtures/config.doctrine-annotations-enabled.neon new file mode 100644 index 0000000..13df824 --- /dev/null +++ b/tests/Bridges/SecurityAnnotationsDI/Fixtures/config.doctrine-annotations-enabled.neon @@ -0,0 +1,2 @@ +securityAnnotations: + enableDoctrineAnnotations: true diff --git a/tests/Bridges/SecurityAnnotationsDI/Fixtures/config.neon b/tests/Bridges/SecurityAnnotationsDI/Fixtures/config.neon index 5d79797..b6103c0 100644 --- a/tests/Bridges/SecurityAnnotationsDI/Fixtures/config.neon +++ b/tests/Bridges/SecurityAnnotationsDI/Fixtures/config.neon @@ -16,3 +16,6 @@ security: services: - Doctrine\Common\Cache\ArrayCache + +securityAnnotations: + enableDoctrineAnnotations: false diff --git a/tests/Bridges/SecurityAnnotationsDI/SecurityAnnotationsExtensionTest.phpt b/tests/Bridges/SecurityAnnotationsDI/SecurityAnnotationsExtensionTest.phpt index 30c07c7..1f2c06b 100644 --- a/tests/Bridges/SecurityAnnotationsDI/SecurityAnnotationsExtensionTest.phpt +++ b/tests/Bridges/SecurityAnnotationsDI/SecurityAnnotationsExtensionTest.phpt @@ -99,12 +99,14 @@ class SecurityAnnotationsExtensionTest extends TestCase public function testDefaultReader(): void { + $this->configurator->addConfig(__DIR__ . '/Fixtures/config.doctrine-annotations-enabled.neon'); + $expected = [ new SecurityAnnotations\Annotations\Role('attribute'), new SecurityAnnotations\Annotations\Role('annotation'), ]; /** @var SecurityAnnotations\AnnotationReaders\AnnotationsReader $reader */ - $reader = $this->configurator->createContainer()->getByType(SecurityAnnotations\AnnotationReaders\AnnotationsReader::class); + $reader = @$this->configurator->createContainer()->getByType(SecurityAnnotations\AnnotationReaders\AnnotationsReader::class); Assert::equal($expected, $reader->getAll(new \ReflectionClass(LoremIpsum::class))); } diff --git a/tests/SecurityAnnotations/AccessValidators/LoggedInValidatorTest.phpt b/tests/SecurityAnnotations/AccessValidators/LoggedInValidatorTest.phpt index 467fed3..19823b2 100644 --- a/tests/SecurityAnnotations/AccessValidators/LoggedInValidatorTest.phpt +++ b/tests/SecurityAnnotations/AccessValidators/LoggedInValidatorTest.phpt @@ -3,8 +3,6 @@ declare(strict_types = 1); namespace NepadaTests\SecurityAnnotations\AccessValidators; -use Doctrine\Common\Annotations\AnnotationRegistry; -use Doctrine\Common\Annotations\DocParser; use Mockery; use Mockery\MockInterface; use Nepada\SecurityAnnotations\AccessValidators; @@ -24,19 +22,15 @@ require_once __DIR__ . '/../../bootstrap.php'; class LoggedInValidatorTest extends TestCase { - private DocParser $docParser; - protected function setUp(): void { parent::setUp(); Environment::bypassFinals(); - AnnotationRegistry::registerUniqueLoader('class_exists'); - $this->docParser = new DocParser(); } public function testAccessAllowed(): void { - $annotation = $this->parseAnnotation('@Nepada\SecurityAnnotations\Annotations\LoggedIn'); + $annotation = new LoggedIn(); $user = $this->mockUser(true); $validator = new AccessValidators\LoggedInValidator($user); @@ -47,7 +41,7 @@ class LoggedInValidatorTest extends TestCase public function testAccessDenied(): void { - $annotation = $this->parseAnnotation('@Nepada\SecurityAnnotations\Annotations\LoggedIn'); + $annotation = new LoggedIn(); $user = $this->mockUser(false); $validator = new AccessValidators\LoggedInValidator($user); @@ -68,14 +62,6 @@ class LoggedInValidatorTest extends TestCase return $user; } - private function parseAnnotation(string $input): LoggedIn - { - $annotations = $this->docParser->parse($input); - Assert::count(1, $annotations); - Assert::type(LoggedIn::class, $annotations[0]); - return $annotations[0]; - } - } diff --git a/tests/SecurityAnnotations/AccessValidators/PermissionValidatorTest.phpt b/tests/SecurityAnnotations/AccessValidators/PermissionValidatorTest.phpt index e2853a7..37f6548 100644 --- a/tests/SecurityAnnotations/AccessValidators/PermissionValidatorTest.phpt +++ b/tests/SecurityAnnotations/AccessValidators/PermissionValidatorTest.phpt @@ -3,8 +3,6 @@ declare(strict_types = 1); namespace NepadaTests\SecurityAnnotations\AccessValidators; -use Doctrine\Common\Annotations\AnnotationRegistry; -use Doctrine\Common\Annotations\DocParser; use Mockery; use Mockery\MockInterface; use Nepada\SecurityAnnotations\AccessValidators; @@ -24,24 +22,14 @@ require_once __DIR__ . '/../../bootstrap.php'; class PermissionValidatorTest extends TestCase { - private DocParser $docParser; - - protected function setUp(): void - { - parent::setUp(); - AnnotationRegistry::registerUniqueLoader('class_exists'); - $this->docParser = new DocParser(); - } - /** * @dataProvider getDataForAccessAllowed - * @param string $input + * @param Allowed $annotation * @param string|null $resource * @param string|null $privilege */ - public function testAccessAllowed(string $input, ?string $resource, ?string $privilege): void + public function testAccessAllowed(Allowed $annotation, ?string $resource, ?string $privilege): void { - $annotation = $this->parseAnnotation($input); $user = $this->mockUser($resource, $privilege, IAuthorizator::ALLOW); $validator = new AccessValidators\PermissionValidator($user); @@ -57,22 +45,22 @@ class PermissionValidatorTest extends TestCase { return [ [ - 'input' => '@Nepada\SecurityAnnotations\Annotations\Allowed()', + 'annotation' => new Allowed(), 'resource' => IAuthorizator::ALL, 'privilege' => IAuthorizator::ALL, ], [ - 'input' => '@Nepada\SecurityAnnotations\Annotations\Allowed(resource="foo")', + 'annotation' => new Allowed('foo'), 'resource' => 'foo', 'privilege' => IAuthorizator::ALL, ], [ - 'input' => '@Nepada\SecurityAnnotations\Annotations\Allowed(privilege="edit")', + 'annotation' => new Allowed(null, 'edit'), 'resource' => IAuthorizator::ALL, 'privilege' => 'edit', ], [ - 'input' => '@Nepada\SecurityAnnotations\Annotations\Allowed(resource="foo", privilege="edit")', + 'annotation' => new Allowed('foo', 'edit'), 'resource' => 'foo', 'privilege' => 'edit', ], @@ -81,14 +69,13 @@ class PermissionValidatorTest extends TestCase /** * @dataProvider getDataForAccessDenied - * @param string $input + * @param Allowed $annotation * @param string|null $resource * @param string|null $privilege * @param string $message */ - public function testAccessDenied(string $input, ?string $resource, ?string $privilege, string $message): void + public function testAccessDenied(Allowed $annotation, ?string $resource, ?string $privilege, string $message): void { - $annotation = $this->parseAnnotation($input); $user = $this->mockUser($resource, $privilege, IAuthorizator::DENY); $validator = new AccessValidators\PermissionValidator($user); @@ -104,25 +91,25 @@ class PermissionValidatorTest extends TestCase { return [ [ - 'input' => '@Nepada\SecurityAnnotations\Annotations\Allowed()', + 'annotation' => new Allowed(), 'resource' => IAuthorizator::ALL, 'privilege' => IAuthorizator::ALL, 'message' => 'User is not allowed to access the resource.', ], [ - 'input' => '@Nepada\SecurityAnnotations\Annotations\Allowed(resource="foo")', + 'annotation' => new Allowed('foo'), 'resource' => 'foo', 'privilege' => IAuthorizator::ALL, 'message' => "User is not allowed to access the resource 'foo'.", ], [ - 'input' => '@Nepada\SecurityAnnotations\Annotations\Allowed(privilege="edit")', + 'annotation' => new Allowed(null, 'edit'), 'resource' => IAuthorizator::ALL, 'privilege' => 'edit', 'message' => 'User is not allowed to edit the resource.', ], [ - 'input' => '@Nepada\SecurityAnnotations\Annotations\Allowed(resource="foo", privilege="edit")', + 'annotation' => new Allowed('foo', 'edit'), 'resource' => 'foo', 'privilege' => 'edit', 'message' => "User is not allowed to edit the resource 'foo'.", @@ -144,14 +131,6 @@ class PermissionValidatorTest extends TestCase return $user; } - private function parseAnnotation(string $input): Allowed - { - $annotations = $this->docParser->parse($input); - Assert::count(1, $annotations); - Assert::type(Allowed::class, $annotations[0]); - return $annotations[0]; - } - } diff --git a/tests/SecurityAnnotations/AccessValidators/RoleValidatorTest.phpt b/tests/SecurityAnnotations/AccessValidators/RoleValidatorTest.phpt index 6698cc6..d4db2fa 100644 --- a/tests/SecurityAnnotations/AccessValidators/RoleValidatorTest.phpt +++ b/tests/SecurityAnnotations/AccessValidators/RoleValidatorTest.phpt @@ -3,8 +3,6 @@ declare(strict_types = 1); namespace NepadaTests\SecurityAnnotations\AccessValidators; -use Doctrine\Common\Annotations\AnnotationRegistry; -use Doctrine\Common\Annotations\DocParser; use Mockery; use Mockery\MockInterface; use Nepada\SecurityAnnotations\AccessValidators; @@ -24,24 +22,14 @@ require_once __DIR__ . '/../../bootstrap.php'; class RoleValidatorTest extends TestCase { - private DocParser $docParser; - - protected function setUp(): void - { - parent::setUp(); - AnnotationRegistry::registerUniqueLoader('class_exists'); - $this->docParser = new DocParser(); - } - /** * @dataProvider getDataForAccessAllowed - * @param string $input + * @param Role $annotation * @param string[] $userRoles * @param array>|null $rolesInheritance */ - public function testAccessAllowed(string $input, array $userRoles, ?array $rolesInheritance): void + public function testAccessAllowed(Role $annotation, array $userRoles, ?array $rolesInheritance): void { - $annotation = $this->parseAnnotation($input); $user = $this->mockUser($userRoles); $permission = $rolesInheritance === null ? null : $this->mockPermission($rolesInheritance); $validator = new AccessValidators\RoleValidator($user, $permission); @@ -58,17 +46,17 @@ class RoleValidatorTest extends TestCase { return [ [ - 'input' => '@Nepada\SecurityAnnotations\Annotations\Role("bar")', + 'annotation' => new Role('bar'), 'userRoles' => ['foo', 'bar', 'baz'], 'rolesInheritance' => null, ], [ - 'input' => '@Nepada\SecurityAnnotations\Annotations\Role({"xyz", "bar", "abc"})', + 'annotation' => new Role(['xyz', 'bar', 'abc']), 'userRoles' => ['foo', 'bar', 'baz'], 'rolesInheritance' => null, ], [ - 'input' => '@Nepada\SecurityAnnotations\Annotations\Role({"abc", "cde"})', + 'annotation' => new Role(['abc', 'cde']), 'userRoles' => ['foo', 'bar', 'baz'], 'rolesInheritance' => ['foo' => ['xyz'], 'bar' => ['abc']], ], @@ -77,13 +65,12 @@ class RoleValidatorTest extends TestCase /** * @dataProvider getDataForAccessDenied - * @param string $input + * @param Role $annotation * @param string[] $userRoles * @param array>|null $rolesInheritance */ - public function testAccessDenied(string $input, array $userRoles, ?array $rolesInheritance): void + public function testAccessDenied(Role $annotation, array $userRoles, ?array $rolesInheritance): void { - $annotation = $this->parseAnnotation($input); $user = $this->mockUser($userRoles); $permission = $rolesInheritance === null ? null : $this->mockPermission($rolesInheritance); $validator = new AccessValidators\RoleValidator($user, $permission); @@ -100,17 +87,17 @@ class RoleValidatorTest extends TestCase { return [ [ - 'input' => '@Nepada\SecurityAnnotations\Annotations\Role("required")', + 'annotation' => new Role('required'), 'userRoles' => [], 'rolesInheritance' => null, ], [ - 'input' => '@Nepada\SecurityAnnotations\Annotations\Role({"required", "another"})', + 'annotation' => new Role(['required', 'another']), 'userRoles' => ['baz'], 'rolesInheritance' => null, ], [ - 'input' => '@Nepada\SecurityAnnotations\Annotations\Role({"required", "another"})', + 'annotation' => new Role(['required', 'another']), 'userRoles' => ['foo', 'bar', 'baz'], 'rolesInheritance' => ['foo' => ['xyz'], 'bar' => ['abc']], ], @@ -143,14 +130,6 @@ class RoleValidatorTest extends TestCase return $permission; } - private function parseAnnotation(string $input): Role - { - $annotations = $this->docParser->parse($input); - Assert::count(1, $annotations); - Assert::type(Role::class, $annotations[0]); - return $annotations[0]; - } - }