-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Latest ACL changes possibly confusing #13791
Comments
@niden I propose to use terms/names from the https://en.wikipedia.org/wiki/Role-based_access_control page. |
@scrnjakovic you are right, the renames are unpleasant. We have been discussing this very thing on discord and so I'll share some of those notes in here. In the chat log Niden explains the reason for the change... Niden said:
So that means we simply can not use In my view as you add an array of Actions along with a Resource to the Acl Memory Adapter, each Resource+Action pair is a Privilege ... so to rename Resource Privilege does not work either. Klay found a good source for established terms: That link defines
so a Permission is an approval to access a resource granted to a Role or Subject (person) I would like to add the concept of but that is outside the scope of this current conversation ... right now I'm just saying that we should not rename More often than not a Resource in an application is a specific section or module ... an Admin section .. or a ManageFoo section would be a protected resource that requires permission to enter or preform actions on ... or it could be an entity .. and permission is needed to preform actions on it ... Resource is such a nice term because in http each endpoint /foo/bar is called a Resource ... and the client application requests to preform actions on those resource endpoints |
Because it was not pasted above I'll add this link now. The source of this issue is the changes in 4.0.0 described here: https://docs.phalconphp.com/4.0/en/upgrade#acl |
My preference goes out to staying with Role and add a prefix like AclResource or AppResource. |
I think @sergeyklay suggested renaming I think we agree on While
What do you think? |
I'm good with 1 or 3 .. though 3 is somewhat Odd as the namespace already has it .. so it would be maybe there is a better prefix or suffix would work |
I guess third is fine. Though it's hard because there is no ideal solution. |
As I mentioned in discord, I really don't mind if we change the names to The key here is that we do not want to re-invent terminology just to be cool. There was a valid reason to make this change and we chose terminology already used for this purpose in the industry for the rename. As it seems that is good for some, bad for others. As Juri mentioned there is no ideal solution so some people are not going to like it some will. My only concern is that yeah Stefan and Todd do not like it, some here are neither here nor there but the bulk of the developers using the framework has not replied or at least indicated yes or no or offer another solution. Not sure how we can bring more people to this other than post it on social media.... At which point, is it worth it to do so? |
@niden I agree that it's impossible to have everyone satisfied. And the reason behind renaming But I don't think we are trying to change the terminology just to be cool. When you renamed those classes, you had community's best interests in mind. So am I now trying to point out that |
Stefan, the |
+1 for the Acl prefix: it is redundant but we understand why. |
@niden Alright. I brought this up to my team here and after 30 mins of discussion we found the solution. Which oddly enough is a word you used in your comment!
is the purpose of an ACL not to allow or deny actions on various components ? .. it seems like a good fit. It avoids a somewhat odd prefix which is already in the namespace .. and it avoids using terms that many will find confusing. For the rename you did chose words related to ACL but you chose the wrong words in my opinion. As I mentioned above the term As for |
Reinventing the wheel in terms of naming is worst than reinventing the wheel in terms of implementation. Plase, if possible, stick to widespread language agnostic terms. Like the wikipedia page describes what ACL is... if this would be some sort of a voting or somethin’ |
That sounds great however the wikipedia page calls it a resource ... and we are not allowed to use that term without a prefix or suffix |
@tempcke As previously suggested by other commenters : A prefix or suffix could do the trick. Or finding a synonim using a dictionary. Role and Operation are like Chicken and Potato. |
@tempcke and I did some brainstorming on word association with the tech-related terminology relating to Regardless what replacement is used, I agree with @adam-rocska that the replacement must be as recognizable as possible and have a close enough meaning. Considering REST developers see resource as a topic or entity as defined in a route, then something like classification|category|scope|component makes sense. |
As much as I would like to agree with a prefix, I have to disagree with that approach. If we honestly go the prefix route, I would opt for one that doesn't contain the namespace. Something like |
Is it possible to use a new namespace called maybe called RBAC and let the ACL just go slowly into deprecated? |
You shouldn't need to change a namespace to satisfy a single class naming issue brought on by an update in PHP. We would still have the same issue and if you're going to have to prefix the class to avoid the issue, the namespace change is completely redundant and unnecessary. |
I'm all in favor of @tempcke opinion, in keeping the https://en.wikipedia.org/wiki/Role-based_access_control .
That' my opinion. |
@jwhenry3 The thing is a Full Fledge RBAC is much more then a simple ACL. @sergeyklay @niden @tempcke @scrnjakovic 50% of the issue of engineering something in software is naming and structure, and naming things is the hardest part, in this case since this is a reserved word issues will arise quickly and stability is important, so we are past the part of should we change or not, it has to be changed and with the adopted standard. Now here comes the problem that I think is happening, what phalcon has ATM is neither pure ACL or full RBAC, more like something in between that is a more modern ACL with role management, maybe that is a bit confusing to people, although they seem the same in function they are not. In ACL Resource = Object, Resource is an API Slang term not and ACL one. Also IMO the ACL, RBAC and ABAC are part of a bigger namespace (i.e. AccessControl, AAA, etc) and we might need to also make that transition in the future. |
Whatever the change will be, I think the choice must be a name that is both logical and understandable, so that people using Phalcon don't get confused, because that's annoying for other developers. |
Keep in mind that newcomers and non-english people like me find it best to use names which are logical and understandable, as @joaorsilva mentioned. While concept is understandable for me, this example from documentation looks so wrong and explains whole problem by itself: https://docs.phalconphp.com/4.0/en/acl-adding-operations
So back to Role for me. Simplicity and consistency is the key. |
I’m glad this caught attention, I raised this issue with an intention to improve on changes done recently so ACL classes make sense and more importantly, don’t confuse people. Now, we would all benefit if we started laying out ideas. |
@UpAbove +1 |
I was asked to chime in, here. So to start, ACLs aren't RBAC aren't ACLs. Using RBAC terminology for ACLs might be misleading in itself, especially as the framework's auth capabilities get more robust and complex/varied. As noted earlier, there's a good chance of adding an actual RBAC Auth option, along with (probably) many others. So, focusing on what RBAC uses seems .... distracting, in this case. If we look, instead, at what these various components actually do, then, we might get more informative names to work with. Best I can see, from the code alone, is that we're building lists of " My proposal, coming at this from a mostly outside perspective (I'm on the Zephir team, and haven't gotten to use Phalcon in anything, yet), is to adjust the naming yet again (as requested in this issue), and to use the following template for the concepts involved:
This decouples the entire ACL feature from explicit concepts of Users, Roles, Groups, Components, etc entirely. It's still clear what each portion of the feature is responsible for, but doesn't tie either to any particular meaning beyond the bare minimum of what it does / is used for. This, in turn, enables developers to think more freely about what kinds of access control make sense for their application. It might be Users, Roles, and Groups with access to Components and Models, but it might also be Components with access to certain actions on various aspects of Users, Roles, and Groups. Anything in your app can be an ACLs are very simple, which is a big part of what makes them very flexible. We should be using names that stay out of the way of that. |
I'd be the opinion that you shouldn't fix something that's not broken. If role-based access control has already been defined, we should use it and its defined terminology unless its inherently broken in our use case. If terms have already been protected by PHP and defined by our source of inspiration (role-based control), then we should simply prefix it. |
So what is the consensus here guys? Leave it, Rename it, Prefix it? I am happy to do the work if it is the latter two. We will not satisfy everyone for sure but we need to at least move towards what the community wants. |
@scrnjakovic @tempcke can you guys take the lead here and get the results? Do we need to do a poll or something? |
Near as I can tell reading the history of this thread we have these options
@danhunsaker if I did not accurately represent your proposal please re-state it in as few words as possible as we can clearly understand your reasoning from your original post but I'm unsure if I got the final proposal down correctly. as for 1-3, @ruudboon 's idea seems the most popular, though I'm convinced we can do better. I personally like @danhunsaker 's points, however I'm left wondering if the namespace should still contain I could back any of these 3. They are all better than what is the best way to do a community vote? |
Great idea @ruudboon So please vote with Icon reactions to this comment: 😄 change |
@tempcke - That's exactly it! NOTE: This poll will close (that is, the numbers at that point will be used to move forward) at 2019 Feb 07 16:30 UTC (two days after it was opened). Any votes cast after that point will not affect the framework moving forward. |
A very big 🚀 |
The poll is now CLOSED. Final numbers: 1 - So If you voted elsewhere and feel your vote should be added to these totals, please let me know. |
@tempcke since your proposal is the winner can you do the honors and create an issue so that I can sort that out ? (chop chop! :D :D) |
@niden isn't that redundant? I thought you were gonna give him the honors of writing a PR instead :D |
Please note: there is comments, docstring, tests, change log, blog post, directory and file names, etc |
Closing this in favor of #13808 |
Congratulations on the 4.0-alpha release, great work. :)
A big finger in the eye, however, is
Role
being renamed toOperation
. Frankly, this is just confusing and wrong. I don’t see how “Admin” or “Guest” could be an operation? Operator, maybe, but operation = action.Anyone want to shed some light on this one?
The text was updated successfully, but these errors were encountered: