perf(auth): cache get_user_by_email at service level to reduce DB hot-path queries

[msureshkumar88]

Summary

EmailAuthService.get_user_by_email() is called on every service operation that reads or verifies a user (login, password change, SSO upsert, OAuth token introspection, profile fetch, admin bootstrap, and more — 12+ internal call sites). Each call issued a synchronous SELECT … WHERE email = ? with no caching, even though the existing AuthCache already had an allocated _user_cache dict and the Redis key mcpgw:auth:user:{email} wired into invalidate_user() but never populated.

• Adds get_user() / set_user() to AuthCache using the same L1 (in-memory) → L2 (Redis) write-through pattern as get_user_role() / set_user_role()
• Wires those methods into EmailAuthService.get_user_by_email() so every call checks cache first and populates on a DB hit
• Adds missing cache invalidation to update_user(), activate_user(), and deactivate_user() — three mutation methods that previously committed user changes without busting the cache
• Exposes user_cache_size in AuthCache.stats() (was always 0 because the cache was never written)
• All cache operations wrapped in try/except; Redis outage falls through to DB transparently

No new config knobs. Existing auth_cache_user_ttl (default 60 s) and auth_cache_enabled govern this path.

Closes #3061
Sub-issue of #2692 (DB query hot-path optimisations)

Files Changed

File	What changed
mcpgateway/cache/auth_cache.py	Added get_user() and set_user() methods; added user_cache_size to stats()
mcpgateway/services/email_auth_service.py	Added _user_obj_to_dict() / _user_dict_to_obj() helpers; updated get_user_by_email() to use cache; added _invalidate_user_auth_cache() calls in update_user(), activate_user(), deactivate_user()
tests/unit/mcpgateway/cache/test_auth_cache_user.py	7 new unit tests for cache hit, miss, expiry, invalidation, Redis write-through, disabled cache, and stats
tests/unit/mcpgateway/services/test_email_auth_get_user_cache.py	8 new unit tests for helper round-trips, cache hit skips DB, cache miss populates cache, error fallback to DB, and all three mutation invalidations

Invalidation Coverage After This PR

Mutation path	Method	Invalidation
Password reset	reset_password()	existing
Password change	change_password()	existing
User update (name, admin flag, etc.)	update_user()	added here
Activate user	activate_user()	added here
Deactivate user	deactivate_user()	added here
User deletion	delete_user()	existing
User creation	create_user()	not needed

[Lang-Akshay]

Thanks for the PR - @msureshkumar88

Please address the following findings

• The PR stores a full user object (including password_hash) in Redis and returns a SQLAlchemy-detached object from cache hits. Any method that mutates the returned object (e.g. authenticate_user, unlock_user_account) silently discards those changes because the ORM session does not track the detached instance. This enables two critical authentication-path bypasses.

#	File	Line	Severity	CWE	Description
1	mcpgateway/services/email_auth_service.py	897	Critical	CWE-307	Brute-force lockout bypass via detached ORM object. authenticate_user calls get_user_by_email → cache hit → _user_dict_to_obj returns a new, session-detached EmailUser. Then user.increment_failed_attempts() is called and self.db.commit() is issued — but the detached object is not tracked, so the DB counter is never updated. Every request within the 60 s TTL window sees failed_login_attempts = <stale> and failed attempts are silently discarded, enabling unlimited password guesses during the TTL.
2	mcpgateway/services/email_auth_service.py	1162–1175	Critical	CWE-613	unlock_user_account mutations are silently dropped + missing invalidation. get_user_by_email on a cache-hit returns a detached object. Assignments user.failed_login_attempts = 0 and user.locked_until = None then self.db.commit() silently no-op. Account is never actually unlocked in DB. No _invalidate_user_auth_cache call either, so stale locked state persists in cache.
3	mcpgateway/services/email_auth_service.py	76–91	High	CWE-312	password_hash serialised into Redis. _user_obj_to_dict includes the Argon2id hash in the JSON payload written to Redis. A Redis compromise (SSRF, misconfigured bind, key dump) directly exposes all cached hashes. The existing module docstring states "JWT payloads are NOT cached (security risk)" — the same principle applies here.
4	mcpgateway/services/email_auth_service.py	121	Medium	CWE-20	is_active defaults to True on deserialization. d.get("is_active", True) means a corrupted/missing key produces an active user — fails open. Should be False to fail closed.
5	mcpgateway/services/email_auth_service.py	579, 596	Low	—	Duplicate import auth_cache inside get_user_by_email. from mcpgateway.cache.auth_cache import auth_cache appears in two separate try blocks within the same method call.
6	mcpgateway/cache/auth_cache.py	~413	Low	CWE-362	L1 cache dict read outside lock. self._user_cache.get(email) at the top of get_user is done without acquiring self._lock, inconsistent with the write path inside the method.

Redundant Code

#	File	Line(s)	Type	Description	Suggestion
1	mcpgateway/services/email_auth_service.py	579, 596	Duplicated import	from mcpgateway.cache.auth_cache import auth_cache appears twice in consecutive try blocks inside get_user_by_email	Import once at the top of the method or hoist above both try blocks
2	mcpgateway/cache/auth_cache.py	~428, ~456	Repeated lazy import	import orjson inside both get_user and set_user try-blocks	Move to top of auth_cache.py alongside other third-party imports (or at class level)

[msureshkumar88]

Thanks for the thorough review @Lang-Akshay — all six findings are addressed in the latest commits. Here's a summary of what changed:

Critical / High

#	Finding	Resolution
1	CWE-307 — brute-force bypass via detached ORM object in authenticate_user	Added _fetch_user_from_db() helper; authenticate_user now calls it directly so increment_failed_attempts / reset_failed_attempts mutations are tracked by the session and committed durably
2	CWE-613 — unlock_user_account mutations silently dropped + missing invalidation	Same fix: _fetch_user_from_db() for the session-attached object; _invalidate_user_auth_cache() call added after commit
3	CWE-312 — password_hash serialised into Redis	Removed from _user_obj_to_dict; _user_dict_to_obj falls back to "" for read-only paths. Mutation paths (authenticate_user, unlock_user_account) always fetch from DB, so they never need the hash from cache

Medium / Low

#	Finding	Resolution
4	CWE-20 — is_active defaults to True on deserialisation	Changed to False (fail-closed)
5	Duplicate from mcpgateway.cache.auth_cache import auth_cache in get_user_by_email	Hoisted above both try blocks; import orjson moved to module-level in auth_cache.py
6	CWE-362 — L1 dict read outside self._lock in get_user	Wrapped self._user_cache.get(email) inside with self._lock

Tests updated / added

• test_auth_cache_user.py — added Redis-miss branch test (line 438 _redis_miss_count); removed password_hash from fixture dict
• test_email_auth_get_user_cache.py — added 5 regression tests: password_hash exclusion, is_active fail-closed, _fetch_user_from_db hits DB directly, unlock_user_account invalidation; removed password_hash from fixture dict
• test_email_auth_basic.py — updated two unlock_user_account tests to patch _fetch_user_from_db instead of get_user_by_email

All 17 286 unit tests pass.

[Lang-Akshay]

Security hardening

Scope includes cache integration for user lookups and mutation invalidation. The changes introduce two password-reset regressions tied to cached detached objects.

#	File	Line	Severity	CWE	Description	Fix
1	mcpgateway/services/email_auth_service.py	reset_password_with_token	High	CWE-610	reset_password_with_token() uses get_user_by_email(), which can return a detached cached user; writes to user.password_hash may not persist, leaving the previous password valid.	Fetch a session-attached user in reset_password_with_token() via _fetch_user_from_db() (or merge the object into the session) before mutating and committing.
2	mcpgateway/services/email_auth_service.py	reuse check	Medium	CWE-521	Password reuse checks can be bypassed on cache hits because cached users store an empty password_hash, making reuse comparisons always pass.	Perform reuse checks using a DB-attached

[msureshkumar88]

Thanks @Lang-Akshay — both findings from the second review are addressed in the latest commit.

Finding 1 (High · CWE-610) — detached object in reset_password_with_token

get_user_by_email at line 1123 could return a cached, session-detached EmailUser. All subsequent field assignments (password_hash, password_change_required, password_changed_at, failed_login_attempts, locked_until) were written to an object not tracked by self.db, so self.db.commit() silently discarded them — the old password remained valid.

Fixed by replacing await self.get_user_by_email(...) with self._fetch_user_from_db(...), which always returns a session-attached object. Same pattern as authenticate_user and unlock_user_account.

Finding 2 (Medium · CWE-521) — reuse check bypassed via empty cached hash

_user_obj_to_dict intentionally excludes password_hash from the cache payload (CWE-312 fix from the first review). _user_dict_to_obj therefore reconstructs the object with password_hash="". The reuse check verify_password_async(new_password, "") always returned False, so any password passed the reuse guard on cache hits.

Resolved by the same fix above — _fetch_user_from_db returns the real password_hash from the DB, so the reuse check operates on the actual credential material.

Tests updated

Four reset_password_with_token test cases in test_email_auth_basic.py were patching get_user_by_email (async). Updated to patch _fetch_user_from_db (sync, no AsyncMock) — all four pass.

[Lang-Akshay]

Thanks for the PR implementing service-level caching for get_user_by_email(), reducing database load during the authentication hot path by storing User objects in AuthCache (both L1 in-memory and L2 Redis)

[brian-hussey]

All tests had passed prior to rebase.
Only conflict was .secrets.baseline - I resolved that and force pushed.

Previous state prior to force push:

Merging without waiting for checks to go through queue again.