Added throttling to token generation (Closes #48) #122

demestav · 2023-05-08T08:54:24Z

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.

psagers

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.

src/django_otp/models.py

src/django_otp/forms.py

src/django_otp/models.py

demestav · 2023-05-09T06:43:29Z

Thank you for the quick response and feedback. I will respond to each comment in their respective dialog.

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.

Let's see how it turns out with the Mixin, and we reconsider.

psagers · 2023-05-20T17:04:10Z

src/django_otp/models.py

+        Update the timestamp of the last token generation to the current time.
+        """
+        self.last_generated_timestamp = timezone.now()
+        super().generate_token(*args, **kwargs)


Is generate_token guaranteed to exist on all devices? This appears to be a secret dependency on SideChannelDevice.

This should be resolved now.

psagers · 2023-05-20T17:06:30Z

I'll take another look at this once it includes documentation and test coverage.

demestav · 2023-05-21T07:10:44Z

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.

demestav · 2023-10-18T08:34:07Z

Had to rebase to resolve conflicts with master. Re-requesting review. @psagers Are you interested in proceeding with this?

psagers

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.

psagers · 2023-10-19T16:05:33Z

src/django_otp/models.py

+        else:
+            return False, {
+                'reason': "COOLDOWN_DURATION_PENDING",
+                'next_generation_at': dt_now


Should dt_now be self.last_generated_timestamp here?

Indeed! I have corrected it (dbc3fb5) and added a test for the message as well.

psagers · 2023-10-19T16:13:30Z

src/django_otp/tests.py

@@ -164,6 +165,45 @@ def test_verify_is_allowed(self):
            self.assertEqual(data3, None)


+class GenerationThrottlingTestMixin:


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.

I agree. I have added generic tests and moved email specific tests to the email test file (29ebe3e).

psagers · 2023-10-19T16:15:03Z

src/django_otp/tests.py

+            # Assert that no email is sent
+            self.assertEqual(len(mail.outbox), 1)
+
+        with freeze_time() as frozen_time:


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.

Yes, it makes perfect sense. I have corrected this in all tests (29ebe3e).

psagers · 2023-10-19T16:19:38Z

src/django_otp/plugins/otp_email/models.py

-                context
+        generate_allowed, data_dict = self.generate_is_allowed()
+        if generate_allowed:
+            self.generate_token(valid_secs=settings.OTP_EMAIL_TOKEN_VALIDITY)


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.

Created a private method for that part (dbe61d7).

demestav · 2023-10-30T07:38:37Z

Remember to run hatch run check (and, if necessary, hatch run fix) before checking in.

Yes, sorry about that, will do.

I added a few suggestions for fixes and cleanup.

Thank you for the feedback. I believe I have addressed all open issues and commented on relevant conversations above.

And of course there's still the documentation.

I used ThrottlingMixin as a guide and replicated for this Mixin. I was not sure whether generate_is_allowed should be listed in "members" in the "Helpers" sections.

Please note

I have renamed the feature as Cooldown to avoid reusing the word "Throttling" which is used in the ThrottlingMixin and to shorten the name a bit. I believe CooldownMixin is a more appropriate name for this feature.
I have set the default value for email cooldown to 60 seconds.

psagers

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.

psagers · 2023-11-02T20:30:37Z

src/django_otp/plugins/otp_email/models.py

        self.generate_token(valid_secs=settings.OTP_EMAIL_TOKEN_VALIDITY)
+        self.last_generated_timestamp = timezone.now()


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.)

src/django_otp/tests.py

demestav · 2023-11-08T06:41:31Z

There's a critical bug in the email integration

Indeed, I missed this. I have added cooldown_set method to the mixin to keep it consistent, and updated the email integration code.

liberal use of self.device.refresh_from_db() or equivalent in the tests will allow them to catch these sorts of bugs.

I have updated the tests.

psagers

Looks like it's ready. Thanks for all the work.

demestav · 2023-11-09T06:45:57Z

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 OTP_EMAIL_COOLDOWN_DURATION should have been 0 in order to disable the cooldown feature by default? The reason I am suggesting this, is that people that are already using the email plugin, and upgrade, and they are unware of this feature, they will unintentionally activate cooldown which might affect how their application works.

psagers · 2023-11-09T15:44:23Z

I considered that, although OTP_EMAIL_THROTTLE_FACTOR also has a default value, so it's consistent. 60 seconds isn't a very aggressive cooldown and I noted the new behavior in the changelog, so erring on the side of safety seems reasonable here.

psagers requested changes May 8, 2023

View reviewed changes

src/django_otp/models.py Outdated Show resolved Hide resolved

src/django_otp/models.py Outdated Show resolved Hide resolved

src/django_otp/forms.py Outdated Show resolved Hide resolved

src/django_otp/models.py Outdated Show resolved Hide resolved

psagers reviewed May 20, 2023

View reviewed changes

demestav requested a review from psagers August 15, 2023 20:24

demestav and others added 6 commits October 18, 2023 11:27

Added throttling to token generation (Closes django-otp#48)

8aa2230

Refactored code to suit better the current structure of codebase

4f20255

Added get_generation_interval method to GenerationThrottlingMixin

72291ef

Fixed import sort

b673da6

Fixed check and generation of token

b966e1d

Removed generate token, adapted EmailDevice, added tests

bb10746

demestav force-pushed the feature/48-add-token-generation-throttling branch from afffcc3 to bb10746 Compare October 18, 2023 08:32

psagers requested changes Oct 19, 2023

View reviewed changes

demestav and others added 6 commits October 22, 2023 12:01

Fixed remaining time message, and added a test

dbc3fb5

Decoupled generic cooldown tests with email specific cooldown tests

29ebe3e

Moved email token delivery process to a private method, for clarity

dbe61d7

Consistent naming of new feature as Cooldown

5b49259

Rename cooldown setting for email plugin

131a10e

Added documentation

87ba728

demestav requested a review from psagers October 30, 2023 07:39

psagers requested changes Nov 2, 2023

View reviewed changes

Commit timestamp update, and improved tests to validate that.

3746d8c

Updated documentation

f3ab24b

demestav requested a review from psagers November 8, 2023 06:46

psagers approved these changes Nov 8, 2023

View reviewed changes

psagers merged commit d6b37d7 into django-otp:master Nov 8, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added throttling to token generation (Closes #48) #122

Added throttling to token generation (Closes #48) #122

demestav commented May 8, 2023

psagers left a comment

demestav commented May 9, 2023 •

edited

psagers May 20, 2023

demestav Aug 15, 2023

psagers commented May 20, 2023

demestav commented May 21, 2023

demestav commented Oct 18, 2023 •

edited

psagers left a comment

psagers Oct 19, 2023

demestav Oct 22, 2023 •

edited

psagers Oct 19, 2023

demestav Oct 22, 2023

psagers Oct 19, 2023

demestav Oct 22, 2023

psagers Oct 19, 2023

demestav Oct 22, 2023

demestav commented Oct 30, 2023

psagers left a comment

psagers Nov 2, 2023

demestav commented Nov 8, 2023

psagers left a comment

demestav commented Nov 9, 2023

psagers commented Nov 9, 2023

		@@ -164,6 +165,45 @@ def test_verify_is_allowed(self):
		self.assertEqual(data3, None)


		class GenerationThrottlingTestMixin:

		self.generate_token(valid_secs=settings.OTP_EMAIL_TOKEN_VALIDITY)
		self.last_generated_timestamp = timezone.now()

Added throttling to token generation (Closes #48) #122

Added throttling to token generation (Closes #48) #122

Conversation

demestav commented May 8, 2023

psagers left a comment

Choose a reason for hiding this comment

demestav commented May 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psagers commented May 20, 2023

demestav commented May 21, 2023

demestav commented Oct 18, 2023 • edited

psagers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demestav Oct 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demestav commented Oct 30, 2023

psagers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demestav commented Nov 8, 2023

psagers left a comment

Choose a reason for hiding this comment

demestav commented Nov 9, 2023

psagers commented Nov 9, 2023

demestav commented May 9, 2023 •

edited

demestav commented Oct 18, 2023 •

edited

demestav Oct 22, 2023 •

edited