From be19b92c9df5289e6e163fc27f4d99cb2d765115 Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Mon, 7 Jul 2025 17:37:52 -0500 Subject: [PATCH 01/16] fix(history): remove the extraneous save call that created duplicate history entries on first try --- scram/route_manager/api/serializers.py | 17 +------------ scram/route_manager/api/views.py | 33 +++++++++++++++----------- 2 files changed, 20 insertions(+), 30 deletions(-) diff --git a/scram/route_manager/api/serializers.py b/scram/route_manager/api/serializers.py index 96283335..d5850db7 100644 --- a/scram/route_manager/api/serializers.py +++ b/scram/route_manager/api/serializers.py @@ -72,7 +72,7 @@ class Meta: """Maps to the Entry model, and specifies the fields exposed by the API.""" model = Entry - fields = ["route", "actiontype", "url", "comment", "who"] + fields = ["route", "actiontype", "url", "comment", "who", "expiration", "originating_scram_instance"] @staticmethod def get_comment(obj): @@ -83,21 +83,6 @@ def get_comment(obj): """ return obj.get_change_reason() - @staticmethod - def create(validated_data): - """Implement custom logic and validates creating a new route.""" - valid_route = validated_data.pop("route") - actiontype = validated_data.pop("actiontype") - comment = validated_data.pop("comment") - - route_instance, _ = Route.objects.get_or_create(route=valid_route) - actiontype_instance = ActionType.objects.get(name=actiontype) - entry_instance, _ = Entry.objects.get_or_create(route=route_instance, actiontype=actiontype_instance) - - logger.debug("Created entry with comment: %s", comment) - - return entry_instance - class IgnoreEntrySerializer(serializers.ModelSerializer): """Map the route to the right field type.""" diff --git a/scram/route_manager/api/views.py b/scram/route_manager/api/views.py index 16fc59d7..7788c3fc 100644 --- a/scram/route_manager/api/views.py +++ b/scram/route_manager/api/views.py @@ -16,7 +16,7 @@ from rest_framework.response import Response from simple_history.utils import update_change_reason -from ..models import ActionType, Client, Entry, IgnoreEntry, WebSocketSequenceElement +from ..models import ActionType, Client, Entry, IgnoreEntry, Route, WebSocketSequenceElement from .exceptions import ActiontypeNotAllowed, IgnoredRoute, PrefixTooLarge from .serializers import ActionTypeSerializer, ClientSerializer, EntrySerializer, IgnoreEntrySerializer @@ -117,6 +117,10 @@ def perform_create(self, serializer): """Create a new Entry, causing that route to receive the actiontype (i.e. block).""" actiontype = serializer.validated_data["actiontype"] route = serializer.validated_data["route"] + + route_instance, _ = Route.objects.get_or_create(route=route) + actiontype_instance = ActionType.objects.get(name=actiontype) + if self.request.user.username: # This is set if our request comes through the WUI path who = self.request.user.username @@ -137,7 +141,7 @@ def perform_create(self, serializer): raise PrefixTooLarge self.check_client_authorization(actiontype) - self.check_ignore_list(route) + self.check_ignore_list(route_instance) elements = WebSocketSequenceElement.objects.filter(action_type__name=actiontype).order_by("order_num") if not elements: @@ -145,24 +149,25 @@ def perform_create(self, serializer): for element in elements: msg = element.websocketmessage - msg.msg_data[msg.msg_data_route_field] = str(route) + msg.msg_data[msg.msg_data_route_field] = str(route_instance) # Must match a channel name defined in asgi.py async_to_sync(channel_layer.group_send)( f"translator_{actiontype}", {"type": msg.msg_type, "message": msg.msg_data}, ) - serializer.save() - - entry = Entry.objects.get(route__route=route, actiontype__name=actiontype) - if expiration: - entry.expiration = expiration - entry.who = who - entry.is_active = True - entry.comment = comment - entry.originating_scram_instance = settings.SCRAM_HOSTNAME - logger.info("Created entry: %s", entry) - entry.save() + logger.info("Created entry %s for route %s", actiontype, route) + + serializer.save( + route=route_instance, + actiontype=actiontype_instance, + who=who, + is_active=True, + comment=comment, + originating_scram_instance=settings.SCRAM_HOSTNAME, + expiration=expiration if expiration else None + ) + entry = serializer.instance update_change_reason(entry, comment) @staticmethod From 1744b669cfd87a1f7e4313950bbf60ef49d8381e Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Tue, 8 Jul 2025 17:22:08 -0500 Subject: [PATCH 02/16] Move the logic from serializer to the api/views perform create. This allows us to only have one historicalentry on initial entry. --- scram/route_manager/api/serializers.py | 5 +++-- scram/route_manager/api/views.py | 10 +++++----- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/scram/route_manager/api/serializers.py b/scram/route_manager/api/serializers.py index d5850db7..30d65409 100644 --- a/scram/route_manager/api/serializers.py +++ b/scram/route_manager/api/serializers.py @@ -67,12 +67,13 @@ class EntrySerializer(serializers.HyperlinkedModelSerializer): else: who = serializers.CharField() comment = serializers.CharField() + originating_scram_instance = serializers.CharField(default="scram_hostname_not_set", read_only=True) + is_active = serializers.BooleanField(default=True, read_only=True) class Meta: """Maps to the Entry model, and specifies the fields exposed by the API.""" - model = Entry - fields = ["route", "actiontype", "url", "comment", "who", "expiration", "originating_scram_instance"] + fields = ["route", "actiontype", "url", "comment", "who", "expiration", "originating_scram_instance", "is_active"] @staticmethod def get_comment(obj): diff --git a/scram/route_manager/api/views.py b/scram/route_manager/api/views.py index 7788c3fc..9256e08c 100644 --- a/scram/route_manager/api/views.py +++ b/scram/route_manager/api/views.py @@ -121,12 +121,13 @@ def perform_create(self, serializer): route_instance, _ = Route.objects.get_or_create(route=route) actiontype_instance = ActionType.objects.get(name=actiontype) - if self.request.user.username: - # This is set if our request comes through the WUI path - who = self.request.user.username - else: + if serializer.validated_data.get("who"): # This is set if we pass the "who" through the json data in an API call (like from Zeek) who = serializer.validated_data["who"] + else: + # This is set if our request comes through the WUI path + who = self.request.user.username + comment = serializer.validated_data["comment"] tmp_exp = self.request.data.get("expiration", "") @@ -165,7 +166,6 @@ def perform_create(self, serializer): is_active=True, comment=comment, originating_scram_instance=settings.SCRAM_HOSTNAME, - expiration=expiration if expiration else None ) entry = serializer.instance update_change_reason(entry, comment) From 36c20041537adc7f571a34aa2badd8baceab16b3 Mon Sep 17 00:00:00 2001 From: samoehlert Date: Wed, 9 Jul 2025 01:23:37 -0500 Subject: [PATCH 03/16] fix(messages framework): the call to home page right before we redirected to home meant none of our messages showed up. this was because it loaded once quickly then again which is what the user would see --- scram/route_manager/views.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/scram/route_manager/views.py b/scram/route_manager/views.py index d6e341db..13871f9f 100644 --- a/scram/route_manager/views.py +++ b/scram/route_manager/views.py @@ -143,8 +143,6 @@ def add_entry(request): messages.add_message(request, messages.ERROR, "Permission Denied") else: messages.add_message(request, messages.WARNING, f"Something went wrong: {res.status_code}") - with transaction.atomic(): - home_page(request) return redirect("route_manager:home") From 3d9e1ddb10d9820c2ae2dfcaf97cdb798162f939 Mon Sep 17 00:00:00 2001 From: samoehlert Date: Wed, 9 Jul 2025 01:27:46 -0500 Subject: [PATCH 04/16] feat(update): This allows updating an entry owned by you. Comment and expiration only. API only for now. --- scram/route_manager/api/views.py | 41 +++++++++++++++++++++++++------- 1 file changed, 32 insertions(+), 9 deletions(-) diff --git a/scram/route_manager/api/views.py b/scram/route_manager/api/views.py index 9256e08c..ae4a3063 100644 --- a/scram/route_manager/api/views.py +++ b/scram/route_manager/api/views.py @@ -76,7 +76,7 @@ class EntryViewSet(viewsets.ModelViewSet): permission_classes = (IsAuthenticated,) serializer_class = EntrySerializer lookup_value_regex = ".*" - http_method_names = ["get", "post", "head", "delete"] + http_method_names = ["get", "post", "put", "patch", "head", "delete"] def get_permissions(self): """Override the permissions classes for POST method since we want to accept Entry creates from any client. @@ -136,7 +136,6 @@ def perform_create(self, serializer): except ValueError: logger.warning("Could not parse expiration DateTime string: %s", tmp_exp) - # Make sure we put in an acceptable sized prefix min_prefix = getattr(settings, f"V{route.version}_MINPREFIX", 0) if route.prefixlen < min_prefix: raise PrefixTooLarge @@ -157,8 +156,6 @@ def perform_create(self, serializer): {"type": msg.msg_type, "message": msg.msg_data}, ) - logger.info("Created entry %s for route %s", actiontype, route) - serializer.save( route=route_instance, actiontype=actiontype_instance, @@ -169,6 +166,35 @@ def perform_create(self, serializer): ) entry = serializer.instance update_change_reason(entry, comment) + logger.info("Created entry %s for route %s", actiontype, route) + + def perform_update(self, serializer): + """Update an existing Entry.""" + comment = serializer.validated_data.get("comment", "") + # Determine who is making this request + if serializer.validated_data.get("who"): + requesting_who = serializer.validated_data["who"] + else: + requesting_who = self.request.user.username + + if serializer.instance.who != requesting_who: + raise PermissionDenied("You can only update your own entries") + + serializer.save(who=serializer.instance.who, originating_scram_instance=settings.SCRAM_HOSTNAME) + + entry = serializer.instance + update_change_reason(entry, comment) + logger.info("Updated entry %s", entry) + + def get_object(self): + """Override get_object to use our custom find_entries logic.""" + pk = self.kwargs.get('pk') + entries = self.find_entries(pk, active_filter=True) + + if entries.count() != 1: + raise Http404 + + return entries.first() @staticmethod def find_entries(arg, active_filter=None): @@ -197,11 +223,8 @@ def find_entries(arg, active_filter=None): def retrieve(self, request, pk=None, **kwargs): """Retrieve a single route.""" - entries = self.find_entries(pk, active_filter=True) - # TODO: What happens if we get multiple? Is that ok? I think yes, and return them all? - if entries.count() != 1: - raise Http404 - serializer = EntrySerializer(entries, many=True, context={"request": request}) + entry = self.get_object() + serializer = EntrySerializer(entry, context={"request": request}) return Response(serializer.data) def destroy(self, request, pk=None, *args, **kwargs): From 6dfd4d379be3a88e6e8edd58c0a4b5998514a97f Mon Sep 17 00:00:00 2001 From: samoehlert Date: Wed, 9 Jul 2025 01:31:48 -0500 Subject: [PATCH 05/16] fix(tests): We now get a permission denied error when we attempt to change something we don't own. --- .../tests/acceptance/features/restrict_changes.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scram/route_manager/tests/acceptance/features/restrict_changes.feature b/scram/route_manager/tests/acceptance/features/restrict_changes.feature index bb24eddc..09b06a70 100644 --- a/scram/route_manager/tests/acceptance/features/restrict_changes.feature +++ b/scram/route_manager/tests/acceptance/features/restrict_changes.feature @@ -7,7 +7,7 @@ Feature: restrict changing entries When we're logged in And we add the entry 192.0.2.208 And we update the entry 192.0.2.208 to 192.0.2.209 - Then we get a 405 status code + Then we get a 403 status code And the number of entrys is 1 And 192.0.2.208 is announced by block translators And 192.0.2.209 is not announced by block translators From 4020575ceb03527cf1ca495f4c0763a5aa4461dd Mon Sep 17 00:00:00 2001 From: samoehlert Date: Wed, 9 Jul 2025 01:34:28 -0500 Subject: [PATCH 06/16] feat(history): make sure we not only get the comment on a new entry, but handle updating it on changes. also define some read only fields since users should not be allowed to change those things --- scram/route_manager/api/serializers.py | 37 +++++++++++++++++++++----- 1 file changed, 30 insertions(+), 7 deletions(-) diff --git a/scram/route_manager/api/serializers.py b/scram/route_manager/api/serializers.py index 30d65409..05c00e7a 100644 --- a/scram/route_manager/api/serializers.py +++ b/scram/route_manager/api/serializers.py @@ -6,6 +6,8 @@ from netfields import rest_framework from rest_framework import serializers from rest_framework.fields import CurrentUserDefault +from simple_history.utils import update_change_reason + from ..models import ActionType, Client, Entry, IgnoreEntry, Route @@ -70,19 +72,40 @@ class EntrySerializer(serializers.HyperlinkedModelSerializer): originating_scram_instance = serializers.CharField(default="scram_hostname_not_set", read_only=True) is_active = serializers.BooleanField(default=True, read_only=True) + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + if self.instance is not None: + self.fields['route'].read_only = True + self.fields['actiontype'].read_only = True + self.fields['who'].read_only = True + class Meta: """Maps to the Entry model, and specifies the fields exposed by the API.""" model = Entry fields = ["route", "actiontype", "url", "comment", "who", "expiration", "originating_scram_instance", "is_active"] - @staticmethod - def get_comment(obj): - """Provide a nicer name for change reason. + def create(self, validated_data): + """Create or update an Entry, handling duplicates gracefully.""" + route_data = validated_data.pop('route') + actiontype_name = validated_data.pop('actiontype') + comment = validated_data.pop('comment') + + route_instance, _ = Route.objects.get_or_create(route=route_data) + actiontype_instance = ActionType.objects.get(name=actiontype_name) + + entry, created = Entry.objects.get_or_create( + route=route_instance, + actiontype=actiontype_instance, + defaults=validated_data + ) + + if not created: + for key, value in validated_data.items(): + setattr(entry, key, value) + entry.save() + update_change_reason(entry, comment) - Returns: - string: The change reason that modified the Entry. - """ - return obj.get_change_reason() + return entry class IgnoreEntrySerializer(serializers.ModelSerializer): From 5249dc4df833af4177dfc228ffd8067866bdc6bd Mon Sep 17 00:00:00 2001 From: samoehlert Date: Wed, 9 Jul 2025 13:07:14 -0500 Subject: [PATCH 07/16] fix(grabbin data): we need to use comment later so we need to use a grab so it stays in the validated_data --- 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 05c00e7a..294854c6 100644 --- a/scram/route_manager/api/serializers.py +++ b/scram/route_manager/api/serializers.py @@ -88,7 +88,7 @@ def create(self, validated_data): """Create or update an Entry, handling duplicates gracefully.""" route_data = validated_data.pop('route') actiontype_name = validated_data.pop('actiontype') - comment = validated_data.pop('comment') + comment = validated_data.get('comment', '') route_instance, _ = Route.objects.get_or_create(route=route_data) actiontype_instance = ActionType.objects.get(name=actiontype_name) From fe97d762a022e8c616de0147dd7d5baabc44f328 Mon Sep 17 00:00:00 2001 From: samoehlert Date: Wed, 9 Jul 2025 13:08:17 -0500 Subject: [PATCH 08/16] test(update comment): add a scenario to test our perform_update comment and change wording for other scenario to make more sense --- .../features/add_automated_block_entry.feature | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/scram/route_manager/tests/acceptance/features/add_automated_block_entry.feature b/scram/route_manager/tests/acceptance/features/add_automated_block_entry.feature index 83af7dc5..c9e5ad82 100644 --- a/scram/route_manager/tests/acceptance/features/add_automated_block_entry.feature +++ b/scram/route_manager/tests/acceptance/features/add_automated_block_entry.feature @@ -33,7 +33,15 @@ Feature: an automated source adds a block entry When we're logged in And we add the entry 192.0.2.133 with comment it's coming from inside the house Then we get a 201 status code - And the change entry for 192.0.2.133 is it's coming from inside the house + And the comment for entry 192.0.2.133 is it's coming from inside the house + + @history: + Scenario: Update comment on a block entry + Given a client with block authorization + When we're logged in + And we add the entry 192.0.2.10 with comment it's coming from inside the house + Then we get a 201 status code + And we update the entry 192.0.2.10 with comment it's coming from outside the house Scenario Outline: add a block entry multiple times and it's accepted Given a client with block authorization From 869a510628ca0b60000a648842a1562fc1f9004f Mon Sep 17 00:00:00 2001 From: samoehlert Date: Wed, 9 Jul 2025 13:09:17 -0500 Subject: [PATCH 09/16] test(update comment): make sure we actually can test the comment update and create a helper for our client to easily get to hostname as the who --- .../tests/acceptance/steps/common.py | 2 +- .../tests/acceptance/steps/ip.py | 24 +++++++++++++++++-- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/scram/route_manager/tests/acceptance/steps/common.py b/scram/route_manager/tests/acceptance/steps/common.py index cb803ba6..4b966a10 100644 --- a/scram/route_manager/tests/acceptance/steps/common.py +++ b/scram/route_manager/tests/acceptance/steps/common.py @@ -43,7 +43,7 @@ def create_authed_client(context, name): 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): diff --git a/scram/route_manager/tests/acceptance/steps/ip.py b/scram/route_manager/tests/acceptance/steps/ip.py index b8fda111..448e5c2f 100644 --- a/scram/route_manager/tests/acceptance/steps/ip.py +++ b/scram/route_manager/tests/acceptance/steps/ip.py @@ -1,6 +1,7 @@ """Define steps used for IP-related logic by the Behave tests.""" import ipaddress +import json from behave import then, when from django.urls import reverse @@ -39,16 +40,35 @@ def check_error(context): assert isinstance(context.queryException, ValueError) -@then("the change entry for {value:S} is {comment}") +@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()[0]["comment"], comment) + 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): From b5b3cc0af58857f14c98e505453b7e51ff69c7cd Mon Sep 17 00:00:00 2001 From: samoehlert Date: Wed, 9 Jul 2025 16:07:42 -0500 Subject: [PATCH 10/16] style(ruff): run ruff formatting and rule checking --- scram/route_manager/api/serializers.py | 34 ++++++++++++------- scram/route_manager/api/views.py | 12 ++----- .../tests/acceptance/steps/common.py | 1 + .../tests/acceptance/steps/ip.py | 12 +++---- 4 files changed, 31 insertions(+), 28 deletions(-) diff --git a/scram/route_manager/api/serializers.py b/scram/route_manager/api/serializers.py index 294854c6..8d08fea6 100644 --- a/scram/route_manager/api/serializers.py +++ b/scram/route_manager/api/serializers.py @@ -8,7 +8,6 @@ from rest_framework.fields import CurrentUserDefault from simple_history.utils import update_change_reason - from ..models import ActionType, Client, Entry, IgnoreEntry, Route logger = logging.getLogger(__name__) @@ -73,30 +72,41 @@ class EntrySerializer(serializers.HyperlinkedModelSerializer): is_active = serializers.BooleanField(default=True, read_only=True) def __init__(self, *args, **kwargs): + """Make sure we do not allow changing these fields in our put/patch calls.""" super().__init__(*args, **kwargs) if self.instance is not None: - self.fields['route'].read_only = True - self.fields['actiontype'].read_only = True - self.fields['who'].read_only = True + self.fields["route"].read_only = True + self.fields["actiontype"].read_only = True + self.fields["who"].read_only = True class Meta: - """Maps to the Entry model, and specifies the fields exposed by the API.""" + """Map to the Entry model, and specify the fields exposed by the API.""" + model = Entry - fields = ["route", "actiontype", "url", "comment", "who", "expiration", "originating_scram_instance", "is_active"] + fields = [ + "route", + "actiontype", + "url", + "comment", + "who", + "expiration", + "originating_scram_instance", + "is_active", + ] + # This needs to be an instance method since thats expected by DRF + # ruff: noqa: PLR6301 def create(self, validated_data): """Create or update an Entry, handling duplicates gracefully.""" - route_data = validated_data.pop('route') - actiontype_name = validated_data.pop('actiontype') - comment = validated_data.get('comment', '') + route_data = validated_data.pop("route") + actiontype_name = validated_data.pop("actiontype") + comment = validated_data.get("comment", "") route_instance, _ = Route.objects.get_or_create(route=route_data) actiontype_instance = ActionType.objects.get(name=actiontype_name) entry, created = Entry.objects.get_or_create( - route=route_instance, - actiontype=actiontype_instance, - defaults=validated_data + route=route_instance, actiontype=actiontype_instance, defaults=validated_data ) if not created: diff --git a/scram/route_manager/api/views.py b/scram/route_manager/api/views.py index ae4a3063..42280703 100644 --- a/scram/route_manager/api/views.py +++ b/scram/route_manager/api/views.py @@ -9,7 +9,6 @@ from django.core.exceptions import PermissionDenied from django.db.models import Q from django.http import Http404 -from django.utils.dateparse import parse_datetime from drf_spectacular.utils import extend_schema from rest_framework import status, viewsets from rest_framework.permissions import AllowAny, IsAuthenticated @@ -129,12 +128,6 @@ def perform_create(self, serializer): who = self.request.user.username comment = serializer.validated_data["comment"] - tmp_exp = self.request.data.get("expiration", "") - - try: - expiration = parse_datetime(tmp_exp) - except ValueError: - logger.warning("Could not parse expiration DateTime string: %s", tmp_exp) min_prefix = getattr(settings, f"V{route.version}_MINPREFIX", 0) if route.prefixlen < min_prefix: @@ -178,7 +171,8 @@ def perform_update(self, serializer): requesting_who = self.request.user.username if serializer.instance.who != requesting_who: - raise PermissionDenied("You can only update your own entries") + msg = "You can only update your own entries" + raise PermissionDenied(msg) serializer.save(who=serializer.instance.who, originating_scram_instance=settings.SCRAM_HOSTNAME) @@ -188,7 +182,7 @@ def perform_update(self, serializer): def get_object(self): """Override get_object to use our custom find_entries logic.""" - pk = self.kwargs.get('pk') + pk = self.kwargs.get("pk") entries = self.find_entries(pk, active_filter=True) if entries.count() != 1: diff --git a/scram/route_manager/tests/acceptance/steps/common.py b/scram/route_manager/tests/acceptance/steps/common.py index 4b966a10..cec0692d 100644 --- a/scram/route_manager/tests/acceptance/steps/common.py +++ b/scram/route_manager/tests/acceptance/steps/common.py @@ -45,6 +45,7 @@ def create_authed_client(context, name): 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.""" diff --git a/scram/route_manager/tests/acceptance/steps/ip.py b/scram/route_manager/tests/acceptance/steps/ip.py index 448e5c2f..5492f96f 100644 --- a/scram/route_manager/tests/acceptance/steps/ip.py +++ b/scram/route_manager/tests/acceptance/steps/ip.py @@ -50,26 +50,24 @@ def check_comment(context, value, comment): 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 - } + 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" + 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.""" From f74ad71c7a460cb6aae357b278b89f19bc44f445 Mon Sep 17 00:00:00 2001 From: samoehlert Date: Thu, 10 Jul 2025 09:18:18 -0500 Subject: [PATCH 11/16] style(route action): remove extraneous code. we already have gotten the route and actiontype in the view and passed it to the serializer --- scram/route_manager/api/serializers.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/scram/route_manager/api/serializers.py b/scram/route_manager/api/serializers.py index 8d08fea6..5c790f54 100644 --- a/scram/route_manager/api/serializers.py +++ b/scram/route_manager/api/serializers.py @@ -102,11 +102,8 @@ def create(self, validated_data): actiontype_name = validated_data.pop("actiontype") comment = validated_data.get("comment", "") - route_instance, _ = Route.objects.get_or_create(route=route_data) - actiontype_instance = ActionType.objects.get(name=actiontype_name) - entry, created = Entry.objects.get_or_create( - route=route_instance, actiontype=actiontype_instance, defaults=validated_data + route=route_data, actiontype=actiontype_name, defaults=validated_data ) if not created: From 6134d10e3007a0afed6b855456e6447fc0579791 Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Tue, 21 Oct 2025 10:20:22 -0500 Subject: [PATCH 12/16] refactor(http-methods): remove put and patch from the list as they are not required this also makes the scram_client experience better as it doesn't need to track entries. see https://github.com/esnet-security/SCRAM/pull/163/files#r2208200704 --- 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 42280703..bdde2a64 100644 --- a/scram/route_manager/api/views.py +++ b/scram/route_manager/api/views.py @@ -75,7 +75,7 @@ class EntryViewSet(viewsets.ModelViewSet): permission_classes = (IsAuthenticated,) serializer_class = EntrySerializer lookup_value_regex = ".*" - http_method_names = ["get", "post", "put", "patch", "head", "delete"] + http_method_names = ["get", "post", "head", "delete"] def get_permissions(self): """Override the permissions classes for POST method since we want to accept Entry creates from any client. From 9bdc979c3ae175392d98f58cd72ed486cc867c6e Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Tue, 21 Oct 2025 10:26:24 -0500 Subject: [PATCH 13/16] refactor(error-message): use a custom error message that gives better information --- scram/route_manager/api/exceptions.py | 8 ++++++++ scram/route_manager/api/views.py | 4 ++-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/scram/route_manager/api/exceptions.py b/scram/route_manager/api/exceptions.py index 0445dbe4..297b4477 100644 --- a/scram/route_manager/api/exceptions.py +++ b/scram/route_manager/api/exceptions.py @@ -28,3 +28,11 @@ class ActiontypeNotAllowed(APIException): status_code = 403 default_detail = "This client is not allowed to use this actiontype" default_code = "actiontype_not_allowed" + + +class NoActiveEntryFound(APIException): + """An active entry was not found.""" + + status_code = 404 + default_detail = "No active entry was found." + default_code = "no_entry_found" diff --git a/scram/route_manager/api/views.py b/scram/route_manager/api/views.py index bdde2a64..91abef60 100644 --- a/scram/route_manager/api/views.py +++ b/scram/route_manager/api/views.py @@ -17,7 +17,7 @@ from ..models import ActionType, Client, Entry, IgnoreEntry, Route, WebSocketSequenceElement from .exceptions import ActiontypeNotAllowed, IgnoredRoute, PrefixTooLarge -from .serializers import ActionTypeSerializer, ClientSerializer, EntrySerializer, IgnoreEntrySerializer +from .serializers import ActionTypeSerializer, ClientSerializer, EntrySerializer, IgnoreEntrySerializer, NoActiveEntryFound channel_layer = get_channel_layer() logger = logging.getLogger(__name__) @@ -186,7 +186,7 @@ def get_object(self): entries = self.find_entries(pk, active_filter=True) if entries.count() != 1: - raise Http404 + raise NoActiveEntryFound return entries.first() From 03828914518aad47b8f3f847ac7b9110938b7bbb Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Tue, 21 Oct 2025 10:34:27 -0500 Subject: [PATCH 14/16] fix(exception): import exception from the exceptions file --- scram/route_manager/api/views.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scram/route_manager/api/views.py b/scram/route_manager/api/views.py index 91abef60..bc97aa11 100644 --- a/scram/route_manager/api/views.py +++ b/scram/route_manager/api/views.py @@ -16,8 +16,8 @@ from simple_history.utils import update_change_reason from ..models import ActionType, Client, Entry, IgnoreEntry, Route, WebSocketSequenceElement -from .exceptions import ActiontypeNotAllowed, IgnoredRoute, PrefixTooLarge -from .serializers import ActionTypeSerializer, ClientSerializer, EntrySerializer, IgnoreEntrySerializer, NoActiveEntryFound +from .exceptions import ActiontypeNotAllowed, IgnoredRoute, NoActiveEntryFound, PrefixTooLarge +from .serializers import ActionTypeSerializer, ClientSerializer, EntrySerializer, IgnoreEntrySerializer channel_layer = get_channel_layer() logger = logging.getLogger(__name__) From 5c79c8cda50de5531cb15a17a3e0c8b2514cb4a5 Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Tue, 21 Oct 2025 10:36:12 -0500 Subject: [PATCH 15/16] style(extraneous-import): remove import we dont use since we set up the custom exception --- scram/route_manager/api/views.py | 1 - 1 file changed, 1 deletion(-) diff --git a/scram/route_manager/api/views.py b/scram/route_manager/api/views.py index bc97aa11..d1da5f8d 100644 --- a/scram/route_manager/api/views.py +++ b/scram/route_manager/api/views.py @@ -8,7 +8,6 @@ from django.conf import settings from django.core.exceptions import PermissionDenied from django.db.models import Q -from django.http import Http404 from drf_spectacular.utils import extend_schema from rest_framework import status, viewsets from rest_framework.permissions import AllowAny, IsAuthenticated From 998d8dde2c376fb53bc9437d38c106eb7d997942 Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Fri, 31 Oct 2025 11:42:38 -0500 Subject: [PATCH 16/16] test(restrict_changes): set this back to a 405 error i think we do want to fail on the method, not on the user permissions. doing a 403 suggests that sometimes you can change an entry, but we always want to block PUT/PATCH as the client should just do a normal post every time and SCRAM handles the logic on our end. We don't want the client to have to track these things. --- .../tests/acceptance/features/restrict_changes.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scram/route_manager/tests/acceptance/features/restrict_changes.feature b/scram/route_manager/tests/acceptance/features/restrict_changes.feature index 09b06a70..bb24eddc 100644 --- a/scram/route_manager/tests/acceptance/features/restrict_changes.feature +++ b/scram/route_manager/tests/acceptance/features/restrict_changes.feature @@ -7,7 +7,7 @@ Feature: restrict changing entries When we're logged in And we add the entry 192.0.2.208 And we update the entry 192.0.2.208 to 192.0.2.209 - Then we get a 403 status code + Then we get a 405 status code And the number of entrys is 1 And 192.0.2.208 is announced by block translators And 192.0.2.209 is not announced by block translators