Roles should follow acls #4895

georgesjamous · 2018-07-14T10:30:46Z

This is related to #4821
Since roles are used to enforce ACL on another object, they cannot be used to enforce ACL on themselves. So the only way I could think of to do this is to fetch all roles normally and build a tree-like list and compute ACL access manually on each Role.
Code enhancements could still be done, I would like another set of eyes on this since its a security matter. Maybe another more straightforward way is available.

Reasoning:
Branch : R1 -> R2 -> R3 -> R4
This is a role tree-branch where R1 is directly accessible (user is in "users" relation).
R2 is fetched because of R2.getRoles().add(R1). R3 is fetched because of R3.getRoles().add(R2)
Computation:
In order to compute access, we need to check every node from the inside out.
R4 then R3 then R2 then R1. If at any time the cycle breaks (ie, we stumbled upon a role that is inaccessible) we throw the branch. (ie, if R2 is inaccessible, R4, R3 & R2 are rejected)
Multiple paths are computed in an Or manner. (ie, at least one path need to be ok)
R1 -> R2 -> R3 -> R4 and R6 -> R4 (for R4 we have R6 'Or' R3 -> R2 -> R1)

Caveats:
Throwing a role and be done with it is not enough, a second branch, later on, could provide us with access to a role we already rejected. A brute force operation would be to retry everything whenever a role is accepted. So in order to do this in a better way, the code will keep a reference to the role that rejected a role and will retry it explicitly when the first role is accepted.

Concerns:
Accepting this PR means that it will break some apps since Role ACLs is not being taken into consideration during "read". How about a server level configuration option to select which type of Role permissions to use "legacy" or "advanced" for example.
Or, we could set ACL by default that a role is 'read' by himself if not explicitly set to 'false' when creating a role.

codecov · 2018-07-14T10:39:58Z

Codecov Report

Merging #4895 into master will decrease coverage by 0.16%.
The diff coverage is 89.62%.

@@            Coverage Diff             @@
##           master    #4895      +/-   ##
==========================================
- Coverage    94.3%   94.14%   -0.17%     
==========================================
  Files         121      122       +1     
  Lines        8760     8845      +85     
==========================================
+ Hits         8261     8327      +66     
- Misses        499      518      +19

Impacted Files	Coverage Δ
src/Auth.js	`100% <100%> (ø)`	⬆️
src/AuthRoles.js	`89.14% <89.14%> (ø)`
src/Adapters/Storage/Mongo/MongoStorageAdapter.js	`91.33% <0%> (-0.75%)`	⬇️
src/RestWrite.js	`92.97% <0%> (-0.37%)`	⬇️

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...96b8ce8. Read the comment docs.

flovilmart

Thanks for the PR, there's a lot of inconsistencies in the style (I know it should be enforced by the eslint but somehow it isnt). Can you make it consistent, including the trailing semi-colons, spaces in if (condition) { etc...

flovilmart · 2018-07-17T12:39:18Z

spec/ParseRole.spec.js

@@ -76,71 +78,13 @@ describe('Parse Role testing', () => {
    return role.save({}, { useMasterKey: true });
  };

-  it("should not recursively load the same role multiple times", (done) => {


we should keep that test please.

Alright, no problem.

flovilmart · 2018-07-17T12:41:41Z

spec/ParseRole.spec.js

      // For each role, fetch their sibling, what they inherit
      // return with result and roleId for later comparison
      const promises = [admin, moderator, contentManager, superModerator].map((role) => {
-        return auth._getAllRolesNamesForRoleIds([role.id]).then((result) => {
+        const authRoles = auth.getAuthRoles()


Ok so this test fails because the roles are not readable by any one so not applied

flovilmart · 2018-07-17T12:44:37Z

src/AuthRoles.js

+  for (let index = 0; index < parentIds.length; index++) {
+    const parentId = parentIds[index];
+    const parentRole = this.manifest[parentId]
+    if(!parentRole) continue;


can we get curly brackets here?

flovilmart · 2018-07-17T12:52:37Z

src/AuthRoles.js

+ * Resolves with a promise once all roles inherited by a role are fetched.
+ * Inherited roles are roles a single role has access to by the 'roles' relation.
+ */
+AuthRoles.prototype.findRolesOfRolesRecursively = function() {


We prefer algorithms without side effects instead of mutations of the global object state: ie: instead of storing the properties in the global state object, store the result of each operation locally, and then return the final result after the recursion for example:

function somethingResursive(currentState = {}) { if (breakingCondition) { return currentState; } const result = doSomething(): const mergedResult = mergeStates(currentState, result); return somethingResursive(mergedResult); }

Alright, let me see what I can do about this.

It’s cleaner and more testable usually :) also, please use ES6 classes now :)

flovilmart · 2018-07-17T12:55:09Z

src/AuthRoles.js

+  this.auth = auth
+  this.userId = auth.user ? auth.user.id : undefined
+  this.master = master
+  this.isMaster = isMaster;


What is the difference between those two? Why do we need both?

Actually i dont think we do. I am working on another branch in which I removed all these unneeded and duplicate checks.

flovilmart · 2018-07-17T12:56:28Z

src/AuthRoles.js

+export function AuthRoles(auth: Auth, master, isMaster = false) {
+  this.auth = auth
+  this.userId = auth.user ? auth.user.id : undefined
+  this.master = master


this parameter shoould likely be config or masterConfig

flovilmart · 2018-07-17T12:57:42Z

src/AuthRoles.js

+    const name = roleNames[index];
+    const statement = acl[name]
+    if(statement){
+      if(statement["read"] === true) return true


please avoid those at all costs, they don't help readability nor maintainability

if (statement["read"] === true) { return true; }

flovilmart · 2018-07-17T12:59:26Z

src/AuthRoles.js

+
+// Or
+function isAnyExplicitlyGranted(acl, roleNames){
+  for (let index = 0; index < roleNames.length; index++) {


use

roleNames.find((name) => { /// logic here });

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/find

I have actually found that its faster to loop over the ACL and look through the role names from a set (.has())
I will change it for you to review

georgesjamous · 2018-07-21T08:03:36Z

src/AuthRoles.js

+ * "read" is true
+ * @returns {Boolean}
+ */
+const _isReadableAcl = (statement) => statement.read === true


@flovilmart don't know if this is needed. I don't recall if an ACL with r/w set to false can be available, because the read key all together will not be available. (ie "roleName" : { "write": true }. without read).
But I thought I would explicitly check the 'read' access.

georgesjamous · 2018-07-21T08:04:57Z

src/AuthRoles.js

+    const acl = role.ACL;
+    // Dont bother checking if the ACL
+    // is empty or corrupt
+    if(acl === {} || !acl){


Not sure if this could happen

georgesjamous · 2018-07-21T08:14:35Z

spec/ParseRole.spec.js

+
+      // 1 query for the direct roles (parent roles).
+      // 1 query for each role after that, including the parent role.
+      expect(restExecute.calls.count()).toEqual(5);


@flovilmart An increase in calls is to be expected here since for each role we have to know the parent to properly compile the tree.

georgesjamous · 2018-08-06T13:30:55Z

@flovilmart i am not sure how to request a review this point :)
my changes are complete, i just have to resolve the new conflict that appeared and that i just noticed.

flovilmart · 2018-08-06T13:38:56Z

I just need to take the time to finally review it and take care of it

georgesjamous · 2018-08-06T17:09:51Z

@flovilmart
wasn't rushing you, just letting you know about the changes and that i am aware of the conflict that arose 👍

georgesjamous · 2018-08-23T06:17:58Z

src/AuthRoles.js

+      .then(response => response.results);
+  }else{
+    const query = new Parse.Query(Parse.Role);
+    _.forEach(restWhere, (value, key) => {


@flovilmart
I wanted to add an additional constraint here, to select only needed keys, just in case more are available to limit the returned data.

// limit returned data just in case other keys are available query.select(['objectId', 'ACL']); // or simply since acl, createdAt, savedAt are selected by default query.select('objectId');

But for some reason the query stoped returning data (no objects).
Do you have knowlege of why this could happen ? I have diregarded it since its not replated to the PR,
but was just wondering why it wouldnt

flovilmart · 2018-08-23T11:49:09Z

I am worried that we’re taking the problem the wrong way. It seems that you attempt to resolve all the roles with the masterKey and then filter out the ones the user can’t read. Which is well, contrary to the promise of parse.

Why don’t we do:

Get all the roles where users == user with userId, *
Get all the roled where roles in found roles with aclList == concat(roles, userId, *)
Recursion until no more roles are discovered

This way, this can only be consistent with the general approach, and the amount of changes will be minimal.

georgesjamous · 2018-08-23T12:07:54Z

So,
If I am not mistaken, you approach is to build the ACL as we go on.
And with each iteration add(+) more roles to ACL and use it in the next iteration.

I will give it more thoughts and attempt to find a case where it won't work.
Will try to write a quick code and run it through the tests to see the results.
I remember being convinced that the only way to cover all cases is by building a tree and go branch by branch, your proposed solution seems to be more elegant tough.

flovilmart · 2018-08-23T12:18:59Z

Well, if it doesn’t work, that has broader implications.

Each role, directly owning a user gives the user this roles’s powers. Using those powers we need to discover which other powers are inherited down the line. If this doesn’t work cleanly this way, we’ll go with the original suggestion of using a new ‘inactive’ Boolean but i’d rather avoid. Also if possible, can we not introduce a new AuthRoles thing as other PR’s are working on abstraction of the role module and cache

flovilmart · 2018-08-23T12:53:27Z

So I ran something quickly, and it seems that the main issue that I face is when a role is self referencing:

{ objectId: 'nlrw6e2GkB',
    name: 'r1',
    createdAt: '2018-08-23T12:51:44.417Z',
    updatedAt: '2018-08-23T12:51:44.536Z',
    users: { __type: 'Relation', className: '_User' },
    roles: { __type: 'Relation', className: '_Role' },
    ACL: { 'role:r1': [Object] } }

As you can see in this example, the role can be read only by users from this role, which will probably be true for every role if the developer says that only people with this role will be able to read.

Somehow, this is consistent with the ACL strategy, If you can't read somethiung, you can't know you're in it.

georgesjamous · 2018-08-23T13:07:15Z

The main reasoning behind your proposed solution is to skip local ACL test all together and rely soley on a list of accessible roles (aclList) to fetch the next one and so on.
This is impossible, because the thing about our problem is that
we need to follow the ACL to find something constrained by the ACL, that we do not have in the first place.

Also the query will look something like this:

query.containedIn('rprem', concat([roleNames, userid, *]))
query.notContainedIn('name', roleNames) // to prevent circles

And personally i really hate using "notContainedIn" since it may slow things down on large arrays and collections.

georgesjamous · 2018-08-23T13:08:57Z

The solution i have proposed, is no less secure that what Auth.js is doing now.
It uses master key to recursevly load all roles (like we are doing write now).
All that AuthRoles.js does is build a tree and reject some roles depending on their branch.

georgesjamous · 2018-08-23T13:11:26Z

Regarding this

If this doesn’t work cleanly this way, we’ll go with the original suggestion of using a new ‘inactive’ Boolean but i’d rather avoid.

I agree with you, roles should follow ACL. Enable/Disable should be avoided. It does not solve the main problem.

flovilmart · 2018-08-23T13:20:30Z

@georgesjamous well, I have a solution that removes the complete necessity of the AuthRoles and that actually is very similar to the current implementation.

I have more tests to write for it stay tuned I'll post the branch soon.

flovilmart · 2018-08-23T13:54:40Z

@georgesjamous if you want to have a look: https://github.com/parse-community/parse-server/compare/role-basedACLs?expand=1#diff-907d30dccd58d6a80778a5ebf1d17ff5

georgesjamous · 2018-08-23T16:50:39Z

will do

flovilmart · 2018-08-27T15:41:19Z

@georgesjamous did you have a look? Anything that look wrong?

georgesjamous · 2018-08-27T18:14:17Z

I haven't had the chance yet.
I will check it out first thing tomorrow

georgesjamous · 2018-08-29T14:17:57Z

I am gonna go ahead and close this then since you have already implemented most changes on
Role based ACLs #5000
I dont think there is much left to do anyways, just some tests and fixes here and there.

Georges Jamous added 6 commits July 14, 2018 12:38

Preperations

a56daa4

Added AuthRole computor

635acc6

Role tests fixes

8942d80

Test fixes

70f53b2

Fixes and cleanups

1ac2877

...

7df1b47

georgesjamous and others added 3 commits July 14, 2018 14:19

Merge branch 'master' into Roles-Should-Follow-ACLs

6a12946

Minor security bug fix

7dc9b7a

Minor stuff here and there..

100cc50

flovilmart suggested changes Jul 17, 2018

View reviewed changes

Georges Jamous added 3 commits July 21, 2018 10:46

Refactoring AuthRole

4e9fc4c

Bringing back old test and some edits

7665ad9

nits

2bd06de

georgesjamous commented Jul 21, 2018

View reviewed changes

Merge branch 'master' into Roles-Should-Follow-ACLs

c9dc3c8

georgesjamous commented Jul 21, 2018

View reviewed changes

Georges Jamous added 2 commits July 21, 2018 11:53

nits

3bb416b

nits

ca4e672

Georges added 3 commits August 22, 2018 16:45

Sync with master

eace6dc

Support for config

666c479

nits

96b8ce8

georgesjamous commented Aug 23, 2018

View reviewed changes

georgesjamous closed this Aug 29, 2018

Roles should follow acls #4895

Roles should follow acls #4895

Conversation

georgesjamous commented Jul 14, 2018 • edited Loading

codecov bot commented Jul 14, 2018 • edited Loading

Codecov Report

flovilmart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgesjamous Jul 21, 2018 • edited Loading

Choose a reason for hiding this comment

georgesjamous Jul 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgesjamous commented Aug 6, 2018 • edited Loading

flovilmart commented Aug 6, 2018

georgesjamous commented Aug 6, 2018

Choose a reason for hiding this comment

flovilmart commented Aug 23, 2018

georgesjamous commented Aug 23, 2018 • edited Loading

flovilmart commented Aug 23, 2018

flovilmart commented Aug 23, 2018

georgesjamous commented Aug 23, 2018

georgesjamous commented Aug 23, 2018 • edited Loading

georgesjamous commented Aug 23, 2018 • edited Loading

flovilmart commented Aug 23, 2018

flovilmart commented Aug 23, 2018

georgesjamous commented Aug 23, 2018

flovilmart commented Aug 27, 2018

georgesjamous commented Aug 27, 2018

georgesjamous commented Aug 29, 2018

georgesjamous commented Jul 14, 2018 •

edited

Loading

codecov bot commented Jul 14, 2018 •

edited

Loading

georgesjamous Jul 21, 2018 •

edited

Loading

georgesjamous Jul 21, 2018 •

edited

Loading

georgesjamous commented Aug 6, 2018 •

edited

Loading

georgesjamous commented Aug 23, 2018 •

edited

Loading

georgesjamous commented Aug 23, 2018 •

edited

Loading

georgesjamous commented Aug 23, 2018 •

edited

Loading