feat: add API Key authentication for GTFS-RT feed consumers#68
feat: add API Key authentication for GTFS-RT feed consumers#68ShinLiX wants to merge 2 commits intoOneBusAway:mainfrom
Conversation
aaronbrethorst
left a comment
There was a problem hiding this comment.
Amelia, this is a well-scoped feature — the SHA-256 key hashing, the APIKeyStore interface for testability, and the thorough test coverage across both the middleware and the store layer are all well done. The seed data with a pre-hashed dev key is a nice touch for local testing. A few things need attention before this can merge.
Critical
- Needs rebase on latest
main— the branch is missing000004_add_vehicle_received_indexwhich has since landed. Your migration number (000005) is correct and won't need renumbering, but the branch needs to include 000004 forgolang-migrateto run them in sequence.
Important
-
UpdateAPIKeyLastUsedfailure blocks feed access (api_key_auth.go:51-54). If thelast_used_atupdate fails (transient DB error, connection hiccup), the middleware returns 500 and the feed consumer gets nothing — even though their API key was already validated. Thelast_used_atupdate is an audit convenience, not a security requirement. On a hot path like the GTFS-RT feed, this should not block the response. Log the error and proceed:if err := store.UpdateAPIKeyLastUsed(r.Context(), apiKey.ID); err != nil { slog.Error("failed to update api key last_used_at", "api_key_id", apiKey.ID, "error", err) }
-
Missing
sloglogging on 500 responses (api_key_auth.go:42,api_key_auth.go:52). WhenGetAPIKeyByHashreturns a non-pgx error, the handler returns 500 but doesn't log the error. The established pattern in this codebase (seehandlers.go,auth.go) is to callslog.Error(...)before returning a 500 so operators can diagnose failures. Please add structured logging to both 500 paths. -
docker-compose.ymluses${JWT_SECRET}from host environment (docker-compose.yml:26). The previous docker-compose had noJWT_SECRETset at all, and the app would fail on startup. Your fix is reasonable, but using${JWT_SECRET}requires developers to set the variable in their host environment or a.envfile, which is a friction change for anyone runningdocker compose upwithout extra setup. Consider using a hardcoded dev value (like other PRs have done) to keep local dev zero-config:JWT_SECRET: "this-is-a-local-dev-secret-1234567890"
Suggestions
-
Missing trailing newlines in
db/query.sql,migrations/000005_add_api_keys.down.sql,migrations/000005_add_api_keys.up.sql, andseed_dev.sql. Most editors and POSIX tools expect a trailing newline. Minor, but easy to fix while you're in there. -
The
updated_attrigger (migrations/000005_add_api_keys.up.sql:12-23) is a valid approach, but the rest of the codebase handlesupdated_atat the application/SQL layer (SET updated_at = NOW()in queries). The trigger works fine, but the inconsistency could confuse future contributors who expect one pattern and find another. Not a blocker — just something to be aware of.
Strengths
- SHA-256 key hashing: Raw keys are never stored, which is the right approach for API key security.
- Complete test coverage: Seven middleware tests covering every branch (missing header, invalid key, inactive key, store failure, update failure, valid key) plus three integration tests on the store layer.
- Clean interface design:
APIKeyStoreis minimal and follows the project's established pattern of handler-scoped interfaces. - Dev seed data: The pre-hashed
dev-feed-keyinseed_dev.sqlwith the raw key documented in a comment makes local testing frictionless.
Recommended Action
Request changes. Rebase on latest main, make the UpdateAPIKeyLastUsed failure non-blocking, and add slog logging to the 500 paths.
6c411eb to
567bcdf
Compare
|
Hi Aaron, thanks for your review! All issues mentioned are addressed:
Thank you, please let me know if these work! |
Summary
This PR adds API key authentication for feed consumers on
GET /gtfs-rt/vehicle-positions.Previously, the GTFS-Realtime vehicle positions feed could be accessed without any feed-specific authentication. As requested in Milestone 2, we need an authentication for accessing the feed.
The implementation hashes raw API keys with SHA-256 before lookup, rejects missing/invalid/inactive keys, and updates
last_used_aton successful requests.Changes
APIKeymodel to represent rows in theapi_keystable.requireAPIKeymiddleware for feed access.hashAPIKey(raw string) string, which computes the SHA-256 hash of the provided raw key and returns the hex-encoded string.api_keystable and a trigger/function pair to automatically maintainupdated_aton updates.seed_dev.sqlto include a development API key entry for local manual testing,Testing
Test file added:
TestHashAPIKeyTestRequireAPIKey_MissingHeaderTestRequireAPIKey_InvalidKeyTestRequireAPIKey_InactiveKeyTestRequireAPIKey_StoreFailureTestRequireAPIKey_UpdateLastUsedFailureTestRequireAPIKey_ValidKeyTestStore_CreateAndGetAPIKeyByHashTestStore_UpdateAPIKeyLastUsedTestStore_GetAPIKeyByHash_InactiveRunning
go test ./...: All tests passedManual Testing with
curl:seed_dev.sql:curl -i -H "X-API-Key: dev-feed-key" http://localhost:8080/gtfs-rt/vehicle-positions": return status 200 OKcurl -i -H "X-API-Key: dev-feed-key-1234567890abcdef" http://localhost:8080/gtfs-rt/vehicle-positions: return 401 unauthorized