Implemente a gestão dos grupos e perfis de usuários#864
Implemente a gestão dos grupos e perfis de usuários#864
Conversation
Co-authored-by: robertatakenaka <505143+robertatakenaka@users.noreply.github.com>
|
@copilot considere que o collection_team_admin pode CRUD journal_team_admin e CRUD company_team_admin |
Co-authored-by: robertatakenaka <505143+robertatakenaka@users.noreply.github.com>
Feito em 740b884. |
|
@copilot também consider que usando wagtail customizado há o usuários do wagtail então trate os grupos de usuários do Upload de forma análoga ao usuários do wagtail. Atualize o USER com os valores dos grupos quando um membro de time é criado |
Co-authored-by: robertatakenaka <505143+robertatakenaka@users.noreply.github.com>
Feito em b2b618c. Adicionei
O mesmo se aplica aos grupos |
There was a problem hiding this comment.
Pull request overview
This PR implements role-based access control for the team module by defining canonical group names, filtering ViewSet querysets based on authenticated user roles, and automatically synchronizing Django auth.Group memberships via post_save/post_delete signals on team member models.
Changes:
- Adds six group constants (COLLECTION_TEAM_ADMIN, COLLECTION_TEAM_MEMBER, JOURNAL_TEAM_ADMIN, JOURNAL_TEAM_MEMBER, COMPANY_TEAM_ADMIN, COMPANY_MEMBER) to team/models.py
- Creates management command to initialize groups and assign model permissions
- Implements automatic group synchronization via signals that update user groups when team memberships change
- Adds get_queryset methods to five ViewSets to filter records based on user roles
- Adds comprehensive test coverage for queryset filtering logic and group synchronization
Reviewed changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| team/models.py | Defines group name constants and GROUP_NAMES list for role-based access control |
| team/signals.py | Implements signal handlers to automatically sync user auth.Groups when team memberships change |
| team/wagtail_hooks.py | Adds get_queryset filtering to CollectionTeamMemberViewSet, CompanyViewSet, JournalTeamMemberViewSet, CompanyTeamMemberViewSet, and JournalCompanyContractViewSet |
| team/apps.py | Registers signal handlers in the ready() method |
| team/management/commands/create_user_groups.py | Management command to create groups and assign Django model permissions |
| team/tests.py | Adds tests for group constants, queryset filtering logic, and group synchronization behavior |
team/wagtail_hooks.py
Outdated
| managed_journal_ids = JournalTeamMember.objects.filter( | ||
| user=user, role=TeamRole.MANAGER, is_active_member=True | ||
| ).values_list("journal", flat=True) | ||
| return qs.filter(journal__in=managed_journal_ids) |
There was a problem hiding this comment.
When a user is not a collection manager and has no managed journals (managed_journal_ids is an empty queryset), the filter returns all contracts instead of an empty queryset. This allows non-manager journal members and company members to see all contracts. Add a check similar to other ViewSets: if managed_journal_ids is truthy, filter by it; otherwise return an empty queryset (qs.none() or an appropriate filter that returns nothing).
| return qs.filter(journal__in=managed_journal_ids) | |
| if managed_journal_ids: | |
| return qs.filter(journal__in=managed_journal_ids) | |
| return qs.none() |
There was a problem hiding this comment.
Fixed in 6d9d38d. The JournalCompanyContract.get_queryset_for_user classmethod now returns qs.none() when the user has no managed journals.
| # Refresh from DB | ||
| self.user.refresh_from_db() | ||
| self.assertNotIn(COLLECTION_TEAM_ADMIN, self._group_names()) | ||
| self.assertIn(COLLECTION_TEAM_MEMBER, self._group_names()) |
There was a problem hiding this comment.
Consider adding a test case where a user is a MANAGER of one collection and a MEMBER of another collection simultaneously. This would verify that the signal correctly assigns both COLLECTION_TEAM_ADMIN and COLLECTION_TEAM_MEMBER groups to the same user. The current signal logic supports this (it checks for any active MANAGER or MEMBER roles), but there's no test verifying this edge case works correctly.
| self.assertIn(COLLECTION_TEAM_MEMBER, self._group_names()) | |
| self.assertIn(COLLECTION_TEAM_MEMBER, self._group_names()) | |
| def test_collection_manager_and_member_on_different_collections_get_both_groups(self): | |
| """ | |
| User who is MANAGER of one collection and MEMBER of another | |
| should have both COLLECTION_TEAM_ADMIN and COLLECTION_TEAM_MEMBER groups. | |
| """ | |
| # MANAGER on the first collection | |
| CollectionTeamMember.objects.create( | |
| user=self.user, | |
| collection=self.collection, | |
| role=TeamRole.MANAGER, | |
| is_active_member=True, | |
| creator=self.user, | |
| ) | |
| # MEMBER on a different collection | |
| other_collection = Collection.objects.create() | |
| CollectionTeamMember.objects.create( | |
| user=self.user, | |
| collection=other_collection, | |
| role=TeamRole.MEMBER, | |
| is_active_member=True, | |
| creator=self.user, | |
| ) | |
| # Refresh from DB to ensure groups are up to date | |
| self.user.refresh_from_db() | |
| group_names = self._group_names() | |
| self.assertIn(COLLECTION_TEAM_ADMIN, group_names) | |
| self.assertIn(COLLECTION_TEAM_MEMBER, group_names) |
There was a problem hiding this comment.
Added in 6d9d38d — test_user_manager_of_one_collection_and_member_of_another_gets_both_groups in UserGroupSyncTest covers this edge case.
team/signals.py
Outdated
| @@ -0,0 +1,75 @@ | |||
| from django.contrib.auth.models import Group | |||
| from django.db.models.signals import post_delete, post_save | |||
| from django.dispatch import receiver | |||
There was a problem hiding this comment.
The receiver decorator is imported but never used. Since the code uses explicit signal.connect() calls instead of @receiver decorators, this import can be removed.
| from django.dispatch import receiver |
| class GetQuerysetFilteringTest(TestCase): | ||
| """Test the queryset filtering logic used in wagtail_hooks get_queryset methods.""" | ||
|
|
||
| def setUp(self): | ||
| self.superuser = User.objects.create_superuser( | ||
| username="superuser", email="super@example.com", password="pass" | ||
| ) | ||
| self.collection_manager = User.objects.create_user( | ||
| username="col_manager", email="col_manager@example.com", password="pass" | ||
| ) | ||
| self.collection_member = User.objects.create_user( | ||
| username="col_member", email="col_member@example.com", password="pass" | ||
| ) | ||
| self.journal_manager = User.objects.create_user( | ||
| username="jour_manager", email="jour_manager@example.com", password="pass" | ||
| ) | ||
| self.journal_member = User.objects.create_user( | ||
| username="jour_member", email="jour_member@example.com", password="pass" | ||
| ) | ||
| self.company_manager = User.objects.create_user( | ||
| username="comp_manager", email="comp_manager@example.com", password="pass" | ||
| ) | ||
| self.company_member_user = User.objects.create_user( | ||
| username="comp_member", email="comp_member@example.com", password="pass" | ||
| ) | ||
|
|
||
| self.collection = Collection.objects.create( | ||
| acron="TST", name="Test Collection", creator=self.superuser | ||
| ) | ||
| self.other_collection = Collection.objects.create( | ||
| acron="OTH", name="Other Collection", creator=self.superuser | ||
| ) | ||
| self.journal = Journal.objects.create(title="Test Journal", creator=self.superuser) | ||
| self.other_journal = Journal.objects.create(title="Other Journal", creator=self.superuser) | ||
| self.company = Company.objects.create(name="Test Company", creator=self.superuser) | ||
| self.other_company = Company.objects.create(name="Other Company", creator=self.superuser) | ||
|
|
||
| # Set up collection team members | ||
| CollectionTeamMember.objects.create( | ||
| user=self.collection_manager, | ||
| collection=self.collection, | ||
| role=TeamRole.MANAGER, | ||
| is_active_member=True, | ||
| creator=self.superuser, | ||
| ) | ||
| CollectionTeamMember.objects.create( | ||
| user=self.collection_member, | ||
| collection=self.collection, | ||
| role=TeamRole.MEMBER, | ||
| is_active_member=True, | ||
| creator=self.superuser, | ||
| ) | ||
|
|
||
| # Set up journal team members | ||
| JournalTeamMember.objects.create( | ||
| user=self.journal_manager, | ||
| journal=self.journal, | ||
| role=TeamRole.MANAGER, | ||
| is_active_member=True, | ||
| creator=self.superuser, | ||
| ) | ||
| JournalTeamMember.objects.create( | ||
| user=self.journal_member, | ||
| journal=self.journal, | ||
| role=TeamRole.MEMBER, | ||
| is_active_member=True, | ||
| creator=self.superuser, | ||
| ) | ||
| # journal_manager also member of other_journal (as member) | ||
| JournalTeamMember.objects.create( | ||
| user=self.journal_manager, | ||
| journal=self.other_journal, | ||
| role=TeamRole.MEMBER, | ||
| is_active_member=True, | ||
| creator=self.superuser, | ||
| ) | ||
|
|
||
| # Set up company team members | ||
| CompanyTeamMember.objects.create( | ||
| user=self.company_manager, | ||
| company=self.company, | ||
| role=TeamRole.MANAGER, | ||
| is_active_member=True, | ||
| creator=self.superuser, | ||
| ) | ||
| CompanyTeamMember.objects.create( | ||
| user=self.company_member_user, | ||
| company=self.company, | ||
| role=TeamRole.MEMBER, | ||
| is_active_member=True, | ||
| creator=self.superuser, | ||
| ) | ||
|
|
||
| # Contracts | ||
| self.contract = JournalCompanyContract.objects.create( | ||
| journal=self.journal, | ||
| company=self.company, | ||
| is_active=True, | ||
| creator=self.superuser, | ||
| ) | ||
| self.other_contract = JournalCompanyContract.objects.create( | ||
| journal=self.other_journal, | ||
| company=self.other_company, | ||
| is_active=True, | ||
| creator=self.superuser, | ||
| ) | ||
|
|
||
| # --- CollectionTeamMember queryset filtering --- | ||
|
|
||
| def test_collection_team_qs_superuser_sees_all(self): | ||
| """Superuser should see all CollectionTeamMember records.""" | ||
| qs = CollectionTeamMember.objects.all() | ||
| self.assertEqual(qs.count(), 2) | ||
|
|
||
| def test_collection_team_qs_manager_sees_own_collection_members(self): | ||
| """COLLECTION_TEAM_ADMIN sees members of their collection(s).""" | ||
| managed_ids = CollectionTeamMember.objects.filter( | ||
| user=self.collection_manager, role=TeamRole.MANAGER, is_active_member=True | ||
| ).values_list("collection", flat=True) | ||
| filtered = CollectionTeamMember.objects.filter(collection__in=managed_ids) | ||
| # Both manager and member of the collection should be visible | ||
| self.assertEqual(filtered.count(), 2) | ||
|
|
||
| def test_collection_team_qs_member_sees_only_self(self): | ||
| """COLLECTION_TEAM_MEMBER sees only their own record.""" | ||
| managed_ids = CollectionTeamMember.objects.filter( | ||
| user=self.collection_member, role=TeamRole.MANAGER, is_active_member=True | ||
| ).values_list("collection", flat=True) | ||
| self.assertFalse(managed_ids.exists()) | ||
| # Falls back to filter(user=self.collection_member) | ||
| filtered = CollectionTeamMember.objects.filter(user=self.collection_member) | ||
| self.assertEqual(filtered.count(), 1) | ||
|
|
||
| # --- Company queryset filtering --- | ||
|
|
||
| def test_company_qs_collection_manager_sees_all(self): | ||
| """COLLECTION_TEAM_ADMIN can see all companies.""" | ||
| is_collection_manager = CollectionTeamMember.objects.filter( | ||
| user=self.collection_manager, role=TeamRole.MANAGER, is_active_member=True | ||
| ).exists() | ||
| self.assertTrue(is_collection_manager) | ||
| # Collection manager should see all companies | ||
| qs = Company.objects.all() | ||
| self.assertEqual(qs.count(), 2) | ||
|
|
||
| def test_company_qs_company_member_sees_own(self): | ||
| """Company member sees only their own companies.""" | ||
| is_collection_manager = CollectionTeamMember.objects.filter( | ||
| user=self.company_member_user, role=TeamRole.MANAGER, is_active_member=True | ||
| ).exists() | ||
| self.assertFalse(is_collection_manager) | ||
| company_ids = CompanyTeamMember.objects.filter( | ||
| user=self.company_member_user, is_active_member=True | ||
| ).values_list("company", flat=True) | ||
| filtered = Company.objects.filter(id__in=company_ids) | ||
| self.assertEqual(filtered.count(), 1) | ||
| self.assertEqual(filtered.first(), self.company) | ||
|
|
||
| # --- JournalTeamMember queryset filtering --- | ||
|
|
||
| def test_journal_team_qs_collection_manager_sees_all(self): | ||
| """COLLECTION_TEAM_ADMIN sees all journal team members.""" | ||
| is_collection_manager = CollectionTeamMember.objects.filter( | ||
| user=self.collection_manager, role=TeamRole.MANAGER, is_active_member=True | ||
| ).exists() | ||
| self.assertTrue(is_collection_manager) | ||
| # Should see all journal team members | ||
| self.assertEqual(JournalTeamMember.objects.count(), 3) | ||
|
|
||
| def test_journal_team_qs_journal_manager_sees_own_journal_members(self): | ||
| """JOURNAL_TEAM_ADMIN sees members of their managed journals.""" | ||
| is_collection_manager = CollectionTeamMember.objects.filter( | ||
| user=self.journal_manager, role=TeamRole.MANAGER, is_active_member=True | ||
| ).exists() | ||
| self.assertFalse(is_collection_manager) | ||
| managed_journal_ids = JournalTeamMember.objects.filter( | ||
| user=self.journal_manager, role=TeamRole.MANAGER, is_active_member=True | ||
| ).values_list("journal", flat=True) | ||
| self.assertTrue(managed_journal_ids.exists()) | ||
| filtered = JournalTeamMember.objects.filter(journal__in=managed_journal_ids) | ||
| # journal_manager and journal_member are in the managed journal | ||
| self.assertEqual(filtered.count(), 2) | ||
|
|
||
| def test_journal_team_qs_journal_member_sees_only_self(self): | ||
| """JOURNAL_TEAM_MEMBER sees only their own record.""" | ||
| managed_journal_ids = JournalTeamMember.objects.filter( | ||
| user=self.journal_member, role=TeamRole.MANAGER, is_active_member=True | ||
| ).values_list("journal", flat=True) | ||
| self.assertFalse(managed_journal_ids.exists()) | ||
| filtered = JournalTeamMember.objects.filter(user=self.journal_member) | ||
| self.assertEqual(filtered.count(), 1) | ||
|
|
||
| # --- CompanyTeamMember queryset filtering --- | ||
|
|
||
| def test_company_team_qs_collection_manager_sees_all(self): | ||
| """COLLECTION_TEAM_ADMIN sees all company team members.""" | ||
| is_collection_manager = CollectionTeamMember.objects.filter( | ||
| user=self.collection_manager, role=TeamRole.MANAGER, is_active_member=True | ||
| ).exists() | ||
| self.assertTrue(is_collection_manager) | ||
| self.assertEqual(CompanyTeamMember.objects.count(), 2) | ||
|
|
||
| def test_company_team_qs_manager_sees_own_company_members(self): | ||
| """COMPANY_TEAM_ADMIN sees members of their managed companies.""" | ||
| managed_company_ids = CompanyTeamMember.objects.filter( | ||
| user=self.company_manager, role=TeamRole.MANAGER, is_active_member=True | ||
| ).values_list("company", flat=True) | ||
| self.assertTrue(managed_company_ids.exists()) | ||
| filtered = CompanyTeamMember.objects.filter(company__in=managed_company_ids) | ||
| self.assertEqual(filtered.count(), 2) | ||
|
|
||
| def test_company_team_qs_member_sees_only_self(self): | ||
| """COMPANY_MEMBER sees only their own record.""" | ||
| managed_company_ids = CompanyTeamMember.objects.filter( | ||
| user=self.company_member_user, role=TeamRole.MANAGER, is_active_member=True | ||
| ).values_list("company", flat=True) | ||
| self.assertFalse(managed_company_ids.exists()) | ||
| filtered = CompanyTeamMember.objects.filter(user=self.company_member_user) | ||
| self.assertEqual(filtered.count(), 1) | ||
|
|
||
| # --- JournalCompanyContract queryset filtering --- | ||
|
|
||
| def test_contract_qs_collection_manager_sees_all(self): | ||
| """COLLECTION_TEAM_ADMIN sees all contracts.""" | ||
| is_collection_manager = CollectionTeamMember.objects.filter( | ||
| user=self.collection_manager, role=TeamRole.MANAGER, is_active_member=True | ||
| ).exists() | ||
| self.assertTrue(is_collection_manager) | ||
| self.assertEqual(JournalCompanyContract.objects.count(), 2) | ||
|
|
||
| def test_contract_qs_journal_manager_sees_own_journal_contracts(self): | ||
| """JOURNAL_TEAM_ADMIN sees contracts for their managed journals.""" | ||
| is_collection_manager = CollectionTeamMember.objects.filter( | ||
| user=self.journal_manager, role=TeamRole.MANAGER, is_active_member=True | ||
| ).exists() | ||
| self.assertFalse(is_collection_manager) | ||
| managed_journal_ids = JournalTeamMember.objects.filter( | ||
| user=self.journal_manager, role=TeamRole.MANAGER, is_active_member=True | ||
| ).values_list("journal", flat=True) | ||
| filtered = JournalCompanyContract.objects.filter(journal__in=managed_journal_ids) | ||
| self.assertEqual(filtered.count(), 1) | ||
| self.assertEqual(filtered.first(), self.contract) | ||
|
|
||
| def test_contract_qs_non_manager_sees_none(self): | ||
| """Users with no manager role see no contracts.""" | ||
| is_collection_manager = CollectionTeamMember.objects.filter( | ||
| user=self.journal_member, role=TeamRole.MANAGER, is_active_member=True | ||
| ).exists() | ||
| self.assertFalse(is_collection_manager) | ||
| managed_journal_ids = JournalTeamMember.objects.filter( | ||
| user=self.journal_member, role=TeamRole.MANAGER, is_active_member=True | ||
| ).values_list("journal", flat=True) | ||
| filtered = JournalCompanyContract.objects.filter(journal__in=managed_journal_ids) | ||
| self.assertEqual(filtered.count(), 0) |
There was a problem hiding this comment.
The tests in GetQuerysetFilteringTest replicate the filtering logic but don't actually test the ViewSet's get_queryset methods. Consider adding integration tests that instantiate the ViewSets and call their get_queryset methods with a mock request object to ensure the actual ViewSet behavior matches expectations. The current tests only verify the ORM queries work correctly, but don't test that the ViewSets use them properly.
team/wagtail_hooks.py
Outdated
| @@ -66,6 +75,23 @@ class CompanyViewSet(SnippetViewSet): | |||
| "url", | |||
| ) | |||
|
|
|||
| def get_queryset(self, request): | |||
| qs = super().get_queryset(request) | |||
| user = request.user | |||
| if user.is_superuser: | |||
| return qs | |||
| # COLLECTION_TEAM_ADMIN (collection managers) can manage all companies | |||
| is_collection_manager = CollectionTeamMember.objects.filter( | |||
| user=user, role=TeamRole.MANAGER, is_active_member=True | |||
| ).exists() | |||
| if is_collection_manager: | |||
| return qs | |||
| # Company members see only their own companies | |||
| company_ids = CompanyTeamMember.objects.filter( | |||
| user=user, is_active_member=True | |||
| ).values_list("company", flat=True) | |||
| return qs.filter(id__in=company_ids) | |||
|
|
|||
|
|
|||
| class JournalTeamMemberViewSet(SnippetViewSet): | |||
| model = JournalTeamMember | |||
| @@ -89,6 +115,26 @@ class JournalTeamMemberViewSet(SnippetViewSet): | |||
| "journal__title", | |||
| ) | |||
|
|
|||
| def get_queryset(self, request): | |||
| qs = super().get_queryset(request) | |||
| user = request.user | |||
| if user.is_superuser: | |||
| return qs | |||
| # COLLECTION_TEAM_ADMIN sees all journal team members | |||
| is_collection_manager = CollectionTeamMember.objects.filter( | |||
| user=user, role=TeamRole.MANAGER, is_active_member=True | |||
| ).exists() | |||
| if is_collection_manager: | |||
| return qs | |||
| # JOURNAL_TEAM_ADMIN sees members of their managed journals | |||
| managed_journal_ids = JournalTeamMember.objects.filter( | |||
| user=user, role=TeamRole.MANAGER, is_active_member=True | |||
| ).values_list("journal", flat=True) | |||
| if managed_journal_ids: | |||
| return qs.filter(journal__in=managed_journal_ids) | |||
| # Regular members see only their own record | |||
| return qs.filter(user=user) | |||
|
|
|||
|
|
|||
| class CompanyTeamMemberViewSet(SnippetViewSet): | |||
| model = CompanyTeamMember | |||
| @@ -112,6 +158,26 @@ class CompanyTeamMemberViewSet(SnippetViewSet): | |||
| "company__name", | |||
| ) | |||
|
|
|||
| def get_queryset(self, request): | |||
| qs = super().get_queryset(request) | |||
| user = request.user | |||
| if user.is_superuser: | |||
| return qs | |||
| # COLLECTION_TEAM_ADMIN sees all company team members | |||
| is_collection_manager = CollectionTeamMember.objects.filter( | |||
| user=user, role=TeamRole.MANAGER, is_active_member=True | |||
| ).exists() | |||
| if is_collection_manager: | |||
| return qs | |||
| # COMPANY_TEAM_ADMIN sees members of their managed companies | |||
| managed_company_ids = CompanyTeamMember.objects.filter( | |||
| user=user, role=TeamRole.MANAGER, is_active_member=True | |||
| ).values_list("company", flat=True) | |||
| if managed_company_ids: | |||
| return qs.filter(company__in=managed_company_ids) | |||
| # Regular members see only their own record | |||
| return qs.filter(user=user) | |||
|
|
|||
|
|
|||
| class JournalCompanyContractViewSet(SnippetViewSet): | |||
| model = JournalCompanyContract | |||
| @@ -133,6 +199,23 @@ class JournalCompanyContractViewSet(SnippetViewSet): | |||
| "company__name", | |||
| ) | |||
|
|
|||
| def get_queryset(self, request): | |||
| qs = super().get_queryset(request) | |||
| user = request.user | |||
| if user.is_superuser: | |||
| return qs | |||
| # COLLECTION_TEAM_ADMIN sees all contracts | |||
| is_collection_manager = CollectionTeamMember.objects.filter( | |||
| user=user, role=TeamRole.MANAGER, is_active_member=True | |||
| ).exists() | |||
| if is_collection_manager: | |||
| return qs | |||
| # JOURNAL_TEAM_ADMIN sees contracts for their managed journals | |||
| managed_journal_ids = JournalTeamMember.objects.filter( | |||
| user=user, role=TeamRole.MANAGER, is_active_member=True | |||
| ).values_list("journal", flat=True) | |||
| return qs.filter(journal__in=managed_journal_ids) | |||
There was a problem hiding this comment.
Each get_queryset method makes multiple database queries to check user permissions (one .exists() call and one or more .values_list() calls). This will execute on every request that accesses these ViewSets. Consider caching the user's role information on the request object or using select_related/prefetch_related to optimize the permission checks. For example, you could check all permission levels once at the start and store the results on the request, rather than running separate queries for collection_manager, journal_manager, and company_manager status.
team/signals.py
Outdated
| def _roles_for_user(model_class, user, fk_field): | ||
| """Return the set of active roles the user holds in a team model.""" | ||
| return set( | ||
| model_class.objects.filter(user=user, is_active_member=True) | ||
| .values_list("role", flat=True) | ||
| ) |
There was a problem hiding this comment.
The _roles_for_user function accepts an fk_field parameter that is never used. Either use this parameter in the function implementation or remove it from the function signature to avoid confusion.
There was a problem hiding this comment.
Removed in 6d9d38d. _roles_for_user(model_class, user) no longer takes the unused fk_field parameter.
| def _assign_permissions(self): | ||
| # COLLECTION_TEAM_ADMIN: can manage all team members and Company CRUD | ||
| collection_admin_group, _ = Group.objects.get_or_create(name=COLLECTION_TEAM_ADMIN) | ||
| collection_admin_permissions = self._get_model_permissions( | ||
| [CollectionTeamMember, Company, JournalTeamMember, CompanyTeamMember, JournalCompanyContract] | ||
| ) | ||
| collection_admin_group.permissions.set(collection_admin_permissions) | ||
|
|
||
| # JOURNAL_TEAM_ADMIN: can manage journal team members and Company Contracts CRUD | ||
| journal_admin_group, _ = Group.objects.get_or_create(name=JOURNAL_TEAM_ADMIN) | ||
| journal_admin_permissions = self._get_model_permissions( | ||
| [JournalTeamMember, JournalCompanyContract] | ||
| ) | ||
| journal_admin_group.permissions.set(journal_admin_permissions) | ||
|
|
||
| # COMPANY_TEAM_ADMIN: can manage company team members | ||
| company_admin_group, _ = Group.objects.get_or_create(name=COMPANY_TEAM_ADMIN) | ||
| company_admin_permissions = self._get_model_permissions([CompanyTeamMember]) | ||
| company_admin_group.permissions.set(company_admin_permissions) |
There was a problem hiding this comment.
The management command assigns Django model permissions to the groups (view, add, change, delete), but the ViewSets don't use a permission_helper_class to enforce these permissions for CRUD operations. The get_queryset methods only filter which records users can see, but don't prevent users from creating/editing/deleting records they shouldn't have access to. Consider implementing a custom permission_helper_class for each ViewSet to enforce the permission rules described in the PR (e.g., COLLECTION_TEAM_ADMIN can manage Company CRUD, JOURNAL_TEAM_ADMIN can manage JournalCompanyContract CRUD). See upload/permission_helper.py and upload/wagtail_hooks.py for examples of this pattern used in the codebase.
team/models.py
Outdated
| # Django group names for team-based access control | ||
| COLLECTION_TEAM_ADMIN = "COLLECTION_TEAM_ADMIN" | ||
| COLLECTION_TEAM_MEMBER = "COLLECTION_TEAM_MEMBER" | ||
| JOURNAL_TEAM_ADMIN = "JOURNAL_TEAM_ADMIN" | ||
| JOURNAL_TEAM_MEMBER = "JOURNAL_TEAM_MEMBER" | ||
| COMPANY_TEAM_ADMIN = "COMPANY_TEAM_ADMIN" | ||
| COMPANY_MEMBER = "COMPANY_MEMBER" | ||
|
|
||
| GROUP_NAMES = [ | ||
| COLLECTION_TEAM_ADMIN, | ||
| COLLECTION_TEAM_MEMBER, | ||
| JOURNAL_TEAM_ADMIN, | ||
| JOURNAL_TEAM_MEMBER, | ||
| COMPANY_TEAM_ADMIN, | ||
| COMPANY_MEMBER, | ||
| ] |
There was a problem hiding this comment.
Consider adding docstring comments to explain the group constants and their intended permissions. While GROUP_NAMES lists all groups, it's not immediately clear from the code what permissions each group should have without reading the PR description or management command. For example, adding comments like "# Collection managers - can manage all companies, journals, and team members" would improve code maintainability.
team/wagtail_hooks.py
Outdated
| # Managers see members of their own collections | ||
| managed_collection_ids = CollectionTeamMember.objects.filter( | ||
| user=user, role=TeamRole.MANAGER, is_active_member=True | ||
| ).values_list("collection", flat=True) | ||
| if managed_collection_ids: | ||
| return qs.filter(collection__in=managed_collection_ids) | ||
| # Regular members see only their own record | ||
| return qs.filter(user=user) |
There was a problem hiding this comment.
@copilot aqui e no restante do seu código de get_queryset, sugiro que se feito como classmethod do modelo correspondente para que fique transparente a implementação, pois este código certamente é repetitivo,
melhor se for um classmethod chamado
There was a problem hiding this comment.
Feito em 6d9d38d. A lógica de filtragem foi movida para classmethods get_queryset_for_user(cls, user, qs) em cada modelo (CollectionTeamMember, Company, JournalTeamMember, CompanyTeamMember, JournalCompanyContract). Os get_queryset dos ViewSets agora simplesmente delegam:
def get_queryset(self, request):
qs = super().get_queryset(request)
if request.user.is_superuser:
return qs
return CollectionTeamMember.get_queryset_for_user(request.user, qs)Isso também corrigiu o bug em JournalCompanyContractViewSet que retornava todos os contratos para usuários sem papel de manager (agora retorna qs.none()).
…ract queryset bug; cleanup signals Co-authored-by: robertatakenaka <505143+robertatakenaka@users.noreply.github.com>
Implementa controle de acesso baseado em papel/grupo para o módulo
team, definindo os grupos canônicos, filtrando querysets nos ViewSets do Wagtail conforme o papel do usuário autenticado, e sincronizando automaticamente os grupos Django (auth.Group) do usuário sempre que um membro de equipe é criado, atualizado ou removido.O que esse PR faz?
COLLECTION_TEAM_ADMIN,COLLECTION_TEAM_MEMBER,JOURNAL_TEAM_ADMIN,JOURNAL_TEAM_MEMBER,COMPANY_TEAM_ADMIN,COMPANY_MEMBER) e listaGROUP_NAMESemteam/models.pycreate_user_groupsque cria os grupos Django (auth.Group) e atribui permissões de modelo adequadas a cada umget_queryset_for_user(cls, user, qs)em cada modelo de equipe, centralizando a lógica de filtragem por papel no próprio modelo:CollectionTeamMemberget_queryset_for_userCompanyget_queryset_for_userCOLLECTION_TEAM_ADMINvê todas as empresas; demais veem apenas as suasJournalTeamMemberget_queryset_for_userCOLLECTION_TEAM_ADMINvê tudo;JOURNAL_TEAM_ADMINvê membros dos seus journals; member vê só o próprioCompanyTeamMemberget_queryset_for_userCOLLECTION_TEAM_ADMINvê todos;COMPANY_TEAM_ADMINvê membros das suas empresas; member vê só o próprioJournalCompanyContractget_queryset_for_userCOLLECTION_TEAM_ADMINvê todos;JOURNAL_TEAM_ADMINvê contratos dos seus journals; demais veem nenhum (qs.none())team/wagtail_hooks.pydelegam inteiramente para o classmethod correspondente, sem duplicação de lógica nos hooksJournalCompanyContractViewSetque retornava todos os contratos para usuários sem papel de manager (agora retornaqs.none())team/signals.pycom handlerspost_save/post_deletenos modelosCollectionTeamMember,JournalTeamMembereCompanyTeamMember, que chamamupdate_user_groups(user)para manter osauth.Groupdo usuário sincronizados de forma análoga aos usuários do Wagtail:role=MANAGEReis_active_member=True*_ADMINcorrespondenterole=MEMBEReis_active_member=True*_MEMBERcorrespondenteis_active_member=Falseou registro deletadoOs grupos são criados automaticamente via
get_or_create, dispensando execução prévia do comandocreate_user_groups.Superusuário não tem restrições em nenhum ViewSet.
Onde a revisão poderia começar?
team/models.py— classmethodsget_queryset_for_userem cada modelo e constantes de grupo com docstrings. Em seguida,team/wagtail_hooks.pypara ver como os ViewSets delegam ao modelo. Por fim,team/signals.pypara a lógica de sincronização de grupos eteam/management/commands/create_user_groups.pypara a criação dos grupos com permissões.Como este poderia ser testado manualmente?
CollectionTeamMember,JournalTeamMemberouCompanyTeamMemberCOLLECTION_TEAM_ADMIN,JOURNAL_TEAM_MEMBER)is_active_member=False) ou delete o registro e confirme que o usuário é removido do grupo correspondenteCOLLECTION_TEAM_ADMINconsegue visualizar e editar membros de equipes de journals (JournalTeamMember) e empresas (CompanyTeamMember) de qualquer journal/empresaJournalCompanyContractAlgum cenário de contexto que queira dar?
Antes desta mudança, todos os ViewSets de
team(excetoCollectionTeamMemberViewSet, que tinha uma implementação defeituosa chamandomembers()que pode retornarNone) exibiam todos os registros para qualquer usuário autenticado no Wagtail admin, e os usuários não tinham seusauth.Groupatualizados ao serem adicionados a equipes. Também havia um bug emJournalCompanyContractViewSetque retornava todos os contratos quando o usuário não possuía journals gerenciados.O projeto já possuía
TeamRole.MANAGER/MEMBERnos modelos; esta PR usa esses papéis como base tanto para a lógica de filtragem quanto para a sincronização automática dos grupos Django, de forma análoga ao tratamento de grupos de usuários no Wagtail.A lógica de filtragem foi centralizada em classmethods
get_queryset_for_usernos próprios modelos, tornando a implementação transparente e reutilizável. Os ViewSets delegam a esses classmethods, eliminando duplicação de código nos hooks.O
COLLECTION_TEAM_ADMINpossui o maior escopo de acesso entre os papéis não-superusuário: pode fazer CRUD deCompany,JournalTeamMembereCompanyTeamMember, além dos membros de suas próprias coleções.Screenshots
N/A — alterações no admin Wagtail (listagem de snippets filtrada por papel).
Quais são tickets relevantes?
Implemente a gestão dos grupos e perfis de usuários
Referências
get_querysetauth.Groupe permissõesOriginal prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.