-
-
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
Added individual attempt expiry feature for #1302 #1313
Conversation
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.
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. |
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.
I'm modifying the changes as per suggestions, & will add test cases accordingly.
b66023d
to
5a3a53c
Compare
Hey @aleksihakli , Added testcases for the feature. Please review the changes. |
Also, i'm curious if we can consider adding a AUTHORS file to add the name of list of contributors |
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.
A few notes here and there to polish the implementation up, looks good otherwise 👍
Codecov ReportAttention: Patch coverage is
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. 🚀 New features to boost your workflow:
|
Looks good, nice job 👍 I made a few suggestions for further improvements but I think this is close to merge ready. |
…et_attempt_expiration()
9258277
to
dcdb83b
Compare
Hi @aleksihakli , Thanks |
) | ||
|
||
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 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?
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) |
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.
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.
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
approved |
) | ||
|
||
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 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", |
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.
"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): |
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.
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?
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.
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: |
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.
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 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.
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.
That's true, I was wondering if they should be co-located in the same file since they have very similar logic.
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 |
Absolutely, No Problem @aleksihakli , Thanks for appreciation & merging my PR. |
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