Only validate token against chosen device 2 (#473) #683
Conversation
|
The tests are not passing because of #684 |
PetrDlouhy
force-pushed
the
validate_token
branch
from
0a2af18 to
c6de35f
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #683 +/- ##
==========================================
+ Coverage 95.52% 95.57% +0.04%
==========================================
Files 78 78
Lines 3354 3389 +35
Branches 377 377
==========================================
+ Hits 3204 3239 +35
Misses 119 119
Partials 31 31 ☔ View full report in Codecov by Sentry. |
PetrDlouhy
force-pushed
the
validate_token
branch
2 times, most recently
from
2c54cf9 to
ffa1633
Compare
two_factor/forms.py
Outdated
| @@ -115,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) | |||
There was a problem hiding this comment.
I don't see why we need this device_id hidden field, if the user has no way to change it anyway. Couldn't we return initial_device from _chosen_device?
two_factor/forms.py
Outdated
| if self.device_cache: | ||
| return self.device_cache | ||
| else: | ||
| return self.initial_device |
There was a problem hiding this comment.
If we skip calling the parent method, we should at least redo the security check done in django_otp parent method.
PetrDlouhy
force-pushed
the
validate_token
branch
2 times, most recently
from
ce9496a to
6b65936
Compare
…g the user in.
PetrDlouhy
force-pushed
the
validate_token
branch
from
6b65936 to
b805807
Compare
|
@claudep Thank you for your comments. I had to investigate this much deeper and I found that the issue #473 might be already resolved by 8deb380 The test from the original PR is passing in current master and I added one more test to ensure that the throttling will work correctly in the future. Would you be OK with merging just those tests? |
|
Absolutely, and thanks a ton for these investigations 😍 |
This is rebase of #521 to the current master branch.
Please follow the description at #521