Skip to content

Commit

Permalink
review fixes #1
Browse files Browse the repository at this point in the history
  • Loading branch information
@ioigoume
ioigoume committed Jan 2, 2025
1 parent 5ea16ac commit a1780a8
Show file tree
Hide file tree
Showing 6 changed files with 69 additions and 57 deletions.
77 changes: 47 additions & 30 deletions src/Cas/ServiceValidator.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,53 +29,70 @@ public function __construct(Configuration $mainConfig)

/**
* Check that the $service is allowed, and if so return the configuration to use.
* @param string $service The service url. Assume to already be url decoded
*
* @param string $service The service url. Assume to already be url decoded
*
* @return Configuration|null Return the configuration to use for this service, or null if service is not allowed
* @throws \ErrorException
*/
public function checkServiceURL(string $service): ?Configuration
{
$isValidService = false;
$legalUrl = 'undefined';
$configOverride = null;
foreach ($this->mainConfig->getOptionalArray('legal_service_urls', []) as $index => $value) {
$legalServiceUrlsConfig = $this->mainConfig->getOptionalArray('legal_service_urls', []);

foreach ($legalServiceUrlsConfig as $index => $value) {
// Support two styles: 0 => 'https://example' and 'https://example' => [ extra config ]
if (is_int($index)) {
$legalUrl = $value;
$configOverride = null;
} else {
$legalUrl = $index;
$configOverride = $value;
}
$legalUrl = \is_int($index) ? $value : $index;
if (empty($legalUrl)) {
Logger::warning("Ignoring empty CAS legal service url '$legalUrl'.");
continue;
}
if (!ctype_alnum($legalUrl[0])) {
// Probably a regex. Suppress errors incase the format is invalid
$result = @preg_match($legalUrl, $service);
if ($result === 1) {
$isValidService = true;
break;
} elseif ($result === false) {
Logger::warning("Invalid CAS legal service url '$legalUrl'. Error " . preg_last_error());

$configOverride = \is_int($index) ? null : $value;

// URL String
if (str_starts_with($service, $legalUrl)) {
$isValidService = true;
break;
}

// Regex
// Since "If the regex pattern passed does not compile to a valid regex, an E_WARNING is emitted. "
// we will throw an exception if the warning is emitted and use try-catch to handle it
set_error_handler(static function ($severity, $message, $file, $line) {
throw new \ErrorException($message, $severity, $severity, $file, $line);
}, E_WARNING);

try {
$result = preg_match($legalUrl, $service);
if ($result !== 1) {
throw new \RuntimeException('Service URL does not match legal service URL.');
}
} elseif (strpos($service, $legalUrl) === 0) {
$isValidService = true;
break;
} catch (\Exception $e) {
// do nothing
Logger::warning("Invalid CAS legal service url '$legalUrl'. Error " . preg_last_error());
} finally {
restore_error_handler();
}
}
if ($isValidService) {
$serviceConfig = $this->mainConfig->toArray();
// Return contextual information about which url rule triggered the validation
$serviceConfig['casService'] = [
'matchingUrl' => $legalUrl,
'serviceUrl' => $service,
];
if ($configOverride) {
$serviceConfig = array_merge($serviceConfig, $configOverride);
}
return Configuration::loadFromArray($serviceConfig);

if (!$isValidService) {
return null;
}

$serviceConfig = $this->mainConfig->toArray();
// Return contextual information about which url rule triggered the validation
$serviceConfig['casService'] = [
'matchingUrl' => $legalUrl,
'serviceUrl' => $service,
];
if ($configOverride) {
$serviceConfig = array_merge($serviceConfig, $configOverride);
}
return null;
return Configuration::loadFromArray($serviceConfig);
}
}
8 changes: 5 additions & 3 deletions src/Cas/Ticket/SQLTicketStore.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -321,9 +321,11 @@ private function get(string $key): ?array


/**
* @param string $key
* @param array $value
* @param int|null $expire
* @param string $key
* @param array $value
* @param int|null $expire
*
* @throws Exception
*/
private function set(string $key, array $value, int $expire = null): void
{
Expand Down
11 changes: 2 additions & 9 deletions src/Controller/Cas10Controller.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,15 +144,8 @@ public function validate(
);
}

// Get the username field
$usernameField = $this->casConfig->getOptionalValue('attrname', 'eduPersonPrincipalName');

// Fail if the username is not present in the ticket
if (empty($serviceTicket['userName'])) {
Logger::error(
'casserver:validate: internal server error. Missing user name attribute: '
. var_export($usernameField, true),
);
return new Response(
$this->cas10Protocol->getValidateFailureResponse(),
Response::HTTP_BAD_REQUEST,
Expand All @@ -169,9 +162,9 @@ public function validate(
/**
* Used by the unit tests
*
* @return mixed
* @return TicketStore
*/
public function getTicketStore(): mixed
public function getTicketStore(): TicketStore
{
return $this->ticketStore;
}
Expand Down
8 changes: 4 additions & 4 deletions src/Controller/Cas20Controller.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ public function __construct(
*/
public function serviceValidate(
Request $request,
#[MapQueryParameter] string $TARGET = null,
#[MapQueryParameter] ?string $TARGET = null,
#[MapQueryParameter] bool $renew = false,
#[MapQueryParameter] ?string $ticket = null,
#[MapQueryParameter] ?string $service = null,
Expand Down Expand Up @@ -124,7 +124,7 @@ public function proxy(
$legal_target_service_urls = $this->casConfig->getOptionalValue('legal_target_service_urls', []);
// Fail if
$message = match (true) {
// targetService pareameter is not defined
// targetService parameter is not defined
$targetService === null => 'Missing target service parameter [targetService]',
// pgt parameter is not defined
$pgt === null => 'Missing proxy granting ticket parameter: [pgt]',
Expand Down Expand Up @@ -226,9 +226,9 @@ public function proxyValidate(
/**
* Used by the unit tests
*
* @return mixed
* @return TicketStore
*/
public function getTicketStore(): mixed
public function getTicketStore(): TicketStore
{
return $this->ticketStore;
}
Expand Down
18 changes: 9 additions & 9 deletions src/Controller/LoginController.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,13 +134,13 @@ public function login(
Request $request,
#[MapQueryParameter] bool $renew = false,
#[MapQueryParameter] bool $gateway = false,
#[MapQueryParameter] string $service = null,
#[MapQueryParameter] string $TARGET = null,
#[MapQueryParameter] string $scope = null,
#[MapQueryParameter] string $language = null,
#[MapQueryParameter] string $entityId = null,
#[MapQueryParameter] string $debugMode = null,
#[MapQueryParameter] string $method = null,
#[MapQueryParameter] ?string $service = null,
#[MapQueryParameter] ?string $TARGET = null,
#[MapQueryParameter] ?string $scope = null,
#[MapQueryParameter] ?string $language = null,
#[MapQueryParameter] ?string $entityId = null,
#[MapQueryParameter] ?string $debugMode = null,
#[MapQueryParameter] ?string $method = null,
): RedirectResponse|XmlResponse|null {
$forceAuthn = $renew;
$serviceUrl = $service ?? $TARGET ?? null;
Expand Down Expand Up @@ -421,9 +421,9 @@ public function getSession(): ?Session
}

/**
* @return mixed
* @return TicketStore
*/
public function getTicketStore(): mixed
public function getTicketStore(): TicketStore
{
return $this->ticketStore;
}
Expand Down
4 changes: 2 additions & 2 deletions src/Controller/LogoutController.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,9 @@ public function logout(
}

/**
* @return mixed
* @return TicketStore
*/
public function getTicketStore(): mixed
public function getTicketStore(): TicketStore
{
return $this->ticketStore;
}
Expand Down

0 comments on commit a1780a8

Please sign in to comment.