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

Only validate token against chosen device (#473) #521

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
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
28 changes: 25 additions & 3 deletions tests/test_views_login.py
Original file line number Diff line number Diff line change
Expand Up @@ -258,8 +258,8 @@ def test_throttle_with_generator(self, mock_signal):
response = self._post({'token-otp_token': totp_str(device.bin_key),
'login_view-current_step': 'token'})
self.assertEqual(response.context_data['wizard']['form'].errors,
{'__all__': ['Invalid token. Please make sure you '
'have entered it correctly.']})
{'__all__': ['Verification temporarily disabled because '
'of 1 failed attempt, please try again soon.']})

@mock.patch('two_factor.gateways.fake.Fake')
@mock.patch('two_factor.views.core.signals.user_verified.send')
Expand Down Expand Up @@ -317,7 +317,8 @@ def test_with_backup_phone(self, mock_signal, fake):

# Valid token should be accepted.
response = self._post({'token-otp_token': totp_str(device.bin_key),
'login_view-current_step': 'token'})
'login_view-current_step': 'token',
'device_id': device.persistent_id})
self.assertRedirects(response, resolve_url(settings.LOGIN_REDIRECT_URL))
self.assertEqual(device.persistent_id,
self.client.session.get(DEVICE_ID_SESSION_KEY))
Expand Down Expand Up @@ -359,6 +360,27 @@ def test_with_backup_token(self, mock_signal):
# Check that the signal was fired.
mock_signal.assert_called_with(sender=mock.ANY, request=mock.ANY, user=user, device=device)

def test_totp_token_does_not_impact_backup_token(self):
user = self.create_user()
backup_device = user.staticdevice_set.create(name='backup')
backup_device.token_set.create(token='abcdef123')
totp_device = user.totpdevice_set.create(name='default', key=random_hex())

response = self._post({'auth-username': 'bouke@example.com',
'auth-password': 'secret',
'login_view-current_step': 'auth'})
self.assertContains(response, 'Token:')

backup_device.refresh_from_db()
self.assertEqual(backup_device.throttling_failure_count, 0)
response = self._post({'token-otp_token': totp_str(totp_device.bin_key),
'login_view-current_step': 'token'})
self.assertRedirects(response, resolve_url(settings.LOGIN_REDIRECT_URL))
self.assertEqual(self.client.session['_auth_user_id'], str(user.pk))

backup_device.refresh_from_db()
self.assertEqual(backup_device.throttling_failure_count, 0)

@mock.patch('two_factor.views.utils.logger')
def test_reset_wizard_state(self, mock_logger):
self.create_user()
Expand Down
31 changes: 27 additions & 4 deletions two_factor/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@

from django import forms
from django.conf import settings
from django.core.exceptions import ObjectDoesNotExist
from django.utils.translation import gettext_lazy as _
from django_otp import devices_for_user
from django_otp.models import Device
from django_otp.forms import OTPAuthenticationFormMixin
from django_otp.oath import totp
from django_otp.plugins.otp_totp.models import TOTPDevice
Expand Down Expand Up @@ -113,6 +116,7 @@ class AuthenticationTokenForm(OTPAuthenticationFormMixin, forms.Form):
'pattern': '[0-9]*', # hint to show numeric keyboard for on-screen keyboards
'autocomplete': 'one-time-code',
})
device_id = forms.CharField(widget=forms.HiddenInput(), required=False)

# Our authentication form has an additional submit button to go to the
# backup token form. When the `required` attribute is set on an input
Expand All @@ -123,16 +127,21 @@ class AuthenticationTokenForm(OTPAuthenticationFormMixin, forms.Form):
use_required_attribute = False
idempotent = False

error_messages = {
'invalid_device_id': _('Device id is not valid.'),
}

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.
`initial_device` is either the user's default device or a backup device
when the user chooses to enter a backup token.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure I understand this new comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the comment to include the crucial missing word "or" in "default device or a backup device"

"""
super().__init__(**kwargs)
self.user = user
self.initial_device = initial_device
if initial_device:
self.fields['device_id'].initial = initial_device.persistent_id
self.device_cache = None

# Add a field to remeber this browser.
if getattr(settings, 'TWO_FACTOR_REMEMBER_COOKIE_AGE', None):
Expand All @@ -152,6 +161,20 @@ def __init__(self, user, initial_device, **kwargs):
label=label
)

def clean_device_id(self):
if self.data.get("device_id"):
device = Device.from_persistent_id(self.data["device_id"])
if device and device.user == self.user:
self.device_cache = device
else:
raise forms.ValidationError(self.error_messages['invalid_device_id'])

def _chosen_device(self, user):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This method doesn't appear to be called by anything, what's it for?

Copy link
Contributor Author

@Gautier Gautier Sep 29, 2022

Choose a reason for hiding this comment

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

_chosen_device is the method called by the OTPTokenForm._clean_otp method in django-otp.

It currently returns None which makes django-otp try to match the token against each user's devices in order until one works (in match_token) but since match_token uses devices_for_token which iterates over the devices that way for model in device_classes(): there is no guarantee that it won't try the backup devices first.

I have added more details in my comment at the related issue at : #473 (comment) under the heading "Full investigation".

if self.device_cache:
return self.device_cache
else:
return self.initial_device

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