Skip to content

feat(auth): record throttled lastUsedAt on authentication and expose in key listing - #587

Merged
Miracle656 merged 1 commit into
Divinity-Labs:mainfrom
gospeltout:Nothing-records-when-a-key-was-last-used
Oct 23, 2026
Merged

Miracle656 merged 1 commit into
Divinity-Labs:mainfrom
gospeltout:Nothing-records-when-a-key-was-last-used

Conversation

@gospeltout

Copy link
Copy Markdown
Contributor

Summary

  • Added lastUsedAt (DateTime? @map("last_used_at")) to the ApiKey model in Prisma.
  • Recorded lastUsedAt on successful API key authentication in apiKeyAuthPlugin.
  • Throttled database writes in-memory to at most once per configured interval per key, avoiding write-amplification on read-heavy traffic.
  • Documented and made the throttle interval configurable via KEY_LAST_USED_THROTTLE_SECONDS (defaults to 60 seconds).
  • Exposed lastUsedAt on key listings via admin-guarded GET /admin/keys and key creation response POST /admin/keys.
  • Added comprehensive Vitest tests verifying write throttling, interval expiration, independent per-key tracking, error resilience, and listing exposure.

Related issue

Closes #500

Type of change

  • Bug fix
  • New feature
  • Refactor
  • Docs
  • Tests
  • CI / tooling

Checklist

  • I have read CONTRIBUTING.md
  • npx tsc --noEmit passes
  • npm run build passes
  • I added / updated tests where relevant
  • I updated docs where relevant

@drips-wave

drips-wave Bot commented Oct 20, 2026

Copy link
Copy Markdown

@gospeltout Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

@Miracle656
Miracle656 merged commit 250d8be into Divinity-Labs:main Oct 23, 2026
@Miracle656

Copy link
Copy Markdown
Collaborator

Merged as 250d8be. The throttle is the part that makes this viable, and you got the subtle bit right: the window is claimed before the await, so concurrent requests for the same key do not all decide they are the one that should write. Making the interval configurable with a documented default, and adding the API_KEY_LAST_USED_THROTTLE_SECONDS alias to the envDrift allow-list rather than leaving the drift test to fail, is the kind of follow-through that is usually missing. The test for "tracks throttle intervals independently per key" is the one I would have asked for.

Merged locally. The branch predated the buildApp() and admin-auth work, so four files conflicted — including the entire /admin/keys handler. I took main for those and re-applied only the #500 parts. One scope note: I did not take your pagination of /admin/keys. That is issue #505, which is assigned to someone else and has its own PRs open, so only lastUsedAt went onto the existing listing.

Three things on top.


1. There was no migration, so the column would never have reached a live database

This PR is the first schema change since #577 landed this morning, and that changes the rules in a way nothing in the repo would have told you. Startup used to run:

prisma db push --accept-data-loss

which diffed the live database against schema.prisma on every boot — so adding a field to schema.prisma and nothing else genuinely worked. Startup now runs prisma migrate deploy, which applies only committed migrations. The baseline is already recorded as applied, there would have been nothing pending, and last_used_at would simply never have existed in any live database.

And the failure would have been silent: every prisma.apiKey.update rejected with "column does not exist", caught by the bare catch, recordApiKeyLastUsed returning false forever, lastUsedAt null on every key in /admin/keys. The feature would look implemented and do nothing.

Added:

-- prisma/migrations/20261023120000_api_key_last_used_at/migration.sql
ALTER TABLE "api_keys" ADD COLUMN "last_used_at" TIMESTAMP(3);

Nullable with no default on purpose — NULL meaning "has not authenticated since this column existed" is the whole signal. Backfilling with now() would mark every key freshly used on the day it shipped.

Worth reading docs/database-migrations.md before the next schema change; npm run db:migrate generates the folder for you.

2. Regenerating the baseline instead would have broken a fresh database — and #577's test demanded exactly that

Worth writing down, because I walked into it myself.

My first attempt regenerated prisma/migrations/0_init/migration.sql from the new schema and added the ALTER. On an existing database that is fine (the baseline is only recorded, never run). On a fresh one — CI, a new local Postgres — migrate deploy runs both: 0_init creates api_keys with last_used_at, then the ALTER adds it again, and 42701 column already exists fails the deploy.

So the baseline stays behind schema.prisma deliberately, with the later migration carrying the difference. Which means #577's DB-free test was wrong the moment a second migration existed:

// it('has an init/baseline migration that reproduces schema.prisma exactly')
expect(actual).toBe(expected)   // baseline vs `migrate diff --from-empty --to-schema-datamodel`

That assertion would have failed on your PR and the obvious way to make it green is the regeneration that causes the collision. It now runs the comparison only while the baseline is the whole history, and once there is more than one migration it defers to the DB-backed replay (scripts/db-drift-check.sh, the db-migrations CI job), which diffs --from-migrations against the schema on a real Postgres and stays correct however long the history gets.

I also added a case asserting that no migration sorts before the baseline. Prisma replays lexicographically, so a hand-made folder named 0000_whatever would run against an empty database and fail — migrate dev's timestamp prefix sorts after 0_ naturally, but nothing was checking.

3. The failed write was swallowed

} catch {
  // Best-effort write: database error updating lastUsedAt should not fail
  // an otherwise valid and authenticated request.
  return false
}

The reasoning is right — refusing a valid authenticated request because an audit timestamp could not be stored would be a bad trade. But after (1), a missing column is a live possibility, and a silent catch means nobody ever learns. It now logs through the house logger, which pipes lines through redactSecrets, once per key id:

} catch (err: unknown) {
  if (!lastUsedWriteFailuresLogged.has(keyId)) {
    lastUsedWriteFailuresLogged.add(keyId)
    log.warn({ err, keyId }, 'could not record lastUsedAt for an API key; authentication is unaffected')
  }
  return false
}

Once per key rather than once per throttle window, so one misconfigured environment is a handful of lines, not a permanent stream.


Two smaller changes

Where the call sits. I moved recordApiKeyLastUsed(keyContext.id) to immediately after the key is accepted, ahead of the scope check and the daily-quota check. "Last used" should mean "last authenticated", not "last allowed through" — otherwise a key parked at its daily quota, or calling a route it lacks the scope for, reads as abandoned, which is the opposite of what an operator wants from this field.

resetApiKeyLastUsedThrottle() in beforeEach. The throttle map is module-level state keyed by id. Your tests avoid collisions by using distinct ids per case (key-1, key-throttled, key-alpha…), which works but is load-bearing on a convention nothing enforces — the next test to reuse an id would see its write silently throttled and fail in a confusing way.

126 test files / 1,499 tests pass. tsc --noEmit clean.

Miracle656 added a commit that referenced this pull request Oct 25, 2026
Rebased as asked: the pagination is folded into the existing handler rather
than added as a second route, and main's selected columns survive — lastUsedAt
from #587 and the quota/overage fields are all still there.

`parsePageValue` rejects rather than coerces, and caps limit at
MAX_KEY_PAGE_SIZE. Bounding offset at Number.MAX_SAFE_INTEGER as well is more
than most listings do and is the reason a hostile offset cannot turn into a
silently wrong query.

The conflict with #595 was additive on both sides — main's zod schemas against
this branch's pagination helpers — and is resolved as a union, keeping both.

tsc clean, 1,734 tests pass.

Claude-Session: https://claude.ai/code/session_01USgemLt4Rnz4SGB1Srf3GB
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Nothing records when a key was last used

2 participants