From 024510115d8df902d217dee59f1b846aa067fa07 Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Fri, 11 Apr 2025 11:15:32 -0500 Subject: [PATCH 01/10] fix(ipaddr): gracefully handle a cidr prepended with whitespace --- scram/route_manager/views.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/scram/route_manager/views.py b/scram/route_manager/views.py index 06d20a73..b28d9310 100644 --- a/scram/route_manager/views.py +++ b/scram/route_manager/views.py @@ -76,7 +76,12 @@ def search_entries(request): # Using ipaddress because we needed to turn off strict mode # (which netfields uses by default with seemingly no toggle) # This caused searches with host bits set to 500 which is bad UX see: 68854ee1ad4789a62863083d521bddbc96ab7025 - addr = ipaddress.ip_network(request.POST.get("cidr"), strict=False) + try: + addr = ipaddress.ip_network(request.POST.get("cidr"), strict=False) + except ValueError: + # leading space was breaking ipaddress module + str_addr = str(request.POST.get("cidr")).strip() + addr = ipaddress.ip_network(str_addr, strict=False) # We call home_page because search is just a more specific case of the same view and template to return. return home_page( request, From 3f484552af81c13d6c3c11507bce974e762b2dee Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Fri, 11 Apr 2025 11:35:49 -0500 Subject: [PATCH 02/10] fix(search): gracefully handle a blank search query as well --- scram/route_manager/views.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/scram/route_manager/views.py b/scram/route_manager/views.py index b28d9310..51bb4863 100644 --- a/scram/route_manager/views.py +++ b/scram/route_manager/views.py @@ -79,9 +79,14 @@ def search_entries(request): try: addr = ipaddress.ip_network(request.POST.get("cidr"), strict=False) except ValueError: - # leading space was breaking ipaddress module - str_addr = str(request.POST.get("cidr")).strip() - addr = ipaddress.ip_network(str_addr, strict=False) + try: + # leading space was breaking ipaddress module + str_addr = str(request.POST.get("cidr")).strip() + addr = ipaddress.ip_network(str_addr, strict=False) + except ValueError: + messages.add_message(request, messages.ERROR, "Search query was blank") + return redirect("route_manager:home") + # We call home_page because search is just a more specific case of the same view and template to return. return home_page( request, From fb020cead5dee0586b352346cfc6b52cae074f12 Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Fri, 11 Apr 2025 14:32:28 -0500 Subject: [PATCH 03/10] refactor(check_ip): change check_ip to actually test our search functionality directly we were sort of hacking around this by looking at entry detail for a specific ip instead of searching for it --- scram/route_manager/tests/acceptance/steps/ip.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/scram/route_manager/tests/acceptance/steps/ip.py b/scram/route_manager/tests/acceptance/steps/ip.py index 89247f6c..dcadd586 100644 --- a/scram/route_manager/tests/acceptance/steps/ip.py +++ b/scram/route_manager/tests/acceptance/steps/ip.py @@ -1,10 +1,13 @@ """Define steps used for IP-related logic by the Behave tests.""" import ipaddress +import logging from behave import then, when from django.urls import reverse +logger = logging.getLogger(__name__) + @then("{route} is contained in our list of {model}s") def check_route(context, route, model): @@ -25,12 +28,8 @@ def check_route(context, route, model): @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 + search_url = reverse("route_manager:search") + context.response = context.test.client.post(search_url, {"cidr": ip}, format="multipart", follow=True) @then("we get a ValueError") From 6f1b92fd3071e95e28061c51026a781cd8b89cf8 Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Fri, 11 Apr 2025 14:44:05 -0500 Subject: [PATCH 04/10] test(prefixlen-test): remove unnecessary prefix length on search test --- .../tests/acceptance/features/query.feature | 20 +++---------------- 1 file changed, 3 insertions(+), 17 deletions(-) diff --git a/scram/route_manager/tests/acceptance/features/query.feature b/scram/route_manager/tests/acceptance/features/query.feature index 534399fb..e4bb076f 100644 --- a/scram/route_manager/tests/acceptance/features/query.feature +++ b/scram/route_manager/tests/acceptance/features/query.feature @@ -37,28 +37,13 @@ Feature: we can query the list of entries for a specific entry | 2001:DB8:950C::/48 | 2001:DB8:950C::1/128 | | 2001:DB8:950D::/48 | 2001:DB8:950D::1/64 | - Scenario Outline: we cant query larger than our prefixmin - Given a client with block authorization - When we're logged in - And the CIDR prefix limits are 24 and 48 - And we add the entry - And the CIDR prefix limits are 32 and 128 - And we query for - Then we get a 400 status code - - Examples: IPs - | ip | - | 192.0.2.0/24 | - | 2001:DB8::/48 | - - Scenario Outline: we cant enter malformed IPs + Scenario Outline: we cant query for malformed IPs and get redirected back to the home page Given a client with block authorization When we're logged in And we add the entry And we query for - Then we get a ValueError - + Then we get a 200 status code Examples: IPs | ip | @@ -66,3 +51,4 @@ Feature: we can query the list of entries for a specific entry | 193.168.0.0/33 | | 2001::::: | | 201::0/129 | + | gibberish_ip | From 14fd4337247262996f34f75fb3eaaf7d7a08828b Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Fri, 11 Apr 2025 14:45:38 -0500 Subject: [PATCH 05/10] fix(searching): better handle invalid search queries --- scram/route_manager/views.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/scram/route_manager/views.py b/scram/route_manager/views.py index 51bb4863..515a6a10 100644 --- a/scram/route_manager/views.py +++ b/scram/route_manager/views.py @@ -76,6 +76,9 @@ def search_entries(request): # Using ipaddress because we needed to turn off strict mode # (which netfields uses by default with seemingly no toggle) # This caused searches with host bits set to 500 which is bad UX see: 68854ee1ad4789a62863083d521bddbc96ab7025 + if request.method != "POST": + return redirect("route_manager:home") + try: addr = ipaddress.ip_network(request.POST.get("cidr"), strict=False) except ValueError: @@ -84,7 +87,8 @@ def search_entries(request): str_addr = str(request.POST.get("cidr")).strip() addr = ipaddress.ip_network(str_addr, strict=False) except ValueError: - messages.add_message(request, messages.ERROR, "Search query was blank") + messages.add_message(request, messages.ERROR, "Search query was not a valid CIDR address") + return redirect("route_manager:home") # We call home_page because search is just a more specific case of the same view and template to return. From 420a02c79f1b2e446f159858d97c184f7cda7e20 Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Fri, 11 Apr 2025 15:17:40 -0500 Subject: [PATCH 06/10] test(whoops): rever check_ip and the query.feature scenario i removed it turns out they were testing the find_entries feature used in the API for deactivating routes --- .../tests/acceptance/features/query.feature | 19 ++++++++++++++++--- .../tests/acceptance/steps/ip.py | 11 ++++++----- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/scram/route_manager/tests/acceptance/features/query.feature b/scram/route_manager/tests/acceptance/features/query.feature index e4bb076f..df8ccf16 100644 --- a/scram/route_manager/tests/acceptance/features/query.feature +++ b/scram/route_manager/tests/acceptance/features/query.feature @@ -37,13 +37,27 @@ Feature: we can query the list of entries for a specific entry | 2001:DB8:950C::/48 | 2001:DB8:950C::1/128 | | 2001:DB8:950D::/48 | 2001:DB8:950D::1/64 | + # This is to make sure the code path for deactivating an entry works correctly + Scenario Outline: we cant query larger than our prefix min + Given a client with block authorization + When we're logged in + And the CIDR prefix limits are 24 and 48 + And we add the entry + And the CIDR prefix limits are 32 and 128 + And we query for + Then we get a 400 status code - Scenario Outline: we cant query for malformed IPs and get redirected back to the home page + Examples: IPs + | ip | + | 192.0.2.0/24 | + | 2001:DB8::/48 | + + Scenario Outline: we cant search malformed IPs and get redirected to the home page Given a client with block authorization When we're logged in And we add the entry And we query for - Then we get a 200 status code + Then we get a ValueError Examples: IPs | ip | @@ -51,4 +65,3 @@ Feature: we can query the list of entries for a specific entry | 193.168.0.0/33 | | 2001::::: | | 201::0/129 | - | gibberish_ip | diff --git a/scram/route_manager/tests/acceptance/steps/ip.py b/scram/route_manager/tests/acceptance/steps/ip.py index dcadd586..89247f6c 100644 --- a/scram/route_manager/tests/acceptance/steps/ip.py +++ b/scram/route_manager/tests/acceptance/steps/ip.py @@ -1,13 +1,10 @@ """Define steps used for IP-related logic by the Behave tests.""" import ipaddress -import logging from behave import then, when from django.urls import reverse -logger = logging.getLogger(__name__) - @then("{route} is contained in our list of {model}s") def check_route(context, route, model): @@ -28,8 +25,12 @@ def check_route(context, route, model): @when("we query for {ip}") def check_ip(context, ip): """Find an Entry for the specified IP.""" - search_url = reverse("route_manager:search") - context.response = context.test.client.post(search_url, {"cidr": ip}, format="multipart", follow=True) + 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") From afa57a53b29afb4d1204bf18e37d92e86094e28d Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Mon, 14 Apr 2025 10:40:46 -0500 Subject: [PATCH 07/10] test(WUI-search): test our search bar both for valid and invalid search queries adds the required scaffolding like using the django test client for WUI stuff vs API --- .../tests/acceptance/environment.py | 2 ++ .../tests/acceptance/features/query.feature | 2 +- .../tests/acceptance/features/search.feature | 25 +++++++++++++++++++ .../tests/acceptance/steps/common.py | 8 +++++- .../tests/acceptance/steps/ip.py | 9 +++++++ 5 files changed, 44 insertions(+), 2 deletions(-) create mode 100644 scram/route_manager/tests/acceptance/features/search.feature diff --git a/scram/route_manager/tests/acceptance/environment.py b/scram/route_manager/tests/acceptance/environment.py index 0d31198b..baf6776e 100644 --- a/scram/route_manager/tests/acceptance/environment.py +++ b/scram/route_manager/tests/acceptance/environment.py @@ -1,5 +1,6 @@ """Setup the environment for tests.""" +from django.test import Client from rest_framework.test import APIClient from scram.users.tests.factories import UserFactory @@ -12,3 +13,4 @@ def django_ready(context): # Default to using the API client. context.test.client = APIClient() + context.test.web_client = Client() diff --git a/scram/route_manager/tests/acceptance/features/query.feature b/scram/route_manager/tests/acceptance/features/query.feature index df8ccf16..d631bd3e 100644 --- a/scram/route_manager/tests/acceptance/features/query.feature +++ b/scram/route_manager/tests/acceptance/features/query.feature @@ -52,7 +52,7 @@ Feature: we can query the list of entries for a specific entry | 192.0.2.0/24 | | 2001:DB8::/48 | - Scenario Outline: we cant search malformed IPs and get redirected to the home page + Scenario Outline: we get the proper error when querying an invalid cidr Given a client with block authorization When we're logged in And we add the entry diff --git a/scram/route_manager/tests/acceptance/features/search.feature b/scram/route_manager/tests/acceptance/features/search.feature new file mode 100644 index 00000000..1d414fe4 --- /dev/null +++ b/scram/route_manager/tests/acceptance/features/search.feature @@ -0,0 +1,25 @@ +Feature: Test our search bar + We need to make sure search works as expected + + Scenario Outline: Searching for a valid CIDR works even with prepending white space + Given a client with block authorization + When we're logged in + And we add the entry + And we search for + Then we get a 200 status code + + Examples: IPs + | ip | + | 192.0.2.168 | + | 2001:DB8:9508::1 | + + Scenario Outline: Searching for an invalid CIDR redirects to the home page and sends a 400 + Given a client with block authorization + When we're logged in + And we search for + Then we get a 400 status code + + Examples: IPs + | ip | + | " " | + | asdf | diff --git a/scram/route_manager/tests/acceptance/steps/common.py b/scram/route_manager/tests/acceptance/steps/common.py index f6c43fc7..cb803ba6 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") @@ -54,6 +59,7 @@ def create_unauthed_client(context, name): 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}") diff --git a/scram/route_manager/tests/acceptance/steps/ip.py b/scram/route_manager/tests/acceptance/steps/ip.py index 89247f6c..b8fda111 100644 --- a/scram/route_manager/tests/acceptance/steps/ip.py +++ b/scram/route_manager/tests/acceptance/steps/ip.py @@ -48,3 +48,12 @@ def check_comment(context, value, comment): except ValueError as e: context.response = None context.queryException = e + + +@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}) From 0de27acf3d39f2d03e29b8e4fd9923a5351a174d Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Mon, 14 Apr 2025 10:41:28 -0500 Subject: [PATCH 08/10] refactor(search-response): send a proper bad request response for an invalid CIDR, but don't show a 400 error page --- scram/route_manager/views.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/scram/route_manager/views.py b/scram/route_manager/views.py index 515a6a10..50abbd7f 100644 --- a/scram/route_manager/views.py +++ b/scram/route_manager/views.py @@ -15,7 +15,7 @@ from django.contrib.auth.mixins import PermissionRequiredMixin from django.core.paginator import EmptyPage, PageNotAnInteger, Paginator from django.db import transaction -from django.http import HttpResponse +from django.http import HttpResponse, HttpResponseBadRequest from django.shortcuts import redirect, render from django.utils import timezone from django.views.decorators.http import require_POST @@ -73,13 +73,13 @@ def home_page(request, prefilter=None): def search_entries(request): """Wrap the home page with a specified CIDR to restrict Entries to.""" - # Using ipaddress because we needed to turn off strict mode - # (which netfields uses by default with seemingly no toggle) - # This caused searches with host bits set to 500 which is bad UX see: 68854ee1ad4789a62863083d521bddbc96ab7025 if request.method != "POST": return redirect("route_manager:home") try: + # Using ipaddress because we needed to turn off strict mode + # (which netfields uses by default with seemingly no toggle) + # This caused searches with host bits set to 500 which is bad UX see: 68854ee1ad4789a62863083d521bddbc96ab7025 addr = ipaddress.ip_network(request.POST.get("cidr"), strict=False) except ValueError: try: @@ -89,7 +89,8 @@ def search_entries(request): except ValueError: messages.add_message(request, messages.ERROR, "Search query was not a valid CIDR address") - return redirect("route_manager:home") + # Send a 400, but show the home page instead of an error page + return HttpResponseBadRequest(render(request, "route_manager/home.html")) # We call home_page because search is just a more specific case of the same view and template to return. return home_page( From 2a9dda945e2b78298ddf153aec911de4d50f05b4 Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Mon, 14 Apr 2025 10:53:14 -0500 Subject: [PATCH 09/10] docs(comments): update comments to match reality --- scram/route_manager/tests/acceptance/features/search.feature | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scram/route_manager/tests/acceptance/features/search.feature b/scram/route_manager/tests/acceptance/features/search.feature index 1d414fe4..44be67a3 100644 --- a/scram/route_manager/tests/acceptance/features/search.feature +++ b/scram/route_manager/tests/acceptance/features/search.feature @@ -1,7 +1,7 @@ Feature: Test our search bar We need to make sure search works as expected - Scenario Outline: Searching for a valid CIDR works even with prepending white space + Scenario Outline: Searching for a valid CIDR works Given a client with block authorization When we're logged in And we add the entry @@ -13,7 +13,7 @@ Feature: Test our search bar | 192.0.2.168 | | 2001:DB8:9508::1 | - Scenario Outline: Searching for an invalid CIDR redirects to the home page and sends a 400 + Scenario Outline: Searching for an invalid CIDR sends a 400 Given a client with block authorization When we're logged in And we search for From 3d24862a5ce7cf258dde0850723304e1fbf2acbe Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Mon, 14 Apr 2025 11:17:44 -0500 Subject: [PATCH 10/10] docs(humanize): write a more human readable description of the test --- scram/route_manager/tests/acceptance/features/search.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scram/route_manager/tests/acceptance/features/search.feature b/scram/route_manager/tests/acceptance/features/search.feature index 44be67a3..c02d5a01 100644 --- a/scram/route_manager/tests/acceptance/features/search.feature +++ b/scram/route_manager/tests/acceptance/features/search.feature @@ -13,7 +13,7 @@ Feature: Test our search bar | 192.0.2.168 | | 2001:DB8:9508::1 | - Scenario Outline: Searching for an invalid CIDR sends a 400 + Scenario Outline: Searching for an invalid CIDR returns a Bad Request error Given a client with block authorization When we're logged in And we search for