Clear, manage and rework feedback data storage in session#1988
Draft
Clear, manage and rework feedback data storage in session#1988
Conversation
…1795) feedbackInfo (the debug context shown on SAML error pages) was stored globally in the session. This caused two bugs: 1. Info from a failed auth flow could bleed into a subsequent unrelated error because storeFeedbackInfo() merged new data on top of old. 2. currentServiceProvider and currentIdentityProvider were never cleared after a successful login, so an early error after a completed auth would still show the SP/IdP from that auth. These two scenarios are added as failing tests to document the expected behaviour before the fix is in place.
johanib
force-pushed
the
feature/EB-1795_clear-sessino
branch
5 times, most recently
from
df24b0b to
8ef3dd2
Compare
feedbackInfo was stored as a flat dict in $_SESSION['feedbackInfo'] and merged with existing data on each write. This meant info collected during one auth flow (e.g. which IdP was involved) could appear on error pages in a completely separate flow. Additionally, currentServiceProvider and currentIdentityProvider were never cleared from the session after a successful login, so they would show up on error pages for unrelated early errors. The fix: - feedbackInfo is now keyed per SAML request ID so each auth flow has its own isolated bucket with no merging across flows - clearFlowContext() is called by ProcessedAssertionConsumer after a successful auth, clearing all flow context from the session - All session access moved from raw $_SESSION to the Symfony session - Session ops for feedbackInfo and flow context are centralised in a new FeedbackStateHelper class, split out from ProcessingStateHelper - FeedbackStateHelper is wired through DiContainerRuntime instead of the legacy DiContainer
Prior to this change, issues in the symfony ci container yaml were not picked up by qa, resulting in runtime errors.
johanib
force-pushed
the
feature/EB-1795_clear-sessino
branch
from
8ef3dd2 to
b50a07a
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Prior to this change, there were two issues:
This change clears the feedback data on successful auth and stores the data separate per request, if possible.
In order to make these changes, the code needed to be refactored. The main issues were:
[FeedbackStateHelper](https://github.com/OpenConext/OpenConext-engineblock/compare/feature/EB-1795_clear-sessino?expand=1#diff-1ee213868cf761152d1481e298c752defbcaac60a770ace86f5b9b921f12abf3R24)Resolves #1795