feat: api key last_used_at and failed auth logging - #266
Conversation
Stamp last_used_at on successful API key auth, debounced to one write per key per hour and best-effort so the hot path never fails or slows. Expose it on the keys list endpoint. Log failed key auth (unknown, revoked, expired) with the display prefix only.
📝 WalkthroughWalkthroughAPI-key authentication now uses timezone-aware expiry checks, debounced best-effort usage stamping, and structured warnings for unknown, revoked, or expired keys. API-key models, listing responses, repository behavior, and unit tests were updated accordingly. ChangesAPI key hygiene
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant get_current_user
participant ApiKeyRepository
participant APIKeyDatabase
get_current_user->>ApiKeyRepository: Authenticate API key
get_current_user->>ApiKeyRepository: Touch last_used_at when stale
ApiKeyRepository->>APIKeyDatabase: Update timestamp
APIKeyDatabase-->>ApiKeyRepository: Return update result
ApiKeyRepository-->>get_current_user: Continue authentication
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Zingzy
left a comment
Zingzy
left a comment
There was a problem hiding this comment.
Mergeable, nothing blocking. Two claims verified, small nits inline.
Verified:
- Prefix parity: key creation stores
token_prefix = raw[:8](services/api_key_service.py:73), so the unknown-key log line exposes exactly what the dashboard already shows for real keys, nothing more. - Merge-order coupling:
key_prefixandkey_idinapi_key_auth_failedonly survive the log redactor because of the safe list added in #265 (both names contain "key" and would otherwise land in Axiom as***REDACTED***). The stack ordering enforces this; do not cherry-pick this without #265.
Notes:
- Concurrent stale auths can each fire
touch_last_used; it is a single-document$setwriting the same value, so harmless. - DB errors during lookup still return None silently (pre-existing, unchanged here).
reason="unknown"only fires when the lookup succeeded and found nothing, which is the right split: a Mongo outage should not masquerade as credential misuse. - The debounce write is awaited inline, so roughly one API-key request per key per hour pays a single-document write of latency. Fine at current volume; if it ever shows up in duration percentiles, fire-and-forget is the escape hatch.
Affected test files pass at this head (341). Full matrix will run once this retargets to main.
Use shared as_aware_utc for both expiry and last_used normalization, trim the failure-log comment to its invariant, and drop the unused key_doc param from the test mock helper.
|
All three resolved at af9aa97, verified in the diff rather than the descriptions: the comment now states just the invariant, both datetime normalizations go through |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
dependencies/auth.py (1)
95-98: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUnexpected repository errors during API-key auth are silently swallowed.
find_by_hashandfind_by_idfailures bothreturn Nonewith no logging, unlike the deliberately-loggedunknown/revoked/expiredreasons. A Mongo outage on the API-key path would look identical to normal invalid-key traffic in logs, undermining the diagnostics goal of this PR. Consider logging (e.g.log.warning("api_key_auth_error", ...)) before returningNonein these two branches.Also applies to: 127-130
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dependencies/auth.py` around lines 95 - 98, Update the exception handlers around key_repo.find_by_hash and key_repo.find_by_id to log unexpected repository failures with the existing logger before returning None. Include sufficient error context, such as the operation and exception, while preserving the current authentication failure return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@dependencies/auth.py`:
- Around line 95-98: Update the exception handlers around key_repo.find_by_hash
and key_repo.find_by_id to log unexpected repository failures with the existing
logger before returning None. Include sufficient error context, such as the
operation and exception, while preserving the current authentication failure
return behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 895673aa-b354-4ada-ac4f-4f432ea4afd1
📒 Files selected for processing (6)
dependencies/auth.pyrepositories/api_key_repository.pyroutes/api_v1/keys.pyschemas/dto/responses/api_key.pyschemas/models/api_key.pytests/unit/test_auth_deps.py
Stacked on #265.
What
last_used_aton API key documents, stamped on successful auth and returned byGET /api/v1/keysas a Unix timestamp (null when never used).api_key_auth_failedwith a reason (unknown,revoked,expired).Performance
The stamp is debounced: it only writes when the stored value is missing or older than one hour, so a key making 100k requests a day costs about 24 single-document writes. The write is best-effort inside a try/except; a failed or slow stamp can never fail or block authentication. The hot path stays read-only otherwise.
Security
Failure logs carry the display prefix only (the same 8 characters the dashboard shows), never hashes or raw material. Volume is bounded upstream by the rate limiter. Revoked and expired entries include
key_idanduser_idso a revoked key still being retried by a forgotten script is visible and attributable.Notes
last_used_atgives the dashboard a "never used / last used N days ago" signal, which is the piece that makes revoking old keys feel safe.Summary by CodeRabbit
New Features
Bug Fixes