Only validate token against chosen device (#473) #521
Conversation
…g the user in.
Gautier
changed the title
Gautier
force-pushed
the
master
branch
2 times, most recently
from
954522d to
08daca8
Compare
| except ObjectDoesNotExist: | ||
| raise forms.ValidationError(self.error_messages['invalid_device_id']) | ||
|
|
||
| def _chosen_device(self, user): |
There was a problem hiding this comment.
This method doesn't appear to be called by anything, what's it for?
There was a problem hiding this comment.
_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".
two_factor/forms.py
Outdated
| def clean_device_id(self): | ||
| if self.data.get("device_id"): | ||
| try: | ||
| for user_device in devices_for_user(self.user): |
There was a problem hiding this comment.
Given that we know the device ID, couldn't we just fetch it directly rather than iterating over all devices like this?
There was a problem hiding this comment.
Good point, I have updated the PR to use Device.from_persistent_id instead.
| 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. |
There was a problem hiding this comment.
Not sure I understand this new comment
There was a problem hiding this comment.
I have updated the comment to include the crucial missing word "or" in "default device or a backup device"
|
Any updates on this? We ran into this issue as well. Would be great to have a fix on the main package. |
|
@moggers87 @Gautier What is state of this PR? Could I help somehow? |
|
Rebasing looks like the next step, then @PetrDlouhy, your review would be welcome, too! |
|
I have tried to rebase this in #683, but I am not able to resolve the review comments there without much deeper investigation. @Gautier Would you be able to look at it? I will try to give it more time, but it would save me some nerves if you could look at it. |
|
I did investigate it a bit more and I realized that in the new version of |
|
The relevant test from this PR is merged now and is passing because of other fixes in the current branch. Other changes included in this PR are probably not needed and if they are, they would need a clear test case first. |
Description
Changes the AuthenticationTokenForm to only validate a token against the chosen device.
The first version of this PR only includes a failing test to demonstrate the problem and the second version is a suggestion on how to fix it.
Motivation and Context
I received the same error report as #473 for one of my system using django-two-factor-auth and noticed the backup tokens
throttling_failure_countwere being incremented even though the phone TOTP worked.Types of changes
Checklist: