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

[PROPOSAL] Add an 'Active' column to Roles #4591

Closed
georgesjamous opened this issue Feb 27, 2018 · 9 comments
Closed

[PROPOSAL] Add an 'Active' column to Roles #4591

georgesjamous opened this issue Feb 27, 2018 · 9 comments

Comments

@georgesjamous
Copy link
Contributor

georgesjamous commented Feb 27, 2018

Currently, i cannot think of a way to disable a Role without deleting it, and keeping a copy under a different name. Then copying it back, with all children roles and users under its initial name.

I think it would be a good idea to address a new Column called "Active" (or Enabled) in the _Role class that is initially set to TRUE. Users with Role write ACL will be able Activate or deactivate this Role.

So any Objects protected by this role (or its children roles) will be locked until this role is activated again.

Simple Example:
RoleA {has privilege A }
RoleB under RoleA {has privilege B & A }
RoleC under RoleB {has privilege C & B & A }

now when RoleB is disabled.
Any objects with RoleB ACL are now Locked (depending on the ACL of course)
Users with RoleC will only have privilege C (since RoleB will not be accessible when performing RolesOfRole() _keep reading_)

I am not greatly familiar with the internals of parse-server and how it gathers the user's roles whenever there is a query. But this task should be fairly simple by addressing a new constraint requiring the 'Active' collumn not to be FALSE (not == TRUE, to be able to support previous schemas maybe), whenever the user's roles or the RolesOfARole are fetched.

However, this constraint should not be added on all queries, where it is intended to aquire the Roles, for example in Parse.Cloud, rather it should be deeply available only when aquiring the Roles of a user for a query security. (hope it makes sense !)

I think this would be a powerfull addition where multiple user privileges segmentations can be easilly achieved by just disabling or enabling a Role.

@flovilmart
Copy link
Contributor

If you set the ACL to masterKey only, does it help? from the logged in user's perspective, the role won't yeild (should not yield).

@georgesjamous
Copy link
Contributor Author

georgesjamous commented Feb 27, 2018

Yes it is a possible solution, but i was aiming for a more simple one since the Role's ACL will also need to be kept saved somewhere. This is required to be able to revert the role back to its initial state.

i think a simple 'Active' field would make more sense, don't you ?

@georgesjamous
Copy link
Contributor Author

@flovilmart question, did you mean to set the ACL to masterKey only, on the Role or on all objects protected by the role?

@flovilmart
Copy link
Contributor

Only on the role, which is the same ideally as the ‘active’ state

@georgesjamous
Copy link
Contributor Author

I tried this, but the object was fetched anyway.
Can you confirm that this should not happen?

Created a Test class with only one object.
Set the object's ACL to a Test-Role.
Added the user to the Role and set the role's ACL to masterKey Only.
Then I used the following query to acquire the object with the user's session token.

const query = new Parse.Query('Test');
query.find({sessionToken:'r:79fcffc974a0f3dd56fd8c17ba3feec7'}).then(function( objects) {
  objects.forEach(element => {
    console.log('ID ' + element.id);
  });
});

Result:
Log : ID 5aH0dnRY7H

Expected:
As I understand the query should not yield any results right?

Version:
"parse-server": "2.7.4"

here are some screenshots:
testobject
testrole
testuserinrole

@flovilmart
Copy link
Contributor

flovilmart commented Mar 30, 2018 via email

@georgesjamous
Copy link
Contributor Author

georgesjamous commented Jun 1, 2018

@flovilmart
I will try and give it a shot and try to peek under the hood.
There is only one class where the ACL fix should happen right ?

in Auth.js
L136
L194

@flovilmart
Copy link
Contributor

I’m not sure immediately on the top of my head, but that’s around that

@georgesjamous
Copy link
Contributor Author

Closing this because of #4821 and #5000

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants