-
-
Notifications
You must be signed in to change notification settings - Fork 448
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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 | ||
|
@@ -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. | ||
""" | ||
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): | ||
|
@@ -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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It currently returns None which makes django-otp try to match the token against each user's devices in order until one works (in 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 | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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"