Skip to content

Commit

Permalink
fix: adds tests and fixes issues raised by them (#83)
Browse files Browse the repository at this point in the history
* fix: passwords are now properly validated so they are not too similar to other account attributes.

* refactor: modifies VerifyAccountView to satisfy the proper separation of concerns.

Serializer validation is just for to make sure entry data is in the expected format.

* chore: add tests for some endpoints

* chore: remove unused ignore comments for mypy

* chore: just for Eugene, you fuck. Here, it is under your comment now

* chore: add first party modules to isort config

* chore: add the dumb linter changes

* chore: review suggestions
  • Loading branch information
corp-0 authored Feb 18, 2024
1 parent 511357c commit 1591ed5
Show file tree
Hide file tree
Showing 15 changed files with 407 additions and 27 deletions.
9 changes: 9 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ ignore = [
"RUF012",
# treats a link with the word "password" in it as a hardcoded password
"S105",
# the limit is too low and dumb. We don't use squared monitors anymore
"E501",
]
select = [
# pyflakes
Expand Down Expand Up @@ -59,13 +61,20 @@ fixable = [
[tool.ruff.lint.isort]
combine-as-imports = true
lines-between-types = 1
known-first-party = [
"accounts",
"central_command",
"commons",
"persistence",
]

[tool.mypy]
show_column_numbers = true
show_error_codes = true

# XXX: add new rules here
check_untyped_defs = true
warn_unused_ignores = true

plugins = [
"mypy_django_plugin.main",
Expand Down
21 changes: 8 additions & 13 deletions src/accounts/api/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,12 @@ class Meta:
fields = ("unique_identifier", "username", "password", "email")
extra_kwargs = {"password": {"write_only": True}}

def validate_password(self, value):
# Validate the password using Django's built-in validators
temp_user = Account(**self.initial_data)
validate_password(value, temp_user)
return value

def create(self, validated_data):
"""Create and return a new account"""
account: Account = Account.objects.create_user(**validated_data)
Expand Down Expand Up @@ -59,25 +65,14 @@ class VerifyAccountSerializer(serializers.Serializer):
unique_identifier = serializers.CharField()
verification_token = serializers.UUIDField()

def validate(self, data):
account = Account.objects.get(unique_identifier=data["unique_identifier"])

data_token = data["verification_token"]
account_token = account.verification_token

if account_token != data_token:
raise serializers.ValidationError(
"Verification token seems invalid or maybe outdated. Try requesting a new one."
)
return account


class ResetPasswordSerializer(serializers.Serializer):
password = serializers.CharField(style={"input_type": "password"})

def validate_password(self, value):
# Validate the password using Django's built-in validators
validate_password(value)
temp_user = Account(**self.initial_data)
validate_password(value, temp_user)
return value


Expand Down
24 changes: 15 additions & 9 deletions src/accounts/api/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@
from urllib.parse import urljoin
from uuid import uuid4

from commons.error_response import ErrorResponse
from commons.mail_wrapper import send_email_with_template
from django.conf import settings
from django.contrib.auth import authenticate
from django.core.exceptions import ObjectDoesNotExist, PermissionDenied
Expand All @@ -19,6 +17,9 @@
from rest_framework.serializers import ValidationError
from rest_framework.views import APIView

from commons.error_response import ErrorResponse
from commons.mail_wrapper import send_email_with_template

from ..models import Account, AccountConfirmation, PasswordResetRequestModel
from .serializers import (
ConfirmAccountSerializer,
Expand Down Expand Up @@ -89,13 +90,13 @@ def post(self, request):
account: Account | None = authenticate(email=email, password=password) # type: ignore[assignment]

if account is None:
return ErrorResponse("Unable to login with provided credentials.", status.HTTP_400_BAD_REQUEST)
return ErrorResponse("Unable to login with provided credentials.", status.HTTP_401_UNAUTHORIZED)

if not account.is_confirmed:
return ErrorResponse("You must confirm your email before attempting to login.", status.HTTP_400_BAD_REQUEST)
return ErrorResponse("You must confirm your email before attempting to login.", status.HTTP_401_UNAUTHORIZED)

if not account.is_active:
return ErrorResponse("Account is suspended.", status.HTTP_400_BAD_REQUEST)
return ErrorResponse("Account is suspended.", status.HTTP_401_UNAUTHORIZED)

return Response(
{
Expand Down Expand Up @@ -203,12 +204,17 @@ class VerifyAccountView(GenericAPIView):
def post(self, request):
serializer = self.get_serializer(data=request.data)

if not serializer.is_valid():
return ErrorResponse(serializer.errors, status.HTTP_400_BAD_REQUEST)

try:
serializer.is_valid(raise_exception=True)
except ValidationError as e:
return ErrorResponse(str(e), e.status_code)
account = Account.objects.get(unique_identifier=serializer.validated_data["unique_identifier"])
except Account.DoesNotExist:
return ErrorResponse("Either token or unique_identifier are invalid.", status.HTTP_400_BAD_REQUEST)

if account.verification_token != serializer.validated_data["verification_token"]:
return ErrorResponse("Either token or unique_identifier are invalid.", status.HTTP_400_BAD_REQUEST)

account = Account.objects.get(unique_identifier=serializer.data["unique_identifier"])
public_data = PublicAccountDataSerializer(account).data

return Response(public_data, status=status.HTTP_200_OK)
Expand Down
36 changes: 36 additions & 0 deletions src/accounts/custom_attribute_similarity_validator.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
from django.contrib.auth.password_validation import UserAttributeSimilarityValidator
from django.core.exceptions import ValidationError
from django.db.models import Model

from accounts.models import Account


class CustomUserAttributeSimilarityValidator(UserAttributeSimilarityValidator):
def validate(self, password: str, user: Model | None = None):
if user is None:
return

if not isinstance(user, Account):
raise Exception(
f"User is not an instance of Account: {user}. Check if the right user model is being set at settings.py."
)

custom_attributes = [
user.username,
user.email,
user.unique_identifier,
]

for attribute in custom_attributes:
if not attribute or not password:
continue

password = password.lower()
attribute = attribute.lower()

if password in attribute or attribute in password:
raise ValidationError(
"The password is too similar to the %(verbose_name)s.",
code="password_too_similar",
params={"verbose_name": attribute},
)
3 changes: 2 additions & 1 deletion src/accounts/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,13 @@
from urllib.parse import urljoin
from uuid import uuid4

from commons.mail_wrapper import send_email_with_template
from django.conf import settings
from django.contrib.auth.models import AbstractUser
from django.db import models
from django.utils import timezone

from commons.mail_wrapper import send_email_with_template

from .validators import AccountNameValidator, UsernameValidator


Expand Down
3 changes: 3 additions & 0 deletions src/central_command/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,9 @@
{
"NAME": "django.contrib.auth.password_validation.NumericPasswordValidator",
},
{
"NAME": "accounts.custom_attribute_similarity_validator.CustomUserAttributeSimilarityValidator",
},
]

# Internationalization
Expand Down
6 changes: 3 additions & 3 deletions src/commons/error_response.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,11 @@ def custom_exception_handler(exc, context):
"""
response = exception_handler(exc, context)

if response is not None:
response.data["status_code"] = response.status_code
else:
if response is None:
logger.error("An unhandled error occurred: %s", exc)
logger.error("Context: %s", context)
return ErrorResponse("Something went wrong on our end. Please try again later.")

response.data["status_code"] = response.status_code

return response
Empty file added src/tests/accounts/__init__.py
Empty file.
Empty file.
57 changes: 57 additions & 0 deletions src/tests/accounts/endpoints/test_login_credentials.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
from django.urls import reverse
from rest_framework import status
from rest_framework.test import APITestCase

from accounts.models import Account


class LoginCredentialsTest(APITestCase):
def setUp(self):
self.url = reverse("account:login-credentials")
self.valid_account = Account.objects.create_user(
username="validUser",
email="[email protected]",
unique_identifier="validUser",
)

self.valid_account.set_password("aValidPss963")
self.valid_account.is_confirmed = True
self.valid_account.save()
self.valid_login_data = {
"email": "[email protected]",
"password": "aValidPss963",
}

def test_login_with_valid_credentials(self):
response = self.client.post(self.url, self.valid_login_data, format="json")
self.assertEqual(response.status_code, status.HTTP_200_OK)

def test_login_with_invalid_credentials(self):
data = {
"email": "[email protected]",
"password": "wrongPassword",
}
response = self.client.post(self.url, data, format="json")
self.assertEqual(response.status_code, status.HTTP_401_UNAUTHORIZED)

def test_login_with_unconfirmed_account(self):
self.valid_account.is_confirmed = False
self.valid_account.save()

response = self.client.post(self.url, self.valid_login_data, format="json")
self.assertEqual(response.status_code, status.HTTP_401_UNAUTHORIZED)

def test_login_with_inactive_account(self):
self.valid_account.is_active = False
self.valid_account.save()

response = self.client.post(self.url, self.valid_login_data, format="json")
self.assertEqual(response.status_code, status.HTTP_401_UNAUTHORIZED)

def test_login_with_invalid_email(self):
data = {
"email": "invalidEmail",
"password": "aValidPss963",
}
response = self.client.post(self.url, data, format="json")
self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
41 changes: 41 additions & 0 deletions src/tests/accounts/endpoints/test_login_token.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
from django.urls import reverse
from rest_framework import status
from rest_framework.test import APITestCase

from accounts.models import Account


class LoginTokenTest(APITestCase):
def setUp(self):
self.url = reverse("account:login-token")
self.valid_account = Account.objects.create_user(
username="validUser",
email="[email protected]",
unique_identifier="validUser",
)

self.valid_account.set_password("aValidPss963")
self.valid_account.is_confirmed = True
self.valid_account.save()
self.valid_login_data = {
"email": "[email protected]",
"password": "aValidPss963",
}

# needs to log in with credentials to get the token
response = self.client.post(reverse("account:login-credentials"), self.valid_login_data, format="json")
self.token = response.data["token"]

def test_login_with_valid_token(self):
self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token}")
response = self.client.post(self.url, format="json")
self.assertEqual(response.status_code, status.HTTP_200_OK)

def test_login_with_invalid_token(self):
self.client.credentials(HTTP_AUTHORIZATION="Token invalidToken")
response = self.client.post(self.url, format="json")
self.assertEqual(response.status_code, status.HTTP_401_UNAUTHORIZED)

def test_login_without_token(self):
response = self.client.post(self.url, format="json")
self.assertEqual(response.status_code, status.HTTP_401_UNAUTHORIZED)
Loading

0 comments on commit 1591ed5

Please sign in to comment.