Conversation
BREAKING CHANGES:
- Room creation is now separate from joining:
- POST /api/rooms creates a new room (returns 201)
- POST /api/rooms/{id}/join joins existing rooms only (returns 404 if not exists)
- Password hashing upgraded from SHA-256 to Argon2id (existing passwords invalidated)
- JWT tokens now have 7-day expiration
Key changes:
- Add ensure_user_exists() to UserService for idempotent user creation
- Users are created on socket connect from JWT claims, not during login
- JWT is now the single source of truth for identity
- Simplified login endpoint - guests get JWT without Redis entry until socket connect
- Added argon2-cffi dependency for secure password hashing
- Frontend: Removed redundant role state, use getRoleFromToken() only
- Updated all tests to use create-then-join pattern
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This refactoring enforces strict single responsibility per endpoint: - /api/user/register: ONLY place for user creation (guest or registered) - /api/login: ONLY authenticates users (never creates) - Socket connect and room join validate user exists Key changes: - Add password validation (minimum 8 characters) - Add get_authoritative_role() for Redis-based role checks - Add is_admin_username() to AdminService for proper error messages - Replace deprecated datetime.utcnow() with time utilities - Remove redundant USERNAME_KEY from frontend localStorage - Update Python client to use register-then-login flow All users must now register before login. Guest flow: 1. POST /api/user/register (creates user) 2. POST /api/login (authenticates, returns JWT) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Security fixes: - Unify all auth error messages to "Authentication failed" to prevent username enumeration attacks - Add username validation (1-64 chars, alphanumeric + underscore/hyphen) to prevent path injection, XSS, and DoS attacks Bug fixes: - Fix 409 handling in frontend auth.ts - now properly handles case where registerData.userName is undefined on conflict response Code cleanup: - Remove unused get_authoritative_role() function (YAGNI) - Update get_current_user_role() docstring to reference AdminService Test updates: - Update tests to expect generic "Authentication failed" message - Fix test usernames to comply with new validation (John Doe → john-doe) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughRefactors auth to use JWT claims (iat/exp) as source-of-truth, adds explicit POST /api/rooms create endpoint and client join-or-create flow, migrates password hashing to Argon2id with username/password validation, and centralizes UTC time utilities across backend modules. Changes
*Examples of files updated to use time helpers: Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthAPI as Auth Routes
participant UserSvc as User Service
participant JWTAuth as JWT Creator
participant Redis
Note over Client,Redis: Guest register-then-login flow
Client->>AuthAPI: POST /api/user/register {userName?}
AuthAPI->>UserSvc: ensure_user_exists(userName)
UserSvc->>Redis: create or return user entry
UserSvc-->>AuthAPI: userName
AuthAPI->>JWTAuth: create_jwt_token(userName, role)
JWTAuth-->>AuthAPI: token (with iat, exp, jti)
AuthAPI-->>Client: 201 {token, userName, role}
sequenceDiagram
participant Client
participant RoomAPI as Room Routes
participant RoomSvc as Room Service
participant UserSvc as User Service
participant Redis
Note over Client,Redis: Create → Join-or-Create flow
alt Client explicit create
Client->>RoomAPI: POST /api/rooms {roomId, copyFrom?}
RoomAPI->>UserSvc: user_exists(currentUser)
UserSvc-->>RoomAPI: true
RoomAPI->>RoomSvc: create_room(roomId, copyFrom)
RoomSvc->>Redis: store room (+copy frames)
RoomSvc-->>RoomAPI: room created
RoomAPI-->>Client: 201 {status, roomId}
else Client join (join-or-create client)
Client->>RoomAPI: POST /api/rooms/{roomId}/join
RoomAPI->>RoomSvc: validate room exists
RoomSvc->>Redis: fetch room
alt room exists
RoomAPI->>Redis: create session (connectedAt via utc_now_iso)
RoomAPI-->>Client: 200 {status, sessionId, roomId}
else missing
RoomAPI-->>Client: 404 Room not found
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/zndraw/services/admin_service.py (1)
61-79: Use constant-time comparison for credential validation.The method uses standard
==operators to compare both username and password against configured values. Standard string equality comparison short-circuits on the first character difference, which makes it vulnerable to timing attacks. An attacker could measure response times to gradually learn the correct credentials character by character.Use
secrets.compare_digest()for constant-time comparison of security-sensitive strings.🔒 Proposed fix using secrets.compare_digest
+import secrets + """Admin access control service. Manages admin user privileges and deployment mode detection.def validate_admin_credentials(self, username: str, password: str) -> bool: """Validate admin credentials against configured values. Parameters ---------- username : str Username to validate password : str Password to validate Returns ------- bool True if credentials match admin configuration """ if not self._deployment_mode: return False - return username == self._admin_username and password == self._admin_password + username_match = secrets.compare_digest( + username.encode('utf-8'), + (self._admin_username or '').encode('utf-8') + ) + password_match = secrets.compare_digest( + password.encode('utf-8'), + (self._admin_password or '').encode('utf-8') + ) + return username_match and password_matchsrc/zndraw/services/user_service.py (1)
492-518: Missing cleanup ofvisited_roomskey leaves orphaned data.The
UserKeysclass defines avisited_rooms()key (persrc/zndraw/app/redis_keys.py), butdelete_userdoesn't delete it. This leaves orphaned data in Redis after user deletion.🐛 Proposed fix
def delete_user(self, user_name: str) -> bool: keys = UserKeys(user_name) pipe = self.r.pipeline() pipe.delete(keys.hash_key()) pipe.delete(keys.admin_key()) + pipe.delete(keys.visited_rooms()) # Remove from indices pipe.srem(GlobalIndexKeys.users_index(), user_name) pipe.srem(GlobalIndexKeys.admins_index(), user_name) pipe.execute()
🤖 Fix all issues with AI agents
In @pyproject.toml:
- Line 14: Update the argon2-cffi dependency version in pyproject.toml by
replacing the existing requirement string "argon2-cffi>=23.1.0" with
"argon2-cffi>=25.1.0"; edit the dependency line so the project will install the
newer 25.1.0+ release and then run your dependency install/update (e.g., poetry
install or pip-tools refresh) and tests to verify compatibility.
In @tests/test_auth.py:
- Around line 130-150: The test test_expired_jwt_token_raises_error uses
deprecated datetime.datetime.utcnow(); import utc_now from zndraw.utils.time and
replace both datetime.datetime.utcnow() calls in that test with utc_now() so the
iat and exp fields are set using utc_now() - ensure you add the import for
utc_now at the top of tests/test_auth.py and remove or keep the datetime import
only if still needed elsewhere.
🧹 Nitpick comments (16)
tests/test_zndraw_admin.py (1)
161-161: Consider using a raw string for the regex pattern.While the current pattern works, best practice for regex patterns in
pytest.raisesis to use raw strings to avoid potential issues with escape sequences.♻️ Proposed fix
- with pytest.raises(RuntimeError, match="Login failed.*Authentication failed"): + with pytest.raises(RuntimeError, match=r"Login failed.*Authentication failed"):tests/test_partial_frame_update.py (1)
466-469: Consider adding timeout parameter for production-grade tests.While this is test code, adding
timeout=5to therequests.post()call would prevent tests from hanging indefinitely if the server becomes unresponsive.♻️ Proposed refinement
create_response = requests.post( - f"{server}/api/rooms", json={"roomId": room}, headers=auth_headers + f"{server}/api/rooms", json={"roomId": room}, headers=auth_headers, timeout=5 )tests/test_chat.py (1)
14-17: Consider adding timeout parameter for production-grade tests.While this is test code, adding
timeout=5to bothrequests.post()calls would prevent tests from hanging indefinitely if the server becomes unresponsive.♻️ Proposed refinement
create_response = requests.post( - f"{server}/api/rooms", json={"roomId": room}, headers=headers + f"{server}/api/rooms", json={"roomId": room}, headers=headers, timeout=5 ) assert create_response.status_code == 201 # Join room response = requests.post( - f"{server}/api/rooms/{room}/join", json={}, headers=headers + f"{server}/api/rooms/{room}/join", json={}, headers=headers, timeout=5 )Also applies to: 20-21
tests/test_auth.py (1)
104-127: Move datetime import to module level.The
datetimeimport on line 106 should be at the top of the file per coding guidelines. Consider importing it alongside other imports at the module level.♻️ Proposed refactor
Move the import to the top of the file (after line 4):
import jwt as pyjwt import pytest +import datetime from zndraw.auth import (And remove the local import:
def test_jwt_token_has_expiration(app): """Test that JWT tokens have a 7-day expiration claim.""" - import datetime - with app.app_context():tests/conftest.py (2)
57-89: LGTM! Register-then-login flow correctly implemented.The updated authentication flow properly registers the user before logging in, aligning with the new API design. The 409 handling for existing users enables test reusability.
Minor: Consider adding
timeoutparameter torequests.postcalls to prevent tests from hanging indefinitely if the server becomes unresponsive.
92-151: Room create-then-join logic is sound.The helper correctly handles the 404 → create → join flow. However, consider adding timeouts to prevent test hangs.
♻️ Optional: Add timeouts to requests calls
def _join_room_and_get_headers( server: str, room_id: str, user: str = "test-user" ) -> dict: ... # Try to join existing room first response = requests.post( f"{server}/api/rooms/{room_id}/join", json={}, headers=auth_headers, + timeout=10, ) # If room doesn't exist (404), create it then join if response.status_code == 404: create_response = requests.post( f"{server}/api/rooms", json={"roomId": room_id}, headers=auth_headers, + timeout=10, ) ... # Now join the newly created room response = requests.post( f"{server}/api/rooms/{room_id}/join", json={}, headers=auth_headers, + timeout=10, )tests/test_step_endpoints.py (2)
8-24: Consolidate duplicated_create_and_join_roomhelper intoconftest.py.This exact helper function is duplicated in
test_lock_socket_events.pyandtest_requires_lock_decorator.py. Consider moving it toconftest.pyand exposing it as a fixture or importable function to follow DRY principles.Based on the relevant code snippets,
conftest.pyalready has_join_room_and_get_headerswhich handles create-on-404. You could either:
- Reuse
_join_room_and_get_headersin tests, or- Add a simpler
_create_and_join_roomtoconftest.pyand remove the duplicates
51-67: Test name is misleading.The function is named
test_get_step_without_authbut it actually tests with authentication (auth_headers). Consider renaming totest_get_step_returns_data_for_existing_roomto match the docstring.tests/test_user_service.py (1)
531-561: Consider usingpytest.mark.parametrizeto reduce test setup duplication.Per coding guidelines, parametrize can help avoid code duplication. The password validation tests follow a similar pattern.
♻️ Refactor suggestion using parametrize
@pytest.mark.parametrize( "password,should_fail,match_pattern", [ ("1", True, "at least 8 characters"), (" ", True, "at least 8 characters"), ("validpass123", False, None), ("x" * 1000, False, None), ], ) def test_password_requirements_enforced(user_service, password, should_fail, match_pattern): """Test that password requirements are enforced.""" from zndraw.services.user_service import PasswordValidationError user_name = f"test-user-{uuid.uuid4().hex[:8]}" user_service.create_user(user_name) if should_fail: with pytest.raises(PasswordValidationError, match=match_pattern): user_service.register_user(user_name, user_name, password) else: user_service.register_user(user_name, user_name, password) assert user_service.verify_password(user_name, password)src/zndraw/app/room_routes.py (1)
135-208: LGTM! Clean implementation of explicit room creation endpoint.Good separation of concerns:
- Validates
roomIdpresence and format- Checks for duplicates (409)
- Supports optional
copyFromfor cloning rooms- Broadcasts creation via socket
- Returns 201 on success
One minor concern: Line 187 returns 404 for
ValueError, but this could mask errors beyond "source room not found" (e.g., validation errors fromcopy_from). Consider more specific error handling.♻️ Optional: More specific error handling
try: result = room_service.create_room(room_id, user_name, description, copy_from) except ValueError as e: - return {"error": str(e)}, 404 + error_msg = str(e) + # 404 only for "not found" errors, 400 for other validation errors + if "not found" in error_msg.lower() or "does not exist" in error_msg.lower(): + return {"error": error_msg}, 404 + return {"error": error_msg}, 400tests/test_auth_integration.py (1)
10-34: Add timeout parameter to requests calls for test reliability.Multiple
requests.postandrequests.getcalls throughout this file lack thetimeoutparameter. This could cause tests to hang indefinitely if the server becomes unresponsive. Static analysis (Ruff S113) flagged this across ~19 locations.Consider adding a consistent timeout (e.g.,
timeout=10) to all requests calls in this file for consistency withtest_lazy_room_loading.py.Example fix for the first few occurrences
# Step 1: Register user register_response = requests.post( - f"{server}/api/user/register", json={"userName": "test-user"} + f"{server}/api/user/register", json={"userName": "test-user"}, timeout=10 ) ... # Step 2: Login to get JWT - response = requests.post(f"{server}/api/login", json={"userName": "test-user"}) + response = requests.post(f"{server}/api/login", json={"userName": "test-user"}, timeout=10)src/zndraw/api_manager.py (2)
233-240: Add timeout to requests.post call.The
create_roommethod's POST request lacks a timeout, which could cause the client to hang indefinitely.Proposed fix
- response = requests.post(f"{self.url}/api/rooms", json=payload, headers=headers) + response = requests.post(f"{self.url}/api/rooms", json=payload, headers=headers, timeout=10.0)
265-278: Add timeout to requests.post calls in join_room.Both POST requests in
join_room(lines 266 and 276) lack timeouts.Proposed fix
# Try to join existing room first response = requests.post( - f"{self.url}/api/rooms/{self.room}/join", json={}, headers=headers + f"{self.url}/api/rooms/{self.room}/join", json={}, headers=headers, timeout=10.0 ) # If room doesn't exist (404), create it then join if response.status_code == 404: log.info(f"Room '{self.room}' doesn't exist, creating it...") self.create_room(description=description, copy_from=copy_from) # Now join the newly created room response = requests.post( - f"{self.url}/api/rooms/{self.room}/join", json={}, headers=headers + f"{self.url}/api/rooms/{self.room}/join", json={}, headers=headers, timeout=10.0 )src/zndraw/app/utility_routes.py (2)
324-326: Consider using logging.exception for full traceback.When catching the generic
Exception, usinglog.exception()instead oflog.error()will include the stack trace, which is helpful for debugging unexpected errors.Proposed fix
except Exception as e: - log.error(f"Error registering user: {e}") + log.exception(f"Error registering user: {e}") return {"error": "Registration failed", "type": "ServerError"}, 500
390-392: Same suggestion: use logging.exception for upgrade_user.Proposed fix
except Exception as e: - log.error(f"Error upgrading user: {e}") + log.exception(f"Error upgrading user: {e}") return {"error": "Upgrade failed", "type": "ServerError"}, 500src/zndraw/services/user_service.py (1)
232-268: Potential TOCTOU race condition in user creation.The check-then-act pattern between
username_exists()and the pipeline execution isn't atomic. Two concurrent calls with the same username could both pass the existence check, resulting in the second call silently overwriting the first user's timestamps.For auto-generated guest usernames with UUIDs, this is extremely unlikely. However, if callers provide explicit usernames, this could cause subtle data corruption.
Consider using
HSETNXfor the initial field or a Lua script for true atomicity if explicit usernames are supported:♻️ Atomic alternative using HSETNX
def ensure_user_exists(self, user_name: str) -> bool: - if self.username_exists(user_name): - return False - keys = UserKeys(user_name) current_time = utc_now_iso() - pipe = self.r.pipeline() - pipe.hset( - keys.hash_key(), - mapping={ - "userName": user_name, - "createdAt": current_time, - "lastLogin": current_time, - }, - ) - pipe.sadd(GlobalIndexKeys.users_index(), user_name) - pipe.execute() + # HSETNX returns 1 if field was set, 0 if it existed + created = self.r.hsetnx(keys.hash_key(), "userName", user_name) + if not created: + return False + + # User was just created, set remaining fields + pipe = self.r.pipeline() + pipe.hset(keys.hash_key(), mapping={ + "createdAt": current_time, + "lastLogin": current_time, + }) + pipe.sadd(GlobalIndexKeys.users_index(), user_name) + pipe.execute() log.info(f"Created guest user {user_name}") return True
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
app/src/hooks/useRestManager.tsapp/src/myapi/client.tsapp/src/utils/auth.tspyproject.tomlsrc/zndraw/api_manager.pysrc/zndraw/app/events.pysrc/zndraw/app/extension_routes.pysrc/zndraw/app/job_dispatcher.pysrc/zndraw/app/job_manager.pysrc/zndraw/app/lock_routes.pysrc/zndraw/app/room_routes.pysrc/zndraw/app/utility_routes.pysrc/zndraw/app/worker_routes.pysrc/zndraw/auth.pysrc/zndraw/services/admin_service.pysrc/zndraw/services/user_service.pysrc/zndraw/utils/__init__.pysrc/zndraw/utils/time.pytests/conftest.pytests/test_admin_auth.pytests/test_auth.pytests/test_auth_integration.pytests/test_chat.pytests/test_lazy_room_loading.pytests/test_lock_socket_events.pytests/test_partial_frame_update.pytests/test_requires_lock_decorator.pytests/test_room_lock_enforcement.pytests/test_step_endpoints.pytests/test_user_service.pytests/test_vis_room.pytests/test_zndraw_admin.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: If sensible, implement collections.abc interfaces for classes, such as MutableMapping or MutableSequence
Use numpy style docstrings
Docstrings must be concise and to the point
Use type hints wherever possible. Importtyping as tif necessary, but uselist[int|float] | Noneinstead oft.Optional[t.List[int|float]]
Imports should always be at the top of the file
Files:
tests/test_room_lock_enforcement.pysrc/zndraw/utils/__init__.pysrc/zndraw/app/events.pysrc/zndraw/utils/time.pysrc/zndraw/app/job_manager.pysrc/zndraw/app/worker_routes.pysrc/zndraw/auth.pysrc/zndraw/app/lock_routes.pysrc/zndraw/services/admin_service.pysrc/zndraw/app/job_dispatcher.pytests/test_lazy_room_loading.pysrc/zndraw/app/extension_routes.pytests/test_auth_integration.pytests/test_auth.pytests/test_chat.pysrc/zndraw/services/user_service.pytests/test_partial_frame_update.pysrc/zndraw/app/utility_routes.pytests/test_step_endpoints.pytests/test_admin_auth.pytests/conftest.pysrc/zndraw/app/room_routes.pytests/test_user_service.pytests/test_zndraw_admin.pytests/test_vis_room.pytests/test_requires_lock_decorator.pysrc/zndraw/api_manager.pytests/test_lock_socket_events.py
**/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/test_*.py: Usepytest.mark.parametrizeto avoid code duplication in tests
Tests should be very specific and test only one thing
Avoid complex test setups
Each test must be a function, not a method of a class
Files:
tests/test_room_lock_enforcement.pytests/test_lazy_room_loading.pytests/test_auth_integration.pytests/test_auth.pytests/test_chat.pytests/test_partial_frame_update.pytests/test_step_endpoints.pytests/test_admin_auth.pytests/test_user_service.pytests/test_zndraw_admin.pytests/test_vis_room.pytests/test_requires_lock_decorator.pytests/test_lock_socket_events.py
🧬 Code graph analysis (20)
src/zndraw/app/events.py (4)
src/zndraw/utils/time.py (1)
utc_now_iso(22-30)tests/test_user_service.py (1)
user_service(17-19)src/zndraw/services/user_service.py (1)
username_exists(131-145)src/zndraw/app/redis_keys.py (3)
UserKeys(518-564)hash_key(534-536)hash_key(642-644)
src/zndraw/app/job_manager.py (2)
src/zndraw/utils/time.py (2)
utc_now_iso(22-30)utc_now_timestamp(33-41)src/zndraw/app/redis_keys.py (1)
jobs_by_time(397-399)
src/zndraw/app/worker_routes.py (1)
src/zndraw/utils/time.py (1)
utc_now_timestamp(33-41)
src/zndraw/auth.py (1)
src/zndraw/utils/time.py (1)
utc_now(9-19)
src/zndraw/app/lock_routes.py (1)
src/zndraw/utils/time.py (1)
utc_now_iso(22-30)
src/zndraw/app/job_dispatcher.py (1)
src/zndraw/utils/time.py (1)
utc_now_timestamp(33-41)
tests/test_lazy_room_loading.py (1)
tests/conftest.py (1)
server(300-306)
src/zndraw/app/extension_routes.py (1)
src/zndraw/utils/time.py (1)
utc_now_timestamp(33-41)
tests/test_auth_integration.py (2)
src/zndraw/app/redis_keys.py (4)
username(573-575)UserKeys(518-564)hash_key(534-536)hash_key(642-644)src/zndraw/app/metadata_manager.py (1)
exists(127-135)
tests/test_auth.py (1)
src/zndraw/auth.py (3)
create_jwt_token(54-86)decode_jwt_token(89-116)AuthError(20-34)
tests/test_chat.py (1)
tests/conftest.py (2)
get_jwt_auth_headers(167-169)server(300-306)
src/zndraw/services/user_service.py (2)
src/zndraw/app/redis_keys.py (5)
GlobalIndexKeys(658-701)UserKeys(518-564)hash_key(534-536)hash_key(642-644)users_index(682-690)src/zndraw/utils/time.py (1)
utc_now_iso(22-30)
tests/test_partial_frame_update.py (2)
tests/conftest.py (1)
server(300-306)tests/test_room_lock_enforcement.py (1)
auth_headers(44-46)
tests/conftest.py (1)
tests/test_room_lock_enforcement.py (1)
auth_headers(44-46)
tests/test_user_service.py (5)
src/zndraw/app/utility_routes.py (1)
change_password(397-434)src/zndraw/services/user_service.py (4)
change_password(402-443)reset_password(445-479)verify_password(369-400)PasswordValidationError(27-30)src/zndraw/app/redis_keys.py (3)
UserKeys(518-564)hash_key(534-536)hash_key(642-644)src/zndraw/frame_cache.py (1)
keys(10-11)src/zndraw/app/metadata_manager.py (1)
exists(127-135)
app/src/hooks/useRestManager.ts (2)
app/src/utils/auth.ts (1)
getUserRole(240-243)app/src/myapi/client.ts (1)
joinOrCreateRoom(471-495)
tests/test_requires_lock_decorator.py (3)
tests/test_lock_socket_events.py (1)
_create_and_join_room(10-25)tests/test_step_endpoints.py (1)
_create_and_join_room(8-23)tests/test_room_lock_enforcement.py (1)
auth_headers(44-46)
src/zndraw/api_manager.py (2)
src/zndraw/app/room_routes.py (2)
create_room(137-207)join_room(257-335)src/zndraw/services/room_service.py (1)
create_room(70-112)
app/src/utils/auth.ts (3)
src/zndraw/app/utility_routes.py (1)
login(156-201)src/zndraw/api_manager.py (1)
login(22-132)src/zndraw/services/user_service.py (1)
UserRole(99-104)
tests/test_lock_socket_events.py (1)
tests/conftest.py (2)
server(300-306)get_jwt_auth_headers(167-169)
🪛 Ruff (0.14.10)
src/zndraw/app/events.py
118-118: Avoid specifying long messages outside the exception class
(TRY003)
tests/test_auth_integration.py
13-13: Probable use of requests call without timeout
(S113)
40-40: Probable use of requests call without timeout
(S113)
48-48: Probable use of requests call without timeout
(S113)
62-62: Probable use of requests call without timeout
(S113)
72-72: Probable use of requests call without timeout
(S113)
89-89: Probable use of requests call without timeout
(S113)
97-97: Probable use of requests call without timeout
(S113)
182-182: Probable use of requests call without timeout
(S113)
209-209: Probable use of requests call without timeout
(S113)
259-259: Probable use of requests call without timeout
(S113)
279-279: Probable use of requests call without timeout
(S113)
302-302: Probable use of requests call without timeout
(S113)
310-310: Probable use of requests call without timeout
(S113)
332-332: Probable use of requests call without timeout
(S113)
349-349: Probable use of requests call without timeout
(S113)
369-369: Probable use of requests call without timeout
(S113)
377-377: Probable use of requests call without timeout
(S113)
394-394: Probable use of requests call without timeout
(S113)
402-402: Probable use of requests call without timeout
(S113)
tests/test_chat.py
14-14: Probable use of requests call without timeout
(S113)
20-20: Probable use of requests call without timeout
(S113)
src/zndraw/services/user_service.py
50-52: Avoid specifying long messages outside the exception class
(TRY003)
85-85: Avoid specifying long messages outside the exception class
(TRY003)
88-90: Avoid specifying long messages outside the exception class
(TRY003)
93-96: Avoid specifying long messages outside the exception class
(TRY003)
398-398: Consider moving this statement to an else block
(TRY300)
432-432: Avoid specifying long messages outside the exception class
(TRY003)
435-435: Avoid specifying long messages outside the exception class
(TRY003)
471-471: Avoid specifying long messages outside the exception class
(TRY003)
tests/test_partial_frame_update.py
466-466: Probable use of requests call without timeout
(S113)
src/zndraw/app/utility_routes.py
318-318: Consider moving this statement to an else block
(TRY300)
324-324: Do not catch blind exception: Exception
(BLE001)
325-325: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
390-390: Do not catch blind exception: Exception
(BLE001)
391-391: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
tests/test_step_endpoints.py
11-11: Probable use of requests call without timeout
(S113)
19-19: Probable use of requests call without timeout
(S113)
tests/conftest.py
75-75: Probable use of requests call without timeout
(S113)
80-80: Avoid specifying long messages outside the exception class
(TRY003)
123-123: Probable use of requests call without timeout
(S113)
131-131: Probable use of requests call without timeout
(S113)
137-139: Avoid specifying long messages outside the exception class
(TRY003)
142-142: Probable use of requests call without timeout
(S113)
416-416: Probable use of requests call without timeout
(S113)
426-426: Probable use of requests call without timeout
(S113)
tests/test_zndraw_admin.py
161-161: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
tests/test_vis_room.py
20-20: Probable use of requests call without timeout
(S113)
32-32: Probable use of requests call without timeout
(S113)
64-64: Probable use of requests call without timeout
(S113)
79-79: Probable use of requests call without timeout
(S113)
87-87: Probable use of requests call without timeout
(S113)
99-99: Probable use of requests call without timeout
(S113)
118-118: Probable use of requests call without timeout
(S113)
132-132: Probable use of requests call without timeout
(S113)
tests/test_requires_lock_decorator.py
11-11: Probable use of requests call without timeout
(S113)
19-19: Probable use of requests call without timeout
(S113)
src/zndraw/api_manager.py
79-81: Avoid specifying long messages outside the exception class
(TRY003)
233-233: Probable use of requests call without timeout
(S113)
236-238: Avoid specifying long messages outside the exception class
(TRY003)
266-266: Probable use of requests call without timeout
(S113)
276-276: Probable use of requests call without timeout
(S113)
tests/test_lock_socket_events.py
13-13: Probable use of requests call without timeout
(S113)
21-21: Probable use of requests call without timeout
(S113)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: pytest (3.13, ubuntu-latest)
- GitHub Check: pytest (3.12, ubuntu-latest)
- GitHub Check: pytest (3.11, ubuntu-latest)
🔇 Additional comments (64)
src/zndraw/services/admin_service.py (1)
81-96: No action needed—code correctly prevents username enumeration.The
is_admin_usernamemethod is used internally for flow control only (line 215 in utility_routes.py), and both the admin-wrong-password path (line 216) and regular-user-wrong-password path (line 220) return the identicalAUTH_FAILED_MSGresponse. The unified error message goal is already maintained. Timing attacks on usernames are not a practical security concern since usernames are non-secret.src/zndraw/utils/__init__.py (1)
1-5: LGTM! Clear module documentation.The module-level docstring provides a concise overview of the utility functions available. This aligns well with the new time utilities introduced in this PR.
tests/test_admin_auth.py (3)
35-48: LGTM! Proper registration flow implemented.The test now correctly follows the register-then-login pattern introduced in this PR. The username format change to "john-doe" (lowercase with hyphen) aligns with the new validation rules (1-64 chars, alphanumeric with underscore/hyphen).
72-80: LGTM! Security improvement through unified error messaging.The updated error message "Authentication failed" instead of "Invalid username or password" is a security best practice that prevents username enumeration attacks.
168-186: LGTM! Comprehensive test for anonymous guest flow.The test effectively validates:
- Anonymous guest registration (no username provided)
- Auto-generated username format (starts with "user-")
- Subsequent login with the generated username
- Admin role assignment in local mode
This thoroughly exercises the new registration behavior.
src/zndraw/app/extension_routes.py (1)
17-17: LGTM! Good refactoring for consistency.Migrating to the centralized
utc_now_timestamp()utility improves maintainability and ensures consistent UTC time handling across the codebase. The functionality is equivalent to the previousdatetime.utcnow().timestamp()implementation.Also applies to: 149-149
tests/test_partial_frame_update.py (1)
464-469: LGTM! Aligns with the new create-then-join pattern.The explicit room creation via
POST /api/roomsbefore joining follows the authentication refactoring's new flow and matches the pattern adopted across the test suite.tests/test_room_lock_enforcement.py (1)
33-36: LGTM! Follows the new register-then-login pattern.The preliminary user registration step via
POST /api/user/registercorrectly implements the authentication flow introduced by this PR, ensuring the user exists before attempting login.src/zndraw/app/job_dispatcher.py (1)
8-9: LGTM! Centralized UTC timestamp generation.The migration from
datetime.utcnow().timestamp()toutc_now_timestamp()utility aligns with the PR's effort to standardize UTC time handling across the codebase.Also applies to: 199-199
src/zndraw/app/lock_routes.py (1)
10-10: LGTM! Standardized ISO timestamp generation.The replacement of
datetime.datetime.utcnow().isoformat()withutc_now_iso()utility in both lock acquisition and refresh paths ensures consistent UTC time handling for lock metadata.Also applies to: 105-105, 170-170
tests/test_chat.py (1)
11-22: LGTM! Adopts the new create-then-join flow.The test correctly obtains JWT headers upfront and explicitly creates the room via
POST /api/roomsbefore joining, aligning with the authentication refactoring's new pattern. Header reuse across both requests is efficient and appropriate.src/zndraw/app/job_manager.py (1)
12-12: LGTM! Consistent use of centralized UTC time utilities.The refactoring to use
utc_now_iso()andutc_now_timestamp()from the centralized time utilities module improves consistency and maintainability across the codebase.Also applies to: 168-168, 201-201, 313-313, 352-352, 406-406
src/zndraw/app/events.py (1)
10-10: LGTM! Improved separation of concerns with user service.The refactoring to use
user_service.username_exists()for user validation provides better separation of concerns and encapsulation. The use ofutc_now_iso()for activity timestamps is consistent with the broader PR changes.Also applies to: 113-126
src/zndraw/app/worker_routes.py (1)
18-18: LGTM! Consistent timestamp handling for worker registration.The refactoring to use
utc_now_timestamp()for recording worker registration times is consistent with the broader UTC time utility migration across the codebase.Also applies to: 206-206, 256-256
src/zndraw/auth.py (2)
3-3: LGTM! Well-structured JWT token implementation.The JWT token creation follows RFC 7519 best practices with standard claims (sub, iat, exp, jti) and uses the centralized
utc_now()utility for consistent UTC timestamp handling. The 7-day expiration is reasonable for this authentication use case.Also applies to: 12-12, 16-17, 54-86
156-177: LGTM! Clear documentation of JWT role vs. authorization.The updated docstring correctly clarifies that JWT roles are informational and that
AdminService.is_admin()should be used for authorization decisions. This helps prevent confusion between authentication (JWT) and authorization (Redis).tests/conftest.py (1)
414-431: Fixture correctly pre-creates room before joining.The
joined_roomfixture now aligns with the new API contract. The assertion on status codes (200, 201, 409) properly handles both new room creation and re-runs where the room may already exist.src/zndraw/utils/time.py (2)
22-41: LGTM!Clean utility functions that properly derive from
utc_now()as the single source of truth. The docstrings follow NumPy style as required.
9-19: The project requires Python 3.11+ (requires-python = ">=3.11"in pyproject.toml), so usingdatetime.UTCis correct and appropriate. No change needed.Likely an incorrect or invalid review comment.
tests/test_step_endpoints.py (2)
26-48: LGTM!The fixture correctly uses the helper to set up a room with a step lock for testing.
380-436: LGTM!Good integration test verifying socket events are emitted on step updates. The setup correctly creates and joins the room before testing socket communication.
tests/test_lock_socket_events.py (2)
37-52: LGTM!The fixture is well-structured, returning all necessary values for socket event testing. The tuple now includes
user_namewhich is correctly unpacked in all test functions.
315-363: LGTM!Good test for verifying multi-client lock event broadcasting. User 1 creates and joins, user 2 joins existing room and listens for events - correctly tests the cross-client notification flow.
tests/test_requires_lock_decorator.py (2)
26-48: LGTM!The
room_with_lockfixture correctly uses the new helper for room setup before acquiring the lock.
298-336: LGTM!Tests for missing/invalid JWT now properly create rooms first, ensuring the test isolates JWT validation from room existence errors. The updated fixture signatures correctly add
get_jwt_auth_headers.tests/test_user_service.py (2)
495-528: LGTM!Excellent test coverage for Argon2 hashing. Verifies that:
- Same password produces different hashes (random salt)
- Password verification works correctly
- Hashes have the correct
$argon2prefix
580-640: LGTM!Comprehensive tests for the new
ensure_user_existsmethod:
- Creates new user when missing
- Returns
Falsewhen user already exists (idempotent)- Preserves existing user data including password hash
This aligns well with the PR's goal of idempotent user creation.
src/zndraw/app/room_routes.py (1)
256-335: LGTM! Join endpoint properly enforces room and user existence.Key improvements:
- Returns 404 if room doesn't exist (line 302) - enforces explicit room creation
- Returns 401 if user doesn't exist (line 306) - enforces registration-first flow
- Uses
utc_now_iso()for timestamps (line 314)- Clean response without
frameCountorcreatedflagsThe error message "User not found. Please register first." provides clear guidance.
tests/test_lazy_room_loading.py (3)
19-34: LGTM! Correctly implements the create-then-join pattern.The test properly separates room creation (POST
/api/rooms) from joining (POST/api/rooms/{id}/join), which aligns with the new API semantics where room creation and joining are distinct operations.
106-120: Good test structure for ZnDraw auto-creation scenario.The comment correctly notes that
ZnDrawauto-creates the room, so a different user joining an existing room doesn't need explicit creation. The assertions for the minimal join response (status,roomId) are appropriate.
167-176: LGTM! Auth-required test correctly pre-creates the room.The test now properly creates the room before testing auth requirements on lazy loading endpoints, ensuring the 401 responses are genuinely about missing authentication rather than missing rooms.
tests/test_auth_integration.py (4)
37-79: LGTM! Guest registration tests cover both empty and whitespace username scenarios.The tests properly verify that:
- Missing username generates a
user-*prefixed guest name- Whitespace-only username is treated the same as missing
- Login works with the server-generated username
Good coverage for edge cases.
244-268: Good test for verifying Redis user creation on register.This test correctly validates that
/api/user/registercreates the Redis entry, which is the new canonical place for user creation. The test properly checks both non-existence before registration and existence after.
271-285: LGTM! Login without registration test validates security.The test confirms that login fails with a generic "Authentication failed" message when the user hasn't registered, which prevents username enumeration attacks as documented in the PR objectives.
322-341: LGTM! Tests correctly validate room not found behavior.The 404 response for joining a non-existent room properly separates room creation from room joining, matching the new API semantics.
src/zndraw/api_manager.py (3)
65-101: LGTM! Register-then-login flow correctly handles guest users.The implementation properly:
- Registers users first via
/api/user/register- Handles 409 (user exists) as acceptable
- Falls back to provided username when registration returns 409
- Performs login with the resolved username
Minor observation: The
and register_response.status_code != 401check on line 77 is technically redundant since 401 isn't in(200, 201, 409), but it doesn't cause issues.
198-240: LGTM! create_room method properly encapsulates room creation.The method correctly:
- Builds the payload with roomId, optional description, and copyFrom
- Uses JWT authorization when available
- Returns the response for 200/201 status codes
- Raises RuntimeError with context on failure
242-288: LGTM! join_room implements create-on-demand pattern correctly.The method properly:
- Attempts to join existing room first
- Creates room on 404 using the provided description/copy_from
- Retries joining after creation
- Stores session ID from response
This aligns with the PR's separation of room creation and joining.
tests/test_vis_room.py (4)
7-48: LGTM! Test correctly implements create-then-join pattern.The test properly:
- Creates room via POST
/api/rooms(expects 201)- Joins via POST
/api/rooms/{roomId}/join(expects 200)- Validates response structure for both operations
- Verifies room appears in room list afterward
Note: Consider adding
timeout=10to requests calls for consistency (as flagged in other test files).
61-70: LGTM! Non-existent room test validates 404 behavior.Test correctly verifies that joining a non-existent room returns 404 with an error message containing the room name.
73-106: LGTM! Test validates multiple users can join existing room.Good coverage of the scenario where:
- First user creates and joins the room
- Second user (different auth) successfully joins the same room
108-140: LGTM! Copy-from test validates room duplication.Test correctly verifies:
- Source room is created with frames via ZnDraw
- New room copies frames from source (frameCount == 3)
- New room without copyFrom starts empty (frameCount == 0)
src/zndraw/app/utility_routes.py (4)
31-32: Good security practice: Generic authentication error message.Using a constant
AUTH_FAILED_MSGprevents username enumeration attacks by returning the same error whether the username doesn't exist or the password is wrong.
155-201: LGTM! Login endpoint properly enforces registration requirement.The refactored login:
- Requires userName (no longer generates guests here)
- Validates username format before proceeding
- Delegates to appropriate handler based on password presence
- Uses consistent error messages to prevent enumeration
204-261: LGTM! Helper functions cleanly separate authentication paths.The helpers provide good separation of concerns:
_handle_password_login: Admin and registered user authentication_handle_guest_login: Guest authentication with registered user check_determine_role: Centralized role determination logic_issue_token: Single point for token generation response
264-327: LGTM! Register endpoint is the canonical user creation point.The endpoint correctly:
- Generates username for anonymous guests (
user-{hex})- Validates username format
- Returns 409 for duplicate usernames
- Handles both guest (no password) and registered (with password) creation
- Returns 201 on successful creation
Minor: The static analysis suggestion about moving the return to an
elseblock (TRY300) is a stylistic preference and doesn't affect correctness.app/src/hooks/useRestManager.ts (3)
4-7: LGTM! Imports updated to reflect new API surface.
throttleimport narrowed (no longer importingset)joinOrCreateRoomreplaces directjoinRoomusagegetUserRolefrom auth utilities for JWT-based role retrieval
66-70: LGTM! User role sourced from JWT claims.Using
getUserRole()as the single source of truth for user role aligns with the PR objective of treating JWT as authoritative for identity. The conditionalsetUserRoleonly updates when a role is present.
88-96: LGTM! joinOrCreateRoom handles on-demand room creation.The call correctly passes:
templatefrom URL query paramscopyFrom: template- uses template as source for copying when creating a new roomThis aligns with the client.ts implementation where
copyFrom ?? templateis used.app/src/myapi/client.ts (3)
424-443: LGTM! createRoom API function properly defined.The function:
- Takes a typed request with required
roomIdand optionaldescription/copyFrom- Supports AbortSignal for cancellation
- Returns typed response with status, roomId, frameCount, and created flag
467-495: LGTM! joinOrCreateRoom implements the recommended join-then-create pattern.The implementation correctly:
- Attempts to join existing room first (optimistic path)
- On 404, creates the room with provided options
- Uses
copyFrom ?? templateto support template-based room creation- Joins the newly created room
- Re-throws non-404 errors
This is the recommended frontend entry point for room joining as noted in the JSDoc.
445-454: LGTM! JoinRoomRequest/Response types simplified.The types correctly reflect the new API:
JoinRoomRequestonly contains optionaltemplateJoinRoomResponseno longer includescreated(room creation is now explicit)src/zndraw/services/user_service.py (7)
7-31: LGTM! Good security choice with Argon2id.Proper import organization and using
PasswordHasher()with secure defaults is the recommended approach.
33-53: LGTM!Password validation is straightforward and the descriptive error message is appropriate for validation errors.
55-97: LGTM!Username validation is comprehensive. The explicit length check before regex provides a clearer error message for users who exceed the limit.
369-401: LGTM!The Argon2 verification pattern is correct. The
VerifyMismatchErrorexception handling clearly distinguishes between "wrong password" (return False) and other issues (propagate exception).
269-367: LGTM!The
register_usermethod correctly handles both same-username registration and username change scenarios, with proper admin status transfer and atomic operations via pipeline.
402-479: LGTM!Both
change_passwordandreset_passwordvalidate the new password upfront and use Argon2 consistently.
520-563: LGTM!Good defensive handling of stale index entries and proper bytes decoding from Redis.
app/src/utils/auth.ts (5)
1-45: LGTM!Types and interfaces are well-defined and consistent with the backend's
UserRoleenum and JWT structure.
56-137: LGTM!The register-then-login flow for guests correctly handles the 409 (already exists) response as a valid path to proceed with login. The defensive check for "409 without username" is a good safeguard against unexpected server behavior.
139-243: LGTM!Good implementation of JWT-centric identity. The deprecation notice on
getUsername()guides consumers to the clearergetUsernameFromToken(), and token validation correctly handles expiry by comparing milliseconds.
245-288: LGTM!The upgrade flow correctly requires prior authentication and updates the stored token to reflect the new role and username from the server's response.
290-318: LGTM!Standard authenticated request pattern with consistent error handling.
There was a problem hiding this comment.
Pull request overview
This PR refactors the authentication system to improve security and clarify the separation between user registration and room management. Key improvements include switching from SHA-256 to Argon2 password hashing, adding JWT token expiration (7 days), and enforcing username validation.
- Introduces register-then-login flow where
/api/user/registeris the single point for user creation - Separates room creation (
POST /api/rooms) from joining (POST /api/rooms/{id}/join) - Replaces SHA-256 password hashing with Argon2id for improved security
- Adds password requirements (minimum 8 characters) and username validation (alphanumeric with underscore/hyphen)
- Implements JWT token expiration (7 days) with proper
iatandexpclaims
Reviewed changes
Copilot reviewed 32 out of 33 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Adds argon2-cffi dependency and updates Python version markers |
| pyproject.toml | Adds argon2-cffi>=23.1.0 to dependencies |
| src/zndraw/services/user_service.py | Replaces SHA-256 with Argon2, adds password/username validation, removes separate salt storage |
| src/zndraw/services/admin_service.py | Adds is_admin_username() helper for wrong password detection |
| src/zndraw/auth.py | Adds JWT expiration (7 days) with iat and exp claims |
| src/zndraw/app/utility_routes.py | Splits registration into /api/user/register (create) and /api/user/upgrade (guest→registered), updates login to never create users |
| src/zndraw/app/room_routes.py | Adds explicit POST /api/rooms endpoint, removes implicit room creation from join endpoint |
| src/zndraw/utils/time.py | New utility module with utc_now() helpers to replace deprecated datetime.utcnow() |
| src/zndraw/api_manager.py | Updates Python client to use register-then-login flow and create_room/join_room separation |
| app/src/utils/auth.ts | Updates frontend auth to use register-then-login flow, removes deprecated localStorage fields |
| app/src/myapi/client.ts | Adds createRoom() and joinOrCreateRoom() helper functions |
| tests/* | Updates all tests for new authentication and room creation flows |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Upgrade argon2-cffi from >=23.1.0 to >=25.1.0 (current stable) - Use datetime.timezone.utc instead of datetime.UTC for Python 3.11.x compatibility - Replace deprecated datetime.utcnow() with utc_now() utility in tests - Add clarifying comment for username regex pattern (1 + 0-63 = 1-64 chars) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #831 +/- ##
==========================================
+ Coverage 78.69% 78.92% +0.23%
==========================================
Files 153 154 +1
Lines 18579 18818 +239
==========================================
+ Hits 14620 14852 +232
- Misses 3959 3966 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Sanitize worker_id to valid username by replacing ':' with '-'
(celery:{uuid} format violates username pattern)
- Remove unused 'public' parameter from _get_extension_class (YAGNI)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/zndraw/job_executor.py (1)
106-109: Consider more robust worker ID sanitization with validation.The current sanitization only replaces colons with dashes. Given the PR introduces strict username validation (1–64 chars, alphanumeric with underscore/hyphen), consider:
- Validating the sanitized username meets all requirements
- Handling edge cases (e.g., very long task IDs, other special characters)
- Adding a fallback or error if sanitization produces an invalid username
♻️ Proposed enhancement with validation
+import re + # Step 2: Create ZnDraw instance in target room # Worker IDs use format "celery:{task_id}" for tracking purposes. # Usernames don't allow colons (Redis key delimiter), so sanitize here. -user_name = worker_id.replace(":", "-") +user_name = worker_id.replace(":", "-") +# Ensure sanitized username meets validation rules (1-64 chars, alphanumeric + _ -) +if not re.match(r"^[a-zA-Z0-9_-]{1,64}$", user_name): + # Fallback: hash the worker_id to create a valid username + import hashlib + user_name = f"worker-{hashlib.sha256(worker_id.encode()).hexdigest()[:16]}" vis = ZnDraw(room=room, url=server_url, user=user_name)src/zndraw/app/utility_routes.py (5)
286-286: Move import to top of file.As per coding guidelines, imports should always be at the top of the file. Consider moving
PasswordValidationErrorimport to the module level with other imports fromzndraw.services.user_service.♻️ Suggested refactor
At the top of the file (around line 22-25), update the import:
from zndraw.services.user_service import ( + PasswordValidationError, UsernameValidationError, validate_username, )Then remove line 286.
324-326: Minor: Redundant exception object in log.exception call.
log.exception()automatically captures the exception context from the current exception handler. Passingeexplicitly is redundant.♻️ Suggested fix
except Exception as e: - log.exception(f"Error registering user: {e}") + log.exception("Error registering user") return {"error": "Registration failed", "type": "ServerError"}, 500
353-353: Move import to top of file.Same as the register endpoint - this import should be at the module level. After moving both imports, this line can be removed.
390-392: Minor: Redundant exception object in log.exception call.Same pattern as register endpoint -
log.exception()already captures exception context.♻️ Suggested fix
except Exception as e: - log.exception(f"Error upgrading user: {e}") + log.exception("Error upgrading user") return {"error": "Upgrade failed", "type": "ServerError"}, 500
246-255: Consider simplifying _determine_role logic.The function checks
user_service.username_exists()after callers have already verified user existence. This is defensive but slightly redundant. The fallback to "guest" at line 255 may never be reached given the caller's preconditions.src/zndraw/api_manager.py (1)
237-242: Consider distinguishing room-already-exists error.Currently all non-2xx responses raise a generic
RuntimeError. For better UX, consider checking for 409 (Conflict) to provide a clearer message that the room already exists.♻️ Optional improvement
if response.status_code not in (200, 201): + if response.status_code == 409: + raise ValueError(f"Room '{self.room}' already exists") raise RuntimeError( f"Failed to create room '{self.room}': {response.status_code} {response.text}" )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
pyproject.tomlsrc/zndraw/api_manager.pysrc/zndraw/app/utility_routes.pysrc/zndraw/job_executor.pysrc/zndraw/services/admin_service.pysrc/zndraw/services/user_service.pysrc/zndraw/utils/time.pytests/test_auth.pytests/test_zndraw_admin.py
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: If sensible, implement collections.abc interfaces for classes, such as MutableMapping or MutableSequence
Use numpy style docstrings
Docstrings must be concise and to the point
Use type hints wherever possible. Importtyping as tif necessary, but uselist[int|float] | Noneinstead oft.Optional[t.List[int|float]]
Imports should always be at the top of the file
Files:
tests/test_zndraw_admin.pysrc/zndraw/services/admin_service.pysrc/zndraw/utils/time.pytests/test_auth.pysrc/zndraw/services/user_service.pysrc/zndraw/app/utility_routes.pysrc/zndraw/api_manager.pysrc/zndraw/job_executor.py
**/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/test_*.py: Usepytest.mark.parametrizeto avoid code duplication in tests
Tests should be very specific and test only one thing
Avoid complex test setups
Each test must be a function, not a method of a class
Files:
tests/test_zndraw_admin.pytests/test_auth.py
🧬 Code graph analysis (3)
tests/test_auth.py (3)
tests/conftest.py (1)
app(179-189)src/zndraw/auth.py (3)
create_jwt_token(54-86)decode_jwt_token(89-116)AuthError(20-34)src/zndraw/utils/time.py (1)
utc_now(9-19)
src/zndraw/services/user_service.py (2)
src/zndraw/app/redis_keys.py (6)
GlobalIndexKeys(658-701)UserKeys(518-564)hash_key(534-536)hash_key(642-644)users_index(682-690)visited_rooms(542-544)src/zndraw/utils/time.py (1)
utc_now_iso(22-30)
src/zndraw/api_manager.py (2)
src/zndraw/app/room_routes.py (2)
create_room(137-207)join_room(257-335)src/zndraw/services/room_service.py (1)
create_room(70-112)
🪛 Ruff (0.14.10)
src/zndraw/services/user_service.py
50-52: Avoid specifying long messages outside the exception class
(TRY003)
86-86: Avoid specifying long messages outside the exception class
(TRY003)
89-91: Avoid specifying long messages outside the exception class
(TRY003)
94-97: Avoid specifying long messages outside the exception class
(TRY003)
399-399: Consider moving this statement to an else block
(TRY300)
433-433: Avoid specifying long messages outside the exception class
(TRY003)
436-436: Avoid specifying long messages outside the exception class
(TRY003)
472-472: Avoid specifying long messages outside the exception class
(TRY003)
src/zndraw/app/utility_routes.py
318-318: Consider moving this statement to an else block
(TRY300)
325-325: Redundant exception object included in logging.exception call
(TRY401)
391-391: Redundant exception object included in logging.exception call
(TRY401)
src/zndraw/api_manager.py
79-81: Avoid specifying long messages outside the exception class
(TRY003)
238-240: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: pytest (3.12, ubuntu-latest)
- GitHub Check: pytest (3.11, ubuntu-latest)
- GitHub Check: pytest (3.13, ubuntu-latest)
🔇 Additional comments (18)
src/zndraw/job_executor.py (1)
207-207: LGTM: Function signature simplification.Removing the unused
publicparameter streamlines the extension retrieval API. The change is consistent with the broader refactoring toward registry-driven extension management.tests/test_zndraw_admin.py (1)
161-161: LGTM: Security improvement with unified error message.Changing to "Authentication failed" prevents username enumeration attacks by not revealing whether the username or password was incorrect. This aligns with the PR's security improvements.
tests/test_auth.py (1)
106-151: LGTM: Well-structured JWT expiration tests.Both new tests are focused and verify specific JWT expiration behavior:
test_jwt_token_has_expiration: Validates the presence and correctness of exp/iat claimstest_expired_jwt_token_raises_error: Confirms expired tokens are rejectedThe tests correctly use the
utc_now()utility instead of deprecateddatetime.utcnow(), addressing previous review feedback.src/zndraw/utils/time.py (1)
9-41: LGTM: Well-implemented UTC time utilities.The implementation correctly:
- Uses
datetime.timezone.utcfor better Python 3.11.x compatibility- Establishes
utc_now()as the single source of truth for other helpers- Follows numpy-style docstrings as per coding guidelines
- Includes appropriate type hints
- Replaces deprecated
datetime.utcnow()usage throughout the codebasesrc/zndraw/services/admin_service.py (2)
80-89: LGTM: Excellent security improvement with constant-time comparison.Using
secrets.compare_digestprevents timing attacks that could leak information about credential validity. The null-safe handling with(self._admin_username or "")ensures the comparison works correctly even when admin credentials aren't configured.
91-106: LGTM: Useful helper for admin username detection.The
is_admin_usernamemethod provides a clean way to distinguish between wrong passwords for the admin user versus non-existent users, supporting better error handling and logging without revealing sensitive information.src/zndraw/app/utility_routes.py (2)
31-32: Good security practice: Generic auth error message.Using a consistent
AUTH_FAILED_MSGprevents username enumeration attacks by not revealing whether a username exists or the password was wrong.
155-201: Login flow correctly enforces register-before-login pattern.The refactored login properly validates username format, delegates to appropriate handlers based on password presence, and uses generic error messages to prevent enumeration.
src/zndraw/api_manager.py (3)
65-101: Register-then-login flow implemented correctly.The guest registration flow properly handles:
- 200/201 for new registration
- 409 for existing user (treated as success, proceeds to login)
- Server-generated usernames extracted from response
One minor observation: 401 is checked but not explicitly handled (falls through to the
ifthat raises RuntimeError).
244-296: Join-or-create pattern implemented correctly.The method properly attempts to join first, and creates the room on-demand if it doesn't exist. The sessionId is correctly stored for subsequent requests.
318-318: Good: Timeouts added consistently across HTTP requests.Adding explicit timeouts to all HTTP requests improves reliability by preventing indefinite hangs when the server is slow or unresponsive. The timeout values are appropriately tiered (5-10s for quick operations, 30s for data-heavy operations).
Also applies to: 357-357, 698-698, 747-748, 767-768, 784-784, 803-804, 824-824, 846-846, 855-855, 871-871, 883-883, 898-898, 918-921, 940-942, 964-966, 988-988, 1013-1013, 1039-1039, 1067-1067, 1088-1088, 1111-1111, 1129-1129, 1158-1160, 1190-1190, 1214-1214, 1259-1259, 1293-1293, 1320-1320, 1347-1347, 1618-1618, 1652-1653, 1672-1672
src/zndraw/services/user_service.py (7)
20-21: Argon2 PasswordHasher with secure defaults is appropriate.Using
argon2-cffi'sPasswordHasher()with default parameters provides secure, well-tuned settings for password hashing (Argon2id algorithm).
33-52: Password validation is appropriately minimal.The 8-character minimum is a reasonable baseline. Additional complexity requirements (uppercase, numbers, symbols) are often counterproductive for security as they encourage predictable patterns. The current approach aligns with modern guidance from NIST SP 800-63B.
270-368: User registration logic handles both upgrade paths correctly.The method properly handles:
- Same username (adding password to existing guest)
- Different username (migrating data to new username, deleting old)
Admin status is correctly transferred, and the pipeline ensures atomic execution of the multi-key operations.
370-401: Argon2 password verification implemented correctly.The verification properly uses
_ph.verify()and handles theVerifyMismatchErrorexception. The byte handling for Redis values is also correct.
493-520: User deletion is thorough.The delete_user method properly cleans up all user-related Redis keys including the visited rooms set, and removes the user from both the users and admins indices.
522-565: list_all_users includes self-healing for stale index entries.Good defensive programming: if a username exists in the index but the user data is missing, the stale entry is cleaned up (lines 543-545).
233-268: TOCTOU race condition in ensure_user_exists, but impact is acceptable for guest users.A race exists between the
username_exists()check (line 249) and pipeline execution (line 265). Two concurrent registrations of the same username could both pass the existence check and execute the creation pipeline.The impact is benign: HSET and SADD are both idempotent, so concurrent writes result in consistent data. For guest users, this is acceptable.
If atomic guarantees become necessary, consider using Redis WATCH to wrap the check-then-create pattern, or use a Lua script.
- Move PasswordValidationError import to module level - Remove redundant exception object from log.exception calls - Add specific error message for 409 Conflict in room creation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/zndraw/api_manager.py:
- Around line 65-95: The registration handling currently treats 401 as
acceptable and can leave login_user_name as None; update the guest-registration
block so that the success codes are explicitly only 200/201 and 409 (remove the
"and != 401" acceptance) and treat 401 as an error (raise RuntimeError with
register_response.text); after parsing reg_data = register_response.json(), if
reg_data.get("userName") is missing and the incoming user_name is None, raise a
clear RuntimeError indicating "userName required from registration response" (or
alternatively generate a safe username) before calling the login request so that
login_payload["userName"] is never None; refer to register_response.status_code,
reg_data.get("userName", user_name), login_user_name and the subsequent
requests.post to locate the code to modify.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/zndraw/api_manager.pysrc/zndraw/app/utility_routes.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: If sensible, implement collections.abc interfaces for classes, such as MutableMapping or MutableSequence
Use numpy style docstrings
Docstrings must be concise and to the point
Use type hints wherever possible. Importtyping as tif necessary, but uselist[int|float] | Noneinstead oft.Optional[t.List[int|float]]
Imports should always be at the top of the file
Files:
src/zndraw/api_manager.pysrc/zndraw/app/utility_routes.py
🧬 Code graph analysis (1)
src/zndraw/api_manager.py (2)
src/zndraw/app/room_routes.py (1)
create_room(137-207)src/zndraw/services/room_service.py (1)
create_room(70-112)
🪛 Ruff (0.14.10)
src/zndraw/api_manager.py
79-81: Avoid specifying long messages outside the exception class
(TRY003)
238-238: Avoid specifying long messages outside the exception class
(TRY003)
240-242: Avoid specifying long messages outside the exception class
(TRY003)
src/zndraw/app/utility_routes.py
317-317: Consider moving this statement to an else block
(TRY300)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: pytest (3.13, ubuntu-latest)
- GitHub Check: pytest (3.11, ubuntu-latest)
- GitHub Check: pytest (3.12, ubuntu-latest)
🔇 Additional comments (7)
src/zndraw/api_manager.py (3)
198-244: LGTM: Room creation method is well-implemented.The new
create_room()method properly:
- Constructs payload with required
roomIdand optional parameters- Includes JWT authorization when available
- Adds timeout for resilience
- Provides specific error handling for 409 (room already exists)
- Returns parsed room data
72-72: Excellent addition of timeouts throughout for resilience.The systematic addition of explicit timeouts to all HTTP requests is a significant reliability improvement:
- Prevents indefinite hangs on network issues or unresponsive servers
- Timeout values are appropriate for operation types (5-10s for quick operations, 30s for data-intensive operations)
- Consistent pattern applied across all request types
Also applies to: 94-94, 100-100, 194-194, 234-234, 274-274, 287-287, 320-320, 359-359, 404-404, 461-461, 509-509, 550-550, 596-596, 662-662, 680-680, 700-700, 749-749, 769-769, 786-786, 805-805, 826-826, 848-848, 857-857, 873-873, 885-885, 900-900, 923-923, 944-944, 968-968, 990-990, 1015-1015, 1041-1041, 1069-1069, 1090-1090, 1113-1113, 1131-1131, 1162-1162, 1192-1192, 1216-1216, 1261-1261, 1295-1295, 1322-1322, 1349-1349, 1620-1620, 1655-1655, 1674-1674
246-298: No changes needed. Thejoin_room()method implements an intentional client-side convenience pattern: if the room doesn't exist (404), it automatically creates it before joining. This behavior is explicitly documented in the docstring, the caller inzndraw.pyintentionally passes the creation parameters, and it aligns with standard "get-or-create" API patterns. The underlying REST endpoint remains pure join-only, and the wrapper provides appropriate abstraction for the client use case.src/zndraw/app/utility_routes.py (4)
22-33: Good security practice: Generic authentication error message prevents username enumeration.The introduction of
AUTH_FAILED_MSGwith the comment explaining its purpose demonstrates proper security awareness. This prevents attackers from determining which usernames exist in the system by observing different error messages for "user not found" vs "wrong password".The centralized validation imports from
user_servicealso promote consistency across the codebase.
156-263: Excellent refactoring: Login logic is well-organized with clear separation of concerns.The refactored login flow demonstrates several best practices:
- Clear authentication paths: Separate handlers for password-based (
_handle_password_login) and guest (_handle_guest_login) authentication- Security-first validation: Username validation occurs early, and the generic
AUTH_FAILED_MSGis used consistently to prevent enumeration- Logical flow: Admin credentials checked first, then regular users, with proper fallbacks
- Single responsibility: Each helper function has a clear, focused purpose
- Documentation: Docstrings clearly state that login "NEVER creates users" - emphasizing that registration must occur first
The separation of role determination (
_determine_role) and token issuance (_issue_token) into dedicated functions improves maintainability.
265-326: Registration endpoint is well-implemented with clear semantics.The
/api/user/registerendpoint properly centralizes user creation with:
- Clear documentation stating this is the "ONLY endpoint that creates users"
- Random username generation for anonymous guests
- Username validation and duplicate detection (409 response)
- Support for both guest (no password) and registered (with password) user creation
The error handling at lines 319-325 uses a broad
except Exception:block, which catches all exceptions including unexpected programming errors. While the logging and 500 response are appropriate for an API endpoint, consider whether specific exception types (beyondPasswordValidationErrorandValueError) should be handled distinctly or allowed to propagate for debugging.
328-390: LGTM: Upgrade endpoint enables smooth guest-to-registered-user transition.The
/api/user/upgradeendpoint provides a clean user experience for guests who want to create permanent accounts:
- Requires authentication (existing guest JWT)
- Validates required fields before processing
- Supports username changes during upgrade
- Issues new JWT with updated username and role
- Proper error handling with specific validation errors
The implementation correctly delegates to
user_service.register_user()which handles the complexity of username changes and password hashing.
| # For guests (no password), register first, then login | ||
| if password is None: | ||
| # Step 1: Register user (creates in backend) | ||
| register_payload = {"userName": user_name} if user_name else {} | ||
| register_response = requests.post( | ||
| f"{self.url}/api/user/register", | ||
| json=register_payload, | ||
| timeout=5.0, | ||
| ) | ||
| # 409 = already exists, which is fine | ||
| if ( | ||
| register_response.status_code not in (200, 201, 409) | ||
| and register_response.status_code != 401 | ||
| ): | ||
| raise RuntimeError( | ||
| f"Registration failed: {register_response.text}" | ||
| ) | ||
|
|
||
| # Get the username (may be server-generated if not provided) | ||
| if register_response.status_code in (200, 201): | ||
| reg_data = register_response.json() | ||
| login_user_name = reg_data.get("userName", user_name) | ||
| else: | ||
| # 409 - user exists, use the provided username | ||
| login_user_name = user_name | ||
|
|
||
| # Step 2: Login (get JWT) | ||
| login_payload = {"userName": login_user_name} | ||
| response = requests.post( | ||
| f"{self.url}/api/login", json=login_payload, timeout=5.0 | ||
| ) |
There was a problem hiding this comment.
Clarify error handling for registration response and handle edge case where username is None.
The error handling logic at lines 75-81 is confusing:
- The condition
!= 401seems to silently accept 401 responses without raising an error. If 401 means the user already exists or authentication failed during registration, this should be documented or handled explicitly. - At line 86, if
user_nameisNone(when not provided by caller) and the registration response doesn't include"userName", thenlogin_user_namebecomesNone, which will cause the subsequent login request to fail with "userName required".
🔧 Suggested improvements
- Clarify the intent of accepting 401 responses:
- if (
- register_response.status_code not in (200, 201, 409)
- and register_response.status_code != 401
- ):
+ # 200/201: user created, 409: user exists, 401: requires password (edge case)
+ if register_response.status_code not in (200, 201, 409, 401):
raise RuntimeError(
f"Registration failed: {register_response.text}"
)- Handle the edge case where username might be None:
if register_response.status_code in (200, 201):
reg_data = register_response.json()
login_user_name = reg_data.get("userName", user_name)
+ if not login_user_name:
+ raise RuntimeError("Server did not return a username")
else:
# 409 - user exists, use the provided username
login_user_name = user_name
+ if not login_user_name:
+ raise RuntimeError("Username is required for existing user login")🧰 Tools
🪛 Ruff (0.14.10)
79-81: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In @src/zndraw/api_manager.py around lines 65 - 95, The registration handling
currently treats 401 as acceptable and can leave login_user_name as None; update
the guest-registration block so that the success codes are explicitly only
200/201 and 409 (remove the "and != 401" acceptance) and treat 401 as an error
(raise RuntimeError with register_response.text); after parsing reg_data =
register_response.json(), if reg_data.get("userName") is missing and the
incoming user_name is None, raise a clear RuntimeError indicating "userName
required from registration response" (or alternatively generate a safe username)
before calling the login request so that login_payload["userName"] is never
None; refer to register_response.status_code, reg_data.get("userName",
user_name), login_user_name and the subsequent requests.post to locate the code
to modify.
🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.5 noreply@anthropic.com
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.