From 8ac3f59f0fc6de5cefc27a68197c3ab63e60cc0b Mon Sep 17 00:00:00 2001 From: Edward Romano Date: Thu, 12 Sep 2024 14:35:06 -0400 Subject: [PATCH] Backend Security Audit fixes (#352) --- .github/workflows/pytest.yml | 2 +- {{cookiecutter.project_slug}}/pytest.ini | 2 +- .../common/models.py | 2 +- .../core/admin.py | 14 +++- .../core/factories.py | 2 + .../core/models.py | 15 ++++ .../core/permissions.py | 10 +-- .../core/serializers.py | 1 + .../core/tests.py | 72 +++++++++++++++++-- .../core/views.py | 48 ++++++------- .../{{cookiecutter.project_slug}}/settings.py | 9 +-- .../test_settings.py | 2 +- .../{{cookiecutter.project_slug}}/urls.py | 3 + 13 files changed, 134 insertions(+), 48 deletions(-) diff --git a/.github/workflows/pytest.yml b/.github/workflows/pytest.yml index a6b5bcfaf..05277c856 100644 --- a/.github/workflows/pytest.yml +++ b/.github/workflows/pytest.yml @@ -49,4 +49,4 @@ jobs: mkdir -p client/dist/static pipenv run python server/manage.py collectstatic pipenv run pytest --mccabe --cov=my_project -vv server/my_project - pipenv run coverage report --fail-under=20 + pipenv run coverage report --fail-under=60 diff --git a/{{cookiecutter.project_slug}}/pytest.ini b/{{cookiecutter.project_slug}}/pytest.ini index 8eed4bed2..e947626ee 100644 --- a/{{cookiecutter.project_slug}}/pytest.ini +++ b/{{cookiecutter.project_slug}}/pytest.ini @@ -2,7 +2,7 @@ DJANGO_SETTINGS_MODULE = {{ cookiecutter.project_slug }}.test_settings python_files = tests.py test_*.py *_tests.py addopts = --strict-markers --no-migrations -mccabe-complexity=10 +mccabe-complexity=8 filterwarnings = ignore::DeprecationWarning diff --git a/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/common/models.py b/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/common/models.py index 9f4b0ac93..44edb9e2c 100644 --- a/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/common/models.py +++ b/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/common/models.py @@ -16,4 +16,4 @@ class Meta: abstract = True def __str__(self): - return "ah yes" + return "__str__ not defined for this model" diff --git a/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/admin.py b/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/admin.py index 71a55e02d..27490cb92 100644 --- a/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/admin.py +++ b/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/admin.py @@ -24,7 +24,7 @@ class CustomUserAdmin(UserAdmin): ) }, ), - ("Admin Options", {"classes": ("collapse",), "fields": ("is_staff", "groups")}), + ("Admin Options", {"classes": ("collapse",), "fields": ("is_active", "is_staff", "is_superuser", "groups")}), ) add_fieldsets = ( ( @@ -35,7 +35,15 @@ class CustomUserAdmin(UserAdmin): }, ), ) - list_display = ("email", "first_name", "last_name", "is_active", "is_staff", "is_superuser", "permissions") + list_display = ( + "email", + "permissions", + "is_active", + "is_staff", + "is_superuser", + "first_name", + "last_name", + ) list_display_links = ( "is_active", "email", @@ -57,7 +65,7 @@ class CustomUserAdmin(UserAdmin): ordering = [] def permissions(self, obj): - return ", ".join([g.name for g in obj.groups.all()]) + return [g.name for g in obj.groups.all()] class Media(AutocompleteAdminMedia): pass diff --git a/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/factories.py b/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/factories.py index 33f9bc7dc..b5072fc54 100755 --- a/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/factories.py +++ b/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/factories.py @@ -7,6 +7,8 @@ class UserFactory(factory.Factory): email = factory.faker.Faker("email") password = factory.PostGenerationMethodCall("set_password", "password") + first_name = factory.faker.Faker("first_name") + last_name = factory.faker.Faker("last_name") class Meta: model = User diff --git a/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/models.py b/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/models.py index c7cceee33..e803538e2 100644 --- a/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/models.py +++ b/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/models.py @@ -11,11 +11,26 @@ logger = logging.getLogger(__name__) +class UserQuerySet(models.QuerySet): + def for_user(self, user): + if not user or user.is_anonymous: + return self.none() + elif user.is_staff: + return self.all() + return self.filter(pk=user.pk) + + class UserManager(BaseUserManager): """Custom User model manager, eliminating the 'username' field.""" use_in_migrations = True + def get_queryset(self): + return UserQuerySet(self.model, using=self.db) + + def for_user(self, user): + return self.get_queryset().for_user(user) + def _create_user(self, email, password, **extra_fields): """ Create and save a User with the given email and password. diff --git a/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/permissions.py b/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/permissions.py index eb6a2282c..0e67f48b4 100644 --- a/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/permissions.py +++ b/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/permissions.py @@ -1,8 +1,8 @@ from rest_framework import permissions -class CreateOnlyPermissions(permissions.BasePermission): - def has_permission(self, request, view): - if view.action == "create": - return True - return False +class HasUserPermissions(permissions.BasePermission): + """Admins should be able to perform any action, regular users should be able to edit and delete self.""" + + def has_object_permission(self, request, view, obj): + return request.user.is_authenticated and (request.user.is_staff or obj == request.user) diff --git a/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/serializers.py b/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/serializers.py index 27dbc64e3..723f88479 100644 --- a/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/serializers.py +++ b/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/serializers.py @@ -16,6 +16,7 @@ class Meta: "last_name", "full_name", ) + read_only_fields = ["email"] class UserLoginSerializer(serializers.ModelSerializer): diff --git a/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/tests.py b/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/tests.py index e3cc149af..fb054748a 100644 --- a/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/tests.py +++ b/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/tests.py @@ -4,6 +4,7 @@ from django.contrib.auth import authenticate from django.test import override_settings from django.test.client import RequestFactory +from rest_framework import status from rest_framework.response import Response from .models import User @@ -34,6 +35,13 @@ def test_create_user(): assert not user.is_superuser +@pytest.mark.django_db +def test_create_user_api(api_client): + data = {"email": "example@example.com", "password": "password", "first_name": "Test", "last_name": "User"} + res = api_client.post("/api/users/", data, format="json") + assert res.status_code == status.HTTP_201_CREATED, res.data + + @pytest.mark.django_db def test_create_superuser(): superuser = User.objects.create_superuser(email="test@example.com", password="password", first_name="Leslie", last_name="Burke") @@ -50,7 +58,59 @@ def test_create_user_from_factory(sample_user): @pytest.mark.django_db def test_user_can_login(api_client, sample_user): res = api_client.post("/api/login/", {"email": sample_user.email, "password": "password"}, format="json") - assert res.status_code == 200 + assert res.status_code == status.HTTP_200_OK + + +@pytest.mark.django_db +def test_wrong_email(api_client, sample_user): + res = api_client.post("/api/login/", {"email": "wrong@example.com", "password": "password"}, format="json") + assert res.status_code == status.HTTP_400_BAD_REQUEST + + +@pytest.mark.django_db +def test_wrong_password(api_client, sample_user): + res = api_client.post("/api/login/", {"email": sample_user.email, "password": "wrong"}, format="json") + assert res.status_code == status.HTTP_400_BAD_REQUEST + + +@pytest.mark.django_db +def test_get_user(api_client, sample_user): + api_client.force_authenticate(sample_user) + res = api_client.get(f"/api/users/{sample_user.pk}/") + assert res.status_code == status.HTTP_200_OK + assert res.data["email"] == sample_user.email + + +@pytest.mark.django_db +def test_get_other_user(api_client, sample_user, user_factory): + api_client.force_authenticate(sample_user) + other_user = user_factory() + other_user.save() + res = api_client.get(f"/api/users/{other_user.pk}/") + assert res.status_code == status.HTTP_404_NOT_FOUND + + +@pytest.mark.django_db +def test_update_user(api_client, sample_user): + existing_email = sample_user.email + api_client.force_authenticate(sample_user) + data = {"email": "example@example.com", "password": "password", "first_name": "Test", "last_name": "User"} + res = api_client.put(f"/api/users/{sample_user.pk}/", data, format="json") + assert res.status_code == status.HTTP_200_OK + sample_user.refresh_from_db() + # Email should NOT have changed + assert sample_user.email == existing_email + assert sample_user.first_name == data["first_name"] == res.data["first_name"] + assert sample_user.last_name == data["last_name"] == res.data["last_name"] + + +@pytest.mark.django_db +def test_delete_user(api_client, sample_user): + api_client.force_authenticate(sample_user) + res = api_client.delete(f"/api/users/{sample_user.pk}/") + assert res.status_code == status.HTTP_204_NO_CONTENT + sample_user.refresh_from_db() + assert sample_user.is_active is False @pytest.mark.use_requests @@ -67,7 +127,7 @@ def test_password_reset(caplog, api_client, sample_user): # Verify the link works for reseting the password response = api_client.post(password_reset_url, data={"password": "new_password"}, format="json") - assert response.status_code == 200 + assert response.status_code == status.HTTP_200_OK # New Password should now work for authentication serializer = UserLoginSerializer(data={"email": sample_user.email, "password": "new_password"}) @@ -87,7 +147,7 @@ class TestPreviewTemplateView: @override_settings(DEBUG=False) def test_disabled_if_not_debug(self, client): response = client.post(self.url) - assert response.status_code == 404 + assert response.status_code == status.HTTP_404_NOT_FOUND @override_settings(DEBUG=True) def test_enabled_if_debug(self, client): @@ -98,19 +158,19 @@ def test_enabled_if_debug(self, client): @override_settings(DEBUG=True) def test_no_template_provided(self, client): response = client.post(self.url, data={"_send_to": "someone@example.com"}) - assert response.status_code == 400 + assert response.status_code == status.HTTP_400_BAD_REQUEST assert any("You must provide a template name" in e for e in response.json()) @override_settings(DEBUG=True) def test_invalid_template_provided(self, client): response = client.post(f"{self.url}?template=SOME_TEMPLATE/WHICH_DOES_NOT/EXIST", data={"_send_to": "someone@example.com"}) - assert response.status_code == 400 + assert response.status_code == status.HTTP_400_BAD_REQUEST assert any("Invalid template name" in e for e in response.json()) @override_settings(DEBUG=True) def test_missing_send_to(self, client): response = client.post(f"{self.url}?template=SOME_TEMPLATE/WHICH_DOES_NOT/EXIST") - assert response.status_code == 400 + assert response.status_code == status.HTTP_400_BAD_REQUEST assert "This field is required." in response.json()["_send_to"] def test_parse_value_without_model(self): diff --git a/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/views.py b/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/views.py index 42b4ce05b..b6365da38 100755 --- a/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/views.py +++ b/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/core/views.py @@ -18,11 +18,11 @@ from rest_framework.exceptions import ValidationError from rest_framework.response import Response -from {{ cookiecutter.project_slug }}.core.forms import PreviewTemplateForm from {{ cookiecutter.project_slug }}.utils.emails import send_html_email +from .forms import PreviewTemplateForm from .models import User -from .permissions import CreateOnlyPermissions +from .permissions import HasUserPermissions from .serializers import UserLoginSerializer, UserRegistrationSerializer, UserSerializer logger = logging.getLogger(__name__) @@ -50,45 +50,45 @@ def post(self, request, *args, **kwargs): return Response(response_data) -class UserViewSet( - viewsets.GenericViewSet, - mixins.RetrieveModelMixin, - mixins.ListModelMixin, - mixins.UpdateModelMixin, - mixins.DestroyModelMixin, -): - queryset = User.objects.all() +class UserViewSet(viewsets.GenericViewSet, mixins.RetrieveModelMixin): + queryset = User.objects serializer_class = UserSerializer # No auth required to create user # Auth required for all other actions - permission_classes = (permissions.IsAuthenticated | CreateOnlyPermissions,) + permission_classes = (HasUserPermissions,) - @transaction.atomic - def create(self, request, *args, **kwargs): + def get_queryset(self): """ - Endpoint to create/register a new user. + Users should only find themselves by default """ + return super().get_queryset().for_user(self.request.user) + + @transaction.atomic + def create(self, request, *args, **kwargs): serializer = UserRegistrationSerializer(data=request.data) serializer.is_valid(raise_exception=True) - serializer.save() # This calls .create() on serializer - user = serializer.instance - + user = serializer.save() # Log-in user and re-serialize response response_data = UserLoginSerializer.login(user, request) return Response(response_data, status=status.HTTP_201_CREATED) def update(self, request, *args, **kwargs): - """ - Endpoint to create/register a new user. - """ serializer = UserSerializer(data=request.data, instance=self.get_object(), partial=True) - serializer.is_valid(raise_exception=True) serializer.save() - user = serializer.data + return Response(serializer.data, status=status.HTTP_200_OK) - return Response(user, status=status.HTTP_200_OK) + def destroy(self, request, *args, **kwargs): + """ + When deleting a user's account, just disable their account first + The user may have a regret and try to get their account back + A background job should then properly delete the data after X days + """ + user = self.get_object() + user.is_active = False + user.save() + return Response(status=status.HTTP_204_NO_CONTENT) @api_view(["post"]) @@ -117,7 +117,7 @@ def request_reset_link(request, *args, **kwargs): def reset_password(request, *args, **kwargs): user_id = kwargs.get("uid") token = kwargs.get("token") - user = User.objects.filter(id=user_id).first() + user = User.objects.filter(pk=user_id).first() if not user or not token: raise ValidationError(detail={"non-field-error": "Invalid or expired token"}) is_valid = default_token_generator.check_token(user, token) diff --git a/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/settings.py b/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/settings.py index be4c00973..db7c19047 100644 --- a/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/settings.py +++ b/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/settings.py @@ -38,6 +38,9 @@ if CURRENT_DOMAIN not in ALLOWED_HOSTS: ALLOWED_HOSTS.append(CURRENT_DOMAIN) +# Used by the corsheaders app/middleware (django-cors-headers) to allow multiple domains to access the backend +CORS_ALLOWED_ORIGINS = [f"https://{host}" for host in ALLOWED_HOSTS] + # Application definition INSTALLED_APPS = [ @@ -306,7 +309,6 @@ if not IN_DEV: SECURE_SSL_REDIRECT = True SECURE_PROXY_SSL_HEADER = ("HTTP_X_FORWARDED_PROTO", "https") - MIDDLEWARE += ["django.middleware.security.SecurityMiddleware"] # # Custom logging configuration @@ -390,11 +392,6 @@ def filter(self, record): # Popular testing framework that allows logging to stdout while running unit tests TEST_RUNNER = "django_nose.NoseTestSuiteRunner" -CORS_ALLOWED_ORIGINS = ["https://{{ cookiecutter.project_slug|replace('_', '-') }}-staging.herokuapp.com", "https://{{ cookiecutter.project_slug|replace('_', '-') }}.herokuapp.com"] -{% if cookiecutter.client_app.lower() != 'none' -%} -CORS_ALLOWED_ORIGINS.append("http://localhost:8080") -{% endif -%} - SWAGGER_SETTINGS = { "LOGIN_URL": "/login", "USE_SESSION_AUTH": False, diff --git a/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/test_settings.py b/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/test_settings.py index 0d2d545f3..c6b2f64ea 100644 --- a/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/test_settings.py +++ b/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/test_settings.py @@ -1,7 +1,7 @@ from decouple import config -from {{ cookiecutter.project_slug }}.settings import LOGGING from {{ cookiecutter.project_slug }}.settings import * # noqa +from {{ cookiecutter.project_slug }}.settings import LOGGING # Override staticfiles setting to avoid cache issues with whitenoise Manifest staticfiles storage # See: https://stackoverflow.com/a/69123932 diff --git a/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/urls.py b/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/urls.py index 3b962cf51..bf86ab21e 100755 --- a/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/urls.py +++ b/{{cookiecutter.project_slug}}/server/{{cookiecutter.project_slug}}/urls.py @@ -1,6 +1,9 @@ from django.contrib import admin from django.urls import include, path +admin.site.site_header = "{{ cookiecutter.project_name }} Admin" +admin.site.site_title = "{{ cookiecutter.project_name }}" + urlpatterns = [ path(r"staff/", admin.site.urls), path(r"", include("{{ cookiecutter.project_slug }}.common.favicon_urls")),