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
Added throttling to token generation (Closes #48) #122
Added throttling to token generation (Closes #48) #122
Conversation
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.
Looks like a good start.
If the interaction between GenerationThrottlingMixin
and SideChannelDevice
gets complicated, it might worth considering whether singledispatch could be used to cut through some of the MRO subtleties.
Thank you for the quick response and feedback. I will respond to each comment in their respective dialog.
Let's see how it turns out with the Mixin, and we reconsider. |
src/django_otp/models.py
Outdated
Update the timestamp of the last token generation to the current time. | ||
""" | ||
self.last_generated_timestamp = timezone.now() | ||
super().generate_token(*args, **kwargs) |
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.
Is generate_token
guaranteed to exist on all devices? This appears to be a secret dependency on SideChannelDevice
.
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.
This should be resolved now.
I'll take another look at this once it includes documentation and test coverage. |
I will re-request a review once I have something ready. |
afffcc3
to
bb10746
Compare
Had to rebase to resolve conflicts with |
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.
Structurally, this is looking pretty solid. I added a few suggestions for fixes and cleanup. And of course there's still the documentation.
Remember to run hatch run check
(and, if necessary, hatch run fix
) before checking in.
src/django_otp/models.py
Outdated
else: | ||
return False, { | ||
'reason': "COOLDOWN_DURATION_PENDING", | ||
'next_generation_at': dt_now |
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.
Should dt_now
be self.last_generated_timestamp
here?
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.
Indeed! I have corrected it (dbc3fb5) and added a test for the message as well.
src/django_otp/tests.py
Outdated
@@ -164,6 +165,45 @@ def test_verify_is_allowed(self): | |||
self.assertEqual(data3, None) | |||
|
|||
|
|||
class GenerationThrottlingTestMixin: |
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.
It looks like this is based on ThrottlingTestMixin
, which makes sense. However, it's asserting on len(mail.outbox)
, which is email-specific. This will presumably need at least one abstract method to actually be generic.
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 agree. I have added generic tests and moved email specific tests to the email test file (29ebe3e).
src/django_otp/tests.py
Outdated
# Assert that no email is sent | ||
self.assertEqual(len(mail.outbox), 1) | ||
|
||
with freeze_time() as frozen_time: |
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.
This test should work in practice as-is, but would it make sense to use this context for the entire method body? The point is to control time for the whole sequence of operations.
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.
Yes, it makes perfect sense. I have corrected this in all tests (29ebe3e).
context | ||
generate_allowed, data_dict = self.generate_is_allowed() | ||
if generate_allowed: | ||
self.generate_token(valid_secs=settings.OTP_EMAIL_TOKEN_VALIDITY) |
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.
This method is now doing two big things. Perhaps it's time to move the body of if generate_allowed:
into a private method for clarity. See https://github.com/django-otp/django-otp-twilio/blob/9e1aab519c926a3d281292d549ce2040c821fe1a/src/otp_twilio/models.py#L55, e.g.
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.
Created a private method for that part (dbe61d7).
Yes, sorry about that, will do.
Thank you for the feedback. I believe I have addressed all open issues and commented on relevant conversations above.
I used Please note
|
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.
Almost there, I think. There's a critical bug in the email integration making it a noop. Also, liberal use of self.device.refresh_from_db()
or equivalent in the tests will allow them to catch these sorts of bugs.
self.generate_token(valid_secs=settings.OTP_EMAIL_TOKEN_VALIDITY) | ||
self.last_generated_timestamp = timezone.now() |
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.
This never gets saved. As importantly, the tests aren't catching this (see companion note in the tests).
(Hint: try swapping these two lines. Adding the technically redundant commit=True
will help to highlight the fact that you're relying on generate_token
to save the model instance for you.)
Indeed, I missed this. I have added
I have updated the tests. |
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.
Looks like it's ready. Thanks for all the work.
I am glad we got this through! Thank you for your valuable feedback and patience. @psagers I know this is merged now, but it occurred to me that maybe the default value for |
I considered that, although |
As the #48 issue describes, it is beneficial to optionally throttle the token generation, to prevent accidental or malicious abuse of the token delivery medium.
The current state of this pull request is functional but not fully tested. If the maintainer sees some value to this, I could use some feedback to finalize this.