[BUG] Fix ownership (#4244)

* Reenable ownership tests

* [BUG] Stock item ownership results in stock item being read-only
Fixes #4229

* rebuild ownership tests

* jsut test stock stuff for now

* move ownership check to Owner

* fix assertation with lazy objects

* test all of stock

* Add edit checks

* remove old tests

* run full coverage again

* fix test

InvenTree/stock/models.py

@@ -146,7 +146,7 @@ class StockLocation(InvenTreeBarcodeMixin, MetadataMixin, InvenTreeTree):
             # So, no ownership checks to perform!
             return True
 
-        return user in owner.get_related_owners(include_group=True)
+        return owner.is_user_allowed(user, include_group=True)
 
     def clean(self):
         """Custom clean action for the StockLocation model:
@@ -864,7 +864,7 @@ class StockItem(InvenTreeBarcodeMixin, MetadataMixin, MPTTModel):
         if owner is None:
             return True
 
-        return user in owner.get_related_owners(include_group=True)
+        return owner.is_user_allowed(user, include_group=True)
 
     def is_stale(self):
         """Returns True if this Stock item is "stale".

InvenTree/stock/test_views.py

@@ -1,10 +1,13 @@
 """Unit tests for Stock views (see views.py)."""
 
+from django.contrib.auth.models import Group
 from django.urls import reverse
 
+from common.models import InvenTreeSetting
 from InvenTree.helpers import InvenTreeTestCase
-
+from InvenTree.status_codes import StockStatus
-# from common.models import InvenTreeSetting
+from stock.models import StockItem, StockLocation
+from users.models import Owner
 
 
 class StockViewTestCase(InvenTreeTestCase):
@@ -84,124 +87,85 @@ class StockDetailTest(StockViewTestCase):
 
 class StockOwnershipTest(StockViewTestCase):
     """Tests for stock ownership views."""
-
+    test_item_id = 11
-    def setUp(self):
+    test_location_id = 1
-        """Add another user for ownership tests."""
-
-    """
-    TODO: Refactor this following test to use the new API form
-
-        super().setUp()
-
-        # Promote existing user with staff, admin and superuser statuses
-        self.user.is_staff = True
-        self.user.is_admin = True
-        self.user.is_superuser = True
-        self.user.save()
-
-        # Create a new user
-        user = get_user_model()
-
-        self.new_user = user.objects.create_user(
-            username='john',
-            email='john@email.com',
-            password='custom123',
-        )
-
-        # Put the user into a new group with the correct permissions
-        group = Group.objects.create(name='new_group')
-        self.new_user.groups.add(group)
-
-        # Give the group *all* the permissions!
-        for rule in group.rule_sets.all():
-            rule.can_view = True
-            rule.can_change = True
-            rule.can_add = True
-            rule.can_delete = True
-
-            rule.save()
 
     def enable_ownership(self):
+        """Helper function to turn on ownership control."""
         # Enable stock location ownership
 
         InvenTreeSetting.set_setting('STOCK_OWNERSHIP_CONTROL', True, self.user)
         self.assertEqual(True, InvenTreeSetting.get_setting('STOCK_OWNERSHIP_CONTROL'))
 
-    def test_owner_control(self):
+    def assert_ownership(self, assertio: bool = True, user=None):
-        # Test stock location and item ownership
+        """Helper function to check ownership control."""
-        from .models import StockLocation
+        if user is None:
-        from users.models import Owner
+            user = self.user
 
-        new_user_group = self.new_user.groups.all()[0]
+        item = StockItem.objects.get(pk=self.test_item_id)
-        new_user_group_owner = Owner.get_owner(new_user_group)
+        self.assertEqual(assertio, item.check_ownership(user))
 
-        user_as_owner = Owner.get_owner(self.user)
+        location = StockLocation.objects.get(pk=self.test_location_id)
-        new_user_as_owner = Owner.get_owner(self.new_user)
+        self.assertEqual(assertio, location.check_ownership(user))
 
-        # Enable ownership control
+    def assert_api_change(self):
+        """Helper function to get response to API change."""
+        return self.client.patch(
+            reverse('api-stock-detail', args=(self.test_item_id,)),
+            {'status': StockStatus.DAMAGED},
+            content_type='application/json',
+        )
+
+    def test_owner_no_ownership(self):
+        """Check without ownership control enabled. Should always return True."""
+        self.assert_ownership(True)
+
+    def test_ownership_as_superuser(self):
+        """Test that superuser are always allowed to access items."""
         self.enable_ownership()
 
-        test_location_id = 4
+        # Check with superuser
-        test_item_id = 11
+        self.user.is_superuser = True
-        # Set ownership on existing item (and change location)
+        self.user.save()
-        response = self.client.post(reverse('stock-item-edit', args=(test_item_id,)),
+        self.assert_ownership(True)
-                                    {'part': 1, 'status': StockStatus.OK, 'owner': user_as_owner.pk},
-                                    HTTP_X_REQUESTED_WITH='XMLHttpRequest')
 
-        self.assertContains(response, '"form_valid": true', status_code=200)
+    def test_ownership_functions(self):
+        """Test that ownership is working correctly for StockItem/StockLocation."""
+        self.enable_ownership()
+        item = StockItem.objects.get(pk=self.test_item_id)
+        location = StockLocation.objects.get(pk=self.test_location_id)
 
+        # Check that user is not allowed to change item
+        self.assertTrue(item.check_ownership(self.user))        # No owner -> True
+        self.assertTrue(location.check_ownership(self.user))    # No owner -> True
+        self.assertContains(self.assert_api_change(), 'You do not have permission to perform this action.', status_code=403)
 
-        # Logout
+        # Adjust group rules
-        self.client.logout()
+        group = Group.objects.get(name='my_test_group')
+        rule = group.rule_sets.get(name='stock')
+        rule.can_change = True
+        rule.save()
 
-        # Login with new user
+        # Set owner to group of user
-        self.client.login(username='john', password='custom123')
+        group_owner = Owner.get_owner(group)
+        item.owner = group_owner
+        item.save()
+        location.owner = group_owner
+        location.save()
 
-        # TODO: Refactor this following test to use the new API form
+        # Check that user is allowed to change item
-        # Test item edit
+        self.assertTrue(item.check_ownership(self.user))        # Owner is group -> True
-        response = self.client.post(reverse('stock-item-edit', args=(test_item_id,)),
+        self.assertTrue(location.check_ownership(self.user))    # Owner is group -> True
-                                    {'part': 1, 'status': StockStatus.OK, 'owner': new_user_as_owner.pk},
+        self.assertContains(self.assert_api_change(), f'"status":{StockStatus.DAMAGED}', status_code=200)
-                                    HTTP_X_REQUESTED_WITH='XMLHttpRequest')
 
-        # Make sure the item's owner is unchanged
+        # Change group
-        item = StockItem.objects.get(pk=test_item_id)
+        new_group = Group.objects.create(name='new_group')
-        self.assertEqual(item.owner, user_as_owner)
+        new_group_owner = Owner.get_owner(new_group)
+        item.owner = new_group_owner
+        item.save()
+        location.owner = new_group_owner
+        location.save()
 
-        # Create new parent location
+        # Check that user is not allowed to change item
-        parent_location = {
+        self.assertFalse(item.check_ownership(self.user))       # Owner is not in group -> False
-            'name': 'John Desk',
+        self.assertFalse(location.check_ownership(self.user))    # Owner is not in group -> False
-            'description': 'John\'s desk',
-            'owner': new_user_group_owner.pk,
-        }
-
-        # Retrieve created location
-        location_created = StockLocation.objects.get(name=new_location['name'])
-
-        # Create new item
-        new_item = {
-            'part': 25,
-            'location': location_created.pk,
-            'quantity': 123,
-            'status': StockStatus.OK,
-        }
-
-        # Try to create new item with no owner
-        response = self.client.post(reverse('stock-item-create'),
-                                    new_item, HTTP_X_REQUESTED_WITH='XMLHttpRequest')
-        self.assertContains(response, '"form_valid": false', status_code=200)
-
-        # Try to create new item with invalid owner
-        new_item['owner'] = user_as_owner.pk
-        response = self.client.post(reverse('stock-item-create'),
-                                    new_item, HTTP_X_REQUESTED_WITH='XMLHttpRequest')
-        self.assertContains(response, '"form_valid": false', status_code=200)
-
-        # Try to create new item with valid owner
-        new_item['owner'] = new_user_as_owner.pk
-        response = self.client.post(reverse('stock-item-create'),
-                                    new_item, HTTP_X_REQUESTED_WITH='XMLHttpRequest')
-        self.assertContains(response, '"form_valid": true', status_code=200)
-
-        # Logout
-        self.client.logout()
-    """

InvenTree/users/models.py

@@ -639,9 +639,9 @@ class Owner(models.Model):
                                 ContentType.objects.get_for_model(user_model).id]
 
         # If instance type is obvious: set content type
-        if type(user_or_group) is Group:
+        if isinstance(user_or_group, Group):
             content_type_id = content_type_id_list[0]
-        elif type(user_or_group) is get_user_model():
+        elif isinstance(user_or_group, get_user_model()):
             content_type_id = content_type_id_list[1]
 
         if content_type_id:
@@ -678,6 +678,12 @@ class Owner(models.Model):
 
         return related_owners
 
+    def is_user_allowed(self, user, include_group: bool = False):
+        """Check if user is allowed to access something owned by this owner."""
+
+        user_owner = Owner.get_owner(user)
+        return user_owner in self.get_related_owners(include_group=include_group)
+
 
 @receiver(post_save, sender=Group, dispatch_uid='create_owner')
 @receiver(post_save, sender=get_user_model(), dispatch_uid='create_owner')