Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 23 additions & 9 deletions axes/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Member

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?

Copy link
Contributor Author

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.

Suggested change
"expiration",
list_display = [
"attempt_time",
"ip_address",
"user_agent",
"username",
"path_info",
"failures_since_start",
]
if settings.AXES_USE_ATTEMPT_EXPIRATION:
list_display.append('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"]

Expand All @@ -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")}),
)
Expand All @@ -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):
Copy link
Member

Choose a reason for hiding this comment

The 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 inlines definition, if those happen to work with One-to-One models at the moment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
so, doesn't benefit much to show separately ?

return obj.expiration.expires_at if hasattr(obj, "expiration") else _("Not set")

class AccessLogAdmin(admin.ModelAdmin):
list_display = (
Expand Down
2 changes: 2 additions & 0 deletions axes/conf.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@

settings.AXES_COOLOFF_TIME = getattr(settings, "AXES_COOLOFF_TIME", None)

settings.AXES_USE_ATTEMPT_EXPIRATION = getattr(settings, "AXES_USE_ATTEMPT_EXPIRATION", False)

settings.AXES_VERBOSE = getattr(settings, "AXES_VERBOSE", settings.AXES_ENABLED)

# whitelist and blacklist
Expand Down
41 changes: 33 additions & 8 deletions axes/handlers/database.py
Original file line number Diff line number Diff line change
Expand Up @@ -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__)
Expand Down Expand Up @@ -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:
Copy link
Member

Choose a reason for hiding this comment

The 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 get_attempt_expiration returns an earlier value?

Suggested change
if not hasattr(attempt, "expiration") or attempt.expiration is None:
if settings.AXES_USE_ATTEMPT_EXPIRATION:
expiration, created = AccessAttemptExpiration.objects.update_or_create(
access_attempt=attempt,
defaults={
"expires_at": get_attempt_expiration(request),
}
)
if created:
log.debug("AXES: Created new AccessAttemptExpiration for %s", client_str)
else:
log.debug("AXES: Updated existing AccessAttemptExpiration for %s", client_str)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
Then as per initial discussions we shold hold it till longest expiration.
Thats what this feature is meant to be.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider the below example with a twist like UserA is only tried before with 60min cool_off & later with 5min cool_off.

image

I think its good to have flow.

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
Expand Down Expand Up @@ -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(
Expand Down
17 changes: 16 additions & 1 deletion axes/helpers.py
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
Expand Down Expand Up @@ -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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

The 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:
"""
Expand Down
41 changes: 41 additions & 0 deletions axes/migrations/0010_accessattemptexpiration.py
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",
},
),
]
17 changes: 17 additions & 0 deletions axes/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,23 @@ class Meta:
unique_together = [["username", "ip_address", "user_agent"]]


class AccessAttemptExpiration(models.Model):
access_attempt = models.OneToOneField(
AccessAttempt,
primary_key=True,
on_delete=models.CASCADE,
related_name="expiration",
verbose_name=_("Access Attempt"),
)
expires_at = models.DateTimeField(
_("Expires At"),
help_text=_("The time when access attempt expires and is no longer valid."),
)

class Meta:
verbose_name = _("access attempt expiration")
verbose_name_plural = _("access attempt expirations")

class AccessLog(AccessBase):
logout_time = models.DateTimeField(_("Logout Time"), null=True, blank=True)
session_hash = models.CharField(_("Session key hash (sha256)"), default="", blank=True, max_length=64)
Expand Down
Loading
Loading