Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate acl_user_manager without implementing AdminAclUserManagerInterface #6480

Merged
merged 1 commit into from
Oct 18, 2020

Conversation

franmomu
Copy link
Member

@franmomu franmomu commented Oct 13, 2020

Subject

Ref: #6476 (comment)

The idea is introducing AdminAclUserManagerInterface which the service configured under sonata_admin.security.acl_user_manager must implement. If it's ok, I'll add the upgrade note.

I am targeting this branch, because these changes are BC.

Changelog

### Deprecated
- Deprecated not configuring `acl_user_manager` service explicitly when using `friendsofsymfony/user-bundle`.
- Deprecated configuring `acl_user_manager` service without implementing `AdminAclUserManagerInterface`.
  • Deprecate not configuring acl_user_manager.

if (null === $config['security']['acl_user_manager'] && isset($bundles['FOSUserBundle'])) {
$container->setParameter('sonata.admin.security.acl_user_manager', 'fos_user.user_manager');
} else {
$container->setParameter('sonata.admin.security.acl_user_manager', $config['security']['acl_user_manager']);
}

// NEXT_MAJOR: Uncomment this code.
//if (null !== $config['security']['acl_user_manager']) {
// $container->setAlias('sonata.admin.security.acl_user_manager', $config['security']['acl_user_manager']);
Copy link
Member

Choose a reason for hiding this comment

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

take a look at my PR to see the correct alias, otherwise it does not work (at least on my tests)

Copy link
Member Author

Choose a reason for hiding this comment

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

I also left this one because from the controller we are fetching it with get('sonata.admin.security.acl_user_manager')

@wbloszyk
Copy link
Member

wbloszyk commented Oct 13, 2020

if (null === $config['security']['acl_user_manager'] && isset($bundles['FOSUserBundle'])) {
$container->setParameter('sonata.admin.security.acl_user_manager', 'fos_user.user_manager');
} else {
$container->setParameter('sonata.admin.security.acl_user_manager', $config['security']['acl_user_manager']);
}

IMO sonata.admin.security.acl_user_manager should look like class below and decorate service taken from configuration. This will allow avoid using extra unnesessery class (compiler pass) and also will allow remove unnesessery checking.

final class AclUserManager implement AdminAclUserManagerInterface
{
    /**
    * @var AdminAclUserManagerInterface
    **/
    private $manager;

    public function __construct(?AdminAclUserManagerInterface $aclManager = null)
    {
        $this->manager = $manager;
    }
    
    public function findUsers(): ?\Traversable
    {
        if (null !== $this->manager) {
            return $this->manager->findUsers();
        }

        return null;
    }
}

Resources/config:

        ->set('sonata.admin.security.acl_user_manager', AclUserManager::class)
            ->public()
            ->args([
                '%sonata.admin.security.acl_user_manager%',
            ])

AclUserManager class can be modify to keep BC in 3.x branch.

@franmomu franmomu force-pushed the acl_manager_interface branch 3 times, most recently from 9e31b6f to 6598a4c Compare October 13, 2020 22:56
@franmomu
Copy link
Member Author

IMO sonata.admin.security.acl_user_manager should look like class below and decorate service taken from configuration. This will allow avoid using extra unnesessery class (compiler pass) and also will allow remove unnesessery checking.

I changed it a little bit, I think there is no need to create a class for that, the compiler pass can be removed in 4.x. The idea is that the user specify a service in acl_user_manager, we create an alias called sonata.admin.security.acl_user_manager to that service and then an alias with the interface.

I've used the same strategy Symfony uses, for example with the session.storage_id:

They create an alias session.storage pointing at the service defined in the configuration: https://github.com/symfony/symfony/blob/8fa0573ab6434dd362fa28765470a95d7e809ce9/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php#L994

And then they create another alias SessionStorageInterface pointing to session.storage: https://github.com/symfony/symfony/blob/494ef421c554a78b38c6779c4b7deb9a20d89923/src/Symfony/Bundle/FrameworkBundle/Resources/config/session.php#L45

/**
* Batch configure the ACLs for all objects handled by an Admin.
*/
public function findUsers(): \Traversable;
Copy link
Member

Choose a reason for hiding this comment

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

isnt it better to use iterable?

Copy link
Member

Choose a reason for hiding this comment

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

I would have use array as return typehint ; like we did anywhere else in our code.

Copy link
Member

Choose a reason for hiding this comment

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

When related to the persistence, we are starting to use Collection too.

Copy link
Member

Choose a reason for hiding this comment

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

Even when persistence are returning Collection we're transforming them to array
https://github.com/sonata-project/SonataAdminBundle/blob/master/src/Datagrid/SimplePager.php#L68

If we start using iterable which is not a bad idea, it's maybe better to change it too for others signatures ?

Copy link
Member

Choose a reason for hiding this comment

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

In any case, iterable or array seems better than \Traversable

jordisala1991
jordisala1991 previously approved these changes Oct 14, 2020
Copy link
Member

@jordisala1991 jordisala1991 left a comment

Choose a reason for hiding this comment

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

Just left a minor comment, but it looks great to me

docs/reference/security.rst Outdated Show resolved Hide resolved
/**
* Batch configure the ACLs for all objects handled by an Admin.
*/
public function findUsers(): \Traversable;
Copy link
Member

Choose a reason for hiding this comment

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

In any case, iterable or array seems better than \Traversable

@franmomu franmomu force-pushed the acl_manager_interface branch 3 times, most recently from df4263d to 8aa3d14 Compare October 15, 2020 16:39
wbloszyk
wbloszyk previously approved these changes Oct 15, 2020
UPGRADE-3.x.md Outdated Show resolved Hide resolved
docs/reference/security.rst Outdated Show resolved Hide resolved
src/Controller/CRUDController.php Show resolved Hide resolved
tests/Controller/CRUDControllerTest.php Show resolved Hide resolved
@franmomu franmomu force-pushed the acl_manager_interface branch 2 times, most recently from 9105fd9 to 5c4e26e Compare October 15, 2020 22:56
@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

jordisala1991
jordisala1991 previously approved these changes Oct 18, 2020
UPGRADE-3.x.md Outdated Show resolved Hide resolved
UPGRADE-3.x.md Outdated Show resolved Hide resolved
src/Controller/CRUDController.php Outdated Show resolved Hide resolved
tests/Controller/CRUDControllerTest.php Outdated Show resolved Hide resolved
…terface

It also deprecates to not explicitly configuring acl_user_manager service when using ACL because
if you use "friendsofsymfony/user-bundle", it adds "fos_user_manager" as "acl_user_manager"
automatically.

After this, this service MUST implements AdminAclUserManagerInterface allowing us to have an API
for retrieving users when using ACL.
@franmomu franmomu force-pushed the acl_manager_interface branch from e45ac13 to d14d872 Compare October 18, 2020 13:41

$userManagerDefinition = $container->findDefinition($userManagerServiceName);

if (!is_a($userManagerDefinition->getClass(), AdminAclUserManagerInterface::class, true)) {
Copy link
Member

Choose a reason for hiding this comment

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

Just a warning: some time ago I added a similar check in our packages (I don't remember which one), and I had to add an extra DI resolution about the return value of Definition::getClass(), since it can be also a parameter (by instance %admin.acl.user_manager.class%) which must be resolved first in order to know the real class.

@jordisala1991 jordisala1991 merged commit 10ec613 into sonata-project:3.x Oct 18, 2020
@jordisala1991
Copy link
Member

Thank you @franmomu

@jordisala1991
Copy link
Member

Do you have the time to do the merge on master? I can handle the upgrade on master to remove next majors, after

@franmomu franmomu deleted the acl_manager_interface branch October 18, 2020 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants