From 489b5ddde94d8c54df0d0d9d85ecfadc5b1161e2 Mon Sep 17 00:00:00 2001 From: Alex TYRODE Date: Sat, 3 May 2025 09:47:14 +0000 Subject: [PATCH 1/2] feat: enhance user synchronization and update logic in PadService and UserService - Refactored user creation logic in PadService to utilize a failsafe mechanism for user creation based on user session data. - Introduced a new method in UserService to update user data only if changes are detected, improving data consistency. - Enhanced error handling during user synchronization to manage race conditions and ensure robust user data management. --- src/backend/database/service/pad_service.py | 43 +++++++------ src/backend/database/service/user_service.py | 65 +++++++++++++------- 2 files changed, 65 insertions(+), 43 deletions(-) diff --git a/src/backend/database/service/pad_service.py b/src/backend/database/service/pad_service.py index 1d36f8b..9f1fdaf 100644 --- a/src/backend/database/service/pad_service.py +++ b/src/backend/database/service/pad_service.py @@ -35,34 +35,33 @@ async def create_pad(self, owner_id: UUID, display_name: str, data: Dict[str, An # Check if owner exists owner = await self.user_repository.get_by_id(owner_id) if not owner and user_session: - # User doesn't exist, create a user record from user session - print(f"ANOMALY DETECTED: User with ID '{owner_id}' does not exist but has valid authentication. Creating user as failsafe.") + # Reaching here is an anomaly, this is a failsafe + # User should already exist in the database # Create a UserService instance user_service = UserService(self.session) - # Create user with data from UserSession + # Create token data dictionary from UserSession properties + token_data = { + "username": user_session.username, + "email": user_session.email, + "email_verified": user_session.email_verified, + "name": user_session.name, + "given_name": user_session.given_name, + "family_name": user_session.family_name, + "roles": user_session.roles + } + + # Use sync_user_with_token_data which handles race conditions try: - await user_service.create_user( - user_id=user_session.id, - username=user_session.username, - email=user_session.email, - email_verified=user_session.email_verified, - name=user_session.name, - given_name=user_session.given_name, - family_name=user_session.family_name, - roles=user_session.roles - ) - print(f"Successfully created user with ID '{owner_id}' as failsafe.") - except ValueError as e: + await user_service.sync_user_with_token_data(owner_id, token_data) + # Get the user again to confirm it exists + owner = await self.user_repository.get_by_id(owner_id) + if not owner: + raise ValueError(f"Failed to create user with ID '{owner_id}'") + except Exception as e: print(f"Error creating user as failsafe: {str(e)}") - # If user creation fails due to race condition, try to get the user again - if "already exists" in str(e): - owner = await self.user_repository.get_by_id(owner_id) - if not owner: - raise ValueError(f"Failed to create user with ID '{owner_id}'") - else: - raise + raise ValueError(f"Failed to create user with ID '{owner_id}': {str(e)}") # Check if pad with same name already exists for this owner existing_pad = await self.repository.get_by_name(owner_id, display_name) diff --git a/src/backend/database/service/user_service.py b/src/backend/database/service/user_service.py index 1ac39d2..00a834d 100644 --- a/src/backend/database/service/user_service.py +++ b/src/backend/database/service/user_service.py @@ -98,6 +98,40 @@ async def delete_user(self, user_id: UUID) -> bool: """Delete a user""" return await self.repository.delete(user_id) + async def update_user_if_needed(self, user_id: UUID, token_data: Dict[str, Any], user_data: Dict[str, Any]) -> Dict[str, Any]: + """ + Update user only if data has changed + + Args: + user_id: The user's UUID + token_data: Dictionary containing user data from the authentication token + user_data: Current user data from the database + + Returns: + The updated user data dictionary or the original if no update was needed + """ + # Check if user data needs to be updated + update_data = {} + fields_to_check = [ + "username", "email", "email_verified", + "name", "given_name", "family_name" + ] + + for field in fields_to_check: + token_value = token_data.get(field) + if token_value is not None and user_data.get(field) != token_value: + update_data[field] = token_value + + # Handle roles separately as they might have a different structure + if "roles" in token_data and user_data.get("roles") != token_data["roles"]: + update_data["roles"] = token_data["roles"] + + # Update user if any field has changed + if update_data: + return await self.update_user(user_id, update_data) + + return user_data + async def sync_user_with_token_data(self, user_id: UUID, token_data: Dict[str, Any]) -> Optional[Dict[str, Any]]: """ Synchronize user data in the database with data from the authentication token. @@ -117,6 +151,7 @@ async def sync_user_with_token_data(self, user_id: UUID, token_data: Dict[str, A # If user doesn't exist, create a new one if not user_data: try: + print(f"User with ID '{user_id}' does not exist. Creating user from token data.") return await self.create_user( user_id=user_id, username=token_data.get("username", ""), @@ -131,28 +166,16 @@ async def sync_user_with_token_data(self, user_id: UUID, token_data: Dict[str, A print(f"Error creating user: {e}") # Handle case where user might have been created in a race condition if "already exists" in str(e): + print(f"Race condition detected: User with ID '{user_id}' was created by another process.") user_data = await self.get_user(user_id) + if user_data: + # User exists now, proceed with update if needed + return await self.update_user_if_needed(user_id, token_data, user_data) + else: + # This shouldn't happen - user creation failed but user doesn't exist + raise ValueError(f"Failed to create or find user with ID '{user_id}'") else: raise e - # Check if user data needs to be updated - update_data = {} - fields_to_check = [ - "username", "email", "email_verified", - "name", "given_name", "family_name" - ] - - for field in fields_to_check: - token_value = token_data.get(field) - if token_value is not None and user_data.get(field) != token_value: - update_data[field] = token_value - - # Handle roles separately as they might have a different structure - if "roles" in token_data and user_data.get("roles") != token_data["roles"]: - update_data["roles"] = token_data["roles"] - - # Update user if any field has changed - if update_data: - return await self.update_user(user_id, update_data) - - return user_data + # User exists, update if needed + return await self.update_user_if_needed(user_id, token_data, user_data) From ca2a88b4488e69824a5edf28b9e6ae246456e0ad Mon Sep 17 00:00:00 2001 From: Alex TYRODE Date: Sat, 3 May 2025 09:47:20 +0000 Subject: [PATCH 2/2] refactor: remove print statement from AuthDependency error handling - Eliminated the print statement for authentication errors in AuthDependency to streamline error handling and reduce console clutter. --- src/backend/dependencies.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/backend/dependencies.py b/src/backend/dependencies.py index 5b36ee4..749c26b 100644 --- a/src/backend/dependencies.py +++ b/src/backend/dependencies.py @@ -137,7 +137,6 @@ def _handle_auth_error(self, detail: str, status_code: int = 401) -> Optional[No """Handle authentication errors based on auto_error setting""" if self.auto_error: headers = {"WWW-Authenticate": "Bearer"} if status_code == 401 else None - print(f"Authentication error: {detail}") raise HTTPException( status_code=status_code, detail=detail,