Skip to content

Commit

Permalink
feat: enhance cookie monitoring and logging
Browse files Browse the repository at this point in the history
Contains a number of cookie monitoring changes.

Enhancements:
- Add sampling capability for cookie logging on headers
 smaller than the threshold. For details, see
 COOKIE_SAMPLING_REQUEST_COUNT.
- Add cookie header size to log message.
- Sort logged cookies starting with largest cookie.
- Move logging from Middleware request processing
 to response processing to ensure the user id is
 available for logging for authenticated calls.
- Added cookies.header.size.computed to check
 if there are any large hidden duplicate cookies.
 Can be compared against the cookies.header.size
 custom attribute.
- Add delimiters into logs to make it simpler to parse
 when the logging tools accidentally exports multiple
 log lines together.

Removed:
- Legacy cookie capture code. This code was dangerous to
  to enable and provided more limited insight than the
  newer logging, so this was removed to simplify the code.

Other refactors:
- Switched Middleware to use new Django format, rather
 than the Mixin.
- Moved tests to its own test class. Note: this
 middleware is likely to move to a separate
 library.

ARCHBOM-2055
  • Loading branch information
robrap committed Mar 10, 2022
1 parent 8b77596 commit 29e5071
Show file tree
Hide file tree
Showing 7 changed files with 154 additions and 321 deletions.
3 changes: 1 addition & 2 deletions cms/envs/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -763,8 +763,7 @@
# Cookie monitoring
'openedx.core.lib.request_utils.CookieMonitoringMiddleware',

# After cookie monitoring, but before anything else that looks at
# cookies, especially the session middleware
# Before anything that looks at cookies, especially the session middleware
'openedx.core.djangoapps.cookie_metadata.middleware.CookieNameChange',

'openedx.core.djangoapps.header_control.middleware.HeaderControlMiddleware',
Expand Down
7 changes: 4 additions & 3 deletions lms/djangoapps/certificates/apis/v0/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory
from openedx.core.djangoapps.user_api.tests.factories import UserPreferenceFactory
from openedx.core.djangoapps.user_authn.tests.utils import JWT_AUTH_TYPES, AuthAndScopesTestMixin, AuthType
from openedx.core.djangoapps.waffle_utils.testutils import WAFFLE_TABLES
from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase # lint-amnesty, pylint: disable=wrong-import-order
from xmodule.modulestore.tests.factories import CourseFactory # lint-amnesty, pylint: disable=wrong-import-order

Expand Down Expand Up @@ -308,7 +309,7 @@ def test_no_certificate(self):
def test_query_counts(self):
# Test student with no certificates
student_no_cert = UserFactory.create(password=self.user_password)
with self.assertNumQueries(18):
with self.assertNumQueries(17, table_ignorelist=WAFFLE_TABLES):
resp = self.get_response(
AuthType.jwt,
requesting_user=self.global_staff,
Expand All @@ -318,7 +319,7 @@ def test_query_counts(self):
assert len(resp.data) == 0

# Test student with 1 certificate
with self.assertNumQueries(12):
with self.assertNumQueries(12, table_ignorelist=WAFFLE_TABLES):
resp = self.get_response(
AuthType.jwt,
requesting_user=self.global_staff,
Expand Down Expand Up @@ -358,7 +359,7 @@ def test_query_counts(self):
download_url='www.google.com',
grade="0.88",
)
with self.assertNumQueries(12):
with self.assertNumQueries(12, table_ignorelist=WAFFLE_TABLES):
resp = self.get_response(
AuthType.jwt,
requesting_user=self.global_staff,
Expand Down
5 changes: 3 additions & 2 deletions lms/djangoapps/course_api/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
from common.djangoapps.student.auth import add_users
from common.djangoapps.student.roles import CourseInstructorRole, CourseStaffRole
from common.djangoapps.student.tests.factories import AdminFactory
from openedx.core.djangoapps.waffle_utils.testutils import WAFFLE_TABLES
from openedx.core.lib.api.view_utils import LazySequence
from openedx.features.content_type_gating.models import ContentTypeGatingConfig
from openedx.features.course_duration_limits.models import CourseDurationLimitConfig
Expand Down Expand Up @@ -416,14 +417,14 @@ def test_too_many_courses(self):
self.setup_user(self.audit_user)

# These query counts were found empirically
query_counts = [54, 46, 46, 46, 46, 46, 46, 46, 46, 46, 16]
query_counts = [50, 46, 46, 46, 46, 46, 46, 46, 46, 46, 16]
ordered_course_ids = sorted([str(cid) for cid in (course_ids + [c.id for c in self.courses])])

self.clear_caches()

for page in range(1, 12):
RequestCache.clear_all_namespaces()
with self.assertNumQueries(query_counts[page - 1]):
with self.assertNumQueries(query_counts[page - 1], table_ignorelist=WAFFLE_TABLES):
response = self.verify_response(params={'page': page, 'page_size': 30})

assert 'results' in response.data
Expand Down
3 changes: 1 addition & 2 deletions lms/envs/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -2067,8 +2067,7 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring
# Generate code ownership attributes. Keep this immediately after RequestCacheMiddleware.
'edx_django_utils.monitoring.CodeOwnerMonitoringMiddleware',

# After cookie monitoring, but before anything else that looks at
# cookies, especially the session middleware
# Before anything that looks at cookies, especially the session middleware
'openedx.core.djangoapps.cookie_metadata.middleware.CookieNameChange',

# Monitoring and logging middleware
Expand Down
21 changes: 11 additions & 10 deletions openedx/core/djangoapps/user_api/accounts/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
from openedx.core.djangoapps.user_api.accounts import ACCOUNT_VISIBILITY_PREF_KEY
from openedx.core.djangoapps.user_api.models import UserPreference
from openedx.core.djangoapps.user_api.preferences.api import set_user_preference
from openedx.core.djangoapps.waffle_utils.testutils import WAFFLE_TABLES
from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, skip_unless_lms
from openedx.features.name_affirmation_api.utils import get_name_affirmation_service

Expand Down Expand Up @@ -208,7 +209,7 @@ def _verify_get_own_username(self, queries, expected_status=200):
"""
Internal helper to perform the actual assertion
"""
with self.assertNumQueries(queries):
with self.assertNumQueries(queries, table_ignorelist=WAFFLE_TABLES):
response = self.send_get(self.client, expected_status=expected_status)
if expected_status == 200:
data = response.data
Expand All @@ -220,7 +221,7 @@ def test_get_username(self):
Test that a client (logged in) can get her own username.
"""
self.client.login(username=self.user.username, password=TEST_PASSWORD)
self._verify_get_own_username(17)
self._verify_get_own_username(16)

def test_get_username_inactive(self):
"""
Expand All @@ -230,7 +231,7 @@ def test_get_username_inactive(self):
self.client.login(username=self.user.username, password=TEST_PASSWORD)
self.user.is_active = False
self.user.save()
self._verify_get_own_username(17)
self._verify_get_own_username(16)

def test_get_username_not_logged_in(self):
"""
Expand All @@ -239,7 +240,7 @@ def test_get_username_not_logged_in(self):
"""

# verify that the endpoint is inaccessible when not logged in
self._verify_get_own_username(13, expected_status=401)
self._verify_get_own_username(12, expected_status=401)


@ddt.ddt
Expand All @@ -256,7 +257,7 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase):
"""

ENABLED_CACHES = ['default']
TOTAL_QUERY_COUNT = 26
TOTAL_QUERY_COUNT = 25
FULL_RESPONSE_FIELD_COUNT = 30

def setUp(self):
Expand Down Expand Up @@ -520,7 +521,7 @@ def test_get_account_different_user_visible(self):
"""
self.different_client.login(username=self.different_user.username, password=TEST_PASSWORD)
self.create_mock_profile(self.user)
with self.assertNumQueries(self._get_num_queries(self.TOTAL_QUERY_COUNT)):
with self.assertNumQueries(self._get_num_queries(self.TOTAL_QUERY_COUNT), table_ignorelist=WAFFLE_TABLES):
response = self.send_get(self.different_client)
self._verify_full_shareable_account_response(response, account_privacy=ALL_USERS_VISIBILITY)

Expand All @@ -535,7 +536,7 @@ def test_get_account_different_user_private(self):
"""
self.different_client.login(username=self.different_user.username, password=TEST_PASSWORD)
self.create_mock_profile(self.user)
with self.assertNumQueries(self._get_num_queries(self.TOTAL_QUERY_COUNT)):
with self.assertNumQueries(self._get_num_queries(self.TOTAL_QUERY_COUNT), table_ignorelist=WAFFLE_TABLES):
response = self.send_get(self.different_client)
self._verify_private_account_response(response)

Expand Down Expand Up @@ -660,7 +661,7 @@ def verify_get_own_information(queries):
"""
Internal helper to perform the actual assertions
"""
with self.assertNumQueries(queries):
with self.assertNumQueries(queries, table_ignorelist=WAFFLE_TABLES):
response = self.send_get(self.client)
data = response.data
assert self.FULL_RESPONSE_FIELD_COUNT == len(data)
Expand All @@ -686,7 +687,7 @@ def verify_get_own_information(queries):
assert data['accomplishments_shared'] is False

self.client.login(username=self.user.username, password=TEST_PASSWORD)
verify_get_own_information(self._get_num_queries(24))
verify_get_own_information(self._get_num_queries(23))

# Now make sure that the user can get the same information, even if not active
self.user.is_active = False
Expand All @@ -706,7 +707,7 @@ def test_get_account_empty_string(self):
legacy_profile.save()

self.client.login(username=self.user.username, password=TEST_PASSWORD)
with self.assertNumQueries(self._get_num_queries(24)):
with self.assertNumQueries(self._get_num_queries(23), table_ignorelist=WAFFLE_TABLES):
response = self.send_get(self.client)
for empty_field in ("level_of_education", "gender", "country", "state", "bio",):
assert response.data[empty_field] is None
Expand Down
Loading

0 comments on commit 29e5071

Please sign in to comment.