-
Notifications
You must be signed in to change notification settings - Fork 4
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?
Changes from all commits
49e3e0a
7f731ed
f721082
976267d
e7ac3c4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |||||||||
|
|
||||||||||
| import datetime | ||||||||||
| import logging | ||||||||||
| import re | ||||||||||
| from functools import wraps | ||||||||||
|
|
||||||||||
| import pytz | ||||||||||
|
|
@@ -1070,7 +1071,7 @@ class LMSAccountRetirementView(ViewSet): | |||||||||
| parser_classes = (JSONParser,) | ||||||||||
|
|
||||||||||
| @request_requires_username | ||||||||||
| def post(self, request): | ||||||||||
| def post(self, request): # pylint: disable=too-many-statements | ||||||||||
| """ | ||||||||||
| POST /api/user/v1/accounts/retire_misc/ | ||||||||||
|
|
||||||||||
|
|
@@ -1114,30 +1115,97 @@ def post(self, request): | |||||||||
| record_exception() | ||||||||||
| return Response(status=status.HTTP_404_NOT_FOUND) | ||||||||||
| 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") | ||||||||||
|
|
||||||||||
| log.error( | ||||||||||
| 'RetirementStateError during user retirement: user_id=%s, error=%s', | ||||||||||
| user_id, str(exc) | ||||||||||
| user_id, log_error | ||||||||||
| ) | ||||||||||
| record_exception() | ||||||||||
| return Response(str(exc), status=status.HTTP_400_BAD_REQUEST) | ||||||||||
| 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' | ||||||||||
| user_id, log_error = self._store_retirement_error(exc, retirement) | ||||||||||
|
|
||||||||||
| log.error( | ||||||||||
| 'Unexpected error during user retirement: user_id=%s, error=%s', | ||||||||||
| user_id, str(exc) | ||||||||||
| user_id, log_error | ||||||||||
| ) | ||||||||||
| record_exception() | ||||||||||
| return Response(str(exc), status=status.HTTP_500_INTERNAL_SERVER_ERROR) | ||||||||||
| 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 | ||||||||||
Akanshu-2u marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
| """ | ||||||||||
| 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) | ||||||||||
|
Comment on lines
+1160
to
+1161
|
||||||||||
| 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) |
Akanshu-2u marked this conversation as resolved.
Show resolved
Hide resolved
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) |
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.
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
retirementvariable may not be defined when this exception handler is reached. If the error occurs on line 1090 (UserRetirementStatus.get_retirement_for_retirement_action(username)), theretirementvariable will not exist yet, causing aNameErrorwhen passed to_store_retirement_error(). Consider initializingretirement = Nonebefore the try block.