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 rolespermissions module, refractor the group and delete submission to use it #4151

Merged
merged 4 commits into from
Nov 7, 2024

Conversation

theskumar
Copy link
Member

@theskumar theskumar commented Sep 27, 2024

Updates group management:

  • Centralized Group Definition: Instead of creating groups within migration files, which can be cumbersome to maintain, group definitions are now managed through the AbstractRoles model. These roles are synchronized using the python manage.py sync_roles command. This ensures no existing groups or their associated permissions are deleted.

  • Module Renaming: The hypha.apply.users.groups module has been renamed to hypha.apply.users.roles to reflect the shift from group-based to role-based permissions. This aligns with upcoming changes utilizing the rolepermissions module.

  • Streamlined Group Descriptions: The GroupDesc model is removed. Instead, help text can be directly defined within the role itself. This simplifies management and allows for translation of group descriptions.

This is the first of a series of pull requests aimed at refactoring the permissions system.

As a sample implementation, converts the delete submission to use this role-permissions. See: 8239c21

Test Steps

  • make sure the migrations run fine.
  • groups are create correctly via python manage.py sync_roles, it should also keep the existing groups.
  • groups description is rendered in the admin.
  • ensure delete submission/application is working as expected

@theskumar theskumar marked this pull request as draft September 27, 2024 09:09
@theskumar theskumar marked this pull request as ready for review September 27, 2024 09:36
Copy link
Contributor

@frjo frjo left a comment

Choose a reason for hiding this comment

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

Notice no changes at all in how Hypha works so that is good. Running the "sync_roles" does not add/delete/change any of the existing roles. Deleting an existing role and running "sync_roles" restores it.

@theskumar
Copy link
Member Author

On another thought, let's hold on to merging this, I'm reading and trying out few things with some of the use-cases we have the project and make a final decision on going with django-rolepermissions package.

esp. i'm evaluating django-rules for object level permissions, while the django-rolepermissions is quite good for group based permissions it doesn't seem to provide much when it comes to object level permissions.

Looking at the use case, hypha seems to have majority object level permissions.

@theskumar theskumar marked this pull request as draft September 30, 2024 04:25
@theskumar theskumar self-assigned this Sep 30, 2024
Update how initial groups are created in the database, instead of
putting them in the migration files, which is quite hard to maintain the
group definitation is now maintained via the AbstractRoles and they
rsync'ed using `python manage.py sync_roles`. It will never delete any
existing group or permissions attached to them.

This PR also updates the `hypha.apply.users.groups` module to
`hypha.apply.users.roles` to better prepare of upcoming changes due to
rolepermissions module
@theskumar
Copy link
Member Author

theskumar commented Nov 3, 2024

Implemented a sample permission and it's checks for the delete submissions, it looks straightforward. I was not confident that the can template tag and HasPermissionMixin will work with seamlessly with @register_object_checker, but it does.

8239c21

@theskumar theskumar changed the title Add rolespermissions module, refractor the group to use it Add rolespermissions module, refractor the group and delete submission to use it Nov 3, 2024
@theskumar theskumar marked this pull request as ready for review November 3, 2024 17:00
@theskumar
Copy link
Member Author

@frjo @sandeepsajan0 @wes-otf If the delete submission implementation looks good, let's try get this merged so rest of the followup PRs will be mostly converting existing permission check to use the rolepermissions package, instead of custom functions we using.

Copy link
Contributor

@frjo frjo left a comment

Choose a reason for hiding this comment

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

A failing test to fix. I also think we need to add the sync_roles to the Procfile so it runs on Heroku deployments.

@theskumar
Copy link
Member Author

Update the test to match new test settings where SECURE_SSL_REDIRECT is set to false by default.

I also think we need to add the sync_roles to the Procfile so it runs on Heroku deployments.

This should covered already, let me know if you face issue when deploying this branch.

@frjo frjo added Type: Enhancement This is an improvement of an existing thing (not a new thing, which would be a feature). Type: Minor Minor change, used in release drafter labels Nov 7, 2024
@frjo frjo merged commit 8f16bd2 into main Nov 7, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement This is an improvement of an existing thing (not a new thing, which would be a feature). Type: Minor Minor change, used in release drafter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants