Only validate token against chosen device (#473)

jazzband / django-two-factor-auth

Complete Two-Factor Authentication for Django providing the easiest integration into most Django projects.

MIT License

1.71k stars 448 forks source link

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

Closed Gautier closed 10 months ago

Gautier commented 2 years ago

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_count were being incremented even though the phone TOTP worked.

Types of changes

[X] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[X] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

[X] My code follows the code style of this project. (no black?)
[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[X] I have added tests to cover my changes.
[X] All new and existing tests passed.

milafrerichs commented 1 year ago

Any updates on this? We ran into this issue as well. Would be great to have a fix on the main package. Can I help in any way?

PetrDlouhy commented 11 months ago

@moggers87 @Gautier What is state of this PR? Could I help somehow?

claudep commented 11 months ago

Rebasing looks like the next step, then @PetrDlouhy, your review would be welcome, too!

PetrDlouhy commented 11 months ago

@claudep I have rebased this in PR #683, all tests are passing now.

PetrDlouhy commented 10 months ago

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 am a bit confused, because I tried to run the tests.test_views_login.LoginTest.test_totp_token_does_not_impact_backup_token test with the unfixed forms.py code and it seems to pass.

I will try to give it more time, but it would save me some nerves if you could look at it.

PetrDlouhy commented 10 months ago

I did investigate it a bit more and I realized that in the new version of django-two-factor-auth there is AuthenticationTokenForm._chosen_device() newly implemented. I am not yet sure if that is enough to fix #473 or there is some case in which it could still fail.

PetrDlouhy commented 10 months ago

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. I am closing this, open new PR in such cases.