Use user_id in case it is not available from principal

rbac/management/principal/cleaner.py

@@ -31,6 +31,7 @@
 from management.tenant_service.tenant_service import TenantBootstrapService
 from prometheus_client import Counter
 from rest_framework import status
+from sentry_sdk import capture_exception
 from stompest.config import StompConfig
 from stompest.error import StompConnectionError
 from stompest.protocol import StompSpec
@@ -212,7 +213,14 @@ def process_umb_event(frame, umb_client: Stomp, bootstrap_service: TenantBootstr
             # If the setting is enabled, process all users.
             if not user.is_active or settings.PRINCIPAL_CLEANUP_UPDATE_ENABLED_UMB:
                 # If Tenant is not already ready, don't ready it
-                bootstrap_service.update_user(user, ready_tenant=False)
+                try:
+                    bootstrap_service.update_user(user, ready_tenant=False)
+                except ValueError as e:
+                    logger.error("process_umb_event: Error updating user: %s", str(e))
+                    capture_exception(e)
+                    umb_client.nack(frame)
+                    umb_message_processed_count.inc()
+                    return True
         else:
             # Message is malformed.
             # Ensure we dont block the entire queue by discarding it.

rbac/management/tenant_service/v2.py

@@ -206,6 +206,7 @@ def _disable_user_in_tenant(self, user: User):
         # Get tenant mapping if present but no need to create if not
         tuples_to_remove = []
         user_id = user.user_id
+        error_message = None
 
         if user_id is None:
             raise ValueError(f"User {user.username} has no user_id.")
@@ -221,7 +222,11 @@ def _disable_user_in_tenant(self, user: User):
 
             for group in principal.group.all():
                 group.principals.remove(principal)
-                tuples_to_remove.append(group.relationship_to_principal(principal))
+                tupple = group.relationship_to_principal(principal)
+                if tupple is None:
+                    error_message = f"principal {principal.username} has no user id."
+                else:
+                    tuples_to_remove.append(tupple)
 
             principal.delete()
         except Principal.DoesNotExist:
@@ -235,6 +240,8 @@ def _disable_user_in_tenant(self, user: User):
                 remove=tuples_to_remove,
             )
         )
+        if error_message:
+            raise ValueError(error_message)
 
     def _get_or_bootstrap_tenant(
         self, org_id: str, ready: bool, account_number: Optional[str] = None

tests/management/principal/test_cleaner.py

@@ -890,6 +890,62 @@ def assertTenantBootstrappedByOrgId(self, org_id: str):
             ),
         )
 
+    @patch(
+        "management.principal.proxy.PrincipalProxy._request_principals",
+        return_value={
+            "status_code": status.HTTP_200_OK,
+            "data": [],
+        },
+    )
+    @patch("management.group.model.AccessCache")
+    @patch("management.principal.cleaner.UMB_CLIENT")
+    def test_cleanup_disabled_principal_in_or_not_in_group(self, client_mock, cache_class, proxy_mock):
+        """Run a principal clean up on a tenant with a principal either in or not in a group."""
+        principal_name = "principal-test"
+        self.principal = Principal(username=principal_name, tenant=self.tenant, user_id="56780000")
+        self.principal.save()
+        self.group.principals.add(self.principal)
+        self.group.save()
+
+        before = REGISTRY.get_sample_value(METRIC_STOMP_MESSAGE_TOTAL)
+        client_mock.canRead.side_effect = [True, False]
+        client_mock.receiveFrame.return_value = MagicMock(body=FRAME_BODY)
+        cache_mock = MagicMock()
+        cache_class.return_value = cache_mock
+        process_principal_events_from_umb()
+
+        after = REGISTRY.get_sample_value(METRIC_STOMP_MESSAGE_TOTAL)
+        client_mock.receiveFrame.assert_called_once()
+        client_mock.disconnect.assert_called_once()
+        client_mock.ack.assert_called_once()
+        self.assertFalse(Principal.objects.filter(username=principal_name).exists())
+        self.group.refresh_from_db()
+        self.assertFalse(self.group.principals.all())
+        cache_mock.delete_policy.assert_called_once_with(self.principal.uuid)
+        self.assertTrue(before + 1 == after)
+
+        # when principal does not have user id set
+        principal = Principal.objects.create(username=principal_name, tenant=self.tenant)
+        self.group.principals.add(principal)
+        self.group.save()
+        client_mock.canRead.side_effect = [True, False]
+        client_mock.ack.reset_mock()
+
+        process_principal_events_from_umb()
+
+        client_mock.nack.assert_called_once()
+        self.assertFalse(Principal.objects.filter(username=principal_name).exists())
+        client_mock.ack.assert_not_called()
+
+        # When principal not in group
+        self.principal = Principal(username=principal_name, tenant=self.tenant, user_id="56780000")
+        self.principal.save()
+        client_mock.canRead.side_effect = [True, False]
+        client_mock.ack.reset_mock()
+        process_principal_events_from_umb()
+        self.assertFalse(Principal.objects.filter(username=principal_name).exists())
+        client_mock.ack.assert_called_once()
+
 
 @override_settings(V2_BOOTSTRAP_TENANT=False)
 class PrincipalUMBTestsWithV1TenantBootstrap(IdentityRequest):