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-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: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: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

robrap · 2026-07-21T13:30:19Z

    user = goal.user
    if not user.has_usable_password():
-        log.info(f'Goal Reminder User is disabled {user.username} course {goal.course_key}')
+        user_identifier_for_log = f"ID {user.id}" if getattr(settings, 'SQUELCH_PII_IN_LOGS', False) else user.username


Suggested change

user_identifier_for_log = f"ID {user.id}" if getattr(settings, 'SQUELCH_PII_IN_LOGS', False) else user.username

user_identifier_for_log = user.id if getattr(settings, 'SQUELCH_PII_IN_LOGS', False) else user.username

robrap · 2026-07-21T13:49:45Z

            except User.DoesNotExist:
-                log.exception(f'The user "{username}" does not exist.')
+                if getattr(settings, 'SQUELCH_PII_IN_LOGS', False):
+                    log.exception('Bulk retirement user at index %s does not exist.', index)


This is one place where adding "index" into the string may help differentiate, because index is non-obvious (unlike user id).

Suggested change

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

user_identifier_for_log = f"index {index}" if getattr(settings, 'SQUELCH_PII_IN_LOGS', False) else username

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

robrap · 2026-07-21T13:56:37Z

+                                f"user ID {user.id}" if getattr(settings, 'SQUELCH_PII_IN_LOGS', False)
+                                else f"username '{username}' and email '{email}'"


This is an example where it is fine as-is, if we want different sets of values to represent the user.

robrap · 2026-07-21T14:08:14Z

+        if getattr(settings, 'SQUELCH_PII_IN_LOGS', False):
+            log.error("Could not parse task input as valid json; task input is redacted")
+        else:
+            log.error("Could not parse task input as valid json; task input: %s", email_task.task_input)


I don't think I'd even use SQUELCH_PII_IN_LOGS here.

I'm not sure what CourseEmail, and whether it is even PII.

If something can't parse and we can't see what it was that couldn't parse, that would probably make it a lot harder to debug.
So, I'd err toward just leave this as is.

Do not do the following, but I'm adding it here to make a different point:

I said in an earlier comment that text like "... task input is redacted" is very misleading, because it makes it seem like we redacted it in a database or something, not in the log message itself.

If we were to proceed with this squelch, I'd possibly do something like the following:

email_task_for_log = "[REDACTED]" if getattr(settings, 'SQUELCH_PII_IN_LOGS', False) else email_task.task_input log.error("Could not parse email task input as valid json; email task input: %s", email_task_for_log)

Again, I don't think this code should be used, but I want you to see the example to understand how I'd redact something in the log, if we ever chose to redact, which is unlikely.

robrap

High-level feedback:

I'm not certain how important this work is, so let's fix the most obvious issues, and possible skip things that are more debatable.
This is linked to @Akanshu-2u's work on linting. Let's ensure that that linter is only finding major issues, or isn't used at all.
Thanks.
Also, I gave a number of example of patterns. Many of these I had given already, but it isn't what I am seeing. I'm not going to comment on every one, but I want the patterns to be reviewed for each case.

robrap · 2026-07-21T14:53:35Z

+            email_for_log = "a redacted email" if getattr(settings, 'SQUELCH_PII_IN_LOGS', False) else f"email {email}"
+            log.error("Tried to enroll %s into course %s, but user not found", email_for_log, course_id)


The past comment about redacting in a message was here: #38775 (comment), and related to this code. Here is how I would update this code:

Suggested change

email_for_log = "a redacted email" if getattr(settings, 'SQUELCH_PII_IN_LOGS', False) else f"email {email}"

log.error("Tried to enroll %s into course %s, but user not found", email_for_log, 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)

Note the difference between these log messages:

Tried to enroll email [REDACTED] into course ...

and

Tried to enroll a redacted email into course ...

The latter sounds like you know the email was redacted (retired), but really it was just an email with no matching user.

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

Comment thread lms/djangoapps/bulk_user_retirement/views.py Outdated

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 6 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

fix: removed linters

466a4e8

robrap reviewed Jul 21, 2026

View reviewed changes

ttak-apphelix added 8 commits July 27, 2026 16:13

Merge branch 'master' into ttak/BOMS-641

0b870c1

fix: simplify user identifier logging across multiple modules

d338220

fix: linter issue

9c3ee2d

fix: test cases

61ef43b

fix: test cases

f9dc2e3

fix: test case failure

6d322cb

fix: test case failure

46a325f

fix: test case

275129f

	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)

	user_identifier_for_log = f"ID {user.id}" if getattr(settings, 'SQUELCH_PII_IN_LOGS', False) else user.username
	user_identifier_for_log = user.id if getattr(settings, 'SQUELCH_PII_IN_LOGS', False) else user.username

	log.exception('Bulk retirement user at index %s does not exist.', index)
	user_identifier_for_log = f"index {index}" if getattr(settings, 'SQUELCH_PII_IN_LOGS', False) else username
	log.exception('Bulk retirement user %s does not exist.', user_identifier_for_log)

		f"user ID {user.id}" if getattr(settings, 'SQUELCH_PII_IN_LOGS', False)
		else f"username '{username}' and email '{email}'"

		email_for_log = "a redacted email" if getattr(settings, 'SQUELCH_PII_IN_LOGS', False) else f"email {email}"
		log.error("Tried to enroll %s into course %s, but user not found", email_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!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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 Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robrap left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

robrap Jul 21, 2026 •

edited

Loading

robrap left a comment •

edited

Loading