Skip to content

Commit 707aa1d

Browse files
johanibjohanib
committed
Session fix
Prior to this change, the session was injected. This change injects a session wrapper, so it does not inject stale session objects at bootstrap time.
1 parent f221872 commit 707aa1d

6 files changed

Lines changed: 68 additions & 84 deletions

File tree

‎library/EngineBlock/ApplicationSingleton.php‎

Lines changed: 0 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -335,25 +335,6 @@ public function getClientIpAddress()
335335
throw new EngineBlock_Exception('Unable to determine IP address!');
336336
}
337337

338-
/**
339-
* Logs exception and redirects user to feedback page
340-
*
341-
* @param Exception $exception
342-
* @param string $feedbackUrl Url to which the user will be redirected
343-
* @param array $feedbackInfo Optional feedback info in name/value format which will be shown on feedback page
344-
*/
345-
public function handleExceptionWithFeedback(
346-
Exception $exception,
347-
$feedbackUrl,
348-
$feedbackInfo = array()
349-
)
350-
{
351-
$messageSuffix = '-> Redirecting to feedback page';
352-
$this->reportError($exception, $messageSuffix);
353-
$this->getSession()->set('feedbackInfo', array_merge($feedbackInfo, $this->getSession()->get('feedbackInfo', [])));
354-
$this->getHttpResponse()->setRedirectUrl($feedbackUrl);
355-
}
356-
357338
//////////// HTTP COMMUNICATION
358339

359340
/**

‎src/OpenConext/EngineBlock/Service/AuthenticationStateHelper.php‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,25 +20,24 @@
2020

2121
use OpenConext\EngineBlockBundle\Authentication\AuthenticationStateInterface;
2222
use Symfony\Component\HttpFoundation\RequestStack;
23-
use Symfony\Component\HttpFoundation\Session\SessionInterface;
2423

2524
class AuthenticationStateHelper implements AuthenticationStateHelperInterface
2625
{
2726
/**
28-
* @var SessionInterface
27+
* @var RequestStack
2928
*/
30-
private $session;
29+
private $requestStack;
3130

3231
public function __construct(RequestStack $requestStack)
3332
{
34-
$this->session = $requestStack->getSession();
33+
$this->requestStack = $requestStack;
3534
}
3635

3736
/**
3837
* @return AuthenticationStateInterface
3938
*/
4039
public function getAuthenticationState()
4140
{
42-
return $this->session->get('authentication_state');
41+
return $this->requestStack->getSession()->get('authentication_state');
4342
}
4443
}

‎src/OpenConext/EngineBlock/Service/ProcessingStateHelper.php‎

Lines changed: 16 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -32,13 +32,18 @@ class ProcessingStateHelper implements ProcessingStateHelperInterface
3232
const SESSION_KEY = 'Processing';
3333

3434
/**
35-
* @var SessionInterface
35+
* @var RequestStack
3636
*/
37-
private $session;
37+
private $requestStack;
3838

3939
public function __construct(RequestStack $requestStack)
4040
{
41-
$this->session = $requestStack->getSession();
41+
$this->requestStack = $requestStack;
42+
}
43+
44+
private function session(): SessionInterface
45+
{
46+
return $this->requestStack->getSession();
4247
}
4348

4449
/**
@@ -56,9 +61,9 @@ public function addStep(
5661
) {
5762
// Add the additional checks to the session
5863
$processingStep = new ProcessingStateStep($response, $role);
59-
$processing = $this->session->get(self::SESSION_KEY);
64+
$processing = $this->session()->get(self::SESSION_KEY);
6065
$processing[$requestId][$name] = $processingStep;
61-
$this->session->set(self::SESSION_KEY, $processing);
66+
$this->session()->set(self::SESSION_KEY, $processing);
6267

6368
return $processingStep;
6469
}
@@ -72,7 +77,7 @@ public function addStep(
7277
*/
7378
public function getStepByRequestId($requestId, $name)
7479
{
75-
$processing = $this->session->get(self::SESSION_KEY);
80+
$processing = $this->session()->get(self::SESSION_KEY);
7681
if (empty($processing)) {
7782
throw new EngineBlock_Corto_Module_Services_SessionLostException('Session lost');
7883
}
@@ -92,7 +97,7 @@ public function getStepByRequestId($requestId, $name)
9297

9398
public function hasStepRequestById(string $requestId, string $name): bool
9499
{
95-
$processing = $this->session->get(self::SESSION_KEY);
100+
$processing = $this->session()->get(self::SESSION_KEY);
96101
if (empty($processing)) {
97102
return false;
98103
}
@@ -119,11 +124,11 @@ public function updateStepResponseByRequestId(
119124
$name,
120125
EngineBlock_Saml2_ResponseAnnotationDecorator $response
121126
) {
122-
$processing = $this->session->get(self::SESSION_KEY);
127+
$processing = $this->session()->get(self::SESSION_KEY);
123128
$processingStep = $this->getStepByRequestId($requestId, $name);
124129
$updatedProcessingStep = new ProcessingStateStep($response, $processingStep->getRole());
125130
$processing[$requestId][$name] = $updatedProcessingStep;
126-
$this->session->set(self::SESSION_KEY, $processing);
131+
$this->session()->set(self::SESSION_KEY, $processing);
127132

128133
return $updatedProcessingStep;
129134
}
@@ -135,7 +140,7 @@ public function updateStepResponseByRequestId(
135140
*/
136141
public function clearStepByRequestId($requestId)
137142
{
138-
$processing = $this->session->get(self::SESSION_KEY);
143+
$processing = $this->session()->get(self::SESSION_KEY);
139144
if (empty($processing)) {
140145
throw new EngineBlock_Corto_Module_Services_SessionLostException('Session lost after consent');
141146
}
@@ -147,6 +152,6 @@ public function clearStepByRequestId($requestId)
147152

148153
unset($processing[$requestId]);
149154

150-
$this->session->set(self::SESSION_KEY, $processing);
155+
$this->session()->set(self::SESSION_KEY, $processing);
151156
}
152157
}

‎src/OpenConext/EngineBlockBridge/ErrorReporter.php‎

Lines changed: 38 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@
2525
use Exception;
2626
use Psr\Log\LoggerInterface;
2727
use Symfony\Component\HttpFoundation\RequestStack;
28-
use Symfony\Component\HttpFoundation\Session\SessionInterface;
2928

3029
class ErrorReporter
3130
{
@@ -38,9 +37,9 @@ class ErrorReporter
3837
*/
3938
private $engineBlockApplicationSingleton;
4039
/**
41-
* @var SessionInterface
40+
* @var RequestStack
4241
*/
43-
private $session;
42+
private $requestStack;
4443

4544
public function __construct(
4645
EngineBlock_ApplicationSingleton $engineBlockApplicationSingleton,
@@ -49,63 +48,64 @@ public function __construct(
4948
) {
5049
$this->engineBlockApplicationSingleton = $engineBlockApplicationSingleton;
5150
$this->logger = $logger;
52-
$this->session = $requestStack->getSession();
51+
$this->requestStack = $requestStack;
5352
}
5453

55-
/**
56-
* @param Exception $exception
57-
* @param string $messageSuffix
58-
*/
59-
public function reportError(Exception $exception, $messageSuffix)
54+
public function reportError(Exception $exception, string $messageSuffix): void
6055
{
61-
$logContext = ['exception' => $exception];
56+
$logContext = $this->buildLogContext($exception);
57+
$severity = $exception instanceof EngineBlock_Exception
58+
? $exception->getSeverity()
59+
: EngineBlock_Exception::CODE_ERROR;
6260

63-
if ($exception instanceof EngineBlock_Exception) {
64-
$severity = $exception->getSeverity();
65-
} else {
66-
$severity = EngineBlock_Exception::CODE_ERROR;
61+
$message = $exception->getMessage() ?: 'Exception without message "' . get_class($exception) . '"';
62+
if ($messageSuffix) {
63+
$message .= ' | ' . $messageSuffix;
6764
}
6865

69-
// unwrap the exception stack
70-
$prevException = $exception;
71-
while ($prevException = $prevException->getPrevious()) {
72-
if (!isset($logContext['previous_exceptions'])) {
73-
$logContext['previous_exceptions'] = [];
74-
}
66+
$this->logger->log($severity, $message, $logContext);
7567

76-
$logContext['previous_exceptions'][] = (string)$prevException;
68+
try {
69+
$this->storeSessionFeedback($exception);
70+
} finally {
71+
// flush all messages in queue, something went wrong!
72+
$this->engineBlockApplicationSingleton->flushLog('An error was caught');
7773
}
74+
}
7875

79-
// message building
80-
$message = $exception->getMessage();
81-
if (empty($message)) {
82-
$message = 'Exception without message "' . get_class($exception) . '"';
83-
}
76+
private function buildLogContext(Exception $exception): array
77+
{
78+
$logContext = ['exception' => $exception];
79+
$prevException = $exception;
8480

85-
if ($messageSuffix) {
86-
$message .= ' | ' . $messageSuffix;
81+
// unwrap the exception stack
82+
while ($prevException = $prevException->getPrevious()) {
83+
$logContext['previous_exceptions'][] = (string) $prevException;
8784
}
8885

89-
$this->logger->log($severity, $message, $logContext);
86+
return $logContext;
87+
}
9088

91-
// Store some valuable debug info in session so it can be displayed on feedback pages
92-
$feedback = $this->session->get('feedbackInfo');
93-
if (empty($feedback)) {
94-
$feedback = [];
89+
private function storeSessionFeedback(Exception $exception): void
90+
{
91+
// Store some valuable debug info in session so it can be displayed on feedback pages.
92+
// This is only applicable to web requests; skip entirely in CLI context or when there is no active request.
93+
if ($this->requestStack->getCurrentRequest() === null) {
94+
return;
9595
}
9696

97+
$session = $this->requestStack->getSession();
98+
$feedback = $session->get('feedbackInfo') ?: [];
99+
97100
if ($exception instanceof EngineBlock_Corto_Exception_HasFeedbackInfoInterface) {
98101
$feedback = array_merge($feedback, $exception->getFeedbackInfo());
99102
} elseif ($exception instanceof EngineBlock_Corto_Exception_PEPNoAccess) {
100-
$this->session->set('error_authorization_policy_decision', $exception->getPolicyDecision());
103+
$session->set('error_authorization_policy_decision', $exception->getPolicyDecision());
101104
}
102105

103-
$this->session->set('feedbackInfo', array_merge(
106+
$session->set('feedbackInfo', array_merge(
104107
$feedback,
105108
$this->engineBlockApplicationSingleton->collectFeedbackInfo($exception)
106109
));
107-
108-
// flush all messages in queue, something went wrong!
109-
$this->engineBlockApplicationSingleton->flushLog('An error was caught');
110110
}
111111
}

‎src/OpenConext/EngineBlockBundle/Controller/DebugController.php‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@
2525
use OpenConext\Value\Saml\EntityId;
2626
use OpenConext\Value\Saml\EntityType;
2727
use Symfony\Component\HttpFoundation\RequestStack;
28-
use Symfony\Component\HttpFoundation\Session\SessionInterface;
2928
use Symfony\Component\Routing\Attribute\Route;
3029

3130
class DebugController implements AuthenticationLoopThrottlingController
@@ -36,16 +35,16 @@ class DebugController implements AuthenticationLoopThrottlingController
3635
private $engineBlockApplicationSingleton;
3736

3837
/**
39-
* @var SessionInterface
38+
* @var RequestStack
4039
*/
41-
private $session;
40+
private $requestStack;
4241

4342
public function __construct(
4443
EngineBlock_ApplicationSingleton $engineBlockApplicationSingleton,
4544
RequestStack $requestStack,
4645
) {
4746
$this->engineBlockApplicationSingleton = $engineBlockApplicationSingleton;
48-
$this->session = $requestStack->getSession();
47+
$this->requestStack = $requestStack;
4948
}
5049

5150
#[Route(path: '/authentication/sp/debug', name: 'authentication_sp_debug', methods: ['GET', 'POST'])]
@@ -57,7 +56,7 @@ public function debugSpConnectionAction()
5756

5857
// Authentication state needs to be registered here as the debug flow differs from the regular flow,
5958
// yet the procedures for both are completed when consuming the assertion in the ServiceProviderController
60-
$authenticationState = $this->session->get('authentication_state');
59+
$authenticationState = $this->requestStack->getSession()->get('authentication_state');
6160

6261
$requestId = '_00000000-0000-0000-0000-000000000000';
6362
$authenticationState->startAuthenticationOnBehalfOf(

‎src/OpenConext/EngineBlockBundle/EventListener/AuthenticationStateInitializer.php‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,19 +22,18 @@
2222
use OpenConext\EngineBlockBundle\Authentication\AuthenticationState;
2323
use OpenConext\EngineBlockBundle\Controller\AuthenticationLoopThrottlingController;
2424
use Symfony\Component\HttpFoundation\RequestStack;
25-
use Symfony\Component\HttpFoundation\Session\Session;
2625
use Symfony\Component\HttpKernel\Event\ControllerEvent;
2726

2827
final class AuthenticationStateInitializer
2928
{
3029
/**
31-
* @var Session
30+
* @var RequestStack
3231
*/
33-
private $session;
32+
private $requestStack;
3433

3534
public function __construct(RequestStack $requestStack)
3635
{
37-
$this->session = $requestStack->getSession();
36+
$this->requestStack = $requestStack;
3837
}
3938

4039
public function onKernelController(ControllerEvent $event): void
@@ -48,11 +47,12 @@ public function onKernelController(ControllerEvent $event): void
4847
return;
4948
}
5049

51-
$authenticationState = $this->session->get('authentication_state');
50+
$session = $this->requestStack->getSession();
51+
$authenticationState = $session->get('authentication_state');
5252
if ($authenticationState === null) {
5353
$authenticationLoopGuard = $this->getAuthenticationLoopGuard();
5454

55-
$this->session->set('authentication_state', new AuthenticationState($authenticationLoopGuard));
55+
$session->set('authentication_state', new AuthenticationState($authenticationLoopGuard));
5656
}
5757
}
5858

0 commit comments

Comments
 (0)