feat: replace user identity strings with user IDs - #38775
Conversation
robrap
left a comment
There was a problem hiding this comment.
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.
| try: | ||
| ace.send(msg) | ||
| except Exception: # pylint: disable=broad-except | ||
| log.warning('Unable to send confirmation email to old address', exc_info=True) |
There was a problem hiding this comment.
- 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-statementsthat was added earlier as well.
- Are there other cases like this?
There was a problem hiding this comment.
- My bad, reverted the changes
- Removed # pylint: disable=too-many-statements
- NO
There was a problem hiding this comment.
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 PR has been internally reviewed. Can you please relook the changes |
|
@robrap Commenting for visibility |
robrap
left a comment
There was a problem hiding this comment.
Thanks for your patience.
| failed_emails_for_log = ( | ||
| '[REDACTED]' if getattr(settings, 'SQUELCH_PII_IN_LOGS', False) else pformat(failed_emails) | ||
| ) | ||
| log.error('Failed emails:%s', failed_emails_for_log) |
There was a problem hiding this comment.
- Minor improvement that is independent.
| failed_emails_for_log = ( | |
| '[REDACTED]' if getattr(settings, 'SQUELCH_PII_IN_LOGS', False) else pformat(failed_emails) | |
| ) | |
| log.error('Failed emails:%s', failed_emails_for_log) | |
| failed_emails_for_log = ( | |
| '[REDACTED]' if getattr(settings, 'SQUELCH_PII_IN_LOGS', False) else pformat(failed_emails) | |
| ) | |
| log.error('Failed emails for manual verification: %s', failed_emails_for_log) |
- I don't love that this makes the error message almost useless, because we don't know which ones failed or passed, and thus how to recover. Can we ticket a follow-up, or do a follow-up PR to improve
_generate_manual_verification_from_fileto return failed indices as well? This doesn't need to block this PR.
There was a problem hiding this comment.
- Updated
- Will create a follow up ticket for this
|
@robrap FYI this fails for all [.format(...)] calls if we don't use f-strings. |
robrap
left a comment
There was a problem hiding this comment.
Very minor comments. Thanks. Let's review/merge on Friday.
| 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 | ||
| def assert_case(squelch_pii, states): |
There was a problem hiding this comment.
Nit: assert_case => test_and_assert_case
I like when assert_ functions just help with asserting on test output values, but this method is actually performing the feature under test as well, right?
There was a problem hiding this comment.
updated
I like when assert_ functions just help with asserting on test output values, but this method is actually performing the feature under test as well, right? - That's correct
| email_for_log, user_identifier_for_log = ( | ||
| ('[REDACTED]', f' for user {user.id}') | ||
| if getattr(settings, 'SQUELCH_PII_IN_LOGS', False) | ||
| else (email, '') | ||
| ) | ||
| log.warning('email %s already exists%s', email_for_log, user_identifier_for_log) |
There was a problem hiding this comment.
If we had to have two different messages, your original code with two log.warning calls would be nicer.
The following is much simpler to read and follow, even though it is a slightly awkward message in the case of squelching. Please choose this or your original. I like this, but you may disagree.
user_identifier_for_log = (
user.id)
if getattr(settings, 'SQUELCH_PII_IN_LOGS', False)
else email
)
log.warning('email for user (%s) already exists', user_identifier_for_log)
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