Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

feat: Update the user_authn app to not log PII by default.

Browse Source 
Instead of optionally not logging usernames and emails, do so by
default.  This mostly removes some complexity from the app and is makes
it so that it's more secure by default.

I considered the question of allowing people to log usernames and
e-mails if they wanted to but opted not to for a couple of reasons:

* It would involve adding a new feature flag that would be the opposite
of the SQUELCH_PII_IN_LOGS which would be a bit confusing.  When do you
use which one? or do you need both? etc.
* There is still a way to correlate the messages to eachother and in
most cases also to a specific user(email being the exception).
This commit is contained in:
Feanil Patel
2021-02-09 15:25:49 -05:00
parent 54505b82c4
commit 62c0aa4917
2 changed files with 23 additions and 25 deletions
Download Patch File Download Diff File Expand all files Collapse all files

26
openedx/core/djangoapps/user_authn/views/login.py
View File

@@ -6,6 +6,7 @@ Much of this file was broken out from views.py, previous history can be found th
import json
import logging
import hashlib
import six
from django.conf import settings
@@ -103,10 +104,8 @@ def _get_user_by_email(request):
try:
return User.objects.get(email=email)
except User.DoesNotExist:
if settings.FEATURES['SQUELCH_PII_IN_LOGS']:
AUDIT_LOG.warning(u"Login failed - Unknown user email")
else:
AUDIT_LOG.warning(u"Login failed - Unknown user email: {0}".format(email))
digest = hashlib.shake_128(email.encode('utf-8')).hexdigest(16) # pylint: disable=too-many-function-args
AUDIT_LOG.warning(f"Login failed - Unknown user email {digest}")
def _check_excessive_login_attempts(user):
@@ -161,15 +160,9 @@ def _log_and_raise_inactive_user_auth_error(unauthenticated_user):
Depending on Django version we can get here a couple of ways, but this takes care of logging an auth attempt
by an inactive user, re-sending the activation email, and raising an error with the correct message.
"""
if settings.FEATURES['SQUELCH_PII_IN_LOGS']:
AUDIT_LOG.warning(
u"Login failed - Account not active for user.id: {0}, resending activation".format(
unauthenticated_user.id)
)
else:
AUDIT_LOG.warning(u"Login failed - Account not active for user {0}, resending activation".format(
unauthenticated_user.username)
)
AUDIT_LOG.warning(
f"Login failed - Account not active for user.id: {unauthenticated_user.id}, resending activation"
)
profile = UserProfile.objects.get(user=unauthenticated_user)
compose_and_send_activation_email(unauthenticated_user, profile)
@@ -224,11 +217,8 @@ def _handle_failed_authentication(user, authenticated_user):
# if we didn't find this username earlier, the account for this email
# doesn't exist, and doesn't have a corresponding password
if settings.FEATURES['SQUELCH_PII_IN_LOGS']:
loggable_id = user.id if user else "<unknown>"
AUDIT_LOG.warning(u"Login failed - password for user.id: {0} is invalid".format(loggable_id))
else:
AUDIT_LOG.warning(u"Login failed - password for {0} is invalid".format(user.email))
loggable_id = user.id if user else "<unknown>"
AUDIT_LOG.warning(f"Login failed - password for user.id: {loggable_id} is invalid")
if user and LoginFailures.is_feature_enabled():
blocked_threshold, failure_count = LoginFailures.check_user_reset_password_threshold(user)

22
openedx/core/djangoapps/user_authn/views/tests/test_login.py
View File

@@ -5,6 +5,7 @@ Tests for student activation and login
import datetime
import hashlib
import json
import unicodedata
@@ -200,6 +201,8 @@ class LoginTest(SiteMixin, CacheIsolationTestCase):
def test_login_fail_no_user_exists(self):
nonexistent_email = u'not_a_user@edx.org'
# pylint: disable=too-many-function-args
email_hash = hashlib.shake_128(nonexistent_email.encode('utf-8')).hexdigest(16)
response, mock_audit_log = self._login_response(
nonexistent_email,
self.password,
@@ -207,7 +210,7 @@ class LoginTest(SiteMixin, CacheIsolationTestCase):
self._assert_response(
response, success=False, value=self.LOGIN_FAILED_WARNING, status_code=400
)
self._assert_audit_log(mock_audit_log, 'warning', [u'Login failed', u'Unknown user email', nonexistent_email])
self._assert_audit_log(mock_audit_log, 'warning', [u'Login failed', u'Unknown user email', email_hash])
@patch.dict("django.conf.settings.FEATURES", {'SQUELCH_PII_IN_LOGS': True})
def test_login_fail_no_user_exists_no_pii(self):
@@ -227,16 +230,17 @@ class LoginTest(SiteMixin, CacheIsolationTestCase):
)
self._assert_response(response, success=False, value=self.LOGIN_FAILED_WARNING)
self._assert_audit_log(mock_audit_log, 'warning',
[u'Login failed', u'password for', self.user_email, u'invalid'])
[u'Login failed', u'password for', str(self.user.id), u'invalid'])
@patch.dict("django.conf.settings.FEATURES", {'SQUELCH_PII_IN_LOGS': True})
def test_login_fail_wrong_password_no_pii(self):
response, mock_audit_log = self._login_response(self.user_email, 'wrong_password')
self._assert_response(response, success=False, value=self.LOGIN_FAILED_WARNING)
self._assert_audit_log(mock_audit_log, 'warning', [u'Login failed', u'password for', u'invalid'])
self._assert_audit_log(
mock_audit_log, 'warning', [u'Login failed', u'password for', str(self.user.id), u'invalid']
)
self._assert_not_in_audit_log(mock_audit_log, 'warning', [self.user_email])
@patch.dict("django.conf.settings.FEATURES", {'SQUELCH_PII_IN_LOGS': True})
def test_login_not_activated_no_pii(self):
# De-activate the user
self.user.is_active = False
@@ -273,22 +277,26 @@ class LoginTest(SiteMixin, CacheIsolationTestCase):
the system does *not* send account activation email notification to the user.
"""
nonexistent_email = 'incorrect@email.com'
# pylint: disable=too-many-function-args
email_hash = hashlib.shake_128(nonexistent_email.encode('utf-8')).hexdigest(16)
self.user.is_active = False
self.user.save()
response, mock_audit_log = self._login_response(nonexistent_email, 'incorrect_password')
self.assertFalse(mock_inactive_user_email_and_error.called)
self._assert_response(response, success=False, value=self.LOGIN_FAILED_WARNING)
self._assert_audit_log(mock_audit_log, 'warning', [u'Login failed', u'Unknown user email', nonexistent_email])
self._assert_audit_log(mock_audit_log, 'warning', [u'Login failed', u'Unknown user email', email_hash])
def test_login_unicode_email(self):
unicode_email = self.user_email + six.unichr(40960)
# pylint: disable=too-many-function-args
email_hash = hashlib.shake_128(unicode_email.encode('utf-8')).hexdigest(16)
response, mock_audit_log = self._login_response(
unicode_email,
self.password,
)
self._assert_response(response, success=False)
self._assert_audit_log(mock_audit_log, 'warning', [u'Login failed', unicode_email])
self._assert_audit_log(mock_audit_log, 'warning', [u'Login failed', email_hash])
def test_login_unicode_password(self):
unicode_password = self.password + six.unichr(1972)
@@ -298,7 +306,7 @@ class LoginTest(SiteMixin, CacheIsolationTestCase):
)
self._assert_response(response, success=False)
self._assert_audit_log(mock_audit_log, 'warning',
[u'Login failed', u'password for', self.user_email, u'invalid'])
[u'Login failed', u'password for', str(self.user.id), u'invalid'])
def test_logout_logging(self):
response, _ = self._login_response(self.user_email, self.password)