Fix an issue where a view scope that depended on a nullable foreign key would trigger a SQL error when doing a PUT / Delete request

Author: Stefan Majoor

binder/views.py

@@ -2717,7 +2717,13 @@ def put(self, request, pk=None):
 		values = self._get_request_values(request)
 
 		try:
-			obj = self.get_queryset(request).select_for_update().get(pk=int(pk))
+			# Step one does the permission check. We cannot do a select for update here, since the queryeset
+			# of get_queryset can potentially have outer joins on nullable values
+			obj = self.get_queryset(request).get(pk=int(pk))
+
+			# Now that we know we have access to this moel, we can get it again, this time with lock.
+			obj = self.model.objects.select_for_update().get(pk=obj.pk)
+
 			# Permission checks are done at this point, so we can avoid get_queryset()
 			include_annotations = self._parse_include_annotations(request)
 			annotations = include_annotations.get('')
@@ -2799,10 +2805,14 @@ def delete(self, request, pk=None, undelete=False, skip_body_check=False):
 			except ValueError:
 				pass
 
+		# This make sure we do all permission checks. We cannot do a select for update here, since there is a possiblity
+		# that we create a queryset that we cannot use in a for select clause.
 		try:
-			obj = self.get_queryset(request).select_for_update().get(pk=int(pk))
+			obj = self.get_queryset(request).get(pk=int(pk))
 		except ObjectDoesNotExist:
 			raise BinderNotFound()
+		# We now retrieve the model again, without the permission checks, which we already this.
+		obj = self.model.objects.select_for_update().get(pk=obj.pk)
 
 		self.delete_obj(obj, undelete, request)
 		logger.info('{}DELETEd {} #{}'.format('UN' if undelete else '', self._model_name(), pk))

tests/test_permission_view.py

@@ -6,7 +6,7 @@
 from django.db.models import Q
 
 from binder.json import jsonloads, jsondumps
-from binder.permissions.views import is_q_stricter, smart_q_or
+from binder.permissions.views import is_q_stricter, smart_q_or, PermissionView
 
 from .testapp.models import Zoo, ZooEmployee, Country, City, PermanentCity, CityState, Animal
 from .testapp.urls import router
@@ -804,3 +804,60 @@ def test_pk_not_in_empty_list_top_level_or(self):
 
 		combined = smart_q_or(pk_not_in_empty_list | foo)
 		self.assertEqual(combined, pk_not_in_empty_list)
+
+class ForUpdateErrorNotOccurringTest(TestCase):
+	"""
+	Previously there was a bug where if you did a delete on an object in a permisionview, while at the same time you
+	also had a view scope that depended on a nullable field, then a SQL error would occur during the deletion:
+
+
+	"FOR UPDATE cannot be applied to the nullable side of an outer join"
+
+	"""
+	def setUp(self):
+		super().setUp()
+
+		group = Group.objects.get()
+
+		u = User(username='testuser', is_active=True, is_superuser=False)
+		u.set_password('test')
+		u.save()
+		u.groups.add(group)
+
+		self.client = Client()
+		r = self.client.login(username='testuser', password='test')
+		self.assertTrue(r)
+
+	@override_settings(BINDER_PERMISSION={
+		'testapp.view_country': [
+			('testapp.view_country', 'all'),
+			('testapp.view_citystate', 'netherlands'),
+			('testapp.delete_citystate', 'all'),
+		],
+	})
+	def test_for_update_bug_not_occurs_on_deletion(self):
+		country = Country.objects.create(name='Netherlands')
+		city_state = CityState.objects.create(name='Luxembourg', country=country)
+
+		res = self.client.delete(f'/city_state/{city_state.id}/')
+
+		self.assertEqual(204, res.status_code)
+
+	@override_settings(BINDER_PERMISSION={
+		'testapp.view_country': [
+			('testapp.view_country', 'all'),
+			('testapp.view_citystate', 'netherlands'),
+			('testapp.delete_citystate', 'all'),
+			('testapp.change_citystate', 'all'),
+		],
+	})
+	def test_for_update_bug_not_occurs_on_put(self):
+		# Same bug occurred with a put request
+		country = Country.objects.create(name='Netherlands')
+		city_state = CityState.objects.create(name='Luxembourg', country=country)
+
+		res = self.client.put(f'/city_state/{city_state.id}/', data=jsondumps({
+		}))
+		print(res.json())
+
+		self.assertEqual(200, res.status_code)

tests/testapp/views/city.py

@@ -1,14 +1,23 @@
+from django.db.models import Q
+
 from binder.permissions.views import PermissionView
 from ..models.city import CityState, City, PermanentCity
 
 
 class CityView(PermissionView):
-    model = City
+	model = City
 
 
 class CityStateView(PermissionView):
-    model = CityState
+	model = CityState
+
+	def _scope_view_netherlands(self, request):
+		"""
+		This is a bit of a weird edge case, but using this type of Q object creates an outer join with
+		nullable side, thus preventhing us from doing a "select FOR UPDATE" clause
+		"""
+		return Q(country__name='Netherlands') | Q(name='Luxembourg')
 
 
 class PermanentCityView(PermissionView):
-    model = PermanentCity
+	model = PermanentCity