Move the ACL part of the Security Component outside of Symfony #8848

Closed
fabpot opened this Issue Aug 24, 2013 · 26 comments

Comments

Projects
None yet
@fabpot
Owner

fabpot commented Aug 24, 2013

I've been thinking about this one for a long time now.

The ACL sub-system has been contributed a long time ago but since then it has been abandoned as nobody really maintain it. Personally, I don't want to maintain that part as I don't use it and frankly I don't any valid use cases for it.

So instead of keeping it in Symfony, I propose to create a new repo for it, where a new team can take care of its future (let's say symfony/acl for instance).

ping @schmittjoh

@hhamon

This comment has been minimized.

Show comment Hide comment
@hhamon

hhamon Aug 24, 2013

Contributor

+1

Contributor

hhamon commented Aug 24, 2013

+1

@schmittjoh

This comment has been minimized.

Show comment Hide comment
@schmittjoh

schmittjoh Aug 24, 2013

Contributor

I think that the ACL system is very suited to do what it was designed to do. I know enough projects, and have done plenty of consulting for companies which use it successfully. I see that there is some barrier to use it and the initial set-up is not as fast as writing a voter. However, it scales far better and is much more maintainable as projects grow or requirements change.

The problem mentioned in #5787 is actually caused due to a bad naming of the getUsername method in the UserInterface which should rather have been getIdentifier or something similar which suggest that it must be immutable. If you return different values for the same user from this method, then this causes problems not only for ACL, but also other parts of the security system.

While I would love to be part of a team which maintains Security, I do not think it makes sense to maintain ACL separately as it depends on other parts of the Security component which need to be changed to solve some issues that users have (#5787 is a prime example here). Unfortunately, I also have a good number of other open-source projects, so that I cannot spend as much time working on symfony anymore.

Contributor

schmittjoh commented Aug 24, 2013

I think that the ACL system is very suited to do what it was designed to do. I know enough projects, and have done plenty of consulting for companies which use it successfully. I see that there is some barrier to use it and the initial set-up is not as fast as writing a voter. However, it scales far better and is much more maintainable as projects grow or requirements change.

The problem mentioned in #5787 is actually caused due to a bad naming of the getUsername method in the UserInterface which should rather have been getIdentifier or something similar which suggest that it must be immutable. If you return different values for the same user from this method, then this causes problems not only for ACL, but also other parts of the security system.

While I would love to be part of a team which maintains Security, I do not think it makes sense to maintain ACL separately as it depends on other parts of the Security component which need to be changed to solve some issues that users have (#5787 is a prime example here). Unfortunately, I also have a good number of other open-source projects, so that I cannot spend as much time working on symfony anymore.

@lavoiesl

This comment has been minimized.

Show comment Hide comment
@lavoiesl

lavoiesl Aug 25, 2013

Contributor

+1

I would gladly volunteer to help improving this component.

Contributor

lavoiesl commented Aug 25, 2013

+1

I would gladly volunteer to help improving this component.

@ste93cry

This comment has been minimized.

Show comment Hide comment
@ste93cry

ste93cry Aug 25, 2013

ACL is always an important part of a big project: Symfony ACL is powerful but also way to complicated. I would like to see something more simple but that it's still extensible. I'm not sure that separating the acl from the Security component would be the right thing to do, it's always part of the security system...

ACL is always an important part of a big project: Symfony ACL is powerful but also way to complicated. I would like to see something more simple but that it's still extensible. I'm not sure that separating the acl from the Security component would be the right thing to do, it's always part of the security system...

@Inoryy

This comment has been minimized.

Show comment Hide comment
@Inoryy

Inoryy Aug 26, 2013

Contributor

+1 on simplifying
-1 on moving

Contributor

Inoryy commented Aug 26, 2013

+1 on simplifying
-1 on moving

@phpmike

This comment has been minimized.

Show comment Hide comment
@phpmike

phpmike Aug 26, 2013

This part is not enough used. I'm agree. But the problem isn't that is unusefull. The problem is that it isn't documented. The examples you can find are full of copy/paste code in controllers that is a bad way for maintenable app. Any big app need Acl. That's my case.

phpmike commented Aug 26, 2013

This part is not enough used. I'm agree. But the problem isn't that is unusefull. The problem is that it isn't documented. The examples you can find are full of copy/paste code in controllers that is a bad way for maintenable app. Any big app need Acl. That's my case.

@sukei

This comment has been minimized.

Show comment Hide comment
@sukei

sukei Aug 26, 2013

Contributor

I don't think that the ACL should be pull out of the framework, but this part suffer of a lack of documentation. Clearly. It is the sole part of symfony that I can't understand from A to Z.

There is another thing that I don't really understand is the need of the Doctrine DBAL. The ACLProvider API is not hidden behind an interface and made it's hard to swap with another one (it also made it hard to understand).

To summarize, I think that the ACLs should be preserved in Symfony, should be documented, and should be maintained. It's a really important and powerfull piece!

Contributor

sukei commented Aug 26, 2013

I don't think that the ACL should be pull out of the framework, but this part suffer of a lack of documentation. Clearly. It is the sole part of symfony that I can't understand from A to Z.

There is another thing that I don't really understand is the need of the Doctrine DBAL. The ACLProvider API is not hidden behind an interface and made it's hard to swap with another one (it also made it hard to understand).

To summarize, I think that the ACLs should be preserved in Symfony, should be documented, and should be maintained. It's a really important and powerfull piece!

@sukei

This comment has been minimized.

Show comment Hide comment
@sukei

sukei Aug 26, 2013

Contributor

@lavoiesl +1 to help improve this component too.

Contributor

sukei commented Aug 26, 2013

@lavoiesl +1 to help improve this component too.

@guillaumepotier

This comment has been minimized.

Show comment Hide comment
@guillaumepotier

guillaumepotier Aug 26, 2013

+1 to move it out of the framework.
+1 to simplify / improve

Very useful for complex permissions and voting. Complex to apprehend but useful for early moving projects.

But not suited for every projects. We used it at first and now our app is stable, we chose to remove it and implement simpler voters that suit more our needs and are way better in term of performances.

+1 to move it out of the framework.
+1 to simplify / improve

Very useful for complex permissions and voting. Complex to apprehend but useful for early moving projects.

But not suited for every projects. We used it at first and now our app is stable, we chose to remove it and implement simpler voters that suit more our needs and are way better in term of performances.

@liuggio

This comment has been minimized.

Show comment Hide comment
@liuggio

liuggio Aug 26, 2013

Contributor

From my point of view the voter have very little visibility,
+1 give more importance to the voter in the documentation.
+1 improve ACL.

Contributor

liuggio commented Aug 26, 2013

From my point of view the voter have very little visibility,
+1 give more importance to the voter in the documentation.
+1 improve ACL.

@ste93cry

This comment has been minimized.

Show comment Hide comment
@ste93cry

ste93cry Aug 26, 2013

Nearly ever big framework (CakePHP, Zend) have an ACL Component

Nearly ever big framework (CakePHP, Zend) have an ACL Component

@lavoiesl

This comment has been minimized.

Show comment Hide comment
@lavoiesl

lavoiesl Aug 26, 2013

Contributor

Many applications do not need such feature. Maybe this should stay as an official component, branded as core, but not included by default, it could sit in the suggest section of Composer. I believe it is important to still be maintained by the core devs to be sure it is always up-to-date and in tune with the rest of the framework.

However, it is not even at that level yet. From what I understand, it was coded this way not to be dependant on Doctrine, but the general feel of the code and usability is not the same as the rest of the framework.

I like Drupal’s way of dealing with optional core modules. Usually, when a module gets very popular, it is merged into core, but all modules are still optional. This ensures strong support in future version and a standardized API for all other modules. Examples of this are views and fields API.

Contributor

lavoiesl commented Aug 26, 2013

Many applications do not need such feature. Maybe this should stay as an official component, branded as core, but not included by default, it could sit in the suggest section of Composer. I believe it is important to still be maintained by the core devs to be sure it is always up-to-date and in tune with the rest of the framework.

However, it is not even at that level yet. From what I understand, it was coded this way not to be dependant on Doctrine, but the general feel of the code and usability is not the same as the rest of the framework.

I like Drupal’s way of dealing with optional core modules. Usually, when a module gets very popular, it is merged into core, but all modules are still optional. This ensures strong support in future version and a standardized API for all other modules. Examples of this are views and fields API.

@sukei

This comment has been minimized.

Show comment Hide comment
@sukei

sukei Aug 26, 2013

Contributor

I am not against the idea to isolate ACL in a separate component. But as @schmittjoh says, it could be tricky to make this separation as it depends on the some parts of the security component. Difficult but still possible.

@lavoiesl You are right, the way it is made looks like Java's and isn't as user-friendly as the other components. It should be right to simplify it's API, but is it possible without loosing the possibilities it offers?

Contributor

sukei commented Aug 26, 2013

I am not against the idea to isolate ACL in a separate component. But as @schmittjoh says, it could be tricky to make this separation as it depends on the some parts of the security component. Difficult but still possible.

@lavoiesl You are right, the way it is made looks like Java's and isn't as user-friendly as the other components. It should be right to simplify it's API, but is it possible without loosing the possibilities it offers?

@lavoiesl

This comment has been minimized.

Show comment Hide comment
@lavoiesl

lavoiesl Aug 26, 2013

Contributor

@sukei ACL depends on Security, but not the opposite, you can have a firewall with simple role checking without ACLs, this is why I say it could be separated, but still maintained as a core component.

As for the code itself, a bit of refactoring, splitting into different files, less repetition and more documentation should help better understand the way it works and provide better grounds for future improvements.

As a start, I was thinking about using Doctrine ORM/ODM. This will allow the removal of everything related to SQL and provide support for foreign keys and events so we can deal with the renaming and deletion of users.

Contributor

lavoiesl commented Aug 26, 2013

@sukei ACL depends on Security, but not the opposite, you can have a firewall with simple role checking without ACLs, this is why I say it could be separated, but still maintained as a core component.

As for the code itself, a bit of refactoring, splitting into different files, less repetition and more documentation should help better understand the way it works and provide better grounds for future improvements.

As a start, I was thinking about using Doctrine ORM/ODM. This will allow the removal of everything related to SQL and provide support for foreign keys and events so we can deal with the renaming and deletion of users.

@sukei

This comment has been minimized.

Show comment Hide comment
@sukei

sukei Aug 26, 2013

Contributor

@lavoiesl I totally agree with you, the ACL are independent enough. Big changes have to be done in the bundle that integrate them. As I said, it is still possible to achieve this.

On the other side, I don't think that it is a good idea to involve all the Doctrine stack. I think it should be better to hide the AclProvider / MutableAclProvider classes behind a clean interface. Doing so allow the developers to use PDO, Doctrine, whatever to retrieve ACEs.

Contributor

sukei commented Aug 26, 2013

@lavoiesl I totally agree with you, the ACL are independent enough. Big changes have to be done in the bundle that integrate them. As I said, it is still possible to achieve this.

On the other side, I don't think that it is a good idea to involve all the Doctrine stack. I think it should be better to hide the AclProvider / MutableAclProvider classes behind a clean interface. Doing so allow the developers to use PDO, Doctrine, whatever to retrieve ACEs.

@lavoiesl

This comment has been minimized.

Show comment Hide comment
@lavoiesl

lavoiesl Aug 26, 2013

Contributor

I don’t agree with you on this, but since we do agree on the fact that it needs some work, I suggest we wait for @fabpot ’s feedback and eventually have this discussion elsewhere.

Contributor

lavoiesl commented Aug 26, 2013

I don’t agree with you on this, but since we do agree on the fact that it needs some work, I suggest we wait for @fabpot ’s feedback and eventually have this discussion elsewhere.

@lsmith77

This comment has been minimized.

Show comment Hide comment
@lsmith77

lsmith77 Aug 26, 2013

Contributor

I am +1 on keeping it in Symfony. I have seen it used in quite a few projects and imho it needs better docs and then it will be used even more. Moving it now is imho too late in the 2.x life cycle.

Contributor

lsmith77 commented Aug 26, 2013

I am +1 on keeping it in Symfony. I have seen it used in quite a few projects and imho it needs better docs and then it will be used even more. Moving it now is imho too late in the 2.x life cycle.

@michaelcullum

This comment has been minimized.

Show comment Hide comment
@michaelcullum

michaelcullum Aug 27, 2013

Member

+1 on simplifying
+1 on separation
-1 on moving from symfony

Member

michaelcullum commented Aug 27, 2013

+1 on simplifying
+1 on separation
-1 on moving from symfony

@ginsen

This comment has been minimized.

Show comment Hide comment
@ginsen

ginsen Aug 28, 2013

+1 to simplify / improve
-1 to move it out of the framework.

ginsen commented Aug 28, 2013

+1 to simplify / improve
-1 to move it out of the framework.

@Taluu

This comment has been minimized.

Show comment Hide comment
@Taluu

Taluu Aug 28, 2013

Contributor

+1 to simplify / improve
+1 to move it out of symfony/symfony
+1 to keep it in symfony/acl (or something like that)

Contributor

Taluu commented Aug 28, 2013

+1 to simplify / improve
+1 to move it out of symfony/symfony
+1 to keep it in symfony/acl (or something like that)

@dunglas

This comment has been minimized.

Show comment Hide comment
@dunglas

dunglas Aug 29, 2013

Member

I've done some work to integrate deeper the ACL component in SonataAdmin:

Symfony now has an easy to use UI with ACL management. Perhaps more projects will start using ACLs.

ACL is a major feature for a framework, Symfony need to have a powerful ACL component in the core framework.

The current ACL component is complicated and strongly coupled with DBAL. The abstraction layer should be refactored to simplify the use of other systems like MongoDB.
With the current implementation, it's hard to do things like "fetch all objects that the user A can delete" in an efficient way. Maybe must we use a pattern more generic than bitmasks for permissions storage.

I am willing to help enhance this component.

Member

dunglas commented Aug 29, 2013

I've done some work to integrate deeper the ACL component in SonataAdmin:

Symfony now has an easy to use UI with ACL management. Perhaps more projects will start using ACLs.

ACL is a major feature for a framework, Symfony need to have a powerful ACL component in the core framework.

The current ACL component is complicated and strongly coupled with DBAL. The abstraction layer should be refactored to simplify the use of other systems like MongoDB.
With the current implementation, it's hard to do things like "fetch all objects that the user A can delete" in an efficient way. Maybe must we use a pattern more generic than bitmasks for permissions storage.

I am willing to help enhance this component.

@sstok

This comment has been minimized.

Show comment Hide comment
@sstok

sstok Sep 2, 2013

Contributor

Each respected database system already supports for bitmasks.

http://stackoverflow.com/questions/11059182/select-users-from-mysql-database-by-privileges-bitmask
http://stackoverflow.com/questions/2180266/bit-masking-in-postgres
http://consultingblogs.emc.com/jamesrowlandjones/archive/2008/07/04/using-a-bitmask-a-practical-example.aspx

And even SQLite http://zetcode.com/db/sqlite/expressions/

Only MongoDB (and maybe some others) don't support bitmask yet.
But that something that should be fixed in Provider IMO.

A bitmask is nothing more then an integer representation of bits, so clearing in it out to be a bit (no pun) more verbose in storage should be no problem.

So there is no direct need in changing that.

But I do agree that it would be nice to query for all objects user A can delete.

Contributor

sstok commented Sep 2, 2013

Each respected database system already supports for bitmasks.

http://stackoverflow.com/questions/11059182/select-users-from-mysql-database-by-privileges-bitmask
http://stackoverflow.com/questions/2180266/bit-masking-in-postgres
http://consultingblogs.emc.com/jamesrowlandjones/archive/2008/07/04/using-a-bitmask-a-practical-example.aspx

And even SQLite http://zetcode.com/db/sqlite/expressions/

Only MongoDB (and maybe some others) don't support bitmask yet.
But that something that should be fixed in Provider IMO.

A bitmask is nothing more then an integer representation of bits, so clearing in it out to be a bit (no pun) more verbose in storage should be no problem.

So there is no direct need in changing that.

But I do agree that it would be nice to query for all objects user A can delete.

@sukei

This comment has been minimized.

Show comment Hide comment
@sukei

sukei Sep 2, 2013

Contributor

But I do agree that it would be nice to query for all objects user A can delete.

It's possible using SQL. Something like that should do the trick:

create table acl (id serial primary key, bitmask bit varying);

insert into 
    acl (bitmask) 
values 
    (B'0001'), 
    (B'0010'), 
    (B'0100'), 
    (B'1000'), 
    (B'0011'), 
    (B'0111'), 
    (B'1111')
;

select * from acl where (bitmask & B'0001') <> B'0000';

Result:

 id   bitmask  
---- --------- 
  1   0001
  5   0011
  6   0111
  7   1111

This example uses PostgreSQL, but something similar should be done using an other SGBD.

Contributor

sukei commented Sep 2, 2013

But I do agree that it would be nice to query for all objects user A can delete.

It's possible using SQL. Something like that should do the trick:

create table acl (id serial primary key, bitmask bit varying);

insert into 
    acl (bitmask) 
values 
    (B'0001'), 
    (B'0010'), 
    (B'0100'), 
    (B'1000'), 
    (B'0011'), 
    (B'0111'), 
    (B'1111')
;

select * from acl where (bitmask & B'0001') <> B'0000';

Result:

 id   bitmask  
---- --------- 
  1   0001
  5   0011
  6   0111
  7   1111

This example uses PostgreSQL, but something similar should be done using an other SGBD.

fabpot added a commit that referenced this issue Sep 18, 2013

merged branch fabpot/security-split (PR #9064)
This PR was merged into the master branch.

Discussion
----------

[Security] Split the component into 3 sub-components Core, ACL, HTTP

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #9047, #8848
| License       | MIT
| Doc PR        | -

The rationale behind this PR is to be able to use any of the sub components without requiring all the dependencies of the other sub components. Specifically, I'd like to use the core utils for an improved CSRF protection mechanism (#6554).

Commits
-------

14e9f46 [Security] removed unneeded hard dependencies in Core
5dbec8a [Security] fixed README files
62bda79 [Security] copied the Resources/ directory to Core/Resources/
7826781 [Security] Split the component into 3 sub-components Core, ACL, HTTP

@fabpot fabpot closed this Sep 18, 2013

fabpot added a commit that referenced this issue Dec 23, 2013

feature #8650 [Security][Acl] Add MutableAclProvider::updateUserSecur…
…ityIdentity (lemoinem)

This PR was merged into the 2.5-dev branch.

Discussion
----------

[Security][Acl] Add MutableAclProvider::updateUserSecurityIdentity

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #5787
| License       | MIT
| Doc PR        | symfony/symfony-docs#3319

This provides a very simple function to enable the update of a User's username while keeping its associated ACEs (by updating its corresponding UserSecurityIdentity)

Developers can add a listener on the preUpdate of a user and remove all the related ACLs:

```php
if ($args->hasChangedField('username')) {
    $aclProvider = $this->container->get('security.acl.provider');

    $oldUsername = $args->getOldValue ('username');
    $user        = $args->getEntity();

    $aclProvider->updateUserSecurityIdentity(UserSecurityIdentity::fromAccount($user) , $oldUsername);
}
```
Among the problems of not updating the UserSecurityIdentity:

- Inconsistent database, referring to a non-existent user.
- The user loses all its associated permissions
- If another user is created with the old username, it will inherit all the first user’s ACEs

This PR intends to fix Issue #5787 and is similar to and inspired from PR #8305.
This will also be heavily impacted by the outcome of #8848

Commits
-------

da53d92 [Security][Acl] Fix #5787 : Add MutableAclProvider::updateUserSecurityIdentity
@ecentinela

This comment has been minimized.

Show comment Hide comment
@ecentinela

ecentinela Jan 1, 2014

@fabpot did you take a decision about this? I'm in the process of use acl or voters and don't want to take a deprectated/not-oficial component for this matter

@fabpot did you take a decision about this? I'm in the process of use acl or voters and don't want to take a deprectated/not-oficial component for this matter

@jakzal

This comment has been minimized.

Show comment Hide comment
@jakzal

jakzal Jan 1, 2014

Member

@ecentinela Security component was split into three subcomponents (see #9064). Acl is still in the core.

Member

jakzal commented Jan 1, 2014

@ecentinela Security component was split into three subcomponents (see #9064). Acl is still in the core.

@ecentinela

This comment has been minimized.

Show comment Hide comment
@ecentinela

ecentinela Jan 1, 2014

@jakzal I saw it, but I don't know if it's the first step to move out the ACL component.

@jakzal I saw it, but I don't know if it's the first step to move out the ACL component.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment