Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 [#4435] Fix infinite redirect on sessionrefresh for OIDC #4438

Merged
merged 2 commits into from
Jul 2, 2024

Conversation

stevenbal
Copy link
Contributor

@stevenbal stevenbal commented Jun 25, 2024

Closes #4435

Changes

  • Fix OIDC SessionRefresh middleware

Checklist

Check off the items that are completed or not relevant.

  • Impact on features

    • Checked copying a form
    • Checked import/export of a form
    • Config checks in the configuration overview admin page
    • Problem detection in the admin email digest is handled
  • Release management

    • I have labelled the PR as "needs-backport" accordingly
  • I have updated the translations assets (you do NOT need to provide translations)

    • Ran ./bin/makemessages_js.sh
    • Ran ./bin/compilemessages_js.sh
  • Commit hygiene

    • Commit messages refer to the relevant Github issue
    • Commit messages explain the "why" of change, not the how

@stevenbal stevenbal marked this pull request as draft June 25, 2024 07:49
@stevenbal stevenbal force-pushed the issue/4435-oidc-infinite-redirect branch from 3fe34f8 to 73dcaa5 Compare June 25, 2024 08:05
@stevenbal
Copy link
Contributor Author

@sergei-maertens since it's quite a few URLs that should be added to the exempt URLs, should I maybe add a migration or management command to do this (if the respective feature flags are set to true)?

@sergei-maertens
Copy link
Member

@sergei-maertens since it's quite a few URLs that should be added to the exempt URLs, should I maybe add a migration or management command to do this (if the respective feature flags are set to true)?

can't we instead rename the field on the config to have an underscore and implement a property we can override at runtime throught the proxy models? All of this seems like something that can be automatically inferred from settings and I would rather not burden administrators with this.

This also makes removing the deprecated code/shims easier in 3.0 as we only need to delete code and we're done, without leaving stale data in the configuration that we cannot possibly explain to users to clean up.

IMO that sounds like the simpler solution.

@stevenbal stevenbal force-pushed the issue/4435-oidc-infinite-redirect branch from 73dcaa5 to 7a59b7d Compare July 1, 2024 11:53
Copy link

codecov bot commented Jul 1, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 96.57%. Comparing base (e64ea6a) to head (3cc8cba).
Report is 571 commits behind head on master.

Files with missing lines Patch % Lines
src/openforms/conf/base.py 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4438      +/-   ##
==========================================
- Coverage   96.58%   96.57%   -0.01%     
==========================================
  Files         720      720              
  Lines       24012    24002      -10     
  Branches     2845     2844       -1     
==========================================
- Hits        23191    23180      -11     
  Misses        559      559              
- Partials      262      263       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stevenbal stevenbal force-pushed the issue/4435-oidc-infinite-redirect branch from 7a59b7d to dc87da4 Compare July 1, 2024 13:54
@stevenbal stevenbal changed the title 📌 Pin mozilla-django-oidc-db to branch 🐛 [#4435] Fix infinite redirect on sessionrefresh for OIDC Jul 2, 2024
@stevenbal stevenbal force-pushed the issue/4435-oidc-infinite-redirect branch from dc87da4 to d17b742 Compare July 2, 2024 11:07
@stevenbal stevenbal marked this pull request as ready for review July 2, 2024 11:07
@stevenbal stevenbal requested a review from sergei-maertens July 2, 2024 11:08
@stevenbal stevenbal force-pushed the issue/4435-oidc-infinite-redirect branch from d17b742 to d1a3f14 Compare July 2, 2024 12:10
stevenbal added 2 commits July 2, 2024 14:11
and django-digid-eherkenning to 0.16.0

to allow the SessionRefresh middleware to work
by default, the OIDC endpoints that are used are marked as exempt from SessionRefresh, but this caused infinite redirects in case the legacy URLs were used
@stevenbal stevenbal force-pushed the issue/4435-oidc-infinite-redirect branch from d1a3f14 to 3cc8cba Compare July 2, 2024 12:12
@stevenbal stevenbal merged commit f8a62b9 into master Jul 2, 2024
28 of 30 checks passed
@stevenbal stevenbal deleted the issue/4435-oidc-infinite-redirect branch July 2, 2024 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Infinite redirect after a while if authenticated with OIDC
2 participants