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

feat: Role Delegation permissions #35

Merged
merged 5 commits into from Sep 20, 2021

Conversation

callinmullaney
Copy link
Contributor

@callinmullaney callinmullaney commented Jun 13, 2021

Purpose:

  • Break up the Administer roles and permissions permission for specific role-to-role assigning. Allowing non-admins to create new users without and assign them to sub-roles without exposing the administrator role.

Notes:

Testing:

Code changes:

  • Install Sous and verify the config changes in this PR import without errors.
  • Go to /admin/modules and verify the "Role Delegation" module is enabled by default
  • Go to /admin/people/permissions and verify the superuser role is present and assigned all the permissions related to "Role Delegation"

module features:

  • Create a new user role /admin/people/roles/add
  • Create another user role /admin/people/roles/add that is intended to have restricted access
  • Go to /admin/people/permissions and grant "Role Delegation" permission for the first role to assign the second role. As well as:
    • View the administration theme
    • Administer users
    • [ ]
  • Create a user account for the two roles above to test the following permissions
    • Log in with the user you assigned with the delegation permissions
    • Go to /admin/people/create and verify the only role available to assign is the second role you created from above

@callinmullaney callinmullaney added the 👍 Ready for Review Work is ready for review. label Jun 13, 2021
@callinmullaney callinmullaney self-assigned this Jun 13, 2021
@callinmullaney callinmullaney changed the base branch from 2.x to admin_install_defaults June 13, 2021 22:01
@ccjjmartin ccjjmartin added 🎉 Passes Code Review Code is approved by the reviewer. Passes Functional and removed 👍 Ready for Review Work is ready for review. labels Sep 15, 2021
Copy link
Collaborator

@ccjjmartin ccjjmartin left a comment

Choose a reason for hiding this comment

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

@callinmullaney I made it through all of your steps in testing. From a code and functional perspective it does everything you said it does. My only hesitation here is that I am not sure that ALL of our sites we build will need this ... using TLSC as an example, what would their use case be for using this module?

Also, for actual use the permission to see the admin toolbar would be helpful for the role who gets to delegate.

@callinmullaney
Copy link
Contributor Author

callinmullaney commented Sep 15, 2021

@ccjjmartin The main use case for most projects is to prevent users that have the administer role and permissions from assigning themselves the superuser/administrator role or creating new administrator accounts.

In the example of TLSC. Content Managers need to be able to have the ability to add new Content Editors or Contractors to the site. However, because the administer role and permissions is a catch-all permission any Content Manager could assign themselves as Administer role or create new Administer user accounts themselves. Essentially bypassing the Organization Admin role. With this module we could prevent this scenario altogether by having specific assign role permissions instead of giving everyone the keys to the kingdom.

Even on simple/smaller projects we generally create a new role for the client that they use for day-to-day updates that isn't the administrator role. This scenario also applies as we wouldn't want them assigning there day-to-day role as the superuser/administrator.

In the context of the PR that blocks user 1 this becomes especially important as we don't want anyone to unblock that user.

@callinmullaney
Copy link
Contributor Author

Updated PR purpose to reflect this.

@ccjjmartin
Copy link
Collaborator

@callinmullaney Ok, looks good to me :shipit:

add comma for valid json
@callinmullaney callinmullaney merged commit 8268334 into admin_install_defaults Sep 20, 2021
@fkbender
Copy link

🎉 This PR is included in version 4.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

4 participants