Valid backup authentication codes don't log the user in.

[miniatureweasle]

Backup authentication codes don't log the user in.

Expected Behavior

Submitting a valid backup authentication code in the login form should result in a successful sign-in, with the user being redirected out of the login page.

Current Behavior

After submitting the valid code, the API responds with 200 but the user remains on the sign-in page, instead of being logged in.

Steps to Reproduce (for bugs)

1. Generate a backup authentication code from the provided setup wizard
2. Store these backup codes for later
3. Logout, and open the login page
4. When prompted to use an authentication code, click the button to use the backup codes instead
5. Enter a valid backup code, instead of a successful login, the page is reloaded.

Context

We are wanting to use this package to provide 2FA to our customers

Your Environment

• Browser and version:
• Python version: 3.9.5
• Django version: 3.1.8
• django-otp version:
• django-two-factor-auth version: 1.13.2 (latest)
• Link to your project: http://hiddenapp.com/

[moggers87]

the API responds with 200

On success, you should get a 302. Are you using your own templates? It's possible there's an error but your template isn't displaying it.

[Gautier]

(edited 02/08/22: the original custom SiteAuthenticationTokenForm was not working)

I have received the same bug report as @miniatureweasle for one of my Django websites and tracked the issue to the backup device throttling_failure_count being incremented every time the user logs in.

I have raised a failing test under #521 to demonstrate the issue as a unit test.

The problem seems to come from the fact that AuthenticationTokenForm tries to verify the token against each device and if the backup device is tried before the phone device a correct token received by SMS will increment the throttling_failure_count of the backup device. The user is able to log in correctly but after enough logins the backup device throttling_failure_count will be high enough for the backup tokens to be effectively unusable.

django-two-factor-auth/two_factor/forms.py

Lines 126 to 132 in 830915a

	def __init__(self, user, initial_device, **kwargs):
	"""
	`initial_device` is either the user's default device, or the backup
	device when the user chooses to enter a backup token. The token will
	be verified against all devices, it is not limited to the given
	device.
	"""

Workaround

My workaround is a custom AuthenticationTokenForm._chosen_device. This makes the form try only the token against the device relevant to the current step (initially the default phone device, but the user can choose an alternative phone device or a backup token).

from django_otp import devices_for_user

class SiteAuthenticationTokenForm(AuthenticationTokenForm):
    device_id = forms.CharField(widget=HiddenInput(), required=False)

    def __init__(self, user, initial_device, **kwargs):
        super(SiteAuthenticationTokenForm, self).__init__(
            user, initial_device, **kwargs
        )
        self.initial_device = initial_device
        self.fields["device_id"].initial = initial_device.persistent_id
        self._device_id_cache = None

    def clean_device_id(self):
        if self.cleaned_data["device_id"]:
            try:
                for user_device in devices_for_user(self.user):
                    if user_device.persistent_id == self.cleaned_data["device_id"]:
                        self._device_id_cache = user_device
                        break
            except ObjectDoesNotExist:
                raise ValidationError(_("Invalid device id"), code="invalid_device_id")

    def _chosen_device(self, user):
        if self._device_id_cache:
           return self._device_id_cache
        else:
           return self.initial_device

Note that the throttling_failure_count and throttling_failure_timestamp columns of the otp_static_staticdevice table need to be reset when the fix is applied.

@miniatureweasle I'd be curious to know if your problem goes away if you tried the same workaround?

Full investigation

Core finding at the end of this section.

AuthenticationTokenForm calls OTPAuthenticationFormMixin.clean_otp:

django-two-factor-auth/two_factor/forms.py

Lines 155 to 157 in 830915a

	def clean(self):
	self.clean_otp(self.user)
	return self.cleaned_data

clean_otp in django-otp looks like this (https://github.com/django-otp/django-otp/blob/3cff74c25ca926d6d6793a17004a406e79650dbb/src/django_otp/forms.py#L71-L80):

    def clean_otp(self, user):
        """
        Processes the ``otp_*`` fields.
        :param user: A user that has been authenticated by the first factor
            (such as a password).
        :type user: :class:`~django.contrib.auth.models.User`
        :raises: :exc:`~django.core.exceptions.ValidationError` if the user is
            not fully authenticated by an OTP token.
        """
        if user is None:
            return

        validation_error = None
        with transaction.atomic():
            try:
                device = self._chosen_device(user)
                token = self.cleaned_data.get('otp_token')

                user.otp_device = None

                try:
                    if self.cleaned_data.get('otp_challenge'):
                        self._handle_challenge(device)
                    elif token:
                        user.otp_device = self._verify_token(user, token, device)
                    else:
                        raise forms.ValidationError(self.otp_error_messages['token_required'], code='token_required')
                finally:
                    if user.otp_device is None:
                        self._update_form(user)
            except forms.ValidationError as e:
                # Validation errors shouldn't abort the transaction, so we have
                # to carefully transport them out.
                validation_error = e

        if validation_error:
            raise validation_error

Note that self._chosen_device(user) returns None because django-two-factor-auth's AuthenticationTokenForm does not define a field otp_device

And _verify_token looks like this (https://github.com/django-otp/django-otp/blob/3cff74c25ca926d6d6793a17004a406e79650dbb/src/django_otp/forms.py#L142):

    def _verify_token(self, user, token, device=None):
        if device is not None:
            (...) # omitted as device is None 
        else:
            device = match_token(user, token)

        if device is None:
            raise forms.ValidationError(self.otp_error_messages['invalid_token'], code='invalid_token')

        return device

And django_otp.match_token (https://github.com/django-otp/django-otp/blob/3cff74c25ca926d6d6793a17004a406e79650dbb/src/django_otp/__init__.py#L73)

def match_token(user, token):
    """
    Attempts to verify a :term:`token` on every device attached to the given
    user until one of them succeeds. When possible, you should prefer to verify
    tokens against specific devices.
    :param user: The user supplying the token.
    :type user: :class:`~django.contrib.auth.models.User`
    :param str token: An OTP token to verify.
    :returns: The device that accepted ``token``, if any.
    :rtype: :class:`~django_otp.models.Device` or ``None``
    """
    with transaction.atomic():
        for device in devices_for_user(user, for_verify=True):
            if device.verify_token(token):
                break
        else:
            device = None
    return device

And finally StaticDevice.verify_token (https://github.com/django-otp/django-otp/blob/3cff74c25ca926d6d6793a17004a406e79650dbb/src/django_otp/plugins/otp_static/models.py#L30)

    def verify_token(self, token):
        verify_allowed, _ = self.verify_is_allowed()
        if verify_allowed:
            match = self.token_set.filter(token=token).first()
            if match is not None:
                match.delete()
                self.throttle_reset()
            else:
                self.throttle_increment()
        else:
            match = None

        return (match is not None)

Which can call throttle_increment every time an incorrect token is used.

In this case the token is valid for the TOTP device but not for the StaticDevice (backup) token. If devices_for_user() yields the backup device before the TOTP device then a valid TOTP token will cause the StaticDevice.verify_token to call self.throttle_increment() and at some point the user will be locked out of their backup tokens.

Note 1: that self.verify_is_allowed() returns a hard-coded (True, None).
Note 2: The other Device classes (TOTP, Static, HOTP and email) also call self.throttle_increment().

[PetrDlouhy]

@Gautier I have tried your commit, and it seems to be working. Should I make an PR, or will you do it?

Also I consider the original error message to be very confusing not only for users but also for developers.
Couldn't we even add info when "soon" will be?

[claudep]

@Gautier I have tried your commit, and it seems to be working. Should I make an PR, or will you do it?

PR is #521

[Gautier]

@PetrDlouhy The PR is moving slowly, but moving :) I raised it a while ago, @moggers87 left comments 2 weeks ago and I've just replied to the feedback.

[PetrDlouhy]

Upon some investigation it seems that this issue has been resolved by commit 8deb380 although the fix works differently than the code in #521.

I the original test is passing and I also tried to add one more test which was not passing in 1.13.2, but is passing now.
I will try to get those tests merged, to ensure this is fixed.

There is still slight chance that this is still not fixed in some special case, but the procedure described under original issue should work.
@miniatureweasle @Gautier Can you please test the problem on 1.15.5 and try if there is no other way the throttling is wrongly affected?

[PetrDlouhy]

I am closing this. Please reopen this or new issue if there are still cases where the throttling is incorrectly increased.

[Gautier]

@PetrDlouhy perfect, appreciated that you took the time to deal with this issue. I don't have a good test scenario apart from the unit test in the PR #521

[PetrDlouhy]

@Gautier Your test is now part of the main branch, so I hope this issue will not occur ever again.