From 9f3dc7c993b602644e303bcbd80c50194807affd Mon Sep 17 00:00:00 2001 From: jedi Date: Wed, 26 Aug 2026 22:18:55 +0200 Subject: [PATCH] stash --- backend/toolshed/api/group.py | 40 +++---- backend/toolshed/api/inventory.py | 79 ++++++++++--- ...ocation_unique_owner_scoped_id_and_more.py | 63 ++++++++++ backend/toolshed/models.py | 42 +++++-- backend/toolshed/serializers.py | 14 ++- backend/toolshed/tests/test_group.py | 66 +++++------ backend/toolshed/tests/test_inventory.py | 25 ++-- backend/toolshed/tests/test_locations.py | 109 +++++++++++++++++- 8 files changed, 335 insertions(+), 103 deletions(-) create mode 100644 backend/toolshed/migrations/0023_remove_storagelocation_storagelocation_unique_owner_scoped_id_and_more.py diff --git a/backend/toolshed/api/group.py b/backend/toolshed/api/group.py index 74e8e47..e682934 100644 --- a/backend/toolshed/api/group.py +++ b/backend/toolshed/api/group.py @@ -40,18 +40,11 @@ class GroupDetail(APIView, ViewSetMixin): authentication_classes = [SignatureAuthentication] permission_classes = [IsAuthenticated] - def get(self, request, pk, format=None): # /api/groups// - group = get_object_or_404(Group, pk=pk) - if not group.is_member(request.user): - return Response(status=status.HTTP_404_NOT_FOUND) - return Response(GroupSerializer(group).data) - - -class GroupDetailByHandle(APIView, ViewSetMixin): - authentication_classes = [SignatureAuthentication] - permission_classes = [IsAuthenticated] - - def get(self, request, name, domain, format=None): # /api/groups/handle/// + def get(self, request, handle, format=None): # /api/groups// + try: + name, domain = split_userhandle_or_throw(handle) + except ValueError: + return Response(status=status.HTTP_400_BAD_REQUEST, data={'status': 'invalid group handle'}) group = get_object_or_404(Group, name=name, domain=domain) if not group.is_member(request.user): return Response(status=status.HTTP_404_NOT_FOUND) @@ -61,8 +54,12 @@ class GroupDetailByHandle(APIView, ViewSetMixin): @api_view(['DELETE']) @authentication_classes([SignatureAuthentication]) @permission_classes([IsAuthenticated]) -def removeGroupMember(request, pk, identity_id, format=None): # /api/groups//members// - group = get_object_or_404(Group, pk=pk) +def removeGroupMember(request, handle, identity_id, format=None): # /api/groups//members// + try: + name, domain = split_userhandle_or_throw(handle) + except ValueError: + return Response(status=status.HTTP_400_BAD_REQUEST, data={'status': 'invalid group handle'}) + group = get_object_or_404(Group, name=name, domain=domain) if not group.is_member(request.user): return Response(status=status.HTTP_404_NOT_FOUND) member = get_object_or_404(group.members, pk=identity_id) @@ -76,8 +73,12 @@ def removeGroupMember(request, pk, identity_id, format=None): # /api/groups//invites/ - group = get_object_or_404(Group, pk=pk) +def createGroupInvite(request, handle, format=None): # /api/groups//invites/ + try: + name, domain = split_userhandle_or_throw(handle) + except ValueError: + return Response(status=status.HTTP_400_BAD_REQUEST, data={'status': 'invalid group handle'}) + group = get_object_or_404(Group, name=name, domain=domain) if not group.is_member(request.user): return Response(status=status.HTTP_404_NOT_FOUND) invitee = request.data.get('invitee') @@ -197,10 +198,9 @@ def acceptGroupInvite(request, format=None): # /api/group_invites/accept/ - lan urlpatterns = [ path('groups/', Groups.as_view(), name='groups'), - path('groups//', GroupDetail.as_view(), name='group_detail'), - path('groups/handle///', GroupDetailByHandle.as_view(), name='group_detail_by_handle'), - path('groups//members//', removeGroupMember, name='remove_group_member'), - path('groups//invites/', createGroupInvite, name='create_group_invite'), + path('groups//', GroupDetail.as_view(), name='group_detail'), + path('groups//members//', removeGroupMember, name='remove_group_member'), + path('groups//invites/', createGroupInvite, name='create_group_invite'), path('groupinvites/', GroupInvitesIncoming.as_view(), name='group_invites_incoming'), path('groupinvites//', declineGroupInvite, name='decline_group_invite'), path('groupinvites//accept/', recordGroupMembership, name='record_group_membership'), diff --git a/backend/toolshed/api/inventory.py b/backend/toolshed/api/inventory.py index 833c00e..3f32258 100644 --- a/backend/toolshed/api/inventory.py +++ b/backend/toolshed/api/inventory.py @@ -15,6 +15,21 @@ from toolshed.serializers import InventoryItemSerializer, StorageLocationSeriali router = routers.SimpleRouter() +def resolve_group_by_handle(handle): + """handle is "name@domain" (no leading '#') - the same format/parser group.py's GroupDetail + uses, so a group reference parses identically everywhere it appears (URL path, ?group= query + param, or an owner_group payload field), rather than some spots taking a handle and others a + bare pk.""" + try: + name, domain = split_userhandle_or_throw(handle) + except ValueError: + return None + try: + return Group.objects.get(name=name, domain=domain) + except Group.DoesNotExist: + return None + + def inventory_items(identity): try: user = identity.user.get() @@ -54,13 +69,10 @@ class InventoryItemViewSet(viewsets.ModelViewSet): if identity.user.exists(): return InventoryItem.objects.filter(owner=identity.user.get()) | group_items return group_items - group_id = self.request.query_params.get('group') - if group_id: - try: - group = Group.objects.get(pk=group_id) - except Group.DoesNotExist: - return InventoryItem.objects.none() - if not group.is_member(identity): + group_handle = self.request.query_params.get('group') + if group_handle: + group = resolve_group_by_handle(group_handle) + if not group or not group.is_member(identity): return InventoryItem.objects.none() return InventoryItem.objects.filter(owner_group=group) if identity.user.exists(): @@ -68,12 +80,11 @@ class InventoryItemViewSet(viewsets.ModelViewSet): return InventoryItem.objects.none() def perform_create(self, serializer): - group_id = self.request.data.get('owner_group') + group_handle = self.request.data.get('owner_group') with transaction.atomic(): - if group_id: - try: - group = Group.objects.get(pk=group_id) - except Group.DoesNotExist: + if group_handle: + group = resolve_group_by_handle(group_handle) + if not group: raise NotFound('No such group') if not group.is_member(self.request.user): raise PermissionDenied('Not a member of this group') @@ -155,21 +166,53 @@ class StorageLocationViewSet(viewsets.ModelViewSet): lookup_url_kwarg = 'pk' def get_queryset(self): - if type(self.request.user) == KnownIdentity and self.request.user.user.exists(): - return StorageLocation.objects.filter(owner=self.request.user.user.get()) + # Mirrors InventoryItemViewSet.get_queryset() - see its own comments for why the + # list/detail scopes differ and why group membership alone (no linked ToolshedUser + # required) is enough for the group branch. + if type(self.request.user) != KnownIdentity: + return StorageLocation.objects.none() + identity = self.request.user + group_locations = StorageLocation.objects.filter(owner_group__in=identity.member_of_groups.all()) + if self.action != 'list': + if identity.user.exists(): + return StorageLocation.objects.filter(owner=identity.user.get()) | group_locations + return group_locations + group_handle = self.request.query_params.get('group') + if group_handle: + group = resolve_group_by_handle(group_handle) + if not group or not group.is_member(identity): + return StorageLocation.objects.none() + return StorageLocation.objects.filter(owner_group=group) + if identity.user.exists(): + return StorageLocation.objects.filter(owner=identity.user.get()) return StorageLocation.objects.none() def perform_create(self, serializer): + group_handle = self.request.data.get('owner_group') with transaction.atomic(): - serializer.save(owner=self.request.user.user.get()) + if group_handle: + group = resolve_group_by_handle(group_handle) + if not group: + raise NotFound('No such group') + if not group.is_member(self.request.user): + raise PermissionDenied('Not a member of this group') + serializer.save(owner=None, owner_group=group).clean() + else: + serializer.save(owner=self.request.user.user.get()).clean() + + @staticmethod + def _is_authorized(request, instance): + if instance.owner_id: + return request.user.user.filter(pk=instance.owner_id).exists() + return instance.owner_group.is_member(request.user) def perform_update(self, serializer): with transaction.atomic(): - if serializer.instance.owner == self.request.user.user.get(): - serializer.save() + if self._is_authorized(self.request, serializer.instance): + serializer.save().clean() def perform_destroy(self, instance): - if instance.owner == self.request.user.user.get(): + if self._is_authorized(self.request, instance): instance.delete() diff --git a/backend/toolshed/migrations/0023_remove_storagelocation_storagelocation_unique_owner_scoped_id_and_more.py b/backend/toolshed/migrations/0023_remove_storagelocation_storagelocation_unique_owner_scoped_id_and_more.py new file mode 100644 index 0000000..6ef08a2 --- /dev/null +++ b/backend/toolshed/migrations/0023_remove_storagelocation_storagelocation_unique_owner_scoped_id_and_more.py @@ -0,0 +1,63 @@ +# Generated by Django 4.2.2 on 2026-08-26 18:55 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('authentication', '0006_groupmembership'), + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ('toolshed', '0022_storagelocation_id_not_null_and_unique'), + ] + + operations = [ + migrations.RemoveConstraint( + model_name='storagelocation', + name='storagelocation_unique_owner_scoped_id', + ), + migrations.AddField( + model_name='ownerstoragelocationsequence', + name='owner_group', + field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, related_name='+', to='authentication.group'), + ), + migrations.AddField( + model_name='storagelocation', + name='owner_group', + field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, related_name='storage_locations', to='authentication.group'), + ), + migrations.AlterField( + model_name='owneritemsequence', + name='id', + field=models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID'), + ), + migrations.AlterField( + model_name='ownerstoragelocationsequence', + name='id', + field=models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID'), + ), + migrations.AlterField( + model_name='ownerstoragelocationsequence', + name='owner', + field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, related_name='+', to=settings.AUTH_USER_MODEL), + ), + migrations.AlterField( + model_name='storagelocation', + name='owner', + field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, related_name='storage_locations', to=settings.AUTH_USER_MODEL), + ), + migrations.AddConstraint( + model_name='ownerstoragelocationsequence', + constraint=models.UniqueConstraint(condition=models.Q(('owner__isnull', False)), fields=('owner',), name='ownerstoragelocationsequence_unique_owner'), + ), + migrations.AddConstraint( + model_name='ownerstoragelocationsequence', + constraint=models.UniqueConstraint(condition=models.Q(('owner_group__isnull', False)), fields=('owner_group',), name='ownerstoragelocationsequence_unique_owner_group'), + ), + migrations.AddConstraint( + model_name='storagelocation', + constraint=models.UniqueConstraint(fields=('owner', 'owner_group', 'id'), name='storagelocation_unique_owner_scoped_id'), + ), + ] diff --git a/backend/toolshed/models.py b/backend/toolshed/models.py index 6136a86..9989cba 100644 --- a/backend/toolshed/models.py +++ b/backend/toolshed/models.py @@ -170,15 +170,25 @@ class ItemTag(models.Model): class OwnerStorageLocationSequence(models.Model): - """Tracks the last StorageLocation id handed out per owner for sequential, gapless allocation - (see StorageLocation.create_for_owner).""" - owner = models.ForeignKey(ToolshedUser, on_delete=models.CASCADE, related_name='+', unique=True) + """Tracks the last StorageLocation id handed out per owner/owner_group scope for sequential, + gapless allocation (see StorageLocation.create_for_owner); exactly one of owner/owner_group is + set, mirroring OwnerItemSequence/InventoryItem's own split.""" + owner = models.ForeignKey(ToolshedUser, on_delete=models.CASCADE, null=True, blank=True, related_name='+') + owner_group = models.ForeignKey(Group, on_delete=models.CASCADE, null=True, blank=True, related_name='+') last_id = models.PositiveIntegerField(default=0) + class Meta: + constraints = [ + models.UniqueConstraint(fields=['owner'], condition=models.Q(owner__isnull=False), + name='ownerstoragelocationsequence_unique_owner'), + models.UniqueConstraint(fields=['owner_group'], condition=models.Q(owner_group__isnull=False), + name='ownerstoragelocationsequence_unique_owner_group'), + ] + @classmethod - def allocate(cls, *, owner): + def allocate(cls, *, owner=None, owner_group=None): with transaction.atomic(): - seq, _ = cls.objects.select_for_update().get_or_create(owner=owner) + seq, _ = cls.objects.select_for_update().get_or_create(owner=owner, owner_group=owner_group) seq.last_id += 1 seq.save(update_fields=['last_id']) return seq.last_id @@ -186,7 +196,7 @@ class OwnerStorageLocationSequence(models.Model): class StorageLocation(models.Model): internal_id = models.AutoField(primary_key=True) - # Externally visible id, sequential/gapless within the owner's own locations (see + # Externally visible id, sequential/gapless within the owner/owner_group's own locations (see # OwnerStorageLocationSequence), never internal_id; always allocate via create_for_owner, not # .objects.create(). id = models.PositiveIntegerField(editable=False) @@ -195,24 +205,32 @@ class StorageLocation(models.Model): category = models.ForeignKey(Category, on_delete=models.CASCADE, null=True, blank=True, related_name='storage_locations') parent = models.ForeignKey('self', on_delete=models.CASCADE, null=True, blank=True, related_name='children') - owner = models.ForeignKey(ToolshedUser, on_delete=models.CASCADE, related_name='storage_locations') + owner = models.ForeignKey(ToolshedUser, on_delete=models.CASCADE, null=True, blank=True, + related_name='storage_locations') + owner_group = models.ForeignKey(Group, on_delete=models.CASCADE, null=True, blank=True, + related_name='storage_locations') class Meta: constraints = [ - models.UniqueConstraint(fields=['owner', 'id'], name='storagelocation_unique_owner_scoped_id'), + models.UniqueConstraint(fields=['owner', 'owner_group', 'id'], + name='storagelocation_unique_owner_scoped_id'), ] def __str__(self): parent = str(self.parent) + "/" if self.parent else "" return parent + self.name + def clean(self): + if (self.owner is None) == (self.owner_group is None): + raise ValidationError("Exactly one of owner or owner_group must be set") + @classmethod - def create_for_owner(cls, *, owner, **kwargs): + def create_for_owner(cls, *, owner=None, owner_group=None, **kwargs): """The only supported way to create a StorageLocation: atomically allocates the next id - for this owner's scope.""" + for this owner/owner_group's scope.""" with transaction.atomic(): - next_id = OwnerStorageLocationSequence.allocate(owner=owner) - return cls.objects.create(owner=owner, id=next_id, **kwargs) + next_id = OwnerStorageLocationSequence.allocate(owner=owner, owner_group=owner_group) + return cls.objects.create(owner=owner, owner_group=owner_group, id=next_id, **kwargs) class WorkflowInstance(models.Model): diff --git a/backend/toolshed/serializers.py b/backend/toolshed/serializers.py index 0bfe6b0..846117b 100644 --- a/backend/toolshed/serializers.py +++ b/backend/toolshed/serializers.py @@ -1,4 +1,5 @@ from django.core.exceptions import ObjectDoesNotExist +from django.db.models import Q from rest_framework import serializers from authentication.models import KnownIdentity, ToolshedUser, FriendRequestIncoming, Group, GroupInviteIncoming, \ GroupMembership @@ -177,9 +178,13 @@ class OwnerScopedPrimaryKeyRelatedField(serializers.PrimaryKeyRelatedField): def get_queryset(self): queryset = super().get_queryset() request = self.context.get('request') - if request is not None and type(request.user) == KnownIdentity and request.user.user.exists(): - return queryset.filter(owner=request.user.user.get()) - return queryset.none() + if request is None or type(request.user) != KnownIdentity: + return queryset.none() + identity = request.user + owner_filter = Q(owner_group__in=identity.member_of_groups.all()) + if identity.user.exists(): + owner_filter |= Q(owner=identity.user.get()) + return queryset.filter(owner_filter) def to_internal_value(self, data): queryset = self.get_queryset() @@ -198,6 +203,7 @@ class OwnerScopedPrimaryKeyRelatedField(serializers.PrimaryKeyRelatedField): class StorageLocationSerializer(serializers.ModelSerializer): owner = OwnerSerializer(read_only=True) + owner_group = GroupOwnerSerializer(read_only=True) category = serializers.CharField(required=False, allow_null=True, allow_blank=True) parent = OwnerScopedPrimaryKeyRelatedField(queryset=StorageLocation.objects.all(), required=False, allow_null=True) @@ -205,7 +211,7 @@ class StorageLocationSerializer(serializers.ModelSerializer): class Meta: model = StorageLocation - fields = ['id', 'name', 'description', 'path', 'category', 'owner', 'parent'] + fields = ['id', 'name', 'description', 'path', 'category', 'owner', 'owner_group', 'parent'] read_only_fields = ['id', 'path'] @staticmethod diff --git a/backend/toolshed/tests/test_group.py b/backend/toolshed/tests/test_group.py index bf9ac7a..f3f1405 100644 --- a/backend/toolshed/tests/test_group.py +++ b/backend/toolshed/tests/test_group.py @@ -53,56 +53,47 @@ class GroupApiTestCase(UserTestMixin, GroupTestMixin, ToolshedTestCase): def test_group_detail_member(self): self.prepare_groups() - reply = client.get('/api/groups/{}/'.format(self.f['group1'].id), self.f['local_user1']) + group = self.f['group1'] + reply = client.get('/api/groups/{}@{}/'.format(group.name, group.domain), self.f['local_user1']) self.assertEqual(reply.status_code, 200) - self.assertEqual(reply.json()['handle'], str(self.f['group1'])) + self.assertEqual(reply.json()['handle'], str(group)) self.assertEqual(len(reply.json()['members']), 1) def test_group_detail_non_member(self): self.prepare_groups() - reply = client.get('/api/groups/{}/'.format(self.f['group1'].id), self.f['local_user2']) + group = self.f['group1'] + reply = client.get('/api/groups/{}@{}/'.format(group.name, group.domain), self.f['local_user2']) self.assertEqual(reply.status_code, 404) - def test_group_detail_by_handle_member(self): - self.prepare_groups() - group = self.f['group1'] - reply = client.get('/api/groups/handle/{}/{}/'.format(group.name, group.domain), self.f['local_user1']) - self.assertEqual(reply.status_code, 200) - self.assertEqual(reply.json()['id'], group.id) - self.assertEqual(reply.json()['handle'], str(group)) - - def test_group_detail_by_handle_non_member(self): - self.prepare_groups() - group = self.f['group1'] - reply = client.get('/api/groups/handle/{}/{}/'.format(group.name, group.domain), self.f['local_user2']) - self.assertEqual(reply.status_code, 404) - - def test_group_detail_by_handle_no_such_group(self): - reply = client.get('/api/groups/handle/nonexistent/example.com/', self.f['local_user1']) + def test_group_detail_no_such_group(self): + reply = client.get('/api/groups/nonexistent@example.com/', self.f['local_user1']) self.assertEqual(reply.status_code, 404) def test_remove_member(self): self.prepare_groups() - self.f['group1'].members.add(self.f['local_user2'].public_identity) - reply = client.delete('/api/groups/{}/members/{}/'.format( - self.f['group1'].id, self.f['local_user2'].public_identity.id), self.f['local_user1']) + group = self.f['group1'] + group.members.add(self.f['local_user2'].public_identity) + reply = client.delete('/api/groups/{}@{}/members/{}/'.format( + group.name, group.domain, self.f['local_user2'].public_identity.id), self.f['local_user1']) self.assertEqual(reply.status_code, 204) - self.assertEqual(self.f['group1'].members.count(), 1) + self.assertEqual(group.members.count(), 1) def test_remove_last_member_blocked(self): self.prepare_groups() - reply = client.delete('/api/groups/{}/members/{}/'.format( - self.f['group1'].id, self.f['local_user1'].public_identity.id), self.f['local_user1']) + group = self.f['group1'] + reply = client.delete('/api/groups/{}@{}/members/{}/'.format( + group.name, group.domain, self.f['local_user1'].public_identity.id), self.f['local_user1']) self.assertEqual(reply.status_code, 400) - self.assertEqual(self.f['group1'].members.count(), 1) + self.assertEqual(group.members.count(), 1) def test_remove_member_non_member_denied(self): self.prepare_groups() - self.f['group1'].members.add(self.f['local_user2'].public_identity) - reply = client.delete('/api/groups/{}/members/{}/'.format( - self.f['group1'].id, self.f['local_user1'].public_identity.id), self.f['ext_user1']) + group = self.f['group1'] + group.members.add(self.f['local_user2'].public_identity) + reply = client.delete('/api/groups/{}@{}/members/{}/'.format( + group.name, group.domain, self.f['local_user1'].public_identity.id), self.f['ext_user1']) self.assertEqual(reply.status_code, 404) - self.assertEqual(self.f['group1'].members.count(), 2) + self.assertEqual(group.members.count(), 2) class GroupInviteApiTestCase(UserTestMixin, GroupTestMixin, ToolshedTestCase): @@ -116,7 +107,8 @@ class GroupInviteApiTestCase(UserTestMixin, GroupTestMixin, ToolshedTestCase): inviter = self.f['local_user1'] invitee = self.f['local_user2'] - reply1 = client.post('/api/groups/{}/invites/'.format(group.id), inviter, {'invitee': str(invitee)}) + reply1 = client.post('/api/groups/{}@{}/invites/'.format(group.name, group.domain), inviter, + {'invitee': str(invitee)}) self.assertEqual(reply1.status_code, 201) secret = reply1.json()['secret'] self.assertEqual(GroupInvite.objects.count(), 1) @@ -152,7 +144,7 @@ class GroupInviteApiTestCase(UserTestMixin, GroupTestMixin, ToolshedTestCase): def test_invite_non_member_denied(self): group = self.f['group1'] - reply = client.post('/api/groups/{}/invites/'.format(group.id), self.f['local_user2'], + reply = client.post('/api/groups/{}@{}/invites/'.format(group.name, group.domain), self.f['local_user2'], {'invitee': str(self.f['ext_user1'])}) self.assertEqual(reply.status_code, 404) self.assertEqual(GroupInvite.objects.count(), 0) @@ -160,7 +152,7 @@ class GroupInviteApiTestCase(UserTestMixin, GroupTestMixin, ToolshedTestCase): def test_invite_already_member(self): group = self.f['group1'] group.members.add(self.f['local_user2'].public_identity) - reply = client.post('/api/groups/{}/invites/'.format(group.id), self.f['local_user1'], + reply = client.post('/api/groups/{}@{}/invites/'.format(group.name, group.domain), self.f['local_user1'], {'invitee': str(self.f['local_user2'])}) self.assertEqual(reply.status_code, 208) @@ -177,7 +169,7 @@ class GroupInviteApiTestCase(UserTestMixin, GroupTestMixin, ToolshedTestCase): group = self.f['group1'] inviter = self.f['local_user1'] invitee = self.f['local_user2'] - client.post('/api/groups/{}/invites/'.format(group.id), inviter, {'invitee': str(invitee)}) + client.post('/api/groups/{}@{}/invites/'.format(group.name, group.domain), inviter, {'invitee': str(invitee)}) reply = client.post('/api/group_invites/accept/', invitee, { 'group': str(group), 'invitee': str(invitee), 'invitee_key': invitee.public_key(), 'secret': 'not-the-right-secret' @@ -189,7 +181,8 @@ class GroupInviteApiTestCase(UserTestMixin, GroupTestMixin, ToolshedTestCase): group = self.f['group1'] inviter = self.f['local_user1'] invitee = DummyExternalUser('newmember', 'remote.example', known=False) - reply1 = client.post('/api/groups/{}/invites/'.format(group.id), inviter, {'invitee': str(invitee)}) + reply1 = client.post('/api/groups/{}@{}/invites/'.format(group.name, group.domain), inviter, + {'invitee': str(invitee)}) secret = reply1.json()['secret'] reply = client.post('/api/group_invites/accept/', invitee, { @@ -206,7 +199,8 @@ class GroupInviteApiTestCase(UserTestMixin, GroupTestMixin, ToolshedTestCase): group = self.f['group1'] inviter = self.f['local_user1'] invitee = self.f['local_user2'] - reply1 = client.post('/api/groups/{}/invites/'.format(group.id), inviter, {'invitee': str(invitee)}) + reply1 = client.post('/api/groups/{}@{}/invites/'.format(group.name, group.domain), inviter, + {'invitee': str(invitee)}) secret = reply1.json()['secret'] bad_signature_client = SignatureAuthClient(bad_signature=True) reply = bad_signature_client.post('/api/group_invites/accept/', invitee, { diff --git a/backend/toolshed/tests/test_inventory.py b/backend/toolshed/tests/test_inventory.py index eba0c12..8a1b1a5 100644 --- a/backend/toolshed/tests/test_inventory.py +++ b/backend/toolshed/tests/test_inventory.py @@ -353,7 +353,7 @@ class GroupOwnedInventoryApiTestCase(UserTestMixin, GroupTestMixin, CategoryTest def create_group_item(self, name='drill'): return client.post('/api/inventory_items/', self.f['local_user1'], { 'name': name, 'owned_quantity': 1, 'availability_policy': 'private', - 'owner_group': self.f['group1'].id, + 'owner_group': str(self.f['group1'])[1:], }) def test_create_group_owned_item(self): @@ -368,7 +368,7 @@ class GroupOwnedInventoryApiTestCase(UserTestMixin, GroupTestMixin, CategoryTest def test_create_group_owned_item_non_member_denied(self): reply = client.post('/api/inventory_items/', self.f['ext_user1'], { 'name': 'drill', 'owned_quantity': 1, 'availability_policy': 'private', - 'owner_group': self.f['group1'].id, + 'owner_group': str(self.f['group1'])[1:], }) self.assertEqual(reply.status_code, 403) self.assertEqual(InventoryItem.objects.count(), 0) @@ -413,14 +413,14 @@ class GroupOwnedInventoryApiTestCase(UserTestMixin, GroupTestMixin, CategoryTest def test_group_items_listed_by_group_query_param(self): self.create_group_item() - reply = client.get('/api/inventory_items/?group={}'.format(self.f['group1'].id), self.f['local_user2']) + reply = client.get('/api/inventory_items/?group={}'.format(str(self.f['group1'])[1:]), self.f['local_user2']) self.assertEqual(reply.status_code, 200) self.assertEqual(len(reply.json()), 1) self.assertEqual(reply.json()[0]['name'], 'drill') def test_group_items_not_listed_for_non_member_query_param(self): self.create_group_item() - reply = client.get('/api/inventory_items/?group={}'.format(self.f['group1'].id), self.f['ext_user1']) + reply = client.get('/api/inventory_items/?group={}'.format(str(self.f['group1'])[1:]), self.f['ext_user1']) self.assertEqual(reply.status_code, 200) self.assertEqual(len(reply.json()), 0) @@ -435,7 +435,7 @@ class GroupOwnedInventoryApiTestCase(UserTestMixin, GroupTestMixin, CategoryTest 'owned_quantity': 3, 'tags': ['tag1', 'tag2'], 'properties': [{'name': 'prop1', 'value': 'value1'}, {'name': 'prop2', 'value': 'value2'}], - 'owner_group': self.f['group1'].id, + 'owner_group': str(self.f['group1'])[1:], }) self.assertEqual(reply.status_code, 201) item = InventoryItem.objects.get(name='drill') @@ -451,20 +451,21 @@ class GroupOwnedInventoryApiTestCase(UserTestMixin, GroupTestMixin, CategoryTest def test_create_group_owned_item_empty_fails(self): # Parity with InventoryApiTestCase.test_post_new_item_empty: clean()'s name-or-files validation still applies. reply = client.post('/api/inventory_items/', self.f['local_user1'], { - 'availability_policy': 'private', 'owned_quantity': 1, 'owner_group': self.f['group1'].id, + 'availability_policy': 'private', 'owned_quantity': 1, 'owner_group': str(self.f['group1'])[1:], }) self.assertEqual(reply.status_code, 400) self.assertEqual(InventoryItem.objects.count(), 0) def test_create_group_owned_item_nonexistent_group(self): reply = client.post('/api/inventory_items/', self.f['local_user1'], { - 'name': 'drill', 'owned_quantity': 1, 'availability_policy': 'private', 'owner_group': 999999, + 'name': 'drill', 'owned_quantity': 1, 'availability_policy': 'private', + 'owner_group': 'nonexistent@example.com', }) self.assertEqual(reply.status_code, 404) self.assertEqual(InventoryItem.objects.count(), 0) def test_group_items_listed_for_nonexistent_group_query_param(self): - reply = client.get('/api/inventory_items/?group=999999', self.f['local_user1']) + reply = client.get('/api/inventory_items/?group=nonexistent@example.com', self.f['local_user1']) self.assertEqual(reply.status_code, 200) self.assertEqual(len(reply.json()), 0) @@ -495,7 +496,7 @@ class GroupOwnedInventoryApiTestCase(UserTestMixin, GroupTestMixin, CategoryTest reply = client.post('/api/inventory_items/', self.f['local_user1'], { 'name': 'drill', 'owned_quantity': 1, 'availability_policy': 'private', 'category': 'cat1', 'tags': ['tag1'], - 'owner_group': self.f['group1'].id, + 'owner_group': str(self.f['group1'])[1:], }) item_id = reply.json()['id'] reply = client.patch('/api/inventory_items/{}/'.format(item_id), self.f['local_user2'], { @@ -509,7 +510,7 @@ class GroupOwnedInventoryApiTestCase(UserTestMixin, GroupTestMixin, CategoryTest def test_group_item_storage_location(self): reply = client.post('/api/inventory_items/', self.f['local_user1'], { 'name': 'drill', 'owned_quantity': 1, 'availability_policy': 'private', - 'storage_location': self.f['loc1'].id, 'owner_group': self.f['group1'].id, + 'storage_location': self.f['loc1'].id, 'owner_group': str(self.f['group1'])[1:], }) self.assertEqual(reply.status_code, 201) item = InventoryItem.objects.get(name='drill') @@ -519,7 +520,7 @@ class GroupOwnedInventoryApiTestCase(UserTestMixin, GroupTestMixin, CategoryTest # Parity with TestInventoryItemWithFileApiTestCase.test_post_item_with_file_id. reply = client.post('/api/inventory_items/', self.f['local_user1'], { 'name': 'drill', 'owned_quantity': 1, 'availability_policy': 'private', - 'files': [self.f['test_file1'].id], 'owner_group': self.f['group1'].id, + 'files': [self.f['test_file1'].id], 'owner_group': str(self.f['group1'])[1:], }) self.assertEqual(reply.status_code, 201) item = InventoryItem.objects.get(name='drill') @@ -529,7 +530,7 @@ class GroupOwnedInventoryApiTestCase(UserTestMixin, GroupTestMixin, CategoryTest reply = client.post('/api/inventory_items/', self.f['local_user1'], { 'name': 'drill', 'owned_quantity': 1, 'availability_policy': 'private', 'files': [{'data': self.f['encoded_content3'], 'mime_type': 'text/plain'}], - 'owner_group': self.f['group1'].id, + 'owner_group': str(self.f['group1'])[1:], }) self.assertEqual(reply.status_code, 201) item = InventoryItem.objects.get(name='drill') diff --git a/backend/toolshed/tests/test_locations.py b/backend/toolshed/tests/test_locations.py index 438775d..6850db4 100644 --- a/backend/toolshed/tests/test_locations.py +++ b/backend/toolshed/tests/test_locations.py @@ -1,4 +1,4 @@ -from authentication.tests import SignatureAuthClient, UserTestMixin, ToolshedTestCase +from authentication.tests import SignatureAuthClient, UserTestMixin, GroupTestMixin, ToolshedTestCase from files.tests import FilesTestMixin from toolshed.models import InventoryItem, Category, StorageLocation from toolshed.tests import InventoryTestMixin, LocationTestMixin @@ -129,3 +129,110 @@ class LocationApiTestCase(UserTestMixin, InventoryTestMixin, LocationTestMixin, self.assertEqual(reply.status_code, 404) self.assertEqual(StorageLocation.objects.filter(id=self.f['loc1'].id).count(), 1) + +class GroupOwnedLocationApiTestCase(UserTestMixin, GroupTestMixin, ToolshedTestCase): + def setUp(self): + super().setUp() + self.prepare_users() + self.prepare_groups() + self.f['group1'].members.add(self.f['local_user2'].public_identity) + + def create_group_location(self, name='shelf'): + return client.post('/api/storage_locations/', self.f['local_user1'], { + 'name': name, 'owner_group': str(self.f['group1'])[1:], + }) + + def test_create_group_owned_location(self): + reply = self.create_group_location() + self.assertEqual(reply.status_code, 201) + location = StorageLocation.objects.get(name='shelf') + self.assertIsNone(location.owner) + self.assertEqual(location.owner_group, self.f['group1']) + self.assertEqual(reply.json()['owner_group'], str(self.f['group1'])) + self.assertIsNone(reply.json()['owner']) + + def test_create_group_owned_location_non_member_denied(self): + reply = client.post('/api/storage_locations/', self.f['ext_user1'], { + 'name': 'shelf', 'owner_group': str(self.f['group1'])[1:], + }) + self.assertEqual(reply.status_code, 403) + self.assertEqual(StorageLocation.objects.count(), 0) + + def test_other_member_can_edit(self): + location_id = self.create_group_location().json()['id'] + reply = client.patch('/api/storage_locations/{}/'.format(location_id), self.f['local_user2'], { + 'name': 'shelf-renamed' + }) + self.assertEqual(reply.status_code, 200) + self.assertEqual(StorageLocation.objects.get(id=location_id).name, 'shelf-renamed') + + def test_other_member_can_delete(self): + location_id = self.create_group_location().json()['id'] + reply = client.delete('/api/storage_locations/{}/'.format(location_id), self.f['local_user2']) + self.assertEqual(reply.status_code, 204) + self.assertEqual(StorageLocation.objects.filter(id=location_id).count(), 0) + + def test_remote_member_without_local_account_can_edit(self): + # A remote member (KnownIdentity, no ToolshedUser row) must still act on group-owned + # locations - not unauthorized just because .user.exists() is False. + self.f['group1'].members.add(self.f['ext_user1'].public_identity) + location_id = self.create_group_location().json()['id'] + reply = client.get('/api/storage_locations/{}/'.format(location_id), self.f['ext_user1']) + self.assertEqual(reply.status_code, 200) + reply = client.patch('/api/storage_locations/{}/'.format(location_id), self.f['ext_user1'], { + 'name': 'shelf-renamed-by-remote-member' + }) + self.assertEqual(reply.status_code, 200) + self.assertEqual(StorageLocation.objects.get(id=location_id).name, 'shelf-renamed-by-remote-member') + + def test_non_member_cannot_see_or_edit(self): + location_id = self.create_group_location().json()['id'] + reply = client.get('/api/storage_locations/{}/'.format(location_id), self.f['ext_user1']) + self.assertEqual(reply.status_code, 404) + + def test_group_locations_excluded_from_personal_list(self): + self.create_group_location() + reply = client.get('/api/storage_locations/', self.f['local_user1']) + self.assertEqual(reply.status_code, 200) + self.assertEqual(len(reply.json()), 0) + + def test_group_locations_listed_by_group_query_param(self): + self.create_group_location() + reply = client.get('/api/storage_locations/?group={}'.format(str(self.f['group1'])[1:]), self.f['local_user2']) + self.assertEqual(reply.status_code, 200) + self.assertEqual(len(reply.json()), 1) + self.assertEqual(reply.json()[0]['name'], 'shelf') + + def test_group_locations_not_listed_for_non_member_query_param(self): + self.create_group_location() + reply = client.get('/api/storage_locations/?group={}'.format(str(self.f['group1'])[1:]), self.f['ext_user1']) + self.assertEqual(reply.status_code, 200) + self.assertEqual(len(reply.json()), 0) + + def test_group_location_can_be_parent_for_group_member(self): + parent_id = self.create_group_location('shelf').json()['id'] + reply = client.post('/api/storage_locations/', self.f['local_user2'], { + 'name': 'bin', 'owner_group': str(self.f['group1'])[1:], 'parent': parent_id, + }) + self.assertEqual(reply.status_code, 201) + self.assertEqual(reply.json()['path'], 'shelf/bin') + + def test_own_personal_location_can_be_parent_of_group_location(self): + # OwnerScopedPrimaryKeyRelatedField (see serializers.py) scopes `parent` to every + # location the requester can act on - own personal ones plus any group's - with no + # further check that it matches the new location's own owner_group. Same behavior + # InventoryItem.storage_location already had before group ownership existed here; not + # something this feature narrows. + personal = StorageLocation.create_for_owner(name='mine', owner=self.f['local_user1']) + reply = client.post('/api/storage_locations/', self.f['local_user1'], { + 'name': 'bin', 'owner_group': str(self.f['group1'])[1:], 'parent': personal.id, + }) + self.assertEqual(reply.status_code, 201) + + def test_group_location_not_valid_parent_for_non_member(self): + parent_id = self.create_group_location('shelf').json()['id'] + reply = client.post('/api/storage_locations/', self.f['ext_user1'], { + 'name': 'bin', 'parent': parent_id, + }) + self.assertEqual(reply.status_code, 400) +