Merge pull request RedHatInsights#1381 from RedHatInsights/rhcloud-36…

…814-empty-replication-no-principal

RHCLOUD-36814: Log when nothing to replicate for disabled user

rbac/management/relation_replicator/outbox_replicator.py

@@ -85,7 +85,10 @@ def _save_replication_event(
 
         if not payload["relations_to_add"] and not payload["relations_to_remove"]:
             logger.warning(
-                "[Dual Write] Skipping empty replication event. aggregateid='%s' event_type='%s' %s",
+                "[Dual Write] Skipping empty replication event. "
+                "An empty event is always a bug. "
+                "Calling code should avoid this and if not obvious, log why there is nothing to replicate. "
+                "aggregateid='%s' event_type='%s' %s",
                 aggregateid,
                 event_type,
                 logged_info,

rbac/management/relation_replicator/relation_replicator.py

@@ -48,6 +48,7 @@ class ReplicationEventType(str, Enum):
     BOOTSTRAP_TENANT = "bootstrap_tenant"
     BULK_BOOTSTRAP_TENANT = "bulk_bootstrap_tenant"
     EXTERNAL_USER_UPDATE = "external_user_update"
+    EXTERNAL_USER_DISABLE = "external_user_disable"
     BULK_EXTERNAL_USER_UPDATE = "bulk_external_user_update"
     MIGRATE_CUSTOM_ROLE = "migrate_custom_role"
     MIGRATE_TENANT_GROUPS = "migrate_tenant_groups"

rbac/management/tenant_service/v2.py

@@ -206,34 +206,52 @@ 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
+        mapping: Optional[TenantMapping] = None
+        principal: Optional[Principal] = None
 
         if user_id is None:
             raise ValueError(f"User {user.username} has no user_id.")
 
+        logger.info(
+            f"Removing Principal and group membership from RBAC and Relations. user_id={user_id} org_id={user.org_id}"
+        )
+
         try:
             mapping = TenantMapping.objects.filter(tenant__org_id=user.org_id).get()
-            tuples_to_remove.append(Group.relationship_to_user_id_for_group(str(mapping.default_group_uuid), user_id))
+            default_group_uuid = str(mapping.default_group_uuid)  # type: ignore
+            tuples_to_remove.append(Group.relationship_to_user_id_for_group(default_group_uuid, user_id))
         except TenantMapping.DoesNotExist:
-            pass
+            logger.info(
+                "No default membership to remove. There is no tenant mapping, so the tenant must not be bootstrapped."
+                f"org_id={user.org_id} user_id={user_id}"
+            )
 
         try:
             principal = Principal.objects.filter(username=user.username, tenant__org_id=user.org_id).get()
 
-            for group in principal.group.all():
+            for group in principal.group.all():  # type: ignore
                 group.principals.remove(principal)
                 # The user id might be None for the principal so we use user instead
                 tuple = group.relationship_to_principal(user)
                 if tuple is None:
-                    raise ValueError(f"relationship_to_principal is None for user {user.username}")
+                    raise ValueError(f"relationship_to_principal is None for user {user_id}")
 
-            principal.delete()
+            principal.delete()  # type: ignore
         except Principal.DoesNotExist:
-            pass
+            logger.info(f"Could not find Principal to remove. org_id={user.org_id} user_id={user_id}")
+
+        if not tuples_to_remove:
+            return
 
         self._replicator.replicate(
             ReplicationEvent(
-                event_type=ReplicationEventType.EXTERNAL_USER_UPDATE,
-                info={"user_id": user_id, "org_id": user.org_id},
+                event_type=ReplicationEventType.EXTERNAL_USER_DISABLE,
+                info={
+                    "user_id": user_id,
+                    "org_id": user.org_id,
+                    "mapping_id": mapping.id if mapping else None,
+                    "principal_id": principal.id if principal else None,
+                },
                 partition_key=PartitionKey.byEnvironment(),
                 remove=tuples_to_remove,
             )

tests/management/principal/test_cleaner.py

@@ -571,7 +571,7 @@ def setUp(self):
     @patch("management.principal.cleaner.UMB_CLIENT")
     @override_settings(PRINCIPAL_CLEANUP_UPDATE_ENABLED_UMB=False)
     def test_principal_creation_event_disabled(self, client_mock, proxy_mock):
-        """Test that we can run principal creation event."""
+        """Test that when update setting is disabled we do not add tenants for new, active users."""
         client_mock.canRead.side_effect = [True, False]
         client_mock.receiveFrame.return_value = MagicMock(body=FRAME_BODY_CREATION)
         Tenant.objects.get(org_id="17685860").delete()
@@ -603,7 +603,6 @@ def test_principal_creation_event_disabled(self, client_mock, proxy_mock):
     )
     @patch("management.principal.cleaner.UMB_CLIENT")
     def test_principal_creation_event_does_not_create_principal(self, client_mock, proxy_mock):
-        """Test that we can run principal creation event."""
         public_tenant = Tenant.objects.get(tenant_name="public")
         Group.objects.create(name="default", platform_default=True, tenant=public_tenant)
         client_mock.canRead.side_effect = [True, False]
@@ -816,6 +815,29 @@ def test_principal_creation_event_does_not_bootstrap_already_bootstraped_tenant(
                 ],
             )
 
+    @patch(
+        "management.principal.proxy.PrincipalProxy.request_filtered_principals",
+        return_value={
+            "status_code": 200,
+            "data": [],
+        },
+    )
+    @patch("management.principal.cleaner.UMB_CLIENT")
+    @patch("management.relation_replicator.outbox_replicator.OutboxReplicator.replicate")
+    def test_non_bootstrapped_tenant_no_principal_disabled_user_does_not_produce_replication_event(
+        self, replicate, client_mock, proxy_mock
+    ):
+        client_mock.canRead.side_effect = [True, False]
+        client_mock.receiveFrame.return_value = MagicMock(body=FRAME_BODY_CREATION)
+
+        process_principal_events_from_umb()
+
+        client_mock.receiveFrame.assert_called_once()
+        client_mock.disconnect.assert_called_once()
+        client_mock.ack.assert_called_once()
+
+        replicate.assert_not_called()
+
     def assertTenantBootstrappedByOrgId(self, org_id: str):
         tenant = Tenant.objects.get(org_id=org_id)
         self.assertIsNotNone(tenant)