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

[Role/Permission] Configuration tool (2 of 4) #3538

Closed
wants to merge 2 commits into from

Conversation

ridz1208
Copy link
Collaborator

@ridz1208 ridz1208 commented Mar 7, 2018

This adds the Role infrastructure to LORIS. This is one of 4 PRs which will ultimately result in assigning roles to users. each role has affiliated permissions and automatically checks the proper permission when selected. This change will be mainly visible in the user_accounts module.

PART 2: Add tool used to configure the role/permission relationships as well as populate the data in the right table given an existing populated database.

in order to test, the following SQL inserts can be used:

INSERT INTO role (Name, Label) VALUES ('coordinator', 'Coordinator');
INSERT INTO role (Name, Label) VALUES ('data', 'Data-Entry');
INSERT INTO role (Name, Label) VALUES ('upload', 'Data-upload');


INSERT INTO role_permission_rel VALUES ((SELECT RoleID FROM role WHERE Name='coordinator'), (SELECT permID FROM permissions WHERE code='send_to_dcc'));
INSERT INTO role_permission_rel SELECT RoleID,permID FROM role JOIN permissions WHERE Name='data' AND code IN ('data_dict_view','data_entry','data_dict_edit','candidate_parameter_view');
INSERT INTO role_permission_rel SELECT RoleID,permID FROM role JOIN permissions WHERE Name='upload' AND code IN ('video_upload','media_write','media_read');

Original PR by @taracampbell : #2642
Updated/Standardized/DAO full version : ridz1208#9

@ridz1208 ridz1208 added Category: Feature PR or issue that aims to introduce a new feature [branch] minor labels Mar 7, 2018
@ridz1208 ridz1208 changed the title 2018 03 10 tar perm roles p2 [Role/Permission] Configuration tool (2 of 4) Mar 7, 2018
@ridz1208
Copy link
Collaborator Author

ridz1208 commented Mar 7, 2018

@ridz1208 ridz1208 added the State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed label Mar 7, 2018
echo "The permissions in the database are:\n\n";
prettyPrint($permissions);
exit(3);
case "rebuildRoles":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exit code other than 0 are normally for error or abnormal ending. When there is no error or problem, exit code is normally 0.

Could also use a single exit() call with a variable.

)
);

echo "\n$permission was added to the $role category.\n\n";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"category" should be "role" as other messages

* if false, removes the role from the user
*
* @return void
*/
Copy link
Contributor

@PapillonMcGill PapillonMcGill Mar 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why a "addPermission" function would remove a role from a user.

This portion should be in a "removeRole" function otherwise user might loose some permissions

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if u add a permission to a rome, you have 2 choices,

  • give that permission to all the users with that role
  • remove the role from all the users who dont have that permission

that decision is left to the administrators running the patch

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, now I understand, maybe I am slow, or maybe the text was not clear.

);

echo "\n$permission was removed from the $role category.\n";

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again "category" instead of "role"

return false;
}
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

multiple return

@driusan
Copy link
Collaborator

driusan commented Mar 12, 2018

Closing until #3537 goes in, until then it's going to need continual rebasing and just make it look like our PRs aren't being worked on.

@driusan driusan closed this Mar 12, 2018
@ridz1208 ridz1208 mentioned this pull request Mar 31, 2021
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Feature PR or issue that aims to introduce a new feature State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants