Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .env
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
DATABASE_URL=mysql://bang:password@rds-bang:3306/test
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,4 @@ composer.lock
/behat.yml
/phpspec.yml
/.phpunit.result.cache
/var
12 changes: 8 additions & 4 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,21 +14,25 @@
],
"require": {
"php": "^8.1",
"symfony/framework-bundle": "~6.0|~7.0",
"symfony/framework-bundle": "^7.0",
"webmozart/assert": "^1.10",
"doctrine/annotations": "^2.0",
"doctrine/doctrine-bundle": "^2.5",
"doctrine/doctrine-migrations-bundle": "^3.2",
"doctrine/orm": "^3.2",
"symfony/security-bundle": "~6.0|~7.0",
"symfony/security-bundle": "^7.0",
"symfony/orm-pack": "^2.4",
"symfony/twig-pack": "^1.0",
"symfony/dotenv": "~6.0|~7.0"
"symfony/dotenv": "^7.0"
},
"autoload": {
"psr-4": { "PhpRbacBundle\\": "src/" }
},
"autoload-dev": {
"psr-4": { "Tests\\PhpRbacBundle\\": "tests/" }
},
"require-dev": {
"phpunit/phpunit": "^11.4",
"zenstruck/foundry": "^2.1",
"symfony/yaml": "^7.0"
}
}
31 changes: 2 additions & 29 deletions phpunit.xml
Original file line number Diff line number Diff line change
Expand Up @@ -6,45 +6,18 @@
backupGlobals="false"
colors="true"
bootstrap="tests/bootstrap.php"
convertDeprecationsToExceptions="false"
>
<php>
<ini name="display_errors" value="1" />
<ini name="error_reporting" value="-1" />
<server name="APP_ENV" value="test" force="true" />
<server name="SHELL_VERBOSITY" value="-1" />
<server name="SYMFONY_PHPUNIT_REMOVE" value="" />
<server name="SYMFONY_PHPUNIT_VERSION" value="9.5" />
<server name="KERNEL_CLASS" value="App\Kernel" />
<env name="SYMFONY_DEPRECATIONS_HELPER" value="disabled" />
<server name="KERNEL_CLASS" value="Tests\PhpRbacBundle\Fixture\TestKernel" />
</php>

<testsuites>
<testsuite name="Project Test Suite">
<directory>tests</directory>
</testsuite>
</testsuites>

<coverage processUncoveredFiles="true">
<include>
<directory suffix=".php">src</directory>
</include>
</coverage>
<!--
<listeners>
<listener class="Symfony\Bridge\PhpUnit\SymfonyTestsListener" />
</listeners> -->

<!-- Run `composer require symfony/panther` before enabling this extension -->
<!--
<extensions>
<extension class="Symfony\Component\Panther\ServerExtension" />
</extensions>
-->
<!-- <logging>
<log type="coverage-xml" target="build/coverage" title="PhpRbacBundle"
charset="UTF-8" yui="true" highlight="true"
lowUpperBound="35" highLowerBound="70"/>
<log type="coverage-clover" target="build/logs/clover.xml"/>
<log type="junit" target="build/logs/junit.xml" logIncompleteSkipped="false"/>
</logging> -->
</phpunit>
5 changes: 4 additions & 1 deletion src/Command/RbacAddPermissionCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -49,13 +49,15 @@ protected function execute(InputInterface $input, OutputInterface $output): int
}

$permissions = [];

foreach ($permissionsTmp as $permission) {
$pathNodes = $this->permissionRepository->getPath($permission->getId());
$path = '/'.implode('/', $pathNodes);
$path = str_replace('/root', '/', $path);
$path = str_replace('//', '/', $path);
$permissions[$path] = $permission;
}

ksort($permissions);

$question = new Question('Enter the code of the permission : ');
Expand All @@ -64,7 +66,8 @@ protected function execute(InputInterface $input, OutputInterface $output): int
$description = $helper->ask($input, $output, $question);
$question = new ChoiceQuestion('Enter the parent of the permission : ', array_keys($permissions), 0);
$parentPath = $helper->ask($input, $output, $question);
$permission = $this->permissionManager->add($code, $description, $permissions[$parentPath]->getId());

$this->permissionManager->add($code, $description, $permissions[$parentPath]->getId());

return Command::SUCCESS;
}
Expand Down
3 changes: 2 additions & 1 deletion src/Command/RbacAddRoleCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,8 @@ protected function execute(InputInterface $input, OutputInterface $output): int
$description = $helper->ask($input, $output, $question);
$question = new ChoiceQuestion('Enter the parent of the role : ', array_keys($roles), 0);
$parentPath = $helper->ask($input, $output, $question);
$role = $this->roleManager->add($code, $description, $roles[$parentPath]->getId());

$this->roleManager->add($code, $description, $roles[$parentPath]->getId());

return Command::SUCCESS;
}
Expand Down
30 changes: 25 additions & 5 deletions src/Core/Manager/NodeManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,11 @@
use PhpRbacBundle\Exception\RbacException;
use PhpRbacBundle\Repository\RoleRepository;
use PhpRbacBundle\Repository\PermissionRepository;
use Symfony\Component\TypeInfo\Type;

/**
* @template T of NodeInterface
*/
abstract class NodeManager implements NodeManagerInterface
{
public function __construct(protected RoleRepository|PermissionRepository $repository)
Expand All @@ -29,6 +33,9 @@ public function add(string $code, string $description, int $parentId = self::ROO
}
}

/**
* @return NodeInterface<T>
*/
public function addPath(string $path, array $descriptions): NodeInterface
{
if ($path[0] !== '/') {
Expand Down Expand Up @@ -64,17 +71,24 @@ public function addPath(string $path, array $descriptions): NodeInterface

public function getPathId(string $path): int
{
if (substr($path, -1) == "/") {
if (str_ends_with($path, '/')) {
$path = substr($path, 0, strlen($path) - 1);
}

return $this->repository->getPathId($path);
}

/**
* @return T
*/
public function getNode(int $nodeId): NodeInterface
{
return $this->repository->getById($nodeId);
}

/**
* @return T
*/
public function updateNode(int $nodeId, string $code, string $description): NodeInterface
{
$node = $this->getNode($nodeId);
Expand All @@ -94,8 +108,8 @@ public function getPath(int $nodeId): string
return "/";
}
foreach ($nodes as $node) {
if ($node->getId() != self::ROOT_ID) {
$path .= "/" . $node->getCode();
if ($node->getId() !== self::ROOT_ID) {
$path .= '/'.$node->getCode();
}
}

Expand All @@ -107,6 +121,9 @@ public function getChildren(int $nodeId): array
return $this->repository->getChildren($nodeId);
}

/**
* @return list<T>
*/
public function getParents(int $nodeId): array
{
return $this->repository->getPath($nodeId);
Expand All @@ -117,6 +134,9 @@ public function getDepth(int $nodeId): int
return count($this->repository->getPath($nodeId));
}

/**
* @return T
*/
public function getParent(int $nodeId): NodeInterface
{
$nodes = $this->repository->getPath($nodeId);
Expand All @@ -127,8 +147,8 @@ public function getParent(int $nodeId): NodeInterface
return $nodes[count($nodes) - 2];
}

public function reset()
public function reset(): void
{
return $this->repository->reset();
$this->repository->reset();
}
}
2 changes: 2 additions & 0 deletions src/Core/Manager/PermissionManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

/**
* @property PermissionRepository $repository
*
* @extends NodeManager<PermissionInterface>
*/
class PermissionManager extends NodeManager implements PermissionManagerInterface
{
Expand Down
6 changes: 0 additions & 6 deletions src/Core/Manager/PermissionManagerInterface.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,14 @@ interface PermissionManagerInterface extends NodeManagerInterface
/**
* Remove permission and attach all the sub-permission to the parent
*
* @param PermissionInterface $permission
*
* @throws RbacPermissionNotFoundException
* @return boolean
*/
public function remove(PermissionInterface $permission): bool;

/**
* Remove Permission and all sub-permissions from system
*
* @param PermissionInterface $permission
*
* @throws RbacPermissionNotFoundException
* @return boolean
*/
public function removeRecursively(PermissionInterface $permission): bool;

Expand Down
2 changes: 2 additions & 0 deletions src/Core/Manager/RoleManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

/**
* @property RoleRepository $repository
*
* @extends NodeManager<RoleInterface>
*/
class RoleManager extends NodeManager implements RoleManagerInterface
{
Expand Down
2 changes: 1 addition & 1 deletion src/Core/RbacVoter.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ protected function voteOnAttribute(string $attribute, mixed $subject, TokenInter
}

try {
return $this->rbacManager->hasPermission($attribute, $user->getId());
return $this->rbacManager->hasPermission($attribute, $user->getUserIdentifier());
Copy link

@anthony-321 anthony-321 Oct 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could break things because subsequent queries rely on the user id, not the public-facing user identifier (email, username, etc). It uses the id for subsequent authorization queries.

But as you know, there is no UserInterface::id:
https://github.com/symfony/symfony/blob/7.1/src/Symfony/Component/Security/Core/User/UserInterface.php

For me, static analysis has highlighted that this bundle is designed to the wrong interface (UserInterface instead of User). I think that the fix here would be to depend on User.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also keep it the way it is, but change the SQL queries

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea I think the issue is that the User entity comes from the config and we're not sure that id will exist either.

in our case, I think userIdentifier works better?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

userIdentifier works, I think we'd just need to update Rbac::hasPermission() to transform the userIdentifier string into an int for subsequent queries. We could also modify PermissionRepository::hasPermission(). Downside is more lookups/joins as opposed to original implementation that relies only on ids.

} catch (Exception) {
return false;
}
Expand Down
10 changes: 6 additions & 4 deletions src/EventSubscriber/AccessControlDriver.php
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,14 @@ public function __construct(
]
];

public function load(array $config)
public function load(array $config): void
{
$this->config = $config;
}

private function checkAttributes(array $attributes, string $controller, string $method = "")
private function checkAttributes(array $attributes, string $controller, string $method = ""): void
{
if (empty($attributes)) {
if ($attributes === []) {
return;
}

Expand All @@ -59,12 +59,14 @@ private function checkAttributes(array $attributes, string $controller, string $
}


public function onKernelController(ControllerEvent $event)
public function onKernelController(ControllerEvent $event): void
{
$controllers = $event->getController();

if (!is_array($controllers)) {
return;
}

$controller = $controllers[0];
$method = $controllers[1];

Expand Down
1 change: 1 addition & 0 deletions src/PhpRbacBundle.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ final class PhpRbacBundle extends Bundle
public function build(ContainerBuilder $container): void
{
parent::build($container);

$container->addCompilerPass(new DoctrineResolveTargetEntityPass(), PassConfig::TYPE_BEFORE_OPTIMIZATION, 10);
}
}
Loading