-
Notifications
You must be signed in to change notification settings - Fork 3
fix: enhance error handling and logging in LMSAccountRetirementView #73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: release-ulmo
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR aims to improve error handling and logging in the LMSAccountRetirementView by sanitizing personally identifiable information (PII) from error messages. The changes introduce a new _sanitize_error_message() method to remove emails and usernames from logs, store detailed error information in UserRetirementStatus.responses, and return generic error messages to API callers.
Key changes:
- Added
_sanitize_error_message()method to remove PII (emails, usernames) from error messages using regex patterns - Enhanced exception handlers to capture and store detailed error information as JSON in
retirement.responses - Modified API responses to return generic error messages instead of exposing detailed error information
Comments suppressed due to low confidence (1)
openedx/core/djangoapps/user_api/accounts/views.py:1203
- The new error handling logic lacks test coverage. There are no tests validating that:
- The
_sanitize_error_messagemethod correctly removes PII from error messages - Error details are properly stored in
retirement.responsesduring exceptions - Generic error messages are returned to API callers instead of detailed errors
- The sanitization is applied before logging
Consider adding test cases for the exception handlers and the sanitization method to ensure the PII protection works as expected.
except RetirementStateError as exc:
try:
user_id = retirement.user.id
except AttributeError:
user_id = 'unknown'
error_details = {
'error_type': 'RetirementStateError',
'user_id': user_id,
'original_error': str(exc),
'timestamp': datetime.datetime.now(pytz.UTC).isoformat()
}
try:
current_responses = json.loads(retirement.responses) if retirement.responses else []
current_responses.append(error_details)
retirement.responses = json.dumps(current_responses)
retirement.save()
except (json.JSONDecodeError, AttributeError):
pass
log_error = self._sanitize_error_message(str(exc))
log.error(
'RetirementStateError during user retirement: user_id=%s, error=%s',
user_id, log_error
)
record_exception()
return Response("RetirementStateError occurred during retirement", status=status.HTTP_400_BAD_REQUEST)
except Exception as exc: # pylint: disable=broad-except
try:
user_id = retirement.user.id
except AttributeError:
user_id = 'unknown'
error_details = {
'error_type': type(exc).__name__,
'user_id': user_id,
'original_error': str(exc),
'timestamp': datetime.datetime.now(pytz.UTC).isoformat()
}
try:
current_responses = json.loads(retirement.responses) if retirement.responses else []
current_responses.append(error_details)
retirement.responses = json.dumps(current_responses)
retirement.save()
except (json.JSONDecodeError, AttributeError):
pass
log_error = self._sanitize_error_message(str(exc))
log.error(
'Unexpected error during user retirement: user_id=%s, error=%s',
user_id, log_error
)
record_exception()
return Response("Internal error occurred during retirement", status=status.HTTP_500_INTERNAL_SERVER_ERROR)
return Response(status=status.HTTP_204_NO_CONTENT)
def _sanitize_error_message(self, error_message):
"""
Remove common PII from error messages while preserving debugging context.
Args:
error_message (str): The original error message
Returns:
str: Error message with PII removed
"""
if not error_message:
return error_message
message = error_message
message = re.sub(r'\b[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\.[A-Z|a-z]{2,}\b',
'-', message, flags=re.IGNORECASE)
message = re.sub(r"username='[^']*'", "username='-'", message)
message = re.sub(r'username="[^"]*"', 'username="-"', message)
return message
class AccountRetirementView(ViewSet):
"""
Provides API endpoint for retiring a user.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except RetirementStateError as exc: | ||
| try: | ||
| user_id = retirement.user.id | ||
| except AttributeError: | ||
| user_id = 'unknown' | ||
| user_id, log_error = self._store_retirement_error(exc, retirement, "RetirementStateError") |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The retirement variable may not be defined when this exception handler is reached. If the error occurs on line 1090 (UserRetirementStatus.get_retirement_for_retirement_action(username)), the retirement variable will not exist yet, causing a NameError when passed to _store_retirement_error(). Consider initializing retirement = None before the try block.
| message = re.sub(r'username:\s*[^\s,]+', 'username: -', message) | ||
| message = re.sub(r'username=\s*[^\s,]+', 'username=-', message) |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex pattern for username removal uses [^\s,]+ which matches one or more non-whitespace, non-comma characters. This might be too broad and could match parts of other words or values. For example, in a message like "Error processing username=john123 for operation", this could match "john123" correctly, but in "Error: username=john123failed", it might incorrectly match "john123failed". Consider using more specific patterns or adding word boundaries where appropriate.
| message = re.sub(r'username:\s*[^\s,]+', 'username: -', message) | |
| message = re.sub(r'username=\s*[^\s,]+', 'username=-', message) | |
| message = re.sub(r'\busername:\s*[A-Za-z0-9._-]+\b', 'username: -', message) | |
| message = re.sub(r'\busername=\s*[A-Za-z0-9._-]+\b', 'username=-', message) |
| def _sanitize_error_message(self, error_message): | ||
| """ | ||
| Remove common PII from error messages while preserving debugging context. | ||
| Args: | ||
| error_message (str): The original error message | ||
| Returns: | ||
| str: Error message with PII removed | ||
| """ | ||
| if not error_message: | ||
| return error_message | ||
|
|
||
| message = error_message | ||
|
|
||
| # Remove email addresses | ||
| message = re.sub(r'\b[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\.[A-Za-z]{2,}\b', | ||
| '-', message, flags=re.IGNORECASE) | ||
|
|
||
| # Remove username values in various formats | ||
| message = re.sub(r"username='[^']*'", "username='-'", message) | ||
| message = re.sub(r'username="[^"]*"', 'username="-"', message) | ||
| message = re.sub(r'username:\s*[^\s,]+', 'username: -', message) | ||
| message = re.sub(r'username=\s*[^\s,]+', 'username=-', message) | ||
|
|
||
| # Remove common username patterns in error messages | ||
| message = re.sub(r'\bUser\s+[A-Za-z0-9._-]+\s+not found', 'User - not found', message, flags=re.IGNORECASE) | ||
| message = re.sub(r'for user\s+[A-Za-z0-9._-]+', 'for user -', message, flags=re.IGNORECASE) | ||
|
|
||
| return message | ||
|
|
||
| def _store_retirement_error(self, exc, retirement, error_prefix=""): | ||
| """ | ||
| Store sanitized error information in retirement status and return user_id and log_error for logging. | ||
| Args: | ||
| exc: The exception object | ||
| retirement: The retirement object (may be None) | ||
| error_prefix: Optional prefix for the error message (e.g., "RetirementStateError") | ||
| Returns: | ||
| tuple: (user_id, log_error) for logging purposes | ||
| """ | ||
| # Get user_id safely | ||
| try: | ||
| user_id = retirement.user.id if retirement else 'unknown' | ||
| except AttributeError: | ||
| user_id = 'unknown' | ||
|
|
||
| # Sanitize error message | ||
| log_error = self._sanitize_error_message(str(exc)) | ||
|
|
||
| # Create error message with prefix | ||
| if error_prefix: | ||
| error_msg = f"{error_prefix}: {log_error}" | ||
| else: | ||
| error_msg = f"{type(exc).__name__}: {log_error}" | ||
|
|
||
| # Store error information in retirement status as plain text | ||
| try: | ||
| if retirement is not None: | ||
| if retirement.responses: | ||
| retirement.responses += f"\n{error_msg}" | ||
| else: | ||
| retirement.responses = error_msg | ||
| retirement.save() | ||
| except AttributeError as e: | ||
| log.warning('Failed to store error in retirement status: %s', str(e)) | ||
|
|
||
| return user_id, log_error |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new error handling logic with PII sanitization and error storage lacks test coverage. The existing test file (test_retirement_views.py) has tests for the LMS retirement endpoint but doesn't cover the new sanitization logic, error storage in retirement.responses, or the new generic error messages. Consider adding tests to verify that PII is properly sanitized from various error message formats, that errors are correctly stored in the retirement status, and that generic error messages are returned to API callers.
| retirement.responses = error_msg | ||
| retirement.save() | ||
| except AttributeError as e: | ||
| log.warning('Failed to store error in retirement status: %s', str(e)) |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The warning log message passes the exception object through str(e) without sanitizing it. While this is an AttributeError that's unlikely to contain PII, it's inconsistent with the goal of sanitizing all error messages. Since this error is related to retirement status operations, it could potentially contain references to user data. Consider sanitizing this error message as well for consistency.
| log.warning('Failed to store error in retirement status: %s', str(e)) | |
| sanitized_error = self._sanitize_error_message(str(e)) | |
| log.warning('Failed to store error in retirement status: %s', sanitized_error) |
Description:
User retirement 500 errors difficult to debug due to PII in error messages.
Solution:
sanitize_error_message()method to replace emails/usernames with-in logsUserRetirementStatus.responsesPrivate JIRA Link:
BOMS-297