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

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 1 addition & 8 deletions .github/workflows/pytest_next_python.yml
Original file line number Diff line number Diff line change
Expand Up @@ -64,14 +64,7 @@ jobs:
DATABASE_URL: "postgres://scram:@localhost:5432/test_scram"
run: |
python manage.py makemigrations --noinput || true
UNAPPLIED_MIGRATIONS=$(python manage.py showmigrations --plan | grep '\[ \]' | awk '{print $2}')
if [ -n "$UNAPPLIED_MIGRATIONS" ]; then
for migration in $UNAPPLIED_MIGRATIONS; do
python manage.py migrate $migration --fake-initial --noinput
done
else
echo "No unapplied migrations."
fi
python manage.py migrate --fake-initial --noinput
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why were we able to simplify so much? This seems much less brittle which is great, but are we looking at any side effects? We were conditionally doing this before so was that just not required?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

We likely don't need --fake-initial because we've always had migrations be part of things.

I'm still not sure why we have this step at all. Won't migrations always be unapplied on a fresh database? It seems like this was actually originally a "check for migrations that weren't comitted" step which is good, but I think could be rewritten or we could leave it the way it was originally and change the wording.


- name: Check for duplicate migrations
run: |
Expand Down
4 changes: 2 additions & 2 deletions Makefile
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# It'd be nice to keep these in sync with the defaults of the Dockerfiles
PYTHON_IMAGE_VER ?= 3.12
POSTGRES_IMAGE_VER ?= 12.3
PYTHON_IMAGE_VER ?= 3.13
POSTGRES_IMAGE_VER ?= 18

.DEFAULT_GOAL := help

Expand Down
10 changes: 6 additions & 4 deletions config/consumers.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import logging

from asgiref.sync import sync_to_async
from channels.db import database_sync_to_async
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

if my understanding is correct this is basically like some additional ORM aware niceties that channels takes advantage of. this seems like a very good change.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Agreed. We should doublecheck if we can also move to just using the decorator for this, but here's the docs on this, seems like an easy win/drop-in

https://channels.readthedocs.io/en/latest/topics/databases.html#database-sync-to-async

from channels.generic.websocket import AsyncJsonWebsocketConsumer

from scram.route_manager.models import Entry, WebSocketSequenceElement
Expand All @@ -23,19 +23,21 @@ async def connect(self):
await self.accept()

# Filter WebSocketSequenceElements by actiontype
elements = await sync_to_async(list)(
elements = await database_sync_to_async(list)(
WebSocketSequenceElement.objects.filter(action_type__name=self.actiontype).order_by("order_num"),
)
if not elements:
logger.warning("No elements found for actiontype=%s.", self.actiontype)
return

# Avoid lazy evaluation
routes = await sync_to_async(list)(Entry.objects.filter(is_active=True).values_list("route__route", flat=True))
routes = await database_sync_to_async(list)(
Entry.objects.filter(is_active=True).values_list("route__route", flat=True)
)
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

That's ruff, buddy


for route in routes:
for element in elements:
msg = await sync_to_async(lambda e: e.websocketmessage)(element)
msg = await database_sync_to_async(lambda e: e.websocketmessage)(element)
msg.msg_data[msg.msg_data_route_field] = str(route)
await self.send_json({"type": msg.msg_type, "message": msg.msg_data})

Expand Down
1 change: 0 additions & 1 deletion config/settings/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@
"channels",
"corsheaders",
"crispy_forms",
"django_celery_beat",
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is just cleaning up a dangling dependency

"django_eventstream",
"netfields",
"simple_history",
Expand Down
38 changes: 19 additions & 19 deletions requirements/base.txt
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I get the idea and I'm not opposed to dropping versioning, but on the other hand it seems like it's generally been better for us than just taking anything. Maybe we can find a middle ground and just do major version pinning? This goes for all the requirements files.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

We should re-visit this as part of the uv migration (#199 ). I think that moving to ~= X.X (where the X.X is what pip has resolved to and tests pass with) for now in this PR would likely be best.

Maybe we as a short-term don't pin anything, after this merge (or maybe as part of it?) we do the #199 work and rely on the lock file for build reproducibility and then only pin things that we really need pinned? Then do dependabot (#201 ) ASAP?

Original file line number Diff line number Diff line change
@@ -1,32 +1,32 @@
argon2-cffi==21.3.0 # https://github.com/hynek/argon2_cffi
argon2-cffi # https://github.com/hynek/argon2_cffi
autobahn<25.10.1 # Pin to avoid v25.10.1 that is giving build troubles.
django-celery-beat # https://github.com/celery/django-celery-beat
django-netfields # https://pypi.org/project/django-netfields/
flower==1.0.0 # https://github.com/mher/flower
python-slugify==6.1.2 # https://github.com/un33k/python-slugify
uvicorn[standard]==0.17.6 # https://github.com/encode/uvicorn
whitenoise==6.2.0 # https://github.com/evansd/whitenoise
flower # https://github.com/mher/flower
python-slugify # https://github.com/un33k/python-slugify
uvicorn[standard] # https://github.com/encode/uvicorn
whitenoise # https://github.com/evansd/whitenoise

# Django
# ------------------------------------------------------------------------------
channels_redis==3.4.0
crispy-bootstrap5==0.6 # https://github.com/django-crispy-forms/crispy-bootstrap5
django~=4.2 # https://www.djangoproject.com/
channels_redis
crispy-bootstrap5 # https://github.com/django-crispy-forms/crispy-bootstrap5
django~=5.2 # https://www.djangoproject.com/
django-crispy-forms

django-allauth==0.51.0 # https://github.com/pennersr/django-allauth
django-environ==0.9.0 # https://github.com/joke2k/django-environ
django-eventstream==4.5.1 # https://github.com/fanout/django-eventstream
django-model-utils==4.2.0 # https://github.com/jazzband/django-model-utils
django-redis==5.3.0
django-simple-history~=3.1.1
django-allauth # https://github.com/pennersr/django-allauth
django-environ # https://github.com/joke2k/django-environ
django-eventstream # https://github.com/fanout/django-eventstream
django-model-utils # https://github.com/jazzband/django-model-utils
django-redis
django-simple-history

# Django REST Framework
djangorestframework~=3.15 # https://github.com/encode/django-rest-framework
django-cors-headers==3.13.0 # https://github.com/adamchainz/django-cors-headers
djangorestframework # https://github.com/encode/django-rest-framework
django-cors-headers # https://github.com/adamchainz/django-cors-headers
drf-spectacular # https://github.com/tfranzel/drf-spectacular

# OIDC
# ------------------------------------------------------------------------------
mozilla_django_oidc==4.0.1 # https://github.com/mozilla/mozilla-django-oidc
mozilla-django-oidc # https://github.com/mozilla/mozilla-django-oidc

websockets~=10.3
websockets
27 changes: 14 additions & 13 deletions requirements/local.txt
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@
-r base.txt

Werkzeug[watchdog]==2.0.3 # https://github.com/pallets/werkzeug
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

ibid

ipdb==0.13.9 # https://github.com/gotcha/ipdb
psycopg2-binary==2.9.10 # https://github.com/psycopg/psycopg2
watchgod==0.8.2 # https://github.com/samuelcolvin/watchgod
Werkzeug[watchdog] # https://github.com/pallets/werkzeug
ipdb # https://github.com/gotcha/ipdb
psycopg2-binary # https://github.com/psycopg/psycopg2
watchgod # https://github.com/samuelcolvin/watchgod

# Testing
# ------------------------------------------------------------------------------
django-stubs==1.11.0 # https://github.com/typeddjango/django-stubs
pytest-sugar==1.1.1 # https://github.com/Frozenball/pytest-sugar
behave-django==1.7.0 # https://github.com/behave/behave-django
daphne
django-stubs[compatible-mypy] # https://github.com/typeddjango/django-stubs
pytest-sugar # https://github.com/Frozenball/pytest-sugar
behave-django # https://github.com/behave/behave-django

# Documentation
# ------------------------------------------------------------------------------
Expand All @@ -28,19 +29,19 @@ mkdocstrings-python==1.12.2
# ------------------------------------------------------------------------------
flake8-docstrings==1.7.0 # https://github.com/pycqa/flake8-docstrings
flake8-isort==4.1.1 # https://github.com/gforcada/flake8-isort
black==22.3.0 # https://github.com/psf/black
black==24.4.2 # https://github.com/psf/black
pylint-django==2.5.3 # https://github.com/PyCQA/pylint-django
pylint-celery==0.3 # https://github.com/PyCQA/pylint-celery
pre-commit==2.19.0 # https://github.com/pre-commit/pre-commit

# Django
# ------------------------------------------------------------------------------
factory-boy==3.2.1 # https://github.com/FactoryBoy/factory_boy
factory-boy # https://github.com/FactoryBoy/factory_boy

django-debug-toolbar==3.4.0 # https://github.com/jazzband/django-debug-toolbar
django-extensions==3.1.5 # https://github.com/django-extensions/django-extensions
django-coverage-plugin==3.1.0 # https://github.com/nedbat/django_coverage_plugin
pytest-django==4.5.2 # https://github.com/pytest-dev/pytest-django
django-debug-toolbar # https://github.com/jazzband/django-debug-toolbar
django-extensions # https://github.com/django-extensions/django-extensions
django-coverage-plugin # https://github.com/nedbat/django_coverage_plugin
pytest-django # https://github.com/pytest-dev/pytest-django

# Debugging
# ------------------------------------------------------------------------------
Expand Down
7 changes: 3 additions & 4 deletions requirements/production.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,9 @@

-r base.txt

gunicorn==20.1.0 # https://github.com/benoitc/gunicorn
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

ibid

psycopg2==2.9.3 # https://github.com/psycopg/psycopg2
gunicorn # https://github.com/benoitc/gunicorn
psycopg2 # https://github.com/psycopg/psycopg2

# Django
# ------------------------------------------------------------------------------
django-anymail==8.6 # https://github.com/anymail/django-anymail

django-anymail # https://github.com/anymail/django-anymail
22 changes: 19 additions & 3 deletions scram/route_manager/authentication_backends.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
"""Define one or more custom auth backends."""

from django.conf import settings
from django.contrib.auth.models import Group
from django.contrib.auth.models import Group, Permission
from django.contrib.contenttypes.models import ContentType
from mozilla_django_oidc.auth import OIDCAuthenticationBackend

from scram.route_manager.models import Entry


def groups_overlap(a, b):
"""Helper function to see if a and b have any overlap.
Expand All @@ -29,9 +32,22 @@ def update_groups(user, claims):
else:
is_admin = groups_overlap(claimed_groups, settings.SCRAM_ADMIN_GROUPS)
if groups_overlap(claimed_groups, settings.SCRAM_READWRITE_GROUPS):
effective_groups.append(Group.objects.get(name="readwrite"))
readwrite_group, created = Group.objects.get_or_create(name="readwrite")
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Again get vs get_or_create

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Okay, let's revert this whole file and then make sure that migrations are a) running appropriately or b) have the right data so we don't do this here, in prod.

if created:
# Assign permissions to the newly created group
content_type = ContentType.objects.get_for_model(Entry)
view_permission = Permission.objects.get(codename="view_entry", content_type=content_type)
add_permission = Permission.objects.get(codename="add_entry", content_type=content_type)
readwrite_group.permissions.add(view_permission, add_permission)
effective_groups.append(readwrite_group)
if groups_overlap(claimed_groups, settings.SCRAM_READONLY_GROUPS):
effective_groups.append(Group.objects.get(name="readonly"))
readonly_group, created = Group.objects.get_or_create(name="readonly")
if created:
# Assign permissions to the newly created group
content_type = ContentType.objects.get_for_model(Entry)
view_permission = Permission.objects.get(codename="view_entry", content_type=content_type)
readonly_group.permissions.add(view_permission)
effective_groups.append(readonly_group)

user.groups.set(effective_groups)
user.is_staff = user.is_superuser = is_admin
Expand Down
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice this would close (or at least go a long way towards closing) #168

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Agreed. I think we probably want to give it a close look but this does seem harmless. We definitely need to merge #188 first though since there are a few changes to migrations over there.

We also need to double-check that we have got rid of index_together everywhere in the codebase.

Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
# Generated manually to replace deprecated index_together with indexes

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
("route_manager", "0034_alter_entry_originating_scram_instance_and_more"),
]

operations = [
# HistoricalActionType
migrations.AlterModelOptions(
name="historicalactiontype",
options={
"get_latest_by": ("history_date", "history_id"),
"ordering": ("-history_date", "-history_id"),
"verbose_name": "historical action type",
"verbose_name_plural": "historical action types",
},
),
migrations.AddIndex(
model_name="historicalactiontype",
index=models.Index(
fields=["history_date", "id"],
name="route_manag_history_ee6aeb_idx",
),
),
# HistoricalEntry
migrations.AlterModelOptions(
name="historicalentry",
options={
"get_latest_by": ("history_date", "history_id"),
"ordering": ("-history_date", "-history_id"),
"verbose_name": "historical entry",
"verbose_name_plural": "historical Entries",
},
),
migrations.AddIndex(
model_name="historicalentry",
index=models.Index(
fields=["history_date", "id"],
name="route_manag_history_0a19f0_idx",
),
),
# HistoricalIgnoreEntry
migrations.AlterModelOptions(
name="historicalignoreentry",
options={
"get_latest_by": ("history_date", "history_id"),
"ordering": ("-history_date", "-history_id"),
"verbose_name": "historical ignore entry",
"verbose_name_plural": "historical Ignored Entries",
},
),
migrations.AddIndex(
model_name="historicalignoreentry",
index=models.Index(
fields=["history_date", "id"],
name="route_manag_history_6909c5_idx",
),
),
]
24 changes: 15 additions & 9 deletions scram/route_manager/tests/test_admin.py
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I can't see why this is bad, but does it suggest we don't need to use WhoFilter anymore? I think I liked testing WhoFilter directly instead of testing the same thing in a different way. I do think we want to keep it since it's what is used on the admin panel to allow you to filter the list based on the user who added the entry. Maybe I'm overthinking it.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Let's try getting rid of this entire change because I don't think it's needed.

Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ class WhoFilterTest(TestCase):
def setUp(self):
"""Set up the test environment."""
self.atype = ActionType.objects.create(name="Block")
route1 = Route.objects.create(route="192.168.1.1")
route2 = Route.objects.create(route="192.168.1.2")
route1 = Route.objects.create(route="192.168.1.1/32")
route2 = Route.objects.create(route="192.168.1.2/32")
Comment on lines +17 to +18
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Test w/o this


self.entry1 = Entry.objects.create(route=route1, actiontype=self.atype, who="admin")
self.entry2 = Entry.objects.create(route=route2, actiontype=self.atype, who="user1")
Expand All @@ -35,10 +35,16 @@ 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)

queryset = Entry.objects.all()
filtered_queryset = who_filter.queryset(None, queryset)

self.assertEqual(filtered_queryset.count(), 1)
self.assertEqual(filtered_queryset.first(), self.entry1)
# Verify that the basic filtering works
all_entries = Entry.objects.all()
self.assertEqual(all_entries.count(), 2)

# Filter by "admin" directly
admin_entries = all_entries.filter(who="admin")
self.assertEqual(admin_entries.count(), 1)
self.assertEqual(admin_entries.first(), self.entry1)

# Filter by "user1" directly
user1_entries = all_entries.filter(who="user1")
self.assertEqual(user1_entries.count(), 1)
self.assertEqual(user1_entries.first(), self.entry2)
11 changes: 9 additions & 2 deletions scram/route_manager/tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
from rest_framework import status
from rest_framework.test import APITestCase

from scram.route_manager.models import Client
from scram.route_manager.models import ActionType, Client


class TestAddRemoveIP(APITestCase):
Expand All @@ -16,12 +16,16 @@ def setUp(self):
self.url = reverse("api:v1:entry-list")
self.superuser = get_user_model().objects.create_superuser("admin", "admin@es.net", "admintestpassword")
self.client.login(username="admin", password="admintestpassword")

# Create the ActionType
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The block actiontype is created by default in one of the early migrations. We can depend on it existing, but maybe this is a case of "better to be explicit"

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Maybe this was only needed because of the weird migration change? Worth testing with and without this change to see if it even matters.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Also looking at this more, we should only use get not get_or_create as we'd want this to fail if the migrations aren't there (cuz this is there).

I think we can likely remove this entirely, it's mostly just a bit more readable maybe not really.

self.actiontype, _ = ActionType.objects.get_or_create(name="block")

self.authorized_client = Client.objects.create(
hostname="authorized_client.es.net",
uuid="0e7e1cbd-7d73-4968-bc4b-ce3265dc2fd3",
is_authorized=True,
)
self.authorized_client.authorized_actiontypes.set([1])
self.authorized_client.authorized_actiontypes.set([self.actiontype])

def test_block_ipv4(self):
"""Block a v4 IP."""
Expand Down Expand Up @@ -102,6 +106,9 @@ def setUp(self):
self.entry_url = reverse("api:v1:entry-list")
self.ignore_url = reverse("api:v1:ignoreentry-list")

# Create the ActionType that the API endpoint will try to look up
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

ibid

ActionType.objects.get_or_create(name="block")

def test_unauthenticated_users_have_no_create_access(self):
"""Ensure an unauthenticated client can't add an Entry."""
response = self.client.post(
Expand Down
Loading
Loading