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

refactor: Safer, more readable user-mismatch detection code in safe-sessions (#29226)

Browse Source 
- 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 <rraposa@edx.org>
This commit is contained in:
Tim McCormack
2021-11-08 16:13:39 +00:00
committed by GitHub
parent fdeb7714aa
commit 8a764cca10
1 changed files with 85 additions and 75 deletions
Download Patch File Download Diff File Expand all files Collapse all files

160
openedx/core/djangoapps/safe_sessions/middleware.py
View File

@@ -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):