Resolved git issue #123

[bryonbaker]

Added feature to suppress showing API keys where there is no subscription. This resolves git issue #123

[guimou]

PR #124 Review

Summary

This PR adds an "archive" concept for API keys. When a subscription is cancelled and an API key has no remaining models, the key is deactivated and archived (hidden from user view but preserved for audit). New archive/unarchive endpoints are added, and the frontend shows archived status.

---

Issues Found

1. Critical: Race condition in removeModelFromUserKeys auto-archive

api-key.service.ts:2207-2215 — The archive query uses AND id NOT IN (SELECT DISTINCT api_key_id FROM api_key_models) but runs after the DELETE FROM api_key_models above. This means ALL keys that had only that one model deleted will match. However, the successfulKeyIds filter means it only targets keys whose LiteLLM update succeeded — but the previous DELETE already removed the api_key_models row for ALL keys (line 2194), not just successful ones. If a LiteLLM update fails for key X, the model is still removed from the junction table, but the archive won't trigger, leaving key X in an inconsistent state (active with no models, not archived).

Recommendation: The DELETE FROM api_key_models should also be scoped to successfulKeyIds, or the archive logic needs to handle all orphaned keys regardless of LiteLLM status.

2. Security: Missing authorization check on archive/unarchive routes

api-keys.ts:757-822 — The archive and unarchive endpoints only use authenticateWithDevBypass but lack any RBAC permission check. While the service layer checks user_id = $2, admins should arguably be able to archive keys for any user, and there's no admin-specific archive route. More importantly, the pattern in this codebase typically uses explicit permission middleware for admin-level operations.

3. Bug: archiveApiKey returns new Date().toISOString() instead of DB value

api-keys.ts:770-773 — The route handler returns archivedAt: new Date().toISOString() rather than the actual CURRENT_TIMESTAMP set by the database. This could be slightly different due to clock skew between app and DB.

4. Missing: archived_at not included in SELECT queries

api-key.service.ts — The getUserApiKeys method adds filtering on archived_at IS NULL, but I don't see archived_at being added to the SELECT column list in the query. The mapApiKeyFromDb method at line 2045 maps it, but only if it's actually selected. Need to verify the SELECT includes ak.archived_at.

5. Migration backfill may be too aggressive

database-migrations.ts:872-878 — The backfill query archives ALL inactive keys with no api_key_models entries. This could incorrectly archive keys that were intentionally deactivated (revoked) but should remain visible to users in their key history. The WHERE is_active = false condition catches revoked keys too.

6. Inconsistent status color mapping between admin and user views

• UserApiKeysTab.tsx:650: Archived = orange
• ApiKeysPage.tsx:403: Archived = grey

These should be consistent.

7. Missing: No admin route for archive/unarchive

The admin user management routes (admin-users.ts) expose includeArchived: true for viewing, but there's no admin endpoint to archive/unarchive keys on behalf of users. The current /api-keys/:id/archive only works for the key owner (user_id = $2).

8. Frontend: Archive/unarchive API methods defined but never called

apiKeys.service.ts:215-221 — The archiveApiKey and unarchiveApiKey methods are added to the service but no UI button or action triggers them. The archive only happens automatically server-side. These are dead code unless there's a planned follow-up.

9. No tests included

No unit or integration tests for the new archive/unarchive functionality. Given the service and route changes, tests for the following are expected:

• archiveApiKey / unarchiveApiKey service methods
• Auto-archive on subscription cancellation
• includeArchived filtering behavior
• Archive/unarchive route handlers

10. Standalone migration script may diverge

migrate-archived-keys.ts duplicates the migration logic that's also in database-migrations.ts. If the migration DDL changes, both files need updating. Consider having the script call the existing migration function or documenting the relationship.

---

Minor Notes

• The i18n entries are only added for English — the project supports 9 languages, so translations for the other 8 are missing (though this is often done as a follow-up).
• Audit log action names API_KEY_ARCHIVED / API_KEY_UNARCHIVED should be documented or added to the audit action enum/constants if one exists.

---

Verdict

The feature concept is sound — archiving orphaned keys rather than leaving them visible is good UX. However, the PR needs:

1. Fix the race condition in removeModelFromUserKeys (critical)
2. Add tests for the new functionality
3. Resolve the inconsistent color mapping between views
4. Verify archived_at is in the SELECT clause of getUserApiKeys
5. Consider the backfill scope — should revoked keys really be auto-archived?

[guimou]

@bryonbaker Do you want me/Claude to take care of the fixes suggested?

[bryonbaker]

Hey Guimou, Let me have a crack at it. Great way to lean the code base. Rookie mistake leaving out test cases too. Oops. ========================== Bryon Baker Ph.: +61(0)457531202 Email: ***@***.***

…

On Fri, 27 Mar 2026 at 1:02 am, Guillaume Moutier ***@***.***> wrote: *guimou* left a comment (rh-aiservices-bu/litemaas#124) <#124?email_source=notifications&email_token=ACHUARVWAJ5UMRG4VHU7OKT4SU2AVA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMJTGUYTCNBQGU4KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJNLQOJPWG33NNVSW45C7N5YGK3S7MNWGSY3L#issuecomment-4135114058> @bryonbaker <https://github.com/bryonbaker> Do you want me/Claude to take care of the fixes suggested? — Reply to this email directly, view it on GitHub <#124?email_source=notifications&email_token=ACHUARVWAJ5UMRG4VHU7OKT4SU2AVA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMJTGUYTCNBQGU4KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJNLQOJPWG33NNVSW45C7N5YGK3S7MNWGSY3L#issuecomment-4135114058>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACHUARVRDUCR5JAASFW4KYT4SU2AVAVCNFSM6AAAAACW6RZYJCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DCMZVGEYTIMBVHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>