Refactor OIDC integration for DigiD/eHerkenning - init flow #4265
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
sergei-maertens
force-pushed
the
feature/4246-record-auth-context
branch
from
f199e3a to
f1cf127
Compare
sergei-maertens
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4265 +/- ##
==========================================
- Coverage 96.13% 96.13% -0.01%
==========================================
Files 733 731 -2
Lines 23538 23529 -9
Branches 2760 2762 +2
==========================================
- Hits 22629 22620 -9
+ Misses 642 641 -1
- Partials 267 268 +1 ☔ View full report in Codecov by Sentry. |
sergei-maertens
force-pushed
the
feature/4246-record-auth-context
branch
from
55792fa to
ac49266
Compare
stevenbal
requested changes
May 6, 2024
Simplified the entire authentication request flow where the user gets redirect to the relevant identity provider. There is now a single 'view' implementation that takes a config class/model to use which can be used to directly obtain the redirect target instead of having to go through multiple redirects on our own URLs. The view takes care of input sanitation and managing the authentication state. This substantially cleans up the inheritance/mixin chains for the OIDC flows and makes the code easier to follow.
The plugin is now able to figure out the final redirect target to the identity provider in one pass rather than having to perform multiple redirects. This also removes the ability of users to tamper with URLs in the
The tests were testing the implementation details way too much, so they've been updated by removing the fluff and asserting the functional aspects instead.
This is a temporary fix, the next steps will refactor the callback view so there's a single URL/entrypoint and that view will load the config to use from the session. But for now, the test suite must pass while we refactor the init phase.
These separate modules serve no shared purpose anymore, instead we can move the configuration directly into the model classes.
* Moved the eHerkenning tests to the proper module * Restructured the auth flow tests similarly to the remaining tests
sergei-maertens
force-pushed
the
feature/4246-record-auth-context
branch
from
ac49266 to
232c866
Compare
stevenbal
approved these changes
May 6, 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.
Closes #4246 partly
Changes
Checklist
Check off the items that are completed or not relevant.
Impact on features
Release management
I have updated the translations assets (you do NOT need to provide translations)
./bin/makemessages.sh./bin/compilemessages_js.shCommit hygiene