feat: replace user identity strings with user IDs by ttak-apphelix · Pull Request #38775 · openedx/openedx-platform

ttak-apphelix · 2026-06-17T12:46:59Z

Update application logging to prevent exposure of customer identity information in logs when SQUELCH_PII_IN_LOGS is enabled. Log records that currently include usernames, email addresses, or other user-identifying strings should instead use non-PII identifiers (for example, numeric user IDs) where appropriate.
This change should be applied consistently across the platform to reduce PII exposure in logs

Private JIRA ticket:
https://2u-internal.atlassian.net/browse/BOMS-641

Akanshu-2u · 2026-06-22T07:32:35Z

-                log.info(f'The user "{username}" has been added to the retirement pipeline \
-                         by "{request.user}"')
+                if settings.FEATURES['SQUELCH_PII_IN_LOGS']:
+                    log.info('A user has been added to the retirement pipeline')


@robrap Should we keep request.user in this logger? request.user is the user who triggered the bulk retirement pipeline.

We should probably just use the request user's id instead.

Akanshu-2u · 2026-06-22T09:02:04Z

                            })
-                            log.warning('email %s already exist', email)
+                            if settings.FEATURES['SQUELCH_PII_IN_LOGS']:
+                                log.warning('email [REDACTED] already exist')


If the logger warning is a necessity to know which user's email is being referred here , then as a suggestion we can use it as:

log.warning( 'email for user_id=%s already exists', user.id, )

Just a suggestion upto you to decide, can we use this or the current change is enough?

Other places also may need some attention to see if user id can be used in the logger, so take a look at those also.

robrap · 2026-06-22T19:18:59Z

    except SMTPException:
-        log.warning("Failure sending 'pending state' e-mail for %s to %s", user.email, studio_request_email)
+        if settings.FEATURES['SQUELCH_PII_IN_LOGS']:
+            log.warning("Failure sending 'pending state' e-mail for user ID %s to [REDACTED]", user.id)


I'd keep away from using "REDACTED". That's a nice automated solution if we were trying to have Datadog redact based on regex, or something. But when updating code, we have the ability to spell things out in english in the message.

Also, I was wondering what studio_request_email was and how we'd spell that out. It turns out it is just a system email coming from a setting, so there is no reason to redact that email in the first place. Something this brings up, if we choose to not redact an email (something that looks like PII), what's a good way to annotate that? For now, you could just add a comment like:

# studio_request_email is a system email address, not PII, which can safely be logged.

robrap

I didn't finish, so I might have missed other things, but I'd rather go through after clean-up of the types of issues I already pointed out. Thank you.

robrap · 2026-07-01T13:47:00Z

        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)
+    except:  # lint-amnesty, pylint: disable=bare-except


As discussed, lint-amnesty should never be add manually. It was used in the past, and means that the disable pragma was necessary to make pylint happy at one point in time, but could possibly be fixed. If the disable pragma is what is wanted permanently, then lint-amnesty should actually be removed. There is an OEP around this that was since deleted, but this should be documented somewhere at some point. Maybe I'll open a ticket.

@ttak-apphelix: I was going to document something, but then I realized there are only 3 usages left: https://github.com/search?q=repo%3Aopenedx%2Fopenedx-platform%20lint-amnesty&type=code. I imagine there were zero at some point, but they returned via copy/paste somehow.

Was there a doc or something you saw that had you add #lint-amnesty? I wonder if there is something we can correct, and if we can help get rid of the remaining usages, even if on a separate PR?

removed lint-amnesty from here. Will create a separate PR to remove lint-amnesty across the project.

robrap · 2026-07-01T16:02:53Z

-    except:  # pylint: disable=bare-except
-        log.warning("Unable to send course creator status e-mail to %s", user.email)
+    except:  # lint-amnesty, pylint: disable=bare-except
+        if settings.FEATURES['SQUELCH_PII_IN_LOGS']:


Didn't we help deprecate the FEATURES dict? Shouldn't we be getting this setting from the top-level now?

Yes, Updated

robrap · 2026-07-01T16:05:01Z

+                    log.warning("User %s failed to enroll in non-existent course %s", user.id, str(course_key))
+                else:
+                    log.warning("User %s failed to enroll in non-existent course %s", user.username, str(course_key))
+                raise NonExistentCourseError from err  # lint-amnesty, pylint: disable=raise-missing-from


I presume you resolved the raise-missing-from linting issue, so you probably don't need the pragma?

Suggested change

raise NonExistentCourseError from err # lint-amnesty, pylint: disable=raise-missing-from

raise NonExistentCourseError from err

Yes, removed pragma.

robrap · 2026-07-01T16:10:39Z

+                if settings.FEATURES['SQUELCH_PII_IN_LOGS']:
+                    log.warning(
+                        "User %s failed to enroll in course %s because enrollment is closed (can_upgrade=%s).",
+                        user.id,
+                        str(course_key),
+                        can_upgrade,
+                    )
+                else:
+                    log.warning(
+                        "User %s failed to enroll in course %s because enrollment is closed (can_upgrade=%s).",
+                        user.username,
+                        str(course_key),
+                        can_upgrade,
+                    )


I'm not sure how many places this would apply, but the following has a lot less redundancy, and thus changes to the log message are more likely to remain consistent.

Suggested change

if settings.FEATURES['SQUELCH_PII_IN_LOGS']:

log.warning(

"User %s failed to enroll in course %s because enrollment is closed (can_upgrade=%s).",

user.id,

str(course_key),

can_upgrade,

)

else:

log.warning(

"User %s failed to enroll in course %s because enrollment is closed (can_upgrade=%s).",

user.username,

str(course_key),

can_upgrade,

)

if settings.FEATURES['SQUELCH_PII_IN_LOGS']:

user_identifier_for_log = user.id

else:

user_identifier_for_log = user.username

log.warning(

"User %s failed to enroll in course %s because enrollment is closed (can_upgrade=%s).",

user_identifier_for_log,

str(course_key),

can_upgrade,

)

Updated across the PR

robrap · 2026-07-01T16:19:09Z

@@ -829,7 +863,10 @@ def enroll_by_email(cls, email, course_id, mode=None, ignore_errors=True):
            return cls.enroll(user, course_id, mode)
        except User.DoesNotExist:
            err_msg = "Tried to enroll email {} into course {}, but user not found"


Not sure why err_msg was broken out and not using %s, but as written, this variable belongs in the else where it is used (if the message were left redundant).

That said, what if you used email_for_log = email and email_for_log = '[REDACTED]', and have a single log?

Refactored to use correct syntax

robrap · 2026-07-01T16:28:24Z

-        raise ValueError(_('Unable to send email activation link. Please try again later.'))  # pylint: disable=raise-missing-from  # noqa: B904
+        if settings.FEATURES['SQUELCH_PII_IN_LOGS']:
+            log.error(
+                'Unable to send email activation link for user %s from a redacted address',


I think redacted address is misleading. It makes it sound like the address in the DB is redacted. I'd probably go with the simpler:

Suggested change

'Unable to send email activation link for user %s from a redacted address',

'Unable to send email activation link for user %s.',

But you could also use:

Suggested change

'Unable to send email activation link for user %s from a redacted address',

'Unable to send email activation link for user %s from email [REDACTED].',

What is the from_address? Is this just a system address, and does it not even need to be redacted?

Are there other places where we used lowercase "redacted", where the meaning might be misleading?

It's a system address. Reverted the message

robrap · 2026-07-01T16:30:19Z

        try:
            ace.send(msg)
        except Exception:  # pylint: disable=broad-except
-            log.warning('Unable to send confirmation email to old address', exc_info=True)


Why did this need to change? It wasn't including any user arguments?

Note: If you remove this, you could probably remove the # pylint: disable=too-many-statements that was added earlier as well.

Are there other cases like this?

My bad, reverted the changes

Removed # pylint: disable=too-many-statements

NO

robrap · 2026-07-01T16:44:51Z

            except User.DoesNotExist:
-                log.exception(f'The user "{username}" does not exist.')
+                if settings.FEATURES['SQUELCH_PII_IN_LOGS']:
+                    log.exception('A user does not exist for bulk retirement.')


One idea is to start the loop with for index, username in enumerate(usernames_to_retire): (note: this is untested code), and update the message to something like:

Suggested change

log.exception('A user does not exist for bulk retirement.')

log.exception('Bulk retirement user at index %s does not exist.', index)

You could possibly add the index to the INFO log for consistency as well.

Alternatively, since we have all the other INFO logs, we could just use username_in_log='[REDACTED]', and someone could probably figure out which inputs weren't found.

robrap · 2026-07-01T16:47:59Z

    if not user.has_usable_password():
-        log.info(f'Goal Reminder User is disabled {user.username} course {goal.course_key}')
+        if settings.FEATURES['SQUELCH_PII_IN_LOGS']:
+            log.info(f'Goal Reminder User is disabled user ID {user.id} course {goal.course_key}')


Alternative example with less redundancy:

Suggested change

log.info(f'Goal Reminder User is disabled user ID {user.id} course {goal.course_key}')

log.info('Goal Reminder User is disabled user %s course %s', user_identifier_for_log, goal.course_key)

robrap · 2026-07-01T16:49:24Z

    if not is_course_passed(student, course):
-        log.info("User %s has not passed the course: %s", student.username, course_id)
+        if settings.FEATURES['SQUELCH_PII_IN_LOGS']:
+            log.info("User ID %s has not passed the course: %s", student.id, course_id)


Another example...

Suggested change

log.info("User ID %s has not passed the course: %s", student.id, course_id)

log.info("User %s has not passed the course: %s", user_identifier_for_log, course_id)

updated. Also updated at other places

feat: replace user identity strings with user IDs

c9d20bf

ttak-apphelix requested review from a team as code owners June 17, 2026 12:46

ttak-apphelix added 7 commits June 18, 2026 07:19

fix: corrected lint error

a06f530

Merge branch 'master' into ttak/BOMS-641

88218fa

Merge branch 'master' into ttak/BOMS-641

020a5f2

fix: resolved pylint issue

35f8781

fix: lint issue fixed

b426fd3

fix: lint issue fixed

b08bbd7

fix: lint issue fixed

5e2a0f7

Akanshu-2u reviewed Jun 22, 2026

View reviewed changes

robrap reviewed Jun 22, 2026

View reviewed changes

ttak-apphelix added 3 commits June 24, 2026 04:43

fix: updated the log message with user id

780ae84

Merge remote-tracking branch 'openedx/master' into ttak/BOMS-641

6b865e7

fix: linter issue resolved

546747c

Akanshu-2u approved these changes Jun 25, 2026

View reviewed changes

Merge branch 'master' into ttak/BOMS-641

7f5a6e9

robrap reviewed Jul 1, 2026

View reviewed changes

ttak-apphelix added 5 commits July 2, 2026 11:01

fix: standardize PII squelching logic across logs

fc1bf2f

fix: standardize PII squelching logic across logs

6d99f4a

fix: standardize PII squelching logic across logs

244c3ce

fix: standardize PII squelching logic across logs

c1faadc

fix: fixed unit test cases

bef58f8

	raise NonExistentCourseError from err # lint-amnesty, pylint: disable=raise-missing-from
	raise NonExistentCourseError from err

	'Unable to send email activation link for user %s from a redacted address',
	'Unable to send email activation link for user %s.',

	log.exception('A user does not exist for bulk retirement.')
	log.exception('Bulk retirement user at index %s does not exist.', index)

	log.info(f'Goal Reminder User is disabled user ID {user.id} course {goal.course_key}')
	log.info('Goal Reminder User is disabled user %s course %s', user_identifier_for_log, goal.course_key)

	log.info("User ID %s has not passed the course: %s", student.id, course_id)
	log.info("User %s has not passed the course: %s", user_identifier_for_log, course_id)

Uh oh!

Conversation

ttak-apphelix commented Jun 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robrap left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ttak-apphelix commented Jun 17, 2026 •

edited

Loading