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

Add ability to deactivate a role (Enable/Disable) #4821

Closed
wants to merge 5 commits into from
Closed

Add ability to deactivate a role (Enable/Disable) #4821

wants to merge 5 commits into from

Conversation

georgesjamous
Copy link
Contributor

This PR addresses the open proposal #4591 to add a functionality to Enable/Disable a role.
Disabling a role will prevent the role from granting permissions to its users or any of its children roles.
I think this functionality can come in handy when it is needed to temporarily disable a role tree.

Another other solution could be as discussed in #4591 is to set the role ACL to masterKey,
preventing access to the role, but:

  1. masterKey will also prevent any other role who has Write Access to be prevented access
  2. ACL needs to be saved and rolled back to restore access
  3. Currently, ACL is not applied to roles when gathering authentication, roles are fetched regardless of their acl in authentication. (as discussed in [PROPOSAL] Add an 'Active' column to Roles #4591)

This is my first PR, hope I did well :)

@georgesjamous
Copy link
Contributor Author

georgesjamous commented Jun 8, 2018

Seems travis failure originated from schema not related to my pull.

@dplewis
Copy link
Member

dplewis commented Jun 8, 2018

@georgesjamous you added a new field to the schema so the test need to be updated

@flovilmart
Copy link
Contributor

@georgesjamous thanks for the PR, but as I mentioned before, the roles availability should be managed by their ACL’s.

@georgesjamous
Copy link
Contributor Author

@dplewis oh alright, my mistake!

@flovilmart yes, I am still trying to figure out a way for the role to follow the ACL. But, I thought this could be a useful addition.
Alright, we can postpone this PR for now if you prefer, till a solution for the ACL arises.

@flovilmart
Copy link
Contributor

This is in the Auth module, the query for the role should also include the visibility for the current user / role being inferred, as with ACL’s.

@georgesjamous
Copy link
Contributor Author

georgesjamous commented Jun 12, 2018

@flovilmart

the query for the role should also include the visibility for the current user / role being inferred,

I looked at the Auth.js code, I don't think we can apply the ACL directly as it is done in other places since the actual ACL _rperm & _wperm are being fetched from Auth.js.
So how could we add constraints on that if we don't have them yet?

One solution I thought of is to fetch all the roles normally (like it is done right now),
then check the _rperm manually to reject the ones the user doesn't have access to.
This will keep the same number of requests and should be efficient.

The method would be to compile a list (tree-like) of direct roles (where the user is part of)
and for each one, the parent role (where he would inherit access from).
Then we could check each role access with _rperm and if it passes continue down the tree, if not we would reject the whole branch.

A simple example for the simplest cases (i have also checked more complex ones):
Role1 has User1 and Role2
Role2 has User2 and Role3
Role3 has User3 and Role4
Tree compiled per user would be:
User1: Role1
User2: Role2 -> Role1
User3: Role3 -> Role2 -> Role1
Then if Role2's users lost access to it, User3 would only have access to Role3, and User2 would not have access to any role. User1 will not be affected.

This is the best solution I could think of that doesn't require additional requests and is not costly.
I will write the code and some tests on the weekend to check out the results.

If you have thought of a better way or think for some reason that this not a good solution, let me know.

@flovilmart
Copy link
Contributor

Let’s try it out, that seems reasonable as the full role tree is pulled. Let’s see what it looks like with the tests etc... what do you think?

@georgesjamous
Copy link
Contributor Author

Its worth a shot, will let you guys know the results

@flovilmart
Copy link
Contributor

Great thanks!

@georgesjamous
Copy link
Contributor Author

Sorry for the delay, haven't been able to find the time for it yet.
But its definitely on my todo list.

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

Successfully merging this pull request may close these issues.

3 participants