From 98a907a418a303d8324de250467c0080889797be Mon Sep 17 00:00:00 2001 From: chriscummings Date: Fri, 8 Nov 2024 13:38:59 -0600 Subject: [PATCH 01/25] chore: bump to django 4.2 --- requirements/base.txt | 12 ++++++------ scram/users/tests/test_forms.py | 2 +- scram/users/tests/test_views.py | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/requirements/base.txt b/requirements/base.txt index cd489161..c378cfc5 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -1,21 +1,21 @@ pytz==2022.1 # https://github.com/stub42/pytz -django-netfields==1.2.2 # https://pypi.org/project/django-netfields/ +django-netfields # https://pypi.org/project/django-netfields/ python-slugify==6.1.2 # https://github.com/un33k/python-slugify Pillow==9.1.1 # https://github.com/python-pillow/Pillow argon2-cffi==21.3.0 # https://github.com/hynek/argon2_cffi whitenoise==6.2.0 # https://github.com/evansd/whitenoise celery==5.2.7 # pyup: < 6.0 # https://github.com/celery/celery -django-celery-beat==2.3.0 # https://github.com/celery/django-celery-beat +django-celery-beat # https://github.com/celery/django-celery-beat flower==1.0.0 # https://github.com/mher/flower uvicorn[standard]==0.17.6 # https://github.com/encode/uvicorn -asgiref~=3.5.0 # https://github.com/django/asgiref +# asgiref~=3.5.0 # https://github.com/django/asgiref # Django # ------------------------------------------------------------------------------ channels==3.0.4 channels_redis==3.4.0 django-redis==5.3.0 -django==3.2.13 # pyup: < 4.0 # https://www.djangoproject.com/ +django~=4.2 # https://www.djangoproject.com/ django-grip==3.4.0 django-eventstream==4.5.1 # https://github.com/fanout/django-eventstream django-environ==0.9.0 # https://github.com/joke2k/django-environ @@ -25,10 +25,10 @@ django-crispy-forms==1.14.0 # https://github.com/django-crispy-forms/django-cri crispy-bootstrap5==0.6 # https://github.com/django-crispy-forms/crispy-bootstrap5 # Django REST Framework -djangorestframework==3.13.1 # https://github.com/encode/django-rest-framework +# djangorestframework==3.13.1 # https://github.com/encode/django-rest-framework django-cors-headers==3.13.0 # https://github.com/adamchainz/django-cors-headers # DRF-spectacular for api documentation -drf-spectacular==0.22.1 # https://github.com/tfranzel/drf-spectacular +drf-spectacular # https://github.com/tfranzel/drf-spectacular # OIDC # ------------------------------------------------------------------------------ diff --git a/scram/users/tests/test_forms.py b/scram/users/tests/test_forms.py index 8041e621..6d0b248b 100644 --- a/scram/users/tests/test_forms.py +++ b/scram/users/tests/test_forms.py @@ -2,7 +2,7 @@ Module for all Form Tests. """ import pytest -from django.utils.translation import ugettext_lazy as _ +from django.utils.translation import gettext_lazy as _ from scram.users.forms import UserCreationForm from scram.users.models import User diff --git a/scram/users/tests/test_views.py b/scram/users/tests/test_views.py index 708f35e5..485370d2 100644 --- a/scram/users/tests/test_views.py +++ b/scram/users/tests/test_views.py @@ -47,8 +47,8 @@ def test_form_valid(self, user: User, rf: RequestFactory): request = rf.get("/fake-url/") # Add the session/message middleware to the request - SessionMiddleware().process_request(request) - MessageMiddleware().process_request(request) + SessionMiddleware(get_response=request).process_request(request) + MessageMiddleware(get_response=request).process_request(request) request.user = user view.request = request From 75190885f996e01bee79f457d77b7e1361124881 Mon Sep 17 00:00:00 2001 From: chriscummings Date: Thu, 14 Nov 2024 10:40:29 -0600 Subject: [PATCH 02/25] chore(deps): bump python image --- compose/local/django/Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compose/local/django/Dockerfile b/compose/local/django/Dockerfile index ca5e2cc7..64bd4a6b 100644 --- a/compose/local/django/Dockerfile +++ b/compose/local/django/Dockerfile @@ -1,4 +1,4 @@ -FROM python:3.8-slim-buster +FROM python:3.12-slim-bookworm ENV PYTHONUNBUFFERED 1 ENV PYTHONDONTWRITEBYTECODE 1 From 6c665543c701bc9754077a930239cd1521d38ded Mon Sep 17 00:00:00 2001 From: chriscummings Date: Thu, 14 Nov 2024 11:09:55 -0600 Subject: [PATCH 03/25] chore(deps): remove pillow pin (it's included by other things) --- requirements/base.txt | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/requirements/base.txt b/requirements/base.txt index c378cfc5..50c1a84a 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -1,7 +1,6 @@ pytz==2022.1 # https://github.com/stub42/pytz django-netfields # https://pypi.org/project/django-netfields/ python-slugify==6.1.2 # https://github.com/un33k/python-slugify -Pillow==9.1.1 # https://github.com/python-pillow/Pillow argon2-cffi==21.3.0 # https://github.com/hynek/argon2_cffi whitenoise==6.2.0 # https://github.com/evansd/whitenoise celery==5.2.7 # pyup: < 6.0 # https://github.com/celery/celery @@ -25,7 +24,7 @@ django-crispy-forms==1.14.0 # https://github.com/django-crispy-forms/django-cri crispy-bootstrap5==0.6 # https://github.com/django-crispy-forms/crispy-bootstrap5 # Django REST Framework -# djangorestframework==3.13.1 # https://github.com/encode/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 # DRF-spectacular for api documentation drf-spectacular # https://github.com/tfranzel/drf-spectacular From a3e6c4e751e6f411372d1699dc4a43b60146f696 Mon Sep 17 00:00:00 2001 From: chriscummings Date: Thu, 14 Nov 2024 13:05:05 -0600 Subject: [PATCH 04/25] chore(deps): Remove more frozen downstream deps --- requirements/base.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/requirements/base.txt b/requirements/base.txt index 50c1a84a..62756c66 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -7,7 +7,6 @@ celery==5.2.7 # pyup: < 6.0 # https://github.com/celery/celery django-celery-beat # https://github.com/celery/django-celery-beat flower==1.0.0 # https://github.com/mher/flower uvicorn[standard]==0.17.6 # https://github.com/encode/uvicorn -# asgiref~=3.5.0 # https://github.com/django/asgiref # Django # ------------------------------------------------------------------------------ From 39ba31cca13a903247b271f2b8e486f5dbbe78d8 Mon Sep 17 00:00:00 2001 From: chriscummings Date: Fri, 15 Nov 2024 09:41:20 -0600 Subject: [PATCH 05/25] ci(docs): only build on main --- .github/workflows/docs.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/docs.yml b/.github/workflows/docs.yml index 1f7e14f0..d3fbb051 100644 --- a/.github/workflows/docs.yml +++ b/.github/workflows/docs.yml @@ -3,7 +3,7 @@ name: Build sphinx docs on: push: branches: - - '**' + - 'main' # Allows you to run this workflow manually from the Actions tab workflow_dispatch: From 6fa56482073d3430fe82b218e5d5f94dcccdec44 Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Fri, 15 Nov 2024 20:05:09 -0600 Subject: [PATCH 06/25] feat(gobgp-neighbor): add makefile target for debugging gobgp neighbors --- Makefile | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Makefile b/Makefile index 74556490..26b6f3a0 100644 --- a/Makefile +++ b/Makefile @@ -85,6 +85,11 @@ down: compose.override.yml exec: compose.override.yml @docker compose exec $(CONTAINER) $(COMMAND) +## gobgp-neighbor: shows the gobgp neighbor information (append neighbor IP for specific information) +.Phony: gobgp-neighbor +gobgp-neighbor: compose.override.yml + @docker compose exec gobgp gobgp neighbor $(NEIGHBOR) + # This automatically builds the help target based on commands prepended with a double hashbang ## help: print this help output .Phony: help From ff716e87b56c84521aeed7464a7e821fd78d4e12 Mon Sep 17 00:00:00 2001 From: Vlad Grigorescu Date: Sat, 16 Nov 2024 10:32:53 -0600 Subject: [PATCH 07/25] Remove requirements that are pulled in by something else. --- requirements/base.txt | 22 +++++++++------------- requirements/local.txt | 7 ------- 2 files changed, 9 insertions(+), 20 deletions(-) diff --git a/requirements/base.txt b/requirements/base.txt index 62756c66..7577c200 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -1,26 +1,23 @@ -pytz==2022.1 # https://github.com/stub42/pytz -django-netfields # https://pypi.org/project/django-netfields/ -python-slugify==6.1.2 # https://github.com/un33k/python-slugify argon2-cffi==21.3.0 # https://github.com/hynek/argon2_cffi -whitenoise==6.2.0 # https://github.com/evansd/whitenoise -celery==5.2.7 # pyup: < 6.0 # https://github.com/celery/celery 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 # Django # ------------------------------------------------------------------------------ -channels==3.0.4 channels_redis==3.4.0 -django-redis==5.3.0 +crispy-bootstrap5==0.6 # https://github.com/django-crispy-forms/crispy-bootstrap5 django~=4.2 # https://www.djangoproject.com/ -django-grip==3.4.0 -django-eventstream==4.5.1 # https://github.com/fanout/django-eventstream + +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-allauth==0.51.0 # https://github.com/pennersr/django-allauth -django-crispy-forms==1.14.0 # https://github.com/django-crispy-forms/django-crispy-forms -crispy-bootstrap5==0.6 # https://github.com/django-crispy-forms/crispy-bootstrap5 +django-redis==5.3.0 +django-simple-history~=3.1.1 # Django REST Framework djangorestframework~=3.15 # https://github.com/encode/django-rest-framework @@ -33,4 +30,3 @@ drf-spectacular # https://github.com/tfranzel/drf-spectacular mozilla_django_oidc==2.0.0 # https://github.com/mozilla/mozilla-django-oidc websockets~=10.3 -django-simple-history~=3.1.1 diff --git a/requirements/local.txt b/requirements/local.txt index edc15a30..a0fbe77b 100644 --- a/requirements/local.txt +++ b/requirements/local.txt @@ -7,12 +7,9 @@ watchgod==0.8.2 # https://github.com/samuelcolvin/watchgod # Testing # ------------------------------------------------------------------------------ -mypy==0.950 # https://github.com/python/mypy django-stubs==1.11.0 # https://github.com/typeddjango/django-stubs -pytest==7.1.2 # https://github.com/pytest-dev/pytest pytest-sugar==0.9.4 # https://github.com/Frozenball/pytest-sugar behave-django==1.4.0 # https://github.com/behave/behave-django -django-debug-toolbar~=3.2 # https://github.com/jazzband/django-debug-toolbar # Documentation # ------------------------------------------------------------------------------ @@ -21,9 +18,7 @@ sphinx-autobuild==2021.3.14 # https://github.com/GaretJax/sphinx-autobuild # Code quality # ------------------------------------------------------------------------------ -flake8==4.0.1 # https://github.com/PyCQA/flake8 flake8-isort==4.1.1 # https://github.com/gforcada/flake8-isort -coverage==6.4.1 # https://github.com/nedbat/coveragepy black==22.3.0 # 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 @@ -37,8 +32,6 @@ 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==2.0.3 # https://github.com/nedbat/django_coverage_plugin pytest-django==4.5.2 # https://github.com/pytest-dev/pytest-django -pytest~=7.1.2 -behave~=1.2.6 # https://behave.readthedocs.io/en/stable/ # Debugging # ------------------------------------------------------------------------------ From ebd3c7a41b85f2ee6c9ddcd5e97760e8b6706f01 Mon Sep 17 00:00:00 2001 From: Vlad Grigorescu Date: Sun, 17 Nov 2024 09:07:36 -0600 Subject: [PATCH 08/25] First step to add coveralls --- .github/workflows/pytest.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/pytest.yml b/.github/workflows/pytest.yml index 0073cd2e..1ea68e78 100644 --- a/.github/workflows/pytest.yml +++ b/.github/workflows/pytest.yml @@ -59,7 +59,10 @@ jobs: POSTGRES_DB: test_scram run: make coverage.xml - - name: Upload Coverage Report + - name: Upload Coverage to Coveralls + uses: coverallsapp/github-action@v2 + + - name: Upload Coverage to GitHub uses: actions/upload-artifact@v4 with: name: coverage-report From 894cef5754dd96864d008104daf005c3156c9480 Mon Sep 17 00:00:00 2001 From: Vlad Grigorescu Date: Sun, 17 Nov 2024 09:09:12 -0600 Subject: [PATCH 09/25] YAML is so fun; fixing indent --- .github/workflows/pytest.yml | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/.github/workflows/pytest.yml b/.github/workflows/pytest.yml index 1ea68e78..f84957df 100644 --- a/.github/workflows/pytest.yml +++ b/.github/workflows/pytest.yml @@ -1,3 +1,4 @@ +--- name: Run pytest on: @@ -59,10 +60,10 @@ jobs: POSTGRES_DB: test_scram run: make coverage.xml - - name: Upload Coverage to Coveralls - uses: coverallsapp/github-action@v2 + - name: Upload Coverage to Coveralls + uses: coverallsapp/github-action@v2 - - name: Upload Coverage to GitHub + - name: Upload Coverage to GitHub uses: actions/upload-artifact@v4 with: name: coverage-report From 21cbe260af878597250018f29472f3ccab5889aa Mon Sep 17 00:00:00 2001 From: Vlad Grigorescu Date: Sun, 17 Nov 2024 11:38:42 -0600 Subject: [PATCH 10/25] Ignore some things from coverage stats --- config/asgi.py | 12 ++++++------ setup.cfg | 10 ++++++++++ 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/config/asgi.py b/config/asgi.py index 1b3cd6e9..98a16ce2 100644 --- a/config/asgi.py +++ b/config/asgi.py @@ -19,10 +19,10 @@ from django.core.asgi import get_asgi_application # Here we setup a debugger if this is desired. This obviously should not be run in production. -debug_mode = os.environ.get("DEBUG") -if debug_mode: - logging.info(f"Django is set to use a debugger. Provided debug mode: {debug_mode}") - if debug_mode == "pycharm-pydevd": +debug = os.environ.get("DEBUG") +if debug: + logging.info(f"Django is set to use a debugger. Provided debug mode: {debug}") + if debug == "pycharm-pydevd": logging.info("Entering debug mode for pycharm, make sure the debug server is running in PyCharm!") import pydevd_pycharm @@ -30,7 +30,7 @@ pydevd_pycharm.settrace("host.docker.internal", port=56783, stdoutToServer=True, stderrToServer=True) logging.info("Debugger started.") - elif debug_mode == "debugpy": + elif debug == "debugpy": logging.info("Entering debug mode for debugpy (VSCode)") import debugpy @@ -39,7 +39,7 @@ logging.info("Debugger listening on port 56780.") else: - logging.warning(f"Invalid debug mode given: {debug_mode}. Debugger not started") + logging.warning(f"Invalid debug mode given: {debug}. Debugger not started") # This allows easy placement of apps within the interior # scram directory. diff --git a/setup.cfg b/setup.cfg index 2c082581..6243121a 100644 --- a/setup.cfg +++ b/setup.cfg @@ -27,3 +27,13 @@ ignore_errors = True [behave] paths = scram/route_manager/tests/acceptance stderr_capture = no + +[coverage:report] +exclude_also = + def __repr__ + if debug: + if self.debug: + if settings.DEBUG + raise AssertionError + raise NotImplementedError + if __name__ == .__main__.: From 41aca9a1cb6d957674d3e43a958c5573b67e3dcc Mon Sep 17 00:00:00 2001 From: Vlad Grigorescu Date: Sun, 17 Nov 2024 12:25:31 -0600 Subject: [PATCH 11/25] Coverage fixes: move settings to a single file, update version, fix configuration for django templates. --- .coveragerc | 3 --- config/settings/local.py | 4 ++++ pyproject.toml | 3 ++- requirements/local.txt | 2 +- setup.cfg | 1 - 5 files changed, 7 insertions(+), 6 deletions(-) delete mode 100644 .coveragerc diff --git a/.coveragerc b/.coveragerc deleted file mode 100644 index 457c5db9..00000000 --- a/.coveragerc +++ /dev/null @@ -1,3 +0,0 @@ -[run] -branch = True -data_file = coverage.coverage diff --git a/config/settings/local.py b/config/settings/local.py index 14ec647b..4bc4ba57 100644 --- a/config/settings/local.py +++ b/config/settings/local.py @@ -33,6 +33,10 @@ # http://whitenoise.evans.io/en/latest/django.html#using-whitenoise-in-development INSTALLED_APPS = ["whitenoise.runserver_nostatic"] + INSTALLED_APPS # noqa F405 +# django-coverage-plugin +# ------------------------------------------------------------------------------ +# https://github.com/nedbat/django_coverage_plugin?tab=readme-ov-file#django-template-coveragepy-plugin +TEMPLATES[0]["OPTIONS"]['debug'] = True # django-debug-toolbar # ------------------------------------------------------------------------------ diff --git a/pyproject.toml b/pyproject.toml index f5e74ec1..5981b6f3 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -12,7 +12,8 @@ python_files = [ include = ["scram/*", "config/*", "translator/*"] omit = ["**/migrations/*", "scram/contrib/*", "*/tests/*"] plugins = ["django_coverage_plugin"] - +branch = true +data_file = coverage.coverage # ==== black ==== diff --git a/requirements/local.txt b/requirements/local.txt index a0fbe77b..6b6f0988 100644 --- a/requirements/local.txt +++ b/requirements/local.txt @@ -30,7 +30,7 @@ factory-boy==3.2.1 # 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==2.0.3 # https://github.com/nedbat/django_coverage_plugin +django-coverage-plugin==3.5.0 # https://github.com/nedbat/django_coverage_plugin pytest-django==4.5.2 # https://github.com/pytest-dev/pytest-django # Debugging diff --git a/setup.cfg b/setup.cfg index 6243121a..09cd74c2 100644 --- a/setup.cfg +++ b/setup.cfg @@ -30,7 +30,6 @@ stderr_capture = no [coverage:report] exclude_also = - def __repr__ if debug: if self.debug: if settings.DEBUG From 310d7896a79219a8e947e8698a5b92313989b685 Mon Sep 17 00:00:00 2001 From: Vlad Grigorescu Date: Sun, 17 Nov 2024 12:46:25 -0600 Subject: [PATCH 12/25] Remove the config star imports; there's really not that many of them. --- config/settings/local.py | 12 ++++++------ config/settings/production.py | 20 ++++++++++---------- config/settings/test.py | 6 +++--- 3 files changed, 19 insertions(+), 19 deletions(-) diff --git a/config/settings/local.py b/config/settings/local.py index 4bc4ba57..a5ababf9 100644 --- a/config/settings/local.py +++ b/config/settings/local.py @@ -1,4 +1,4 @@ -from .base import * # noqa +from .base import INSTALLED_APPS, MIDDLEWARE, TEMPLATES from .base import env # GENERAL @@ -31,7 +31,7 @@ # WhiteNoise # ------------------------------------------------------------------------------ # http://whitenoise.evans.io/en/latest/django.html#using-whitenoise-in-development -INSTALLED_APPS = ["whitenoise.runserver_nostatic"] + INSTALLED_APPS # noqa F405 +INSTALLED_APPS = ["whitenoise.runserver_nostatic"] + INSTALLED_APPS # django-coverage-plugin # ------------------------------------------------------------------------------ @@ -41,9 +41,9 @@ # django-debug-toolbar # ------------------------------------------------------------------------------ # https://django-debug-toolbar.readthedocs.io/en/latest/installation.html#prerequisites -INSTALLED_APPS += ["debug_toolbar"] # noqa F405 +INSTALLED_APPS += ["debug_toolbar"] # https://django-debug-toolbar.readthedocs.io/en/latest/installation.html#middleware -MIDDLEWARE += ["debug_toolbar.middleware.DebugToolbarMiddleware"] # noqa F405 +MIDDLEWARE += ["debug_toolbar.middleware.DebugToolbarMiddleware"] # https://django-debug-toolbar.readthedocs.io/en/latest/configuration.html#debug-toolbar-config DEBUG_TOOLBAR_CONFIG = { "DISABLE_PANELS": ["debug_toolbar.panels.redirects.RedirectsPanel"], @@ -60,7 +60,7 @@ # django-extensions # ------------------------------------------------------------------------------ # https://django-extensions.readthedocs.io/en/latest/installation_instructions.html#configuration -INSTALLED_APPS += ["django_extensions"] # noqa F405 +INSTALLED_APPS += ["django_extensions"] # Your stuff... # ------------------------------------------------------------------------------ @@ -70,4 +70,4 @@ } # Behave Django testing framework -INSTALLED_APPS += ["behave_django"] # noqa F405 +INSTALLED_APPS += ["behave_django"] diff --git a/config/settings/production.py b/config/settings/production.py index 6d4484cc..248f4a23 100644 --- a/config/settings/production.py +++ b/config/settings/production.py @@ -1,7 +1,7 @@ import os -from .base import * # noqa -from .base import AUTHENTICATION_BACKENDS, MIDDLEWARE, env +from .base import AUTHENTICATION_BACKENDS, DATABASES, INSTALLED_APPS, MIDDLEWARE, TEMPLATES +from .base import env # GENERAL # ------------------------------------------------------------------------------ @@ -12,11 +12,11 @@ # DATABASES # ------------------------------------------------------------------------------ -DATABASES["default"] = env.db("DATABASE_URL") # noqa F405 -DATABASES["default"]["ATOMIC_REQUESTS"] = True # noqa F405 -DATABASES["default"]["CONN_MAX_AGE"] = env.int("CONN_MAX_AGE", default=60) # noqa F405 +DATABASES["default"] = env.db("DATABASE_URL") +DATABASES["default"]["ATOMIC_REQUESTS"] = True +DATABASES["default"]["CONN_MAX_AGE"] = env.int("CONN_MAX_AGE", default=60) if env("POSTGRES_SSL"): - DATABASES["default"]["OPTIONS"] = {"sslmode": "require"} # noqa F405 + DATABASES["default"]["OPTIONS"] = {"sslmode": "require"} # CACHES # ------------------------------------------------------------------------------ @@ -63,7 +63,7 @@ # TEMPLATES # ------------------------------------------------------------------------------ # https://docs.djangoproject.com/en/dev/ref/settings/#templates -TEMPLATES[-1]["OPTIONS"]["loaders"] = [ # type: ignore[index] # noqa F405 +TEMPLATES[-1]["OPTIONS"]["loaders"] = [ # type: ignore[index] ( "django.template.loaders.cached.Loader", [ @@ -90,7 +90,7 @@ # Anymail # ------------------------------------------------------------------------------ # https://anymail.readthedocs.io/en/stable/installation/#installing-anymail -INSTALLED_APPS += ["anymail"] # noqa F405 +INSTALLED_APPS += ["anymail"] # https://docs.djangoproject.com/en/dev/ref/settings/#email-backend # https://anymail.readthedocs.io/en/stable/installation/#anymail-settings-reference # https://anymail.readthedocs.io/en/stable/esps @@ -142,10 +142,10 @@ # Your stuff... # ------------------------------------------------------------------------------ # Extend middleware to add OIDC middleware -MIDDLEWARE += ["mozilla_django_oidc.middleware.SessionRefresh"] # noqa F405 +MIDDLEWARE += ["mozilla_django_oidc.middleware.SessionRefresh"] # Extend middleware to add OIDC auth backend -AUTHENTICATION_BACKENDS += ["scram.route_manager.authentication_backends.ESnetAuthBackend"] # noqa F405 +AUTHENTICATION_BACKENDS += ["scram.route_manager.authentication_backends.ESnetAuthBackend"] # https://docs.djangoproject.com/en/dev/ref/settings/#login-url LOGIN_URL = "oidc_authentication_init" diff --git a/config/settings/test.py b/config/settings/test.py index 1eb5ed4a..6c5b06f5 100644 --- a/config/settings/test.py +++ b/config/settings/test.py @@ -4,7 +4,7 @@ import os -from .base import * # noqa +from .base import MIDDLEWARE, TEMPLATES from .base import env # GENERAL @@ -24,7 +24,7 @@ # TEMPLATES # ------------------------------------------------------------------------------ -TEMPLATES[-1]["OPTIONS"]["loaders"] = [ # type: ignore[index] # noqa F405 +TEMPLATES[-1]["OPTIONS"]["loaders"] = [ # type: ignore[index] ( "django.template.loaders.cached.Loader", [ @@ -42,7 +42,7 @@ # Your stuff... # ------------------------------------------------------------------------------ # Extend middleware to add OIDC middleware -MIDDLEWARE += ["mozilla_django_oidc.middleware.SessionRefresh"] # noqa F405 +MIDDLEWARE += ["mozilla_django_oidc.middleware.SessionRefresh"] OIDC_OP_JWKS_ENDPOINT = os.environ.get( "OIDC_OP_JWKS_ENDPOINT", From dd12466a85b18761702aacfdaab981519e9e06d8 Mon Sep 17 00:00:00 2001 From: Vlad Grigorescu Date: Sun, 17 Nov 2024 12:54:42 -0600 Subject: [PATCH 13/25] Fix django-coverage-plugin version --- requirements/local.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements/local.txt b/requirements/local.txt index 6b6f0988..11b526f1 100644 --- a/requirements/local.txt +++ b/requirements/local.txt @@ -30,7 +30,7 @@ factory-boy==3.2.1 # 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.5.0 # https://github.com/nedbat/django_coverage_plugin +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 # Debugging From 72d40b147760e0ae4886b40ec9a71dc193a49d08 Mon Sep 17 00:00:00 2001 From: Vlad Grigorescu Date: Sun, 17 Nov 2024 13:16:51 -0600 Subject: [PATCH 14/25] Bring syntax inline with the rest --- config/settings/local.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/settings/local.py b/config/settings/local.py index a5ababf9..1d781593 100644 --- a/config/settings/local.py +++ b/config/settings/local.py @@ -31,7 +31,7 @@ # WhiteNoise # ------------------------------------------------------------------------------ # http://whitenoise.evans.io/en/latest/django.html#using-whitenoise-in-development -INSTALLED_APPS = ["whitenoise.runserver_nostatic"] + INSTALLED_APPS +INSTALLED_APPS += ["whitenoise.runserver_nostatic"] # django-coverage-plugin # ------------------------------------------------------------------------------ From 5a68c46718043cdb69fdb4a2703d4f12bad8455d Mon Sep 17 00:00:00 2001 From: Vlad Grigorescu Date: Sun, 17 Nov 2024 13:27:19 -0600 Subject: [PATCH 15/25] Reverting 3 commits. They introduced a test failure, and I couldn't figure out how to resolve it. Leaving this as future work, and fixing the original issue (that those commits were trying to solve) a different way. --- config/settings/local.py | 14 +++++++------- config/settings/production.py | 20 ++++++++++---------- config/settings/test.py | 6 +++--- 3 files changed, 20 insertions(+), 20 deletions(-) diff --git a/config/settings/local.py b/config/settings/local.py index 1d781593..aaf58204 100644 --- a/config/settings/local.py +++ b/config/settings/local.py @@ -1,4 +1,4 @@ -from .base import INSTALLED_APPS, MIDDLEWARE, TEMPLATES +from .base import * # noqa from .base import env # GENERAL @@ -31,19 +31,19 @@ # WhiteNoise # ------------------------------------------------------------------------------ # http://whitenoise.evans.io/en/latest/django.html#using-whitenoise-in-development -INSTALLED_APPS += ["whitenoise.runserver_nostatic"] +INSTALLED_APPS = ["whitenoise.runserver_nostatic"] + INSTALLED_APPS # noqa F405 # django-coverage-plugin # ------------------------------------------------------------------------------ # https://github.com/nedbat/django_coverage_plugin?tab=readme-ov-file#django-template-coveragepy-plugin -TEMPLATES[0]["OPTIONS"]['debug'] = True +TEMPLATES[0]["OPTIONS"]['debug'] = True # noqa F405 # django-debug-toolbar # ------------------------------------------------------------------------------ # https://django-debug-toolbar.readthedocs.io/en/latest/installation.html#prerequisites -INSTALLED_APPS += ["debug_toolbar"] +INSTALLED_APPS += ["debug_toolbar"] # noqa F405 # https://django-debug-toolbar.readthedocs.io/en/latest/installation.html#middleware -MIDDLEWARE += ["debug_toolbar.middleware.DebugToolbarMiddleware"] +MIDDLEWARE += ["debug_toolbar.middleware.DebugToolbarMiddleware"] # noqa F405 # https://django-debug-toolbar.readthedocs.io/en/latest/configuration.html#debug-toolbar-config DEBUG_TOOLBAR_CONFIG = { "DISABLE_PANELS": ["debug_toolbar.panels.redirects.RedirectsPanel"], @@ -60,7 +60,7 @@ # django-extensions # ------------------------------------------------------------------------------ # https://django-extensions.readthedocs.io/en/latest/installation_instructions.html#configuration -INSTALLED_APPS += ["django_extensions"] +INSTALLED_APPS += ["django_extensions"] # noqa F405 # Your stuff... # ------------------------------------------------------------------------------ @@ -70,4 +70,4 @@ } # Behave Django testing framework -INSTALLED_APPS += ["behave_django"] +INSTALLED_APPS += ["behave_django"] # noqa F405 diff --git a/config/settings/production.py b/config/settings/production.py index 248f4a23..6d4484cc 100644 --- a/config/settings/production.py +++ b/config/settings/production.py @@ -1,7 +1,7 @@ import os -from .base import AUTHENTICATION_BACKENDS, DATABASES, INSTALLED_APPS, MIDDLEWARE, TEMPLATES -from .base import env +from .base import * # noqa +from .base import AUTHENTICATION_BACKENDS, MIDDLEWARE, env # GENERAL # ------------------------------------------------------------------------------ @@ -12,11 +12,11 @@ # DATABASES # ------------------------------------------------------------------------------ -DATABASES["default"] = env.db("DATABASE_URL") -DATABASES["default"]["ATOMIC_REQUESTS"] = True -DATABASES["default"]["CONN_MAX_AGE"] = env.int("CONN_MAX_AGE", default=60) +DATABASES["default"] = env.db("DATABASE_URL") # noqa F405 +DATABASES["default"]["ATOMIC_REQUESTS"] = True # noqa F405 +DATABASES["default"]["CONN_MAX_AGE"] = env.int("CONN_MAX_AGE", default=60) # noqa F405 if env("POSTGRES_SSL"): - DATABASES["default"]["OPTIONS"] = {"sslmode": "require"} + DATABASES["default"]["OPTIONS"] = {"sslmode": "require"} # noqa F405 # CACHES # ------------------------------------------------------------------------------ @@ -63,7 +63,7 @@ # TEMPLATES # ------------------------------------------------------------------------------ # https://docs.djangoproject.com/en/dev/ref/settings/#templates -TEMPLATES[-1]["OPTIONS"]["loaders"] = [ # type: ignore[index] +TEMPLATES[-1]["OPTIONS"]["loaders"] = [ # type: ignore[index] # noqa F405 ( "django.template.loaders.cached.Loader", [ @@ -90,7 +90,7 @@ # Anymail # ------------------------------------------------------------------------------ # https://anymail.readthedocs.io/en/stable/installation/#installing-anymail -INSTALLED_APPS += ["anymail"] +INSTALLED_APPS += ["anymail"] # noqa F405 # https://docs.djangoproject.com/en/dev/ref/settings/#email-backend # https://anymail.readthedocs.io/en/stable/installation/#anymail-settings-reference # https://anymail.readthedocs.io/en/stable/esps @@ -142,10 +142,10 @@ # Your stuff... # ------------------------------------------------------------------------------ # Extend middleware to add OIDC middleware -MIDDLEWARE += ["mozilla_django_oidc.middleware.SessionRefresh"] +MIDDLEWARE += ["mozilla_django_oidc.middleware.SessionRefresh"] # noqa F405 # Extend middleware to add OIDC auth backend -AUTHENTICATION_BACKENDS += ["scram.route_manager.authentication_backends.ESnetAuthBackend"] +AUTHENTICATION_BACKENDS += ["scram.route_manager.authentication_backends.ESnetAuthBackend"] # noqa F405 # https://docs.djangoproject.com/en/dev/ref/settings/#login-url LOGIN_URL = "oidc_authentication_init" diff --git a/config/settings/test.py b/config/settings/test.py index 6c5b06f5..1eb5ed4a 100644 --- a/config/settings/test.py +++ b/config/settings/test.py @@ -4,7 +4,7 @@ import os -from .base import MIDDLEWARE, TEMPLATES +from .base import * # noqa from .base import env # GENERAL @@ -24,7 +24,7 @@ # TEMPLATES # ------------------------------------------------------------------------------ -TEMPLATES[-1]["OPTIONS"]["loaders"] = [ # type: ignore[index] +TEMPLATES[-1]["OPTIONS"]["loaders"] = [ # type: ignore[index] # noqa F405 ( "django.template.loaders.cached.Loader", [ @@ -42,7 +42,7 @@ # Your stuff... # ------------------------------------------------------------------------------ # Extend middleware to add OIDC middleware -MIDDLEWARE += ["mozilla_django_oidc.middleware.SessionRefresh"] +MIDDLEWARE += ["mozilla_django_oidc.middleware.SessionRefresh"] # noqa F405 OIDC_OP_JWKS_ENDPOINT = os.environ.get( "OIDC_OP_JWKS_ENDPOINT", From f8b9bff77591d1ade63646340d290688e5d1df11 Mon Sep 17 00:00:00 2001 From: Vlad Grigorescu Date: Sun, 17 Nov 2024 13:41:16 -0600 Subject: [PATCH 16/25] Add Coveralls badge --- README.rst | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/README.rst b/README.rst index 9be49463..0b5ed230 100644 --- a/README.rst +++ b/README.rst @@ -3,6 +3,9 @@ SCRAM Security Catch and Release Automation Manager +.. image:: https://coveralls.io/repos/github/esnet-security/SCRAM/badge.svg + :target: https://coveralls.io/github/esnet-security/SCRAM + :alt: Coveralls Code Coverage Stats .. image:: https://img.shields.io/badge/built%20with-Cookiecutter%20Django-ff69b4.svg?logo=cookiecutter :target: https://github.com/pydanny/cookiecutter-django/ :alt: Built with Cookiecutter Django @@ -10,7 +13,6 @@ Security Catch and Release Automation Manager :target: https://github.com/ambv/black :alt: Black code style - :License: BSD ==== From 95f34d1ffdab8421ac5c617a3b687e6fe49f174b Mon Sep 17 00:00:00 2001 From: Vlad Grigorescu Date: Sun, 17 Nov 2024 13:43:36 -0600 Subject: [PATCH 17/25] Consolidate coverage config --- pyproject.toml | 8 ++++++++ setup.cfg | 9 --------- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 5981b6f3..768016a0 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -15,6 +15,14 @@ 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__.: # ==== black ==== [tool.black] diff --git a/setup.cfg b/setup.cfg index 09cd74c2..2c082581 100644 --- a/setup.cfg +++ b/setup.cfg @@ -27,12 +27,3 @@ ignore_errors = True [behave] paths = scram/route_manager/tests/acceptance stderr_capture = no - -[coverage:report] -exclude_also = - if debug: - if self.debug: - if settings.DEBUG - raise AssertionError - raise NotImplementedError - if __name__ == .__main__.: From 5fecad96b378a46d0b7ff9a2b87598c1b2ef9214 Mon Sep 17 00:00:00 2001 From: Vlad Grigorescu Date: Sun, 17 Nov 2024 13:55:13 -0600 Subject: [PATCH 18/25] Fix toml syntax --- pyproject.toml | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 768016a0..910571d6 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -13,16 +13,17 @@ include = ["scram/*", "config/*", "translator/*"] omit = ["**/migrations/*", "scram/contrib/*", "*/tests/*"] plugins = ["django_coverage_plugin"] branch = true -data_file = coverage.coverage +data_file = "coverage.coverage" [tool.coverage.report] -exclude_also = - if debug: - if self.debug: - if settings.DEBUG - raise AssertionError - raise NotImplementedError - if __name__ == .__main__.: +exclude_also = [ + "if debug:", + "if self.debug:", + "if settings.DEBUG", + "raise AssertionError", + "raise NotImplementedError", + "if __name__ == .__main__.:", + ] # ==== black ==== [tool.black] From 58ebc6ffccfccc98450ae7b6a077b0c7479779a6 Mon Sep 17 00:00:00 2001 From: Vlad Grigorescu Date: Sun, 17 Nov 2024 15:32:01 -0600 Subject: [PATCH 19/25] Add docstrings to scram/ --- scram/__init__.py | 4 ++- scram/conftest.py | 4 +++ scram/contrib/__init__.py | 2 +- scram/contrib/sites/__init__.py | 2 +- scram/route_manager/__init__.py | 1 + scram/route_manager/admin.py | 4 +++ scram/route_manager/api/__init__.py | 1 + scram/route_manager/api/exceptions.py | 8 +++++ scram/route_manager/api/serializers.py | 24 ++++++++++++++ scram/route_manager/api/views.py | 22 +++++++++++-- scram/route_manager/apps.py | 4 +++ .../route_manager/authentication_backends.py | 9 ++++-- scram/route_manager/context_processors.py | 3 ++ scram/route_manager/models.py | 28 ++++++++++++---- scram/route_manager/tests/__init__.py | 1 + .../tests/acceptance/environment.py | 3 ++ .../tests/acceptance/steps/common.py | 21 ++++++++++++ .../tests/acceptance/steps/ip.py | 7 +++- .../tests/acceptance/steps/translator.py | 5 +++ scram/route_manager/tests/functional_tests.py | 3 ++ scram/route_manager/tests/test_api.py | 15 +++++++++ .../route_manager/tests/test_authorization.py | 32 +++++++++++-------- scram/route_manager/tests/test_history.py | 10 ++++++ scram/route_manager/tests/test_views.py | 5 +++ scram/route_manager/tests/test_websockets.py | 14 ++++++-- scram/route_manager/urls.py | 2 ++ scram/route_manager/views.py | 12 +++++++ scram/users/__init__.py | 1 + scram/users/admin.py | 3 ++ scram/users/api/serializers.py | 6 ++++ scram/users/api/views.py | 6 ++++ scram/users/apps.py | 5 +++ scram/users/forms.py | 10 ++++++ scram/users/models.py | 2 ++ scram/users/tests/__init__.py | 1 + scram/users/tests/factories.py | 6 ++++ scram/users/tests/test_admin.py | 8 +++++ scram/users/tests/test_drf_urls.py | 5 +++ scram/users/tests/test_drf_views.py | 6 ++++ scram/users/tests/test_forms.py | 13 +++----- scram/users/tests/test_models.py | 3 ++ scram/users/tests/test_urls.py | 5 +++ scram/users/tests/test_views.py | 14 ++++++++ scram/users/urls.py | 2 ++ scram/users/views.py | 8 +++++ scram/utils/__init__.py | 1 + scram/utils/context_processors.py | 4 ++- 47 files changed, 316 insertions(+), 39 deletions(-) diff --git a/scram/__init__.py b/scram/__init__.py index eed836da..d5feb05a 100644 --- a/scram/__init__.py +++ b/scram/__init__.py @@ -1,2 +1,4 @@ -__version__ = "0.1.0" +"""The Django project for Security Catch and Release Automation Manager (SCRAM).""" + +__version__ = "1.1.1" __version_info__ = tuple([int(num) if num.isdigit() else num for num in __version__.replace("-", ".", 1).split(".")]) diff --git a/scram/conftest.py b/scram/conftest.py index 52d755b4..29a5bc61 100644 --- a/scram/conftest.py +++ b/scram/conftest.py @@ -1,3 +1,5 @@ +"""Some simple tests for the User app.""" + import pytest from scram.users.models import User @@ -6,9 +8,11 @@ @pytest.fixture(autouse=True) def media_storage(settings, tmpdir): + """Configure the test to use the temp directory.""" settings.MEDIA_ROOT = tmpdir.strpath @pytest.fixture def user() -> User: + """Return the UserFactory.""" return UserFactory() diff --git a/scram/contrib/__init__.py b/scram/contrib/__init__.py index 1c7ecc89..88d92bd3 100644 --- a/scram/contrib/__init__.py +++ b/scram/contrib/__init__.py @@ -1,5 +1,5 @@ """ -To understand why this file is here, please read: +To understand why this file is here, please read the CookieCutter documentation. http://cookiecutter-django.readthedocs.io/en/latest/faq.html#why-is-there-a-django-contrib-sites-directory-in-cookiecutter-django """ diff --git a/scram/contrib/sites/__init__.py b/scram/contrib/sites/__init__.py index 1c7ecc89..88d92bd3 100644 --- a/scram/contrib/sites/__init__.py +++ b/scram/contrib/sites/__init__.py @@ -1,5 +1,5 @@ """ -To understand why this file is here, please read: +To understand why this file is here, please read the CookieCutter documentation. http://cookiecutter-django.readthedocs.io/en/latest/faq.html#why-is-there-a-django-contrib-sites-directory-in-cookiecutter-django """ diff --git a/scram/route_manager/__init__.py b/scram/route_manager/__init__.py index e69de29b..db37df27 100644 --- a/scram/route_manager/__init__.py +++ b/scram/route_manager/__init__.py @@ -0,0 +1 @@ +"""Route Manager is the core app, and it handles adding and removing routes that will be blocked.""" diff --git a/scram/route_manager/admin.py b/scram/route_manager/admin.py index 96a6e324..172dd6c7 100644 --- a/scram/route_manager/admin.py +++ b/scram/route_manager/admin.py @@ -1,3 +1,5 @@ +"""Register models in the Admin site.""" + from django.contrib import admin from simple_history.admin import SimpleHistoryAdmin @@ -6,6 +8,8 @@ @admin.register(ActionType) class ActionTypeAdmin(SimpleHistoryAdmin): + """Configure the ActionType and how it shows up in the Admin site.""" + list_filter = ("available",) list_display = ("name", "available") diff --git a/scram/route_manager/api/__init__.py b/scram/route_manager/api/__init__.py index e69de29b..26fc9ceb 100644 --- a/scram/route_manager/api/__init__.py +++ b/scram/route_manager/api/__init__.py @@ -0,0 +1 @@ +"""The API, which leverages Django Request Framework.""" diff --git a/scram/route_manager/api/exceptions.py b/scram/route_manager/api/exceptions.py index 5483055b..e75c87b9 100644 --- a/scram/route_manager/api/exceptions.py +++ b/scram/route_manager/api/exceptions.py @@ -1,8 +1,12 @@ +"""Custom exceptions for the API.""" + from django.conf import settings from rest_framework.exceptions import APIException class PrefixTooLarge(APIException): + """The CIDR prefix that was specified is larger than 32 bits for IPv4 of 128 bits for IPv6.""" + v4_min_prefix = getattr(settings, "V4_MINPREFIX", 0) v6_min_prefix = getattr(settings, "V6_MINPREFIX", 0) status_code = 400 @@ -11,12 +15,16 @@ class PrefixTooLarge(APIException): class IgnoredRoute(APIException): + """An operation attempted to add a route that overlaps with a route on the ignore list.""" + status_code = 400 default_detail = "This CIDR is on the ignore list. You are not allowed to add it here." default_code = "ignored_route" class ActiontypeNotAllowed(APIException): + """An operation attempted to perform an action on behalf of a client that is unauthorized to perform that type.""" + status_code = 403 default_detail = "This client is not allowed to use this actiontype" default_code = "actiontype_not_allowed" diff --git a/scram/route_manager/api/serializers.py b/scram/route_manager/api/serializers.py index dec9df15..6baf7d95 100644 --- a/scram/route_manager/api/serializers.py +++ b/scram/route_manager/api/serializers.py @@ -1,3 +1,5 @@ +"""Serializers provide mappings between the API and the underlying model.""" + import logging from netfields import rest_framework @@ -11,15 +13,23 @@ class ActionTypeSerializer(serializers.ModelSerializer): + """This serializer defines no new fields.""" + class Meta: + """Maps to the ActionType model, and specifies the fields exposed by the API.""" + model = ActionType fields = ["pk", "name", "available"] class RouteSerializer(serializers.ModelSerializer): + """Exposes route as a CIDR field.""" + route = rest_framework.CidrAddressField() class Meta: + """Maps to the Route model, and specifies the fields exposed by the API.""" + model = Route fields = [ "route", @@ -27,12 +37,18 @@ class Meta: class ClientSerializer(serializers.ModelSerializer): + """This serializer defines no new fields.""" + class Meta: + """Maps to the Client model, and specifies the fields exposed by the API.""" + model = Client fields = ["hostname", "uuid"] class EntrySerializer(serializers.HyperlinkedModelSerializer): + """Due to the use of ForeignKeys, this follows some relationships to make sense via the API.""" + url = serializers.HyperlinkedIdentityField( view_name="api:v1:entry-detail", lookup_url_kwarg="pk", lookup_field="route" ) @@ -46,13 +62,17 @@ class EntrySerializer(serializers.HyperlinkedModelSerializer): comment = serializers.CharField() class Meta: + """Maps to the Entry model, and specifies the fields exposed by the API.""" + model = Entry fields = ["route", "actiontype", "url", "comment", "who"] def get_comment(self, obj): + """Provide a nicer name for change reason.""" return obj.get_change_reason() def create(self, 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") @@ -68,6 +88,10 @@ def create(self, validated_data): class IgnoreEntrySerializer(serializers.ModelSerializer): + """This serializer defines no new fields.""" + class Meta: + """Maps to the IgnoreEntry model, and specifies the fields exposed by the API.""" + model = IgnoreEntry fields = ["route", "comment"] diff --git a/scram/route_manager/api/views.py b/scram/route_manager/api/views.py index 2972e6dd..ca3a5d09 100644 --- a/scram/route_manager/api/views.py +++ b/scram/route_manager/api/views.py @@ -1,3 +1,5 @@ +"""Views provide mappings between the underlying model and how they're listed in the API.""" + import ipaddress import logging @@ -20,6 +22,8 @@ class ActionTypeViewSet(viewsets.ReadOnlyModelViewSet): + """Lookup ActionTypes by name when authenticated, and bind to the serializer.""" + queryset = ActionType.objects.all() permission_classes = (IsAuthenticated,) serializer_class = ActionTypeSerializer @@ -27,6 +31,8 @@ class ActionTypeViewSet(viewsets.ReadOnlyModelViewSet): class IgnoreEntryViewSet(viewsets.ModelViewSet): + """Lookup IgnoreEntries by route when authenticated, and bind to the serializer.""" + queryset = IgnoreEntry.objects.all() permission_classes = (IsAuthenticated,) serializer_class = IgnoreEntrySerializer @@ -34,6 +40,8 @@ class IgnoreEntryViewSet(viewsets.ModelViewSet): class ClientViewSet(viewsets.ModelViewSet): + """Lookup Client by hostname 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,) @@ -43,20 +51,27 @@ class ClientViewSet(viewsets.ModelViewSet): class EntryViewSet(viewsets.ModelViewSet): + """Lookup Entry when authenticated, and bind to the serializer.""" + queryset = Entry.objects.filter(is_active=True) permission_classes = (IsAuthenticated,) serializer_class = EntrySerializer lookup_value_regex = ".*" http_method_names = ["get", "post", "head", "delete"] - # Ovveride the permissions classes for POST method since we want to accept Entry creates from any client - # Note: We make authorization decisions on whether to actually create the object in the perform_create method later def get_permissions(self): + """ + Override the permissions classes for POST method since we want to accept Entry creates from any client. + + Note: We make authorization decisions on whether to actually create the object in the perform_create method + later. + """ if self.request.method == "POST": return [AllowAny()] return super().get_permissions() 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"] if self.request.user.username: @@ -126,6 +141,7 @@ def perform_create(self, serializer): @staticmethod def find_entries(arg, active_filter=None): + """Query entries either by pk or overlapping route.""" if not arg: return Entry.objects.none() @@ -149,6 +165,7 @@ def find_entries(arg, active_filter=None): return Entry.objects.filter(query) def retrieve(self, request, pk=None, **kwargs): + """Limit retrieval to 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: @@ -157,6 +174,7 @@ def retrieve(self, request, pk=None, **kwargs): return Response(serializer.data) def destroy(self, request, pk=None, *args, **kwargs): + """Only delete active (e.g. announced) entries.""" for entry in self.find_entries(pk, active_filter=True): entry.delete() diff --git a/scram/route_manager/apps.py b/scram/route_manager/apps.py index 319a8d17..bf13bf9e 100644 --- a/scram/route_manager/apps.py +++ b/scram/route_manager/apps.py @@ -1,5 +1,9 @@ +"""Register ourselves with Django.""" + from django.apps import AppConfig class RouteManagerConfig(AppConfig): + """Define the name of the module that's the main app.""" + name = "scram.route_manager" diff --git a/scram/route_manager/authentication_backends.py b/scram/route_manager/authentication_backends.py index 1c549376..dc151718 100644 --- a/scram/route_manager/authentication_backends.py +++ b/scram/route_manager/authentication_backends.py @@ -1,12 +1,15 @@ +"""Define one or more custom auth backends.""" + from django.conf import settings from django.contrib.auth.models import Group from mozilla_django_oidc.auth import OIDCAuthenticationBackend class ESnetAuthBackend(OIDCAuthenticationBackend): - def update_groups(self, user, claims): - """Sets the users group(s) to whatever is in the claims.""" + """Extend the OIDC backend with a custom permission model.""" + def update_groups(self, user, claims): + """Set the user's group(s) to whatever is in the claims.""" claimed_groups = claims.get("groups", []) effective_groups = [] @@ -38,10 +41,12 @@ def update_groups(self, user, claims): user.save() def create_user(self, claims): + """Wrap the superclass's user creation.""" user = super(ESnetAuthBackend, self).create_user(claims) return self.update_user(user, claims) def update_user(self, user, claims): + """Determine the user name from the claims.""" user.name = claims.get("given_name", "") + " " + claims.get("family_name", "") user.username = claims.get("preferred_username", "") if claims.get("groups", False): diff --git a/scram/route_manager/context_processors.py b/scram/route_manager/context_processors.py index 3781c3d2..61abde75 100644 --- a/scram/route_manager/context_processors.py +++ b/scram/route_manager/context_processors.py @@ -1,8 +1,11 @@ +"""Define custom functions that take a request and add to the context before template rendering.""" + from django.conf import settings from django.urls import reverse def login_logout(request): + """Pass through the relevant URLs from the settings.""" login_url = reverse(settings.LOGIN_URL) logout_url = reverse(settings.LOGOUT_URL) return {"login": login_url, "logout": logout_url} diff --git a/scram/route_manager/models.py b/scram/route_manager/models.py index ef063ead..101c8bd7 100644 --- a/scram/route_manager/models.py +++ b/scram/route_manager/models.py @@ -1,3 +1,5 @@ +"""Define the models used in the route_manager app.""" + import logging import uuid as uuid_lib @@ -10,33 +12,36 @@ class Route(models.Model): - """Model describing a route""" + """Define a route as a CIDR route and a UUID.""" route = CidrAddressField(unique=True) uuid = models.UUIDField(db_index=True, default=uuid_lib.uuid4, editable=False) def get_absolute_url(self): + """Ensure we use UUID on the API side instead.""" return reverse("") def __str__(self): + """Don't display the UUID, only the route.""" return str(self.route) class ActionType(models.Model): - """Defines an action that can be done with a given route. e.g. Block, shunt, redirect, etc.""" + """Define a type of action that can be done with a given route. e.g. Block, shunt, redirect, etc.""" name = models.CharField(help_text="One-word description of the action", max_length=30) available = models.BooleanField(help_text="Is this a valid choice for new entries?", default=True) history = HistoricalRecords() def __str__(self): + """Display clearly whether or not the action is currently available.""" if not self.available: return f"{self.name} (Inactive)" return self.name class WebSocketMessage(models.Model): - """Defines a single message sent to downstream translators via WebSocket.""" + """Define a single message sent to downstream translators via WebSocket.""" msg_type = models.CharField("The type of the message", max_length=50) msg_data = models.JSONField("The JSON payload. See also msg_data_route_field.", default=dict) @@ -47,11 +52,12 @@ class WebSocketMessage(models.Model): ) def __str__(self): + """Display clearly what the fields are used for.""" return f"{self.msg_type}: {self.msg_data} with the route in key {self.msg_data_route_field}" class WebSocketSequenceElement(models.Model): - """In a sequence of messages, defines a single element.""" + """In a sequence of messages, define a single element.""" websocketmessage = models.ForeignKey("WebSocketMessage", on_delete=models.CASCADE) order_num = models.SmallIntegerField( @@ -70,6 +76,7 @@ class WebSocketSequenceElement(models.Model): action_type = models.ForeignKey("ActionType", on_delete=models.CASCADE) def __str__(self): + """Summarize the fields into something short and readable.""" return ( f"{self.websocketmessage} as order={self.order_num} for " + f"{self.verb} actions on actiontype={self.action_type}" @@ -96,6 +103,7 @@ class Entry(models.Model): ) def delete(self, *args, **kwargs): + """Set inactive instead of deleting, as we want to ensure a history of entries.""" if not self.is_active: # We've already expired this route, don't send another message return @@ -115,36 +123,43 @@ def delete(self, *args, **kwargs): ) class Meta: + """Ensure that multiple routes can be added as long as they have different action types.""" + unique_together = ["route", "actiontype"] verbose_name_plural = "Entries" def __str__(self): + """Summarize the most important fields to something easily readable.""" desc = f"{self.route} ({self.actiontype})" if not self.is_active: desc += " (inactive)" return desc def get_change_reason(self): + """Traverse come complex relationships to determine the most recent change reason.""" hist_mgr = getattr(self, self._meta.simple_history_manager_attribute) return hist_mgr.order_by("-history_date").first().history_change_reason class IgnoreEntry(models.Model): - """For cidrs you NEVER want to block ie don't shoot yourself in the foot list""" + """Define CIDRs you NEVER want to block (i.e. the "don't shoot yourself in the foot" list).""" route = CidrAddressField(unique=True) comment = models.CharField(max_length=100) history = HistoricalRecords() class Meta: + """Ensure the plural is grammatically correct.""" + verbose_name_plural = "Ignored Entries" def __str__(self): + """Only display the route.""" return str(self.route) class Client(models.Model): - """Any client that would like to hit the API to add entries (e.g. Zeek)""" + """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() @@ -153,6 +168,7 @@ class Client(models.Model): authorized_actiontypes = models.ManyToManyField(ActionType) def __str__(self): + """Only display the hostname.""" return str(self.hostname) diff --git a/scram/route_manager/tests/__init__.py b/scram/route_manager/tests/__init__.py index e69de29b..fb031a32 100644 --- a/scram/route_manager/tests/__init__.py +++ b/scram/route_manager/tests/__init__.py @@ -0,0 +1 @@ +"""Define tests executed by pytest.""" diff --git a/scram/route_manager/tests/acceptance/environment.py b/scram/route_manager/tests/acceptance/environment.py index 5bae6990..0d31198b 100644 --- a/scram/route_manager/tests/acceptance/environment.py +++ b/scram/route_manager/tests/acceptance/environment.py @@ -1,9 +1,12 @@ +"""Setup the environment for tests.""" + from rest_framework.test import APIClient from scram.users.tests.factories import UserFactory def django_ready(context): + """Create a user that tests can use for authenticated/unauthenticated testing.""" # Create a user UserFactory(username="user", password="password") diff --git a/scram/route_manager/tests/acceptance/steps/common.py b/scram/route_manager/tests/acceptance/steps/common.py index 1d502e96..3885e4bb 100644 --- a/scram/route_manager/tests/acceptance/steps/common.py +++ b/scram/route_manager/tests/acceptance/steps/common.py @@ -1,3 +1,5 @@ +"""Define steps used extensively by the Behave tests.""" + import datetime import time @@ -12,6 +14,7 @@ @given("a {name} actiontype is defined") def step_impl(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": {}} @@ -26,6 +29,7 @@ def step_impl(context, name): @given("a client with {name} authorization") def step_impl(context, name): + """Create a client and authorize it for that action type.""" at, created = ActionType.objects.get_or_create(name=name) authorized_client = Client.objects.create( hostname="authorized_client.es.net", @@ -37,6 +41,7 @@ def step_impl(context, name): @given("a client without {name} authorization") def step_impl(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", @@ -46,22 +51,26 @@ def step_impl(context, name): @when("we're logged in") def step_impl(context): + """Login.""" context.test.client.login(username="user", password="password") @when("the CIDR prefix limits are {v4_minprefix:d} and {v6_minprefix:d}") def step_impl(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 step_impl(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 step_impl(context, value): + """Block the provided route.""" context.response = context.test.client.post( reverse("api:v1:entry-list"), { @@ -78,6 +87,7 @@ def step_impl(context, value): @when("we add the entry {value:S} with comment {comment}") def step_impl(context, value, comment): + """Block the provided route and add a comment.""" context.response = context.test.client.post( reverse("api:v1:entry-list"), { @@ -93,6 +103,7 @@ def step_impl(context, value, comment): @when("we add the entry {value:S} with expiration {exp:S}") def step_impl(context, value, exp): + """Block the provided route and add an absolute expiration datetime.""" context.response = context.test.client.post( reverse("api:v1:entry-list"), { @@ -109,6 +120,7 @@ def step_impl(context, value, exp): @when("we add the entry {value:S} with expiration in {secs:d} seconds") def step_impl(context, value, secs): + """Block the provided route and add a relative expiration.""" td = datetime.timedelta(seconds=secs) expiration = datetime.datetime.now() + td @@ -128,16 +140,19 @@ def step_impl(context, value, secs): @step("we wait {secs:d} seconds") def step_impl(context, secs): + """Wait to allow messages to propagate.""" time.sleep(secs) @then("we remove expired entries") def step_impl(context): + """Call the function that removes expired entries.""" context.response = context.test.client.get(reverse("route_manager:process-expired")) @when("we add the ignore entry {value:S}") def step_impl(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"} ) @@ -145,16 +160,19 @@ def step_impl(context, value): @when("we remove the {model} {value}") def step_impl(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 step_impl(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 step_impl(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}, @@ -163,6 +181,7 @@ def step_impl(context, model, value_from, value_to): @then("the number of {model}s is {num:d}") def step_impl(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()), num) @@ -172,6 +191,7 @@ def step_impl(context, model, num): @then("{value} is one of our list of {model}s") def step_impl(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 @@ -187,6 +207,7 @@ def step_impl(context, value, model): @when("we register a client named {hostname} with the uuid of {uuid}") def step_impl(context, hostname, uuid): + """Create a client with a specific UUID.""" context.response = context.test.client.post( reverse("api:v1:client-list"), { diff --git a/scram/route_manager/tests/acceptance/steps/ip.py b/scram/route_manager/tests/acceptance/steps/ip.py index a346e954..8154f04b 100644 --- a/scram/route_manager/tests/acceptance/steps/ip.py +++ b/scram/route_manager/tests/acceptance/steps/ip.py @@ -1,12 +1,14 @@ +"""Define steps used for IP-related logic by the Behave tests.""" + import ipaddress from behave import then, when from django.urls import reverse -# This does a CIDR match @then("{route} is contained in our list of {model}s") def step_impl(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) @@ -22,6 +24,7 @@ def step_impl(context, route, model): @when("we query for {ip}") def step_impl(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 @@ -32,11 +35,13 @@ def step_impl(context, ip): @then("we get a ValueError") def step_impl(context): + """Ensure we received a ValueError exception.""" assert isinstance(context.queryException, ValueError) @then("the change entry for {value:S} is {comment}") def step_impl(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) diff --git a/scram/route_manager/tests/acceptance/steps/translator.py b/scram/route_manager/tests/acceptance/steps/translator.py index 85621f21..c6c2ff49 100644 --- a/scram/route_manager/tests/acceptance/steps/translator.py +++ b/scram/route_manager/tests/acceptance/steps/translator.py @@ -1,3 +1,5 @@ +"""Define steps used for translator integration testing by the Behave tests.""" + from behave import then from behave.api.async_step import async_run_until_complete from channels.testing import WebsocketCommunicator @@ -6,6 +8,7 @@ async def query_translator(route, actiontype, is_announced=True): + """Ensure the specified route is currently either blocked or unblocked.""" communicator = WebsocketCommunicator(ws_application, f"/ws/route_manager/webui_{actiontype}/") connected, subprotocol = await communicator.connect() assert connected @@ -21,10 +24,12 @@ async def query_translator(route, actiontype, is_announced=True): @then("{route} is announced by {actiontype} translators") @async_run_until_complete async def step_impl(context, route, actiontype): + """Ensure the specified route is currently blocked.""" await query_translator(route, actiontype) @then("{route} is not announced by {actiontype} translators") @async_run_until_complete async def step_impl(context, route, actiontype): + """Ensure the specified route is currently unblocked.""" await query_translator(route, actiontype, is_announced=False) diff --git a/scram/route_manager/tests/functional_tests.py b/scram/route_manager/tests/functional_tests.py index c4312ecf..c4515b13 100644 --- a/scram/route_manager/tests/functional_tests.py +++ b/scram/route_manager/tests/functional_tests.py @@ -1,7 +1,10 @@ +"""Use the Django web client to perform end-to-end, WebUI-based testing.""" + import unittest class HomePageTest(unittest.TestCase): + """Ensure the home page works.""" pass diff --git a/scram/route_manager/tests/test_api.py b/scram/route_manager/tests/test_api.py index 8152b877..1a81e907 100644 --- a/scram/route_manager/tests/test_api.py +++ b/scram/route_manager/tests/test_api.py @@ -1,3 +1,5 @@ +"""Use pytest to unit test the API.""" + from django.contrib.auth import get_user_model from django.urls import reverse from rest_framework import status @@ -7,7 +9,10 @@ class TestAddRemoveIP(APITestCase): + """Ensure that we can block IPs, and that duplicate blocks don't generate an error.""" + 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.client.login(username="admin", password="admintestpassword") @@ -19,6 +24,7 @@ def setUp(self): self.authorized_client.authorized_actiontypes.set([1]) def test_block_ipv4(self): + """Block a v4 IP.""" response = self.client.post( self.url, { @@ -31,6 +37,7 @@ def test_block_ipv4(self): self.assertEqual(response.status_code, status.HTTP_201_CREATED) def test_block_duplicate_ipv4(self): + """Block an existing v4 IP and ensure we don't get an error.""" self.client.post( self.url, { @@ -52,6 +59,7 @@ def test_block_duplicate_ipv4(self): self.assertEqual(response.status_code, status.HTTP_201_CREATED) def test_block_ipv6(self): + """Block a v6 IP.""" response = self.client.post( self.url, { @@ -64,6 +72,7 @@ def test_block_ipv6(self): self.assertEqual(response.status_code, status.HTTP_201_CREATED) def test_block_duplicate_ipv6(self): + """Block an existing v6 IP and ensure we don't get an error.""" self.client.post( self.url, { @@ -86,11 +95,15 @@ def test_block_duplicate_ipv6(self): class TestUnauthenticatedAccess(APITestCase): + """Ensure that an unathenticated client can't do anything.""" + def setUp(self): + """Define some helper variables.""" self.entry_url = reverse("api:v1:entry-list") self.ignore_url = reverse("api:v1:ignoreentry-list") def test_unauthenticated_users_have_no_create_access(self): + """Ensure an unauthenticated client can't add an Entry.""" response = self.client.post( self.entry_url, { @@ -104,9 +117,11 @@ def test_unauthenticated_users_have_no_create_access(self): self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) 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") self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) def test_unauthenticated_users_have_no_list_access(self): + """Ensure an unauthenticated client can't list Entries.""" response = self.client.get(self.entry_url, format="json") self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) diff --git a/scram/route_manager/tests/test_authorization.py b/scram/route_manager/tests/test_authorization.py index 6f4269d1..54f372b0 100644 --- a/scram/route_manager/tests/test_authorization.py +++ b/scram/route_manager/tests/test_authorization.py @@ -1,3 +1,5 @@ +"""Define tests for authorization and permissions.""" + from django.conf import settings from django.contrib.auth.models import Group from django.test import Client, TestCase @@ -10,7 +12,10 @@ class AuthzTest(TestCase): + """Define tests using the built-in authentication.""" + def setUp(self): + """Define several users for our tests.""" self.client = Client() self.unauthorized_user = User.objects.create(username="unauthorized") @@ -49,6 +54,7 @@ def setUp(self): ) def create_entry(self): + """Ensure the admin user can create an Entry.""" self.client.force_login(self.admin_user) self.client.post( reverse("route_manager:add"), @@ -63,8 +69,7 @@ def create_entry(self): return Entry.objects.latest("id").id def test_unauthorized_add_entry(self): - """Unauthorized users should not be able to add an entry""" - + """Unauthorized users should not be able to add an Entry.""" for user in self.write_blocked_users: if user: self.client.force_login(user) @@ -79,6 +84,7 @@ def test_unauthorized_add_entry(self): self.assertEqual(response.status_code, 302) def test_authorized_add_entry(self): + """Test authorized users with various permissions to ensure they can add an Entry.""" for user in self.write_allowed_users: self.client.force_login(user) response = self.client.post( @@ -92,6 +98,7 @@ def test_authorized_add_entry(self): self.assertEqual(response.status_code, 200) def test_unauthorized_detail_view(self): + """Ensure that unauthorized users can't view the blocked IPs.""" pk = self.create_entry() for user in self.detail_blocked_users: @@ -101,6 +108,7 @@ def test_unauthorized_detail_view(self): self.assertIn(response.status_code, [302, 403], msg=f"username={user}") def test_authorized_detail_view(self): + """Test authorized users with various permissions to ensure they can view block details.""" pk = self.create_entry() for user in self.detail_allowed_users: @@ -110,7 +118,6 @@ def test_authorized_detail_view(self): def test_unauthorized_after_group_removal(self): """The user has r/w access, then when we remove them from the r/w group, they no longer do.""" - test_user = User.objects.create(username="tmp_readwrite") test_user.groups.set([self.readwrite_group]) test_user.save() @@ -126,7 +133,10 @@ def test_unauthorized_after_group_removal(self): class OidcTest(TestCase): + """Define tests using OIDC authentication.""" + def setUp(self): + """Create a sample OIDC user.""" self.client = Client() self.claims = { "given_name": "Edward", @@ -136,8 +146,7 @@ def setUp(self): } def test_unauthorized(self): - """A user with no groups should have no access""" - + """A user with no groups should have no access.""" claims = dict(self.claims) user = ESnetAuthBackend().create_user(claims) @@ -146,8 +155,7 @@ def test_unauthorized(self): self.assertEqual(list(user.user_permissions.all()), []) def test_readonly(self): - """Test r/o groups""" - + """Test r/o groups.""" claims = dict(self.claims) claims["groups"] = [settings.SCRAM_READONLY_GROUPS[0]] user = ESnetAuthBackend().create_user(claims) @@ -158,8 +166,7 @@ def test_readonly(self): self.assertFalse(user.has_perm("route_manager.add_entry")) def test_readwrite(self): - """Test r/w groups""" - + """Test r/w groups.""" claims = dict(self.claims) claims["groups"] = [settings.SCRAM_READWRITE_GROUPS[0]] user = ESnetAuthBackend().create_user(claims) @@ -171,8 +178,7 @@ def test_readwrite(self): self.assertTrue(user.has_perm("route_manager.add_entry")) def test_admin(self): - """Test admin_groups""" - + """Test admin_groups.""" claims = dict(self.claims) claims["groups"] = [settings.SCRAM_ADMIN_GROUPS[0]] user = ESnetAuthBackend().create_user(claims) @@ -183,8 +189,7 @@ def test_admin(self): self.assertTrue(user.has_perm("route_manager.add_entry")) def test_authorized_removal(self): - """Have an authorized user, then downgrade them and make sure they're unauthorized""" - + """Have an authorized user, then downgrade them and make sure they're unauthorized.""" claims = dict(self.claims) claims["groups"] = [settings.SCRAM_ADMIN_GROUPS[0]] user = ESnetAuthBackend().create_user(claims) @@ -230,7 +235,6 @@ def test_authorized_removal(self): def test_disabled(self): """Pass all the groups, user should be disabled as it takes precedence.""" - claims = dict(self.claims) claims["groups"] = [settings.SCRAM_GROUPS] user = ESnetAuthBackend().create_user(claims) diff --git a/scram/route_manager/tests/test_history.py b/scram/route_manager/tests/test_history.py index f00f16bb..b234e621 100644 --- a/scram/route_manager/tests/test_history.py +++ b/scram/route_manager/tests/test_history.py @@ -1,3 +1,5 @@ +"""Define tests for the history feature.""" + from django.test import TestCase from simple_history.utils import get_change_reason_from_object, update_change_reason @@ -5,10 +7,14 @@ class TestActiontypeHistory(TestCase): + """Test the history on an action type.""" + def setUp(self): + """Set up the test environment.""" self.atype = ActionType.objects.create(name="Block") def test_comments(self): + """Ensure we can go back and set a reason.""" self.atype.name = "Nullroute" self.atype._change_reason = "Use more descriptive name" self.atype.save() @@ -16,9 +22,12 @@ def test_comments(self): class TestEntryHistory(TestCase): + """Test the history on an Entry.""" + routes = ["192.0.2.16/32", "198.51.100.16/28"] def setUp(self): + """Set up the test environment.""" self.atype = ActionType.objects.create(name="Block") for r in self.routes: route = Route.objects.create(route=r) @@ -28,6 +37,7 @@ def setUp(self): self.assertEqual(entry.get_change_reason(), create_reason) def test_comments(self): + """Ensure we can update the reason.""" for r in self.routes: route_old = Route.objects.get(route=r) e = Entry.objects.get(route=route_old) diff --git a/scram/route_manager/tests/test_views.py b/scram/route_manager/tests/test_views.py index 415cdae0..bcc3a4ad 100644 --- a/scram/route_manager/tests/test_views.py +++ b/scram/route_manager/tests/test_views.py @@ -1,3 +1,5 @@ +"""Define simple tests for the template-based Views.""" + from django.test import TestCase from django.urls import resolve @@ -5,6 +7,9 @@ class HomePageTest(TestCase): + """Test how the home page renders.""" + def test_root_url_resolves_to_home_page_view(self): + """Ensure we can find the home page.""" found = resolve("/") self.assertEqual(found.func, home_page) diff --git a/scram/route_manager/tests/test_websockets.py b/scram/route_manager/tests/test_websockets.py index 367359b8..306e772b 100644 --- a/scram/route_manager/tests/test_websockets.py +++ b/scram/route_manager/tests/test_websockets.py @@ -1,3 +1,5 @@ +"""Define unit tests for the websockets-based communication.""" + import json from asyncio import gather from contextlib import asynccontextmanager @@ -15,7 +17,7 @@ @asynccontextmanager async def get_communicators(actiontypes, should_match, *args, **kwds): - """Creates a set of communicators, and then handles tear-down. + """Create a set of communicators, and then handle tear-down. Given two lists of the same length, a set of actiontypes, and set of boolean values, creates that many communicators, one for each actiontype-bool pair. @@ -48,6 +50,7 @@ class TestTranslatorBaseCase(TestCase): """Base case that other test cases build on top of. Three translators in one group, test one v4 and one v6.""" 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") @@ -80,7 +83,7 @@ def setUp(self): self.local_setUp() def local_setUp(self): - # Allow child classes to override this if desired + """Allow child classes to override this if desired.""" return async def get_messages(self, communicator, messages, should_match): @@ -95,6 +98,7 @@ async def get_nothings(self, communicator): assert await communicator.receive_nothing(timeout=0.1, interval=0.01) is False 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: await self.api_create_entry(ip) @@ -119,6 +123,7 @@ async def ensure_no_more_msgs(self, communicators): # Django ensures that the create is synchronous, so we have some extra steps to do @sync_to_async def api_create_entry(self, route): + """Ensure we can create an Entry via the API.""" return self.client.post( self.url, { @@ -131,12 +136,14 @@ def api_create_entry(self, route): ) async def test_add_v4(self): + """Test adding a few v4 routes.""" await self.add_ip("192.0.2.224", 32) await self.add_ip("192.0.2.225", 32) await self.add_ip("192.0.2.226", 32) await self.add_ip("198.51.100.224", 32) async def test_add_v6(self): + """Test adding a few v6 routes.""" await self.add_ip("2001:DB8:FDF0::", 128) await self.add_ip("2001:DB8:FDF0::D", 128) await self.add_ip("2001:DB8:FDF0::DB", 128) @@ -147,6 +154,7 @@ class TranslatorDontCrossTheStreamsTestCase(TestTranslatorBaseCase): """Two translators in the same group, two in another group, single IP, ensure we get only the messages we expect.""" def local_setUp(self): + """Define the actions and what we expect.""" self.actiontypes = ["block", "block", "noop", "noop"] self.should_match = [True, True, False, False] @@ -155,6 +163,7 @@ class TranslatorSequenceTestCase(TestTranslatorBaseCase): """Test a sequence of WebSocket messages.""" def local_setUp(self): + """Define the messages we want to send.""" 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 @@ -175,6 +184,7 @@ class TranslatorParametersTestCase(TestTranslatorBaseCase): """Additional parameters in the JSONField.""" 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.save() diff --git a/scram/route_manager/urls.py b/scram/route_manager/urls.py index dde55639..2735cc64 100644 --- a/scram/route_manager/urls.py +++ b/scram/route_manager/urls.py @@ -1,3 +1,5 @@ +"""Register URLs known to Django, and the View that will handle each.""" + from django.urls import path from . import views diff --git a/scram/route_manager/views.py b/scram/route_manager/views.py index 54ff321a..99cc2c52 100644 --- a/scram/route_manager/views.py +++ b/scram/route_manager/views.py @@ -1,3 +1,5 @@ +"""Define the Views that will handle the HTTP requests.""" + import ipaddress import json @@ -23,6 +25,7 @@ def home_page(request, prefilter=Entry.objects.all()): + """Return the home page, autocreating a user if none exists.""" num_entries = settings.RECENT_LIMIT if request.user.has_perms(("route_manager.view_entry", "route_manager.add_entry")): readwrite = True @@ -60,6 +63,7 @@ def home_page(request, prefilter=Entry.objects.all()): 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 @@ -77,11 +81,14 @@ def search_entries(request): @require_POST @permission_required(["route_manager.view_entry", "route_manager.delete_entry"]) def delete_entry(request, pk): + """Wrap delete via the API and redirect to the home page.""" delete_entry_api(request, pk) return redirect("route_manager:home") class EntryDetailView(PermissionRequiredMixin, DetailView): + """Define a view for the API to use.""" + permission_required = ["route_manager.view_entry"] model = Entry template_name = "route_manager/entry_detail.html" @@ -92,6 +99,7 @@ class EntryDetailView(PermissionRequiredMixin, DetailView): @permission_required(["route_manager.view_entry", "route_manager.add_entry"]) def add_entry(request): + """Send a WebSocket message when adding a new entry.""" with transaction.atomic(): res = add_entry_api(request) @@ -121,6 +129,7 @@ def add_entry(request): def process_expired(request): + """For entries with an expiration, set them to inactive if expired. Return some simple stats.""" current_time = timezone.now() with transaction.atomic(): entries_start = Entry.objects.filter(is_active=True).count() @@ -140,10 +149,13 @@ def process_expired(request): class EntryListView(ListView): + """Define a view for the API to use.""" + model = Entry template_name = "route_manager/entry_list.html" def get_context_data(self, **kwargs): + """Group entries by action type.""" context = {"entries": {}} for at in ActionType.objects.all(): queryset = Entry.objects.filter(actiontype=at).order_by("-pk") diff --git a/scram/users/__init__.py b/scram/users/__init__.py index e69de29b..8728ced7 100644 --- a/scram/users/__init__.py +++ b/scram/users/__init__.py @@ -0,0 +1 @@ +"""The Users app defines users and a permission scheme for them.""" diff --git a/scram/users/admin.py b/scram/users/admin.py index c69eb541..ae35bc3f 100644 --- a/scram/users/admin.py +++ b/scram/users/admin.py @@ -1,3 +1,5 @@ +"""Register Admin models with the Django Admin site.""" + from django.contrib import admin from django.contrib.auth import admin as auth_admin from django.contrib.auth import get_user_model @@ -10,6 +12,7 @@ @admin.register(User) class UserAdmin(auth_admin.UserAdmin): + """Allow managing Users via the Django admin site.""" form = UserChangeForm add_form = UserCreationForm diff --git a/scram/users/api/serializers.py b/scram/users/api/serializers.py index c95bfe62..a49520d0 100644 --- a/scram/users/api/serializers.py +++ b/scram/users/api/serializers.py @@ -1,3 +1,5 @@ +"""Serializers provide mappings between the API and the underlying model.""" + from django.contrib.auth import get_user_model from rest_framework import serializers @@ -5,7 +7,11 @@ class UserSerializer(serializers.ModelSerializer): + """This serializer defines no new fields.""" + class Meta: + """Maps to the User model, and specifies the fields exposed by the API.""" + model = User fields = ["username", "name", "url"] diff --git a/scram/users/api/views.py b/scram/users/api/views.py index 288ea7ab..ce70a713 100644 --- a/scram/users/api/views.py +++ b/scram/users/api/views.py @@ -1,3 +1,5 @@ +"""Views provide mappings between the underlying model and how they're listed in the API.""" + from django.contrib.auth import get_user_model from rest_framework import status from rest_framework.decorators import action @@ -11,14 +13,18 @@ class UserViewSet(RetrieveModelMixin, ListModelMixin, UpdateModelMixin, GenericViewSet): + """Lookup Users by username.""" + serializer_class = UserSerializer queryset = User.objects.all() lookup_field = "username" def get_queryset(self, *args, **kwargs): + """Query on User ID.""" return self.queryset.filter(id=self.request.user.id) @action(detail=False, methods=["GET"]) def me(self, request): + """Return the current user.""" serializer = UserSerializer(request.user, context={"request": request}) return Response(status=status.HTTP_200_OK, data=serializer.data) diff --git a/scram/users/apps.py b/scram/users/apps.py index 2ce72578..449ebe39 100644 --- a/scram/users/apps.py +++ b/scram/users/apps.py @@ -1,3 +1,5 @@ +"""Register ourselves with Django.""" + import logging from django.apps import AppConfig @@ -7,10 +9,13 @@ class UsersConfig(AppConfig): + """Define the name of the module for the Users app.""" + name = "scram.users" verbose_name = _("Users") def ready(self): + """Check if signals are registered for User events.""" try: import scram.users.signals # noqa F401 except ImportError: diff --git a/scram/users/forms.py b/scram/users/forms.py index e2792fc2..b079f2a4 100644 --- a/scram/users/forms.py +++ b/scram/users/forms.py @@ -1,3 +1,5 @@ +"""Define forms for the Admin site.""" + from django.contrib.auth import forms as admin_forms from django.contrib.auth import get_user_model from django.utils.translation import gettext_lazy as _ @@ -6,12 +8,20 @@ class UserChangeForm(admin_forms.UserChangeForm): + """Define a form to edit a User.""" + class Meta(admin_forms.UserChangeForm.Meta): + """Map to the User model.""" + model = User class UserCreationForm(admin_forms.UserCreationForm): + """Define a form to create a User.""" + class Meta(admin_forms.UserCreationForm.Meta): + """Map to the User model and provide custom error messages.""" + model = User error_messages = {"username": {"unique": _("This username has already been taken.")}} diff --git a/scram/users/models.py b/scram/users/models.py index c2e1914f..81a905ca 100644 --- a/scram/users/models.py +++ b/scram/users/models.py @@ -1,3 +1,5 @@ +"""Define models for the User application.""" + from django.contrib.auth.models import AbstractUser from django.db.models import CharField from django.urls import reverse diff --git a/scram/users/tests/__init__.py b/scram/users/tests/__init__.py index e69de29b..f1101332 100644 --- a/scram/users/tests/__init__.py +++ b/scram/users/tests/__init__.py @@ -0,0 +1 @@ +"""Define tests for the User application.""" diff --git a/scram/users/tests/factories.py b/scram/users/tests/factories.py index edd306cb..1eca95c0 100644 --- a/scram/users/tests/factories.py +++ b/scram/users/tests/factories.py @@ -1,3 +1,5 @@ +"""Define Factory tests for the Users application.""" + from typing import Any, Sequence from django.contrib.auth import get_user_model @@ -6,6 +8,7 @@ class UserFactory(DjangoModelFactory): + """Test the UserFactory.""" username = Faker("user_name") email = Faker("email") @@ -13,6 +16,7 @@ class UserFactory(DjangoModelFactory): @post_generation def password(self, create: bool, extracted: Sequence[Any], **kwargs): + """Test password assignment.""" password = ( extracted if extracted @@ -28,5 +32,7 @@ def password(self, create: bool, extracted: Sequence[Any], **kwargs): self.set_password(password) class Meta: + """Map to User model.""" + model = get_user_model() django_get_or_create = ["username"] diff --git a/scram/users/tests/test_admin.py b/scram/users/tests/test_admin.py index 66f20cda..fd49e7ae 100644 --- a/scram/users/tests/test_admin.py +++ b/scram/users/tests/test_admin.py @@ -1,3 +1,5 @@ +"""Tests for the Admin site for the Users application.""" + import pytest from django.urls import reverse @@ -7,17 +9,22 @@ class TestUserAdmin: + """Test various User operations via the Admin site.""" + def test_changelist(self, admin_client): + """Ensure we can view the changelist.""" url = reverse("admin:users_user_changelist") response = admin_client.get(url) assert response.status_code == 200 def test_search(self, admin_client): + """Ensure we can view the search.""" url = reverse("admin:users_user_changelist") response = admin_client.get(url, data={"q": "test"}) assert response.status_code == 200 def test_add(self, admin_client): + """Ensure we can add a user.""" url = reverse("admin:users_user_add") response = admin_client.get(url) assert response.status_code == 200 @@ -34,6 +41,7 @@ def test_add(self, admin_client): assert User.objects.filter(username="test").exists() def test_view_user(self, admin_client): + """Ensure we can view a user.""" user = User.objects.get(username="admin") url = reverse("admin:users_user_change", kwargs={"object_id": user.pk}) response = admin_client.get(url) diff --git a/scram/users/tests/test_drf_urls.py b/scram/users/tests/test_drf_urls.py index 6e28c266..8e587486 100644 --- a/scram/users/tests/test_drf_urls.py +++ b/scram/users/tests/test_drf_urls.py @@ -1,3 +1,5 @@ +"""Test Django Request Framework use of the Users application.""" + import pytest from django.urls import resolve, reverse @@ -7,15 +9,18 @@ def test_user_detail(user: User): + """Ensure we can view details for a single User.""" assert reverse("api:v1:user-detail", kwargs={"username": user.username}) == f"/api/v1/users/{user.username}/" assert resolve(f"/api/v1/users/{user.username}/").view_name == "api:v1:user-detail" def test_user_list(): + """Ensure we can list all Users.""" assert reverse("api:v1:user-list") == "/api/v1/users/" assert resolve("/api/v1/users/").view_name == "api:v1:user-list" def test_user_me(): + """Ensure we can view info for the current user.""" assert reverse("api:v1:user-me") == "/api/v1/users/me/" assert resolve("/api/v1/users/me/").view_name == "api:v1:user-me" diff --git a/scram/users/tests/test_drf_views.py b/scram/users/tests/test_drf_views.py index 6e7b19e5..bbe56598 100644 --- a/scram/users/tests/test_drf_views.py +++ b/scram/users/tests/test_drf_views.py @@ -1,3 +1,5 @@ +"""Test Django Request Framework Views in the Users application.""" + import pytest from django.test import RequestFactory @@ -8,7 +10,10 @@ class TestUserViewSet: + """Test a couple simple View operations.""" + def test_get_queryset(self, user: User, rf: RequestFactory): + """Ensure we can view an arbitrary URL.""" view = UserViewSet() request = rf.get("/fake-url/") request.user = user @@ -18,6 +23,7 @@ def test_get_queryset(self, user: User, rf: RequestFactory): assert user in view.get_queryset() def test_me(self, user: User, rf: RequestFactory): + """Ensure we can view info on the current user.""" view = UserViewSet() request = rf.get("/fake-url/") request.user = user diff --git a/scram/users/tests/test_forms.py b/scram/users/tests/test_forms.py index 6d0b248b..4ff89f37 100644 --- a/scram/users/tests/test_forms.py +++ b/scram/users/tests/test_forms.py @@ -1,6 +1,5 @@ -""" -Module for all Form Tests. -""" +"""Module for all Form Tests.""" + import pytest from django.utils.translation import gettext_lazy as _ @@ -11,18 +10,16 @@ class TestUserCreationForm: - """ - Test class for all tests related to the UserCreationForm - """ + """Test class for all tests related to the UserCreationForm.""" def test_username_validation_error_msg(self, user: User): """ - Tests UserCreation Form's unique validator functions correctly by testing: + Tests UserCreation Form's unique validator functions correctly by testing 3 things. + 1) A new user with an existing username cannot be added. 2) Only 1 error is raised by the UserCreation Form 3) The desired error message is raised """ - # The user already exists, # hence cannot be created. form = UserCreationForm( diff --git a/scram/users/tests/test_models.py b/scram/users/tests/test_models.py index 9920c0ff..a224cd7d 100644 --- a/scram/users/tests/test_models.py +++ b/scram/users/tests/test_models.py @@ -1,3 +1,5 @@ +"""Define tests for the Models in the Users application.""" + import pytest from scram.users.models import User @@ -6,4 +8,5 @@ def test_user_get_absolute_url(user: User): + """Ensure w ecan convert a User object into a URL with info about that user.""" assert user.get_absolute_url() == f"/users/{user.username}/" diff --git a/scram/users/tests/test_urls.py b/scram/users/tests/test_urls.py index dd36a987..aa5a57ad 100644 --- a/scram/users/tests/test_urls.py +++ b/scram/users/tests/test_urls.py @@ -1,3 +1,5 @@ +"""Define tests for the URL resolution for the Users application.""" + import pytest from django.urls import resolve, reverse @@ -7,15 +9,18 @@ def test_detail(user: User): + """Ensure we can get the URL to view details about a single User.""" assert reverse("users:detail", kwargs={"username": user.username}) == f"/users/{user.username}/" assert resolve(f"/users/{user.username}/").view_name == "users:detail" def test_update(): + """Ensure we can get the URL to update a User.""" assert reverse("users:update") == "/users/~update/" assert resolve("/users/~update/").view_name == "users:update" def test_redirect(): + """Ensure we can get the URL to redirect to a User.""" assert reverse("users:redirect") == "/users/~redirect/" assert resolve("/users/~redirect/").view_name == "users:redirect" diff --git a/scram/users/tests/test_views.py b/scram/users/tests/test_views.py index 485370d2..63ad97bc 100644 --- a/scram/users/tests/test_views.py +++ b/scram/users/tests/test_views.py @@ -1,3 +1,5 @@ +"""Define tests for the Views of the Users application.""" + import pytest from django.conf import settings from django.contrib import messages @@ -17,6 +19,8 @@ class TestUserUpdateView: """ + Define tests related to the Update View. + TODO: extracting view initialization code as class-scoped fixture would be great if only pytest-django supported non-function-scoped @@ -25,6 +29,7 @@ class TestUserUpdateView: """ def test_get_success_url(self, user: User, rf: RequestFactory): + """Ensure we can view an arbitrary URL.""" view = UserUpdateView() request = rf.get("/fake-url/") request.user = user @@ -34,6 +39,7 @@ def test_get_success_url(self, user: User, rf: RequestFactory): assert view.get_success_url() == f"/users/{user.username}/" def test_get_object(self, user: User, rf: RequestFactory): + """Ensure we can retrieve the User object.""" view = UserUpdateView() request = rf.get("/fake-url/") request.user = user @@ -43,6 +49,7 @@ def test_get_object(self, user: User, rf: RequestFactory): assert view.get_object() == user def test_form_valid(self, user: User, rf: RequestFactory): + """Ensure the form validation works.""" view = UserUpdateView() request = rf.get("/fake-url/") @@ -63,7 +70,10 @@ def test_form_valid(self, user: User, rf: RequestFactory): class TestUserRedirectView: + """Define tests related to the Redirect View.""" + def test_get_redirect_url(self, user: User, rf: RequestFactory): + """Ensure the redirection URL works.""" view = UserRedirectView() request = rf.get("/fake-url") request.user = user @@ -74,7 +84,10 @@ def test_get_redirect_url(self, user: User, rf: RequestFactory): class TestUserDetailView: + """Define tests related to the User Detail View.""" + def test_authenticated(self, user: User, rf: RequestFactory): + """Ensure an authenticated user has access.""" request = rf.get("/fake-url/") request.user = UserFactory() @@ -83,6 +96,7 @@ def test_authenticated(self, user: User, rf: RequestFactory): assert response.status_code == 200 def test_not_authenticated(self, user: User, rf: RequestFactory): + """Ensure an unauthenticated user gets redirected to login.""" request = rf.get("/fake-url/") request.user = AnonymousUser() diff --git a/scram/users/urls.py b/scram/users/urls.py index 64f9d9cb..cf8647f3 100644 --- a/scram/users/urls.py +++ b/scram/users/urls.py @@ -1,3 +1,5 @@ +"""Register URLs known to Django, and the View that will handle each.""" + from django.urls import path from scram.users.views import user_detail_view, user_redirect_view, user_update_view diff --git a/scram/users/views.py b/scram/users/views.py index 2b077d42..e05f5cec 100644 --- a/scram/users/views.py +++ b/scram/users/views.py @@ -1,3 +1,5 @@ +"""Define the Views that will handle the HTTP requests.""" + from django.contrib.auth import get_user_model from django.contrib.auth.mixins import LoginRequiredMixin from django.contrib.messages.views import SuccessMessageMixin @@ -9,6 +11,7 @@ class UserDetailView(LoginRequiredMixin, DetailView): + """Map this view to the User model, and rely on the DetailView generic view.""" model = User slug_field = "username" @@ -19,15 +22,18 @@ class UserDetailView(LoginRequiredMixin, DetailView): class UserUpdateView(LoginRequiredMixin, SuccessMessageMixin, UpdateView): + """Map this view to the User model, and rely on the UpdateView generic view.""" model = User fields = ["name"] success_message = _("Information successfully updated") def get_success_url(self): + """Return the User detail view.""" return reverse("users:detail", kwargs={"username": self.request.user.username}) def get_object(self): + """Return the User object.""" return self.request.user @@ -35,10 +41,12 @@ def get_object(self): class UserRedirectView(LoginRequiredMixin, RedirectView): + """Map this view to the User model, and rely on the RedirectView generic view.""" permanent = False def get_redirect_url(self): + """Return the User detail view.""" return reverse("users:detail", kwargs={"username": self.request.user.username}) diff --git a/scram/utils/__init__.py b/scram/utils/__init__.py index e69de29b..e5a29f6c 100644 --- a/scram/utils/__init__.py +++ b/scram/utils/__init__.py @@ -0,0 +1 @@ +"""A module for defining utility functions and code.""" diff --git a/scram/utils/context_processors.py b/scram/utils/context_processors.py index 3c535141..2a86dfa4 100644 --- a/scram/utils/context_processors.py +++ b/scram/utils/context_processors.py @@ -1,8 +1,10 @@ +"""Define custom functions that take a request and add to the context before template rendering.""" + from django.conf import settings def settings_context(_request): - """Settings available by default to the templates context.""" + """Define settings available by default to the templates context.""" # Note: we intentionally do NOT expose the entire settings # to prevent accidental leaking of sensitive information return {"DEBUG": settings.DEBUG} From 954861020e2c067f64ad84c1609440049fd5e6aa Mon Sep 17 00:00:00 2001 From: Vlad Grigorescu Date: Sun, 17 Nov 2024 15:36:29 -0600 Subject: [PATCH 20/25] Require docstrings on everything on scram/ --- .github/workflows/flake8.yml | 20 ++++++++++++++++++++ requirements/local.txt | 1 + 2 files changed, 21 insertions(+) diff --git a/.github/workflows/flake8.yml b/.github/workflows/flake8.yml index 7f8d5c53..41626baf 100644 --- a/.github/workflows/flake8.yml +++ b/.github/workflows/flake8.yml @@ -31,3 +31,23 @@ jobs: - name: Run flake8 run: | flake8 + + # This will merge into the above once the other parts pass this check + ensure_docstrings: + runs-on: ubuntu-latest + steps: + - name: Checkout code + uses: actions/checkout@v4 + + - name: Cache Docker images. + uses: ScribeMD/docker-cache@0.3.7 + with: + key: docker-${{ runner.os }}-${{ hashFiles('docker-compose.yaml') }} + + - name: Install dependencies + run: | + pip install flake8 flake8-docstrings + + - name: Ensure scram has docstrings + run: | + flake8 scram diff --git a/requirements/local.txt b/requirements/local.txt index a0fbe77b..68accd63 100644 --- a/requirements/local.txt +++ b/requirements/local.txt @@ -18,6 +18,7 @@ sphinx-autobuild==2021.3.14 # https://github.com/GaretJax/sphinx-autobuild # Code quality # ------------------------------------------------------------------------------ +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 pylint-django==2.5.3 # https://github.com/PyCQA/pylint-django From ef42a0ff8684ca6198cd7b112a4836541c7d446c Mon Sep 17 00:00:00 2001 From: chriscummings Date: Sun, 17 Nov 2024 15:46:54 -0600 Subject: [PATCH 21/25] chore(deps): bump django prod, whoops --- compose/production/django/Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compose/production/django/Dockerfile b/compose/production/django/Dockerfile index e4a786a2..cbb5aeb5 100644 --- a/compose/production/django/Dockerfile +++ b/compose/production/django/Dockerfile @@ -1,5 +1,5 @@ -FROM python:3.8-slim-buster +FROM python:3.12-slim-bookwork ENV PYTHONUNBUFFERED 1 From bf023485be6f9bd03e5f1b1f4b67ec4704981323 Mon Sep 17 00:00:00 2001 From: Vlad Grigorescu Date: Sun, 17 Nov 2024 15:59:15 -0600 Subject: [PATCH 22/25] Add docstrings to translator/ --- translator/exceptions.py | 8 ++------ translator/gobgp.py | 10 ++++++++++ translator/shared.py | 4 +--- translator/tests/acceptance/environment.py | 3 +++ .../acceptance/features/00_clean_slate.feature | 9 +++++++++ translator/tests/acceptance/steps/actions.py | 8 ++++++++ translator/translator.py | 14 +++++++++++--- 7 files changed, 44 insertions(+), 12 deletions(-) create mode 100644 translator/tests/acceptance/features/00_clean_slate.feature diff --git a/translator/exceptions.py b/translator/exceptions.py index f1a54299..a999ec8c 100644 --- a/translator/exceptions.py +++ b/translator/exceptions.py @@ -1,9 +1,5 @@ -""" -This module holds all of the exceptions we want to raise in our translators. -""" +"""This module holds all of the exceptions we want to raise in our translators.""" class ASNError(TypeError): - """ - ASNError provides an error class to use when there is an issue with an Autonomous System Number. - """ + """ASNError provides an error class to use when there is an issue with an Autonomous System Number.""" diff --git a/translator/gobgp.py b/translator/gobgp.py index 23ae2572..6c10e159 100644 --- a/translator/gobgp.py +++ b/translator/gobgp.py @@ -1,3 +1,5 @@ +"""A translator interface for GoBGP (https://github.com/osrg/gobgp).""" + import logging import attribute_pb2 @@ -18,7 +20,10 @@ class GoBGP(object): + """Represents a GoBGP instance.""" + def __init__(self, url): + """Configure the channel used for communication.""" channel = grpc.insecure_channel(url) self.stub = gobgp_pb2_grpc.GobgpApiStub(channel) @@ -116,6 +121,7 @@ def _build_path(self, ip, event_data={}): ) def add_path(self, ip, event_data): + """Announce a single route.""" logging.info(f"Blocking {ip}") try: path = self._build_path(ip, event_data) @@ -128,11 +134,13 @@ def add_path(self, ip, event_data): logging.warning(f"ASN assertion failed with error: {e}") def del_all_paths(self): + """Remove all routes from being announced.""" logging.warning("Withdrawing ALL routes") self.stub.DeletePath(gobgp_pb2.DeletePathRequest(table_type=gobgp_pb2.GLOBAL), _TIMEOUT_SECONDS) def del_path(self, ip, event_data): + """Remove a single route from being announced.""" logging.info(f"Unblocking {ip}") try: path = self._build_path(ip, event_data) @@ -144,6 +152,7 @@ def del_path(self, ip, event_data): logging.warning(f"ASN assertion failed with error: {e}") def get_prefixes(self, ip): + """Retrieve the routes that match a prefix and are announced.""" prefixes = [gobgp_pb2.TableLookupPrefix(prefix=str(ip.ip))] family_afi = self._get_family_AFI(ip.ip.version) result = self.stub.ListPath( @@ -157,4 +166,5 @@ def get_prefixes(self, ip): return list(result) def is_blocked(self, ip): + """Return True if at least one route matching the prefix is being announced.""" return len(self.get_prefixes(ip)) > 0 diff --git a/translator/shared.py b/translator/shared.py index 1fe048bd..27e89f97 100644 --- a/translator/shared.py +++ b/translator/shared.py @@ -1,6 +1,4 @@ -""" -This module provides a location for code that we want to share between all translators. -""" +"""This module provides a location for code that we want to share between all translators.""" from exceptions import ASNError diff --git a/translator/tests/acceptance/environment.py b/translator/tests/acceptance/environment.py index 1f06b08d..b5c57b41 100644 --- a/translator/tests/acceptance/environment.py +++ b/translator/tests/acceptance/environment.py @@ -1,6 +1,9 @@ +"""Configure the test environment before executing acceptance tests.""" + from gobgp import GoBGP def before_all(context): + """Create a GoBGP object.""" context.gobgp = GoBGP("gobgp:50051") context.config.setup_logging() diff --git a/translator/tests/acceptance/features/00_clean_slate.feature b/translator/tests/acceptance/features/00_clean_slate.feature new file mode 100644 index 00000000..e4a9f47a --- /dev/null +++ b/translator/tests/acceptance/features/00_clean_slate.feature @@ -0,0 +1,9 @@ +Feature: Tests run correctly + When running tests, there are no leftover blocks + + Scenario: No leftover v4 blocks + Then 0.0.0.0/0 is unblocked + + Scenario: No leftover v6 blocks + Then ::/0 is unblocked + aaaaaaaaaaaaaaaa \ No newline at end of file diff --git a/translator/tests/acceptance/steps/actions.py b/translator/tests/acceptance/steps/actions.py index ce340de1..1e03e8e1 100644 --- a/translator/tests/acceptance/steps/actions.py +++ b/translator/tests/acceptance/steps/actions.py @@ -1,3 +1,5 @@ +"""Define the steps used by Behave.""" + import ipaddress import logging import time @@ -10,6 +12,7 @@ @when("we add {route} with {asn} and {community} to the block list") def add_block(context, route, asn, community): + """Block a single IP.""" ip = ipaddress.ip_interface(route) event_data = {"asn": int(asn), "community": int(community)} context.gobgp.add_path(ip, event_data) @@ -17,12 +20,14 @@ def add_block(context, route, asn, community): @then("we delete {route} with {asn} and {community} from the block list") def del_block(context, route, asn, community): + """Remove a single IP.""" ip = ipaddress.ip_interface(route) event_data = {"asn": int(asn), "community": int(community)} context.gobgp.del_path(ip, event_data) def get_block_status(context, ip): + """Check if the IP is currently blocked.""" # Allow our add/delete requests to settle time.sleep(1) @@ -38,15 +43,18 @@ def get_block_status(context, ip): @capture @when("{route} and {community} with invalid {asn} is sent") def asn_validation_fails(context, route, asn, community): + """Ensure the ASN was invalid.""" add_block(context, route, asn, community) assert context.log_capture.find_event("ASN assertion failed") @then("{ip} is blocked") def check_block(context, ip): + """Ensure that the IP is currently blocked.""" assert get_block_status(context, ip) @then("{ip} is unblocked") def check_unblock(context, ip): + """Ensure that the IP is currently unblocked.""" assert not get_block_status(context, ip) diff --git a/translator/translator.py b/translator/translator.py index 8521d7ab..b5a7dd02 100644 --- a/translator/translator.py +++ b/translator/translator.py @@ -1,5 +1,7 @@ #!/usr/bin/env python3 +"""Define the main event loop for the translator.""" + import asyncio import ipaddress import json @@ -14,9 +16,14 @@ if debug_mode: def install_deps(): - # Because of how we build translator currently, we don't have a great way to selectively install things at - # build, so we just do it here! Right now this also includes base.txt, which is unecessary, but in the - # future when we build a little better, it'll already be setup. + """ + Install necessary dependencies for debuggers. + + Because of how we build translator currently, we don't have a great way to selectively + install things at build, so we just do it here! Right now this also includes base.txt, + which is unecessary, but in the future when we build a little better, it'll already be + setup. + """ logging.info("Installing dependencies for debuggers") import subprocess @@ -58,6 +65,7 @@ def install_deps(): async def main(): + """Connect to the websocket and start listening for messages.""" g = GoBGP("gobgp:50051") async for websocket in websockets.connect(url): try: From 27293023eccd6e76d52f3e8f4ccda6629d5f4727 Mon Sep 17 00:00:00 2001 From: Vlad Grigorescu Date: Sun, 17 Nov 2024 16:00:01 -0600 Subject: [PATCH 23/25] Enforce docstrings on translator too. --- .github/workflows/flake8.yml | 24 ++---------------------- 1 file changed, 2 insertions(+), 22 deletions(-) diff --git a/.github/workflows/flake8.yml b/.github/workflows/flake8.yml index 41626baf..887ebc50 100644 --- a/.github/workflows/flake8.yml +++ b/.github/workflows/flake8.yml @@ -14,26 +14,6 @@ on: jobs: flake8: - runs-on: ubuntu-latest - steps: - - name: Checkout code - uses: actions/checkout@v4 - - - name: Cache Docker images. - uses: ScribeMD/docker-cache@0.3.7 - with: - key: docker-${{ runner.os }}-${{ hashFiles('docker-compose.yaml') }} - - - name: Install dependencies - run: | - pip install flake8 - - - name: Run flake8 - run: | - flake8 - - # This will merge into the above once the other parts pass this check - ensure_docstrings: runs-on: ubuntu-latest steps: - name: Checkout code @@ -48,6 +28,6 @@ jobs: run: | pip install flake8 flake8-docstrings - - name: Ensure scram has docstrings + - name: Run flake8 run: | - flake8 scram + flake8 scram translator From 2e52f9fc8d3dc489054c2b27ffd41704531cef64 Mon Sep 17 00:00:00 2001 From: Vlad Grigorescu Date: Sun, 17 Nov 2024 16:06:10 -0600 Subject: [PATCH 24/25] I didn't actually mean to commit this, but feels small enough to leave for now. --- .../features/{00_clean_slate.feature => clean_slate.feature} | 1 - 1 file changed, 1 deletion(-) rename translator/tests/acceptance/features/{00_clean_slate.feature => clean_slate.feature} (91%) diff --git a/translator/tests/acceptance/features/00_clean_slate.feature b/translator/tests/acceptance/features/clean_slate.feature similarity index 91% rename from translator/tests/acceptance/features/00_clean_slate.feature rename to translator/tests/acceptance/features/clean_slate.feature index e4a9f47a..9de4e337 100644 --- a/translator/tests/acceptance/features/00_clean_slate.feature +++ b/translator/tests/acceptance/features/clean_slate.feature @@ -6,4 +6,3 @@ Feature: Tests run correctly Scenario: No leftover v6 blocks Then ::/0 is unblocked - aaaaaaaaaaaaaaaa \ No newline at end of file From 100bedfdfefad23cad400df4d19f337cec62ef82 Mon Sep 17 00:00:00 2001 From: Sam Oehlert Date: Sun, 17 Nov 2024 22:30:12 -0600 Subject: [PATCH 25/25] docs(docstrings): update a few docstrings to better match the overall goal of a function or method --- scram/route_manager/api/exceptions.py | 2 +- scram/route_manager/api/views.py | 2 +- scram/route_manager/authentication_backends.py | 2 +- scram/route_manager/models.py | 2 +- scram/route_manager/tests/acceptance/steps/common.py | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/scram/route_manager/api/exceptions.py b/scram/route_manager/api/exceptions.py index e75c87b9..74d6f2e2 100644 --- a/scram/route_manager/api/exceptions.py +++ b/scram/route_manager/api/exceptions.py @@ -5,7 +5,7 @@ class PrefixTooLarge(APIException): - """The CIDR prefix that was specified is larger than 32 bits for IPv4 of 128 bits for IPv6.""" + """The CIDR prefix that was specified is larger than the prefix allowed in the settings.""" v4_min_prefix = getattr(settings, "V4_MINPREFIX", 0) v6_min_prefix = getattr(settings, "V6_MINPREFIX", 0) diff --git a/scram/route_manager/api/views.py b/scram/route_manager/api/views.py index ca3a5d09..ad26127e 100644 --- a/scram/route_manager/api/views.py +++ b/scram/route_manager/api/views.py @@ -165,7 +165,7 @@ def find_entries(arg, active_filter=None): return Entry.objects.filter(query) def retrieve(self, request, pk=None, **kwargs): - """Limit retrieval to a single route.""" + """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: diff --git a/scram/route_manager/authentication_backends.py b/scram/route_manager/authentication_backends.py index dc151718..30288520 100644 --- a/scram/route_manager/authentication_backends.py +++ b/scram/route_manager/authentication_backends.py @@ -46,7 +46,7 @@ def create_user(self, claims): return self.update_user(user, claims) def update_user(self, user, claims): - """Determine the user name from the claims.""" + """Determine the user name from the claims and update said user's groups.""" user.name = claims.get("given_name", "") + " " + claims.get("family_name", "") user.username = claims.get("preferred_username", "") if claims.get("groups", False): diff --git a/scram/route_manager/models.py b/scram/route_manager/models.py index 101c8bd7..3bbf67e3 100644 --- a/scram/route_manager/models.py +++ b/scram/route_manager/models.py @@ -34,7 +34,7 @@ class ActionType(models.Model): history = HistoricalRecords() def __str__(self): - """Display clearly whether or not the action is currently available.""" + """Display clearly whether the action is currently available.""" if not self.available: return f"{self.name} (Inactive)" return self.name diff --git a/scram/route_manager/tests/acceptance/steps/common.py b/scram/route_manager/tests/acceptance/steps/common.py index 3885e4bb..c7c03f8c 100644 --- a/scram/route_manager/tests/acceptance/steps/common.py +++ b/scram/route_manager/tests/acceptance/steps/common.py @@ -1,4 +1,4 @@ -"""Define steps used extensively by the Behave tests.""" +"""Define steps used exclusively by the Behave tests.""" import datetime import time