Address comments

Remove filter of binding.
Fix unit test failure

rbac/api/utils.py

@@ -1,15 +1,16 @@
 """Helper utilities for api module."""
 
 import logging
+
 from django.db import transaction
 from django.shortcuts import get_object_or_404
-from management.utils import account_id_for_tenant
-
-from api.models import Tenant
 from management.role.model import BindingMapping
 from management.tenant_mapping.model import TenantMapping
+from management.utils import account_id_for_tenant
 from management.workspace.model import Workspace
 
+from api.models import Tenant
+
 
 RESOURCE_MODEL_MAPPING = {
     "workspace": Workspace,
@@ -18,6 +19,7 @@
 }
 logger = logging.getLogger(__name__)
 
+
 def populate_tenant_account_id():
     """Populate tenant's account id's."""
     tenants = Tenant.objects.filter(account_id__isnull=True).exclude(tenant_name="public")
@@ -27,22 +29,23 @@ def populate_tenant_account_id():
             tenant.save()
 
 
-def migration_resource_deletion(resource, org_id):
-    resource_objs = RESOURCE_MODEL_MAPPING[resource].objects.all()
-    if org_id:
+def get_resources(resource, org_id):
+    """Get queryset by org_id."""
+    queryset = RESOURCE_MODEL_MAPPING[resource].objects.all()
+    if resource != "binding" and org_id:
         tenant = get_object_or_404(Tenant, org_id=org_id)
-        if resource == "binding":
-            resource_objs = resource_objs.filter(role__tenant=tenant)
-        else:
-            resource_objs = resource_objs.filter(tenant=tenant)
-    else:
-        if resource == "binding":
-            public_tenant = Tenant.objects.get(tenant_name="public")
-            resource_objs = resource_objs.exclude(role__tenant=public_tenant)
+        queryset = queryset.filter(tenant=tenant)
+    return queryset
+
+
+def migration_resource_deletion(resource, org_id):
+    """Delete migration related resources."""
+    resource_objs = get_resources(resource, org_id)
+
     if resource == "workspace":
         # Have to delete the ones without children first or deletion will fail
         logger.info("Deleting workspaces without children.")
-        while(resource_objs.filter(children=None).exists()):
+        while resource_objs.filter(children=None).exists():
             resource_objs.filter(children=None).delete()
         logger.info("All workspaces without children removed.")
     resource_objs.delete()

rbac/internal/views.py

@@ -52,7 +52,7 @@
 from api.common.pagination import StandardResultsSetPagination
 from api.models import Tenant
 from api.tasks import cross_account_cleanup, populate_tenant_account_id_in_worker, run_migration_resource_deletion
-from api.utils import RESOURCE_MODEL_MAPPING
+from api.utils import RESOURCE_MODEL_MAPPING, get_resources
 
 logger = logging.getLogger(__name__)
 TENANTS = TenantCache()
@@ -507,9 +507,11 @@ class SentryDiagnosticError(Exception):
 
 def migration_resources(request):
     """View or delete specific resources related to migration.
+
     DELETE /_private/api/utils/migration_resources/?resource=xxx&org_id=xxx
     GET /_private/api/utils/migration_resources/?resource=xxx&org_id=xxx&limit=1000
     optinos of resource: workspace, mapping(tenantmapping), binding(bindingmapping)
+    org_id does not work for bindingmapping
     """
     resource = request.GET.get("resource")
     if not resource:
@@ -518,13 +520,12 @@ def migration_resources(request):
             status=400,
         )
     resource = resource.lower()
-    resource_model = RESOURCE_MODEL_MAPPING.get(resource)
-    if not resource_model:
+    if resource not in RESOURCE_MODEL_MAPPING:
         return HttpResponse(
             f"Invalid request, resource should be in '{RESOURCE_MODEL_MAPPING.keys()}'.",
             status=400,
         )
-    
+
     org_id = request.GET.get("org_id")
 
     if request.method == "DELETE":
@@ -533,17 +534,8 @@ def migration_resources(request):
         run_migration_resource_deletion.delay({"resource": resource, "org_id": org_id})
         logger.info(f"Deleting resources of type {resource}. Requested by '{request.user.username}'")
         return HttpResponse("Resource deletion is running in a background worker.", status=202)
-
     elif request.method == "GET":
-        resource_objs = resource_model.objects.all()
-        if org_id:
-            tenant = get_object_or_404(Tenant, org_id=org_id)
-            if tenant.tenant_name == "public":
-                return HttpResponse("Invalid request, tenant cannot be the public one.", status=400)
-            if resource == "binding":
-                resource_objs = resource_objs.filter(role__tenant=tenant)
-            else:
-                resource_objs = resource_objs.filter(tenant=tenant)
+        resource_objs = get_resources(resource, org_id)
         limit = request.GET.get("limit", 1000)
         resource_list = [str(resource_obj.id) for resource_obj in resource_objs[:limit]]
         return HttpResponse(json.dumps(resource_list), content_type="application/json", status=200)

tests/api/test_utils.py

@@ -1,4 +1,3 @@
-
 from django.test import TestCase
 
 
@@ -9,7 +8,6 @@
 
 
 class TestAPIUtils(TestCase):
-
     def test_migration_resource_deletion(self):
         org_id_1 = "12345678"
         org_id_2 = "87654321"
@@ -63,7 +61,7 @@ def test_migration_resource_deletion(self):
             tenant=another_tenant,
             parent=root,
         )
-        
+
         migration_resource_deletion("workspace", None)
         self.assertFalse(Workspace.objects.exists())
 
@@ -104,11 +102,11 @@ def test_migration_resource_deletion(self):
         BindingMapping.objects.create(
             role=another_role,
         )
-        system_role_binding = BindingMapping.objects.create(
+        BindingMapping.objects.create(
             role=system_role,
         )
         migration_resource_deletion("binding", org_id_2)
         self.assertFalse(BindingMapping.objects.filter(role=another_role).exists())
 
         migration_resource_deletion("binding", None)
-        self.assertEqual(list(BindingMapping.objects.values_list("id", flat=True)), [system_role_binding.id])
+        self.assertFalse(BindingMapping.objects.exists())

tests/internal/test_views.py

@@ -574,13 +574,14 @@ def test_listing_migration_resources(self):
             id_set.add(str(workspace.id))
         self.assertEqual(set(json.loads(response.getvalue())), id_set)
 
-
         response = self.client.get(
             f"/_private/api/utils/migration_resources/?resource=workspace&org_id={org_id}",
             **self.request.META,
         )
         self.assertEqual(response.status_code, status.HTTP_200_OK)
-        self.assertEqual(set(json.loads(response.getvalue())), {str(default_workspaces[1].id), str(root_workspaces[1].id)})
+        self.assertEqual(
+            set(json.loads(response.getvalue())), {str(default_workspaces[1].id), str(root_workspaces[1].id)}
+        )
 
         # Listing tenantmappings
         tenant_mappings = TenantMapping.objects.bulk_create(
@@ -598,15 +599,16 @@ def test_listing_migration_resources(self):
             **self.request.META,
         )
         self.assertEqual(response.status_code, status.HTTP_200_OK)
-        self.assertEqual(set(json.loads(response.getvalue())), {str(tenant_mapping.id) for tenant_mapping in tenant_mappings})
+        self.assertEqual(
+            set(json.loads(response.getvalue())), {str(tenant_mapping.id) for tenant_mapping in tenant_mappings}
+        )
         response = self.client.get(
             f"/_private/api/utils/migration_resources/?resource=mapping&org_id={org_id}",
             **self.request.META,
         )
         self.assertEqual(json.loads(response.getvalue()), [str(tenant_mappings[1].id)])
 
         # List bindingmappings
-        # Delete bindingmappings
         tenant_role = Role.objects.create(
             name="role",
             tenant=self.tenant,
@@ -622,18 +624,12 @@ def test_listing_migration_resources(self):
             role=another_role,
         )
         response = self.client.get(
-            "/_private/api/utils/migration_resources/?resource=mapping",
+            "/_private/api/utils/migration_resources/?resource=binding",
             **self.request.META,
         )
         self.assertEqual(response.status_code, status.HTTP_200_OK)
         self.assertEqual(set(json.loads(response.getvalue())), {str(binding.id), str(another_binding.id)})
 
-        response = self.client.get(
-            f"/_private/api/utils/migration_resources/?resource=mapping&org_id={org_id}",
-            **self.request.META,
-        )
-        self.assertEqual(json.loads(response.getvalue()), [str(another_binding.id)])
-
     @override_settings(INTERNAL_DESTRUCTIVE_API_OK_UNTIL=valid_destructive_time())
     @patch("api.tasks.run_migration_resource_deletion.delay")
     def test_deleting_migration_resources(self, delay_mock):