🐛 Fix SessionRefresh flow for new architecture #112
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stevenbal
force-pushed
the
issue/fix-infinite-redirect-on-sessionrefresh
branch
from
eaff13f
to
4809d7c
Compare
stevenbal
changed the title
9 tasks
stevenbal
force-pushed
the
issue/fix-infinite-redirect-on-sessionrefresh
branch
2 times, most recently
from
b7fbb42
to
515e6c7
Compare
stevenbal
force-pushed
the
issue/fix-infinite-redirect-on-sessionrefresh
branch
from
515e6c7
to
d0561ae
Compare
sergei-maertens
requested changes
Jun 26, 2024
stevenbal
force-pushed
the
issue/fix-infinite-redirect-on-sessionrefresh
branch
5 times, most recently
from
018fa6d
to
a4b45eb
Compare
Comment on lines
138
to
148
| django_login_response = client.get(login_url) | ||
| assert django_login_response.status_code == 302 | ||
|
|
||
| # simulate login to Keycloak | ||
| redirect_uri = keycloak_login(django_login_response["Location"], session=session) | ||
|
|
||
| # complete the login flow on our end | ||
| callback_response = client.get(redirect_uri) | ||
|
|
||
| assert callback_response.status_code == 302 | ||
| assert callback_response["Location"] == "/admin/" | ||
|
|
||
| # a user was created | ||
| assert django_user_model.objects.count() == 1 | ||
|
|
||
| admin_response = client.get("/admin/") | ||
|
|
||
| # User was successfully logged in | ||
| assert admin_response.status_code == 200 |
There was a problem hiding this comment.
I think you can clean up a bunch of assertions here that are not relevant for this test. The fact that a user is created after a valid login is already tested and asserted in another test, and removing the asserts makes it much more obvious what is actually being tested in this test.
You should always aim for a visual block structure in tests:
setup some thing
setup another thing
call the system under test
assert a thing
assert another thing
a4b45eb
to
e04febd
Compare
sergei-maertens
force-pushed
the
issue/fix-infinite-redirect-on-sessionrefresh
branch
from
e04febd
to
ebed28c
Compare
previously, the callback view never had access to the config class, because this is deleted after successful login by mozilla-django-oidc, causing the callbackview to crash
stevenbal
force-pushed
the
issue/fix-infinite-redirect-on-sessionrefresh
branch
from
ebed28c
to
bcd89a2
Compare
this field was never actually used, and it does not make sense to make settings that refer to Django stuff (URL patterns in this case) manually configurable instead of programmatically. It caused issues in Open Forms (#4435) where it required manual action from admins if it was kept as a model field.
previously it was not possible to fall back on falsy defaults (empty lists, sets, strings, etc.), this made it required to specify settings such as OIDC_EXEMPT_URLS in order to not let the SessionRefresh middleware crash
and add missing testapp migrations
stevenbal
force-pushed
the
issue/fix-infinite-redirect-on-sessionrefresh
branch
from
71f86d4
to
9cdd3c6
Compare
sergei-maertens
approved these changes
Jul 2, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
issue: open-formulieren/open-forms#4435