feat: replace user identity strings with user IDs

cms/djangoapps/course_creators/admin.py

@@ -169,7 +169,10 @@ def send_user_notification_callback(sender, **kwargs):  # pylint: disable=unused
     try:
         user.email_user(subject, message, studio_request_email)
     except:  # pylint: disable=bare-except
-        log.warning("Unable to send course creator status e-mail to %s", user.email)
+        user_identifier_for_log = (
+            user.id if getattr(settings, 'SQUELCH_PII_IN_LOGS', False) else user.email
+        )
+        log.warning("Unable to send course creator status e-mail to %s", user_identifier_for_log)
 
 
 @receiver(send_admin_notification, sender=CourseCreator)
@@ -179,6 +182,7 @@ def send_admin_notification_callback(sender, **kwargs):  # pylint: disable=unuse
     """
     user = kwargs['user']
 
+    # studio_request_email is a system email address, not PII, which can safely be logged.
     context = course_creator_notification_context(user)
     studio_request_email = context['studio_request_email']
 
@@ -195,7 +199,14 @@ def send_admin_notification_callback(sender, **kwargs):  # pylint: disable=unuse
             fail_silently=False
         )
     except SMTPException:
-        log.warning("Failure sending 'pending state' e-mail for %s to %s", user.email, studio_request_email)
+        user_identifier_for_log = (
+            user.id if getattr(settings, 'SQUELCH_PII_IN_LOGS', False) else user.email
+        )
+        log.warning(
+            "Failure sending 'pending state' e-mail for %s to %s",
+            user_identifier_for_log,
+            studio_request_email,
+        )
 
 
 @receiver(m2m_changed, sender=CourseCreator.organizations.through)

cms/djangoapps/course_creators/tests/test_admin.py

@@ -3,6 +3,7 @@
 """
 
 
+from smtplib import SMTPException
 from unittest import mock
 
 from django.contrib.admin.sites import AdminSite
@@ -55,6 +56,7 @@ def setUp(self):
             'user_name': 'test_user',
             'user_email': 'test_user+courses@edx.org',
         }
+        self.enable_creator_group_patch = {'ENABLE_CREATOR_GROUP': True}
 
     @override_settings(ENABLE_CREATOR_GROUP=True, STUDIO_REQUEST_EMAIL='mark@marky.mark')
     @mock.patch('django.contrib.auth.models.User.email_user')
@@ -165,3 +167,73 @@ def test_change_permission(self):
 
         self.request.user = self.user
         self.assertFalse(self.creator_admin.has_change_permission(self.request))  # noqa: PT009
+
+    @override_settings(ENABLE_CREATOR_GROUP=True, STUDIO_REQUEST_EMAIL='mark@marky.mark')
+    @mock.patch('cms.djangoapps.course_creators.admin.log')
+    @mock.patch('django.contrib.auth.models.User.email_user')
+    def test_send_user_notification_error_logging(self, mock_email_user, mock_log):
+        """
+        Test that email_user raising an exception logs the correct message based on SQUELCH_PII_IN_LOGS setting.
+        """
+        mock_email_user.side_effect = Exception("SMTP error")
+
+        with self.settings(SQUELCH_PII_IN_LOGS=True), mock.patch.dict(
+            'django.conf.settings.FEATURES', self.enable_creator_group_patch
+        ):
+            self._change_state(CourseCreator.GRANTED)
+            self.assertTrue(  # noqa: PT009
+                any(
+                    call.args
+                    and call.args[0] == "Unable to send course creator status e-mail to %s"
+                    and str(self.user.id) in str(call.args[1])
+                    for call in mock_log.warning.call_args_list
+                )
+            )
+
+        mock_log.reset_mock()
+
+        with self.settings(SQUELCH_PII_IN_LOGS=False), mock.patch.dict(
+            'django.conf.settings.FEATURES', self.enable_creator_group_patch
+        ):
+            self._change_state(CourseCreator.DENIED)
+            self._change_state(CourseCreator.GRANTED)
+            mock_log.warning.assert_any_call(
+                "Unable to send course creator status e-mail to %s",
+                self.user.email
+            )
+
+    @override_settings(ENABLE_CREATOR_GROUP=True, STUDIO_REQUEST_EMAIL='mark@marky.mark')
+    @mock.patch('cms.djangoapps.course_creators.admin.log')
+    @mock.patch('cms.djangoapps.course_creators.admin.send_mail')
+    def test_send_admin_notification_error_logging(self, mock_send_mail, mock_log):
+        """
+        Test that send_mail raising SMTPException logs the correct message based on SQUELCH_PII_IN_LOGS setting.
+        """
+        mock_send_mail.side_effect = SMTPException("SMTP error")
+
+        with self.settings(SQUELCH_PII_IN_LOGS=True), mock.patch.dict(
+            'django.conf.settings.FEATURES', self.enable_creator_group_patch
+        ):
+            self._change_state(CourseCreator.PENDING)
+            self.assertTrue(  # noqa: PT009
+                any(
+                    call.args
+                    and call.args[0] == "Failure sending 'pending state' e-mail for %s to %s"
+                    and str(self.user.id) in str(call.args[1])
+                    and call.args[2] == self.studio_request_email
+                    for call in mock_log.warning.call_args_list
+                )
+            )
+
+        mock_log.reset_mock()
+
+        with self.settings(SQUELCH_PII_IN_LOGS=False), mock.patch.dict(
+            'django.conf.settings.FEATURES', self.enable_creator_group_patch
+        ):
+            self._change_state(CourseCreator.UNREQUESTED)
+            self._change_state(CourseCreator.PENDING)
+            mock_log.warning.assert_any_call(
+                "Failure sending 'pending state' e-mail for %s to %s",
+                self.user.email,
+                self.studio_request_email
+            )

common/djangoapps/student/emails.py

@@ -27,8 +27,10 @@ def send_proctoring_requirements_email(context):
             user_context={'full_name': user.profile.name}
         )
         ace.send(msg)
-        log.info('Proctoring requirements email sent to user: %r', user.username)
+        user_identifier_for_log = user.id if getattr(settings, 'SQUELCH_PII_IN_LOGS', False) else user.username
+        log.info('Proctoring requirements email sent to user %s', user_identifier_for_log)
         return True
     except Exception:  # pylint: disable=broad-except
-        log.exception('Could not send email for proctoring requirements to user %s', user.username)
+        user_identifier_for_log = user.id if getattr(settings, 'SQUELCH_PII_IN_LOGS', False) else user.username
+        log.exception('Could not send email for proctoring requirements to user %s', user_identifier_for_log)
         return False

common/djangoapps/student/models/course_enrollment.py

@@ -664,10 +664,13 @@ def emit_event(self, event_name, enterprise_uuid=None):
 
         except Exception:  # pylint: disable=broad-except
             if event_name and self.course_id:
+                user_identifier_for_log = (
+                    self.user.id if getattr(settings, 'SQUELCH_PII_IN_LOGS', False) else self.user.username
+                )
                 log.exception(
                     'Unable to emit event %s for user %s and course %s',
                     event_name,
-                    self.user.username,
+                    user_identifier_for_log,
                     self.course_id,
                 )
 
@@ -731,39 +734,55 @@ def enroll(cls, user, course_key, mode=None, check_access=False, can_upgrade=Fal
                 course_key=course.id,
                 display_name=course.display_name,
             )
-        except CourseOverview.DoesNotExist:
+        except CourseOverview.DoesNotExist as err:
             # This is here to preserve legacy behavior which allowed enrollment in courses
             # announced before the start of content creation.
             course_data = CourseData(
                 course_key=course_key,
             )
             if check_access:
-                log.warning("User %s failed to enroll in non-existent course %s", user.username, str(course_key))
-                raise NonExistentCourseError  # pylint: disable=raise-missing-from  # noqa: B904
+                user_identifier_for_log = (
+                    user.id if getattr(settings, 'SQUELCH_PII_IN_LOGS', False) else user.username
+                )
+                log.warning(
+                    "User %s failed to enroll in non-existent course %s",
+                    user_identifier_for_log,
+                    str(course_key),
+                )
+                raise NonExistentCourseError from err
 
         if check_access:
             if cls.is_enrollment_closed(user, course) and not can_upgrade:
+                user_identifier_for_log = (
+                    user.id if getattr(settings, 'SQUELCH_PII_IN_LOGS', False) else user.username
+                )
                 log.warning(
                     "User %s failed to enroll in course %s because enrollment is closed (can_upgrade=%s).",
-                    user.username,
+                    user_identifier_for_log,
                     str(course_key),
                     can_upgrade,
                 )
                 raise EnrollmentClosedError
 
             if cls.objects.is_course_full(course):
+                user_identifier_for_log = (
+                    user.id if getattr(settings, 'SQUELCH_PII_IN_LOGS', False) else user.username
+                )
                 log.warning(
                     "Course %s has reached its maximum enrollment of %d learners. User %s failed to enroll.",
                     str(course_key),
                     course.max_student_enrollments_allowed,
-                    user.username,
+                    user_identifier_for_log,
                 )
                 raise CourseFullError
         if cls.is_enrolled(user, course_key):
+            user_identifier_for_log = (
+                user.id if getattr(settings, 'SQUELCH_PII_IN_LOGS', False) else user.username
+            )
             log.warning(
                 "User %s attempted to enroll in %s, but they were already enrolled",
-                user.username,
-                str(course_key)
+                user_identifier_for_log,
+                str(course_key),
             )
             if check_access:
                 raise AlreadyEnrolledError
@@ -828,8 +847,8 @@ def enroll_by_email(cls, email, course_id, mode=None, ignore_errors=True):
             user = User.objects.get(email=email)
             return cls.enroll(user, course_id, mode)
         except User.DoesNotExist:
-            err_msg = "Tried to enroll email {} into course {}, but user not found"
-            log.error(err_msg.format(email, course_id))
+            email_for_log = "[REDACTED]" if getattr(settings, 'SQUELCH_PII_IN_LOGS', False) else email
+            log.error("Tried to enroll email %s into course %s, but user not found", email_for_log, course_id)
             if ignore_errors:
                 return None
             raise

common/djangoapps/student/models/user.py

@@ -887,7 +887,10 @@ def activate(self):  # pylint: disable=missing-function-docstring
         self.activation_timestamp = datetime.utcnow()
         self.save()
         USER_ACCOUNT_ACTIVATED.send_robust(self.__class__, user=self.user)
-        log.info('User %s (%s) account is successfully activated.', self.user.username, self.user.email)
+        if getattr(settings, 'SQUELCH_PII_IN_LOGS', False):
+            log.info('User %s account is successfully activated.', self.user.id)
+        else:
+            log.info('User %s (%s) account is successfully activated.', self.user.username, self.user.email)
 
 
 class PendingNameChange(DeletableByUserValue, models.Model):  # noqa: DJ008
@@ -1313,10 +1316,11 @@ def log_successful_login(sender, request, user, **kwargs):  # pylint: disable=un
             'event_type': "login",
         }
     )
-    if settings.FEATURES['SQUELCH_PII_IN_LOGS']:
-        AUDIT_LOG.info(f"Login success - user.id: {user.id}")
-    else:
-        AUDIT_LOG.info(f"Login success - {user.username} ({user.email})")
+    user_identifier_for_log = (
+        user.id if getattr(settings, 'SQUELCH_PII_IN_LOGS', False)
+        else f"{user.username} ({user.email})"
+    )
+    AUDIT_LOG.info(f"Login success - {user_identifier_for_log}")
 
 
 @receiver(user_logged_out)
@@ -1330,10 +1334,11 @@ def log_successful_logout(sender, request, user, **kwargs):  # pylint: disable=u
                 'event_type': "logout",
             }
         )
-        if settings.FEATURES['SQUELCH_PII_IN_LOGS']:
-            AUDIT_LOG.info(f'Logout - user.id: {request.user.id}')  # pylint: disable=logging-format-interpolation
-        else:
-            AUDIT_LOG.info(f'Logout - {request.user}')  # pylint: disable=logging-format-interpolation
+        user_identifier_for_log = (
+            request.user.id if getattr(settings, 'SQUELCH_PII_IN_LOGS', False)
+            else f"{request.user}"
+        )
+        AUDIT_LOG.info(f'Logout - {user_identifier_for_log}')  # pylint: disable=logging-format-interpolation
         if request.user.id:
             segment.track(request.user.id, 'edx.bi.user.account.logout')
 

common/djangoapps/student/tests/tests.py

@@ -9,6 +9,7 @@
 from zoneinfo import ZoneInfo
 
 import ddt
+import pytest
 from config_models.models import cache
 from django.conf import settings
 from django.contrib.auth.models import AnonymousUser, User  # pylint: disable=imported-auth-user
@@ -28,6 +29,7 @@
     AnonymousUserId,
     CourseEnrollment,
     LinkedInAddToProfileConfiguration,
+    NonExistentCourseError,
     UserAttribute,
     anonymous_id_for_user,
     unique_id_for_user,
@@ -882,6 +884,59 @@ def test_enrollment_by_email(self):
         CourseEnrollment.unenroll_by_email("not_jack@fake.edx.org", course_id)
         self.assert_no_events_were_emitted()
 
+    @skip_unless_lms
+    @patch('common.djangoapps.student.models.course_enrollment.log')
+    def test_enroll_non_existent_course_squelch_logs(self, mock_log):
+        user = UserFactory.create(username="squelchy", email="squelchy@example.com")
+        course_id = CourseLocator("edX", "NoExist", "2013")
+
+        with self.settings(SQUELCH_PII_IN_LOGS=True):
+            with pytest.raises(NonExistentCourseError):
+                CourseEnrollment.enroll(user, course_id, check_access=True)
+            mock_log.warning.assert_any_call(
+                "User %s failed to enroll in non-existent course %s",
+                user.id,
+                str(course_id)
+            )
+
+        mock_log.reset_mock()
+
+        with self.settings(SQUELCH_PII_IN_LOGS=False):
+            with pytest.raises(NonExistentCourseError):
+                CourseEnrollment.enroll(user, course_id, check_access=True)
+            mock_log.warning.assert_any_call(
+                "User %s failed to enroll in non-existent course %s",
+                user.username,
+                str(course_id)
+            )
+
+    @skip_unless_lms
+    @patch('common.djangoapps.student.models.course_enrollment.log')
+    def test_enroll_by_email_non_existent_user_squelch_logs(self, mock_log):
+        course_id = CourseLocator("edX", "Test101", "2013")
+        CourseOverviewFactory.create(id=course_id)
+        email = "non_existent_user@example.com"
+
+        with self.settings(SQUELCH_PII_IN_LOGS=True):
+            with pytest.raises(User.DoesNotExist):
+                CourseEnrollment.enroll_by_email(email, course_id, ignore_errors=False)
+            mock_log.error.assert_any_call(
+                "Tried to enroll email %s into course %s, but user not found",
+                "[REDACTED]",
+                course_id
+            )
+
+        mock_log.reset_mock()
+
+        with self.settings(SQUELCH_PII_IN_LOGS=False):
+            with pytest.raises(User.DoesNotExist):
+                CourseEnrollment.enroll_by_email(email, course_id, ignore_errors=False)
+            mock_log.error.assert_any_call(
+                "Tried to enroll email %s into course %s, but user not found",
+                email,
+                course_id
+            )
+
     @skip_unless_lms
     def test_enrollment_multiple_classes(self):
         user = UserFactory(username="rusty", email="rusty@fake.edx.org")

common/djangoapps/student/views/management.py

@@ -489,7 +489,8 @@ def change_enrollment(request, check_access=True):
         except UnenrollmentNotAllowed as exc:
             return HttpResponseBadRequest(str(exc))
 
-        log.info("User %s unenrolled from %s; sending REFUND_ORDER", user.username, course_id)
+        user_identifier_for_log = user.id if getattr(settings, 'SQUELCH_PII_IN_LOGS', False) else user.username
+        log.info("User %s unenrolled from %s; sending REFUND_ORDER", user_identifier_for_log, course_id)
         REFUND_ORDER.send(sender=None, course_enrollment=enrollment)
         return HttpResponse()
     else:
@@ -557,11 +558,17 @@ def disable_account_ajax(request):
         if account_action == 'disable':
             user_account.account_status = UserStanding.ACCOUNT_DISABLED
             context['message'] = _("Successfully disabled {}'s account").format(username)
-            log.info("%s disabled %s's account", request.user, username)
+            if getattr(settings, 'SQUELCH_PII_IN_LOGS', False):
+                log.info("User %s disabled user %s's account", request.user.id, user.id)
+            else:
+                log.info("%s disabled %s's account", request.user, username)
         elif account_action == 'reenable':
             user_account.account_status = UserStanding.ACCOUNT_ENABLED
             context['message'] = _("Successfully reenabled {}'s account").format(username)
-            log.info("%s reenabled %s's account", request.user, username)
+            if getattr(settings, 'SQUELCH_PII_IN_LOGS', False):
+                log.info("User %s reenabled user %s's account", request.user.id, user.id)
+            else:
+                log.info("%s reenabled %s's account", request.user, username)
         else:
             context['message'] = _("Unexpected account status")
             return JsonResponse(context, status=400)
@@ -847,11 +854,12 @@ def do_email_change_request(user, new_email, activation_key=None, secondary_emai
 
     try:
         ace.send(msg)
-        log.info("Email activation link sent to user [%s].", new_email)
-    except Exception:
+        user_identifier_for_log = user.id if getattr(settings, 'SQUELCH_PII_IN_LOGS', False) else new_email
+        log.info("Email activation link sent to user [%s].", user_identifier_for_log)
+    except Exception as err:
         from_address = configuration_helpers.get_value('email_from_address', settings.DEFAULT_FROM_EMAIL)
         log.error('Unable to send email activation link to user from "%s"', from_address, exc_info=True)
-        raise ValueError(_('Unable to send email activation link. Please try again later.'))  # pylint: disable=raise-missing-from  # noqa: B904
+        raise ValueError(_('Unable to send email activation link. Please try again later.')) from err
 
     if not secondary_email_change_request:
         # When the email address change is complete, a "edx.user.settings.changed" event will be emitted.