Skip to content

Commit a69ed2c

Browse files
committed
Log rememberedidps cookie write failures
# If applied, this commit will Log an error and skip success logging when writing the rememberedidps cookie fails. # Why is this change needed? Prior to this change, a failed rememberedidps cookie write was still logged as a successful remember-choice update. # How does it address the issue? This change checks the cookie write result, logs an error for failures, and covers the behavior with a regression test. # Provide links to any relevant tickets, articles or other resources Found during review of PR #2091.
1 parent c6789a0 commit a69ed2c

2 files changed

Lines changed: 66 additions & 1 deletion

File tree

‎library/EngineBlock/Corto/Module/Service/ContinueToIdp.php‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,15 @@ private function _persistRememberedChoice(ServiceProvider $sp, $idp, $rememberCh
153153
}
154154

155155
$result = $rememberedIdpCookie->add($entries, $sp->entityId, $idp->entityId);
156-
$rememberedIdpCookie->write($result['entries'], $result['expires']);
156+
if (!$rememberedIdpCookie->write($result['entries'], $result['expires'])) {
157+
$log->error(sprintf(
158+
'WAYF-remember-my-choice cookie write failed for SP %s and IdP %s',
159+
$sp->entityId,
160+
$idp->entityId
161+
));
162+
163+
return;
164+
}
157165

158166
$log->info(sprintf(
159167
'WAYF-remember-my-choice set for SP %s and IdP %s and expiry %d',

‎tests/library/EngineBlock/Test/Corto/Module/Service/ContinueToIdpTest.php‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,63 @@ public static function rememberChoiceGateProvider(): array
135135
];
136136
}
137137

138+
public function testPersistRememberedChoiceLogsAnErrorWhenCookieWriteFails(): void
139+
{
140+
$application = EngineBlock_ApplicationSingleton::getInstance();
141+
142+
$diContainer = Phake::mock(EngineBlock_Application_DiContainer::class);
143+
Phake::when($diContainer)->getRememberChoice()->thenReturn(true);
144+
Phake::when($diContainer)->getSymfonyRequest()->thenReturn(Request::create('/'));
145+
$this->setDiContainer($application, $diContainer);
146+
147+
$cookieService = Phake::mock(CookieService::class);
148+
Phake::when($cookieService)->setCookieWithSameSite(Phake::anyParameters())->thenReturn(false);
149+
150+
$rememberedIdpCookie = new RememberedIdpCookie(
151+
new TimeProvider(),
152+
$cookieService,
153+
7776000,
154+
16,
155+
'engine.example.org',
156+
'/',
157+
true,
158+
);
159+
160+
$application->setDiContainerRuntime(new DiContainerRuntime(
161+
$this->createStub(Environment::class),
162+
$this->createStub(WayfRenderer::class),
163+
$this->createStub(CorrelationIdServiceInterface::class),
164+
new CurrentCorrelationId(),
165+
$this->createStub(FeedbackStateHelperInterface::class),
166+
$this->createStub(FeedbackInfoCollectorInterface::class),
167+
$this->createStub(LoginLogger::class),
168+
$rememberedIdpCookie,
169+
true,
170+
));
171+
172+
$logger = Phake::mock(LoggerInterface::class);
173+
$proxyServerMock = Phake::mock(EngineBlock_Corto_ProxyServer::class);
174+
Phake::when($proxyServerMock)->getLogger()->thenReturn($logger);
175+
176+
$service = new EngineBlock_Corto_Module_Service_ContinueToIdp(
177+
$proxyServerMock,
178+
Phake::mock(EngineBlock_Corto_XmlToArray::class),
179+
$this->createStub(Environment::class),
180+
Phake::mock(ServiceProviderFactory::class),
181+
);
182+
183+
$sp = new ServiceProvider('https://sp.example.org', wayfRememberChoice: true);
184+
$idp = new IdentityProvider('https://idp.example.org');
185+
186+
$method = new ReflectionMethod($service, '_persistRememberedChoice');
187+
$method->invoke($service, $sp, $idp, '1');
188+
189+
Phake::verify($logger)->error(
190+
'WAYF-remember-my-choice cookie write failed for SP https://sp.example.org and IdP https://idp.example.org'
191+
);
192+
Phake::verify($logger, Phake::never())->info(Phake::anyParameters());
193+
}
194+
138195
private function setDiContainer(EngineBlock_ApplicationSingleton $application, $diContainer): void
139196
{
140197
$property = new ReflectionProperty(EngineBlock_ApplicationSingleton::class, '_diContainer');

0 commit comments

Comments
 (0)