It was copied there in 4.7.0 (openedx/edx-django-utils#209) so it can be used in more IDAs.
Includes dropping dependency on PyNacl, which was only in use by that module.
It's likely that someone will at some point enable encrypted logging but
forget to deploy the config change that sets the key; if this happens, we
should gracefully return a warning rather than raise an exception.
Along the same lines, make sure that safe-sessions won't raise an exception
if the setting is missing, and document the suggested use of getattr.
- test: Remove reference to `REDIRECT_TO_LOGIN_ON_SAFE_SESSION_AUTH_FAILURE`,
since it was removed in commit bd7653aefcd77a/PR #29132.
- docs: Clarify what "work correctly" means for header-logging
This is more correct and may reduce the likelihood of perpetuating a bad
mixed-auth state.
In general, we should probably be modifying session and JWT cookies in
sync at all times, never individually. This specific code probably won't
make anything worse, but a clean reset might improve user experience in
the rare cases where someone somehow gets their browser into a weird
state.
- Switch from `response.set_cookie` with past expiry to just using the
`response.delete_cookie` method.
- Docstring improvements.
ref: ARCHBOM-2030 (internal)
Change `mark_user_change_as_expected` to no longer take the response object
and instead convey the expected-change information via RequestCache.
This requires edx-django-utils 4.4.2, which fixes the bug where
RequestCache was cleared in the exception phase.
Also, no longer mark `ENFORCE_SAFE_SESSIONS` toggle as
temporary. We'll want it as an opt-out.
I was tempted to take this opportunity to move any existing
`mark_user_change_as_expected` calls to be closer to where the actual
change request.user occurs, reducing risk of both false positives and false
negatives, but it would be better to do that one at a time in case a move
breaks something. (Ideally it would be called right after any
`django.contrib.auth` `login` or `logout` call; previously, we were
constrained by having to make the call after a response object had been
created.) These changes can be made later if it becomes necessary.
* fix: safe session bug when request has no user
Fixes a bug during safe session monitoring when
request has no user.
ARCHBOM-1940
* fixup! add comment and loosen if condition
Fixes a bug where a custom attribute was being set
even for cases where we did not have the appropriate
data, and an exception was being raised.
ARCHBOM-1940
* add custom attribute for list of user ids on mismatch.
* log request header for all mismatched users for all
requests for N seconds after the mismatch is found, if
LOG_REQUEST_USER_CHANGE_HEADERS is enabled. See toggle
docs for more details.
ARCHBOM-1940
A new feature toggle, default off, causes the session to be deleted when
the user identity on the response does not match the session or request.
There are a small number of requests that cause the user present on the
session at the time of the request to be a different user by the time of
the response. As far as I can tell, these are all cases where a user's
browser somehow ends up with a mix of cookies from multiple legitimate
login sessions on different accounts on the same device.
Because there no longer seems to be any case where this mismatch occurs
and where the response should be allowed through, this commit introduces
a feature toggle `ENFORCE_SAFE_SESSIONS` which will destroy the active
session and overwrite the response.
The plan is to make this behavior available in the next named release and
permanent in the one after.
Also:
- Use less fragile method of checking mocked set_attribute calls in tests
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
We didn't see any errors after enabling this feature toggle, so remove it
in favor of the "True" setting.
Compare to PR #29306, which created the toggle.
ref: ARCHBOM-1952
Contingent on new feature toggle `VERIFY_USER_CHANGE_UNCONDITIONAL`, check
for request/response user mismatches on all requests, not just those
setting a session cookie on the response.
This is intended to *restore* an older behavior. I believe that almost all
requests used to set a new session cookie, and for some reason no longer
do, so this is really just an attempt to return to that previous behavior
no matter whether a new session cookie will be set. (Previously, the
cookie-to-be-deleted check would still have been in effect, so this is
actually a slight change from the earlier behavior -- the logout response
will now be included, and then quickly ignored due to a later check.)
The off-by-default switch moves several lines of code out of a try block,
but also out from under an if guard that checks for certain cookie
conditions. The movement out of the try block should be irrelevant, since
neither of the relocated lines should be raising a SafeCookieError.
However, there is some chance that they could raise other exceptions when
called from their new location (and new situations), hence the use of a
feature toggle -- we'll want to make it easy to switch the new behavior off
quickly if we start seeing an increase in errors.
Once the change is well-exercised, we can remove the toggle and the old
call locations.
I'm not entirely sure about the change to the `verify_error` utility
function in the unit tests, but it seems like even unauthenticated requests
in Django end up with a user and session on the request object, so this is
probably a close-enough way to mock that out.
I duplicated a couple of tests to test with feature toggle on/off.
ref: ARCHBOM-1952
- 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
