Skip to content
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

Closed
scrnjakovic opened this issue Jan 30, 2019 · 40 comments
Closed

Latest ACL changes possibly confusing #13791

scrnjakovic opened this issue Jan 30, 2019 · 40 comments
Assignees
Labels
discussion Request for comments and discussion
Milestone

Comments

@scrnjakovic
Copy link
Contributor

scrnjakovic commented Jan 30, 2019

Congratulations on the 4.0-alpha release, great work. :)

A big finger in the eye, however, is Role being renamed to Operation. 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?

@sergeyklay
Copy link
Contributor

@niden I propose to use terms/names from the https://en.wikipedia.org/wiki/Role-based_access_control page.

@tempcke
Copy link

tempcke commented Jan 31, 2019

@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:

The word Resource is a reserved word in php and it was causing all sorts of issues with zephir. That was the primary reason for renaming those. We know this is not going to be a popular change but it had to be done.

As for the role -> operation and resource -> subject, there are plenty of different terms used for those two. As you mentioned you use the term privilidge.

The rationale behind choosing those terms was the scope of each term and usage. So operation is actors of the system that do a particular task. Some people might call that a group. Subject is where this operation needs to be applied. Privilidge would have been equally good.

So that means we simply can not use Resource so lets keep Role as it was and find an alternative to Resource

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:

https://en.wikipedia.org/wiki/Role-based_access_control

That link defines

S = Subject = A person or automated agent
R = Role = Job function or title which defines an authority level
P = Permissions = An approval of a mode of access to a resource

so a Permission is an approval to access a resource granted to a Role or Subject (person)
so to rename Resource to Subject ... breaks these definitions

I would like to add the concept of Subject as a person .. currently in the ACL's I've written I have to create psudo roles such as user_role_USERID .. which inherit from the roles the person belongs to
to load the Acl Memory Adapter

but that is outside the scope of this current conversation ... right now I'm just saying that we should not rename Resource to Subject and for sure not rename Role to Operation
if we can not use the word Resource that is fine but looking at the definitions on that article, Subject is not the correct term.

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

@tempcke
Copy link

tempcke commented Jan 31, 2019

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

@ruudboon
Copy link
Member

My preference goes out to staying with Role and add a prefix like AclResource or AppResource.

@scrnjakovic
Copy link
Contributor Author

I think @sergeyklay suggested renaming Role to Subject, not Resource to Subject. Either way, I think we should come up with something better as Subject may also cause confusion: one could view Subject as a subject who is being granted certain permission (Role) or they could see Subject as a subject for which permission is granted (Resource). So at least in my head Subject can go either way.

I think we agree on Operation being inappropriate substitution for Role.

While resource is reserved as of PHP7, role is not, which is why I suggest following:

  1. Revert Operation to Role and find replacement for Subject, previously Resource; or
  2. Revert Operation to Role and keep Subject; or finally
  3. Prefix Role and Resource with Acl so they become AclRole and AclResource

What do you think?

cc @Jurigag @niden

@scrnjakovic scrnjakovic changed the title Latesr Acl refactor Latest ACL changes possibly confusing Jan 31, 2019
@tempcke
Copy link

tempcke commented Jan 31, 2019

I'm good with 1 or 3 .. though 3 is somewhat Odd as the namespace already has it .. so it would be Phalcon\Acl\AclResource

maybe there is a better prefix or suffix would work

@Jurigag
Copy link
Contributor

Jurigag commented Jan 31, 2019

I guess third is fine. Though it's hard because there is no ideal solution.

@niden
Copy link
Member

niden commented Feb 1, 2019

As I mentioned in discord, I really don't mind if we change the names to Jack and Jill or Vanilla and Chocolate. So long as the community understands what the component does etc. then we are all fine.

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?

@scrnjakovic
Copy link
Contributor Author

@niden I agree that it's impossible to have everyone satisfied. And the reason behind renaming Resource is perfectly clear: resource is PHP's reserved word as of PHP7 and it had to go.

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 Operation as substitution for Role will cause a lot of confusion as operation implies verb / action. Try it in plain language: Operation Admin is allowed to take action Delete on subject Article.

@niden
Copy link
Member

niden commented Feb 1, 2019

Stefan, the cool comment was not for that reason. It was to strengthen the argument that we had to change it. (just fyi) :)

@diplopito
Copy link
Contributor

+1 for the Acl prefix: it is redundant but we understand why.

@niden niden pinned this issue Feb 1, 2019
@tempcke
Copy link

tempcke commented Feb 1, 2019

@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!

Resource should be renamed Component

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 Subject is a defined term in this context .. it refers to the PERSON wanting to preform the action ... and NOT the component/resource they want to preform the action on.

As for Operation @scrnjakovic has already very clearly explained why that is not a good fit. if the word Operation where to represent something one would expect it to represent the action to be preformed, not the actor or group to which the actor is a member. If we MUST rename Role (which I did not think we did have to?) then Group or Team would be better fits as they represent a collection of actors associated together by some common attribute.

@adam-rocska
Copy link

adam-rocska commented Feb 1, 2019

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’

@tempcke
Copy link

tempcke commented Feb 1, 2019

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

@adam-rocska
Copy link

@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.

@jwhenry3
Copy link

jwhenry3 commented Feb 1, 2019

@tempcke and I did some brainstorming on word association with the tech-related terminology relating to Resource, and Component seemed to be one of the closest transposable words that we could think of. In my mind, another that would compare is Scope as a Resource could refer to a section of an application, or something as narrow as a single tool or entity.

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.

@jwhenry3
Copy link

jwhenry3 commented Feb 1, 2019

As much as I would like to agree with a prefix, I have to disagree with that approach.
In the past we have used prefixes like "ISomething" to indicate something was intended as an interface, but there are keywords to identify that now. In the past we have also had snake-cased prefixes to satisfy namespacing requirements, but that is no longer an issue.

If we honestly go the prefix route, I would opt for one that doesn't contain the namespace. Something like AccessibleResource or RestrictedResource in which the intent was clear.

@rudiservo
Copy link
Contributor

Is it possible to use a new namespace called maybe called RBAC and let the ACL just go slowly into deprecated?

@jwhenry3
Copy link

jwhenry3 commented Feb 1, 2019

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.

@ghost
Copy link

ghost commented Feb 1, 2019

I'm all in favor of @tempcke opinion, in keeping the https://en.wikipedia.org/wiki/Role-based_access_control .

  • It's ok, to change some things when they evolve due technology and become something else, but changing terms that are already defined for years and remain the same thing is far from ideal as people are already aware of what those terms mean.

That' my opinion.

@rudiservo
Copy link
Contributor

@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.
It is going to hurt either you indent or change, just make it a definitive logical standard.

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.

@ghost
Copy link

ghost commented Feb 1, 2019

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.

@sergeyklay sergeyklay added the discussion Request for comments and discussion label Feb 1, 2019
@aircodepl
Copy link

aircodepl commented Feb 1, 2019

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

$operationAdmins = new Operation('admins', 'Administrator Access'); $operationAccounting = new Operation('accounting', 'Accounting Department Access');

So back to Role for me. Simplicity and consistency is the key.

@naliferov
Copy link

@scrnjakovic
Copy link
Contributor Author

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.

@rudiservo
Copy link
Contributor

@UpAbove +1
And with the standard of Operation in RBAC is actualy the Object/Resource/Action/Method.

@danhunsaker
Copy link

danhunsaker commented Feb 2, 2019

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 "Operation [ has | doesn't have ] access access to Subject". An immediate response to that might be, quite reasonably, "OK, so Operation is user/role, and Subject is the target object to grant access to", and then to be confused why those names were chosen. This is actually a bit naive, however, as whatever "has access to" something might not be a user or a role at all - it could be any object throughout the application, or even something external to the application itself, such as "CDN has read access to Assets". So Role isn't quite accurate even in the first place. I suspect Operation was originally intended to be Operator?

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:

Actor [ has | doesn't have ] scope access to Target

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 Actor, and act upon other things in your app. Meanwhile, anything can also be a Target, and be the target of actions being taken. And any given Actor can only take actions within approved scopes on any given Target, to prevent accidental or malicious changes and breakage.

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.

@davidsharpbell
Copy link

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.

@sergeyklay sergeyklay added this to the 4.0.0 milestone Feb 3, 2019
@niden
Copy link
Member

niden commented Feb 5, 2019

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.

@niden
Copy link
Member

niden commented Feb 5, 2019

@scrnjakovic @tempcke can you guys take the lead here and get the results? Do we need to do a poll or something?

@tempcke
Copy link

tempcke commented Feb 5, 2019

Near as I can tell reading the history of this thread we have these options

  1. @ruudboon proposes we prefix the original names to AclRole and AclResource
  2. @danhunsaker proposed Role to become Actor and Resource to become Target because what we have is not a real ACL, so he is avoiding acl terms and choosing terms which better reflect the objects intent.
  3. @tempcke (me) proposed we keep Role as is, and change Resource to Component

@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 Acl My idea, number 3 seems the smallest change and keeps a meaningful name rather than a prefix.

I could back any of these 3. They are all better than Operation and Subject (no offense)

what is the best way to do a community vote?

@tempcke
Copy link

tempcke commented Feb 5, 2019

Great idea @ruudboon So please vote with Icon reactions to this comment:

😄 change Acl\Role to Acl\AclRole and Acl\Resource to Acl\AclResource
❤️ change Acl\Role to Acl\Actor and Acl\Resource to Acl\Target
🚀 keep Acl\Role as is and change Acl\Resource to Acl\Component
😕 meh?

@danhunsaker
Copy link

danhunsaker commented Feb 5, 2019

@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.

@aat2703
Copy link

aat2703 commented Feb 6, 2019

A very big 🚀

@danhunsaker
Copy link

danhunsaker commented Feb 7, 2019

The poll is now CLOSED. Final numbers:

1 - AclRole and AclResource: 0
2 - Actor and Target: 10
3 - Role and Component: 14
4 - ¯\_:roll_eyes:_/¯: 2

So Role and Component it is.

If you voted elsewhere and feel your vote should be added to these totals, please let me know.

@niden
Copy link
Member

niden commented Feb 7, 2019

@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)

@scrnjakovic
Copy link
Contributor Author

@niden isn't that redundant? I thought you were gonna give him the honors of writing a PR instead :D

@tempcke
Copy link

tempcke commented Feb 7, 2019

@niden issue created here: #13808
As for doing the PR myself I have yet to work in zephir. If I jetbrains refactor/rename works in zephir then I'd be happy to as this becomes super easy. If not I would fear missing some reference?

@sergeyklay
Copy link
Contributor

Please note: there is comments, docstring, tests, change log, blog post, directory and file names, etc

@niden
Copy link
Member

niden commented Feb 8, 2019

Closing this in favor of #13808

@niden niden closed this as completed Feb 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Request for comments and discussion
Projects
None yet
Development

No branches or pull requests