Skip to content

Commit 526159d

Browse files
ioigoumeioigoume
committed
Remove loop handler query parameter. Loop prevention in passive mode is not a realistic scenario.
1 parent f71834b commit 526159d

2 files changed

Lines changed: 35 additions & 119 deletions

File tree

‎src/Controller/LoginController.php‎

Lines changed: 7 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,6 @@ public function login(
176176
// Protocol (gateway set): CAS MUST NOT prompt for credentials during this branch.
177177
if ($gateway && !$this->authSource->isAuthenticated() && !$requestForceAuthenticate) {
178178
$response = $this->handleUnauthenticatedGateway(
179-
$request,
180179
$serviceUrl,
181180
$entityId,
182181
$returnToUrl,
@@ -503,7 +502,6 @@ private function handleInteractiveAuthenticate(
503502
* - null to indicate: proceed with interactive login (non-passive).
504503
*/
505504
private function handleUnauthenticatedGateway(
506-
Request $request,
507505
?string $serviceUrl,
508506
?string $entityId,
509507
string $returnToUrl,
@@ -520,24 +518,13 @@ private function handleUnauthenticatedGateway(
520518
return $this->gatewayFallback($serviceUrl);
521519
}
522520

523-
// Passive mode enabled: try a passive (non-interactive) authentication once
524-
// OPTIONAL (implementation): Attempt passive exactly once while avoiding loops.
525-
$gatewayTried = $this->getRequestParam($request, 'gatewayTried');
526-
if ($gatewayTried !== '1') {
527-
$rt = str_contains($returnToUrl, 'gatewayTried=')
528-
? $returnToUrl
529-
: $returnToUrl . (str_contains($returnToUrl, '?') ? '&' : '?') . 'gatewayTried=1';
530-
531-
return $this->handleAuthenticate(
532-
forceAuthn: false,
533-
gateway: true,
534-
returnToUrl: $rt,
535-
entityId: $entityId,
536-
);
537-
}
538-
539-
// Passive attempt already performed and still not authenticated.
540-
return $this->gatewayFallback($serviceUrl);
521+
// Passive mode enabled: attempt a passive (non-interactive) authentication.
522+
return $this->handleAuthenticate(
523+
forceAuthn: false,
524+
gateway: true,
525+
returnToUrl: $returnToUrl,
526+
entityId: $entityId,
527+
);
541528
}
542529

543530
/**

‎tests/src/Controller/LoginControllerTest.php‎

Lines changed: 28 additions & 99 deletions
Original file line numberDiff line numberDiff line change
@@ -318,6 +318,7 @@ public function testValidServiceUrl(string $serviceParam, string $redirectURL, b
318318
parameters: $queryParameters,
319319
);
320320

321+
/** @psalm-suppress InvalidArgument */
321322
$response = $controllerMock->login($loginRequest, ...$queryParameters);
322323
$this->assertInstanceOf(RunnableResponse::class, $response);
323324
$arguments = $response->getArguments();
@@ -327,15 +328,35 @@ public function testValidServiceUrl(string $serviceParam, string $redirectURL, b
327328
$this->assertEquals('redirectTrustedURL', $callable[1] ?? '');
328329
}
329330

330-
public function testGatewayPassiveDisabledRedirectsWithoutParams(): void
331+
/**
332+
* @return array<array{0:string}>
333+
*/
334+
public function serviceUrlsProvider(): array
335+
{
336+
return [
337+
['https://example.com/ssp/module.php/cas/linkback.php'],
338+
['https://example.com/ssp/module.php/cas/linkback.php?foo=1&bar=2'],
339+
];
340+
}
341+
342+
/**
343+
* When passive is disabled and a service is provided, CAS must redirect to the service without appending CAS params
344+
*
345+
* @dataProvider serviceUrlsProvider
346+
*/
347+
public function testGatewayPassiveDisabledRedirectsWithoutParams(string $serviceUrl): void
331348
{
332349
// enable_passive_mode disabled
333350
$moduleConfig = $this->moduleConfig;
334351
$moduleConfig['enable_passive_mode'] = false;
352+
353+
// Ensure the exact service URL (including its query string, if any) is allowed
354+
$moduleConfig['legal_service_urls'] = [$serviceUrl];
355+
335356
$casconfig = Configuration::loadFromArray($moduleConfig);
336357

337358
$params = [
338-
'service' => 'https://example.com/ssp/module.php/cas/linkback.php',
359+
'service' => $serviceUrl,
339360
'gateway' => true,
340361
];
341362
$loginRequest = Request::create(
@@ -355,15 +376,16 @@ public function testGatewayPassiveDisabledRedirectsWithoutParams(): void
355376
$controllerMock->expects($this->once())->method('getSession')->willReturn($this->sessionMock);
356377
$this->sessionMock->expects($this->once())->method('getSessionId')->willReturn(session_create_id());
357378

379+
// Execute
358380
$response = $controllerMock->login($loginRequest, ...$params);
359381

382+
// Validate redirect with original service URL and no CAS parameters appended
360383
$this->assertInstanceOf(RunnableResponse::class, $response);
361384
$callable = (array)$response->getCallable();
362385
$this->assertEquals('redirectTrustedURL', $callable[1] ?? '');
363386

364-
// Service URL unchanged and NO CAS params appended
365387
$arguments = $response->getArguments();
366-
$this->assertEquals('https://example.com/ssp/module.php/cas/linkback.php', $arguments[0]);
388+
$this->assertEquals($serviceUrl, $arguments[0]);
367389
$this->assertSame([], $arguments[1] ?? []);
368390
}
369391

@@ -403,7 +425,7 @@ public function testGatewayPassiveEnabledPerformsPassiveAttempt(): void
403425
$callable = (array)$response->getCallable();
404426
$this->assertEquals('login', $callable[1] ?? '');
405427

406-
// Verify isPassive=true, ForceAuthn=false, ReturnTo contains gatewayTried=1
428+
// Verify isPassive=true, ForceAuthn=false
407429
$arguments = $response->getArguments();
408430
$actualLoginParams = $arguments[0] ?? [];
409431
$this->assertArrayHasKey('ForceAuthn', $actualLoginParams);
@@ -412,50 +434,6 @@ public function testGatewayPassiveEnabledPerformsPassiveAttempt(): void
412434
$this->assertTrue($actualLoginParams['isPassive']);
413435
$this->assertArrayHasKey('ReturnTo', $actualLoginParams);
414436
$this->assertIsString($actualLoginParams['ReturnTo']);
415-
$this->assertStringContainsString('gatewayTried=1', $actualLoginParams['ReturnTo']);
416-
}
417-
418-
public function testGatewayPassiveEnabledSecondPassRedirectsWithoutParams(): void
419-
{
420-
// enable_passive_mode enabled
421-
$moduleConfig = $this->moduleConfig;
422-
$moduleConfig['enable_passive_mode'] = true;
423-
$casconfig = Configuration::loadFromArray($moduleConfig);
424-
425-
// Include gatewayTried in the Request only
426-
$requestParams = [
427-
'service' => 'https://example.com/ssp/module.php/cas/linkback.php',
428-
'gateway' => true,
429-
'gatewayTried' => '1', // simulate second pass after passive attempt
430-
];
431-
$loginRequest = Request::create(
432-
uri: Module::getModuleURL('casserver/login'),
433-
parameters: $requestParams,
434-
);
435-
436-
$controllerMock = $this->getMockBuilder(LoginController::class)
437-
->setConstructorArgs([$this->sspConfig, $casconfig, $this->authSimpleMock, $this->httpUtils])
438-
->onlyMethods(['getSession'])
439-
->getMock();
440-
441-
$this->authSimpleMock->expects($this->atLeastOnce())->method('isAuthenticated')->willReturn(false);
442-
443-
$controllerMock->expects($this->once())->method('getSession')->willReturn($this->sessionMock);
444-
$this->sessionMock->expects($this->once())->method('getSessionId')->willReturn(session_create_id());
445-
446-
// Do not pass 'gatewayTried' as named argument
447-
$callParams = $requestParams;
448-
unset($callParams['gatewayTried']);
449-
450-
$response = $controllerMock->login($loginRequest, ...$callParams);
451-
452-
$this->assertInstanceOf(RunnableResponse::class, $response);
453-
$callable = (array)$response->getCallable();
454-
$this->assertEquals('redirectTrustedURL', $callable[1] ?? '');
455-
456-
$arguments = $response->getArguments();
457-
$this->assertEquals('https://example.com/ssp/module.php/cas/linkback.php', $arguments[0]);
458-
$this->assertSame([], $arguments[1] ?? []);
459437
}
460438

461439
public function testGatewayNoServicePassiveDisabledFallsBackToInteractive(): void
@@ -572,6 +550,7 @@ public function testAuthenticatedPostSubmitsViaPostWithTicket(): void
572550
],
573551
]);
574552

553+
/** @psalm-suppress InvalidArgument */
575554
$response = $controllerMock->login($loginRequest, ...$requestParams);
576555

577556
$this->assertInstanceOf(RunnableResponse::class, $response);
@@ -585,54 +564,4 @@ public function testAuthenticatedPostSubmitsViaPostWithTicket(): void
585564
$this->assertArrayHasKey('ticket', $params);
586565
$this->assertStringStartsWith('ST-', $params['ticket']);
587566
}
588-
589-
public function testGatewayRedirectPreservesServiceQueryWithoutTicket(): void
590-
{
591-
// Passive disabled to trigger direct redirect with no ticket
592-
$moduleConfig = $this->moduleConfig;
593-
$moduleConfig['enable_passive_mode'] = false;
594-
595-
// Service URL with existing query parameters
596-
$serviceWithQuery = 'https://example.com/ssp/module.php/cas/linkback.php?foo=1&bar=2';
597-
// Ensure the exact service URL (including its query string) is allowed
598-
$moduleConfig['legal_service_urls'] = [$serviceWithQuery];
599-
600-
$casconfig = Configuration::loadFromArray($moduleConfig);
601-
602-
$params = [
603-
'service' => $serviceWithQuery,
604-
'gateway' => true,
605-
];
606-
607-
$loginRequest = Request::create(
608-
uri: Module::getModuleURL('casserver/login'),
609-
parameters: $params,
610-
);
611-
612-
$controllerMock = $this->getMockBuilder(LoginController::class)
613-
->setConstructorArgs([$this->sspConfig, $casconfig, $this->authSimpleMock, $this->httpUtils])
614-
->onlyMethods(['getSession'])
615-
->getMock();
616-
617-
// Unauthenticated so gateway path is exercised
618-
$this->authSimpleMock->expects($this->atLeastOnce())->method('isAuthenticated')->willReturn(false);
619-
620-
// Session used to build ReturnTo
621-
$controllerMock->expects($this->once())->method('getSession')->willReturn($this->sessionMock);
622-
$this->sessionMock->expects($this->once())->method('getSessionId')->willReturn(session_create_id());
623-
624-
// Execute
625-
$response = $controllerMock->login($loginRequest, ...$params);
626-
627-
// Validate redirect with original query intact and no CAS parameters appended
628-
$this->assertInstanceOf(RunnableResponse::class, $response);
629-
$callable = (array)$response->getCallable();
630-
$this->assertEquals('redirectTrustedURL', $callable[1] ?? '');
631-
632-
$arguments = $response->getArguments();
633-
// URL should be exactly the original service URL including its query string
634-
$this->assertEquals($serviceWithQuery, $arguments[0]);
635-
// No additional parameters (e.g., no ticket) should be appended by CAS
636-
$this->assertSame([], $arguments[1] ?? []);
637-
}
638567
}

0 commit comments

Comments
 (0)