Role based ACLs

[flovilmart]

No description provided.

[codecov]

Codecov Report

Merging #5000 into master will decrease coverage by 0.02%.
The diff coverage is 95.45%.

@@            Coverage Diff             @@
##           master    #5000      +/-   ##
==========================================
- Coverage    94.3%   94.28%   -0.03%     
==========================================
  Files         121      121              
  Lines        8760     8763       +3     
==========================================
+ Hits         8261     8262       +1     
- Misses        499      501       +2

Impacted Files	Coverage Δ
src/Auth.js	99.32% <95.45%> (-0.68%)	⬇️
...dapters/Storage/Postgres/PostgresStorageAdapter.js	97.21% <0%> (-0.09%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6172018...bf1d2e0. Read the comment docs.

[georgesjamous]

@flovilmart

Your code is building the role names as we go on, and will use them in the next query by simply indicating this.fetchedRoles = true, but this may not cover all cases. More test should be performed but a quick look revealed a different reasoning from the one i attempted to achive:

I noticed that in the first iteration when the direct roles are fetched, Roles have to be Publicly Readable, otherwise, the query won't yield results.
Also, in each iteration after that, the inherited role's ACL have to either be Publicly Readble or Readable By its children roles (or any previously fetched roles by chance) for this to work.

Expl:

r1.getUsers().add(user)
r2.getRoles().add(r1)
r3.getRoles().add(r2)
this is the resulting branch: R1 -> R2 -> R3

In order for the user to inherit all of these roles:
R1: Must be either Publicly Readable or Explicitly Readable by the user.
R2: Must be either Publicly Readable or Explicitly Readable by the user or R1.
R3: Must be either Publicly Readable or Explicitly Readable by the user or R1 or R2.

if R3's ACL is different, lets say Read/Write by R3 only, then on query number 3:
We have user.roles = [userId, r1, r2]. R3 will not be fetched.

This is what i got from your code, or did i miss something ?
And is this the intended Role/ACL result(reasoning) you were looking for ?

[georgesjamous]

If my previous reasoning was correct,
The way I see it, depending on how we define Role ACL, we should shape the solution.

• Roles should not follow the ACL. (Current)
  Easiest, but far less customization and control over roles.

• Roles should follow a simpler ACL reasoning.
  PubliclyReadable (means ON), MasterKeyOnly (means OFF).
  Adds the advantage of new "Disabled" like feature but using ACLs.
  "A Role provides Users and Roles it owns, its Powers as long as its ACL is Publicly Accessible, when its not, the role i considered Disabled."

• Roles should follow the ACL like any other Object.
  Solution is more complex, but provides more role control and customization.

[flovilmart]

if R3's ACL is different, lets say Read/Write by R3 only, then on query number 3:
We have user.roles = [userId, r1, r2]. R3 will not be fetched.

This is true, and this makes sense. You have to be able to read the role in order to benefit from it’s features (this is the feature being built now)

With the current implementation, role follow ACL’s like any other object, as well, we’re using ACL’s in order to discover which roles are applied to the user based on what is already discovered. Any other implementation in not following the ACL’s.

The only difference with your implementation (besides the amount of code and custom logic), is that self referencing roles can’t be discovered (Role3 only readable by Role3). And to be honest I’m not sure if that’s a corner case that we wanted in the first place.

[georgesjamous]

self-referencing roles can't be discovered (Role3 only readable by Role3)

You meant can be discovered.

I am not completely disagreeing with you on this, its a fine and is the middle solution for now.
But the only thing i don't like about it and seem to confuse me, is the fact that Roles are being computed on different conditions depending on their level. (how deep they are)

In other words, if we have 2 roles: Admins and Staff.
Then for some reason, admins loose their public readability. Users from that role will loose the role as well as the Staff role which they should inherit by role hierarchy.
Whereas, Staff role users will still have their access to the Staff role.

But from a general role reasoning, the lower level roles should loose their privileges before the higher level ones, not the other way around. It doesnt make sence in my openion to allow such condition to take place. Its just a mater of small technicality.

If you feel that this is a good solution, i am all for it. At least it adds a bit of flexibility.

[georgesjamous]

In other words, if we have 2 roles: Admins and Staff.
Then for some reason, admins loose their public readability. Users from that role will loose the role as well as the Staff role which they should inherit by role hierarchy.
Whereas, Staff role users will still have their access to the Staff role.

Small disclaimer, just noticed that the solution i proposed before does not get around that problem also. This could still happen if we explicitly prevent admins from accessing their role. (such like when we set the read access to master key only)

[flovilmart]

Ok let's backtrack a minute, If you wanted to disable a role right now, you'd delete it right? If you'd need to adjust the role hierarchy, you'd move the users from one role to the other that would be the base role, or you would readjust the linking between the roles.

But the only thing i don't like about it and seem to confuse me, is the fact that Roles are being computed on different conditions depending on their level. (how deep they are)

They are not, they are recursively discovered. The first layer of roles is and should always be the roles where the users is directly assigned. The n up layers of roles are the roles that are referenced by the discovered roles in the previous steps.

Then for some reason, admins loose their public readability. Users from that role will loose the role as well as the Staff role which they should inherit by role hierarchy.
Whereas, Staff role users will still have their access to the Staff role.

Well that's true, but your role is disabled and doesn't exists anymore, as well as all its associated privileges.

What you describe is not roles following ACL's. It's roles following a complex logic of inheritance where a container role can be deleted and it's children associations are still available. And given the complexity of the code involved in this logic I am totally against supporting it.

[georgesjamous]

That's true, and I was not going to suggest supporting such logic, let's keep it simple then.
I am with your solution at this point.

[acinader]

I have now taken two quick passes. Has not sunk in yet. Will come back to it again. At the least, I should be able to generate some questions. It is not obvious to me yet what is changing here or why.

[acinader]

intentional?

[acinader]

is this comment still correct....

[acinader]

Took a look 3rd try. I re-read the comments to try and guess what issue this PR is trying to solve, and so far, I am still not sure. The operative change in src/Auth.js is not clear to me what the purpose or intent is.

[flovilmart]

@acinader the goal is to make role discovery dépendant on ACL’s instead of being run with the masterKey. This way, this let users enable and disable roles by changing their visibility through ACLs

[acinader]

ok, I think I get what's going on.

what's the plan with the tests? do you want to leave all the XIT's in?

[flovilmart]

I need to have a look, but I believe the xited tests are irrelevant to the current behavior

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[parse-github-assistant]

Thanks for opening this pull request!

• ❌ Please edit your post and use the provided template when creating a new pull request. This helps everyone to understand your post better and asks for essential information to quicker review the pull request.