feat: implement multi-user auth, usage attribution, UI, and MVP hardening#123
feat: implement multi-user auth, usage attribution, UI, and MVP hardening#123
Conversation
|
Consolidation update: I moved all follow-up hardening work from #124-#127 into this branch so review stays in one place. Included in this PR now:
Verification on consolidated branch:
tests/test_api_keys.py::test_create_list_revoke_api_key_hides_plaintext_at_rest tests/test_auth.py::test_auth_mode_matrix[users-user-token-user_api_key] tests/test_auth.py::test_auth_mode_matrix[users-user-token-user_api_key] tests/test_auth.py::test_legacy_token_hash_opportunistic_migration tests/test_usage_retention.py::test_prune_usage_events_removes_old_rows tests/test_usage_retention.py::test_prune_usage_events_removes_old_rows -- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html |
|
Correction (escaped formatting): consolidated hardening from #124-#127 is now included here in #123. Included in this PR now:
Verification on consolidated branch:
|
Mirrowel
added
enhancement
|
I'm starting a comprehensive review of the multi-user auth and usage attribution implementation. This is a significant update with many moving parts (auth modes, SQLite persistence, and a new dashboard), so I'll be taking a close look at the security and reliability aspects. Back soon with the full report! |
![mirrobot-agent[bot]](https://avatars.githubusercontent.com/in/2140342?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overall Assessment
This is a robust implementation of multi-user authentication and usage attribution. The addition of a server-rendered dashboard, session management, and CSRF protection significantly hardens the proxy for multi-user environments. The use of an async batch recorder for usage events is a well-considered performance choice.
Architectural Feedback
- Auth Modes: The
AUTH_MODEtransition strategy (users,legacy,both) is excellent for ensuring backward compatibility while migrating to a more secure model. - SQLite Hardening: Applying WAL mode and other pragmas shows good attention to SQLite's performance characteristics in an async environment.
- Security: The proactive validation of secrets in production and the comprehensive redaction logic in the request logger are high-value additions.
Key Suggestions
- Timestamp Deprecation: Update
datetime.utcnow()todatetime.now(datetime.UTC)to align with modern Python standards and resolve the deprecation warnings noted in your verification. - Error Handling Refactor: Consider centralizing the repeated
litellmexception blocks inmain.pyto keep the endpoint logic cleaner. - Usage Durability: While the async recorder is efficient, adding a retry for flush failures would make usage attribution more reliable under load.
Nitpicks and Minor Points
- Tying the
Securecookie flag to theis_prod()check would be a safer default thanfalse. - A small amount of code duplication exists between the database initialization functions.
Questions for the Author
- None
This review was generated by an AI assistant.
| password_hash: Mapped[str] = mapped_column(Text) | ||
| role: Mapped[str] = mapped_column(String(16), default="user") | ||
| is_active: Mapped[bool] = mapped_column(Boolean, default=True) | ||
| created_at: Mapped[datetime] = mapped_column(DateTime, default=datetime.utcnow) |
There was a problem hiding this comment.
datetime.utcnow() is deprecated since Python 3.12. Consider using datetime.now(datetime.UTC) to ensure timezone-aware objects and avoid future compatibility issues. This pattern appears in several files (e.g., auth_api.py, user_api.py).
|
|
||
|
|
||
| def _get_cookie_secure() -> bool: | ||
| return os.getenv("SESSION_COOKIE_SECURE", "false").lower() in { |
There was a problem hiding this comment.
The Secure flag for session cookies currently defaults to false. For production deployments (APP_ENV=prod), it is highly recommended to default this to true to ensure session tokens are only transmitted over HTTPS.
| legacy_key = ( | ||
| get_legacy_master_key() if legacy_master_key is None else legacy_master_key | ||
| ) | ||
| if legacy_key and token == legacy_key: |
There was a problem hiding this comment.
Consider using hmac.compare_digest(token, legacy_key) here to prevent potential timing attacks when validating the legacy master key.
| session.add_all(rows) | ||
| await session.commit() | ||
| except Exception: | ||
| logger.exception("Failed to flush %d usage events", len(batch)) |
There was a problem hiding this comment.
If _flush_batch fails (e.g., due to a transient database lock), the current batch of usage events is logged and then discarded. For more reliable usage attribution, you might consider a retry mechanism or a temporary fallback storage.
| return session_maker | ||
|
|
||
|
|
||
| async def init_db_runtime( |
There was a problem hiding this comment.
init_db_runtime shares most of its implementation with init_db. Consider refactoring to reduce duplication, perhaps by having init_db return the engine as well.
| usage=None, | ||
| error=e, | ||
| ) | ||
| raise HTTPException(status_code=400, detail=f"Invalid Request: {_safe_error_message(e, default='Invalid request')}") |
There was a problem hiding this comment.
The detailed litellm exception handling logic is repeated across several endpoints (chat_completions, anthropic_messages, embeddings). Refactoring this into a shared utility or a custom exception handler would significantly reduce boilerplate and improve maintainability.
There was a problem hiding this comment.
Follow-up Review
I've reviewed the latest changes, which primarily consist of the addition of the PROJECT_OVERVIEW.md file. This document provides a solid high-level summary of the project's goals, especially regarding multi-user access control and security hardening.
Assessment of New Changes:
- PROJECT_OVERVIEW.md: The overview is clear and accurately reflects the architectural direction of the proxy. It's a great addition for maintainability and onboarding.
Overall Status:
Since there have been no other changes since my previous review, all prior feedback remains applicable. To ensure the "MVP hardening" goals are fully met, please consider addressing the following unresolved points:
- Timestamp Deprecation: Replace
datetime.utcnow()withdatetime.now(datetime.UTC)across the codebase (db_models.py,user_api.py,ui.py, etc.). - Secure Cookie Defaults: Default the
Secureflag for session cookies totruewhenis_prod()is active. - Timing Attack Protection: Use
hmac.compare_digestfor legacy key comparisons inapi_token_auth.py. - Usage Recorder Reliability: Implement a retry mechanism for
_flush_batchfailures inusage_recorder.pyto prevent data loss during transient database locks. - Refactoring: Reduce code duplication in
db.py(initialization logic) andmain.py(exception handling).
I'm happy to re-review once these hardening points are addressed!
This review was generated by an AI assistant.
2 participants
Summary
AUTH_MODE=users|legacy|both), and role-based admin/user APIs/v1/*endpoints to actor-based auth while preserving OpenAI and Anthropic compatibility, and emit usage attribution for non-stream and streaming paths (including fallback request-count events)/app/dataSQLite volume and migration guidance from legacyPROXY_API_KEYauth to user API keysVerification
PYTHONPATH=src .venv/bin/python -m compileall src/proxy_app scriptsPYTHONPATH=src .venv/bin/pytest -qdocker compose -f docker-compose.dev.yml config127.0.0.1:8011(admin/user login, user key creation, auth-mode checks, usage summary endpoints)Important
Implements multi-user authentication, usage attribution, and UI enhancements with async SQLite persistence, session and API key-based authentication, and role-based access control.
db.pyanddb_models.py.api_token_auth.pyandauth.py.AUTH_MODEforusers,legacy, andbothmodes.usage_recorder.pyandusage_queries.py.routers/ui.pyandtemplates/.tests/test_auth.py,tests/test_api_keys.py, andtests/test_usage_attribution.py.This description was created by
for 250cc6c. You can customize this summary. It will automatically update as commits are pushed.