Recording exceeded limits #212

liamrabetian · 2020-11-05T19:42:22Z

Hi @jsocol . I've been busy doing all these for this issue #210 .

So I've made a proxy and two handlers, one for cache and another for database backend as you said "I can definitely also see wanting to know which users had historically exceeded the limits, not just current."
You can make as many as handlers you want, if there's any!

This functionality is completely separated from the core to not produce any security flaws.

Tests for the new functionality has been added and all the tests pass.

An abstract doc has been added for the settings. I wasn't sure if there's more explaining needed on the doc as it's simple and straight forward, but if needed, let me know.

I also did some re-structuring to the tests module because the test file was getting a Mega file.

Providing signal and app configure to allow configuration of app witout having to modify its code. Also to provide reasonable defaults that users can override.

Attempt records

jsocol · 2020-11-12T22:39:34Z

Hi, sorry for not responding sooner—I wanted to say this looks like a really good direction! I gave it a read and there are probably some nuances in implementation, like avoiding accidentally running migrations for users who don't want to opt into this behavior.

It'll probably take me a couple of more weeks to do a good, thorough dive, just to set expectations. But I did see this and will follow-up and pursue it.

One global request, if you're willing: this project has preferred single quotes, and small as it might seem I'd rather that be consistent (except in cases where the quoted string contains a single quote, e.g. "'" > '\''). I noticed some places that convention was changed, and the new code generally uses double quotes.

Andrew-Chen-Wang

Just added some of my thoughts and performance recommendations (though I think this is pretty out of date now):

Andrew-Chen-Wang · 2021-05-06T17:51:42Z

ratelimit/migrations/0001_initial.py

+        migrations.CreateModel(
+            name='ExceededLimitRecord',
+            fields=[
+                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),


Note in Django 3.2 that the default is BigAutoField.

Suggested change

('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),

('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),

Andrew-Chen-Wang · 2021-05-06T17:52:22Z

ratelimit/migrations/0001_initial.py

+            options={
+                'verbose_name': 'exceeded limit record',
+                'verbose_name_plural': 'exceeded limit records',
+            },


I'd use a linter if possible since there's this weird mix of single quote vs double quotes. But I'm just nit picking.

The convention is to not touch the migration files! As we have consistent code signature everywhere else.

Andrew-Chen-Wang · 2021-05-06T17:52:46Z

ratelimit/migrations/0002_exceededlimitrecord_last_blocked_at.py

+        migrations.AddField(
+            model_name="exceededlimitrecord",
+            name="last_blocked_at",
+            field=models.DateTimeField(auto_now=True, verbose_name="Last Blocked At"),


Shouldn't this be a part of the first migration?

Andrew-Chen-Wang · 2021-05-06T17:53:47Z

ratelimit/models.py

+from django.utils.translation import gettext_lazy as _
+
+
+class BaseModel(models.Model):


I'd rename this to:

Suggested change

class BaseModel(models.Model):

class BaseRatelimitLog(models.Model):

this would also mean you need to regenerate the migration file

Andrew-Chen-Wang · 2021-05-06T18:00:24Z

ratelimit/record_handlers/database.py

+            limit_record = ExceededLimitRecord.objects.get(
+                user_agent=user_agent,
+                ip_address=client_ip_address,
+                username=username,
+                path_info=path_info,
+            )
+            limit_record.access_attempt_failures += 1
+            limit_record.save()


This can be made more efficient with F expressions and only getting the ID so we can update the record via the ID but not have to get all the values from the record (without the only).

Suggested change

limit_record = ExceededLimitRecord.objects.get(

user_agent=user_agent,

ip_address=client_ip_address,

username=username,

path_info=path_info,

)

limit_record.access_attempt_failures += 1

limit_record.save()

limit_record = ExceededLimitRecord.objects.get(

user_agent=user_agent,

ip_address=client_ip_address,

username=username,

path_info=path_info,

).only("id")

limit_record.access_attempt_failures = F("access_attempt_failures") + 1

limit_record.save()

The "only" should be checked in a test. Test case can look like:

with self.assertNumQueries(2): limit_record = ExceededLimitRecord.objects.get( user_agent=user_agent, ip_address=client_ip_address, username=username, path_info=path_info, ).only("id") limit_record.access_attempt_failures = F("access_attempt_failures") + 1 limit_record.save()

Good suggestion!

Andrew-Chen-Wang · 2021-05-06T18:04:06Z

ratelimit/record_handlers/helpers.py

+from ratelimit.conf import settings
+
+
+def get_client_ip_address(request) -> str:


There's already a tool that does that so that we don't need to install an external package (i.e. ipware):

django-ratelimit/django_ratelimit/core.py

Lines 29 to 59 in 651c730

def _get_ip(request):

ip_meta = getattr(settings, 'RATELIMIT_IP_META_KEY', None)

if not ip_meta:

ip = request.META['REMOTE_ADDR']

if not ip:

raise ImproperlyConfigured(

'IP address in REMOTE_ADDR is empty. This can happen when '

'using a reverse proxy and connecting to the app server with '

'Unix sockets. See the documentation for '

'RATELIMIT_IP_META_KEY: https://bit.ly/3iIpy2x')

elif callable(ip_meta):

ip = ip_meta(request)

elif isinstance(ip_meta, str) and '.' in ip_meta:

ip_meta_fn = import_string(ip_meta)

ip = ip_meta_fn(request)

elif ip_meta in request.META:

ip = request.META[ip_meta]

else:

raise ImproperlyConfigured(

'Could not get IP address from "%s"' % ip_meta)

if ':' in ip:

# IPv6

mask = getattr(settings, 'RATELIMIT_IPV6_MASK', 64)

else:

# IPv4

mask = getattr(settings, 'RATELIMIT_IPV4_MASK', 32)

network = ipaddress.ip_network('{}/{}'.format(ip, mask), strict=False)

return str(network.network_address)

Andrew-Chen-Wang · 2021-05-06T18:05:58Z

ratelimit/record_handlers/helpers.py

+def get_client_username(request) -> str:
+    if request.user.is_authenticated:
+        return request.user.username
+    return "Anonymous"


Not everyone has a "username" field. You might want to look into django.contrib.auth.models where they define the Username field inside a list (specifically AbstractUser or AbstractBaseUser). I also think this should instead be based on user ID/pk instead of username to be more efficient (string lookup vs integer lookup).

If you're going to stick with username, then try this (but I'd still recommend using the integer pk AutoField):

Suggested change

def get_client_username(request) -> str:

if request.user.is_authenticated:

return request.user.username

return "Anonymous"

from django.contrib.auth.models import AnonymousUser

def get_client_username(request) -> str:

if request.user.is_authenticated:

return getattr(request.user, request.user.USERNAME_FIELD)

return AnonymousUser

Andrew-Chen-Wang · 2021-05-06T18:09:10Z

ratelimit/record_handlers/helpers.py

+def make_cache_key(key_components):
+
+    cache_key_components = "".join(value for value in key_components.values() if value)
+    cache_key_digest = md5(cache_key_components.encode()).hexdigest()
+    cache_key = "drl-{}".format(cache_key_digest)
+
+    return cache_key


isn't this in django_ratelimit.core?

jsocol · 2021-05-06T18:46:25Z

This probably needs a bit of rework with the conflicts that have cropped up from some other refactors.

One thing I want to avoid is baking too much optional behavior into the "core." I wonder if we could tuck some of this into something like ratelimit.ext.recorder with its own AppConfig, so that installation requires explicit opt-in like INSTALLED_APPS += ['ratelimit.ext.recorder'] and we don't end up installing unused models or causing inadvertent migrations.

Andrew-Chen-Wang · 2021-05-06T19:00:17Z

Yes definitely doable. I think all you gotta do is just move the dirs and it should be fine (ref the token_blacklist app from here: https://github.com/jazzband/django-rest-framework-simplejwt/tree/master/rest_framework_simplejwt where the rest of the dir is not affiliated with any app)

Update ratelimit with latest changes from the fork

liamrabetian · 2021-06-21T13:40:37Z

This probably needs a bit of rework with the conflicts that have cropped up from some other refactors.

One thing I want to avoid is baking too much optional behavior into the "core." I wonder if we could tuck some of this into something like ratelimit.ext.recorder with its own AppConfig, so that installation requires explicit opt-in like INSTALLED_APPS += ['ratelimit.ext.recorder'] and we don't end up installing unused models or causing inadvertent migrations.

I have updated the PR and fixed the conflicts. Sadly it's hard for me to find some spare time to work on this more but, I will make some more adjustments. I appreciate any help 👍

…-update Efficient access attempts update

mohammadrabetian added 22 commits November 3, 2020 22:03

Models for storing exceeded limits in DB

abebea9

Base and Proxy classes to handle different record designs

4eff2e6

Database handler for recording exceeded limits

b960416

Cache handler for recording exceeded limits in cache

6e19b9f

Model migrations

0e99373

Seperation of concerns

9e81281

Admin panel for exceeded limit records

c487fd8

Using django-ipware for address resolution

f25542b

Using proxy for recording limits in decorator

95eb117

Provide fieldsets for admin panel

19a0c86

Fix get_client_cache_key arguments

288a402

Setting config for tests

244ce0c

Re-structure tests

05798ba

Test admin panel

49f9a28

Migration test

1888236

Test helpers

4c96780

Testing proxy implementation.

f3aed44

Providing signal and app configure to allow configuration of app witout having to modify its code. Also to provide reasonable defaults that users can override.

Test database record handler

11a0b58

Docs for ratelimit record

2431e34

Fix installed apps, remove redundant

808cd0f

Merge pull request #1 from mohammadrabetian/attempt-records

78ff799

Attempt records

Substitude (path, f-string) with (urls, format)

cc0de8b

liamrabetian force-pushed the main branch from f746985 to cc0de8b Compare November 6, 2020 10:57

Andrew-Chen-Wang suggested changes May 6, 2021

View reviewed changes

Eike Rabetian added 3 commits June 20, 2021 19:34

Merge branch 'main' into update-ratelimit

f060d09

Merge pull request #3 from mohammadrabetian/update-ratelimit

c2306c4

Update ratelimit with latest changes from the fork

Efficient update for access attempts

0818618

Test for more efficient access attempt update

9f4c499

Eike Rabetian added 2 commits June 21, 2021 18:15

Merge pull request #4 from mohammadrabetian/efficient-access-attempts…

d39f52c

…-update Efficient access attempts update

Update README.rst

0b8a757

liamrabetian closed this Jul 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Recording exceeded limits #212

Recording exceeded limits #212

liamrabetian commented Nov 5, 2020 •

edited

Loading

jsocol commented Nov 12, 2020

Andrew-Chen-Wang left a comment

Andrew-Chen-Wang May 6, 2021

Andrew-Chen-Wang May 6, 2021

liamrabetian Jun 21, 2021

Andrew-Chen-Wang May 6, 2021

Andrew-Chen-Wang May 6, 2021

Andrew-Chen-Wang May 6, 2021

Andrew-Chen-Wang May 6, 2021

liamrabetian Jun 21, 2021

Andrew-Chen-Wang May 6, 2021

Andrew-Chen-Wang May 6, 2021

Andrew-Chen-Wang May 6, 2021

jsocol commented May 6, 2021

Andrew-Chen-Wang commented May 6, 2021

liamrabetian commented Jun 21, 2021

	('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
	('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),

		from django.utils.translation import gettext_lazy as _


		class BaseModel(models.Model):

	class BaseModel(models.Model):
	class BaseRatelimitLog(models.Model):

		from ratelimit.conf import settings


		def get_client_ip_address(request) -> str:

	def _get_ip(request):
	ip_meta = getattr(settings, 'RATELIMIT_IP_META_KEY', None)
	if not ip_meta:
	ip = request.META['REMOTE_ADDR']
	if not ip:
	raise ImproperlyConfigured(
	'IP address in REMOTE_ADDR is empty. This can happen when '
	'using a reverse proxy and connecting to the app server with '
	'Unix sockets. See the documentation for '
	'RATELIMIT_IP_META_KEY: https://bit.ly/3iIpy2x')
	elif callable(ip_meta):
	ip = ip_meta(request)
	elif isinstance(ip_meta, str) and '.' in ip_meta:
	ip_meta_fn = import_string(ip_meta)
	ip = ip_meta_fn(request)
	elif ip_meta in request.META:
	ip = request.META[ip_meta]
	else:
	raise ImproperlyConfigured(
	'Could not get IP address from "%s"' % ip_meta)

	if ':' in ip:
	# IPv6
	mask = getattr(settings, 'RATELIMIT_IPV6_MASK', 64)
	else:
	# IPv4
	mask = getattr(settings, 'RATELIMIT_IPV4_MASK', 32)

	network = ipaddress.ip_network('{}/{}'.format(ip, mask), strict=False)

	return str(network.network_address)

Recording exceeded limits #212

Recording exceeded limits #212

Conversation

liamrabetian commented Nov 5, 2020 • edited Loading

jsocol commented Nov 12, 2020

Andrew-Chen-Wang left a 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

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

jsocol commented May 6, 2021

Andrew-Chen-Wang commented May 6, 2021

liamrabetian commented Jun 21, 2021

liamrabetian commented Nov 5, 2020 •

edited

Loading