From 00d5ae0dfe660671c85eae26960f8dc21d2e3936 Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Fri, 21 Nov 2025 00:39:19 -0600 Subject: [PATCH 01/31] refactor(clients): make some changes to how clients work s/hostname/client_name because it was confusing. make UUID a unique field. add the ability to check that a client is bound to a specific IP. --- scram/route_manager/api/serializers.py | 3 ++- .../migrations/0035_alter_client_uuid.py | 19 +++++++++++++++++++ ...0036_rename_hostname_client_client_name.py | 18 ++++++++++++++++++ .../0037_client_registered_from_ip.py | 18 ++++++++++++++++++ .../migrations/0038_unique_client_uuid.py | 19 +++++++++++++++++++ scram/route_manager/models.py | 9 +++++---- .../tests/acceptance/steps/common.py | 10 +++++----- .../tests/acceptance/steps/ip.py | 2 +- scram/route_manager/tests/test_api.py | 2 +- .../route_manager/tests/test_authorization.py | 4 ++-- scram/route_manager/tests/test_websockets.py | 2 +- 11 files changed, 91 insertions(+), 15 deletions(-) create mode 100644 scram/route_manager/migrations/0035_alter_client_uuid.py create mode 100644 scram/route_manager/migrations/0036_rename_hostname_client_client_name.py create mode 100644 scram/route_manager/migrations/0037_client_registered_from_ip.py create mode 100644 scram/route_manager/migrations/0038_unique_client_uuid.py diff --git a/scram/route_manager/api/serializers.py b/scram/route_manager/api/serializers.py index 5c790f54..f47e9416 100644 --- a/scram/route_manager/api/serializers.py +++ b/scram/route_manager/api/serializers.py @@ -49,7 +49,8 @@ class Meta: """Maps to the Client model, and specifies the fields exposed by the API.""" model = Client - fields = ["hostname", "uuid"] + fields = ["client_name", "uuid", "registered_from_ip"] + read_only_fields = ["registered_from_ip"] class EntrySerializer(serializers.HyperlinkedModelSerializer): diff --git a/scram/route_manager/migrations/0035_alter_client_uuid.py b/scram/route_manager/migrations/0035_alter_client_uuid.py new file mode 100644 index 00000000..39ff3bab --- /dev/null +++ b/scram/route_manager/migrations/0035_alter_client_uuid.py @@ -0,0 +1,19 @@ +# Generated by Django 4.2.26 on 2025-11-21 05:52 + +from django.db import migrations, models +import uuid + + +class Migration(migrations.Migration): + + dependencies = [ + ("route_manager", "0034_alter_entry_originating_scram_instance_and_more"), + ] + + operations = [ + migrations.AlterField( + model_name="client", + name="uuid", + field=models.UUIDField(default=uuid.uuid4, editable=False), + ), + ] diff --git a/scram/route_manager/migrations/0036_rename_hostname_client_client_name.py b/scram/route_manager/migrations/0036_rename_hostname_client_client_name.py new file mode 100644 index 00000000..1beafff8 --- /dev/null +++ b/scram/route_manager/migrations/0036_rename_hostname_client_client_name.py @@ -0,0 +1,18 @@ +# Generated by Django 4.2.26 on 2025-11-21 06:03 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("route_manager", "0035_alter_client_uuid"), + ] + + operations = [ + migrations.RenameField( + model_name="client", + old_name="hostname", + new_name="client_name", + ), + ] diff --git a/scram/route_manager/migrations/0037_client_registered_from_ip.py b/scram/route_manager/migrations/0037_client_registered_from_ip.py new file mode 100644 index 00000000..6a734313 --- /dev/null +++ b/scram/route_manager/migrations/0037_client_registered_from_ip.py @@ -0,0 +1,18 @@ +# Generated by Django 4.2.26 on 2025-11-21 06:08 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("route_manager", "0036_rename_hostname_client_client_name"), + ] + + operations = [ + migrations.AddField( + model_name="client", + name="registered_from_ip", + field=models.GenericIPAddressField(blank=True, null=True), + ), + ] diff --git a/scram/route_manager/migrations/0038_unique_client_uuid.py b/scram/route_manager/migrations/0038_unique_client_uuid.py new file mode 100644 index 00000000..7a38ec5e --- /dev/null +++ b/scram/route_manager/migrations/0038_unique_client_uuid.py @@ -0,0 +1,19 @@ +# Generated by Django 4.2.26 on 2025-11-21 06:17 + +from django.db import migrations, models +import uuid + + +class Migration(migrations.Migration): + + dependencies = [ + ("route_manager", "0037_client_registered_from_ip"), + ] + + operations = [ + migrations.AlterField( + model_name="client", + name="uuid", + field=models.UUIDField(default=uuid.uuid4, editable=False, unique=True), + ), + ] diff --git a/scram/route_manager/models.py b/scram/route_manager/models.py index 2f169cbd..3b5268ec 100644 --- a/scram/route_manager/models.py +++ b/scram/route_manager/models.py @@ -169,15 +169,16 @@ def __str__(self): class Client(models.Model): """Any client that would like to hit the API to add entries (e.g. Zeek).""" - hostname = models.CharField(max_length=50, unique=True) - uuid = models.UUIDField() + client_name = models.CharField(max_length=50, unique=True) + uuid = models.UUIDField(default=uuid_lib.uuid4, editable=False, unique=True) + registered_from_ip = models.GenericIPAddressField(null=True, blank=True) is_authorized = models.BooleanField(null=True, blank=True, default=False) authorized_actiontypes = models.ManyToManyField(ActionType) def __str__(self): - """Only display the hostname.""" - return str(self.hostname) + """Only display the client_name.""" + return str(self.client_name) channel_layer = get_channel_layer() diff --git a/scram/route_manager/tests/acceptance/steps/common.py b/scram/route_manager/tests/acceptance/steps/common.py index cec0692d..9436e609 100644 --- a/scram/route_manager/tests/acceptance/steps/common.py +++ b/scram/route_manager/tests/acceptance/steps/common.py @@ -38,7 +38,7 @@ def create_authed_client(context, name): """Create a client and authorize it for that action type.""" at, _ = ActionType.objects.get_or_create(name=name) authorized_client = Client.objects.create( - hostname="authorized_client.es.net", + client_name="authorized_client.es.net", uuid="0e7e1cbd-7d73-4968-bc4b-ce3265dc2fd3", is_authorized=True, ) @@ -50,7 +50,7 @@ def create_authed_client(context, name): def create_unauthed_client(context, name): """Create a client that has no authorized action types.""" unauthorized_client = Client.objects.create( - hostname="unauthorized_client.es.net", + client_name="unauthorized_client.es.net", uuid="91e134a5-77cf-4560-9797-6bbdbffde9f8", ) unauthorized_client.authorized_actiontypes.set([]) @@ -214,13 +214,13 @@ def check_object(context, value, model): context.test.assertTrue(found) -@when("we register a client named {hostname} with the uuid of {uuid}") -def add_client(context, hostname, uuid): +@when("we register a client named {client_name} with the uuid of {uuid}") +def add_client(context, client_name, uuid): """Create a client with a specific UUID.""" context.response = context.test.client.post( reverse("api:v1:client-list"), { - "hostname": hostname, + "client_name": client_name, "uuid": uuid, }, ) diff --git a/scram/route_manager/tests/acceptance/steps/ip.py b/scram/route_manager/tests/acceptance/steps/ip.py index 5492f96f..a0f3bc07 100644 --- a/scram/route_manager/tests/acceptance/steps/ip.py +++ b/scram/route_manager/tests/acceptance/steps/ip.py @@ -54,7 +54,7 @@ def check_comment(context, value, comment): @then("we update the entry {value:S} with comment {comment}") def update_entry_comment(context, value, comment): """Update the entry with a new comment.""" - data = {"comment": comment, "who": context.client.hostname} + data = {"comment": comment, "who": context.client.client_name} context.response = context.test.client.put( reverse("api:v1:entry-detail", args=[value]), data=json.dumps(data), content_type="application/json" diff --git a/scram/route_manager/tests/test_api.py b/scram/route_manager/tests/test_api.py index 1a81e907..7995f14b 100644 --- a/scram/route_manager/tests/test_api.py +++ b/scram/route_manager/tests/test_api.py @@ -17,7 +17,7 @@ def setUp(self): self.superuser = get_user_model().objects.create_superuser("admin", "admin@es.net", "admintestpassword") self.client.login(username="admin", password="admintestpassword") self.authorized_client = Client.objects.create( - hostname="authorized_client.es.net", + client_name="authorized_client.es.net", uuid="0e7e1cbd-7d73-4968-bc4b-ce3265dc2fd3", is_authorized=True, ) diff --git a/scram/route_manager/tests/test_authorization.py b/scram/route_manager/tests/test_authorization.py index 596455c3..532c5c14 100644 --- a/scram/route_manager/tests/test_authorization.py +++ b/scram/route_manager/tests/test_authorization.py @@ -42,14 +42,14 @@ def setUp(self): ] self.authorized_client = ClientModel.objects.create( - hostname="authorized_client.es.net", + client_name="authorized_client.es.net", uuid="0e7e1cbd-7d73-4968-bc4b-ce3265dc2fd3", is_authorized=True, ) self.authorized_client.authorized_actiontypes.set([1]) self.unauthorized_client = ClientModel.objects.create( - hostname="unauthorized_client.es.net", + client_name="unauthorized_client.es.net", uuid="91e134a5-77cf-4560-9797-6bbdbffde9f8", ) diff --git a/scram/route_manager/tests/test_websockets.py b/scram/route_manager/tests/test_websockets.py index ca9cd528..39d921b3 100644 --- a/scram/route_manager/tests/test_websockets.py +++ b/scram/route_manager/tests/test_websockets.py @@ -61,7 +61,7 @@ def setUp(self): self.actiontype.save() self.authorized_client = Client.objects.create( - hostname="authorized_client.example.net", + client_name="authorized_client.example.net", uuid=self.uuid, is_authorized=True, ) From 77ce3fe4e8573bb91fc9466a62c15953dc305cda Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Fri, 21 Nov 2025 10:30:49 -0600 Subject: [PATCH 02/31] feat(client-admin): show the UUID with the client object in the admin page --- scram/route_manager/admin.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/scram/route_manager/admin.py b/scram/route_manager/admin.py index 00d267b3..16edc225 100644 --- a/scram/route_manager/admin.py +++ b/scram/route_manager/admin.py @@ -51,9 +51,13 @@ class EntryAdmin(SimpleHistoryAdmin): ] search_fields = ["route", "comment"] +@admin.register(Client) +class ClientAdmin(admin.ModelAdmin): + list_display = ('client_name', 'uuid', 'registered_from_ip') + readonly_fields = ('uuid',) + admin.site.register(IgnoreEntry, SimpleHistoryAdmin) admin.site.register(Route) -admin.site.register(Client) admin.site.register(WebSocketMessage) admin.site.register(WebSocketSequenceElement) From 6182b89ebbee8e199c519b3a2b6b6200483eecd9 Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Fri, 21 Nov 2025 10:48:26 -0600 Subject: [PATCH 03/31] feat(client-IP): grab the client IP for further authentication There's probably a better way to do this. Definitely needs a look and think --- scram/shared/shared_code.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/scram/shared/shared_code.py b/scram/shared/shared_code.py index 926d7bb7..32f74e7d 100644 --- a/scram/shared/shared_code.py +++ b/scram/shared/shared_code.py @@ -57,3 +57,16 @@ def make_random_password(length: int = 20, min_digits: int = 5, max_attempts: in # required password length being very long and the min_digits being high. message = f"Failed to generate a valid password after {max_attempts} attempts" raise RuntimeError(message) + + +def get_client_ip(request) -> str | None: + """Get the client's IP address from the request. + + Checks the HTTP_X_FORWARDED_FOR header first, then falls back to REMOTE_ADDR. + """ + x_forwarded_for = request.META.get("HTTP_X_FORWARDED_FOR") + if x_forwarded_for: + ip = x_forwarded_for.split(",")[0] + else: + ip = request.META.get("REMOTE_ADDR") + return ip From 16b0d23d92e78247f57a05a0f4d6e11bf1a8d4b2 Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Fri, 21 Nov 2025 10:52:29 -0600 Subject: [PATCH 04/31] feat(client-updates): change hostname to client name, grab the client IP, and use it to further verify a client --- scram/route_manager/api/views.py | 54 ++++++++++++++++++++++++++------ 1 file changed, 44 insertions(+), 10 deletions(-) diff --git a/scram/route_manager/api/views.py b/scram/route_manager/api/views.py index d1da5f8d..ef1fae60 100644 --- a/scram/route_manager/api/views.py +++ b/scram/route_manager/api/views.py @@ -14,6 +14,8 @@ from rest_framework.response import Response from simple_history.utils import update_change_reason +from scram.shared.shared_code import get_client_ip + from ..models import ActionType, Client, Entry, IgnoreEntry, Route, WebSocketSequenceElement from .exceptions import ActiontypeNotAllowed, IgnoredRoute, NoActiveEntryFound, PrefixTooLarge from .serializers import ActionTypeSerializer, ClientSerializer, EntrySerializer, IgnoreEntrySerializer @@ -53,15 +55,31 @@ class IgnoreEntryViewSet(viewsets.ModelViewSet): responses={200: ClientSerializer}, ) class ClientViewSet(viewsets.ModelViewSet): - """Lookup Client by hostname on POSTs regardless of authentication, and bind to the serializer.""" + """Lookup Client by client_name on POSTs regardless of authentication, and bind to the serializer.""" queryset = Client.objects.all() # We want to allow a client to be registered from anywhere permission_classes = (AllowAny,) serializer_class = ClientSerializer - lookup_field = "hostname" + lookup_field = "client_name" http_method_names = ["post"] + def perform_create(self, serializer): + """Create a new Client, capturing the IP address it was created from.""" + ip = get_client_ip(self.request) + serializer.save(registered_from_ip=ip) + + def create(self, request, *args, **kwargs): + """Create a new Client or retrieve an existing one, avoiding client_name enumeration.""" + client_name = request.data.get("client_name") + client = self.queryset.filter(client_name=client_name).first() + + if client: + serializer = self.get_serializer(client) + return Response(serializer.data, status=status.HTTP_200_OK) + + return super().create(request, *args, **kwargs) + @extend_schema( description="API endpoint for entries", @@ -87,18 +105,34 @@ def get_permissions(self): return super().get_permissions() def check_client_authorization(self, actiontype): - """Ensure that a given client is authorized to use a given actiontype.""" + """Ensure that a given client is authorized to use a given actiontype and IP address.""" uuid = self.request.data.get("uuid") if uuid: - authorized_actiontypes = Client.objects.filter(uuid=uuid).values_list( - "authorized_actiontypes__name", - flat=True, - ) - authorized_client = Client.objects.filter(uuid=uuid).values("is_authorized") - if not authorized_client or actiontype not in authorized_actiontypes: - logger.debug("Client: %s, actiontypes: %s", uuid, authorized_actiontypes) + try: + client = Client.objects.get(uuid=uuid) + except Client.DoesNotExist: + raise PermissionDenied("Client not found.") + + # Check if client is authorized for the action type + if not client.is_authorized or actiontype not in client.authorized_actiontypes.values_list("name", flat=True): + logger.debug( + "Client: %s, actiontypes: %s", uuid, list(client.authorized_actiontypes.values_list("name", flat=True)) + ) logger.info("%s is not allowed to add an entry to the %s list.", uuid, actiontype) raise ActiontypeNotAllowed + + # Check if the client's IP address is whitelisted + if client.registered_from_ip: + request_ip = self.get_client_ip() + if client.registered_from_ip != request_ip: + logger.warning( + "Client %s attempted to authorize from unauthorized IP %s (expected %s)", + uuid, + request_ip, + client.registered_from_ip, + ) + raise PermissionDenied("Request from unauthorized IP address.") + elif not self.request.user.has_perm("route_manager.can_add_entry"): raise PermissionDenied From 17d78d9094471cdb81b601ab907c8466ddb14ab7 Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Sat, 22 Nov 2025 19:37:56 -0600 Subject: [PATCH 05/31] style(formatting): fix some formatting --- scram/route_manager/admin.py | 6 ++++-- scram/route_manager/api/views.py | 8 ++++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/scram/route_manager/admin.py b/scram/route_manager/admin.py index 16edc225..42eadc51 100644 --- a/scram/route_manager/admin.py +++ b/scram/route_manager/admin.py @@ -51,10 +51,12 @@ class EntryAdmin(SimpleHistoryAdmin): ] search_fields = ["route", "comment"] + @admin.register(Client) class ClientAdmin(admin.ModelAdmin): - list_display = ('client_name', 'uuid', 'registered_from_ip') - readonly_fields = ('uuid',) + """Configure the Client and how it shows up in the Admin site.""" + list_display = ("client_name", "uuid", "registered_from_ip") + readonly_fields = ("uuid",) admin.site.register(IgnoreEntry, SimpleHistoryAdmin) diff --git a/scram/route_manager/api/views.py b/scram/route_manager/api/views.py index ef1fae60..44281248 100644 --- a/scram/route_manager/api/views.py +++ b/scram/route_manager/api/views.py @@ -114,9 +114,13 @@ def check_client_authorization(self, actiontype): raise PermissionDenied("Client not found.") # Check if client is authorized for the action type - if not client.is_authorized or actiontype not in client.authorized_actiontypes.values_list("name", flat=True): + if not client.is_authorized or actiontype not in client.authorized_actiontypes.values_list( + "name", flat=True + ): logger.debug( - "Client: %s, actiontypes: %s", uuid, list(client.authorized_actiontypes.values_list("name", flat=True)) + "Client: %s, actiontypes: %s", + uuid, + list(client.authorized_actiontypes.values_list("name", flat=True)), ) logger.info("%s is not allowed to add an entry to the %s list.", uuid, actiontype) raise ActiontypeNotAllowed From 24c9d7f2d3181dd16a20ec3ac3319ffb5e3726b0 Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Sat, 22 Nov 2025 19:44:31 -0600 Subject: [PATCH 06/31] style(ruff): updating some ruff stuff --- scram/route_manager/api/views.py | 8 +++++--- scram/shared/shared_code.py | 3 +++ 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/scram/route_manager/api/views.py b/scram/route_manager/api/views.py index 44281248..f966c4aa 100644 --- a/scram/route_manager/api/views.py +++ b/scram/route_manager/api/views.py @@ -110,8 +110,9 @@ def check_client_authorization(self, actiontype): if uuid: try: client = Client.objects.get(uuid=uuid) - except Client.DoesNotExist: - raise PermissionDenied("Client not found.") + except Client.DoesNotExist as client_dne: + msg = f"Client {self.client_name} does not exist" + raise PermissionDenied(msg) from client_dne # Check if client is authorized for the action type if not client.is_authorized or actiontype not in client.authorized_actiontypes.values_list( @@ -135,7 +136,8 @@ def check_client_authorization(self, actiontype): request_ip, client.registered_from_ip, ) - raise PermissionDenied("Request from unauthorized IP address.") + msg = "Request from unauthorized IP address %s" + raise PermissionDenied(msg) elif not self.request.user.has_perm("route_manager.can_add_entry"): raise PermissionDenied diff --git a/scram/shared/shared_code.py b/scram/shared/shared_code.py index 32f74e7d..766455bd 100644 --- a/scram/shared/shared_code.py +++ b/scram/shared/shared_code.py @@ -63,6 +63,9 @@ def get_client_ip(request) -> str | None: """Get the client's IP address from the request. Checks the HTTP_X_FORWARDED_FOR header first, then falls back to REMOTE_ADDR. + + Returns: + IP address from the client """ x_forwarded_for = request.META.get("HTTP_X_FORWARDED_FOR") if x_forwarded_for: From 464b8e00d674e633e9df0f2e0b26cda9614a80a3 Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Wed, 10 Dec 2025 15:31:20 -0600 Subject: [PATCH 07/31] fix(error): there was a timing issue getting this information into the error message we can live without that info for now --- scram/route_manager/api/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scram/route_manager/api/views.py b/scram/route_manager/api/views.py index f966c4aa..a37d68e1 100644 --- a/scram/route_manager/api/views.py +++ b/scram/route_manager/api/views.py @@ -111,7 +111,7 @@ def check_client_authorization(self, actiontype): try: client = Client.objects.get(uuid=uuid) except Client.DoesNotExist as client_dne: - msg = f"Client {self.client_name} does not exist" + msg = f"Client does not exist" raise PermissionDenied(msg) from client_dne # Check if client is authorized for the action type From 3334e72d31e8c402d82346f94087eaee22ab688c Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Wed, 17 Dec 2025 14:44:34 -0600 Subject: [PATCH 08/31] test(API-call): fix the API call that had the wrong arguments causing a failure --- scram/route_manager/tests/test_api.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/scram/route_manager/tests/test_api.py b/scram/route_manager/tests/test_api.py index 7995f14b..66b7d27f 100644 --- a/scram/route_manager/tests/test_api.py +++ b/scram/route_manager/tests/test_api.py @@ -14,7 +14,9 @@ class TestAddRemoveIP(APITestCase): def setUp(self): """Set up the environment for our tests.""" self.url = reverse("api:v1:entry-list") - self.superuser = get_user_model().objects.create_superuser("admin", "admin@es.net", "admintestpassword") + self.superuser = get_user_model().objects.create_superuser( + "admin", "admin@es.net", "admintestpassword" + ) self.client.login(username="admin", password="admintestpassword") self.authorized_client = Client.objects.create( client_name="authorized_client.es.net", @@ -110,7 +112,7 @@ def test_unauthenticated_users_have_no_create_access(self): "route": "192.0.2.4", "comment": "test", "uuid": "0e7e1cbd-7d73-4968-bc4b-ce3265dc2fd3", - "who": "person", + # "who": "person", }, format="json", ) @@ -118,7 +120,9 @@ def test_unauthenticated_users_have_no_create_access(self): def test_unauthenticated_users_have_no_ignore_create_access(self): """Ensure an unauthenticated client can't add an IgnoreEntry.""" - response = self.client.post(self.ignore_url, {"route": "192.0.2.4"}, format="json") + response = self.client.post( + self.ignore_url, {"route": "192.0.2.4"}, format="json" + ) self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) def test_unauthenticated_users_have_no_list_access(self): From 02fb391765550ab1b0e37c44a34ab2da3fe90c14 Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Thu, 18 Dec 2025 10:39:46 -0600 Subject: [PATCH 09/31] style(ruff): fix ruff errors --- scram/route_manager/api/views.py | 2 +- scram/route_manager/tests/test_api.py | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/scram/route_manager/api/views.py b/scram/route_manager/api/views.py index a37d68e1..db6a1ebb 100644 --- a/scram/route_manager/api/views.py +++ b/scram/route_manager/api/views.py @@ -111,7 +111,7 @@ def check_client_authorization(self, actiontype): try: client = Client.objects.get(uuid=uuid) except Client.DoesNotExist as client_dne: - msg = f"Client does not exist" + msg = "Client does not exist" raise PermissionDenied(msg) from client_dne # Check if client is authorized for the action type diff --git a/scram/route_manager/tests/test_api.py b/scram/route_manager/tests/test_api.py index 66b7d27f..56255e57 100644 --- a/scram/route_manager/tests/test_api.py +++ b/scram/route_manager/tests/test_api.py @@ -112,7 +112,6 @@ def test_unauthenticated_users_have_no_create_access(self): "route": "192.0.2.4", "comment": "test", "uuid": "0e7e1cbd-7d73-4968-bc4b-ce3265dc2fd3", - # "who": "person", }, format="json", ) From b42ae9785ddada7044268452866338b36a06e4be Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Thu, 18 Dec 2025 10:40:39 -0600 Subject: [PATCH 10/31] ci(docker): make sure to clean old containers before we start building and running tests It seems like we were hitting a subtle problem where it was using cached images which caused it to run the main code instead of my code. --- .github/workflows/behave.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/behave.yml b/.github/workflows/behave.yml index ad82b635..e3d66421 100644 --- a/.github/workflows/behave.yml +++ b/.github/workflows/behave.yml @@ -43,6 +43,9 @@ jobs: username: ${{ secrets.DOCKER_HUB_PULL_AUTH_USER }} password: ${{ secrets.DOCKER_HUB_PULL_AUTH_PAT }} + - name: Clean Docker Environment + run: make clean + - name: Build Docker images run: make build env: From bd29206516af6aefc5f3ccee8a044fe53bf7adaa Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Thu, 18 Dec 2025 10:44:02 -0600 Subject: [PATCH 11/31] Revert "ci(docker): make sure to clean old containers before we start building and running tests" This reverts commit b42ae9785ddada7044268452866338b36a06e4be. --- .github/workflows/behave.yml | 3 --- 1 file changed, 3 deletions(-) diff --git a/.github/workflows/behave.yml b/.github/workflows/behave.yml index e3d66421..ad82b635 100644 --- a/.github/workflows/behave.yml +++ b/.github/workflows/behave.yml @@ -43,9 +43,6 @@ jobs: username: ${{ secrets.DOCKER_HUB_PULL_AUTH_USER }} password: ${{ secrets.DOCKER_HUB_PULL_AUTH_PAT }} - - name: Clean Docker Environment - run: make clean - - name: Build Docker images run: make build env: From 63b6cf916f025d46f40c05a5bb55aa8882abfbd5 Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Thu, 18 Dec 2025 10:46:03 -0600 Subject: [PATCH 12/31] test(hostname-rename): fix the setup on TestIsActive to use client_name instead of hostname apparently the failure was because there were changes on main that hadn't been pulled here and they were run causing failures --- scram/route_manager/tests/test_api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scram/route_manager/tests/test_api.py b/scram/route_manager/tests/test_api.py index 6af9f41d..037d490f 100644 --- a/scram/route_manager/tests/test_api.py +++ b/scram/route_manager/tests/test_api.py @@ -137,7 +137,7 @@ def setUp(self): """Set up test data.""" self.url = reverse("api:v1:is_active-list") self.authorized_client = Client.objects.create( - hostname="authorized_client.es.net", + client_name="authorized_client.es.net", uuid="0e7e1cbd-7d73-4968-bc4b-ce3265dc2fd3", is_authorized=True, ) From a7289fc3edb78d51ebd5ec1a10f5a9aabaf42edb Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Thu, 18 Dec 2025 10:47:03 -0600 Subject: [PATCH 13/31] docs(update-docstring): use allow listed to match our nomenclature --- scram/route_manager/api/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scram/route_manager/api/views.py b/scram/route_manager/api/views.py index 59126db0..ff505763 100644 --- a/scram/route_manager/api/views.py +++ b/scram/route_manager/api/views.py @@ -178,7 +178,7 @@ def check_client_authorization(self, actiontype): logger.info("%s is not allowed to add an entry to the %s list.", uuid, actiontype) raise ActiontypeNotAllowed - # Check if the client's IP address is whitelisted + # Check if the client's IP address is allow listed if client.registered_from_ip: request_ip = self.get_client_ip() if client.registered_from_ip != request_ip: From 12240f9e8968f2b86d6cfb46210db62c80d7819f Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Thu, 18 Dec 2025 10:51:06 -0600 Subject: [PATCH 14/31] style(black): ran black --- scram/route_manager/admin.py | 11 ++- .../tests/acceptance/steps/common.py | 19 ++++-- .../tests/acceptance/steps/ip.py | 8 ++- .../tests/acceptance/steps/syncing.py | 4 +- .../tests/acceptance/steps/translator.py | 8 ++- scram/route_manager/tests/test_admin.py | 16 +++-- scram/route_manager/tests/test_api.py | 28 ++++++-- .../route_manager/tests/test_authorization.py | 22 ++++-- scram/route_manager/tests/test_history.py | 4 +- scram/route_manager/tests/test_pagination.py | 61 +++++++++++------ scram/route_manager/tests/test_views.py | 4 +- scram/route_manager/tests/test_websockets.py | 68 +++++++++++++++---- 12 files changed, 194 insertions(+), 59 deletions(-) diff --git a/scram/route_manager/admin.py b/scram/route_manager/admin.py index 42eadc51..dfeb4224 100644 --- a/scram/route_manager/admin.py +++ b/scram/route_manager/admin.py @@ -3,7 +3,15 @@ from django.contrib import admin from simple_history.admin import SimpleHistoryAdmin -from .models import ActionType, Client, Entry, IgnoreEntry, Route, WebSocketMessage, WebSocketSequenceElement +from .models import ( + ActionType, + Client, + Entry, + IgnoreEntry, + Route, + WebSocketMessage, + WebSocketSequenceElement, +) class WhoFilter(admin.SimpleListFilter): @@ -55,6 +63,7 @@ class EntryAdmin(SimpleHistoryAdmin): @admin.register(Client) class ClientAdmin(admin.ModelAdmin): """Configure the Client and how it shows up in the Admin site.""" + list_display = ("client_name", "uuid", "registered_from_ip") readonly_fields = ("uuid",) diff --git a/scram/route_manager/tests/acceptance/steps/common.py b/scram/route_manager/tests/acceptance/steps/common.py index f6fde841..90fd8054 100644 --- a/scram/route_manager/tests/acceptance/steps/common.py +++ b/scram/route_manager/tests/acceptance/steps/common.py @@ -9,7 +9,12 @@ from django import conf from django.urls import reverse -from scram.route_manager.models import ActionType, Client, WebSocketMessage, WebSocketSequenceElement +from scram.route_manager.models import ( + ActionType, + Client, + WebSocketMessage, + WebSocketSequenceElement, +) @given("a {name} actiontype is defined") @@ -22,9 +27,13 @@ def create_actiontype(context, name): ) at, _ = ActionType.objects.get_or_create(name=name) - wsm, _ = WebSocketMessage.objects.get_or_create(msg_type="translator_add", msg_data_route_field="route") + wsm, _ = WebSocketMessage.objects.get_or_create( + msg_type="translator_add", msg_data_route_field="route" + ) wsm.save() - wsse, _ = WebSocketSequenceElement.objects.get_or_create(websocketmessage=wsm, verb="A", action_type=at) + wsse, _ = WebSocketSequenceElement.objects.get_or_create( + websocketmessage=wsm, verb="A", action_type=at + ) wsse.save() @@ -165,7 +174,9 @@ def add_ignore_entry(context, value): @when("we remove the {model} {value}") def remove_an_object(context, model, value): """Remove any model object with the matching value.""" - context.response = context.test.client.delete(reverse(f"api:v1:{model.lower()}-detail", args=[value])) + context.response = context.test.client.delete( + reverse(f"api:v1:{model.lower()}-detail", args=[value]) + ) @when("we list the {model}s") diff --git a/scram/route_manager/tests/acceptance/steps/ip.py b/scram/route_manager/tests/acceptance/steps/ip.py index a0f3bc07..a4d09faf 100644 --- a/scram/route_manager/tests/acceptance/steps/ip.py +++ b/scram/route_manager/tests/acceptance/steps/ip.py @@ -27,7 +27,9 @@ def check_route(context, route, model): def check_ip(context, ip): """Find an Entry for the specified IP.""" try: - context.response = context.test.client.get(reverse("api:v1:entry-detail", args=[ip])) + context.response = context.test.client.get( + reverse("api:v1:entry-detail", args=[ip]) + ) context.queryException = None except ValueError as e: context.response = None @@ -57,7 +59,9 @@ def update_entry_comment(context, value, comment): data = {"comment": comment, "who": context.client.client_name} context.response = context.test.client.put( - reverse("api:v1:entry-detail", args=[value]), data=json.dumps(data), content_type="application/json" + reverse("api:v1:entry-detail", args=[value]), + data=json.dumps(data), + content_type="application/json", ) diff --git a/scram/route_manager/tests/acceptance/steps/syncing.py b/scram/route_manager/tests/acceptance/steps/syncing.py index 95e5ba5c..efe99c85 100644 --- a/scram/route_manager/tests/acceptance/steps/syncing.py +++ b/scram/route_manager/tests/acceptance/steps/syncing.py @@ -43,7 +43,9 @@ def check_not_announced(context, ip): testing_hostname = settings.SCRAM_HOSTNAME try: - entry = Entry.objects.get(route__route=ip, originating_scram_instance=testing_hostname) + entry = Entry.objects.get( + route__route=ip, originating_scram_instance=testing_hostname + ) except Entry.DoesNotExist: return diff --git a/scram/route_manager/tests/acceptance/steps/translator.py b/scram/route_manager/tests/acceptance/steps/translator.py index ff478a9a..0ba67d2b 100644 --- a/scram/route_manager/tests/acceptance/steps/translator.py +++ b/scram/route_manager/tests/acceptance/steps/translator.py @@ -9,11 +9,15 @@ async def query_translator(route, actiontype, is_announced): """Ensure the specified route is currently either blocked or unblocked.""" - communicator = WebsocketCommunicator(ws_application, f"/ws/route_manager/webui_{actiontype}/") + communicator = WebsocketCommunicator( + ws_application, f"/ws/route_manager/webui_{actiontype}/" + ) connected, _ = await communicator.connect() assert connected - await communicator.send_json_to({"type": "wui_check_req", "message": {"route": route}}) + await communicator.send_json_to( + {"type": "wui_check_req", "message": {"route": route}} + ) response = await communicator.receive_json_from(timeout=10) assert response["type"] == "wui_check_resp" assert response["message"]["is_blocked"] == is_announced diff --git a/scram/route_manager/tests/test_admin.py b/scram/route_manager/tests/test_admin.py index 4cf0a4d3..bc54727e 100644 --- a/scram/route_manager/tests/test_admin.py +++ b/scram/route_manager/tests/test_admin.py @@ -17,12 +17,18 @@ def setUp(self): route1 = Route.objects.create(route="192.168.1.1") route2 = Route.objects.create(route="192.168.1.2") - self.entry1 = Entry.objects.create(route=route1, actiontype=self.atype, who="admin") - self.entry2 = Entry.objects.create(route=route2, actiontype=self.atype, who="user1") + self.entry1 = Entry.objects.create( + route=route1, actiontype=self.atype, who="admin" + ) + self.entry2 = Entry.objects.create( + route=route2, actiontype=self.atype, who="user1" + ) def test_who_filter_lookups(self): """Test that the WhoFilter returns the correct users who have made entries.""" - who_filter = WhoFilter(request=None, params={}, model=Entry, model_admin=EntryAdmin) + who_filter = WhoFilter( + request=None, params={}, model=Entry, model_admin=EntryAdmin + ) mock_request = MagicMock() mock_model_admin = MagicMock(spec=EntryAdmin) @@ -35,7 +41,9 @@ def test_who_filter_lookups(self): def test_who_filter_queryset_with_value(self): """Test that the queryset is filtered correctly when a user is selected.""" - who_filter = WhoFilter(request=None, params={"who": "admin"}, model=Entry, model_admin=EntryAdmin) + who_filter = WhoFilter( + request=None, params={"who": "admin"}, model=Entry, model_admin=EntryAdmin + ) queryset = Entry.objects.all() filtered_queryset = who_filter.queryset(None, queryset) diff --git a/scram/route_manager/tests/test_api.py b/scram/route_manager/tests/test_api.py index 037d490f..64a21d18 100644 --- a/scram/route_manager/tests/test_api.py +++ b/scram/route_manager/tests/test_api.py @@ -142,32 +142,50 @@ def setUp(self): is_authorized=True, ) self.authorized_client.authorized_actiontypes.set([1]) - self.actiontype, _ = ActionType.objects.get_or_create(pk=1, defaults={"name": "block"}) + self.actiontype, _ = ActionType.objects.get_or_create( + pk=1, defaults={"name": "block"} + ) # Create some active entries # Active IPv4 route_v4 = Route.objects.create(route="192.0.2.100") Entry.objects.create( - route=route_v4, is_active=True, comment="test active", who="test", actiontype=self.actiontype + route=route_v4, + is_active=True, + comment="test active", + who="test", + actiontype=self.actiontype, ) # Active IPv6 route_v6 = Route.objects.create(route="2001:db8::1") Entry.objects.create( - route=route_v6, is_active=True, comment="test active v6", who="test", actiontype=self.actiontype + route=route_v6, + is_active=True, + comment="test active v6", + who="test", + actiontype=self.actiontype, ) # Deactivated IPv4 entry route_inactive = Route.objects.create(route="192.0.2.200") Entry.objects.create( - route=route_inactive, is_active=False, comment="inactive", who="test", actiontype=self.actiontype + route=route_inactive, + is_active=False, + comment="inactive", + who="test", + actiontype=self.actiontype, ) # Deactived IPv6 entry route_inactive = Route.objects.create(route="2001:db8::5") Entry.objects.create( - route=route_inactive, is_active=False, comment="inactive", who="test", actiontype=self.actiontype + route=route_inactive, + is_active=False, + comment="inactive", + who="test", + actiontype=self.actiontype, ) def test_active_ipv4_returns_true(self): diff --git a/scram/route_manager/tests/test_authorization.py b/scram/route_manager/tests/test_authorization.py index 532c5c14..a8925c8d 100644 --- a/scram/route_manager/tests/test_authorization.py +++ b/scram/route_manager/tests/test_authorization.py @@ -29,7 +29,9 @@ def setUp(self): self.readwrite_user.groups.set([self.readwrite_group]) self.readwrite_user.save() - self.admin_user = User.objects.create(username="admin", is_staff=True, is_superuser=True) + self.admin_user = User.objects.create( + username="admin", is_staff=True, is_superuser=True + ) self.write_blocked_users = [None, self.unauthorized_user, self.readonly_user] self.write_allowed_users = [self.readwrite_user, self.admin_user] @@ -104,7 +106,9 @@ def test_unauthorized_detail_view(self): for user in self.detail_blocked_users: if user: self.client.force_login(user) - response = self.client.get(reverse("route_manager:detail", kwargs={"pk": pk})) + response = self.client.get( + reverse("route_manager:detail", kwargs={"pk": pk}) + ) self.assertIn(response.status_code, [302, 403], msg=f"username={user}") def test_authorized_detail_view(self): @@ -113,7 +117,9 @@ def test_authorized_detail_view(self): for user in self.detail_allowed_users: self.client.force_login(user) - response = self.client.get(reverse("route_manager:detail", kwargs={"pk": pk})) + response = self.client.get( + reverse("route_manager:detail", kwargs={"pk": pk}) + ) self.assertEqual(response.status_code, 200, msg=f"username={user}") def test_unauthorized_after_group_removal(self): @@ -123,12 +129,18 @@ def test_unauthorized_after_group_removal(self): test_user.save() self.client.force_login(test_user) - response = self.client.post(reverse("route_manager:add"), {"route": "192.0.2.4/32", "actiontype": "block"}) + response = self.client.post( + reverse("route_manager:add"), + {"route": "192.0.2.4/32", "actiontype": "block"}, + ) self.assertEqual(response.status_code, 302) test_user.groups.set([]) - response = self.client.post(reverse("route_manager:add"), {"route": "192.0.2.5/32", "actiontype": "block"}) + response = self.client.post( + reverse("route_manager:add"), + {"route": "192.0.2.5/32", "actiontype": "block"}, + ) self.assertEqual(response.status_code, 302) diff --git a/scram/route_manager/tests/test_history.py b/scram/route_manager/tests/test_history.py index d2848e83..cdb328ac 100644 --- a/scram/route_manager/tests/test_history.py +++ b/scram/route_manager/tests/test_history.py @@ -41,7 +41,9 @@ def test_comments(self): for r in self.routes: route_old = Route.objects.get(route=r) e = Entry.objects.get(route=route_old) - self.assertEqual(e.get_change_reason(), "Zeek detected a scan from 192.0.2.1.") + self.assertEqual( + e.get_change_reason(), "Zeek detected a scan from 192.0.2.1." + ) route_new = str(route_old).replace("16", "32") e.route = Route.objects.create(route=route_new) diff --git a/scram/route_manager/tests/test_pagination.py b/scram/route_manager/tests/test_pagination.py index dfa0ffaa..38ac3ee0 100644 --- a/scram/route_manager/tests/test_pagination.py +++ b/scram/route_manager/tests/test_pagination.py @@ -21,37 +21,60 @@ def setUp(self): """Set up the test environment.""" self.fake = Faker() self.fake.add_provider(internet) - get_user_model().objects.create_user(username="testuser", password="testpass123") + get_user_model().objects.create_user( + username="testuser", password="testpass123" + ) self.atype1 = ActionType.objects.create(name="Type1", available=True) self.atype2 = ActionType.objects.create(name="Type2", available=True) self.atype3 = ActionType.objects.create(name="Type3", available=False) # Create enough entries to test pagination - created_routes = Route.objects.bulk_create([ - Route(route=self.fake.unique.ipv4_public()) for x in range(self.TEST_PAGINATION_SIZE + 3) - ]) - entries_type1 = Entry.objects.bulk_create([ - Entry(route=route, actiontype=self.atype1, is_active=True) for route in created_routes - ]) + created_routes = Route.objects.bulk_create( + [ + Route(route=self.fake.unique.ipv4_public()) + for x in range(self.TEST_PAGINATION_SIZE + 3) + ] + ) + entries_type1 = Entry.objects.bulk_create( + [ + Entry(route=route, actiontype=self.atype1, is_active=True) + for route in created_routes + ] + ) # Create a second type of entries to test filtering per actiontype - created_routes = Route.objects.bulk_create([Route(route=self.fake.unique.ipv4_public()) for x in range(3)]) - entries_type2 = Entry.objects.bulk_create([ - Entry(route=route, actiontype=self.atype2, is_active=True) for route in created_routes - ]) + created_routes = Route.objects.bulk_create( + [Route(route=self.fake.unique.ipv4_public()) for x in range(3)] + ) + entries_type2 = Entry.objects.bulk_create( + [ + Entry(route=route, actiontype=self.atype2, is_active=True) + for route in created_routes + ] + ) # Create inactive entries to test filtering by available actiontypes - created_routes = Route.objects.bulk_create([Route(route=self.fake.unique.ipv4_public()) for x in range(3)]) - Entry.objects.bulk_create([ - Entry(route=route, actiontype=self.atype1, is_active=False) for route in created_routes - ]) + created_routes = Route.objects.bulk_create( + [Route(route=self.fake.unique.ipv4_public()) for x in range(3)] + ) + Entry.objects.bulk_create( + [ + Entry(route=route, actiontype=self.atype1, is_active=False) + for route in created_routes + ] + ) # Create entries for an invalid actiontype to test that - created_routes = Route.objects.bulk_create([Route(route=self.fake.unique.ipv4_public()) for x in range(3)]) - Entry.objects.bulk_create([ - Entry(route=route, actiontype=self.atype3, is_active=False) for route in created_routes - ]) + created_routes = Route.objects.bulk_create( + [Route(route=self.fake.unique.ipv4_public()) for x in range(3)] + ) + Entry.objects.bulk_create( + [ + Entry(route=route, actiontype=self.atype3, is_active=False) + for route in created_routes + ] + ) self.entries = { "type1": entries_type1, diff --git a/scram/route_manager/tests/test_views.py b/scram/route_manager/tests/test_views.py index 1433135a..7a59a4d0 100644 --- a/scram/route_manager/tests/test_views.py +++ b/scram/route_manager/tests/test_views.py @@ -54,5 +54,7 @@ def test_404(self): """Grab a bad URL.""" response = self.client.get("/foobarbaz") self.assertContains( - response, b'
The page you are looking for was not found.
', status_code=404 + response, + b'
The page you are looking for was not found.
', + status_code=404, ) diff --git a/scram/route_manager/tests/test_websockets.py b/scram/route_manager/tests/test_websockets.py index 39d921b3..a8d23fe9 100644 --- a/scram/route_manager/tests/test_websockets.py +++ b/scram/route_manager/tests/test_websockets.py @@ -12,7 +12,12 @@ from django.urls import reverse from config.routing import websocket_urlpatterns -from scram.route_manager.models import ActionType, Client, WebSocketMessage, WebSocketSequenceElement +from scram.route_manager.models import ( + ActionType, + Client, + WebSocketMessage, + WebSocketSequenceElement, +) @asynccontextmanager @@ -28,7 +33,8 @@ async def get_communicators(actiontypes, should_match, *args, **kwds): """ router = URLRouter(websocket_urlpatterns) communicators = [ - WebsocketCommunicator(router, f"/ws/route_manager/translator_{actiontype}/") for actiontype in actiontypes + WebsocketCommunicator(router, f"/ws/route_manager/translator_{actiontype}/") + for actiontype in actiontypes ] response = zip(communicators, should_match, strict=True) @@ -51,7 +57,9 @@ def setUp(self): """Set up our test environment.""" # TODO: This is copied from test_api; should de-dupe this. self.url = reverse("api:v1:entry-list") - self.superuser = get_user_model().objects.create_superuser("admin", "admin@example.net", "admintestpassword") + self.superuser = get_user_model().objects.create_superuser( + "admin", "admin@example.net", "admintestpassword" + ) self.client.login(username="admin", password="admintestpassword") self.uuid = "0e7e1cbd-7d73-4968-bc4b-ce3265dc2fd3" @@ -67,7 +75,9 @@ def setUp(self): ) self.authorized_client.authorized_actiontypes.set([self.actiontype]) - wsm, _ = WebSocketMessage.objects.get_or_create(msg_type="translator_add", msg_data_route_field="route") + wsm, _ = WebSocketMessage.objects.get_or_create( + msg_type="translator_add", msg_data_route_field="route" + ) _, _ = WebSocketSequenceElement.objects.get_or_create( websocketmessage=wsm, verb="A", @@ -77,7 +87,12 @@ def setUp(self): # Set some defaults; some child classes override this self.actiontypes = ["block"] * 3 self.should_match = [True] * 3 - self.generate_add_msgs = [lambda ip, mask: {"type": "translator_add", "message": {"route": f"{ip}/{mask}"}}] + self.generate_add_msgs = [ + lambda ip, mask: { + "type": "translator_add", + "message": {"route": f"{ip}/{mask}"}, + } + ] # Now we run any local setup actions by the child classes self.local_setup() @@ -99,7 +114,9 @@ async def get_nothings(self, communicator): async def add_ip(self, ip, mask): """Ensure we can add an IP to block.""" - async with get_communicators(self.actiontypes, self.should_match) as communicators: + async with get_communicators( + self.actiontypes, self.should_match + ) as communicators: await self.api_create_entry(ip) # A list of that many function calls to verify the response @@ -164,14 +181,18 @@ class TranslatorSequenceTestCase(TestTranslatorBaseCase): def local_setup(self): """Define the messages we want to send.""" - wsm2 = WebSocketMessage.objects.create(msg_type="translator_add", msg_data_route_field="foo") + wsm2 = WebSocketMessage.objects.create( + msg_type="translator_add", msg_data_route_field="foo" + ) _ = WebSocketSequenceElement.objects.create( websocketmessage=wsm2, verb="A", action_type=self.actiontype, order_num=20, ) - wsm3 = WebSocketMessage.objects.create(msg_type="translator_add", msg_data_route_field="bar") + wsm3 = WebSocketMessage.objects.create( + msg_type="translator_add", msg_data_route_field="bar" + ) _ = WebSocketSequenceElement.objects.create( websocketmessage=wsm3, verb="A", @@ -180,9 +201,18 @@ def local_setup(self): ) self.generate_add_msgs = [ - lambda ip, mask: {"type": "translator_add", "message": {"route": f"{ip}/{mask}"}}, # order_num=0 - lambda ip, mask: {"type": "translator_add", "message": {"bar": f"{ip}/{mask}"}}, # order_num=2 - lambda ip, mask: {"type": "translator_add", "message": {"foo": f"{ip}/{mask}"}}, # order_num=20 + lambda ip, mask: { + "type": "translator_add", + "message": {"route": f"{ip}/{mask}"}, + }, # order_num=0 + lambda ip, mask: { + "type": "translator_add", + "message": {"bar": f"{ip}/{mask}"}, + }, # order_num=2 + lambda ip, mask: { + "type": "translator_add", + "message": {"foo": f"{ip}/{mask}"}, + }, # order_num=20 ] @@ -191,8 +221,14 @@ class TranslatorParametersTestCase(TestTranslatorBaseCase): def local_setup(self): """Define the message we want to send.""" - wsm = WebSocketMessage.objects.get(msg_type="translator_add", msg_data_route_field="route") - wsm.msg_data = {"asn": 65550, "community": 100, "route": "Ensure this gets overwritten."} + wsm = WebSocketMessage.objects.get( + msg_type="translator_add", msg_data_route_field="route" + ) + wsm.msg_data = { + "asn": 65550, + "community": 100, + "route": "Ensure this gets overwritten.", + } wsm.save() self.generate_add_msgs = [ @@ -202,6 +238,10 @@ def local_setup(self): }, lambda ip, mask: { "type": "translator_add", - "message": {"asn": 64496, "community": 4294967295, "route": f"{ip}/{mask}"}, + "message": { + "asn": 64496, + "community": 4294967295, + "route": f"{ip}/{mask}", + }, }, ] From c3269d90b83a4e74a521e326bac7300ebcb79151 Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Thu, 18 Dec 2025 10:52:55 -0600 Subject: [PATCH 15/31] style(black): ran black but formatter this time vs check? sighhh --- .../tests/acceptance/steps/common.py | 12 +--- .../tests/acceptance/steps/ip.py | 4 +- .../tests/acceptance/steps/syncing.py | 4 +- .../tests/acceptance/steps/translator.py | 8 +-- scram/route_manager/tests/test_admin.py | 16 ++--- scram/route_manager/tests/test_api.py | 12 +--- .../route_manager/tests/test_authorization.py | 12 +--- scram/route_manager/tests/test_history.py | 4 +- scram/route_manager/tests/test_pagination.py | 61 ++++++------------- scram/route_manager/tests/test_websockets.py | 27 +++----- 10 files changed, 44 insertions(+), 116 deletions(-) diff --git a/scram/route_manager/tests/acceptance/steps/common.py b/scram/route_manager/tests/acceptance/steps/common.py index 90fd8054..9436e609 100644 --- a/scram/route_manager/tests/acceptance/steps/common.py +++ b/scram/route_manager/tests/acceptance/steps/common.py @@ -27,13 +27,9 @@ def create_actiontype(context, name): ) at, _ = ActionType.objects.get_or_create(name=name) - wsm, _ = WebSocketMessage.objects.get_or_create( - msg_type="translator_add", msg_data_route_field="route" - ) + wsm, _ = WebSocketMessage.objects.get_or_create(msg_type="translator_add", msg_data_route_field="route") wsm.save() - wsse, _ = WebSocketSequenceElement.objects.get_or_create( - websocketmessage=wsm, verb="A", action_type=at - ) + wsse, _ = WebSocketSequenceElement.objects.get_or_create(websocketmessage=wsm, verb="A", action_type=at) wsse.save() @@ -174,9 +170,7 @@ def add_ignore_entry(context, value): @when("we remove the {model} {value}") def remove_an_object(context, model, value): """Remove any model object with the matching value.""" - context.response = context.test.client.delete( - reverse(f"api:v1:{model.lower()}-detail", args=[value]) - ) + context.response = context.test.client.delete(reverse(f"api:v1:{model.lower()}-detail", args=[value])) @when("we list the {model}s") diff --git a/scram/route_manager/tests/acceptance/steps/ip.py b/scram/route_manager/tests/acceptance/steps/ip.py index a4d09faf..93b3c6e8 100644 --- a/scram/route_manager/tests/acceptance/steps/ip.py +++ b/scram/route_manager/tests/acceptance/steps/ip.py @@ -27,9 +27,7 @@ def check_route(context, route, model): def check_ip(context, ip): """Find an Entry for the specified IP.""" try: - context.response = context.test.client.get( - reverse("api:v1:entry-detail", args=[ip]) - ) + context.response = context.test.client.get(reverse("api:v1:entry-detail", args=[ip])) context.queryException = None except ValueError as e: context.response = None diff --git a/scram/route_manager/tests/acceptance/steps/syncing.py b/scram/route_manager/tests/acceptance/steps/syncing.py index efe99c85..95e5ba5c 100644 --- a/scram/route_manager/tests/acceptance/steps/syncing.py +++ b/scram/route_manager/tests/acceptance/steps/syncing.py @@ -43,9 +43,7 @@ def check_not_announced(context, ip): testing_hostname = settings.SCRAM_HOSTNAME try: - entry = Entry.objects.get( - route__route=ip, originating_scram_instance=testing_hostname - ) + entry = Entry.objects.get(route__route=ip, originating_scram_instance=testing_hostname) except Entry.DoesNotExist: return diff --git a/scram/route_manager/tests/acceptance/steps/translator.py b/scram/route_manager/tests/acceptance/steps/translator.py index 0ba67d2b..ff478a9a 100644 --- a/scram/route_manager/tests/acceptance/steps/translator.py +++ b/scram/route_manager/tests/acceptance/steps/translator.py @@ -9,15 +9,11 @@ async def query_translator(route, actiontype, is_announced): """Ensure the specified route is currently either blocked or unblocked.""" - communicator = WebsocketCommunicator( - ws_application, f"/ws/route_manager/webui_{actiontype}/" - ) + communicator = WebsocketCommunicator(ws_application, f"/ws/route_manager/webui_{actiontype}/") connected, _ = await communicator.connect() assert connected - await communicator.send_json_to( - {"type": "wui_check_req", "message": {"route": route}} - ) + await communicator.send_json_to({"type": "wui_check_req", "message": {"route": route}}) response = await communicator.receive_json_from(timeout=10) assert response["type"] == "wui_check_resp" assert response["message"]["is_blocked"] == is_announced diff --git a/scram/route_manager/tests/test_admin.py b/scram/route_manager/tests/test_admin.py index bc54727e..4cf0a4d3 100644 --- a/scram/route_manager/tests/test_admin.py +++ b/scram/route_manager/tests/test_admin.py @@ -17,18 +17,12 @@ def setUp(self): route1 = Route.objects.create(route="192.168.1.1") route2 = Route.objects.create(route="192.168.1.2") - self.entry1 = Entry.objects.create( - route=route1, actiontype=self.atype, who="admin" - ) - self.entry2 = Entry.objects.create( - route=route2, actiontype=self.atype, who="user1" - ) + self.entry1 = Entry.objects.create(route=route1, actiontype=self.atype, who="admin") + self.entry2 = Entry.objects.create(route=route2, actiontype=self.atype, who="user1") def test_who_filter_lookups(self): """Test that the WhoFilter returns the correct users who have made entries.""" - who_filter = WhoFilter( - request=None, params={}, model=Entry, model_admin=EntryAdmin - ) + who_filter = WhoFilter(request=None, params={}, model=Entry, model_admin=EntryAdmin) mock_request = MagicMock() mock_model_admin = MagicMock(spec=EntryAdmin) @@ -41,9 +35,7 @@ def test_who_filter_lookups(self): def test_who_filter_queryset_with_value(self): """Test that the queryset is filtered correctly when a user is selected.""" - who_filter = WhoFilter( - request=None, params={"who": "admin"}, model=Entry, model_admin=EntryAdmin - ) + who_filter = WhoFilter(request=None, params={"who": "admin"}, model=Entry, model_admin=EntryAdmin) queryset = Entry.objects.all() filtered_queryset = who_filter.queryset(None, queryset) diff --git a/scram/route_manager/tests/test_api.py b/scram/route_manager/tests/test_api.py index 64a21d18..047d5f4b 100644 --- a/scram/route_manager/tests/test_api.py +++ b/scram/route_manager/tests/test_api.py @@ -14,9 +14,7 @@ class TestAddRemoveIP(APITestCase): def setUp(self): """Set up the environment for our tests.""" self.url = reverse("api:v1:entry-list") - self.superuser = get_user_model().objects.create_superuser( - "admin", "admin@es.net", "admintestpassword" - ) + self.superuser = get_user_model().objects.create_superuser("admin", "admin@es.net", "admintestpassword") self.client.login(username="admin", password="admintestpassword") self.authorized_client = Client.objects.create( client_name="authorized_client.es.net", @@ -119,9 +117,7 @@ def test_unauthenticated_users_have_no_create_access(self): def test_unauthenticated_users_have_no_ignore_create_access(self): """Ensure an unauthenticated client can't add an IgnoreEntry.""" - response = self.client.post( - self.ignore_url, {"route": "192.0.2.4"}, format="json" - ) + response = self.client.post(self.ignore_url, {"route": "192.0.2.4"}, format="json") self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) def test_unauthenticated_users_have_no_list_access(self): @@ -142,9 +138,7 @@ def setUp(self): is_authorized=True, ) self.authorized_client.authorized_actiontypes.set([1]) - self.actiontype, _ = ActionType.objects.get_or_create( - pk=1, defaults={"name": "block"} - ) + self.actiontype, _ = ActionType.objects.get_or_create(pk=1, defaults={"name": "block"}) # Create some active entries diff --git a/scram/route_manager/tests/test_authorization.py b/scram/route_manager/tests/test_authorization.py index a8925c8d..9f00dfbd 100644 --- a/scram/route_manager/tests/test_authorization.py +++ b/scram/route_manager/tests/test_authorization.py @@ -29,9 +29,7 @@ def setUp(self): self.readwrite_user.groups.set([self.readwrite_group]) self.readwrite_user.save() - self.admin_user = User.objects.create( - username="admin", is_staff=True, is_superuser=True - ) + self.admin_user = User.objects.create(username="admin", is_staff=True, is_superuser=True) self.write_blocked_users = [None, self.unauthorized_user, self.readonly_user] self.write_allowed_users = [self.readwrite_user, self.admin_user] @@ -106,9 +104,7 @@ def test_unauthorized_detail_view(self): for user in self.detail_blocked_users: if user: self.client.force_login(user) - response = self.client.get( - reverse("route_manager:detail", kwargs={"pk": pk}) - ) + response = self.client.get(reverse("route_manager:detail", kwargs={"pk": pk})) self.assertIn(response.status_code, [302, 403], msg=f"username={user}") def test_authorized_detail_view(self): @@ -117,9 +113,7 @@ def test_authorized_detail_view(self): for user in self.detail_allowed_users: self.client.force_login(user) - response = self.client.get( - reverse("route_manager:detail", kwargs={"pk": pk}) - ) + response = self.client.get(reverse("route_manager:detail", kwargs={"pk": pk})) self.assertEqual(response.status_code, 200, msg=f"username={user}") def test_unauthorized_after_group_removal(self): diff --git a/scram/route_manager/tests/test_history.py b/scram/route_manager/tests/test_history.py index cdb328ac..d2848e83 100644 --- a/scram/route_manager/tests/test_history.py +++ b/scram/route_manager/tests/test_history.py @@ -41,9 +41,7 @@ def test_comments(self): for r in self.routes: route_old = Route.objects.get(route=r) e = Entry.objects.get(route=route_old) - self.assertEqual( - e.get_change_reason(), "Zeek detected a scan from 192.0.2.1." - ) + self.assertEqual(e.get_change_reason(), "Zeek detected a scan from 192.0.2.1.") route_new = str(route_old).replace("16", "32") e.route = Route.objects.create(route=route_new) diff --git a/scram/route_manager/tests/test_pagination.py b/scram/route_manager/tests/test_pagination.py index 38ac3ee0..dfa0ffaa 100644 --- a/scram/route_manager/tests/test_pagination.py +++ b/scram/route_manager/tests/test_pagination.py @@ -21,60 +21,37 @@ def setUp(self): """Set up the test environment.""" self.fake = Faker() self.fake.add_provider(internet) - get_user_model().objects.create_user( - username="testuser", password="testpass123" - ) + get_user_model().objects.create_user(username="testuser", password="testpass123") self.atype1 = ActionType.objects.create(name="Type1", available=True) self.atype2 = ActionType.objects.create(name="Type2", available=True) self.atype3 = ActionType.objects.create(name="Type3", available=False) # Create enough entries to test pagination - created_routes = Route.objects.bulk_create( - [ - Route(route=self.fake.unique.ipv4_public()) - for x in range(self.TEST_PAGINATION_SIZE + 3) - ] - ) - entries_type1 = Entry.objects.bulk_create( - [ - Entry(route=route, actiontype=self.atype1, is_active=True) - for route in created_routes - ] - ) + created_routes = Route.objects.bulk_create([ + Route(route=self.fake.unique.ipv4_public()) for x in range(self.TEST_PAGINATION_SIZE + 3) + ]) + entries_type1 = Entry.objects.bulk_create([ + Entry(route=route, actiontype=self.atype1, is_active=True) for route in created_routes + ]) # Create a second type of entries to test filtering per actiontype - created_routes = Route.objects.bulk_create( - [Route(route=self.fake.unique.ipv4_public()) for x in range(3)] - ) - entries_type2 = Entry.objects.bulk_create( - [ - Entry(route=route, actiontype=self.atype2, is_active=True) - for route in created_routes - ] - ) + created_routes = Route.objects.bulk_create([Route(route=self.fake.unique.ipv4_public()) for x in range(3)]) + entries_type2 = Entry.objects.bulk_create([ + Entry(route=route, actiontype=self.atype2, is_active=True) for route in created_routes + ]) # Create inactive entries to test filtering by available actiontypes - created_routes = Route.objects.bulk_create( - [Route(route=self.fake.unique.ipv4_public()) for x in range(3)] - ) - Entry.objects.bulk_create( - [ - Entry(route=route, actiontype=self.atype1, is_active=False) - for route in created_routes - ] - ) + created_routes = Route.objects.bulk_create([Route(route=self.fake.unique.ipv4_public()) for x in range(3)]) + Entry.objects.bulk_create([ + Entry(route=route, actiontype=self.atype1, is_active=False) for route in created_routes + ]) # Create entries for an invalid actiontype to test that - created_routes = Route.objects.bulk_create( - [Route(route=self.fake.unique.ipv4_public()) for x in range(3)] - ) - Entry.objects.bulk_create( - [ - Entry(route=route, actiontype=self.atype3, is_active=False) - for route in created_routes - ] - ) + created_routes = Route.objects.bulk_create([Route(route=self.fake.unique.ipv4_public()) for x in range(3)]) + Entry.objects.bulk_create([ + Entry(route=route, actiontype=self.atype3, is_active=False) for route in created_routes + ]) self.entries = { "type1": entries_type1, diff --git a/scram/route_manager/tests/test_websockets.py b/scram/route_manager/tests/test_websockets.py index a8d23fe9..b9e4dc55 100644 --- a/scram/route_manager/tests/test_websockets.py +++ b/scram/route_manager/tests/test_websockets.py @@ -33,8 +33,7 @@ async def get_communicators(actiontypes, should_match, *args, **kwds): """ router = URLRouter(websocket_urlpatterns) communicators = [ - WebsocketCommunicator(router, f"/ws/route_manager/translator_{actiontype}/") - for actiontype in actiontypes + WebsocketCommunicator(router, f"/ws/route_manager/translator_{actiontype}/") for actiontype in actiontypes ] response = zip(communicators, should_match, strict=True) @@ -57,9 +56,7 @@ def setUp(self): """Set up our test environment.""" # TODO: This is copied from test_api; should de-dupe this. self.url = reverse("api:v1:entry-list") - self.superuser = get_user_model().objects.create_superuser( - "admin", "admin@example.net", "admintestpassword" - ) + self.superuser = get_user_model().objects.create_superuser("admin", "admin@example.net", "admintestpassword") self.client.login(username="admin", password="admintestpassword") self.uuid = "0e7e1cbd-7d73-4968-bc4b-ce3265dc2fd3" @@ -75,9 +72,7 @@ def setUp(self): ) self.authorized_client.authorized_actiontypes.set([self.actiontype]) - wsm, _ = WebSocketMessage.objects.get_or_create( - msg_type="translator_add", msg_data_route_field="route" - ) + wsm, _ = WebSocketMessage.objects.get_or_create(msg_type="translator_add", msg_data_route_field="route") _, _ = WebSocketSequenceElement.objects.get_or_create( websocketmessage=wsm, verb="A", @@ -114,9 +109,7 @@ async def get_nothings(self, communicator): async def add_ip(self, ip, mask): """Ensure we can add an IP to block.""" - async with get_communicators( - self.actiontypes, self.should_match - ) as communicators: + async with get_communicators(self.actiontypes, self.should_match) as communicators: await self.api_create_entry(ip) # A list of that many function calls to verify the response @@ -181,18 +174,14 @@ class TranslatorSequenceTestCase(TestTranslatorBaseCase): def local_setup(self): """Define the messages we want to send.""" - wsm2 = WebSocketMessage.objects.create( - msg_type="translator_add", msg_data_route_field="foo" - ) + wsm2 = WebSocketMessage.objects.create(msg_type="translator_add", msg_data_route_field="foo") _ = WebSocketSequenceElement.objects.create( websocketmessage=wsm2, verb="A", action_type=self.actiontype, order_num=20, ) - wsm3 = WebSocketMessage.objects.create( - msg_type="translator_add", msg_data_route_field="bar" - ) + wsm3 = WebSocketMessage.objects.create(msg_type="translator_add", msg_data_route_field="bar") _ = WebSocketSequenceElement.objects.create( websocketmessage=wsm3, verb="A", @@ -221,9 +210,7 @@ class TranslatorParametersTestCase(TestTranslatorBaseCase): def local_setup(self): """Define the message we want to send.""" - wsm = WebSocketMessage.objects.get( - msg_type="translator_add", msg_data_route_field="route" - ) + wsm = WebSocketMessage.objects.get(msg_type="translator_add", msg_data_route_field="route") wsm.msg_data = { "asn": 65550, "community": 100, From 92ce954820e7f2ca8f67fd6ad87e71e263a548b9 Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Fri, 19 Dec 2025 10:01:59 -0600 Subject: [PATCH 16/31] docs(docstring): update docstring to remove extraneous bits --- scram/route_manager/api/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scram/route_manager/api/views.py b/scram/route_manager/api/views.py index ff505763..9f2ff2f9 100644 --- a/scram/route_manager/api/views.py +++ b/scram/route_manager/api/views.py @@ -77,7 +77,7 @@ def perform_create(self, serializer): serializer.save(registered_from_ip=ip) def create(self, request, *args, **kwargs): - """Create a new Client or retrieve an existing one, avoiding client_name enumeration.""" + """Create a new Client or retrieve an existing one.""" client_name = request.data.get("client_name") client = self.queryset.filter(client_name=client_name).first() From 8fd268694bd8a6aae1b9fa549c8bd855fcf9de01 Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Fri, 19 Dec 2025 11:18:16 -0600 Subject: [PATCH 17/31] refactor(basenames): add basename parameter to all of the registered endpoints for consistency's sake and explicitness --- config/api_router.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/config/api_router.py b/config/api_router.py index e7f0ffbf..1705f3cc 100644 --- a/config/api_router.py +++ b/config/api_router.py @@ -13,12 +13,14 @@ router = DefaultRouter() -router.register("users", UserViewSet) -router.register("actiontypes", ActionTypeViewSet) -router.register("register_client", ClientViewSet) -router.register("entries", EntryViewSet) -router.register("ignore_entries", IgnoreEntryViewSet) -router.register("is_active", IsActiveViewSet, "is_active") +# The basename parameter is really only required for the IsActiveViewSet since that has a dynamic queryset. +# The others have a static queryset so they don't strictly require it. I added it for consistency. +router.register("users", UserViewSet, basename="user") +router.register("actiontypes", ActionTypeViewSet, basename="actiontype") +router.register("register_client", ClientViewSet, basename="client") +router.register("entries", EntryViewSet, basename="entry") +router.register("ignore_entries", IgnoreEntryViewSet, basename="ignoreentry") +router.register("is_active", IsActiveViewSet, basename="is_active") app_name = "api" urlpatterns = router.urls From b2030215a12a60a063a0a7f6e7c9f8e60b40dcbc Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Fri, 19 Dec 2025 11:20:08 -0600 Subject: [PATCH 18/31] fix(UUID-registration): add the UUID field to the serializer this lets people register if they add one to the registration POST. it was just set to autocreate one only. this is more flexible. --- scram/route_manager/api/serializers.py | 1 + 1 file changed, 1 insertion(+) diff --git a/scram/route_manager/api/serializers.py b/scram/route_manager/api/serializers.py index 192668fb..fe0c7f78 100644 --- a/scram/route_manager/api/serializers.py +++ b/scram/route_manager/api/serializers.py @@ -44,6 +44,7 @@ class Meta: class ClientSerializer(serializers.ModelSerializer): """Map the serializer to the model via Meta.""" + uuid = serializers.UUIDField() class Meta: """Maps to the Client model, and specifies the fields exposed by the API.""" From 502de710ee477d2b9f4da36b63ebcb25ce996647 Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Fri, 19 Dec 2025 11:40:35 -0600 Subject: [PATCH 19/31] fix(UUID): dont require the UUID on registration This lets us still use the autocreate UUID that we've done here --- scram/route_manager/api/serializers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scram/route_manager/api/serializers.py b/scram/route_manager/api/serializers.py index fe0c7f78..1bca99b6 100644 --- a/scram/route_manager/api/serializers.py +++ b/scram/route_manager/api/serializers.py @@ -44,7 +44,7 @@ class Meta: class ClientSerializer(serializers.ModelSerializer): """Map the serializer to the model via Meta.""" - uuid = serializers.UUIDField() + uuid = serializers.UUIDField(required=False) class Meta: """Maps to the Client model, and specifies the fields exposed by the API.""" From 8f797e025ed755dcd4cd514603863fdff83246c8 Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Fri, 19 Dec 2025 11:41:52 -0600 Subject: [PATCH 20/31] test(client-registration): make sure to test that we can create clients with UUID, create clients without a UUID provided, do an idempotent create, and not leak client_names --- .../tests/acceptance/features/client.feature | 19 ++++++++++++++++++- .../tests/acceptance/steps/common.py | 12 +++++++++++- 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/scram/route_manager/tests/acceptance/features/client.feature b/scram/route_manager/tests/acceptance/features/client.feature index 493c4531..ad80e35a 100644 --- a/scram/route_manager/tests/acceptance/features/client.feature +++ b/scram/route_manager/tests/acceptance/features/client.feature @@ -1,9 +1,26 @@ Feature: We can register and use clients - Scenario: We can register a client + Scenario: We can register a client with a UUID provided When we register a client named authorized_client.es.net with the uuid of 0e7e1cbd-7d73-4968-bc4b-ce3265dc2fd3 Then we get a 201 status code + Scenario: We can register a client without a UUID provided + When we register a client named authorized_client.es.net and no UUID + Then we get a 201 status code + + Scenario: We can do an idempotent create of a client with UUID + When we register a client named authorized_client2.es.net with the uuid of 1e24e7c3-f746-401e-8ae6-3fc3ff3aa814 + Then we get a 201 status code + When we register a client named authorized_client2.es.net with the uuid of 1e24e7c3-f746-401e-8ae6-3fc3ff3aa814 + Then we get a 200 status code + + Scenario: We do not leak client names + When we register a client named authorized_client3.es.net with the uuid of 9b3b9fee-3658-4e70-a52d-662f3b2d68ab + Then we get a 201 status code + When we register a client named authorized_client3.es.net with the uuid of da6845fb-f8a6-44ec-846a-54f0ee78fcf8 + Then we get a 400 status code + + Scenario: We can add a block using an authorized client Given a client with block authorization When we're logged in diff --git a/scram/route_manager/tests/acceptance/steps/common.py b/scram/route_manager/tests/acceptance/steps/common.py index 9436e609..dbaa2484 100644 --- a/scram/route_manager/tests/acceptance/steps/common.py +++ b/scram/route_manager/tests/acceptance/steps/common.py @@ -215,7 +215,7 @@ def check_object(context, value, model): @when("we register a client named {client_name} with the uuid of {uuid}") -def add_client(context, client_name, uuid): +def add_client_uuid(context, client_name, uuid): """Create a client with a specific UUID.""" context.response = context.test.client.post( reverse("api:v1:client-list"), @@ -224,3 +224,13 @@ def add_client(context, client_name, uuid): "uuid": uuid, }, ) + +@when("we register a client named {client_name} and no UUID") +def add_client_no_uuid(context, client_name): + """Create a client without a UUID.""" + context.response = context.test.client.post( + reverse("api:v1:client-list"), + { + "client_name": client_name, + } + ) From cba39df22257affadc722a34c56a9d0b5a0d86e8 Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Fri, 19 Dec 2025 12:12:36 -0600 Subject: [PATCH 21/31] fix(update-client-code): fix client creation to allow for both creation with UUID and without (for our autocreate UUID code branch) also up our documentation game so we have much better info in DRF spectacular --- scram/route_manager/api/views.py | 90 ++++++++++++++++++++++++++------ 1 file changed, 75 insertions(+), 15 deletions(-) diff --git a/scram/route_manager/api/views.py b/scram/route_manager/api/views.py index 9f2ff2f9..491a97be 100644 --- a/scram/route_manager/api/views.py +++ b/scram/route_manager/api/views.py @@ -8,7 +8,7 @@ from django.conf import settings from django.core.exceptions import PermissionDenied from django.db.models import Q -from drf_spectacular.utils import extend_schema +from drf_spectacular.utils import extend_schema, OpenApiResponse from rest_framework import status, viewsets from rest_framework.exceptions import ValidationError from rest_framework.permissions import AllowAny, IsAuthenticated @@ -32,11 +32,15 @@ @extend_schema( - description="API endpoint for actiontypes", - responses={200: ActionTypeSerializer}, + description="API endpoint for actiontypes.", + responses={ + 200: OpenApiResponse(response=ActionTypeSerializer, description="Successful retrieval of actiontype(s)."), + 403: OpenApiResponse(description="Authentication credentials were not provided."), + 404: OpenApiResponse(description="The requested actiontype does not exist."), + }, ) class ActionTypeViewSet(viewsets.ReadOnlyModelViewSet): - """Lookup ActionTypes by name when authenticated, and bind to the serializer.""" + """Lookup ActionTypes by name, and bind to the serializer.""" queryset = ActionType.objects.all() permission_classes = (IsAuthenticated,) @@ -45,8 +49,15 @@ class ActionTypeViewSet(viewsets.ReadOnlyModelViewSet): @extend_schema( - description="API endpoint for ignore entries", - responses={200: IgnoreEntrySerializer}, + description="API endpoint for ignore entries.", + responses={ + 200: OpenApiResponse(response=IgnoreEntrySerializer, description="Successful retrieval or update of an ignore entry."), + 201: OpenApiResponse(response=IgnoreEntrySerializer, description="Ignore entry successfully created."), + 204: OpenApiResponse(description="Ignore entry successfully deleted."), + 400: OpenApiResponse(description="Invalid data provided."), + 403: OpenApiResponse(description="Authentication credentials were not provided."), + 404: OpenApiResponse(description="The requested ignore entry does not exist."), + }, ) class IgnoreEntryViewSet(viewsets.ModelViewSet): """Lookup IgnoreEntries by route when authenticated, and bind to the serializer.""" @@ -58,8 +69,17 @@ class IgnoreEntryViewSet(viewsets.ModelViewSet): @extend_schema( - description="API endpoint for clients", - responses={200: ClientSerializer}, + description="API endpoint for clients.", + responses={ + 200: OpenApiResponse( + response=ClientSerializer, + description="Client already existed and was retrieved successfully.", + ), + 201: OpenApiResponse(response=ClientSerializer, description="Client successfully created."), + 400: OpenApiResponse( + description="A client with this name may already exist with a different UUID, or client_name was not provided." + ), + }, ) class ClientViewSet(viewsets.ModelViewSet): """Lookup Client by client_name on POSTs regardless of authentication, and bind to the serializer.""" @@ -77,17 +97,50 @@ def perform_create(self, serializer): serializer.save(registered_from_ip=ip) def create(self, request, *args, **kwargs): - """Create a new Client or retrieve an existing one.""" + """Create a new Client while avoiding information leaks hopefully.""" client_name = request.data.get("client_name") - client = self.queryset.filter(client_name=client_name).first() + request_uuid = request.data.get("uuid") - if client: - serializer = self.get_serializer(client) - return Response(serializer.data, status=status.HTTP_200_OK) + if not client_name: + return Response({"detail": "client_name is required."}, status=status.HTTP_400_BAD_REQUEST) + + existing_client = self.queryset.filter(client_name=client_name).first() + + if existing_client: + if request_uuid and str(existing_client.uuid) == request_uuid: + # Idempotent success + serializer = self.get_serializer(existing_client) + return Response(serializer.data, status=status.HTTP_200_OK) + + # Log the conflict without leaking client_names to the user + logger.warning( + "Client registration conflict for name: %s. A client with this name already exists with a different UUID.", + client_name, + ) + return Response( + {"detail": "Invalid client registration request."}, + status=status.HTTP_400_BAD_REQUEST, + ) return super().create(request, *args, **kwargs) +@extend_schema( + description="API endpoint to check if a route is active.", + parameters=[ + { + "name": "cidr", + "required": True, + "type": "string", + "description": "The CIDR network to check (e.g., 192.0.2.0/24).", + "in": "query", + } + ], + responses={ + 200: OpenApiResponse(response=IsActiveSerializer, description="The 'is_active' field indicates the status of the route."), + 400: OpenApiResponse(description="The 'cidr' parameter is missing or invalid."), + }, +) class IsActiveViewSet(viewsets.ReadOnlyModelViewSet): """Look up a route to see if SCRAM considers it active or deactivated.""" @@ -134,8 +187,15 @@ def list(self, request): @extend_schema( - description="API endpoint for entries", - responses={200: EntrySerializer}, + description="API endpoint for entries.", + responses={ + 200: OpenApiResponse(response=EntrySerializer, description="Successful retrieval of an entry/entries."), + 201: OpenApiResponse(response=EntrySerializer, description="Entry successfully created."), + 204: OpenApiResponse(description="Entry successfully deleted."), + 400: OpenApiResponse(description="The route is likely on the ignore list or the prefix is too large."), + 403: OpenApiResponse(description="The client is not authorized for this action."), + 404: OpenApiResponse(description="The requested entry does not exist."), + }, ) class EntryViewSet(viewsets.ModelViewSet): """Lookup Entry when authenticated, and bind to the serializer.""" From e6d053ad3d0f158bb7788906f46c422e04c346a3 Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Fri, 19 Dec 2025 12:27:47 -0600 Subject: [PATCH 22/31] style(ruff): ran ruff format and check also added pre commit config that hopefully doesnt break anything because i couldnt commit otherwise --- .pre-commit-config.yaml | 2 +- scram/route_manager/api/serializers.py | 1 + scram/route_manager/api/views.py | 14 +++++++++----- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 5315066a..7b903538 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -1,5 +1,5 @@ exclude: 'docs|migrations|.git|.tox' -default_stages: [commit] +default_stages: [pre-commit] fail_fast: true repos: diff --git a/scram/route_manager/api/serializers.py b/scram/route_manager/api/serializers.py index 1bca99b6..3bc3df1a 100644 --- a/scram/route_manager/api/serializers.py +++ b/scram/route_manager/api/serializers.py @@ -44,6 +44,7 @@ class Meta: class ClientSerializer(serializers.ModelSerializer): """Map the serializer to the model via Meta.""" + uuid = serializers.UUIDField(required=False) class Meta: diff --git a/scram/route_manager/api/views.py b/scram/route_manager/api/views.py index 491a97be..dae3cf2d 100644 --- a/scram/route_manager/api/views.py +++ b/scram/route_manager/api/views.py @@ -8,7 +8,7 @@ from django.conf import settings from django.core.exceptions import PermissionDenied from django.db.models import Q -from drf_spectacular.utils import extend_schema, OpenApiResponse +from drf_spectacular.utils import OpenApiResponse, extend_schema from rest_framework import status, viewsets from rest_framework.exceptions import ValidationError from rest_framework.permissions import AllowAny, IsAuthenticated @@ -51,7 +51,9 @@ class ActionTypeViewSet(viewsets.ReadOnlyModelViewSet): @extend_schema( description="API endpoint for ignore entries.", responses={ - 200: OpenApiResponse(response=IgnoreEntrySerializer, description="Successful retrieval or update of an ignore entry."), + 200: OpenApiResponse( + response=IgnoreEntrySerializer, description="Successful retrieval or update of an ignore entry." + ), 201: OpenApiResponse(response=IgnoreEntrySerializer, description="Ignore entry successfully created."), 204: OpenApiResponse(description="Ignore entry successfully deleted."), 400: OpenApiResponse(description="Invalid data provided."), @@ -77,7 +79,7 @@ class IgnoreEntryViewSet(viewsets.ModelViewSet): ), 201: OpenApiResponse(response=ClientSerializer, description="Client successfully created."), 400: OpenApiResponse( - description="A client with this name may already exist with a different UUID, or client_name was not provided." + description="Client with this name already exists with a different UUID, or client_name was not provided." ), }, ) @@ -114,7 +116,7 @@ def create(self, request, *args, **kwargs): # Log the conflict without leaking client_names to the user logger.warning( - "Client registration conflict for name: %s. A client with this name already exists with a different UUID.", + "Client named %s already exists with a different UUID", client_name, ) return Response( @@ -137,7 +139,9 @@ def create(self, request, *args, **kwargs): } ], responses={ - 200: OpenApiResponse(response=IsActiveSerializer, description="The 'is_active' field indicates the status of the route."), + 200: OpenApiResponse( + response=IsActiveSerializer, description="The 'is_active' field indicates the status of the route." + ), 400: OpenApiResponse(description="The 'cidr' parameter is missing or invalid."), }, ) From 43053d1fba1c5683b74e6572038e1b59a1dbaf33 Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Fri, 19 Dec 2025 11:18:16 -0600 Subject: [PATCH 23/31] refactor(basenames): add basename parameter to all of the registered endpoints for consistency's sake and explicitness --- config/api_router.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/config/api_router.py b/config/api_router.py index e7f0ffbf..1705f3cc 100644 --- a/config/api_router.py +++ b/config/api_router.py @@ -13,12 +13,14 @@ router = DefaultRouter() -router.register("users", UserViewSet) -router.register("actiontypes", ActionTypeViewSet) -router.register("register_client", ClientViewSet) -router.register("entries", EntryViewSet) -router.register("ignore_entries", IgnoreEntryViewSet) -router.register("is_active", IsActiveViewSet, "is_active") +# The basename parameter is really only required for the IsActiveViewSet since that has a dynamic queryset. +# The others have a static queryset so they don't strictly require it. I added it for consistency. +router.register("users", UserViewSet, basename="user") +router.register("actiontypes", ActionTypeViewSet, basename="actiontype") +router.register("register_client", ClientViewSet, basename="client") +router.register("entries", EntryViewSet, basename="entry") +router.register("ignore_entries", IgnoreEntryViewSet, basename="ignoreentry") +router.register("is_active", IsActiveViewSet, basename="is_active") app_name = "api" urlpatterns = router.urls From d7043869d67058d2f1ecf75a2bdff60dca817fa5 Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Fri, 19 Dec 2025 11:20:08 -0600 Subject: [PATCH 24/31] fix(UUID-registration): add the UUID field to the serializer this lets people register if they add one to the registration POST. it was just set to autocreate one only. this is more flexible. --- scram/route_manager/api/serializers.py | 1 + 1 file changed, 1 insertion(+) diff --git a/scram/route_manager/api/serializers.py b/scram/route_manager/api/serializers.py index 192668fb..fe0c7f78 100644 --- a/scram/route_manager/api/serializers.py +++ b/scram/route_manager/api/serializers.py @@ -44,6 +44,7 @@ class Meta: class ClientSerializer(serializers.ModelSerializer): """Map the serializer to the model via Meta.""" + uuid = serializers.UUIDField() class Meta: """Maps to the Client model, and specifies the fields exposed by the API.""" From 920485fb11e8c171421f969329c045dbf727d089 Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Fri, 19 Dec 2025 11:40:35 -0600 Subject: [PATCH 25/31] fix(UUID): dont require the UUID on registration This lets us still use the autocreate UUID that we've done here --- scram/route_manager/api/serializers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scram/route_manager/api/serializers.py b/scram/route_manager/api/serializers.py index fe0c7f78..1bca99b6 100644 --- a/scram/route_manager/api/serializers.py +++ b/scram/route_manager/api/serializers.py @@ -44,7 +44,7 @@ class Meta: class ClientSerializer(serializers.ModelSerializer): """Map the serializer to the model via Meta.""" - uuid = serializers.UUIDField() + uuid = serializers.UUIDField(required=False) class Meta: """Maps to the Client model, and specifies the fields exposed by the API.""" From 873d977949e50f76aff6cceadb8fcc6cd0f483d6 Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Fri, 19 Dec 2025 11:41:52 -0600 Subject: [PATCH 26/31] test(client-registration): make sure to test that we can create clients with UUID, create clients without a UUID provided, do an idempotent create, and not leak client_names --- .../tests/acceptance/features/client.feature | 19 ++++++++++++++++++- .../tests/acceptance/steps/common.py | 12 +++++++++++- 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/scram/route_manager/tests/acceptance/features/client.feature b/scram/route_manager/tests/acceptance/features/client.feature index 493c4531..ad80e35a 100644 --- a/scram/route_manager/tests/acceptance/features/client.feature +++ b/scram/route_manager/tests/acceptance/features/client.feature @@ -1,9 +1,26 @@ Feature: We can register and use clients - Scenario: We can register a client + Scenario: We can register a client with a UUID provided When we register a client named authorized_client.es.net with the uuid of 0e7e1cbd-7d73-4968-bc4b-ce3265dc2fd3 Then we get a 201 status code + Scenario: We can register a client without a UUID provided + When we register a client named authorized_client.es.net and no UUID + Then we get a 201 status code + + Scenario: We can do an idempotent create of a client with UUID + When we register a client named authorized_client2.es.net with the uuid of 1e24e7c3-f746-401e-8ae6-3fc3ff3aa814 + Then we get a 201 status code + When we register a client named authorized_client2.es.net with the uuid of 1e24e7c3-f746-401e-8ae6-3fc3ff3aa814 + Then we get a 200 status code + + Scenario: We do not leak client names + When we register a client named authorized_client3.es.net with the uuid of 9b3b9fee-3658-4e70-a52d-662f3b2d68ab + Then we get a 201 status code + When we register a client named authorized_client3.es.net with the uuid of da6845fb-f8a6-44ec-846a-54f0ee78fcf8 + Then we get a 400 status code + + Scenario: We can add a block using an authorized client Given a client with block authorization When we're logged in diff --git a/scram/route_manager/tests/acceptance/steps/common.py b/scram/route_manager/tests/acceptance/steps/common.py index 9436e609..dbaa2484 100644 --- a/scram/route_manager/tests/acceptance/steps/common.py +++ b/scram/route_manager/tests/acceptance/steps/common.py @@ -215,7 +215,7 @@ def check_object(context, value, model): @when("we register a client named {client_name} with the uuid of {uuid}") -def add_client(context, client_name, uuid): +def add_client_uuid(context, client_name, uuid): """Create a client with a specific UUID.""" context.response = context.test.client.post( reverse("api:v1:client-list"), @@ -224,3 +224,13 @@ def add_client(context, client_name, uuid): "uuid": uuid, }, ) + +@when("we register a client named {client_name} and no UUID") +def add_client_no_uuid(context, client_name): + """Create a client without a UUID.""" + context.response = context.test.client.post( + reverse("api:v1:client-list"), + { + "client_name": client_name, + } + ) From e2ee4acc67fc69f8496f4d6426c250f6a5b101a6 Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Fri, 19 Dec 2025 12:12:36 -0600 Subject: [PATCH 27/31] fix(update-client-code): fix client creation to allow for both creation with UUID and without (for our autocreate UUID code branch) also up our documentation game so we have much better info in DRF spectacular --- scram/route_manager/api/views.py | 90 ++++++++++++++++++++++++++------ 1 file changed, 75 insertions(+), 15 deletions(-) diff --git a/scram/route_manager/api/views.py b/scram/route_manager/api/views.py index 9f2ff2f9..491a97be 100644 --- a/scram/route_manager/api/views.py +++ b/scram/route_manager/api/views.py @@ -8,7 +8,7 @@ from django.conf import settings from django.core.exceptions import PermissionDenied from django.db.models import Q -from drf_spectacular.utils import extend_schema +from drf_spectacular.utils import extend_schema, OpenApiResponse from rest_framework import status, viewsets from rest_framework.exceptions import ValidationError from rest_framework.permissions import AllowAny, IsAuthenticated @@ -32,11 +32,15 @@ @extend_schema( - description="API endpoint for actiontypes", - responses={200: ActionTypeSerializer}, + description="API endpoint for actiontypes.", + responses={ + 200: OpenApiResponse(response=ActionTypeSerializer, description="Successful retrieval of actiontype(s)."), + 403: OpenApiResponse(description="Authentication credentials were not provided."), + 404: OpenApiResponse(description="The requested actiontype does not exist."), + }, ) class ActionTypeViewSet(viewsets.ReadOnlyModelViewSet): - """Lookup ActionTypes by name when authenticated, and bind to the serializer.""" + """Lookup ActionTypes by name, and bind to the serializer.""" queryset = ActionType.objects.all() permission_classes = (IsAuthenticated,) @@ -45,8 +49,15 @@ class ActionTypeViewSet(viewsets.ReadOnlyModelViewSet): @extend_schema( - description="API endpoint for ignore entries", - responses={200: IgnoreEntrySerializer}, + description="API endpoint for ignore entries.", + responses={ + 200: OpenApiResponse(response=IgnoreEntrySerializer, description="Successful retrieval or update of an ignore entry."), + 201: OpenApiResponse(response=IgnoreEntrySerializer, description="Ignore entry successfully created."), + 204: OpenApiResponse(description="Ignore entry successfully deleted."), + 400: OpenApiResponse(description="Invalid data provided."), + 403: OpenApiResponse(description="Authentication credentials were not provided."), + 404: OpenApiResponse(description="The requested ignore entry does not exist."), + }, ) class IgnoreEntryViewSet(viewsets.ModelViewSet): """Lookup IgnoreEntries by route when authenticated, and bind to the serializer.""" @@ -58,8 +69,17 @@ class IgnoreEntryViewSet(viewsets.ModelViewSet): @extend_schema( - description="API endpoint for clients", - responses={200: ClientSerializer}, + description="API endpoint for clients.", + responses={ + 200: OpenApiResponse( + response=ClientSerializer, + description="Client already existed and was retrieved successfully.", + ), + 201: OpenApiResponse(response=ClientSerializer, description="Client successfully created."), + 400: OpenApiResponse( + description="A client with this name may already exist with a different UUID, or client_name was not provided." + ), + }, ) class ClientViewSet(viewsets.ModelViewSet): """Lookup Client by client_name on POSTs regardless of authentication, and bind to the serializer.""" @@ -77,17 +97,50 @@ def perform_create(self, serializer): serializer.save(registered_from_ip=ip) def create(self, request, *args, **kwargs): - """Create a new Client or retrieve an existing one.""" + """Create a new Client while avoiding information leaks hopefully.""" client_name = request.data.get("client_name") - client = self.queryset.filter(client_name=client_name).first() + request_uuid = request.data.get("uuid") - if client: - serializer = self.get_serializer(client) - return Response(serializer.data, status=status.HTTP_200_OK) + if not client_name: + return Response({"detail": "client_name is required."}, status=status.HTTP_400_BAD_REQUEST) + + existing_client = self.queryset.filter(client_name=client_name).first() + + if existing_client: + if request_uuid and str(existing_client.uuid) == request_uuid: + # Idempotent success + serializer = self.get_serializer(existing_client) + return Response(serializer.data, status=status.HTTP_200_OK) + + # Log the conflict without leaking client_names to the user + logger.warning( + "Client registration conflict for name: %s. A client with this name already exists with a different UUID.", + client_name, + ) + return Response( + {"detail": "Invalid client registration request."}, + status=status.HTTP_400_BAD_REQUEST, + ) return super().create(request, *args, **kwargs) +@extend_schema( + description="API endpoint to check if a route is active.", + parameters=[ + { + "name": "cidr", + "required": True, + "type": "string", + "description": "The CIDR network to check (e.g., 192.0.2.0/24).", + "in": "query", + } + ], + responses={ + 200: OpenApiResponse(response=IsActiveSerializer, description="The 'is_active' field indicates the status of the route."), + 400: OpenApiResponse(description="The 'cidr' parameter is missing or invalid."), + }, +) class IsActiveViewSet(viewsets.ReadOnlyModelViewSet): """Look up a route to see if SCRAM considers it active or deactivated.""" @@ -134,8 +187,15 @@ def list(self, request): @extend_schema( - description="API endpoint for entries", - responses={200: EntrySerializer}, + description="API endpoint for entries.", + responses={ + 200: OpenApiResponse(response=EntrySerializer, description="Successful retrieval of an entry/entries."), + 201: OpenApiResponse(response=EntrySerializer, description="Entry successfully created."), + 204: OpenApiResponse(description="Entry successfully deleted."), + 400: OpenApiResponse(description="The route is likely on the ignore list or the prefix is too large."), + 403: OpenApiResponse(description="The client is not authorized for this action."), + 404: OpenApiResponse(description="The requested entry does not exist."), + }, ) class EntryViewSet(viewsets.ModelViewSet): """Lookup Entry when authenticated, and bind to the serializer.""" From d3d41607e8d3efb1960de326d0a672ba2a05d6d8 Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Fri, 19 Dec 2025 09:59:43 -0600 Subject: [PATCH 28/31] fix(process_updates): Fix expirations, cleanup syncing, and add integration tests (#157) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This one was a doozie. Expired/deleted entries on one SCRAM instance were never unblocked on other instances because there is no mechanism for postgres to let django know that it needs to re-run stuff, only the active django re-runs stuff. Now, we get around that by calling process_updates and re-updating anything changed recently, however, before, the sync logic only looked at creation time (`when` field), missing any entries that were removed.. To fix this in a lot less dumb way than we used to, we now we use `django-simple-history` to detect _**any**_ entry changes and then go ahead and just reprocess them to what the DB says they should be doing. There is a lot in this PR, but the main changes are: - Split up `process_updates()` into: - `get_entries_to_process(cutoff_time)` - This is a function that we could reuse that simply finds all recently modified entries from other instances and returns them so you can Do Stuff® on em. - `reprocess_entries(entries)` - This actually "Does Stuff" by sending websocket messages to translators. We could eventually move this out of here and make it more generic to be used everywhere we Do A Websocket® but it needs to be a bit more dynamic to support future action types (see note below, not dealing with this now). - `_check_for_orphaned_history()` - This is a total edge case, but since we don't ever delete entries (our delete() override) I wanted to make sure that we catch and log any orphaned history objects that don't have a corresponding entry (say someone goes into the admin panel and actually "hard" deletes an entry, but the history stuff is still there). - Made sure to be efficient with DB calls by using `select_related()`,`.exclude()`, and `values_list()` on most of our django/db queries to make sure that we're relying on SQL to do the filtering (using built in django stuff obviously) instead of iterating on the python side (in the case of prod where we have a TON of entries.) - We used to rely on `msg.msg_type` from WebSocketMessage (which is always `translator_add`), but now we set that based on `entry.is_active` when we run the sync from DB to translator. Eventually though this needs to be more dynamic to support other action types, not sure how to solve that though. There are a few caveats here to keep in mind, but for now i think it's fine: - This breaks from the existing behave patterns, and adds a pattern where we connect directly to the containers running via compose and ignores the test database. I tried to seperate this out into full integration tests. This means that test data is left behind so we blow away everything in the DB to reset it before and after test runs. - It's possible that a docker health check could run while the behave integration tests are running. This is unlikely and I made the health check run less frequently just to help avoid that but... it's gross. For now, I think this gets us further, so it's still good. It also fixes things to use postgres instead of sqlite, which is a win overall. # Conflicts: # scram/route_manager/tests/acceptance/steps/common.py # scram/route_manager/tests/acceptance/steps/ip.py --- .envs/.local/.django | 2 +- .github/workflows/ruff.yml | 2 +- .pre-commit-config.yaml | 10 +- Makefile | 9 +- compose.override.local.yml | 2 + docs/decisions.md | 13 +- pyproject.toml | 170 ++++++------ scram/route_manager/api/serializers.py | 1 + scram/route_manager/api/views.py | 14 +- .../features/syncing_entries.feature | 17 -- .../tests/acceptance/steps/common.py | 3 +- .../tests/acceptance/steps/syncing.py | 50 ---- .../tests/integration/environment.py | 61 +++++ .../features/multi_instance_sync.feature | 53 ++++ .../tests/integration/steps/common.py | 3 + .../tests/integration/steps/ip.py | 3 + .../tests/integration/steps/multi_instance.py | 244 ++++++++++++++++++ .../tests/test_common/__init__.py | 1 + .../tests/test_common/steps_common.py | 234 +++++++++++++++++ .../tests/test_common/steps_ip.py | 77 ++++++ .../tests/test_process_updates.py | 210 +++++++++++++++ scram/route_manager/views.py | 120 +++++++-- 22 files changed, 1110 insertions(+), 189 deletions(-) delete mode 100644 scram/route_manager/tests/acceptance/features/syncing_entries.feature delete mode 100644 scram/route_manager/tests/acceptance/steps/syncing.py create mode 100644 scram/route_manager/tests/integration/environment.py create mode 100644 scram/route_manager/tests/integration/features/multi_instance_sync.feature create mode 100644 scram/route_manager/tests/integration/steps/common.py create mode 100644 scram/route_manager/tests/integration/steps/ip.py create mode 100644 scram/route_manager/tests/integration/steps/multi_instance.py create mode 100644 scram/route_manager/tests/test_common/__init__.py create mode 100644 scram/route_manager/tests/test_common/steps_common.py create mode 100644 scram/route_manager/tests/test_common/steps_ip.py create mode 100644 scram/route_manager/tests/test_process_updates.py diff --git a/.envs/.local/.django b/.envs/.local/.django index 7c979d82..a625f808 100644 --- a/.envs/.local/.django +++ b/.envs/.local/.django @@ -6,5 +6,5 @@ IPYTHONDIR=/app/.ipython # ------------------------------------------------------------------------------ # Django -DATABASE_URL=sqlite:///django.db +DATABASE_URL=postgres://debug:debug@postgres:5432/scram DJANGO_SETTINGS_MODULE=config.settings.local diff --git a/.github/workflows/ruff.yml b/.github/workflows/ruff.yml index 31647732..2c005a9a 100644 --- a/.github/workflows/ruff.yml +++ b/.github/workflows/ruff.yml @@ -20,7 +20,7 @@ jobs: uses: actions/checkout@v4 - name: Install dependencies - run: pip install ruff + run: pip install ruff==0.14.10 - name: Fail if we have any style errors run: ruff check --output-format=github diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index f5bc8d3a..5315066a 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -1,5 +1,5 @@ exclude: 'docs|migrations|.git|.tox' -default_stages: [pre-commit] +default_stages: [commit] fail_fast: true repos: @@ -10,10 +10,8 @@ repos: - id: end-of-file-fixer - repo: https://github.com/astral-sh/ruff-pre-commit - rev: v0.8.0 + rev: v0.14.10 hooks: - # Run the linter. - - id: ruff - args: [ --fix ] - # Run the formatter. - id: ruff-format + - id: ruff-check + args: [--fix, --exit-non-zero-on-fix] diff --git a/Makefile b/Makefile index 3913183d..9c9d9b13 100644 --- a/Makefile +++ b/Makefile @@ -33,6 +33,11 @@ behave-all: compose.override.yml behave: compose.override.yml @docker compose run --rm django python manage.py behave --no-input --simple -i $(FEATURE) +## integration-tests: runs multi-instance system tests against docker compose running containers +.Phony: integration-tests +integration-tests: run + @docker compose exec -T django coverage run -a manage.py behave --no-input --use-existing-database scram/route_manager/tests/integration + ## behave-translator .Phony: behave-translator behave-translator: compose.override.yml @@ -46,11 +51,11 @@ build: compose.override.yml @docker compose restart $(CONTAINER) ## coverage.xml: generate coverage from test runs -coverage.xml: pytest behave-all behave-translator +coverage.xml: pytest behave-all integration-tests behave-translator @docker compose run --rm django coverage report @docker compose run --rm django coverage xml -## ci-test: runs all tests just like Gitlab CI does +## ci-test: runs all tests just like Github CI does .Phony: ci-test ci-test: | toggle-local build migrate run coverage.xml diff --git a/compose.override.local.yml b/compose.override.local.yml index 6c40ff96..c1fb81e4 100644 --- a/compose.override.local.yml +++ b/compose.override.local.yml @@ -19,6 +19,7 @@ services: - ./.envs/.local/.postgres healthcheck: test: ["CMD", "curl", "-f", "http://django:8000/process_updates/"] + interval: 120s ports: - "8000" - 56780:56780 @@ -40,6 +41,7 @@ services: - ./.envs/.local/.postgres healthcheck: test: ["CMD", "curl", "-f", "http://django-secondary:8000/process_updates/"] + interval: 120s ports: - "8000" - 56781:56780 diff --git a/docs/decisions.md b/docs/decisions.md index 5be1c7d1..a98cc70c 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -79,11 +79,18 @@ If you want two or more instances of SCRAM to share data between themselves we h 3. For normal syncing where both translators have been connected, we are currently using process_updates (since it runs regularly) to grab new data out of the database that comes from other connected instances and reannounces those locally. -Honestly, step 3 is kind of gross and we realize this. We are probably looking at a task runner or something to handle -this moving forward, but we needed to get this fixed in the meantime. Status can be tracked with -[Github Issue 125](https://github.com/esnet-security/SCRAM/issues/125) +##### The Unblocking Problem® (PR #157) + +We had a bug where entries that had expired or been deleted on one SCRAM instance were never being unblocked on other instances. The original sync logic only looked at the `when` field (creation time), so it would happily find new entries but completely miss any that were modified, expired, or de-activated after creation. This was bad because things could get stuck being blocked forever (until gobgp and translator are restarted) and not show up in the web UI (because the database says it's not blocked.) Eventually, we should add "ghost" routes to the UI to show someone if there is something that's advertised by its translator but not in the database. We could even use a cute little ghost icon for it! + +The fix takes advantage of the already existing `django-simple-history` models that track any entry modifications. To sync, we query the history models to see if something has changed in any way, and we just go ahead and re-send that to the translator, ensuring eventual consistency (since translator is idempotent). + +##### Future Work + +Honestly, the use of compose health checks is kind of gross and we realize this. The `process_updates` polling approach works, but it's not elegant. We're probably looking at Celery or some other task runner to handle this properly. it'd be good to have something that can react to database changes on a message bus rather than polling every 30 seconds. But this gets things fixed for now, and the history-based seems solid enough for our needs. #### Entries Page + We intentionally chose to only list the active entries. Our thinking is that the home page shows the most recent additions. Then, if you went to the entries page, it would be overwhelmingly huge to show all the historical entries including the ones that timed out/were deactivated. If you wanted to know about a specific entry even if it were not currently active diff --git a/pyproject.toml b/pyproject.toml index c7370eb1..2646174f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,117 +1,119 @@ # ==== pytest ==== [tool.pytest.ini_options] -minversion = "6.0" addopts = "--ds=config.settings.test --reuse-db" +minversion = "6.0" python_files = [ - "tests.py", - "test_*.py", + "tests.py", + "test_*.py", ] # ==== Coverage ==== [tool.coverage.run] +branch = true +data_file = "coverage.coverage" include = ["scram/*", "config/*", "translator/*"] omit = ["**/migrations/*", "scram/contrib/*", "*/tests/*"] plugins = ["django_coverage_plugin"] -branch = true -data_file = "coverage.coverage" [tool.coverage.report] exclude_also = [ - "if debug:", - "if self.debug:", - "if settings.DEBUG", - "raise AssertionError", - "raise NotImplementedError", - "if __name__ == .__main__.:", - ] + "if debug:", + "if self.debug:", + "if settings.DEBUG", + "raise AssertionError", + "raise NotImplementedError", + "if __name__ == .__main__.:", +] # ===== ruff ==== [tool.ruff] exclude = [ - "migrations", + "migrations", ] line-length = 119 -target-version = 'py312' preview = true +target-version = 'py312' [tool.ruff.lint] -select = [ - "A", # builtins - "ASYNC", # async - "B", # bugbear - "BLE", # blind-except - "C4", # comprehensions - "C90", # complexity - "COM", # commas - "D", # pydocstyle - "DJ", # django - "DOC", # pydoclint - "DTZ", # datetimez - "E", # pycodestyle - "EM", # errmsg - "ERA", # eradicate - "F", # pyflakes - "FBT", # boolean-trap - "FLY", # flynt - "G", # logging-format - "I", # isort - "ICN", # import-conventions - "ISC", # implicit-str-concat - "LOG", # logging - "N", # pep8-naming - "PERF", # perflint - "PIE", # pie - "PL", # pylint - "PTH", # use-pathlib - "Q", # quotes - "RET", # return - "RSE", # raise - "RUF", # Ruff - "S", # bandit - "SIM", # simplify - "SLF", # self - "SLOT", # slots - "T20", # print - "TRY", # tryceratops - "UP", # pyupgrade -] ignore = [ - "COM812", # handled by the formatter - "DOC501", # add possible exceptions to the docstring (TODO) - "ISC001", # handled by the formatter - "RUF012", # need more widespread typing - "SIM102", # use a single `if` statement instead of nested `if` statements - "SIM108", # use ternary operator instead of `if`-`else`-block - "PERF401", # list comprehensions are harder to read in our opinion and not worth the performance gain - "PERF403", # dict comprehensions are harder to read in our opinion and not worth the performance gain + "COM812", # handled by the formatter + "DOC501", # add possible exceptions to the docstring (TODO) + "ISC001", # handled by the formatter + "RUF012", # need more widespread typing + "SIM102", # use a single `if` statement instead of nested `if` statements + "SIM108", # use ternary operator instead of `if`-`else`-block + "PERF401", # list comprehensions are harder to read in our opinion and not worth the performance gain + "PERF403", # dict comprehensions are harder to read in our opinion and not worth the performance gain +] +select = [ + "A", # builtins + "ASYNC", # async + "B", # bugbear + "BLE", # blind-except + "C4", # comprehensions + "C90", # complexity + "COM", # commas + "D", # pydocstyle + "DJ", # django + "DOC", # pydoclint + "DTZ", # datetimez + "E", # pycodestyle + "EM", # errmsg + "ERA", # eradicate + "F", # pyflakes + "FBT", # boolean-trap + "FLY", # flynt + "G", # logging-format + "I", # isort + "ICN", # import-conventions + "ISC", # implicit-str-concat + "LOG", # logging + "N", # pep8-naming + "PERF", # perflint + "PIE", # pie + "PL", # pylint + "PTH", # use-pathlib + "Q", # quotes + "RET", # return + "RSE", # raise + "RUF", # Ruff + "S", # bandit + "SIM", # simplify + "SLF", # self + "SLOT", # slots + "T20", # print + "TRY", # tryceratops + "UP", # pyupgrade ] [tool.ruff.lint.mccabe] max-complexity = 7 # our current code adheres to this without too much effort [tool.ruff.lint.per-file-ignores] -"**/{tests}/*" = [ - "DOC201", # documenting return values - "DOC402", # documenting yield values - "PLR6301", # could be a static method - "S101", # use of assert - "S106", # hardcoded password - "PLR2004" # magic value used in comparison +"**/tests/**" = [ + "C901", # function is too complex (applies to test helpers) + "DOC201", # documenting return values + "DOC402", # documenting yield values + "FBT002", # boolean default argument in function definition + "PLR6301", # could be a static method + "S101", # use of assert + "S106", # hardcoded password + "PLR2004", # magic value used in comparison ] -"test.py" = [ - "S105", # hardcoded password as argument +"**/views.py" = [ + "DOC201", # documenting return values; it's fairly obvious in a View ] "scram/route_manager/**" = [ - "DOC201", # documenting return values + "DOC201", # documenting return values ] "scram/users/**" = [ - "DOC201", # documenting return values - "FBT001", # minimal issue; don't need to mess with in the User app - "PLR2004", # magic values when checking HTTP status codes + "DOC201", # documenting return values + "FBT001", # minimal issue; don't need to mess with in the User app + "PLR2004", # magic values when checking HTTP status codes ] -"**/views.py" = [ - "DOC201", # documenting return values; it's fairly obvious in a View +"test.py" = [ + "S105", # hardcoded password as argument ] [tool.ruff.lint.pydocstyle] @@ -119,21 +121,21 @@ convention = "google" # ==== mypy ==== [tool.mypy] -python_version = "3.11" check_untyped_defs = true ignore_missing_imports = true -warn_unused_ignores = true -warn_redundant_casts = true -warn_unused_configs = true plugins = [ - "mypy_django_plugin.main", - "mypy_drf_plugin.main", + "mypy_django_plugin.main", + "mypy_drf_plugin.main", ] +python_version = "3.11" +warn_redundant_casts = true +warn_unused_configs = true +warn_unused_ignores = true [[tool.mypy.overrides]] # Django migrations should not produce any errors: -module = "*.migrations.*" ignore_errors = true +module = "*.migrations.*" [tool.django-stubs] django_settings_module = "config.settings.test" diff --git a/scram/route_manager/api/serializers.py b/scram/route_manager/api/serializers.py index 1bca99b6..3bc3df1a 100644 --- a/scram/route_manager/api/serializers.py +++ b/scram/route_manager/api/serializers.py @@ -44,6 +44,7 @@ class Meta: class ClientSerializer(serializers.ModelSerializer): """Map the serializer to the model via Meta.""" + uuid = serializers.UUIDField(required=False) class Meta: diff --git a/scram/route_manager/api/views.py b/scram/route_manager/api/views.py index 491a97be..0c194b10 100644 --- a/scram/route_manager/api/views.py +++ b/scram/route_manager/api/views.py @@ -8,7 +8,7 @@ from django.conf import settings from django.core.exceptions import PermissionDenied from django.db.models import Q -from drf_spectacular.utils import extend_schema, OpenApiResponse +from drf_spectacular.utils import OpenApiResponse, extend_schema from rest_framework import status, viewsets from rest_framework.exceptions import ValidationError from rest_framework.permissions import AllowAny, IsAuthenticated @@ -51,7 +51,9 @@ class ActionTypeViewSet(viewsets.ReadOnlyModelViewSet): @extend_schema( description="API endpoint for ignore entries.", responses={ - 200: OpenApiResponse(response=IgnoreEntrySerializer, description="Successful retrieval or update of an ignore entry."), + 200: OpenApiResponse( + response=IgnoreEntrySerializer, description="Successful retrieval or update of an ignore entry." + ), 201: OpenApiResponse(response=IgnoreEntrySerializer, description="Ignore entry successfully created."), 204: OpenApiResponse(description="Ignore entry successfully deleted."), 400: OpenApiResponse(description="Invalid data provided."), @@ -77,7 +79,7 @@ class IgnoreEntryViewSet(viewsets.ModelViewSet): ), 201: OpenApiResponse(response=ClientSerializer, description="Client successfully created."), 400: OpenApiResponse( - description="A client with this name may already exist with a different UUID, or client_name was not provided." + description="A client with this name already exists with a different UUID, or client_name was not provided." ), }, ) @@ -114,7 +116,7 @@ def create(self, request, *args, **kwargs): # Log the conflict without leaking client_names to the user logger.warning( - "Client registration conflict for name: %s. A client with this name already exists with a different UUID.", + "Client named: %s exists with different UUID", client_name, ) return Response( @@ -137,7 +139,9 @@ def create(self, request, *args, **kwargs): } ], responses={ - 200: OpenApiResponse(response=IsActiveSerializer, description="The 'is_active' field indicates the status of the route."), + 200: OpenApiResponse( + response=IsActiveSerializer, description="The 'is_active' field indicates the status of the route." + ), 400: OpenApiResponse(description="The 'cidr' parameter is missing or invalid."), }, ) diff --git a/scram/route_manager/tests/acceptance/features/syncing_entries.feature b/scram/route_manager/tests/acceptance/features/syncing_entries.feature deleted file mode 100644 index 4d8d0e10..00000000 --- a/scram/route_manager/tests/acceptance/features/syncing_entries.feature +++ /dev/null @@ -1,17 +0,0 @@ -Feature: Syncing Entries from Other Instances - - Scenario: Entry added from another instance is ingested and announced - Given a block actiontype is defined - And a client with block authorization - When we're logged in - And an entry 192.168.0.10/32 from scram instance "scram2.example.com" is added to the database - When process_updates is run - Then 192.168.0.10/32 is announced by block translators - - Scenario: Don't reannounce entries - Given a block actiontype is defined - And a client with block authorization - When we're logged in - And an entry 192.168.0.20/32 from scram instance "current" is added to the database - When process_updates is run - Then the entry 192.168.0.20/32 should not be announced again diff --git a/scram/route_manager/tests/acceptance/steps/common.py b/scram/route_manager/tests/acceptance/steps/common.py index dbaa2484..2278d0b4 100644 --- a/scram/route_manager/tests/acceptance/steps/common.py +++ b/scram/route_manager/tests/acceptance/steps/common.py @@ -225,6 +225,7 @@ def add_client_uuid(context, client_name, uuid): }, ) + @when("we register a client named {client_name} and no UUID") def add_client_no_uuid(context, client_name): """Create a client without a UUID.""" @@ -232,5 +233,5 @@ def add_client_no_uuid(context, client_name): reverse("api:v1:client-list"), { "client_name": client_name, - } + }, ) diff --git a/scram/route_manager/tests/acceptance/steps/syncing.py b/scram/route_manager/tests/acceptance/steps/syncing.py deleted file mode 100644 index 95e5ba5c..00000000 --- a/scram/route_manager/tests/acceptance/steps/syncing.py +++ /dev/null @@ -1,50 +0,0 @@ -"""Define steps used for syncing-related logic by the Behave tests.""" - -from behave import step, then, when -from django.conf import settings -from django.urls import reverse - -from scram.route_manager.models import ActionType, Entry, Route - - -@step("an entry {ip} from scram instance {instance_name} is added to the database") -def add_entry_with_instance_name(context, ip, instance_name): - """Creates an entry in the database with the specified data and originating instance.""" - if instance_name == "current": - instance_name = settings.SCRAM_HOSTNAME - - route = Route(route=ip) - route.save() - at = ActionType.objects.get(pk=1) - - entry_data = { - "route": route, - "is_active": True, - "actiontype": at, - "comment": "behave sync blocking", - "who": "behave", - "originating_scram_instance": instance_name, - } - - entry = Entry(**entry_data) - entry.save() - context.entry = entry - - -@when("process_updates is run") -def run_process_updates(context): - """Runs the process_updates view.""" - context.response = context.test.client.get(reverse("route_manager:process-updates")) - - -@then("the entry {ip} should not be announced again") -def check_not_announced(context, ip): - """Checks if the entry has not been re-announced because this instance originally announced it.""" - testing_hostname = settings.SCRAM_HOSTNAME - - try: - entry = Entry.objects.get(route__route=ip, originating_scram_instance=testing_hostname) - except Entry.DoesNotExist: - return - - assert entry.is_active is False, f"Entry {ip} was unexpectedly announced" diff --git a/scram/route_manager/tests/integration/environment.py b/scram/route_manager/tests/integration/environment.py new file mode 100644 index 00000000..aca60e8e --- /dev/null +++ b/scram/route_manager/tests/integration/environment.py @@ -0,0 +1,61 @@ +"""Setup the environment for integration tests.""" +# Note: this kinda abuses behave as just a generic test runner, but it works for now. + +from django.contrib.auth import get_user_model + +from scram.route_manager.models import ( + ActionType, + Entry, + HistoricalEntry, + Route, + WebSocketMessage, + WebSocketSequenceElement, +) +from scram.route_manager.models import ( + Client as DjangoClient, +) + +TEST_USER_USERNAME = "user" +TEST_USER_PASSWORD = "password" # noqa: S105 #gitleaks: allow +TEST_CLIENT_UUID = "0e7e1cbd-7d73-4968-bc4b-ce3265dc2fd3" +TEST_CLIENT_HOSTNAME = "authorized_client.es.net" + + +def before_feature(context, feature): + """Clean up database and setup test user and client.""" + user_model = get_user_model() + + # Cleanup any data related to what integration tests will create to avoid conflicts. + Entry.objects.all().delete() + Route.objects.all().delete() + HistoricalEntry.objects.all().delete() + WebSocketSequenceElement.objects.all().delete() + WebSocketMessage.objects.all().delete() + + # Create test user + user, _ = user_model.objects.get_or_create( + username=TEST_USER_USERNAME, + ) + user.set_password(TEST_USER_PASSWORD) + user.save() + + # Create test client with authorization to block (reused across tests) + client, _ = DjangoClient.objects.update_or_create( + uuid=TEST_CLIENT_UUID, + defaults={ + "hostname": TEST_CLIENT_HOSTNAME, + "is_authorized": True, + }, + ) + action_type, _ = ActionType.objects.get_or_create(name="block") + client.authorized_actiontypes.clear() + client.authorized_actiontypes.add(action_type) + + +def after_feature(context, feature): + """Cleanup test data related to what we created during tests.""" + Entry.objects.all().delete() + Route.objects.all().delete() + HistoricalEntry.objects.all().delete() + WebSocketSequenceElement.objects.all().delete() + WebSocketMessage.objects.all().delete() diff --git a/scram/route_manager/tests/integration/features/multi_instance_sync.feature b/scram/route_manager/tests/integration/features/multi_instance_sync.feature new file mode 100644 index 00000000..dee31f2e --- /dev/null +++ b/scram/route_manager/tests/integration/features/multi_instance_sync.feature @@ -0,0 +1,53 @@ +Feature: Multi-Instance Synchronization + + Scenario: Entry created on primary syncs to secondary (IPv4) + Given a block actiontype is defined + When we create an entry 192.0.2.1/32 on primary instance + And we wait for database commit + When secondary instance runs process_updates + Then secondary announces 192.0.2.1/32 addition to block translators + + Scenario: Entry created on primary syncs to secondary (IPv6) + Given a block actiontype is defined + When we create an entry 2001:db8::10/128 on primary instance + And we wait for database commit + When secondary instance runs process_updates + Then secondary announces 2001:db8::10/128 addition to block translators + + Scenario: Entry expired on primary syncs removal to secondary (IPv4) + Given a block actiontype is defined + When we create entry 192.0.2.2/32 with 1 second expiration on primary instance + And we wait 2 seconds for expiration + When primary instance runs process_updates to expire entries + And we wait for database commit + When secondary instance runs process_updates + Then the entry 192.0.2.2/32 is inactive on secondary instance + And secondary announces 192.0.2.2/32 removal to block translators + + Scenario: Entry expired on primary syncs removal to secondary (IPv6) + Given a block actiontype is defined + When we create entry 2001:db8::20/128 with 1 second expiration on primary instance + And we wait 2 seconds for expiration + When primary instance runs process_updates to expire entries + And we wait for database commit + When secondary instance runs process_updates + Then the entry 2001:db8::20/128 is inactive on secondary instance + And secondary announces 2001:db8::20/128 removal to block translators + + Scenario: Entry deactivated on primary syncs to secondary (IPv4) + Given a block actiontype is defined + When we create an entry 192.0.2.3/32 on primary instance + And we deactivate entry 192.0.2.3/32 on primary instance + And we wait for database commit + When secondary instance runs process_updates + Then the entry 192.0.2.3/32 is inactive on secondary instance + And secondary announces 192.0.2.3/32 removal to block translators + + Scenario: Entry deactivated on primary syncs to secondary (IPv6) + Given a block actiontype is defined + When we create an entry 2001:db8::30/128 on primary instance + And we deactivate entry 2001:db8::30/128 on primary instance + And we wait for database commit + When secondary instance runs process_updates + Then the entry 2001:db8::30/128 is inactive on secondary instance + And secondary announces 2001:db8::30/128 removal to block translators diff --git a/scram/route_manager/tests/integration/steps/common.py b/scram/route_manager/tests/integration/steps/common.py new file mode 100644 index 00000000..fb491768 --- /dev/null +++ b/scram/route_manager/tests/integration/steps/common.py @@ -0,0 +1,3 @@ +"""Import the shared test_common steps. Add all new shared steps go in steps_common.""" + +from scram.route_manager.tests.test_common.steps_common import * # noqa: F403 diff --git a/scram/route_manager/tests/integration/steps/ip.py b/scram/route_manager/tests/integration/steps/ip.py new file mode 100644 index 00000000..7e2ad9cd --- /dev/null +++ b/scram/route_manager/tests/integration/steps/ip.py @@ -0,0 +1,3 @@ +"""Import in the shared test_common IP related steps. Add all new shared IP steps go there.""" + +from scram.route_manager.tests.test_common.steps_ip import * # noqa: F403 diff --git a/scram/route_manager/tests/integration/steps/multi_instance.py b/scram/route_manager/tests/integration/steps/multi_instance.py new file mode 100644 index 00000000..611d0ea4 --- /dev/null +++ b/scram/route_manager/tests/integration/steps/multi_instance.py @@ -0,0 +1,244 @@ +"""Define steps for multi-instance testing that actually hit both running containers.""" + +import datetime +import time +import urllib.parse + +import requests +from behave import then, when + +DJANGO_PRIMARY_URL = "http://django:8000" +DJANGO_SECONDARY_URL = "http://django-secondary:8000" +TEST_CLIENT_UUID = "0e7e1cbd-7d73-4968-bc4b-ce3265dc2fd3" + + +def get_auth_token(base_url: str = DJANGO_PRIMARY_URL): + """Obtain an API authentication token for the test user.""" + response = requests.post(f"{base_url}/auth-token/", data={"username": "user", "password": "password"}, timeout=10) + response.raise_for_status() + return response.json()["token"] + + +@when("we create an entry {ip:S} on primary instance") +def create_entry_on_primary(context, ip): + """Creates an entry via API call to the primary container.""" + try: + response = requests.post( + f"{DJANGO_PRIMARY_URL}/api/v1/entries/", + json={ + "route": ip, + "actiontype": "block", + "comment": "behave multi-instance test", + "who": "behave", + "uuid": TEST_CLIENT_UUID, + }, + timeout=10, + ) + response.raise_for_status() + context.response = response + context.test_ip = ip + except requests.exceptions.RequestException as e: + context.test.fail(f"Failed to call primary instance API: {e}") + + +@when("we create entry {ip:S} with {seconds:d} second expiration on primary instance") +def create_entry_with_expiration(context, ip, seconds): + """Creates an entry with expiration via API call to the primary container.""" + td = datetime.timedelta(seconds=int(seconds)) + expiration = (datetime.datetime.now(tz=datetime.UTC) + td).isoformat() + + try: + response = requests.post( + f"{DJANGO_PRIMARY_URL}/api/v1/entries/", + json={ + "route": ip, + "actiontype": "block", + "comment": "behave multi-instance expiration test", + "expiration": expiration, + "who": "behave", + "uuid": TEST_CLIENT_UUID, + }, + timeout=10, + ) + response.raise_for_status() + context.response = response + context.test_ip = ip + except requests.exceptions.RequestException as e: + context.test.fail(f"Failed to call primary instance API: {e}") + + +@when("we deactivate entry {ip:S} on primary instance") +def deactivate_entry_on_primary(context, ip): + """Deactivates an entry via API call to the primary container.""" + if not hasattr(context, "auth_token"): + context.auth_token = get_auth_token() + + try: + list_response = requests.get( + f"{DJANGO_PRIMARY_URL}/api/v1/entries/", + headers={"Authorization": f"Token {context.auth_token}"}, + timeout=10, + ) + list_response.raise_for_status() + + entries = list_response.json().get("results", []) + entry_route = None + for entry in entries: + if entry.get("route") == ip: + entry_route = entry["route"] + break + + if not entry_route: + context.test.fail(f"Entry {ip} not found in API response") + + encoded_route = urllib.parse.quote(entry_route, safe="") + response = requests.delete( + f"{DJANGO_PRIMARY_URL}/api/v1/entries/{encoded_route}/", + headers={"Authorization": f"Token {context.auth_token}"}, + timeout=10, + ) + response.raise_for_status() + context.response = response + except requests.exceptions.RequestException as e: + context.test.fail(f"Failed to call primary instance API: {e}") + + +@when("we wait for database commit") +def wait_for_database_commit(context): + """Wait briefly to ensure database changes are committed.""" + time.sleep(2) + + +@when("we wait {seconds:d} seconds for expiration") +def wait_for_expiration(context, seconds): + """Waits for an entry to expire.""" + time.sleep(int(seconds)) + + +@when("secondary instance runs process_updates") +def secondary_runs_process_updates(context): + """Makes API call to secondary instance's process_updates endpoint.""" + try: + response = requests.get(f"{DJANGO_SECONDARY_URL}/process_updates/", timeout=10) + response.raise_for_status() + context.secondary_response = response + context.secondary_process_data = response.json() + except requests.exceptions.RequestException as e: + context.test.fail(f"Failed to call secondary instance process_updates: {e}") + + +@when("primary instance runs process_updates to expire entries") +def primary_runs_process_updates(context): + """Runs process_updates on the primary instance via API.""" + try: + response = requests.get(f"{DJANGO_PRIMARY_URL}/process_updates/", timeout=10) + response.raise_for_status() + context.primary_response = response + context.primary_process_data = response.json() + except requests.exceptions.RequestException as e: + context.test.fail(f"Failed to call primary instance process_updates: {e}") + + +@then("the entry {ip:S} is inactive on secondary instance") +def check_entry_inactive_on_secondary(context, ip): + """Checks if entry is inactive by verifying it's NOT in the active entries. + + Note that this isn't whenwe run process_updates on this instance, so don't use this step out of context. + If we mock websockets inthe future, then we can test this a bit better. + """ + if not hasattr(context, "auth_token"): + context.auth_token = get_auth_token(DJANGO_SECONDARY_URL) + + try: + list_response = requests.get( + f"{DJANGO_SECONDARY_URL}/api/v1/entries/", + headers={"Authorization": f"Token {context.auth_token}"}, + timeout=10, + ) + list_response.raise_for_status() + + entries = list_response.json().get("results", []) + for entry in entries: + if entry.get("route") == ip: + context.test.fail(f"Entry {ip} should be inactive but was found in active entries list") + + # If we get here, the entry was not found in active entries, which is correct + except requests.exceptions.RequestException as e: + context.test.fail(f"Failed to call API: {e}") + + +@then("secondary announces {ip:S} addition to block translators") +def check_announced_on_secondary(context, ip): + """Verifies entry was processed and announced to translators.""" + if not hasattr(context, "auth_token"): + context.auth_token = get_auth_token(DJANGO_SECONDARY_URL) + + # First verify the entry exists and is active via API + try: + list_response = requests.get( + f"{DJANGO_SECONDARY_URL}/api/v1/entries/", + headers={"Authorization": f"Token {context.auth_token}"}, + timeout=10, + ) + list_response.raise_for_status() + + entries = list_response.json().get("results", []) + entry_found = False + for entry in entries: + if entry.get("route") == ip: + entry_found = True + assert entry.get("is_active") is True, f"Entry {ip} should be active" + break + + assert entry_found, f"Entry {ip} not found in API response" + except requests.exceptions.RequestException as e: + context.test.fail(f"Failed to call API: {e}") + + # Verify process_updates actually processed this specific entry + if hasattr(context, "secondary_process_data"): + reprocessed_list = context.secondary_process_data.get("entries_reprocessed_list", []) + secondary_hostname = context.secondary_process_data.get("scram_hostname", "UNKNOWN") + + originating_instance = None + for entry in entries: + if entry.get("route") == ip: + originating_instance = entry.get("originating_scram_instance", "UNKNOWN") + break + assert ip in reprocessed_list, ( + f"Expected {ip} in reprocessed list, got {reprocessed_list}. " + f"Secondary hostname: {secondary_hostname}, Entry origin: {originating_instance}" + ) + + +@then("secondary announces {ip:S} removal to block translators") +def check_removal_announced_on_secondary(context, ip): + """Verifies entry removal was processed and announced to translators.""" + if not hasattr(context, "auth_token"): + context.auth_token = get_auth_token(DJANGO_SECONDARY_URL) + + # First verify the entry is NOT in the active entries list (since it was removed) + try: + list_response = requests.get( + f"{DJANGO_SECONDARY_URL}/api/v1/entries/", + headers={"Authorization": f"Token {context.auth_token}"}, + timeout=10, + ) + list_response.raise_for_status() + + entries = list_response.json().get("results", []) + for entry in entries: + if entry.get("route") == ip: + context.test.fail(f"Entry {ip} should be inactive but was found in active entries list") + + # Entry is not in active list, which is correct for a removed entry + except requests.exceptions.RequestException as e: + context.test.fail(f"Failed to call API: {e}") + + # Verify process_updates actually processed this specific entry + process_data = getattr(context, "secondary_process_data", None) or getattr(context, "primary_process_data", {}) + if process_data: + reprocessed_list = process_data.get("entries_reprocessed_list", []) + hostname = process_data.get("scram_hostname", "UNKNOWN") + assert ip in reprocessed_list, ( + f"Expected {ip} in reprocessed list, got {reprocessed_list}. Instance hostname: {hostname}" + ) diff --git a/scram/route_manager/tests/test_common/__init__.py b/scram/route_manager/tests/test_common/__init__.py new file mode 100644 index 00000000..21120074 --- /dev/null +++ b/scram/route_manager/tests/test_common/__init__.py @@ -0,0 +1 @@ +"""Shared test framework for acceptance and integration tests.""" diff --git a/scram/route_manager/tests/test_common/steps_common.py b/scram/route_manager/tests/test_common/steps_common.py new file mode 100644 index 00000000..52efe8b1 --- /dev/null +++ b/scram/route_manager/tests/test_common/steps_common.py @@ -0,0 +1,234 @@ +"""Define shared steps used by the Behave tests.""" + +import datetime +import time + +from asgiref.sync import async_to_sync +from behave import given, step, then, when +from channels.layers import get_channel_layer +from django import conf +from django.urls import reverse + +from scram.route_manager.models import ActionType, Client, WebSocketMessage, WebSocketSequenceElement + + +@given("a {name} actiontype is defined") +def create_actiontype(context, name): + """Create an actiontype of that name.""" + context.channel_layer = get_channel_layer() + async_to_sync(context.channel_layer.group_send)( + f"translator_{name}", + {"type": "translator_remove_all", "message": {}}, + ) + + at, _ = ActionType.objects.get_or_create(name=name) + wsm, _ = WebSocketMessage.objects.get_or_create(msg_type="translator_add", msg_data_route_field="route") + wsm.save() + wsse, _ = WebSocketSequenceElement.objects.get_or_create(websocketmessage=wsm, verb="A", action_type=at) + wsse.save() + + +@given("a client with {name} authorization") +def create_authed_client(context, name): + """Create a client and authorize it for that action type.""" + at, _ = ActionType.objects.get_or_create(name=name) + authorized_client, _ = Client.objects.update_or_create( + uuid="0e7e1cbd-7d73-4968-bc4b-ce3265dc2fd3", + defaults={ + "hostname": "authorized_client.es.net", + "is_authorized": True, + }, + ) + authorized_client.authorized_actiontypes.set([at]) + context.client = authorized_client + + +@given("a client without {name} authorization") +def create_unauthed_client(context, name): + """Create a client that has no authorized action types.""" + unauthorized_client = Client.objects.create( + hostname="unauthorized_client.es.net", + uuid="91e134a5-77cf-4560-9797-6bbdbffde9f8", + ) + unauthorized_client.authorized_actiontypes.set([]) + + +@when("we're logged in") +def login(context): + """Login.""" + context.test.client.login(username="user", password="password") + context.test.web_client.login(username="user", password="password") + + +@when("the CIDR prefix limits are {v4_minprefix:d} and {v6_minprefix:d}") +def set_cidr_limit(context, v4_minprefix, v6_minprefix): + """Override our settings with the provided values.""" + conf.settings.V4_MINPREFIX = v4_minprefix + conf.settings.V6_MINPREFIX = v6_minprefix + + +@then("we get a {status_code:d} status code") +def check_status_code(context, status_code): + """Ensure the status code response matches the expected value.""" + context.test.assertEqual(context.response.status_code, status_code) + + +@when("we add the entry {value:S}") +def add_entry(context, value): + """Block the provided route.""" + context.response = context.test.client.post( + reverse("api:v1:entry-list"), + { + "route": value, + "actiontype": "block", + "comment": "behave", + # Authorized uuid + "uuid": "0e7e1cbd-7d73-4968-bc4b-ce3265dc2fd3", + "who": "person", + }, + format="json", + ) + + +@when("we add the entry {value:S} with comment {comment}") +def add_entry_with_comment(context, value, comment): + """Block the provided route and add a comment.""" + context.response = context.test.client.post( + reverse("api:v1:entry-list"), + { + "route": value, + "actiontype": "block", + "comment": comment, + # Authorized uuid + "uuid": "0e7e1cbd-7d73-4968-bc4b-ce3265dc2fd3", + "who": "person", + }, + ) + + +@when("we add the entry {value:S} with expiration {exp:S}") +def add_entry_with_absolute_expiration(context, value, exp): + """Block the provided route and add an absolute expiration datetime.""" + context.response = context.test.client.post( + reverse("api:v1:entry-list"), + { + "route": value, + "actiontype": "block", + "comment": "test", + "expiration": exp, + # Authorized uuid + "uuid": "0e7e1cbd-7d73-4968-bc4b-ce3265dc2fd3", + "who": "person", + }, + ) + + +@when("we add the entry {value:S} with expiration in {secs:d} seconds") +def add_entry_with_relative_expiration(context, value, secs): + """Block the provided route and add a relative expiration.""" + td = datetime.timedelta(seconds=secs) + expiration = datetime.datetime.now(tz=datetime.UTC) + td + + context.response = context.test.client.post( + reverse("api:v1:entry-list"), + { + "route": value, + "actiontype": "block", + "comment": "test", + "expiration": expiration, + # Authorized uuid + "uuid": "0e7e1cbd-7d73-4968-bc4b-ce3265dc2fd3", + "who": "person", + }, + ) + + +@step("we wait {secs:d} seconds") +def wait(context, secs): + """Wait to allow messages to propagate.""" + time.sleep(secs) + + +@then("we remove expired entries") +def remove_expired(context): + """Call the function that removes expired entries.""" + context.response = context.test.client.get(reverse("route_manager:process-updates")) + + +@when("we add the ignore entry {value:S}") +def add_ignore_entry(context, value): + """Add an IgnoreEntry with the specified route.""" + context.response = context.test.client.post( + reverse("api:v1:ignoreentry-list"), + {"route": value, "comment": "test api"}, + ) + + +@when("we remove the {model} {value}") +def remove_an_object(context, model, value): + """Remove any model object with the matching value.""" + context.response = context.test.client.delete(reverse(f"api:v1:{model.lower()}-detail", args=[value])) + + +@when("we list the {model}s") +def list_objects(context, model): + """List all objects of an arbitrary model.""" + context.response = context.test.client.get(reverse(f"api:v1:{model.lower()}-list")) + + +@when("we update the {model} {value_from} to {value_to}") +def update_object(context, model, value_from, value_to): + """Modify any model object with the matching value to the new value instead.""" + context.response = context.test.client.patch( + reverse(f"api:v1:{model.lower()}-detail", args=[value_from]), + {model.lower(): value_to}, + ) + + +@then("the number of {model}s is {num:d}") +def count_objects(context, model, num): + """Count the number of objects of an arbitrary model.""" + objs = context.test.client.get(reverse(f"api:v1:{model.lower()}-list")) + context.test.assertEqual(len(objs.json()["results"]), num) + + +model_to_field_mapping = {"entry": "route"} + + +@then("{value} is one of our list of {model}s") +def check_object(context, value, model): + """Ensure that the arbitrary model has an object with the specified value.""" + objs = context.test.client.get(reverse(f"api:v1:{model.lower()}-list")) + + found = False + for obj in objs.json()["results"]: + # For some models, we need to look at a different field. + model = model_to_field_mapping.get(model.lower(), model.lower()) + if obj[model].lower() == value.lower(): + found = True + break + + context.test.assertTrue(found) + + +@when("we register a client named {hostname} with the uuid of {uuid}") +def add_client(context, hostname, uuid): + """Create a client with a specific UUID.""" + context.response = context.test.client.post( + reverse("api:v1:client-list"), + { + "hostname": hostname, + "uuid": uuid, + }, + ) + + +@when("we register a client named {client_name} and no UUID") +def add_client_no_uuid(context, client_name): + """Create a client without a UUID.""" + context.response = context.test.client.post( + reverse("api:v1:client-list"), + { + "client_name": client_name, + }, + ) diff --git a/scram/route_manager/tests/test_common/steps_ip.py b/scram/route_manager/tests/test_common/steps_ip.py new file mode 100644 index 00000000..ad5c894e --- /dev/null +++ b/scram/route_manager/tests/test_common/steps_ip.py @@ -0,0 +1,77 @@ +"""Define steps used for IP-related logic used by the Behave tests.""" + +import ipaddress +import json + +from behave import then, when +from django.urls import reverse + + +@then("{route} is contained in our list of {model}s") +def check_route(context, route, model): + """Perform a CIDR match on the matching object.""" + objs = context.test.client.get(reverse(f"api:v1:{model.lower()}-list")) + ip_target = ipaddress.ip_address(route) + + ip_found = False + for obj in objs.json()["results"]: + net = ipaddress.ip_network(obj["route"]) + if ip_target in net: + ip_found = True + break + + context.test.assertTrue(ip_found) + + +@when("we query for {ip}") +def check_ip(context, ip): + """Find an Entry for the specified IP.""" + try: + context.response = context.test.client.get(reverse("api:v1:entry-detail", args=[ip])) + context.queryException = None + except ValueError as e: + context.response = None + context.queryException = e + + +@then("we get a ValueError") +def check_error(context): + """Ensure we received a ValueError exception.""" + assert isinstance(context.queryException, ValueError) + + +@then("the comment for entry {value:S} is {comment}") +def check_comment(context, value, comment): + """Verify the comment for the Entry.""" + try: + objs = context.test.client.get(reverse("api:v1:entry-detail", args=[value])) + context.test.assertEqual(objs.json()["comment"], comment) + except ValueError as e: + context.response = None + context.queryException = e + + +@then("we update the entry {value:S} with comment {comment}") +def update_entry_comment(context, value, comment): + """Update the entry with a new comment.""" + data = {"comment": comment, "who": context.client.hostname} + + context.response = context.test.client.put( + reverse("api:v1:entry-detail", args=[value]), data=json.dumps(data), content_type="application/json" + ) + + +@then("the entry {value:S} comment is {comment}") +def check_entry_comment_not_equal(context, value, comment): + """Verify the comment was updated.""" + objs = context.test.client.get(reverse("api:v1:entry-detail", args=[value])) + context.test.assertEqual(objs.json()["comment"], comment) + + +@when("we search for {ip}") +def search_ip(context, ip): + """Search our main search bar for an IP.""" + client = context.test.web_client + search_url = reverse("route_manager:search") + + context.response = client.post(search_url, data={"cidr": ip}) diff --git a/scram/route_manager/tests/test_process_updates.py b/scram/route_manager/tests/test_process_updates.py new file mode 100644 index 00000000..411b632e --- /dev/null +++ b/scram/route_manager/tests/test_process_updates.py @@ -0,0 +1,210 @@ +"""Unit tests for process_updates syncing logic.""" + +from datetime import UTC, datetime, timedelta + +import pytest +from django.conf import settings + +from scram.route_manager.models import ActionType, Entry, Route, WebSocketMessage, WebSocketSequenceElement +from scram.route_manager.views import check_for_orphaned_history, get_entries_to_process + + +@pytest.fixture +def actiontype(db): + """Create a block actiontype for tests.""" + return ActionType.objects.create(name="block") + + +@pytest.fixture +def websocket_config(actiontype): + """Create the WebSocket configuration needed for reprocess_entries.""" + wsm = WebSocketMessage.objects.create( + msg_type="translator_add", + msg_data_route_field="route", + msg_data={"route": "placeholder"}, + ) + WebSocketSequenceElement.objects.create( + websocketmessage=wsm, + action_type=actiontype, + verb="A", + order_num=1, + ) + return wsm + + +@pytest.fixture +def other_instance(): + """Return a hostname different from the current instance.""" + return "scram2.example.com" + + +@pytest.fixture +def current_instance(): + """Return the current SCRAM hostname.""" + return settings.SCRAM_HOSTNAME + + +def create_entry(actiontype, ip, instance, is_active=True, **kwargs): + """Helper to create an entry with the given parameters.""" + route = Route.objects.create(route=ip) + return Entry.objects.create( + route=route, + actiontype=actiontype, + is_active=is_active, + who="test", + comment="test entry", + originating_scram_instance=instance, + **kwargs, + ) + + +class TestGetEntriesToProcess: + """Tests for get_entries_to_process().""" + + def test_empty_when_no_entries(self, db): + """Returns empty list when no entries exist.""" + cutoff = datetime.now(UTC) - timedelta(minutes=2) + assert get_entries_to_process(cutoff) == [] + + def test_finds_entry_from_other_instance(self, actiontype, other_instance): + """Finds entries created by other SCRAM instances.""" + entry = create_entry(actiontype, "192.0.2.1/32", other_instance) + cutoff = datetime.now(UTC) - timedelta(minutes=2) + + result = get_entries_to_process(cutoff) + + assert len(result) == 1 + assert result[0].id == entry.id + + def test_excludes_current_instance_entries(self, actiontype, current_instance): + """Does not return entries from the current SCRAM instance.""" + create_entry(actiontype, "192.0.2.2/32", current_instance) + cutoff = datetime.now(UTC) - timedelta(minutes=2) + + result = get_entries_to_process(cutoff) + + assert result == [] + + def test_finds_modified_entries(self, actiontype, other_instance): + """Finds entries modified after creation (uses history tracking).""" + entry = create_entry(actiontype, "192.0.2.3/32", other_instance) + + # Set cutoff after creation, then modify + cutoff = datetime.now(UTC) + entry.comment = "modified" + entry.save() + + result = get_entries_to_process(cutoff) + + assert len(result) == 1 + assert result[0].comment == "modified" + + def test_finds_soft_deleted_entries(self, actiontype, other_instance): + """Finds entries that were deactivated (expiration, by hand, etc.).""" + entry = create_entry(actiontype, "192.0.2.4/32", other_instance) + cutoff = datetime.now(UTC) + + entry.is_active = False + entry.save() + + result = get_entries_to_process(cutoff) + + assert len(result) == 1 + assert result[0].is_active is False + + def test_respects_cutoff_time(self, actiontype, other_instance): + """Only returns entries modified after the cutoff time by faking the modified time.""" + old_entry = create_entry(actiontype, "192.0.2.5/32", other_instance) + + # Backdate the history + history = old_entry.history.latest() + history.history_date = datetime.now(UTC) - timedelta(minutes=5) + history.save() + + cutoff = datetime.now(UTC) - timedelta(minutes=2) + result = get_entries_to_process(cutoff) + + assert result == [] + + def test_multiple_entries_from_different_instances(self, actiontype): + """Entries from multiple other instances are all processed.""" + entries = [] + entries.append(create_entry(actiontype, "192.0.2.20/32", "scram2.example.com")) + entries.append(create_entry(actiontype, "192.0.2.21/32", "scram3.example.com")) + cutoff = datetime.now(UTC) - timedelta(minutes=2) + + result = get_entries_to_process(cutoff) + + assert len(result) == 2 + for entry in entries: + assert any(r.id == entry.id for r in result) + + def test_reactivated_entry_found(self, actiontype, other_instance): + """Reactivated entries are found for reprocessing.""" + entry = create_entry(actiontype, "192.0.2.30/32", other_instance, is_active=False) + cutoff = datetime.now(UTC) + entry.is_active = True + entry.save() + + result = get_entries_to_process(cutoff) + + assert len(result) == 1 + assert result[0].is_active is True + + def test_future_expiration_entry_active(self, actiontype, other_instance): + """Entries with future expiration are processed as active.""" + _ = create_entry( + actiontype, "192.0.2.40/32", other_instance, expiration=datetime.now(UTC) + timedelta(hours=1) + ) + cutoff = datetime.now(UTC) - timedelta(minutes=2) + + result = get_entries_to_process(cutoff) + + assert len(result) == 1 + assert result[0].is_active is True + + def test_expired_entry_found_as_inactive(self, actiontype, other_instance): + """Expired entries are found but marked inactive after process_updates expires them.""" + entry = create_entry( + actiontype, "192.0.2.50/32", other_instance, expiration=datetime.now(UTC) - timedelta(hours=1) + ) + cutoff = datetime.now(UTC) - timedelta(minutes=2) + + result = get_entries_to_process(cutoff) + + assert len(result) == 1 + assert result[0].id == entry.id + + +class TestCheckForOrphanedHistory: + """Tests for check_for_orphaned_history().""" + + def test_logs_warning_for_orphaned_entries(self, caplog, actiontype, other_instance): + """Make sure we log a warning when history exists but Entry was deleted from underneath us.""" + entry = create_entry(actiontype, "10.1.0.1/32", other_instance) + orphaned_id = entry.id + + # Hard delete (bypass model's soft delete) + Entry.objects.filter(id=orphaned_id).delete() + check_for_orphaned_history({orphaned_id}, []) + + assert len(caplog.records) == 1 + assert caplog.records[0].levelname == "WARNING" + assert str(orphaned_id) in caplog.records[0].message + + def test_no_warning_when_entry_exists(self, caplog, actiontype, other_instance): + """No warning when all entries in the set exist.""" + entry = create_entry(actiontype, "10.1.0.2/32", other_instance) + + check_for_orphaned_history({entry.id}, [entry]) + + assert len(caplog.records) == 0 + + def test_accounts_for_local_entries(self, caplog, actiontype, current_instance): + """Local entries excluded from processing are not flagged as orphans.""" + local_entry = create_entry(actiontype, "10.1.0.3/32", current_instance) + + # Entry exists but was excluded from entries_to_process because it's local + check_for_orphaned_history({local_entry.id}, []) + + assert len(caplog.records) == 0 diff --git a/scram/route_manager/views.py b/scram/route_manager/views.py index a4f6a544..68104726 100644 --- a/scram/route_manager/views.py +++ b/scram/route_manager/views.py @@ -147,6 +147,98 @@ def add_entry(request): return redirect("route_manager:home") +def get_entries_to_process(cutoff_time: timedelta) -> list[Entry]: + """Return entries that have been recently modified by another SCRAM instance. + + Queries the Entry history (simple history) table to find any entries modified + since the cutoff time, then filters to only those originating from other SCRAM instances. + + Args: + cutoff_time(timedelta): Only consider entries modified after this time. + + Returns: + List of Entry objects that need to be reprocessed. + """ + logger.debug("Looking for entries modified by other SCRAM instances") + + # Grab (only, via values_list) the Entry IDs that have had their history records touched since the cutoff time. + recently_touched_ids = set(Entry.history.filter(history_date__gt=cutoff_time).values_list("id", flat=True)) + + if not recently_touched_ids: + logger.debug("No recently modified entries found") + return [] + + logger.debug("Found recently touched entry IDs: %s", recently_touched_ids) + + # Using the ID's from above, fetch all matching entries and associated models excluding entries from this instance. + entries_to_process = list( + Entry.objects.filter(id__in=recently_touched_ids) + .exclude(originating_scram_instance=settings.SCRAM_HOSTNAME) + .select_related("actiontype", "route") + ) + + check_for_orphaned_history(recently_touched_ids, entries_to_process) + + logger.debug("Found %d entries to process from other instances", len(entries_to_process)) + return entries_to_process + + +def check_for_orphaned_history(recently_touched_ids: set[int], entries_to_process: list[Entry]) -> None: + """Check for orphaned history records where the Entry was deleted but history remains. + + This shouldn't happen in production since Entry.delete() is overridden on the model, + but we log a warning if it occurs for debugging purposes. + + Args: + recently_touched_ids(set): Set of Entry IDs that have recent history records. + entries_to_process(list): Entry objects fetched from the database (excludes local instance). + """ + # IDs of entries that currently exist in the DB (excluding local instance entries) + found_ids = {entry.id for entry in entries_to_process} + # Account for entries filtered out cuz they're from this instance + local_ids = set( + Entry.objects.filter( + id__in=recently_touched_ids, originating_scram_instance=settings.SCRAM_HOSTNAME + ).values_list("id", flat=True) + ) + # IDs with history but no corresponding Entry row = orphaned (hard-deleted outside of Entry.delete()) + orphaned_ids = recently_touched_ids - found_ids - local_ids + if orphaned_ids: + logger.warning("Found history records with no corresponding Entry: %s", orphaned_ids) + + +def reprocess_entries(entries_to_process: list[Entry]) -> None: + """Take a list of Entries and send appropriate websocket messages to translators. + + Effectively, this is a way to tell the translator "hey, do the stuff you need to do for this list of entries", + whether that is to block or unblock them. This is used by `process_updates()`. In this, we take each entry + and send either a translator_add or translator_remove message, depending on the entry's is_active state, and + notify translators to Do the Thing. + + Args: + entries_to_process(list[Entry]): Entry objects that need to be sent to translators. + """ + logger.info("Reprocessing %d entries", len(entries_to_process)) + + for entry in entries_to_process: + message_type = "translator_add" if entry.is_active else "translator_remove" + logger.info("Processing entry %s (active=%s)", entry, entry.is_active) + + translator_group = f"translator_{entry.actiontype}" + elements = ( + WebSocketSequenceElement.objects.filter(action_type__name=entry.actiontype) + .order_by("order_num") + .select_related("websocketmessage") + ) + + for element in elements: + msg = element.websocketmessage + msg.msg_data[msg.msg_data_route_field] = str(entry.route) + + json_to_send = {"type": message_type, "message": msg.msg_data} + async_to_sync(channel_layer.group_send)(translator_group, json_to_send) + + def process_updates(request): """For entries with an expiration, set them to inactive if expired. @@ -166,33 +258,23 @@ def process_updates(request): obj.delete() entries_end = Entry.objects.filter(is_active=True).count() - logger.debug("Looking for new entries from other SCRAM instances") - # Grab all entries from the last 2 minutes that originated from a different SCRAM instance. + # Grab all of the other entries that need processing and... process them! cutoff_time = current_time - timedelta(minutes=2) - new_entries = Entry.objects.filter(when__gt=cutoff_time).exclude( - originating_scram_instance=settings.SCRAM_HOSTNAME - ) - - # Resend new entries that popped up in the database - for entry in new_entries: - logger.info("Processing new entry: %s", entry) - translator_group = f"translator_{entry.actiontype}" - elements = list( - WebSocketSequenceElement.objects.filter(action_type__name=entry.actiontype).order_by("order_num") - ) - for element in elements: - msg = element.websocketmessage - msg.msg_data[msg.msg_data_route_field] = str(entry.route) + entries_to_process = get_entries_to_process(cutoff_time=cutoff_time) + entries_reprocessed_list = [str(entry.route) for entry in entries_to_process] - json_to_send = {"type": msg.msg_type, "message": msg.msg_data} - async_to_sync(channel_layer.group_send)(translator_group, json_to_send) + if entries_to_process: + reprocess_entries(entries_to_process) + else: + logger.info("No new entries to process") return HttpResponse( json.dumps( { "entries_deleted": entries_start - entries_end, "active_entries": entries_end, - "remote_entries_added": new_entries.count(), + "entries_reprocessed": len(entries_to_process), + "entries_reprocessed_list": entries_reprocessed_list, }, ), content_type="application/json", From 0724e295b7273bccc825416206aa6bf981a6d0b3 Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Fri, 19 Dec 2025 12:27:47 -0600 Subject: [PATCH 29/31] style(ruff): ran ruff format and check also added pre commit config that hopefully doesnt break anything because i couldnt commit otherwise --- .pre-commit-config.yaml | 2 +- scram/route_manager/api/views.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 5315066a..7b903538 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -1,5 +1,5 @@ exclude: 'docs|migrations|.git|.tox' -default_stages: [commit] +default_stages: [pre-commit] fail_fast: true repos: diff --git a/scram/route_manager/api/views.py b/scram/route_manager/api/views.py index 0c194b10..dae3cf2d 100644 --- a/scram/route_manager/api/views.py +++ b/scram/route_manager/api/views.py @@ -79,7 +79,7 @@ class IgnoreEntryViewSet(viewsets.ModelViewSet): ), 201: OpenApiResponse(response=ClientSerializer, description="Client successfully created."), 400: OpenApiResponse( - description="A client with this name already exists with a different UUID, or client_name was not provided." + description="Client with this name already exists with a different UUID, or client_name was not provided." ), }, ) @@ -116,7 +116,7 @@ def create(self, request, *args, **kwargs): # Log the conflict without leaking client_names to the user logger.warning( - "Client named: %s exists with different UUID", + "Client named %s already exists with a different UUID", client_name, ) return Response( From c36eae78e63ef25acc56cf0c73a4665f875bba6d Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Fri, 19 Dec 2025 15:58:34 -0600 Subject: [PATCH 30/31] fix(hostname): change hostname to client_name how can there be any left i keep looking --- scram/route_manager/tests/test_common/steps_ip.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scram/route_manager/tests/test_common/steps_ip.py b/scram/route_manager/tests/test_common/steps_ip.py index ad5c894e..e51e0262 100644 --- a/scram/route_manager/tests/test_common/steps_ip.py +++ b/scram/route_manager/tests/test_common/steps_ip.py @@ -54,7 +54,7 @@ def check_comment(context, value, comment): @then("we update the entry {value:S} with comment {comment}") def update_entry_comment(context, value, comment): """Update the entry with a new comment.""" - data = {"comment": comment, "who": context.client.hostname} + data = {"comment": comment, "who": context.client.client_name} context.response = context.test.client.put( reverse("api:v1:entry-detail", args=[value]), data=json.dumps(data), content_type="application/json" From 7d0386b3b00ad78dca60025174b8889dc38e6dc0 Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Fri, 19 Dec 2025 16:13:50 -0600 Subject: [PATCH 31/31] fix(hostname): nope theres still one more --- scram/route_manager/tests/integration/environment.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scram/route_manager/tests/integration/environment.py b/scram/route_manager/tests/integration/environment.py index aca60e8e..6d072085 100644 --- a/scram/route_manager/tests/integration/environment.py +++ b/scram/route_manager/tests/integration/environment.py @@ -43,7 +43,7 @@ def before_feature(context, feature): client, _ = DjangoClient.objects.update_or_create( uuid=TEST_CLIENT_UUID, defaults={ - "hostname": TEST_CLIENT_HOSTNAME, + "client_name": TEST_CLIENT_HOSTNAME, "is_authorized": True, }, )