Use redis cache in get_approx_account_lockouts_from_login_attempts

[irkonikovich]

No description provided.

[kencochrane]

@irkonikovich The tests are not passing.

I also passed this through claude and got the following output, please take a look.

PR #250 Review: Use redis cache in get_approx_account_lockouts_from_login_attempts

I found several significant issues with this PR:

Critical Issues

1. The test is backwards

The test test_approx_account_lockout_uses_redis_cache asserts that the cached value is None:

self.assertIsNone(cached_value)

But based on the implementation, after calling get_approx_account_lockouts_from_login_attempts(), the value should be cached. The test is verifying that caching is not working, which is the opposite of what the test name implies. This should be:

self.assertIsNotNone(cached_value)

2. Missing import in tests.py

The test uses get_redis_connection() but never imports it:

redis_client = get_redis_connection()  # NameError - not imported!
The import line only adds get_approx_lockouts_cache_key:

from defender.data import get_approx_account_lockouts_from_login_attempts, get_approx_lockouts_cache_key
It needs to also import from defender.connection:

from defender.connection import get_redis_connection

Design Concerns

3. No cache invalidation - potential security issue

When new login attempts are stored via store_login_attempt(), the cache is never invalidated. This creates a stale data problem:

1. User has 4 failed attempts (below threshold of 5)
2. get_approx_account_lockouts_from_login_attempts() is called, returns 0, caches it for hours
3. User makes more failed attempts triggering lockouts
4. Function returns stale cached value of 0 instead of actual lockout count
5. User gets shorter cooloff times than they should (security issue)

The store_login_attempt function in data.py:9-20 should invalidate or update the relevant cache key when recording new attempts.

4. Long cache TTL

The cache TTL is set to ACCESS_ATTEMPT_EXPIRATION * 60 * 60 seconds (default is likely hours). Combined with no invalidation, this means stale data could persist for a very long time.

Summary

This PR is not ready to merge. The test would fail to run (missing import) and is testing the wrong behavior. More importantly, the lack of cache invalidation could lead to security issues where attackers receive shorter lockout cooloff periods than intended.

[kencochrane]

Look close, just a comment

[kencochrane]

This will always return True because get_approx_lockouts_cache_key() always returns a string. So this is redundant. Is there a reason why you are doing this check?

[irkonikovich]

You're right. I was looking at too many things at once.

This if is only valid where get it from the cache, not when I generate the key. It's a copy-paste error.

I will remove it, thank you!

[kencochrane]

This will always return True because get_approx_lockouts_cache_key() always returns a string. So this is redundant. Is there a reason why you are doing this check?

[kencochrane]

This will always return True because get_approx_lockouts_cache_key() always returns a string. So this is redundant. Is there a reason why you are doing this check?