Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions rbac/management/notifications/notification_handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

from core.kafka import RBACProducer
from django.conf import settings
from management.role.v2_model import RoleV2

from api.models import Tenant

Expand Down Expand Up @@ -117,6 +118,29 @@ def role_obj_change_notification_handler(role_obj, operation, user=None):
notify(event_type, payload, org_id)


def custom_v2_role_obj_change_notification_handler(role_obj: RoleV2, operation: str, user):
"""Signal handler for setting notification message when RoleV2 object changes."""
if not settings.NOTIFICATIONS_ENABLED:
return

if role_obj.type != RoleV2.Types.CUSTOM:
raise ValueError("Notifications are supported only for custom V2 roles")

org_id = user.org_id
payload = payload_builder(user.username, role_obj)

event_type = {
"created": "custom-v2-role-created",
"deleted": "custom-v2-role-deleted",
"updated": "custom-v2-role-updated",
}.get(operation)

if event_type is None:
raise ValueError(f"Invalid operation: {operation!r}")

notify(event_type, payload, org_id)


def group_obj_change_notification_handler(user, group_obj, operation):
"""Signal handler for sending notification message when Group object changes."""
if not settings.NOTIFICATIONS_ENABLED:
Expand Down
18 changes: 18 additions & 0 deletions rbac/management/role/v2_view.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,12 @@
#
"""View for RoleV2 management."""

import logging

from management.atomic_transactions import atomic_block
from management.audit_log.model import AuditLog
from management.base_viewsets import BaseV2ViewSet
from management.notifications.notification_handlers import custom_v2_role_obj_change_notification_handler
from management.permissions.role_v2_access import RoleV2KesselAccessPermission
from management.permissions.v2_edit_api_access import V2WriteRequiresWorkspacesEnabled
from management.role.v2_exceptions import CustomRoleRequiredError, RolesNotFoundError
Expand Down Expand Up @@ -131,6 +134,8 @@ def perform_atomic_create(self, request, *args, **kwargs):
audit_log = AuditLog()
audit_log.log_create(request=request, resource=AuditLog.ROLE_V2)

custom_v2_role_obj_change_notification_handler(role, "created", request.user)

# Build response with field selection (from context) and permission ordering
input_permissions = request.data.get("permissions", [])
context = self.get_serializer_context()
Expand All @@ -150,6 +155,8 @@ def perform_atomic_update(self, request, *args, **kwargs):
serializer.is_valid(raise_exception=True)
role = serializer.save()

custom_v2_role_obj_change_notification_handler(role, "updated", request.user)

# Build response with field selection (from context) and permission ordering
input_permissions = request.data.get("permissions", [])
context = self.get_serializer_context()
Expand Down Expand Up @@ -192,4 +199,15 @@ def _perform_bulk_destroy(self, request, *args, **kwargs):
status=status.HTTP_500_INTERNAL_SERVER_ERROR,
)

# We don't allow a revert here because sending a notification in the middle could fail. This would make it
# unclear what to do with all of the earlier notifications that have already been sent.
for role in removed_roles:
try:
custom_v2_role_obj_change_notification_handler(role, "deleted", request.user)
except Exception:
logging.error(
f"Failed to send notification for deleted role: role pk={role.pk!r}, name={role.name!r}",
exc_info=True,
)

return Response(status=status.HTTP_204_NO_CONTENT)
80 changes: 77 additions & 3 deletions tests/management/role/test_v2_view.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
from importlib import reload
from unittest.mock import ANY, patch

from django.conf import settings
from django.test import override_settings
from django.urls import clear_url_caches, reverse
from django.utils.dateparse import parse_datetime
Expand All @@ -43,6 +44,7 @@
from rbac import urls
from tests.identity_request import IdentityRequest
from tests.v2_util import bootstrap_tenant_for_v2_test
from unittest.mock import ANY, patch

CACHE_PATCH_TARGET = "management.role.v2_service.permission_scope_cache"

Expand Down Expand Up @@ -1063,7 +1065,9 @@ def test_create_role_blocked_when_feature_flag_disabled(self, mock_is_v2_edit_en
self.assertIn("workspaces", str(response.data).lower())
mock_is_v2_edit_enabled.assert_called_once_with(self.customer_data["org_id"])

def test_create_role_success(self):
@override_settings(NOTIFICATIONS_ENABLED=True)
@patch("core.kafka.RBACProducer.send_kafka_message")
def test_create_role_success(self, send_kafka_message):
"""Test creating a role via API returns 201"""
data = {
"name": "API Test Role",
Expand All @@ -1085,6 +1089,28 @@ def test_create_role_success(self):

self._assert_audit_log(action=AuditLog.CREATE, description=f"Created V2 role: {data['name']}")

send_kafka_message.assert_called_with(
settings.NOTIFICATIONS_TOPIC,
{
"bundle": "console",
"application": "rbac",
"event_type": "custom-v2-role-created",
"timestamp": ANY,
"events": [
{
"metadata": {},
Comment on lines +1092 to +1101
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Consider using assert_called_once_with to also verify call count for the notification producer

Currently the test only checks the call arguments, not that send_kafka_message is called exactly once. Using send_kafka_message.assert_called_once_with(...) would also guard against accidental duplicate notifications; you can apply this to the create, update, and delete notification checks.

Suggested implementation:

        self._assert_audit_log(action=AuditLog.CREATE, description=f"Created V2 role: {data['name']}")

        send_kafka_message.assert_called_once_with(
            ANY,
        )

To fully apply your suggestion, similar changes should be made for the update and delete notification tests in this file:

  1. In test_update_role_success (and any other update-related tests using the same mock), replace send_kafka_message.assert_called_with(...) with send_kafka_message.assert_called_once_with(...).
  2. In the corresponding delete test(s) (e.g., test_delete_role_success or similar), also replace assert_called_with with assert_called_once_with for the send_kafka_message mock.
  3. Ensure all such tests that validate notification payloads also verify the call count via assert_called_once_with.

"payload": {
"name": data["name"],
"username": self.user_data["username"],
"uuid": response.data["id"],
},
}
],
"org_id": self.tenant.org_id,
},
ANY,
)

def test_create_role_multiple_permissions(self):
"""Test creating a role with multiple permissions returns all permissions."""
data = {
Expand Down Expand Up @@ -1406,7 +1432,9 @@ def test_create_role_rejects_migration_excluded_application(self):
# Tests for PUT /api/v2/roles/{uuid}/ (update)
# ==========================================================================

def test_update_role_success(self):
@override_settings(NOTIFICATIONS_ENABLED=True)
@patch("core.kafka.RBACProducer.send_kafka_message")
def test_update_role_success(self, send_kafka_message):
"""Test updating a role via API returns 200."""
role = CustomRoleV2.objects.create(
name="Original Role",
Expand Down Expand Up @@ -1440,6 +1468,28 @@ def test_update_role_success(self):
description=f"V2 role {role.name}:\nEdited name\nEdited description\nEdited permissions",
)

send_kafka_message.assert_called_with(
settings.NOTIFICATIONS_TOPIC,
{
"bundle": "console",
"application": "rbac",
"event_type": "custom-v2-role-updated",
"timestamp": ANY,
"events": [
{
"metadata": {},
"payload": {
"name": data["name"],
"username": self.user_data["username"],
"uuid": response.data["id"],
},
}
],
"org_id": self.tenant.org_id,
},
ANY,
)

def test_update_role_changes_permissions(self):
"""Test that updating a role replaces all permissions."""
role = CustomRoleV2.objects.create(
Expand Down Expand Up @@ -1854,7 +1904,9 @@ def _assert_delete_not_found(self, response, uuids: Iterable[str | uuid.UUID]):
self.assertIn("errors", data)
self.assertEqual(data["errors"], [{"message": data["detail"], "field": "ids"}])

def test_delete(self):
@override_settings(NOTIFICATIONS_ENABLED=True)
@patch("core.kafka.RBACProducer.send_kafka_message")
def test_delete(self, send_kafka_message):
create_response = self._create_role()
response = self._request_delete({"ids": [create_response["id"]]})

Expand All @@ -1863,6 +1915,28 @@ def test_delete(self):

self._assert_audit_log(action=AuditLog.DELETE, description=f"Deleted V2 role: {create_response['name']}")

send_kafka_message.assert_called_with(
settings.NOTIFICATIONS_TOPIC,
{
"bundle": "console",
"application": "rbac",
"event_type": "custom-v2-role-deleted",
"timestamp": ANY,
"events": [
{
"metadata": {},
"payload": {
"name": create_response["name"],
"username": self.user_data["username"],
"uuid": create_response["id"],
},
}
],
"org_id": self.tenant.org_id,
},
ANY,
)

def test_delete_empty(self):
"""Test that deleting 0 roles is successful."""
response = self._request_delete({"ids": []})
Expand Down
Loading