-
-
Notifications
You must be signed in to change notification settings - Fork 359
Added individual attempt expiry feature for #1302 #1313
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
Changes from all commits
5b2a483
21e1636
d5e4268
cdf911d
fc60c44
1a293ff
9efedb2
8cac081
dcdb83b
717ec51
5794c88
6378589
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,14 +7,25 @@ | |
|
||
|
||
class AccessAttemptAdmin(admin.ModelAdmin): | ||
list_display = ( | ||
"attempt_time", | ||
"ip_address", | ||
"user_agent", | ||
"username", | ||
"path_info", | ||
"failures_since_start", | ||
) | ||
if settings.AXES_USE_ATTEMPT_EXPIRATION: | ||
list_display = ( | ||
"attempt_time", | ||
"expiration", | ||
"ip_address", | ||
"user_agent", | ||
"username", | ||
"path_info", | ||
"failures_since_start", | ||
) | ||
else: | ||
list_display = ( | ||
"attempt_time", | ||
"ip_address", | ||
"user_agent", | ||
"username", | ||
"path_info", | ||
"failures_since_start", | ||
) | ||
|
||
list_filter = ["attempt_time", "path_info"] | ||
|
||
|
@@ -23,7 +34,7 @@ class AccessAttemptAdmin(admin.ModelAdmin): | |
date_hierarchy = "attempt_time" | ||
|
||
fieldsets = ( | ||
(None, {"fields": ("username", "path_info", "failures_since_start")}), | ||
(None, {"fields": ("username", "path_info", "failures_since_start", "expiration")}), | ||
(_("Form Data"), {"fields": ("get_data", "post_data")}), | ||
(_("Meta Data"), {"fields": ("user_agent", "ip_address", "http_accept")}), | ||
) | ||
|
@@ -38,11 +49,14 @@ class AccessAttemptAdmin(admin.ModelAdmin): | |
"get_data", | ||
"post_data", | ||
"failures_since_start", | ||
"expiration", | ||
] | ||
|
||
def has_add_permission(self, request: HttpRequest) -> bool: | ||
return False | ||
|
||
def expiration(self, obj: AccessAttempt): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering if this should be just part of the modeladmin as an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But i don't think we have any editable scope of this field at the moment. |
||
return obj.expiration.expires_at if hasattr(obj, "expiration") else _("Not set") | ||
|
||
class AccessLogAdmin(admin.ModelAdmin): | ||
list_display = ( | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -19,8 +19,9 @@ | |||||||||||||||||||||||||
get_failure_limit, | ||||||||||||||||||||||||||
get_lockout_parameters, | ||||||||||||||||||||||||||
get_query_str, | ||||||||||||||||||||||||||
get_attempt_expiration, | ||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||
from axes.models import AccessAttempt, AccessFailureLog, AccessLog | ||||||||||||||||||||||||||
from axes.models import AccessAttempt, AccessAttemptExpiration, AccessFailureLog, AccessLog | ||||||||||||||||||||||||||
from axes.signals import user_locked_out | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
log = getLogger(__name__) | ||||||||||||||||||||||||||
|
@@ -219,6 +220,21 @@ def user_login_failed(self, sender, credentials: dict, request=None, **kwargs): | |||||||||||||||||||||||||
client_str, | ||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
if settings.AXES_USE_ATTEMPT_EXPIRATION: | ||||||||||||||||||||||||||
if not hasattr(attempt, "expiration") or attempt.expiration is None: | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe just create the object if it doesn't exist? Is the expiration timestamp ever earlier than it previously was? Why should it only be updated to a later value if the
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it can be earlier as explaind in the initial communication of the issue also #1302 What if some kept cool_off 60 mins in first time and then changed to 1 min. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If someone changes the expiration then we can just use the new expiration value, I don't think there's much value added in adding dynamic logic for calculating maximum expiration values - if the user is specializing the expiration times they can calculate them as they see fit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||||||||||||||||||||||
log.debug( | ||||||||||||||||||||||||||
"AXES: Creating new AccessAttemptExpiration for %s", client_str | ||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||
attempt.expiration = AccessAttemptExpiration.objects.create( | ||||||||||||||||||||||||||
access_attempt=attempt, | ||||||||||||||||||||||||||
expires_at=get_attempt_expiration(request) | ||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||
else: | ||||||||||||||||||||||||||
attempt.expiration.expires_at = max( | ||||||||||||||||||||||||||
get_attempt_expiration(request), attempt.expiration.expires_at | ||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||
attempt.expiration.save() | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
# 3. or 4. database query: Calculate the current maximum failure number from the existing attempts | ||||||||||||||||||||||||||
failures_since_start = self.get_failures(request, credentials) | ||||||||||||||||||||||||||
request.axes_failures_since_start = failures_since_start | ||||||||||||||||||||||||||
|
@@ -382,13 +398,22 @@ def clean_expired_user_attempts( | |||||||||||||||||||||||||
) | ||||||||||||||||||||||||||
return 0 | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
threshold = get_cool_off_threshold(request) | ||||||||||||||||||||||||||
count, _ = AccessAttempt.objects.filter(attempt_time__lt=threshold).delete() | ||||||||||||||||||||||||||
log.info( | ||||||||||||||||||||||||||
"AXES: Cleaned up %s expired access attempts from database that were older than %s", | ||||||||||||||||||||||||||
count, | ||||||||||||||||||||||||||
threshold, | ||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||
if settings.AXES_USE_ATTEMPT_EXPIRATION: | ||||||||||||||||||||||||||
threshold = timezone.now() | ||||||||||||||||||||||||||
count, _ = AccessAttempt.objects.filter(expiration__expires_at__lte=threshold).delete() | ||||||||||||||||||||||||||
log.info( | ||||||||||||||||||||||||||
"AXES: Cleaned up %s expired access attempts from database that expiry were older than %s", | ||||||||||||||||||||||||||
count, | ||||||||||||||||||||||||||
threshold, | ||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||
else: | ||||||||||||||||||||||||||
threshold = get_cool_off_threshold(request) | ||||||||||||||||||||||||||
count, _ = AccessAttempt.objects.filter(attempt_time__lte=threshold).delete() | ||||||||||||||||||||||||||
log.info( | ||||||||||||||||||||||||||
"AXES: Cleaned up %s expired access attempts from database that were older than %s", | ||||||||||||||||||||||||||
count, | ||||||||||||||||||||||||||
threshold, | ||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||
return count | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
def reset_user_attempts( | ||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
from datetime import timedelta | ||
from datetime import timedelta, datetime | ||
from hashlib import sha256 | ||
from logging import getLogger | ||
from string import Template | ||
|
@@ -100,6 +100,21 @@ def get_cool_off_iso8601(delta: timedelta) -> str: | |
return f"P{days_str}T{time_str}" | ||
return f"P{days_str}" | ||
|
||
def get_attempt_expiration(request: Optional[HttpRequest] = None) -> datetime: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This duplicates existing code at https://github.com/jazzband/django-axes/blob/master/axes/attempts.py#L12 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function have separate usecase than get_cool_off_threshold() function. get_attempt_expiration() get call at the time of attempt executed. while get_cool_off_threshold() get called inside the clean_expired_user_attempts() function. Also get_attempt_expiration() works on adding the cool_off time while get_cool_off_threshold() works on subtracting the cool_off time. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's true, I was wondering if they should be co-located in the same file since they have very similar logic. |
||
""" | ||
Get threshold for fetching access attempts from the database. | ||
""" | ||
|
||
cool_off = get_cool_off(request) | ||
if cool_off is None: | ||
raise TypeError( | ||
"Cool off threshold can not be calculated with settings.AXES_COOLOFF_TIME set to None" | ||
) | ||
|
||
attempt_time = request.axes_attempt_time | ||
if attempt_time is None: | ||
return datetime.now() + cool_off | ||
return attempt_time + cool_off | ||
|
||
def get_credentials(username: Optional[str] = None, **kwargs) -> dict: | ||
""" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
# Generated by Django 5.2.1 on 2025-06-10 20:21 | ||
|
||
import django.db.models.deletion | ||
from django.db import migrations, models | ||
|
||
|
||
class Migration(migrations.Migration): | ||
|
||
dependencies = [ | ||
("axes", "0009_add_session_hash"), | ||
] | ||
|
||
operations = [ | ||
migrations.CreateModel( | ||
name="AccessAttemptExpiration", | ||
fields=[ | ||
( | ||
"access_attempt", | ||
models.OneToOneField( | ||
on_delete=django.db.models.deletion.CASCADE, | ||
primary_key=True, | ||
related_name="expiration", | ||
serialize=False, | ||
to="axes.accessattempt", | ||
verbose_name="Access Attempt", | ||
), | ||
), | ||
( | ||
"expires_at", | ||
models.DateTimeField( | ||
help_text="The time when access attempt expires and is no longer valid.", | ||
verbose_name="Expires At", | ||
), | ||
), | ||
], | ||
options={ | ||
"verbose_name": "access attempt expiration", | ||
"verbose_name_plural": "access attempt expirations", | ||
}, | ||
), | ||
] |
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.
Maybe just always display this field and avoid the if-else logic?
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.
Always display may lead to confution of the field usecase, if exsting user don't aware about this new flag.
May be we can re-write it like below.