SonataAdminBundle icon indicating copy to clipboard operation
SonataAdminBundle copied to clipboard

make symfony/security-acl optional dependency

Open Tobion opened this issue 3 years ago • 6 comments

Using Symfony ACL is not recommended for most situations anymore. I also think sonata should not require symfony/security-acl component by default and instead it should be an optional dependency in case people really want acl functionality. This is somehow the case already as sontata would also require symfony/acl-bundle in practice which it does not require. So people need to require certain packages manually to add acl support.

Currently sonata/admin-bundle and doctrine-orm-admin-bundle require symfony/security-acl. Some classes implement interfaces from security-acl like AbstractAdmin implements Symfony\Component\Security\Acl\Model\DomainObjectInterface. So I think it would be better if sonata runs without acl by default and people wanting acl functionality would just implement acl interfaces in their own classes.

Tobion avatar Jul 04 '21 18:07 Tobion

Indeed, we got an issue because someone wasn't using the acl-bundle https://github.com/sonata-project/SonataAdminBundle/issues/7086. So this can be related to https://github.com/sonata-project/SonataAdminBundle/issues/7097.

The Security\Acl namespace is used in

  • AbstractAdmin which implements DomainObjectInterface
  • GenerateObjectAclCommand
  • AdminPermissionMap
  • MaskBuilder
  • AclSecurityHandler (and interface)
  • SecurityExtension
  • AdminAclManipulator (and interface)
  • AdminObjectAclData
  • AdminObjectAclManipulator
  • ObjectAclManipulator (and interface) it also used in the config
set('sonata.admin.security.mask.builder.class', MaskBuilder::class)

In SonataDoctrineORM, it's just used in the ObjectAclManipulator.

What is the way to make this dependency optional ? Do you want to make the PR ?

I assume that the AbstractAdmin won't work if it implements a non-existing interface.

This might be a good idea to move the Acl related class from Util to an Acl folder too @sonata-project/contributors

VincentLanglet avatar Jul 04 '21 23:07 VincentLanglet

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

github-actions[bot] avatar Jan 01 '22 09:01 github-actions[bot]

I created a discussion about it on symfony to get some help https://github.com/symfony/symfony/discussions/48257

WDYT about this @jordisala1991 ? How should we update our code to make acl optionnal ?

VincentLanglet avatar Nov 21 '22 12:11 VincentLanglet

Yes, I think we should make it optional. And if we manage to split it in a separate bundle, even better, never used ACL and when I tried, never got to make it work.

jordisala1991 avatar Mar 18 '23 12:03 jordisala1991

I dunno how we can split since, for example AbstractAdmin implements DomainObjectInterface...

VincentLanglet avatar Mar 18 '23 13:03 VincentLanglet

Yep, we need to tackle like other things we split from the adminInterface, like breadcrumbs, templates...

Not an easy task tho.

jordisala1991 avatar Mar 18 '23 15:03 jordisala1991