feat: Remove monitoring for now-concluded verify-all work (#29495)

The removed attributes were needed in order to inform the move of the
`_verify_user` function call up out of the try/except block. That work has
concluded (https://github.com/edx/edx-platform/pull/29324) so the
monitoring can be removed.

Also:

- Bring a comment on some other monitoring up to date
- Make long-needed corrections to an existing docstring
- Remove malformed-cookie logging, since we haven't been using it
This commit is contained in:
Tim McCormack
2021-12-02 18:45:48 +00:00
committed by GitHub
parent 038d68d53c
commit 02b9e059a2

View File

@@ -76,7 +76,6 @@ Custom Attributes:
"""
import inspect
from base64 import b64encode
from hashlib import sha1, sha256
from logging import getLogger
from typing import Union
@@ -301,12 +300,14 @@ class SafeSessionMiddleware(SessionMiddleware, MiddlewareMixin):
final verification before sending the response (in
process_response).
"""
# 2021-10-29: Temporary debugging attr to answer the question
# 2021-12-01: Temporary debugging attr to answer the question
# "are browsers sometimes sending in multiple session
# cookies?" We've observed behavior that might be consistent
# with this, perhaps due to an additional cookie set on the
# wrong domain, and assuming that the cookies *occasionally*
# are sent in a different order. -- timmc
# cookies?" Answer: Yes, this sometimes happens, although it
# does not appear to cause user mismatches. (There was a theory
# that multiple cookies might sometimes be sent in a varying
# order.) We may still want to have the ability to monitor this
# oddity, but as far as we can tell, it is not essential to the
# core safe-sessions monitoring.
try:
set_custom_attribute(
'safe_sessions.session_cookie_count',
@@ -358,12 +359,11 @@ class SafeSessionMiddleware(SessionMiddleware, MiddlewareMixin):
cookie.
Also, the session cookie is deleted if prior verification failed
or the designated user in the request has changed since the
original request.
or the new cookie can't be created for some reason.
Processing the response is a multi-step process, as follows:
Step 1. Call the parent's method to generate the basic cookie.
Step 1. Call the parent's method to (maybe) generate the basic cookie.
Step 2. Verify that the user marked at the time of
process_request matches the user at this time when processing
@@ -378,26 +378,6 @@ class SafeSessionMiddleware(SessionMiddleware, MiddlewareMixin):
"""
response = super().process_response(request, response) # Step 1
# 2021-10-29: Temporary debugging attrs, to answer the
# question "are we calling _verify_user on too few responses?"
# We should probably be calling it on every response, and it
# looks like we might be missing most responses -- some
# testing shows that most of my LMS responses do *not* get a
# newly coined session cookie, but I seem to recall that that
# used to happen. If so, we may have at some point stopped
# calling _verify_user as often as we should. -- timmc
try:
set_custom_attribute(
'safe_sessions.request_had_valid_session',
hasattr(request, 'safe_cookie_verified_session_id')
)
set_custom_attribute(
'safe_sessions.response_has_session_cookie',
_is_cookie_present(response)
)
except: # pylint: disable=bare-except
pass
user_id_in_session = self.get_user_id_from_session(request)
self._verify_user(request, response, user_id_in_session) # Step 2
@@ -661,14 +641,6 @@ def _delete_cookie(request, response):
httponly=settings.SESSION_COOKIE_HTTPONLY or None,
)
# Log the cookie, but cap the length and base64 encode to make sure nothing
# malicious gets directly dumped into the log.
cookie_header = request.META.get('HTTP_COOKIE', '')[:4096]
log.warning(
"Malformed Cookie Header? First 4K, in Base64: %s",
b64encode(str(cookie_header).encode())
)
# Note, there is no request.user attribute at this point.
if hasattr(request, 'session') and hasattr(request.session, 'session_key'):
log.warning(