From 03a14ebf397b0f9263dfe7a12f49f7a6c9ad420f Mon Sep 17 00:00:00 2001 From: Marnie0415 Date: Fri, 17 Jul 2026 03:40:28 +0800 Subject: [PATCH 1/5] fix: count actual existing owners in _validate_owner_removal validation When enforce_feature_owners is enabled, _validate_owner_removal was using the count of IDs provided in the request rather than the count of IDs that actually exist as owners. This caused false rejections when non-existent IDs were included in the group owner removal request. Changed the method to accept ID sets and filter against actual existing owners before counting. Added test cases covering the bug scenario. --- api/features/views.py | 36 +++++++--- .../unit/features/test_unit_features_views.py | 70 +++++++++++++++++++ 2 files changed, 96 insertions(+), 10 deletions(-) diff --git a/api/features/views.py b/api/features/views.py index e4d25e761a59..60bae9c8c506 100644 --- a/api/features/views.py +++ b/api/features/views.py @@ -433,8 +433,8 @@ def remove_group_owners(self, request, *args, **kwargs): # type: ignore[no-unty serializer.is_valid(raise_exception=True) self._validate_owner_removal( feature, - owners_to_remove=0, - group_owners_to_remove=len(serializer.validated_data["group_ids"]), + owner_ids=set(), + group_owner_ids=set(serializer.validated_data["group_ids"]), ) serializer.remove_group_owners(feature) response = Response(self.get_serializer(instance=feature).data) @@ -471,8 +471,8 @@ def remove_owners(self, request, *args, **kwargs): # type: ignore[no-untyped-de feature = self.get_object() self._validate_owner_removal( feature, - owners_to_remove=len(serializer.validated_data["user_ids"]), - group_owners_to_remove=0, + owner_ids=set(serializer.validated_data["user_ids"]), + group_owner_ids=set(), ) serializer.remove_users(feature) @@ -481,16 +481,32 @@ def remove_owners(self, request, *args, **kwargs): # type: ignore[no-untyped-de def _validate_owner_removal( self, feature: Feature, - owners_to_remove: int, - group_owners_to_remove: int, + owner_ids: set[int], + group_owner_ids: set[int], ) -> None: if not feature.project.enforce_feature_owners: return + + existing_owners = list(feature.owners.all()) + existing_group_owners = list(feature.group_owners.all()) + + existing_owner_ids = {o.id for o in existing_owners} + existing_group_owner_ids = {g.id for g in existing_group_owners} + + actual_owners_to_remove = ( + len(owner_ids & existing_owner_ids) if owner_ids else 0 + ) + actual_group_owners_to_remove = ( + len(group_owner_ids & existing_group_owner_ids) + if group_owner_ids + else 0 + ) + remaining = ( - feature.owners.count() - - owners_to_remove - + feature.group_owners.count() - - group_owners_to_remove + len(existing_owners) + - actual_owners_to_remove + + len(existing_group_owners) + - actual_group_owners_to_remove ) if remaining < 1: raise serializers.ValidationError( diff --git a/api/tests/unit/features/test_unit_features_views.py b/api/tests/unit/features/test_unit_features_views.py index 26b85fdbdc5d..f1a4afbd66db 100644 --- a/api/tests/unit/features/test_unit_features_views.py +++ b/api/tests/unit/features/test_unit_features_views.py @@ -5346,3 +5346,73 @@ def test_remove_group_owners__enforce_owners_user_owners_remain__returns_200( feature.refresh_from_db() assert group not in feature.group_owners.all() assert admin_user in feature.owners.all() + + +def test_remove_owners__enforce_owners_with_nonexistent_user_ids__still_blocks_when_needed( + admin_client_new: APIClient, + project: Project, + feature: Feature, + admin_user: FFAdminUser, +) -> None: + # Given — feature has only one owner, enforce_feature_owners is on + project.enforce_feature_owners = True + project.save() + # Create a real user who is NOT an owner of this feature + non_owner = FFAdminUser.objects.create_user(email="nonowner@example.com") # type: ignore[no-untyped-call] + feature.owners.add(admin_user) + + url = reverse( + "api-v1:projects:project-features-remove-owners", + args=[project.id, feature.id], + ) + # Request removes the real owner AND a user who is not an owner + data = {"user_ids": [admin_user.id, non_owner.id]} + + # When + response = admin_client_new.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then — should still block because the real owner would be removed + assert response.status_code == status.HTTP_400_BAD_REQUEST + feature.refresh_from_db() + assert admin_user in feature.owners.all() + + +def test_remove_group_owners__enforce_owners_with_nonexistent_group_ids__allows_when_real_owner_remains( + admin_client_new: APIClient, + project: Project, + feature: Feature, + admin_user: FFAdminUser, + organisation: Organisation, +) -> None: + # Given — feature has one user owner and one group owner + project.enforce_feature_owners = True + project.save() + group = UserPermissionGroup.objects.create( + name="Test Group", organisation=organisation + ) + # Create a real group that is NOT an owner of this feature + non_owner_group = UserPermissionGroup.objects.create( + name="Non Owner Group", organisation=organisation + ) + feature.owners.add(admin_user) + feature.group_owners.add(group) + + url = reverse( + "api-v1:projects:project-features-remove-group-owners", + args=[project.id, feature.id], + ) + # Request removes the real group and a group that is not an owner + data = {"group_ids": [group.id, non_owner_group.id]} + + # When + response = admin_client_new.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then — should allow because admin_user remains as owner + assert response.status_code == status.HTTP_200_OK + feature.refresh_from_db() + assert group not in feature.group_owners.all() + assert admin_user in feature.owners.all() From 8cbd1597cf2efdba08154702ddc684fdc733acba Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 16 Jul 2026 19:40:58 +0000 Subject: [PATCH 2/5] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- api/features/views.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/api/features/views.py b/api/features/views.py index 60bae9c8c506..f498d2e9acba 100644 --- a/api/features/views.py +++ b/api/features/views.py @@ -497,9 +497,7 @@ def _validate_owner_removal( len(owner_ids & existing_owner_ids) if owner_ids else 0 ) actual_group_owners_to_remove = ( - len(group_owner_ids & existing_group_owner_ids) - if group_owner_ids - else 0 + len(group_owner_ids & existing_group_owner_ids) if group_owner_ids else 0 ) remaining = ( From d08d9fb923dd8c06176a3e1e3ed12ed8fe2da9d6 Mon Sep 17 00:00:00 2001 From: Marnie0415 Date: Fri, 17 Jul 2026 08:15:56 +0800 Subject: [PATCH 3/5] fix(api): simplify _validate_owner_removal with set arithmetic Use values_list('id', flat=True) to fetch only IDs instead of full objects, and use set difference to check remaining owners instead of counting. Also update new tests to use inline URLs and format='json' per review feedback. --- api/features/views.py | 25 +++++----------- .../unit/features/test_unit_features_views.py | 30 ++++--------------- 2 files changed, 13 insertions(+), 42 deletions(-) diff --git a/api/features/views.py b/api/features/views.py index f498d2e9acba..dbd41f1a73ae 100644 --- a/api/features/views.py +++ b/api/features/views.py @@ -487,26 +487,15 @@ def _validate_owner_removal( if not feature.project.enforce_feature_owners: return - existing_owners = list(feature.owners.all()) - existing_group_owners = list(feature.group_owners.all()) - - existing_owner_ids = {o.id for o in existing_owners} - existing_group_owner_ids = {g.id for g in existing_group_owners} - - actual_owners_to_remove = ( - len(owner_ids & existing_owner_ids) if owner_ids else 0 - ) - actual_group_owners_to_remove = ( - len(group_owner_ids & existing_group_owner_ids) if group_owner_ids else 0 + existing_owner_ids = set(feature.owners.values_list("id", flat=True)) + existing_group_owner_ids = set( + feature.group_owners.values_list("id", flat=True) ) - remaining = ( - len(existing_owners) - - actual_owners_to_remove - + len(existing_group_owners) - - actual_group_owners_to_remove - ) - if remaining < 1: + if not ( + (existing_owner_ids - owner_ids) + or (existing_group_owner_ids - group_owner_ids) + ): raise serializers.ValidationError( "This project requires at least one owner or group owner per feature." ) diff --git a/api/tests/unit/features/test_unit_features_views.py b/api/tests/unit/features/test_unit_features_views.py index f1a4afbd66db..ca8e1e79324b 100644 --- a/api/tests/unit/features/test_unit_features_views.py +++ b/api/tests/unit/features/test_unit_features_views.py @@ -5354,26 +5354,17 @@ def test_remove_owners__enforce_owners_with_nonexistent_user_ids__still_blocks_w feature: Feature, admin_user: FFAdminUser, ) -> None: - # Given — feature has only one owner, enforce_feature_owners is on project.enforce_feature_owners = True project.save() - # Create a real user who is NOT an owner of this feature non_owner = FFAdminUser.objects.create_user(email="nonowner@example.com") # type: ignore[no-untyped-call] feature.owners.add(admin_user) - url = reverse( - "api-v1:projects:project-features-remove-owners", - args=[project.id, feature.id], - ) - # Request removes the real owner AND a user who is not an owner - data = {"user_ids": [admin_user.id, non_owner.id]} - - # When response = admin_client_new.post( - url, data=json.dumps(data), content_type="application/json" + f"/api/v1/projects/{project.id}/features/{feature.id}/remove-owners/", + data={"user_ids": [admin_user.id, non_owner.id]}, + format="json", ) - # Then — should still block because the real owner would be removed assert response.status_code == status.HTTP_400_BAD_REQUEST feature.refresh_from_db() assert admin_user in feature.owners.all() @@ -5386,32 +5377,23 @@ def test_remove_group_owners__enforce_owners_with_nonexistent_group_ids__allows_ admin_user: FFAdminUser, organisation: Organisation, ) -> None: - # Given — feature has one user owner and one group owner project.enforce_feature_owners = True project.save() group = UserPermissionGroup.objects.create( name="Test Group", organisation=organisation ) - # Create a real group that is NOT an owner of this feature non_owner_group = UserPermissionGroup.objects.create( name="Non Owner Group", organisation=organisation ) feature.owners.add(admin_user) feature.group_owners.add(group) - url = reverse( - "api-v1:projects:project-features-remove-group-owners", - args=[project.id, feature.id], - ) - # Request removes the real group and a group that is not an owner - data = {"group_ids": [group.id, non_owner_group.id]} - - # When response = admin_client_new.post( - url, data=json.dumps(data), content_type="application/json" + f"/api/v1/projects/{project.id}/features/{feature.id}/remove-group-owners/", + data={"group_ids": [group.id, non_owner_group.id]}, + format="json", ) - # Then — should allow because admin_user remains as owner assert response.status_code == status.HTTP_200_OK feature.refresh_from_db() assert group not in feature.group_owners.all() From 90b2727b93ebcfe1406efb4fa25cf87ad4e9cca7 Mon Sep 17 00:00:00 2001 From: Marnie0415 Date: Sat, 18 Jul 2026 05:43:33 +0800 Subject: [PATCH 4/5] fix(api): use prefetch cache to avoid additional database queries Use feature.owners.all() and feature.group_owners.all() instead of .values_list('id', flat=True) to leverage the prefetch cache and avoid triggering additional database queries. --- api/features/views.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/api/features/views.py b/api/features/views.py index dbd41f1a73ae..43497af9698d 100644 --- a/api/features/views.py +++ b/api/features/views.py @@ -487,10 +487,11 @@ def _validate_owner_removal( if not feature.project.enforce_feature_owners: return - existing_owner_ids = set(feature.owners.values_list("id", flat=True)) - existing_group_owner_ids = set( - feature.group_owners.values_list("id", flat=True) - ) + existing_owners = feature.owners.all() + existing_group_owners = feature.group_owners.all() + + existing_owner_ids = {owner.id for owner in existing_owners} + existing_group_owner_ids = {group.id for group in existing_group_owners} if not ( (existing_owner_ids - owner_ids) From 25679cb3f758226efe2685c681b7d6041d1bf45a Mon Sep 17 00:00:00 2001 From: Marnie0415 Date: Sat, 18 Jul 2026 05:48:40 +0800 Subject: [PATCH 5/5] fix(test): add bare GWT comments to new tests Add # Given / # When / # Then comments to the two new tests as requested by reviewer. These are bare (no explanation after # Then) to match the project's test conventions. --- api/tests/unit/features/test_unit_features_views.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/api/tests/unit/features/test_unit_features_views.py b/api/tests/unit/features/test_unit_features_views.py index ca8e1e79324b..cd2c981171b8 100644 --- a/api/tests/unit/features/test_unit_features_views.py +++ b/api/tests/unit/features/test_unit_features_views.py @@ -5354,17 +5354,20 @@ def test_remove_owners__enforce_owners_with_nonexistent_user_ids__still_blocks_w feature: Feature, admin_user: FFAdminUser, ) -> None: + # Given project.enforce_feature_owners = True project.save() non_owner = FFAdminUser.objects.create_user(email="nonowner@example.com") # type: ignore[no-untyped-call] feature.owners.add(admin_user) + # When response = admin_client_new.post( f"/api/v1/projects/{project.id}/features/{feature.id}/remove-owners/", data={"user_ids": [admin_user.id, non_owner.id]}, format="json", ) + # Then assert response.status_code == status.HTTP_400_BAD_REQUEST feature.refresh_from_db() assert admin_user in feature.owners.all() @@ -5377,6 +5380,7 @@ def test_remove_group_owners__enforce_owners_with_nonexistent_group_ids__allows_ admin_user: FFAdminUser, organisation: Organisation, ) -> None: + # Given project.enforce_feature_owners = True project.save() group = UserPermissionGroup.objects.create( @@ -5388,12 +5392,14 @@ def test_remove_group_owners__enforce_owners_with_nonexistent_group_ids__allows_ feature.owners.add(admin_user) feature.group_owners.add(group) + # When response = admin_client_new.post( f"/api/v1/projects/{project.id}/features/{feature.id}/remove-group-owners/", data={"group_ids": [group.id, non_owner_group.id]}, format="json", ) + # Then assert response.status_code == status.HTTP_200_OK feature.refresh_from_db() assert group not in feature.group_owners.all()