* feat!: `sha1` has been deprecated in django32 and removed in django42.
* test: fix quality failure
* fixup! update custom attribute tests (#33436)
I was wondering about all the cases, so I
updated the test to reflect this. I also
made some other minor adjustments.
---------
Co-authored-by: Muhammad Soban Javed <iamsobanjaved@gmai.com>
Co-authored-by: Robert Raposa <rraposa@edx.org>
Co-authored-by: Muhammad Soban Javed <58461728+iamsobanjaved@users.noreply.github.com>
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'
