- 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>
Add logging in case a safe-session user mismatch is related to wrong
session being retrieved from cache. This additional logging should
reveal any such mismatch (without revealing the actual session ID in
logs).
Send to metrics as custom attributes as well.
Also:
- Compute "session_id_changed" based on all three session IDs (and
send as custom attribute)
- Put all _verify_user logs into one (multiline) log line
- Accordingly, change logging assertion to only require a substring,
at-least-once match rather than a full-and-only match.
ref: ARCHBOM-1939
Also:
- Normalize response of a helper function to always be a boolean
- Make unit test accepting of unrelated custom attr calls
ref: ARCHBOM-1939, ARCHBOM-1941
Commit modifies safe session middleware to return an 401 in case of authentication failure and lack of 'text/html' in Accept header.
Previously, the middleware would always redirect to login in case of auth failure, but this was deemed inappropriate for any requests that are not top-level page navigation requests(we check this by seeming if 'text/html' is precent in Accept header)
Co-authored-by: Robert Raposa <rraposa@edx.org>
This is intended to silence a rare false positive that seems to happen
when someone logs in on a browser that already has an active session
for another user. We believe there should be no further positives once
this case is handled.
- login and logout views annotate the response to indicate the session
user should be changing between the request and response phases
- safe-sessions middleware skips the verify-user check when this
annotation is present
Also:
- Adds a test around existing behavior for unexpected user-changes
- Remove logging control based on `is_from_log_out`. This reverts most
of af9e26f/PR #11479 for two reasons:
- The safe-sessions `_verify_user` code has since changed to check for
`request.user.id == None`
- A commit later in the PR changes the login and logout pages to
signal that the user/session change is expected
Add pinning test for SafeCookieData values, and update SafeSessions
middleware comments to match code.
Main comment changes:
- Fix description of cookie structure:
- Specify hash algorithm (SHA256, not "H")
- Don't try to describe internals of TimestampSigner; description was
incorrect in several ways: Did not include string delimiters under
base64 (there's JSON in there); did not include the actual MAC
portion. Just describe general effect and shape of output.
- Add missing trailing pipe delimiter in signed data hash input
- Use phrase "intermediate key" rather than the less familiar term "usage
key"
This was initially introduced as a temporary flag to be able to get more
information. But if we get this kind of issue again, we'll need
something like this logging to determine the source of the session
collision. Rather than removing the code and adding it back in later,
convert this temporary switch into an opt-in setting that can be used
again in the future.
BREAKING_CHANGE: 'safe_session.log_request_user_changes' switch no
longer exists and is replaced with the 'LOG_REQUEST_USER_CHANGES' django
setting which defaults to 'False'
Six frames was not enough because for DRF views the request gets wrapped
in a proxy object and so we need more of the stack to see what part of
the code we're in that actually invokes the use change.
When a user logs out, there are warnings logged right now because the
session user_id mismatches(it becomes None on logout). Previously we
would log the request mismatch on debug and the session mismatch as
normal.
This change will result in us logging nothing if the session change is
not abnormal.
Waffle flags are useful for gradual rollout but that's not possible this
high in the middleware because a lot of the data needed (request.user)
to partition incoming requests is not availabale this high in the
middleware.
Convert this to a WaffleSwitch which will be safer to operate.
Also increase the number of frames in the stack to print per change.
Printing just 1 did not provide enough info because DRF requests wrap
WSGI requests and have a setter proxy. It will be useful to figure out
how they do this in case it's better that what we're doing in the safe
sessions middleware.
Ticket: https://openedx.atlassian.net/browse/ARCHBOM-1718
The change is behind a WafleFlag and can be turned on to log every time
the `user` attribute of an instrumented request changes. This will
significantly increase log volume so it shouldn't be left on all the
time but can be turned on to quickly debug issues related to sessions.
Ticket: https://openedx.atlassian.net/browse/ARCHBOM-1718
* temp: remove false positives on safe sessions middleware
This is a temporary fix to deal with false positives in the system due
to the masquerading feature. Long term we may not want to rely on
knowing about how masquerding works in the safe sessions middleware and
instead manage masquerding of the requset user in some other way.
Co-authored-by: Robert Raposa <rraposa@edx.org>
If this happens, it's usefule to know what main view code was being
targetd so that we can more easily investigate what might have caused
this issue.
Add a new custom attribute 'safe_sessions.user_mismatch' to find
requests that had this issue in our monitoring system.
* Generate common/djangoapps import shims for LMS
* Generate common/djangoapps import shims for Studio
* Stop appending project root to sys.path
* Stop appending common/djangoapps to sys.path
* Import from common.djangoapps.course_action_state instead of course_action_state
* Import from common.djangoapps.course_modes instead of course_modes
* Import from common.djangoapps.database_fixups instead of database_fixups
* Import from common.djangoapps.edxmako instead of edxmako
* Import from common.djangoapps.entitlements instead of entitlements
* Import from common.djangoapps.pipline_mako instead of pipeline_mako
* Import from common.djangoapps.static_replace instead of static_replace
* Import from common.djangoapps.student instead of student
* Import from common.djangoapps.terrain instead of terrain
* Import from common.djangoapps.third_party_auth instead of third_party_auth
* Import from common.djangoapps.track instead of track
* Import from common.djangoapps.util instead of util
* Import from common.djangoapps.xblock_django instead of xblock_django
* Add empty common/djangoapps/__init__.py to fix pytest collection
* Fix pylint formatting violations
* Exclude import_shims/ directory tree from linting
- Hide the submit-button CTA link to reset dates in the mobile
app. They are working on their own solution.
- Don't show the dates_banner.html code in the courseware. It has
new CTA banner support with updated wording.
-Updated middleware setting to use use middleware insted of
middleware_classes
-github install of django-method-override fork to support
new style middleware in django1.11
* INCR-219: Run python-modernize on `openedx/core/djangoapps/safe_sessions`
* disable pylint on six import to avoid isort and pylint guideline conflict
An @contextmanager will raise an exception from its yield statement if
an exception happens in the with-block that uses it. If the context
manager needs to do clean up, it should do it even if an exception is
raised, so it needs to be done in a finally clause.
1. Use request.session instead of request.user, since request.user
won't necessarily be properly set.
2. Be extra paranoid by putting logging after session cookie deletion,
so that even if there is some error related to logging, the important
work will complete and the browser won't get left in a broken state.
3. Write out the full contents of the Cookie header (up to 4096 bytes)
in the log as a base64 encoded string. This way we can look at broken
cookie states and diagnose what's breaking them (the Python parser will
just silently skip anything past a corrupted cookie entry). We base64
encode mostly to prevent people from maliciously injecting garbage into
our logs.
