Potential ValueError for expired AccessAttempt records

[okapies]

In AxesDatabaseHandler, user_login_failed() can accidentally cause the following ValueError if another concurrent process deletes (e.g. clean_expired_user_attempts) an existing record retrieved by AccessAttempt.objects.get_or_create() before committing attempt.save() on the DB.

ValueError: Failed to insert expression "Concat(ConcatPair(Col(axes_accessattempt, axes.AccessAttempt.get_data), Value(
---------
)))" on axes.AccessAttempt.get_data. F() expressions can only be used to update, not to insert.

https://github.com/jazzband/django-axes/blob/ac86d4b2139f632cee972e874984982a9628ab27/axes/handlers/database.py#L155

[aleksihakli]

Thanks for reporting @okapies. Do you have a suggestion for fixing this?

[okapies]

One idea is not to run clean_expired_user_attempts in the handler. IMHO, we should not remove existing records especially when they have a chance to be updated.

[okapies]

Another option is to develop a new DatabaseHandler which stores an Attempt record per request. In this case, the handler calculates a failure count by aggregating the saved records, and we can remove the expired ones.

[aleksihakli]

One idea is not to run clean_expired_user_attempts in the handler. IMHO, we should not remove existing records especially when they have a chance to be updated.

How would we distinguish the expired attempts with this approach, and how would expired data be cleaned up?

Another option is to develop a new DatabaseHandler which stores an Attempt record per request. In this case, the handler calculates a failure count by aggregating the saved records, and we can remove the expired ones.

This is viable and semantically correct, but what would happen in a case where someone e.g. DDoSes the service? It would be very easy to generate a lot of bloat data into the database in a very short time.

[okapies]

Hmm, or add select_for_update() when getting the records?

attempt, created = AccessAttempt.objects.select_for_update().get_or_create(...)

[aleksihakli]

select_for_update does actually seem like a really good option. I tested it locally, and the queryset API does support using it together with get_or_create. Would you like to open a PR for this?

I checked the API reference and most commonly used databases including mysql and postgresql support the select_for_update queryset method at the very least:

https://docs.djangoproject.com/en/3.2/ref/models/querysets/#select-for-update

[frisbm]

Hey, not sure if anyone has shared this yet but this same error is actually occurring when tracking AccessAttempts via simple history. The history tries to save on update but the F() statement breaks the save since it tries to insert instead of update.

Here are our relevant package versions: Django - 3.2.8 django-simple-history - 3.0.0 django-axes - 5.25.0

[aleksihakli]

Can you post your full listing of packages for the project for reference?