Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backend Security Audit fixes #352

Merged
merged 9 commits into from
Sep 12, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/pytest.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion {{cookiecutter.project_slug}}/pytest.ini
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,4 @@ class Meta:
abstract = True

def __str__(self):
return "ah yes"
return "__str__ not defined for this model"
Copy link
Member Author

Choose a reason for hiding this comment

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

gentle nudge for devs to set this for their new models

Copy link
Member

Choose a reason for hiding this comment

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

Thank you

Original file line number Diff line number Diff line change
Expand Up @@ -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 = (
(
Expand All @@ -35,7 +35,7 @@ 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",
Expand All @@ -57,7 +57,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()]
Copy link
Member

Choose a reason for hiding this comment

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

@oudeismetis - I trust that you checked that this works? This is changing the return type from str to list[str].

Copy link
Member Author

Choose a reason for hiding this comment

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

correct. I pulled this from a project where I made the change.
The join isn't needed and happens for you

Copy link
Member Author

Choose a reason for hiding this comment

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

Just re-tested on a locally generated my_project.
image

Copy link
Member

Choose a reason for hiding this comment

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

Great! This is good to go then.


class Media(AutocompleteAdminMedia):
pass
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ class Meta:
"last_name",
"full_name",
)
read_only_fields = ["email"]
Copy link
Member Author

Choose a reason for hiding this comment

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

Default behavior shouldn't allow changing of a users email just by having a valid token.
We should consider implement a proper endpoint for this, but it should require a user to re-enter their password and probably should trigger an email to the old email address when the change is made and possibly require them to verify they control the new email address. (currently this last one doesn't happen for registration either, but it probably should)

Copy link
Member Author

Choose a reason for hiding this comment

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

@paribaker I know you had thoughts about this one



class UserLoginSerializer(serializers.ModelSerializer):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,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")
Expand All @@ -50,7 +57,57 @@ 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, sample_user_2):
api_client.force_authenticate(sample_user)
res = api_client.get(f"/api/users/{sample_user_2.pk}/")
assert res.status_code == status.HTTP_403_FORBIDDEN


@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
Expand All @@ -67,7 +124,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"})
Expand All @@ -87,7 +144,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):
Expand All @@ -98,19 +155,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):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@
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 .serializers import UserLoginSerializer, UserRegistrationSerializer, UserSerializer
Expand Down Expand Up @@ -50,13 +50,7 @@ def post(self, request, *args, **kwargs):
return Response(response_data)


class UserViewSet(
viewsets.GenericViewSet,
mixins.RetrieveModelMixin,
mixins.ListModelMixin,
mixins.UpdateModelMixin,
mixins.DestroyModelMixin,
):
class UserViewSet(viewsets.GenericViewSet, mixins.RetrieveModelMixin):
Copy link
Member Author

Choose a reason for hiding this comment

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

We implement some of these with overrides below.
Oddly, by implementing update() and having the UpdateModelMixin above, we ended up with PATCH and POST endpoints that both handled updating, but had different rules and validation.
Most projects shouldn't need all of these (ex: listing all users). If they do, it's easy enough to add them, along with the proper security controls.

queryset = User.objects.all()
Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to remove this but it raises an error:
AssertionError: basenameargument not specified, and could not automatically determine the name from the viewset, as it does not have a.queryset attribute.

I couldn't find any deep discussion on how to better use the queryset here. You can't auto-filter here by the logged in user or anything like that. All examples just have it as .all(). So this seems to be field you just HAVE to have.
But it doesn't degrade our security at all as additional filters are tacked on to this

Copy link
Member

Choose a reason for hiding this comment

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

Ah, in that case I think you could replace it with User.objects.none()

Copy link
Member Author

Choose a reason for hiding this comment

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

I like it, but that doesn't work. A couple of tests are failing now because the chain of filters starts by finding nothing.

Copy link
Member Author

Choose a reason for hiding this comment

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

setting it to just User.objects works though. And that would imply to devs that filters get layered on elsewhere

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think that's a good place to land

serializer_class = UserSerializer

Expand All @@ -66,29 +60,29 @@ class UserViewSet(

@transaction.atomic
def create(self, request, *args, **kwargs):
"""
Endpoint to create/register a new user.
"""
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)
Copy link
Member Author

Choose a reason for hiding this comment

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

Comment above should be clear.
By default apps shouldn't just delete important data from the DB like this.



@api_view(["post"])
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [
Expand Down Expand Up @@ -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"]
Copy link
Member Author

Choose a reason for hiding this comment

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

duplicate from another spot in this file.


#
# Custom logging configuration
Expand Down Expand Up @@ -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 -%}

Copy link
Member Author

Choose a reason for hiding this comment

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

See further up.
This is now controlled by ALLOWED_HOSTS. Hardcoding values like localhost into this file is a security risk.

SWAGGER_SETTINGS = {
"LOGIN_URL": "/login",
"USE_SESSION_AUTH": False,
Expand Down
Original file line number Diff line number Diff line change
@@ -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 }}"

Copy link
Member Author

Choose a reason for hiding this comment

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

Just a nice little addition so that managing multiple projects makes it more obvious which admin you are in.

Also, non-dev project stakeholders don't know (or need to care) what the "Django Admin" is. It's just the admin for the app.

urlpatterns = [
path(r"staff/", admin.site.urls),
path(r"", include("{{ cookiecutter.project_slug }}.common.favicon_urls")),
Expand Down
Loading