feat(merchant): API key management service#22
Merged
Conversation
Add protected CRUD endpoints for merchant API keys with SHA-256 storage, prefix display, 10-key limit, and unified JWT/session/API-key authentication middleware. Closes ShadeProtocol#14 Co-authored-by: Cursor <cursoragent@cursor.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds merchant API key generation, listing, revocation, and authentication support, including schema and migration updates, new middleware and route wiring, and unit/integration coverage for the new API-key flow. ChangesAPI Key Management
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthMiddleware
participant ApiKeyService
participant MerchantRoutes
participant ApiKeyControllers
Client->>AuthMiddleware: Bearer token
AuthMiddleware->>ApiKeyService: authenticateApiKey() or merchant lookup
MerchantRoutes->>ApiKeyControllers: create/list/revoke
ApiKeyControllers->>ApiKeyService: createApiKey/listApiKeys/revokeApiKey
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
tests/helpers/api-key.fixtures.tsParsing error: "parserOptions.project" has been provided for tests/integration/api-key.routes.test.tsParsing error: "parserOptions.project" has been provided for tests/integration/auth.middleware.test.tsParsing error: "parserOptions.project" has been provided for
Comment |
There was a problem hiding this comment.
Pull request overview
Adds first-class merchant API key management and authentication, enabling server-to-server integrations while storing only hashed keys and supporting multiple auth methods (refresh token, JWT, API key) in the same middleware.
Changes:
- Added API key generation + hashing utilities, plus a service layer to create/list/revoke/authenticate API keys.
- Added protected merchant routes/controllers for API key CRUD and expanded
authenticateMerchantto accept API keys. - Added Prisma schema + migration for persisting an API key
prefix, plus unit/integration coverage for key management + auth flows.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/api-key.utils.test.ts | Unit tests for key material generation, hashing, and token identification. |
| tests/unit/api-key.services.test.ts | Unit tests for API key service behaviors (limits, listing, revocation, auth). |
| tests/integration/auth.middleware.test.ts | Integration coverage for refresh/JWT/API-key auth paths and lastUsedAt updates. |
| tests/integration/api-key.routes.test.ts | Integration coverage for merchant API key CRUD endpoints and middleware auth behavior. |
| tests/helpers/api-key.fixtures.ts | Test fixtures for API keys (avoids secret-scanning false positives). |
| src/utils/api-key.utils.ts | Implements API key format, hashing, and key material generation. |
| src/services/api-key.services.ts | Implements create/list/revoke/authenticate behaviors for API keys. |
| src/routes/merchant.routes.ts | Adds /merchants/api-keys CRUD endpoints protected by merchant auth. |
| src/middlewares/auth.middleware.ts | Extends merchant auth to support API keys + JWTs; adds apiKeyAuth. |
| src/controllers/api-key.controllers.ts | Adds controllers for create/list/revoke API keys with AppError handling. |
| prisma/schema.prisma | Adds required prefix column to ApiKey model. |
| prisma/migrations/20260627120000_add_api_key_prefix/migration.sql | Migration adding ApiKey.prefix. |
Comments suppressed due to low confidence (1)
src/middlewares/auth.middleware.ts:41
- apiKeyAuth duplicates the API-key branch of authenticateMerchant but is not referenced anywhere in src/. Keeping two implementations increases the risk they diverge over time; consider either removing apiKeyAuth or wiring routes to use it consistently (and sharing common logic).
/**
* Authenticates API key bearer tokens, updates lastUsedAt, and attaches the merchant.
*/
export const apiKeyAuth = async (
req: Request,
res: Response,
next: NextFunction,
): Promise<void> => {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+39
to
+42
| const activeKeys = await countActiveApiKeys(merchantId); | ||
| if (activeKeys >= MAX_ACTIVE_API_KEYS) { | ||
| throw new AppError(400, 'Maximum of 10 active API keys allowed'); | ||
| } |
Comment on lines
+62
to
+69
| export const listApiKeys = async (merchantId: string): Promise<ApiKeySummary[]> => { | ||
| const apiKeys = await prisma.apiKey.findMany({ | ||
| where: { | ||
| merchantId, | ||
| revokedAt: null, | ||
| }, | ||
| orderBy: { createdAt: 'desc' }, | ||
| }); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
tests/unit/api-key.services.test.ts (2)
120-126: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert the merchant-scoped revoke lookup here.
This case only preloads
findFirstwithnull, so it would still pass ifrevokeApiKeyaccidentally queried byidalone. Add an assertion that the lookup includes bothidandmerchantId, since that ownership check is part of the feature contract.🤖 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 `@tests/unit/api-key.services.test.ts` around lines 120 - 126, The revoke test only checks the 404 outcome and could still pass if revokeApiKey ignored merchant scoping, so update this case to assert the prismaMock.apiKey.findFirst lookup includes both id and merchantId. Use the revokeApiKey flow in api-key.services.test.ts to verify the ownership check contract directly, and keep the existing rejection assertion for the 404 behavior.
128-143: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin the hashed lookup contract in the auth test.
The happy-path assertion verifies
lastUsedAt, but it never checks thatauthenticateApiKeycalls Prisma withwhere: { keyHash: ... }andinclude: { merchant: true }. If the service regressed to using the raw bearer token directly, this test would still pass.🤖 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 `@tests/unit/api-key.services.test.ts` around lines 128 - 143, The auth happy-path test for authenticateApiKey only checks lastUsedAt, so it should also pin the Prisma lookup contract. Update the test to assert that prismaMock.apiKey.findUnique is called with where using the hashed key field (keyHash derived from the raw API key) and include: { merchant: true }, while keeping the existing lastUsedAt update and merchant return assertions. This will ensure authenticateApiKey continues to look up api keys by hash rather than the raw token.
🤖 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.
Inline comments:
In `@prisma/migrations/20260627120000_add_api_key_prefix/migration.sql`:
- Around line 1-2: The migration for adding ApiKey.prefix should not assign a
blank default to existing rows, because legacy prefixes cannot be recovered from
keyHash. Update the migration and related ApiKey handling so old keys are
treated explicitly in the ApiKey model and API key list flow (for example by
making prefix nullable and handling legacy records, or by revoking/forcing
regeneration for existing keys) rather than backfilling empty strings.
In `@src/controllers/api-key.controllers.ts`:
- Around line 13-17: The api-key creation flow in the controller is silently
converting non-string label values to undefined, which lets invalid payloads
succeed. Update the request handling in the api-key controller around
createApiKey so that if req.body.label is present but not a string, the handler
returns a 400 response instead of proceeding; only pass a label through when it
is a valid string, and otherwise reject the request explicitly.
In `@src/middlewares/auth.middleware.ts`:
- Around line 64-65: The auth middleware catch blocks in auth.middleware.ts are
turning unexpected Prisma/service failures into 401 Unauthorized, which should
be reserved for real auth failures. Update the relevant catch handling in the
middleware functions to only return 401 for known authentication errors, and let
unexpected exceptions propagate to Express or convert them to a 500 response
instead. Use the existing auth-related logic in the middleware methods to
distinguish expected credential failures from infrastructure/service errors.
In `@src/routes/merchant.routes.ts`:
- Around line 18-20: The merchant API-key management routes currently use
authenticateMerchant, which now allows API-key auth and lets one key create,
list, or revoke other keys. Update the route protection in merchant.routes.ts so
the /api-keys endpoints require the interactive merchant auth path only
(JWT/session), using the existing controller names createApiKeyController,
listApiKeysController, and revokeApiKeyController but swapping in a stricter
middleware or auth guard that rejects API keys.
In `@src/services/api-key.services.ts`:
- Around line 39-54: The active API key limit check in the api-key creation flow
is not atomic, so concurrent requests can both pass the count and create steps
and exceed MAX_ACTIVE_API_KEYS. Update the create path in the api-key service to
enforce the cap within a single DB-backed transaction or lock around
countActiveApiKeys and prisma.apiKey.create, so only one request can succeed
when the merchant is at the limit. Use the api-key creation logic and related
symbols like countActiveApiKeys, generateApiKeyMaterial, and
prisma.apiKey.create to locate the fix.
In `@tests/helpers/api-key.fixtures.ts`:
- Around line 4-14: The “valid” API key fixtures are using bodies that do not
match the expected sk_live_<32-char> format, so update TEST_RAW_API_KEY and
TEST_INTEGRATION_RAW_API_KEY in api-key.fixtures.ts to be exactly 32 characters
after the prefix. Keep the related constants like TEST_KEY_PREFIX_DISPLAY and
TEST_INTEGRATION_PREFIX aligned with the new values so tests continue to reflect
a compliant key shape.
---
Nitpick comments:
In `@tests/unit/api-key.services.test.ts`:
- Around line 120-126: The revoke test only checks the 404 outcome and could
still pass if revokeApiKey ignored merchant scoping, so update this case to
assert the prismaMock.apiKey.findFirst lookup includes both id and merchantId.
Use the revokeApiKey flow in api-key.services.test.ts to verify the ownership
check contract directly, and keep the existing rejection assertion for the 404
behavior.
- Around line 128-143: The auth happy-path test for authenticateApiKey only
checks lastUsedAt, so it should also pin the Prisma lookup contract. Update the
test to assert that prismaMock.apiKey.findUnique is called with where using the
hashed key field (keyHash derived from the raw API key) and include: { merchant:
true }, while keeping the existing lastUsedAt update and merchant return
assertions. This will ensure authenticateApiKey continues to look up api keys by
hash rather than the raw token.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f907a1d5-c5d1-456d-b7c1-679750292398
📒 Files selected for processing (12)
prisma/migrations/20260627120000_add_api_key_prefix/migration.sqlprisma/schema.prismasrc/controllers/api-key.controllers.tssrc/middlewares/auth.middleware.tssrc/routes/merchant.routes.tssrc/services/api-key.services.tssrc/utils/api-key.utils.tstests/helpers/api-key.fixtures.tstests/integration/api-key.routes.test.tstests/integration/auth.middleware.test.tstests/unit/api-key.services.test.tstests/unit/api-key.utils.test.ts
Comment on lines
+4
to
+14
| export const TEST_RAW_API_KEY = `${TEST_API_KEY_PREFIX}testkey123456789012345678901234`; | ||
|
|
||
| export const TEST_KEY_PREFIX_DISPLAY = `${TEST_API_KEY_PREFIX}testkey1`; | ||
|
|
||
| export const TEST_KEY_HASH = `hash-${TEST_RAW_API_KEY}`; | ||
|
|
||
| export const TEST_INTEGRATION_RAW_API_KEY = `${TEST_API_KEY_PREFIX}integrationtest123456789012345678`; | ||
|
|
||
| export const TEST_INTEGRATION_PREFIX = `${TEST_API_KEY_PREFIX}integrat`; | ||
|
|
||
| export const TEST_UNKNOWN_RAW_API_KEY = `${TEST_API_KEY_PREFIX}unknownkey1234567890123456789012`; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the “valid” API key fixtures to use 32-character bodies.
TEST_RAW_API_KEY is 31 characters after sk_live_, while TEST_INTEGRATION_RAW_API_KEY is 33. That means several tests are exercising non-compliant keys even though the feature contract is sk_live_<32-char>, so format regressions can slip through unnoticed.
Suggested fix
-export const TEST_RAW_API_KEY = `${TEST_API_KEY_PREFIX}testkey123456789012345678901234`;
+export const TEST_RAW_API_KEY = `${TEST_API_KEY_PREFIX}testkey1234567890123456789012345`;
export const TEST_KEY_PREFIX_DISPLAY = `${TEST_API_KEY_PREFIX}testkey1`;
export const TEST_KEY_HASH = `hash-${TEST_RAW_API_KEY}`;
-export const TEST_INTEGRATION_RAW_API_KEY = `${TEST_API_KEY_PREFIX}integrationtest123456789012345678`;
+export const TEST_INTEGRATION_RAW_API_KEY = `${TEST_API_KEY_PREFIX}integrat123456789012345678901234`;
export const TEST_INTEGRATION_PREFIX = `${TEST_API_KEY_PREFIX}integrat`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export const TEST_RAW_API_KEY = `${TEST_API_KEY_PREFIX}testkey123456789012345678901234`; | |
| export const TEST_KEY_PREFIX_DISPLAY = `${TEST_API_KEY_PREFIX}testkey1`; | |
| export const TEST_KEY_HASH = `hash-${TEST_RAW_API_KEY}`; | |
| export const TEST_INTEGRATION_RAW_API_KEY = `${TEST_API_KEY_PREFIX}integrationtest123456789012345678`; | |
| export const TEST_INTEGRATION_PREFIX = `${TEST_API_KEY_PREFIX}integrat`; | |
| export const TEST_UNKNOWN_RAW_API_KEY = `${TEST_API_KEY_PREFIX}unknownkey1234567890123456789012`; | |
| export const TEST_RAW_API_KEY = `${TEST_API_KEY_PREFIX}testkey1234567890123456789012345`; | |
| export const TEST_KEY_PREFIX_DISPLAY = `${TEST_API_KEY_PREFIX}testkey1`; | |
| export const TEST_KEY_HASH = `hash-${TEST_RAW_API_KEY}`; | |
| export const TEST_INTEGRATION_RAW_API_KEY = `${TEST_API_KEY_PREFIX}integrat123456789012345678901234`; | |
| export const TEST_INTEGRATION_PREFIX = `${TEST_API_KEY_PREFIX}integrat`; | |
| export const TEST_UNKNOWN_RAW_API_KEY = `${TEST_API_KEY_PREFIX}unknownkey1234567890123456789012`; |
🤖 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 `@tests/helpers/api-key.fixtures.ts` around lines 4 - 14, The “valid” API key
fixtures are using bodies that do not match the expected sk_live_<32-char>
format, so update TEST_RAW_API_KEY and TEST_INTEGRATION_RAW_API_KEY in
api-key.fixtures.ts to be exactly 32 characters after the prefix. Keep the
related constants like TEST_KEY_PREFIX_DISPLAY and TEST_INTEGRATION_PREFIX
aligned with the new values so tests continue to reflect a compliant key shape.
Contributor
Author
|
@codebestia could you please take a look at this and give it your approval? |
Restrict key CRUD to session/JWT auth, enforce atomic key limits, validate labels, and exclude key hashes from list responses. Co-authored-by: Cursor <cursoragent@cursor.com>
codebestia
approved these changes
Jun 29, 2026
codebestia
left a comment
codebestia
left a comment
Contributor
There was a problem hiding this comment.
LGTM!
Nice Implementation.
Thank you for your contribution.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
closes #14
Test plan
Summary by CodeRabbit