8000 Move the ACL part of the Security Component outside of Symfony · Issue #8848 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
fabpot opened this issue Aug 24, 2013 · 26 comments
Closed

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

fabpot opened this issue Aug 24, 2013 · 26 comments
Labels
Security

Comments

@fabpot
Copy link
Member
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
@fabpot fabpot mentioned this issue Aug 24, 2013
Closed
@hhamon
Copy link
Contributor
hhamon commented Aug 24, 2013

+1

@schmittjoh
Copy link
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 probl 8000 em 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
Copy link
Contributor

+1

I would gladly volunteer to help improving this component.

@ste93cry
Copy link
Contributor

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
Copy link
Contributor
inoryy commented Aug 26, 2013

+1 on simplifying
-1 on moving

@phpmike
Copy link
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
Copy link
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
Copy link
Contributor
sukei commented Aug 26, 2013

@lavoiesl +1 to help improve this component too.

@guillaumepotier
Copy link

+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
Copy link
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
Copy link
Contributor

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

@lavoiesl
Copy link
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.

@sukei
Copy link
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
Copy link
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.

@lemoinem lemoinem mentioned this issue Aug 26, 2013
Merged
@sukei
Copy link
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
Copy link
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.

@lsmith77
Copy link
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.

@michaelcullum
Copy link

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

@ginsen
Copy link
ginsen commented Aug 28, 2013

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

@Taluu
Copy link
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
Copy link
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 strong 8000 ly 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
Copy link
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
Copy link
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.

This was referenced Sep 16, 2013
Closed
Merged
fabpot added a commit that referenced this issue Sep 18, 2013
@fabpot
merged branch fabpot/security-split (PR #9064)
b44bc0e 
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 as completed Sep 18, 2013
fabpot added a commit that referenced this issue Dec 23, 2013
@fabpot
feature #8650 [Security][Acl] Add MutableAclProvider::updateUserSecur…
8d39213 
…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
@jakzal jakzal mentioned this issue Dec 28, 2013
Closed
@ecentinela
Copy link

@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
Copy link
Contributor
jakzal commented Jan 1, 2014

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

@ecentinela
Copy link

@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
Assignees
No one assigned
Labels
Security
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
0