Legacy Lobotomy — Refactoring of the Password Reset Confirm Endpoint

Photo by rc.xyz NFT gallery on Unsplash

This is the 5th tutorial in the series about refactoring a legacy Django project. In this tutorial I will be working on the second step of the password reset functionality, when a user needs to set up a new password.

To get more ideas regarding the project used for this tutorial or check other tutorials in this series, check the introductory article published earlier.

Legacy Lobotomy — Confident Refactoring of a Django Project

This endpoint is provided by the django-rest-auth package, but it was changed in order to satisfy the project needs. Here is the list of custom changes for this functionality:

1. The original implementation trims all leading and trailing white spaces for the values passed in the fields new_password1 and new_password2. For this project this rule was changed to allow a user to enter a password with leading and trailing white spaces. But the system shouldn't allow to save such a password. This is implemented this way to let a user know if they have white spaces in their password, and it's not allowed, instead of just trimming spaces and saving password silently. For this purpose a new PasswordResetConfirmSerializer serializer inherited from the original rest_auth.serializers.PasswordResetConfirmSerializer serializer was created.
2. In order to make the view use the custom serializer, another view PasswordResetConfirmView inherited from the original rest_auth.views.PasswordResetConfirmView view was created.
3. To make the new view handle incoming requests, a new path was added to the URL patterns.
4. If we take a look at the base.py file which contains base settings of the project, we will see that AUTH_PASSWORD_VALIDATORS property is overridden that means custom password validation rules were configured. More information about password validation in Django can be taken from the official documentation.
5. We also can see that a set of custom password validators was implemented and added for password validation:
   — users.validators.NumberValidator - checks that a password contains at least 1 digit
   — users.validators.UppercaseValidator - checks that a password contains at least 1 uppercase letter
   — users.validators.LowercaseValidator - checks that a password contains at least 1 lowercase letter
   — users.validators.SymbolValidator - checks that a password contains at least 1 special symbol
   — users.validators.SpaceValidator - checks that a password shouldn't contain any space symbols

As we can see, there are a lot of changes made comparing to the out-of-the-box solution, so we need to prepare a set of tests to make sure everything works as expected.

Origin branch and destination branch

1. If you want to follow the steps described in this tutorial, you should start from the password-reset-request-refactoring branch.
2. If you want to only see final code state after all changes from this tutorial are applied, you can check the password-reset-confirm-refactoring branch.

Preparing tests

Let’s start from creating a new test_password_reset_confirm.py file with the content as below in the ~/src/users/tests/api directory.

import random

# 1. Provides a convenient way to get access to groups of symbols
import string

import pytest
from django.contrib.auth.tokens import default_token_generator
from rest_framework import status


class TestPasswordResetConfirm:
    # 2. Define an endpoint which should handle requests for changing the password
    reset_password_confirm_endpoint = '/auth/password/reset/confirm/'

    # 3. Define a password that's guaranteed to be valid
    valid_password = 'Super#StrongPassword123'

    # 4. Define a password that will be valid if add a digit
    password_without_digit = 'StrongPassword?'

    # 5. Define a password that will be valid if add an uppercase letter
    password_without_uppercase_letter = 'password1?'

    # 6. Define a password that will be valid if add a lowercase letter
    password_without_lowercase_letter = 'PASSWORD1?'

    # 7. Define a password that will be valid if add a special symbol
    password_without_special_symbol = 'StrongPassword1'

    # 8. A list of possible locations of the symbol in the string
    symbol_positions = ['beginning', 'middle', 'ending']

    # 9. Define a fixture with a valid body for changing the password of the user passed as an argument
    @pytest.fixture
    def valid_request_body(self, user, uid):
        return {
            'new_password1': self.valid_password,
            'new_password2': self.valid_password,
            'uid': uid(user.pk),
            'token': default_token_generator.make_token(user),
        }
    
    # 10. A helper method which inserts a tested symbol into the tested password value
    def _insert_symbol_into_string(self, destination, symbol, position):
        if position == 'beginning':
            return f'{symbol}{destination}'
        if position == 'ending':
            return f'{destination}{symbol}'

        index = random.randint(1, len(destination) - 1)

        return f'{destination[:index]}{symbol}{destination[index:]}'

Validation tests

As in the previous tutorial, let’s start from creating tests to check if validation works as expected.

Current endpoint expects a client to pass 4 fields:

1. new_password1 - a new password which the user wants to set up
2. new_password2 - a password confirmation
3. uid - an encoded identifier of the user who wants to change the password
4. token - a token sent to the user which contains information about current user and time until which the token is valid

All these fields are mandatory and the client must pass them in the request body. Let’s create a test which checks the case when the client sends a request with empty body:

class TestPasswordResetConfirm:
   # ---------- Other content is omitted for simplicity ------------ #
   def test_when_request_body_is_empty_then_400_bad_request_status_should_be_returned(self, client):
       response = client.post(self.reset_password_confirm_endpoint, {})
   
       assert response.status_code == status.HTTP_400_BAD_REQUEST
       assert response.json() == {
           'new_password1': ['This field is required.'],
           'new_password2': ['This field is required.'],
           'uid': ['This field is required.'],
           'token': ['This field is required.']
       }

Also let’s check the case when the client passes valid values for 3 fields but the last one is absent. For this test case we need to use our valid_request_body fixture and add parametrization to the test, so that a single test will be used for checking all possible cases. Here is the test which checks this scenario:

class TestPasswordResetConfirm:
   # ---------- Other content is omitted for simplicity ------------ #
   @pytest.mark.django_db
   @pytest.mark.parametrize('missed_field_name', [
       pytest.param('new_password1', id='new_password1 is not present'),
       pytest.param('new_password2', id='new_password2 is not present'),
       pytest.param('uid', id='uid is not present'),
       pytest.param('token', id='token is not present'),
   ])
   def test_when_a_single_required_field_is_missing_then_400_bad_request_status_should_be_returned(
           self, valid_request_body, missed_field_name, client
   ):
       request_body = {**valid_request_body}
       request_body.pop(missed_field_name, None)
       response = client.post(self.reset_password_confirm_endpoint, request_body)
   
       assert response.status_code == status.HTTP_400_BAD_REQUEST
       assert response.json() == {
           missed_field_name: ['This field is required.'],
       }

In this test we just specify which field should be removed, and it’s passed as missed_field_name argument in each test run, and remove this field from the valid request body.

Next scenario which we will check is when all fields are passed but one of the values is invalid:

1. new_password1 field is blank
2. new_password1 field has more than 128 symbols
3. new_password2 field is blank
4. new_password2 field has more than 128 symbols
5. uid field is blank
6. uid field has an invalid value
7. token field is blank
8. token field has an invalid value

Let’s add the test below which covers all these situations.

class TestPasswordResetConfirm:
   # ---------- Other content is omitted for simplicity ------------ #
   @pytest.mark.django_db
   @pytest.mark.parametrize(
       'field_name, value, expected_message', [
           pytest.param('new_password1', '', 'This field may not be blank.', id='new_password1 is empty'),
           pytest.param(
               'new_password1', 'a' * 129,
               'Ensure this field has no more than 128 characters.',
               id='new_password1 consists of 129 symbols'
           ),
           pytest.param('new_password2', '', 'This field may not be blank.', id='new_password2 is empty'),
           pytest.param(
               'new_password2', 'a' * 129,
               'Ensure this field has no more than 128 characters.',
               id='new_password2 consists of 129 symbols'
           ),
           pytest.param('uid', '', 'This field may not be blank.', id='uid is empty'),
           pytest.param('uid', 'invalid_uid_value', 'Invalid value', id='uid is invalid'),
           pytest.param('token', '', 'This field may not be blank.', id='token is empty'),
           pytest.param('token', 'invalid_token_value', 'Invalid value', id='token is invalid'),
       ]
   )
   def test_when_at_least_one_field_is_invalid_then_400_bad_request_status_should_be_returned(
           self, field_name, value, expected_message, valid_request_body, client
   ):
       request_body = {**valid_request_body, field_name: value}
       response = client.post(self.reset_password_confirm_endpoint, request_body)
   
       assert response.status_code == status.HTTP_400_BAD_REQUEST
       assert response.json() == {field_name: [expected_message]}

In this test we pass a field name, value, expected error message and identifier of the test case to be able to have better names of the tests in the test run logs. Here we defined a parameters set for each of 8 test cases listed above.

Now let’s create a test for the case when password does not satisfy at least one validator configured for it:

1. The password is too short
2. The password is too common
3. The password doesn’t have digits
4. The password doesn’t have uppercase letters
5. The password doesn’t have lowercase letters
6. The password doesn’t have special symbols
7. The password has a white space either at the beginning, in the middle or at the end

The code of the test is below. If we run the tests now, we will see that 3 tests don’t pass. We will fix all the problems later, when all tests are written.

class TestPasswordResetConfirm:
   # ---------- Other content is omitted for simplicity ------------ #
    @pytest.mark.django_db
    @pytest.mark.parametrize('password, expected_message', [
        pytest.param(
            'Short#1', 'This password is too short. It must contain at least 8 characters.', id='short password'
        ),
        pytest.param('12345678', 'This password is too common.', id='common password'),
        pytest.param(
            password_without_digit,
            'The password must contain at least 1 digit, 0-9.',
            id='password without digit'
        ),
        pytest.param(
            password_without_uppercase_letter,
            'The password must contain at least 1 uppercase letter, A-Z.',
            id='password without uppercase letter'
        ),
        pytest.param(
            password_without_lowercase_letter,
            'The password must contain at least 1 lowercase letter, a-z.',
            id='password without lowercase letter'
        ),
        pytest.param(
            password_without_special_symbol,
            f'The password must contain at least 1 symbol: {string.punctuation}',
            id='password without special symbol'
        ),
        pytest.param(
            ' StrongPassword1?',
            'The password must not contain any space character.',
            id='password with a space at the beginning'
        ),
        pytest.param(
            'StrongPassword1? ',
            'The password must not contain any space character.',
            id='password with a space at the end'
        ),
        pytest.param(
            'Strong Password1?',
            'The password must not contain any space character.',
            id='password with a space in the middle'
        ),
    ])
    def test_when_password_is_not_strong_enough_then_400_bad_request_status_should_be_returned(
            self, password, expected_message, valid_request_body, client
    ):
        request_body = {**valid_request_body, 'new_password1': password, 'new_password2': password}
        response = client.post(self.reset_password_confirm_endpoint, request_body)

        assert response.status_code == status.HTTP_400_BAD_REQUEST
        assert expected_message in response.json()['new_password2']

Since trimming of white spaces was turned off for this endpoint, we need to ensure that when the client passes the same values for the new_password1 or new_password2 fields, but one of the values contain a white space at the beginning or at the end, the system should complain about passwords don't match. Here is the code of the test that checks all such possible combinations.

class TestPasswordResetConfirm:
    # ---------- Other content is omitted for simplicity ------------ #
    @pytest.mark.django_db
    @pytest.mark.parametrize('field_name, value', [
        ('new_password1', ' ' + valid_password),
        ('new_password1', valid_password + ' '),
        ('new_password2', ' ' + valid_password),
        ('new_password2', valid_password + ' '),
    ])
    def test_when_password_field_has_extra_leading_or_trailing_space_then_passwords_should_not_match(
            self, field_name, value, valid_request_body, client
    ):
        request_body = {**valid_request_body, field_name: value}
        response = client.post(self.reset_password_confirm_endpoint, request_body)
    
        assert response.status_code == status.HTTP_400_BAD_REQUEST
        assert response.json() == {'new_password2': ['The two password fields didn’t match.']}

Successful path tests

Once we are done with all validation tests, we should make sure that all scenarios we consider valid work as expected and a user can change the password.

Let’s start from the verifying that if a digit is present at any place of the password, this password will be set up for the user. The test below has two parametrize decorators: one contains all possible values for digits and another one contains all possible values for digit positions in the tested password. In this test we use password_without_digit property which should be valid if we add at least 1 digit. This configuration allows us to generate 30 different tests.

class TestPasswordResetConfirm:
    # ---------- Other content is omitted for simplicity ------------ #
    @pytest.mark.django_db
    @pytest.mark.parametrize('symbol_position', symbol_positions)
    @pytest.mark.parametrize('symbol', list(string.digits))
    def test_when_password_has_digit_at_any_position_it_should_be_saved(
            self, symbol_position, symbol, user, valid_request_body, client
    ):
        password = self._insert_symbol_into_string(self.password_without_digit, symbol, symbol_position)
        request_body = {**valid_request_body, 'new_password1': password, 'new_password2': password}
        response = client.post(self.reset_password_confirm_endpoint, request_body)

        user.refresh_from_db()

        assert response.status_code == status.HTTP_200_OK
        assert user.check_password(password)

Using the same logic, we should add 3 more tests to check how adding of lowercase and uppercase letters and special symbols make an invalid password valid. As base values we will be using password_without_lowercase_letter, password_without_uppercase_letter and password_without_special_symbol properties accordingly.

class TestPasswordResetConfirm:
    # ---------- Other content is omitted for simplicity ------------ #
    @pytest.mark.django_db
    @pytest.mark.parametrize('symbol_position', symbol_positions)
    @pytest.mark.parametrize('symbol', list(string.ascii_lowercase))
    def test_when_password_has_lowercase_letter_at_any_position_it_should_be_saved(
            self, symbol_position, symbol, user, valid_request_body, client
    ):
        password = self._insert_symbol_into_string(self.password_without_lowercase_letter, symbol, symbol_position)
        request_body = {**valid_request_body, 'new_password1': password, 'new_password2': password}
        response = client.post(self.reset_password_confirm_endpoint, request_body)

        user.refresh_from_db()

        assert response.status_code == status.HTTP_200_OK
        assert user.check_password(password)

    @pytest.mark.django_db
    @pytest.mark.parametrize('symbol_position', symbol_positions)
    @pytest.mark.parametrize('symbol', list(string.ascii_uppercase))
    def test_when_password_has_uppercase_letter_at_any_position_it_should_be_saved(
            self, symbol_position, symbol, user, valid_request_body, client
    ):
        password = self._insert_symbol_into_string(self.password_without_uppercase_letter, symbol, symbol_position)
        request_body = {**valid_request_body, 'new_password1': password, 'new_password2': password}
        response = client.post(self.reset_password_confirm_endpoint, request_body)

        user.refresh_from_db()

        assert response.status_code == status.HTTP_200_OK
        assert user.check_password(password)

    @pytest.mark.django_db
    @pytest.mark.parametrize('symbol_position', symbol_positions)
    @pytest.mark.parametrize('symbol', list(string.punctuation))
    def test_when_password_has_special_symbol_at_any_position_it_should_be_saved(
            self, symbol_position, symbol, user, valid_request_body, client
    ):
        password = self._insert_symbol_into_string(self.password_without_special_symbol, symbol, symbol_position)
        request_body = {**valid_request_body, 'new_password1': password, 'new_password2': password}
        response = client.post(self.reset_password_confirm_endpoint, request_body)

        user.refresh_from_db()

        assert response.status_code == status.HTTP_200_OK
        assert user.check_password(password)

Bug fixing

Now let’s run all the tests. We can see that 6 of 314 tests don’t pass.

Result of running all tests — 6 tests were fallen

We should start from fixing test_when_password_has_special_symbol_at_any_position_it_should_be_saved test. As shown on the screenshot, when we pass backslash our validation fails, but it shouldn't. It means something's wrong with the custom SymbolValidator validator. Here is its code:

class SymbolValidator(object):
    def validate(self, password, user=None):
        if not re.findall('[()[\]{}|\\`~!@#$%^&*_\-+=;:\'",<>./?]', password):
            raise ValidationError(
                _("The password must contain at least 1 symbol: " + "()[]{}|\`~!@#$%^&*_-+=;:'\",<>./?"),
                code='password_no_symbol', )

If we take a look at the string passed as a regular expression into the re.findall method, we will see that backslash is escaped with only one backslash "\\" but it should be escaped with 2 backslashes "\\\". Let's replace "\\" with "\\\" and run the tests again. Now all subtests of the test_when_password_has_special_symbol_at_any_position_it_should_be_saved test should pass.

Now we need to fix 3 subtests of the test_when_password_is_not_strong_enough_then_400_bad_request_status_should_be_returned test.

In the running details of the password without uppercase letter subtest we see this assertion error:

AssertionError: assert 'The password must contain at least 1 uppercase letter, A-Z.' in ['The password must contain at least 1 uppercase letter, A-Z']

In the running details of the password without digit subtest we see this assertion error:

AssertionError: assert 'The password must contain at least 1 digit, 0-9.' in ['The password must contain at least 1 digit, 0-9']

As we can see, both subtests expect the error message to end with a dot. Let’s add dots to the end of the error messages in the NumberValidator and UppercaseValidator validators and run all the tests again. Now we should have only password without special symbol subtest not passed.

This subtest doesn’t pass because it uses string.punctuation property to show in the error message a list of acceptable special symbols when SymbolValidator validator has a hardcoded list of such symbols. Are there any differences between symbols the string.punctuation consists of and the ones hardcoded in the validator? Let's compare both the lists.

# The list from the validator:
# ['!', '"', '#', '$', '%', '&', "'", '(', ')', '*', '+', ',', '-', '.', '/', ':', ';', '<', '=', '>', '?', '@', '[', '\\', '\\', '\\', '\\', ']', '^', '_', '`', '{', '|', '}', '~']
print(sorted(list('()[\]{}|\\\`~!@#$%^&*_\-+=;:\'",<>./?')))

# The list from the string.punctuation property
# ['!', '"', '#', '$', '%', '&', "'", '(', ')', '*', '+', ',', '-', '.', '/', ':', ';', '<', '=', '>', '?', '@', '[', '\\', ']', '^', '_', '`', '{', '|', '}', '~']
print(sorted(list(string.punctuation)))

As we can see, both lists contain the same values, but the list hardcoded in the validator has backslash added 4 times: '\\', '\\', '\\', '\\' while there is only one backslash symbol in the string.punctuation property. So we can just simply replace

"()[]{}|\`~!@#$%^&*_-+=;:'\",<>./?"

with string.punctuation:

class SymbolValidator(object):
    def validate(self, password, user=None):
        if not re.findall('[()[\]{}|\\\`~!@#$%^&*_\-+=;:\'",<>./?]', password):
            raise ValidationError(
                _("The password must contain at least 1 symbol: " + string.punctuation),
                code='password_no_symbol', )

    def get_help_text(self):
        return _("Your password must contain at least 1 symbol: " + string.punctuation)

Now all tests should pass.

Refactoring

By this moment we have already created all needed tests for the password reset confirm endpoint. It’s time to refactor the code a bit and make it look better.

First of all, let’s replace this line of the SymbolValidator:

if not re.findall('[()[\]{}|\\\`~!@#$%^&*_\-+=;:\'",<>./?]', password):

with this line:

if not re.findall(f'[{re.escape(string.punctuation)}]', password):

By doing this, we make the validator less error-prone and more clear and consistent with the error messages it uses.

Another thing we should do is replace findall method with search in all validators. We don't need to find all matches, it's enough to know that at least one match exists in the template. Thus, we can use search method which stops scanning the string once a first match is found. So the performance will be a bit improved. I am not sure if we can notice this improvement, but when we can make such a change easily, we should better do this. Run all the tests one more time and make sure they all pass.

I also updated formatting of the code in the validators.py file to make it look better. The final version of this file can be seen here.

The last thing we should change is delete PasswordResetConfirmView view and 2 endpoints where it's used. This view was created only to override the serializer which should be used for this endpoint. But django-rest-auth allows developers to override a serializer for any endpoint right in the project settings. Here is what we need to do:

Step 1. Delete PasswordResetConfirmView class from the ~/src/users/views.py file.

Step 2. Delete the following lines from the ~/src/app/urls.py file:

    path('auth/password/reset/confirm/', PasswordResetConfirmView.as_view(),
         name='rest_password_reset_confirm'),
    path('auth/password-reset-confirm/<uidb64>/<token>/',
         PasswordResetConfirmView.as_view(), name='password_reset_confirm'),

Step 3. If we run tests at this moment, we will see that 6 tests don’t pass. They all are intended for checking different cases related to not trimming leading and trailing spaces from the password value. It’s very good sign for 2 reasons: we know that after removing these 2 endpoints, we still can use the path /auth/password/reset/confirm/ for changing the password, and that we haven't updated the serializer yet and the default one is being used.

Step 4. Extend REST_AUTH_SERIALIZERS property in the ~/src/app/settings/configs/base.py file with this new item:

'PASSWORD_RESET_CONFIRM_SERIALIZER': 'users.serializers.PasswordResetConfirmSerializer'

Step 5. Also, we can rename RAPasswordResetConfirmSerializer to RestAuthPasswordResetConfirmSerializer. It's not necessary, but this name looks better and more descriptive from my point of view. If we run tests once again, they all should pass.

Conclusion

In this tutorial we’ve finished refactoring of the password reset functionality. By creating meaningful and detailed tests, we were able to detect and fix some important problems. Otherwise, these issues would be encountered by end users. In the next tutorial we will make sure that the login endpoint works as expected and refactor it a little.