-
Notifications
You must be signed in to change notification settings - Fork 153
Add permissions based on department #572
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements department-based permissions in ChiefOnboarding, changing how managers access data. Instead of managers seeing all items or only items assigned to them directly, they now only see items that belong to their assigned departments. Admins retain full access to everything.
- Users can now belong to multiple departments (replacing single department field)
- All templates (to-do items, resources, sequences, etc.) can be assigned to specific departments
- Managers only see and can manage items/users within their departments
Reviewed Changes
Copilot reviewed 77 out of 77 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| back/users/models.py | Converted single department field to many-to-many relationship and updated permission logic |
| back/users/selectors.py | Added new functions to filter users and content by department permissions |
| back/organization/models.py | Added department filtering queryset and managers for template access control |
| back/admin/*/views.py | Updated all admin views to use department-filtered querysets instead of direct model access |
| back/admin/*/forms.py | Added department field filtering to all template forms based on user permissions |
| back/users/migrations/* | Database migration to convert single department to many-to-many relationship |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 77 out of 77 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
cscheng
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, the approach of implementing the new permissions via custom queryset methods and some added logic to views/forms is fine given the amount of custom behavior for the available roles. This will get more unwieldy once there are more actions that require different permissions for roles. Especially when you want to be able to answer questions such as, what can a manager vs. other user? You'll need a more formalized, explicit role based access control (RBAC). That does add more abstraction which is now not yet warranted, but if more permission changes are expected in the near future, you should definitely look into that.
The selectors pattern from the HackSoft style guide, in their current form in this project, I don't see much added value. Almost all of the are one-liners, referencing the custom queryset method .for_user(). The selector functions that do contain some logic, this logic can be moved the custom queryset method instead. I think the selectors would have more value in the following scenarios:
- Your custom querysets have lots of methods (10-20+) and you want to group certain code (for instance helper functions/methods) that's more related together.
- You need the abstraction layer for refactoring or pluggable backends, for instance, if
appointmentsare stored in a different (Django) app/service, so your views call the selector/service functions which return Django model instances.
Personally, they feel a bit unnatural when chaining queryset methods, e.g. get_appointments_for_user(user).select_related('content'). Appointment.objects.for_user(user).select_related('content') looks better to me, but that's just my personal taste.
The departments field in the various forms in its current form allows the manager to remove a department that was set earlier by an admin or another manager, and where that manager is not a member of. Does this need to be prevented in code, or is this not an issue in practice?
Furthermore, almost if not all views that now have manager-specific behavior don't have specific tests for it yet.
| model.template, TemplateManager | ||
| ): | ||
| objects = model.templates.filter(name__search=query) | ||
| objects = model.templates.for_user(user=self.request.user).filter( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing test coverage, add test to assert that only objects that are visible to the user are found.
|
Is this whats needed to be merged first, before the next rleease, After the merging of #387 |
Permissions
Right now, ChiefOnboarding has minimal permission levels:
The manager's permission will change here. They will only be able to access items/users that belong to the group(s) they are part of. By default, all existing items belong to all groups, admins can change the groups. For every template (to do item, resource, sequence etc), you can assign one or more departments to each manager. Only the relevant ones will show up for the managers.