NTFS / Permission inheritance

[danyellnoe]

Greetings and thanks for your work on this package.

I just spent the last couple hours trying to figure out why the permissions inheritance was not working as per your example in the wiki. Alas, it was the 'ntfs' setting in the config file.

I suggested an edit on the Wiki page to inform the reader that the example will not work as expected if the default value of true is left unchanged.

Do you believe it is a more common desire to have the default behavior of "more permissive wins"? I would think not, but I have not implemented many ACL systems, so perhaps this is my ignorance.

[cirdog]

I'd also like to join into this discussion to give my 2 cents, surely I'm no expert but I still hope to contribute some ideas if it ever helps on this useful module.

On naming

I too agree that the name ntfs might not be as descriptive. How about just call it permissive? I think it's very suitable & straightforward.

On permissiveness

Say (ntfs == true) being permissive & the opposite being restrictive. It affects only how the permission hierarchy would be laid out. On permissive settings, the permission closer to root SHOULD be more restrictive, and the leaves would override their parents by permitting, and vice versa.

So for permissive setting, I just lay it out like a firewall table, deny all by default at root, then allow by exception. For restrictive setting, it allows all then deny by exception.

On Hierarchical Permission

I think being "Globally" permissive or restrictive would only be a transitional solution for now. In a long run maybe we need a "Hierarchical" permission inheritance where the children override only their immediate parents (edit: no level wins globally), just like the common ACL implementations. Then "permissive vs restrictive" would become a non-issue.

Say now we are evaluating the "resulting permission" of a user, we fetch his role-permissions & user-permissions as a tree, then we flatten the involved paths into a table, where children override parents, then we have the user's resulting permissions.

Implementation

But the technicality of such "Hierarchical" layout would require a quick fetching of the "path of permission" from the tree, and I'd suggest looking into the MPTT (concept & laravel module) implementation.

EDIT: the above is either overkill or mistaken...

I by no means have a working prototype but just some rough ideas for sharing. I personally think @kodeine's implementation of ACL middleware is elegant and I'd like to submit some pull requests if my free time allows. However I'll keep on chatting for now since recently I'm quite busy.

Thanks for your consideration and we shall further explore the implementation of such "hierarchical permission" usage.

[danyellnoe]

@cirdog Wow, looking at that nested set pattern stuff just gave me a flashback to studying CS in college some dozen years ago! Good stuff... Actually, you provide a lot of good information, so thanks!

I generally agree with your suggestions. Also, note that the approach of starting with least permissive at the top of the inheritance hierarchy was also previously suggested by @ssssteve in issue #12 , where he also gives a great example that could be added to the documentation.

I think it does make more sense for this "firewall approach" to be the default, so that failed inheritance does not result in unintended access rights. This is precisely the undesirable result I ended up with from attempting to follow the current example in the documentations without switching the "ntfs" default to true.

Therefore, to immediately alleviate confusion and improve the general security of this ACL system (with very little work), I propose the following:

1. _Change the name of the config value from ntfs to most_permissive_wins_ - I am very verbose when it comes to variable names and I noticed @kodeine used a similar phrase in previous discussions about this matter.
2. _Change the default value of the most_permissive_wins variable from true to false_ - This will support the "firewall approach" where you begin with denying all access at the top of the tree and will also maintain the ability to override permissions all the way down the tree, thus maintaining hierarchical integrity.
3. _Update the documentation to use @ssssteve's example as the recommended way of structuring the hierarchy_ - This way, users are more likely to implement in the safer, deny-all-first "firewall approach".

Would love to hear @kodeine 's input on this...

[cirdog]

@danyellnoe

+1 on your proposal.

I just had the urge to come back to tone down my nested set idea, I think it's overkill (or even mistaken).

Maybe a simple ancestor-trace would do the job of letting the children overriding its immediate parent, level by level, no matter how the permission was laid out (you know it could ends up being a mess after years of usage).

Please don't mind me if I'm mistaken on other places, I better go back to read the code more carefully before ranting here & there... heh.

[danyellnoe]

@cirdog Well, I'm not entirely sure if I'm on target with this stuff or not either. But I think that's what these types of discussions are for! @kodeine has likely spent much more time thinking about this stuff recently than you or I, thus why I will am interested to hear the feedback.

[kodeine]

@danyellnoe @cirdog

I agree with changing the name ntfs to most_permissive_wins and setting its default to false. It really comes to the implementation of each app. But i guess setting it to false by default would be good for most of the people as most have firewall approach. If you guys could update the wiki, i'll be pushing mentioned updates.

Also, i was thinking to create a general control panel implementation of this acl, so anyone can use the base panel in their implementation easily. Any thoughts on that?

[danyellnoe]

Hey @kodeine, I can lend a hand on the wiki. And I do have some thoughts on the control panel as I just integrated both role and permission controls into the admin system I am currently building at work. I am pretty busy today, but I will be happy to share thoughts with you this weekend.

[kodeine]

@danyellnoe sure that would be great. Also if you can show us the screen shots in case you cant share control panel source.

[danyellnoe]

@cirdog Sorry, I'm out of town right now, but I will follow up on this soon. Hasn't fallen off the radar!

[kodeine]

@danyellnoe @cirdog Ive pushed our discussed updates to master branch. Let me know if that is okay

[danyellnoe]

Let me know when and I will help update the wiki. Should be straightforward. Will include the example mentioned before.