Skip to content

Commit

Permalink
Only validate token against chosen device (jazzband#473)
Browse files Browse the repository at this point in the history
  • Loading branch information
Gautier authored and PetrDlouhy committed Dec 18, 2023
1 parent f9f243d commit b1b0cc4
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 10 deletions.
4 changes: 2 additions & 2 deletions tests/test_views_login.py
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,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 @@ -408,7 +409,6 @@ def test_with_backup_token(self, mock_signal):

def test_totp_token_does_not_impact_backup_token(self):
user = self.create_user()
user.totpdevice_set.create(name='default', key=random_hex())
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())
Expand Down
34 changes: 26 additions & 8 deletions two_factor/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@

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.forms import OTPAuthenticationFormMixin
from django_otp.oath import totp
from django_otp.plugins.otp_totp.models import TOTPDevice
Expand Down Expand Up @@ -115,6 +117,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 @@ -125,16 +128,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 a backup device
when the user chooses to enter a backup token.
"""
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 remember this browser.
if getattr(settings, 'TWO_FACTOR_REMEMBER_COOKIE_AGE', None):
Expand All @@ -154,11 +162,21 @@ def __init__(self, user, initial_device, **kwargs):
label=label
)

def clean_device_id(self):
if self.data.get("device_id"):
try:
for user_device in devices_for_user(self.user):
if user_device.persistent_id == self.data["device_id"]:
self.device_cache = user_device
break
except ObjectDoesNotExist:
raise forms.ValidationError(self.error_messages['invalid_device_id'])

def _chosen_device(self, user):
device = super()._chosen_device(user)
if not device:
device = self.initial_device
return device
if self.device_cache:
return self.device_cache
else:
return self.initial_device

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

0 comments on commit b1b0cc4

Please sign in to comment.