Skip to content

Conversation

kuldeepkhatke
Copy link
Contributor

Individual attempt based longest expiry attempt restriction feature

Implemented a AXES_INDIVIDUAL_ATTEMPT_EXPIRY flag based feature,
such that if we enable it then it will handle attempts cleanup based on the longest expiry of the attempt.
Hence added expire_at field to implement this feature.

All testcases passed after implementation.

Fixes #1302

@aleksihakli
Copy link
Member

Comparing to #1310 this adds state to the database which is only used if the flag is switched on, otherwise growing the database table size and seldom using the expiration times. I would separate the new expiration time field into it's own table if it's not used often, and only write it to the database when necessary.

Individual attempt based longest expiry attempt restriction feature

Implemented a AXES_INDIVIDUAL_ATTEMPT_EXPIRY flag based feature, such that if we enable it then it will handle attempts cleanup based on the longest expiry of the attempt. Hence added expire_at field to implement this feature.

All testcases passed after implementation.

Fixes #1302

Existing test cases pass, but there are no test cases for the new flag / feature, which need to be added if this is to be merged upstream.

Copy link
Contributor Author

@kuldeepkhatke kuldeepkhatke left a comment

Choose a reason for hiding this comment

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

I'm modifying the changes as per suggestions, & will add test cases accordingly.

@kuldeepkhatke kuldeepkhatke force-pushed the individual-attempt-expiry branch from b66023d to 5a3a53c Compare June 7, 2025 17:08
@kuldeepkhatke
Copy link
Contributor Author

Hey @aleksihakli ,
I've made the required changes as per suggestion.

Added testcases for the feature.

Please review the changes.

@kuldeepkhatke
Copy link
Contributor Author

Also, i'm curious if we can consider adding a AUTHORS file to add the name of list of contributors

Copy link
Member

@aleksihakli aleksihakli left a comment

Choose a reason for hiding this comment

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

A few notes here and there to polish the implementation up, looks good otherwise 👍

Copy link

codecov bot commented Jun 7, 2025

Codecov Report

Attention: Patch coverage is 55.81395% with 19 lines in your changes missing coverage. Please review.

Project coverage is 92.17%. Comparing base (3ff5ada) to head (5a3a53c).
Report is 17 commits behind head on master.

Files with missing lines Patch % Lines
axes/attempts.py 12.50% 7 Missing ⚠️
axes/handlers/database.py 56.25% 6 Missing and 1 partial ⚠️
axes/admin.py 28.57% 4 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1313      +/-   ##
==========================================
- Coverage   93.55%   92.17%   -1.38%     
==========================================
  Files          36       37       +1     
  Lines        1179     1214      +35     
  Branches      158      165       +7     
==========================================
+ Hits         1103     1119      +16     
- Misses         57       74      +17     
- Partials       19       21       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@aleksihakli
Copy link
Member

Hey @aleksihakli , I've made the required changes as per suggestion.

Added testcases for the feature.

Please review the changes.

Looks good, nice job 👍 I made a few suggestions for further improvements but I think this is close to merge ready.

@kuldeepkhatke kuldeepkhatke force-pushed the individual-attempt-expiry branch from 9258277 to dcdb83b Compare June 8, 2025 09:06
@kuldeepkhatke
Copy link
Contributor Author

Hi @aleksihakli ,
Updated the code as per suggestions, rebased with latest code.

Thanks

@kuldeepkhatke kuldeepkhatke requested a review from aleksihakli June 8, 2025 13:40
)

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.

@blockchainGuru1018
Copy link

approved

)

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.

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.

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

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

@aleksihakli
Copy link
Member

Sorry for the slow merge on this one, I haven't had the opportunity to bake a new release in the last couple of weeks. Merging now 👍 Thank you for the effort and PR @kuldeepkhatke

@aleksihakli aleksihakli merged commit 392dfa0 into jazzband:master Jul 5, 2025
17 checks passed
@kuldeepkhatke
Copy link
Contributor Author

Absolutely, No Problem @aleksihakli ,

Thanks for appreciation & merging my PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

User-dependant cool-off can reset attempts related to other users

3 participants