From 8a764cca103054818ec8818d944ef84becdb8355 Mon Sep 17 00:00:00 2001 From: Tim McCormack Date: Mon, 8 Nov 2021 16:13:39 +0000 Subject: [PATCH] refactor: Safer, more readable user-mismatch detection code in safe-sessions (#29226) - Add early exit for readability. Less indentation here may make the control flow easier to read. - Wrap debug info generation in error-suppressing try-except block. Co-authored-by: Robert Raposa --- .../djangoapps/safe_sessions/middleware.py | 160 ++++++++++-------- 1 file changed, 85 insertions(+), 75 deletions(-) diff --git a/openedx/core/djangoapps/safe_sessions/middleware.py b/openedx/core/djangoapps/safe_sessions/middleware.py index e797f6ea1f..977ab24a43 100644 --- a/openedx/core/djangoapps/safe_sessions/middleware.py +++ b/openedx/core/djangoapps/safe_sessions/middleware.py @@ -457,92 +457,102 @@ class SafeSessionMiddleware(SessionMiddleware, MiddlewareMixin): if getattr(response, 'safe_sessions_expected_user_change', None): return - if hasattr(request, 'safe_cookie_verified_user_id'): - if hasattr(request.user, 'real_user'): - # If a view overrode the request.user with a masqueraded user, this will - # revert/clean-up that change during response processing. - request.user = request.user.real_user + if not hasattr(request, 'safe_cookie_verified_user_id'): + # Skip verification if request didn't come in with a session cookie + return - # determine if the request.user is different now than it was on the initial request - request_user_object_mismatch = request.safe_cookie_verified_user_id != request.user.id and\ - request.user.id is not None + if hasattr(request.user, 'real_user'): + # If a view overrode the request.user with a masqueraded user, this will + # revert/clean-up that change during response processing. + request.user = request.user.real_user - # determine if the current session user is different than the user in the initial request - session_user_mismatch = request.safe_cookie_verified_user_id != userid_in_session and\ - userid_in_session is not None + # determine if the request.user is different now than it was on the initial request + request_user_object_mismatch = request.safe_cookie_verified_user_id != request.user.id and\ + request.user.id is not None - if request_user_object_mismatch or session_user_mismatch: - # Log accumulated information stored on request for each change of user - extra_logs = [] + # determine if the current session user is different than the user in the initial request + session_user_mismatch = request.safe_cookie_verified_user_id != userid_in_session and\ + userid_in_session is not None - response_session_id = getattr(getattr(request, 'session', None), 'session_key', None) + if not (request_user_object_mismatch or session_user_mismatch): + # Great! No mismatch. + return - # A safe-session user mismatch could be caused by the - # wrong session being retrieved from cache. This - # additional logging should reveal any such mismatch - # (without revealing the actual session ID in logs). - sessions_raw = [ - ('parsed_cookie', request.cookie_session_field), - ('at_request', request.safe_cookie_verified_session_id), - ('at_response', response_session_id), - ] - # Note that this is an ordered list of pairs, not a - # dict, so that the output order is consistent. - session_hashes = [(k, obscure_token(v)) for (k, v) in sessions_raw] - session_id_changed = len(set(kv[1] for kv in sessions_raw)) > 1 + # Log accumulated information stored on request for each change of user + extra_logs = [] - # delete old session id for security - del request.safe_cookie_verified_session_id - del request.cookie_session_field + # Attach extra logging and metrics, but don't fail the request if there's a bug in here. + try: + response_session_id = getattr(getattr(request, 'session', None), 'session_key', None) - extra_logs.append('Session changed.' if session_id_changed else 'Session did not change.') + # A safe-session user mismatch could be caused by the + # wrong session being retrieved from cache. This + # additional logging should reveal any such mismatch + # (without revealing the actual session ID in logs). + sessions_raw = [ + ('parsed_cookie', request.cookie_session_field), + ('at_request', request.safe_cookie_verified_session_id), + ('at_response', response_session_id), + ] + # Note that this is an ordered list of pairs, not a + # dict, so that the output order is consistent. + session_hashes = [(k, obscure_token(v)) for (k, v) in sessions_raw] + session_id_changed = len(set(kv[1] for kv in sessions_raw)) > 1 - # Allow comparing session IDs in both logs and metrics + # delete old session id for security + del request.safe_cookie_verified_session_id + del request.cookie_session_field + + extra_logs.append('Session changed.' if session_id_changed else 'Session did not change.') + + # Allow comparing session IDs in both logs and metrics + extra_logs.append( + "Hash of session ID from various sources: " + + '; '.join(f'{k}={v}' for (k, v) in session_hashes) + ) + for source_name, id_hash in session_hashes: + set_custom_attribute(f'safe_sessions.session_id_hash.{source_name}', id_hash) + set_custom_attribute('safe_sessions.session_id_changed', session_id_changed) + + if hasattr(request, 'debug_user_changes'): extra_logs.append( - "Hash of session ID from various sources: " + - '; '.join(f'{k}={v}' for (k, v) in session_hashes) + 'An unsafe user transition was found. It either needs to be fixed or exempted.\n' + + '\n'.join(request.debug_user_changes) ) - for source_name, id_hash in session_hashes: - set_custom_attribute(f'safe_sessions.session_id_hash.{source_name}', id_hash) - set_custom_attribute('safe_sessions.session_id_changed', session_id_changed) + except BaseException as e: + log.exception("SafeCookieData error while computing additional logs.") - if hasattr(request, 'debug_user_changes'): - extra_logs.append( - 'An unsafe user transition was found. It either needs to be fixed or exempted.\n' + - '\n'.join(request.debug_user_changes) - ) - - if request_user_object_mismatch and not session_user_mismatch: - log.warning( - ( - "SafeCookieData user at initial request '{}' does not match user at response time: '{}' " - "for request path '{}'.\n{}" - ).format( # pylint: disable=logging-format-interpolation - request.safe_cookie_verified_user_id, request.user.id, request.path, '\n'.join(extra_logs) - ), - ) - set_custom_attribute("safe_sessions.user_mismatch", "request-response-mismatch") - elif session_user_mismatch and not request_user_object_mismatch: - log.warning( - ( - "SafeCookieData user at initial request '{}' does not match user in session: '{}' " - "for request path '{}'.\n{}" - ).format( # pylint: disable=logging-format-interpolation - request.safe_cookie_verified_user_id, userid_in_session, request.path, '\n'.join(extra_logs) - ), - ) - set_custom_attribute("safe_sessions.user_mismatch", "request-session-mismatch") - else: - log.warning( - ( - "SafeCookieData user at initial request '{}' matches neither user in session: '{}' " - "nor user at response time: '{}' for request path '{}'.\n{}" - ).format( # pylint: disable=logging-format-interpolation - request.safe_cookie_verified_user_id, userid_in_session, request.user.id, request.path, - '\n'.join(extra_logs) - ), - ) - set_custom_attribute("safe_sessions.user_mismatch", "request-response-and-session-mismatch") + if request_user_object_mismatch and not session_user_mismatch: + log.warning( + ( + "SafeCookieData user at initial request '{}' does not match user at response time: '{}' " + "for request path '{}'.\n{}" + ).format( # pylint: disable=logging-format-interpolation + request.safe_cookie_verified_user_id, request.user.id, request.path, '\n'.join(extra_logs) + ), + ) + set_custom_attribute("safe_sessions.user_mismatch", "request-response-mismatch") + elif session_user_mismatch and not request_user_object_mismatch: + log.warning( + ( + "SafeCookieData user at initial request '{}' does not match user in session: '{}' " + "for request path '{}'.\n{}" + ).format( # pylint: disable=logging-format-interpolation + request.safe_cookie_verified_user_id, userid_in_session, request.path, '\n'.join(extra_logs) + ), + ) + set_custom_attribute("safe_sessions.user_mismatch", "request-session-mismatch") + else: + log.warning( + ( + "SafeCookieData user at initial request '{}' matches neither user in session: '{}' " + "nor user at response time: '{}' for request path '{}'.\n{}" + ).format( # pylint: disable=logging-format-interpolation + request.safe_cookie_verified_user_id, userid_in_session, request.user.id, request.path, + '\n'.join(extra_logs) + ), + ) + set_custom_attribute("safe_sessions.user_mismatch", "request-response-and-session-mismatch") @staticmethod def get_user_id_from_session(request):