ADR: Per-User API Keys for Multi-Tenant SaaS Support

[StuMason]

Summary

This PR proposes an architecture decision for implementing per-user API keys to support multi-tenant SaaS deployments.

Problem

Currently, a single API key grants access to ALL users' data. If compromised in a SaaS context, it's a total data breach.

Solution

Each user gets their own API key, scoped to only their data:

• Key generated on OAuth completion
• Returned to calling app (Laravel) for storage
• All API calls validated against user_id in path
• Compromised key = single user affected

Key Changes Proposed

1. Add api_key_hash, api_key_prefix columns to User model
2. Generate API key during OAuth callback
3. New auth guard that validates key ownership
4. Backward compatible with existing single API key mode

Test plan

• Review ADR for architectural soundness
• Identify any security concerns
• Validate backward compatibility approach
• Confirm implementation plan is complete

🤖 Generated with Claude Code

[StuMason]

@claude please rereview

[claude]

Claude finished @StuMason's task in 2m 0s —— View job

---

Re-review Complete: Per-User API Keys

I've completed a comprehensive re-review of this PR. Overall, the implementation is well-architected and production-ready with strong security foundations. Here's my detailed analysis:

---

✅ Strengths

1. Excellent ADR Documentation

• The ADR is comprehensive, well-structured, and clearly articulates the problem, decision, and consequences
• The two-step OAuth flow (temp code → API key exchange) is a critical security improvement that prevents API keys from appearing in URLs
• Good use of diagrams and code examples

2. Solid Security Design

• ✅ SHA-256 hashing for API keys
• ✅ Constant-time comparison to prevent timing attacks (secrets.compare_digest in auth.py:254)
• ✅ Rate limiting implemented at the key level
• ✅ Proper foreign key cascades (CASCADE on user deletion)
• ✅ Single-use temporary codes with 5-minute expiration
• ✅ Key prefix for identification without exposing full key

3. Clean Database Schema

• Migration in alembic/versions/c4d5e6f7g8h9_add_per_user_api_keys.py is well-structured
• Proper indexes on key_hash, user_id, key_prefix, is_active
• Server defaults for rate limiting columns ensure safe deployment
• Good separation: APIKey model extended, TempAuthCode model added

4. Backward Compatibility

• Existing API_KEY env var mode preserved (auth.py:194-197, 254-256)
• Service-level keys (user_id=NULL) maintain full access pattern
• Self-hosted deployments unaffected

---

⚠️ Issues Requiring Attention

CRITICAL: Missing Per-User Guard on Data Endpoints

The existing data endpoints (data_router, sleep_router) still use the old api_key_guard (data.py:573, sleep.py:144) instead of per_user_api_key_guard.

Security Impact:

• User-scoped keys won't have their scope validated
• Rate limiting won't be enforced
• The core security benefit of this PR is not applied to the main data endpoints

Evidence:

# src/polar_flow_server/api/data.py:571-573
data_router = Router(
    path="/api/v1",
    guards=[api_key_guard],  # ❌ Should be per_user_api_key_guard

# src/polar_flow_server/api/sleep.py:142-144
sleep_router = Router(
    path="/api/v1",
    guards=[api_key_guard],  # ❌ Should be per_user_api_key_guard

Recommendation:
Replace api_key_guard with per_user_api_key_guard on all user data routers. The new guard already handles backward compatibility (it checks config-based keys first at auth.py:254-256).

Fix data router guard →

Fix sleep router guard →

---

HIGH: Duplicate Key Generation Logic

There are two implementations of API key generation:

1. core/api_keys.py:41-64 - New, preferred implementation
2. core/auth.py:46-57 - Legacy implementation with different prefix (pfs_ vs pfk_)

Issues:

• Keys generated via auth.py:generate_api_key() won't match the pfk_ prefix convention
• Inconsistent key length (64 hex chars vs 40 hex chars)
• Potential confusion for maintainers

Current Usage:

• ✅ New OAuth flow uses core/api_keys.py (correct)
• ❓ Legacy auth.py function appears unused but still present

Recommendation:
Remove or deprecate the legacy generate_api_key() function in auth.py:46-57. If needed for backward compatibility, make it call the new implementation.

Remove duplicate function →

---

MEDIUM: Rate Limit Headers Missing from Response

The per_user_api_key_guard stores rate limit info in connection.state[RATE_LIMIT_STATE_KEY] (auth.py:275) and provides get_rate_limit_headers() helper (auth.py:306-323), but no response middleware adds these headers.

Impact:

• Clients can't see their rate limit status
• Unclear when they'll be reset
• Reduces API usability

Recommendation:
Add a response middleware or use Litestar's after_response hook to inject rate limit headers on all authenticated endpoints.

Example:

@litestar.after_response
async def add_rate_limit_headers(response: Response, connection: ASGIConnection) -> Response:
    headers = get_rate_limit_headers(connection)
    response.headers.update(headers)
    return response

Add rate limit headers middleware →

---

MEDIUM: In-Memory OAuth State Storage

The SaaS OAuth flow stores state in a module-level dict: _saas_oauth_states (keys.py:40).

Issues:

• Not horizontally scalable - won't work with multiple server instances
• Lost on restart - in-flight OAuth flows will fail
• Memory leak potential - expired states cleaned up only during new OAuth requests (keys.py:140-144)

Recommendation for Production:

• Use Redis or database for state storage in SaaS mode
• Add periodic cleanup job for expired states
• Document this limitation in the ADR

Acceptable for MVP: This is fine for single-instance deployments and early SaaS testing, but should be flagged as a known limitation.

---

LOW: Missing Test Coverage

Based on file changes, there are no new tests for:

• Per-user API key validation
• Rate limiting logic
• Temp code exchange flow
• User scope enforcement
• CSRF attacks on OAuth state

Recommendation:
Add integration tests covering at minimum:

1. User-scoped key can only access own data (✅ pass)
2. User-scoped key cannot access other user's data (❌ 403)
3. Rate limiting works correctly (✅ succeed → ❌ 429 → ✅ succeed after reset)
4. Temp code is single-use (❌ second exchange fails)
5. Expired temp code cannot be exchanged

---

🔍 Code Quality Observations

Good Patterns:

• Clean separation of concerns (core/api_keys.py vs core/auth.py)
• Proper use of NamedTuple for return values (api_keys.py:26-39)
• Good use of TYPE_CHECKING to avoid circular imports (api_key.py:6, user.py:11)
• Relationships properly configured with back_populates and cascade

Minor Suggestions:

1. api_keys.py:366 - Type annotation CursorResult[tuple[()]] is a workaround for mypy. Consider adding a # type: ignore comment explaining why.

2. keys.py:40 - Consider moving _saas_oauth_states to a class or singleton for better testability

3. auth.py:85 - The noqa: E712 comment is needed because SQLAlchemy requires == True instead of is True. Good to document this.

---

📊 Implementation Completeness

Checking against ADR Phase checklist:

Phase	Status	Notes
Phase 1: Database & Rate Limiting	✅ Complete	Migration, models, rate limiting implemented
Phase 2: OAuth Integration	✅ Complete	Temp codes, exchange endpoint working
Phase 3: Authentication Guard	⚠️ Partial	Guard implemented but not applied to data endpoints
Phase 4: Key Management	✅ Complete	Regenerate, revoke, status endpoints present
Phase 5: Admin Dashboard	❓ Unknown	Not reviewed in this PR
Phase 6: Testing	❌ Missing	No test files in changeset
Phase 7: Documentation	✅ Complete	Excellent ADR

---

🎯 Summary & Recommendation

Overall Assessment: This is high-quality work with strong architectural foundations. The ADR is exemplary, the security design is sound, and the code is clean.

Critical Path to Merge:

1. ✅ MUST FIX: Apply per_user_api_key_guard to data/sleep routers
2. ⚠️ SHOULD FIX: Remove duplicate key generation in auth.py
3. ⚠️ SHOULD FIX: Add rate limit headers to responses
4. 📝 NICE TO HAVE: Add integration tests
5. 📝 NICE TO HAVE: Document OAuth state storage limitation

Verdict: Approve after addressing item #1 (critical guard replacement). Items #2-3 can be follow-up PRs if needed, but #1 is essential for the security model to work correctly.

---