From 218d33dad21aa4a9118aea249bca3863dac18f99 Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Tue, 10 Sep 2024 13:34:51 +0200 Subject: [PATCH 1/3] Remove usage of getimagesize We can not use the url_fopen portion of the getimagesize function. This is the case when you use getimagesize with a remote file location. 1. For the ValidLogoValidator: We can exclusively use the curl based validation (already implemented in the CurlLogoValidationHelper 2. The message sent to Manage does not require a valid with and height to be set, as they will be esteablished on the logo 'cdn' See: https://www.pivotaltracker.com/n/projects/1400064/stories/186921877 --- ci/qa/phpstan-baseline.php | 47 +------------------ .../Application/Metadata/JsonGenerator.php | 37 +-------------- ...thClientCredentialsClientJsonGenerator.php | 33 +------------ .../Metadata/OidcngJsonGenerator.php | 33 +------------ .../Service/CurlLogoValidationHelper.php | 10 ++-- .../Service/LogoValidationHelperInterface.php | 4 +- .../Constraints/ValidLogoValidator.php | 8 ++-- .../Constraints/ValidLogoValidatorTest.php | 6 +-- 8 files changed, 20 insertions(+), 158 deletions(-) diff --git a/ci/qa/phpstan-baseline.php b/ci/qa/phpstan-baseline.php index 112fa9bc1..e0953ab7d 100644 --- a/ci/qa/phpstan-baseline.php +++ b/ci/qa/phpstan-baseline.php @@ -928,7 +928,7 @@ ]; $ignoreErrors[] = [ 'message' => '#^If condition is always true\\.$#', - 'count' => 2, + 'count' => 1, 'path' => __DIR__ . '/../../src/Surfnet/ServiceProviderDashboard/Application/Metadata/JsonGenerator.php', ]; $ignoreErrors[] = [ @@ -991,11 +991,6 @@ 'count' => 1, 'path' => __DIR__ . '/../../src/Surfnet/ServiceProviderDashboard/Application/Metadata/JsonGenerator.php', ]; -$ignoreErrors[] = [ - 'message' => '#^Method Surfnet\\\\ServiceProviderDashboard\\\\Application\\\\Metadata\\\\JsonGenerator\\:\\:generateLogoMetadata\\(\\) return type has no value type specified in iterable type array\\.$#', - 'count' => 1, - 'path' => __DIR__ . '/../../src/Surfnet/ServiceProviderDashboard/Application/Metadata/JsonGenerator.php', -]; $ignoreErrors[] = [ 'message' => '#^Method Surfnet\\\\ServiceProviderDashboard\\\\Application\\\\Metadata\\\\JsonGenerator\\:\\:generateOrganizationMetadata\\(\\) return type has no value type specified in iterable type array\\.$#', 'count' => 1, @@ -1011,11 +1006,6 @@ 'count' => 1, 'path' => __DIR__ . '/../../src/Surfnet/ServiceProviderDashboard/Application/Metadata/JsonGenerator.php', ]; -$ignoreErrors[] = [ - 'message' => '#^Parameter \\#1 \\$filename of function getimagesize expects string, string\\|null given\\.$#', - 'count' => 1, - 'path' => __DIR__ . '/../../src/Surfnet/ServiceProviderDashboard/Application/Metadata/JsonGenerator.php', -]; $ignoreErrors[] = [ 'message' => '#^Right side of && is always true\\.$#', 'count' => 1, @@ -1181,11 +1171,6 @@ 'count' => 1, 'path' => __DIR__ . '/../../src/Surfnet/ServiceProviderDashboard/Application/Metadata/OauthClientCredentialsClientJsonGenerator.php', ]; -$ignoreErrors[] = [ - 'message' => '#^Method Surfnet\\\\ServiceProviderDashboard\\\\Application\\\\Metadata\\\\OauthClientCredentialsClientJsonGenerator\\:\\:generateLogoMetadata\\(\\) return type has no value type specified in iterable type array\\.$#', - 'count' => 1, - 'path' => __DIR__ . '/../../src/Surfnet/ServiceProviderDashboard/Application/Metadata/OauthClientCredentialsClientJsonGenerator.php', -]; $ignoreErrors[] = [ 'message' => '#^Method Surfnet\\\\ServiceProviderDashboard\\\\Application\\\\Metadata\\\\OauthClientCredentialsClientJsonGenerator\\:\\:generateMetadataFields\\(\\) never returns float so it can be removed from the return type\\.$#', 'count' => 1, @@ -1221,11 +1206,6 @@ 'count' => 2, 'path' => __DIR__ . '/../../src/Surfnet/ServiceProviderDashboard/Application/Metadata/OauthClientCredentialsClientJsonGenerator.php', ]; -$ignoreErrors[] = [ - 'message' => '#^Parameter \\#1 \\$filename of function getimagesize expects string, string\\|null given\\.$#', - 'count' => 1, - 'path' => __DIR__ . '/../../src/Surfnet/ServiceProviderDashboard/Application/Metadata/OauthClientCredentialsClientJsonGenerator.php', -]; $ignoreErrors[] = [ 'message' => '#^Cannot call method getClientSecret\\(\\) on Surfnet\\\\ServiceProviderDashboard\\\\Domain\\\\Entity\\\\Entity\\\\OidcClientInterface\\|null\\.$#', 'count' => 1, @@ -1341,11 +1321,6 @@ 'count' => 1, 'path' => __DIR__ . '/../../src/Surfnet/ServiceProviderDashboard/Application/Metadata/OidcngJsonGenerator.php', ]; -$ignoreErrors[] = [ - 'message' => '#^Method Surfnet\\\\ServiceProviderDashboard\\\\Application\\\\Metadata\\\\OidcngJsonGenerator\\:\\:generateLogoMetadata\\(\\) return type has no value type specified in iterable type array\\.$#', - 'count' => 1, - 'path' => __DIR__ . '/../../src/Surfnet/ServiceProviderDashboard/Application/Metadata/OidcngJsonGenerator.php', -]; $ignoreErrors[] = [ 'message' => '#^Method Surfnet\\\\ServiceProviderDashboard\\\\Application\\\\Metadata\\\\OidcngJsonGenerator\\:\\:generateMetadataFields\\(\\) never returns float so it can be removed from the return type\\.$#', 'count' => 1, @@ -1381,11 +1356,6 @@ 'count' => 2, 'path' => __DIR__ . '/../../src/Surfnet/ServiceProviderDashboard/Application/Metadata/OidcngJsonGenerator.php', ]; -$ignoreErrors[] = [ - 'message' => '#^Parameter \\#1 \\$filename of function getimagesize expects string, string\\|null given\\.$#', - 'count' => 1, - 'path' => __DIR__ . '/../../src/Surfnet/ServiceProviderDashboard/Application/Metadata/OidcngJsonGenerator.php', -]; $ignoreErrors[] = [ 'message' => '#^Cannot call method getClientSecret\\(\\) on Surfnet\\\\ServiceProviderDashboard\\\\Domain\\\\Entity\\\\Entity\\\\OidcClientInterface\\|null\\.$#', 'count' => 1, @@ -3866,11 +3836,6 @@ 'count' => 1, 'path' => __DIR__ . '/../../src/Surfnet/ServiceProviderDashboard/Infrastructure/DashboardBundle/Service/CurlLogoValidationHelper.php', ]; -$ignoreErrors[] = [ - 'message' => '#^Method Surfnet\\\\ServiceProviderDashboard\\\\Infrastructure\\\\DashboardBundle\\\\Service\\\\LogoValidationHelperInterface\\:\\:validateLogo\\(\\) has no return type specified\\.$#', - 'count' => 1, - 'path' => __DIR__ . '/../../src/Surfnet/ServiceProviderDashboard/Infrastructure/DashboardBundle/Service/LogoValidationHelperInterface.php', -]; $ignoreErrors[] = [ 'message' => '#^Method Surfnet\\\\ServiceProviderDashboard\\\\Infrastructure\\\\DashboardBundle\\\\Service\\\\LogoValidationHelperInterface\\:\\:validateLogo\\(\\) has parameter \\$url with no type specified\\.$#', 'count' => 1, @@ -3956,16 +3921,6 @@ 'count' => 2, 'path' => __DIR__ . '/../../src/Surfnet/ServiceProviderDashboard/Infrastructure/DashboardBundle/Validator/Constraints/ValidLogoValidator.php', ]; -$ignoreErrors[] = [ - 'message' => '#^Method Surfnet\\\\ServiceProviderDashboard\\\\Infrastructure\\\\DashboardBundle\\\\Validator\\\\Constraints\\\\ValidLogoValidator\\:\\:getImageSizeValidation\\(\\) has parameter \\$value with no type specified\\.$#', - 'count' => 1, - 'path' => __DIR__ . '/../../src/Surfnet/ServiceProviderDashboard/Infrastructure/DashboardBundle/Validator/Constraints/ValidLogoValidator.php', -]; -$ignoreErrors[] = [ - 'message' => '#^Method Surfnet\\\\ServiceProviderDashboard\\\\Infrastructure\\\\DashboardBundle\\\\Validator\\\\Constraints\\\\ValidLogoValidator\\:\\:validate\\(\\) has parameter \\$value with no type specified\\.$#', - 'count' => 1, - 'path' => __DIR__ . '/../../src/Surfnet/ServiceProviderDashboard/Infrastructure/DashboardBundle/Validator/Constraints/ValidLogoValidator.php', -]; $ignoreErrors[] = [ 'message' => '#^Access to an undefined property Symfony\\\\Component\\\\Validator\\\\Constraint\\:\\:\\$protocols\\.$#', 'count' => 1, diff --git a/src/Surfnet/ServiceProviderDashboard/Application/Metadata/JsonGenerator.php b/src/Surfnet/ServiceProviderDashboard/Application/Metadata/JsonGenerator.php index c181fc284..b72b7f144 100644 --- a/src/Surfnet/ServiceProviderDashboard/Application/Metadata/JsonGenerator.php +++ b/src/Surfnet/ServiceProviderDashboard/Application/Metadata/JsonGenerator.php @@ -227,7 +227,7 @@ private function generateMetadataFields(ManageEntity $entity): array $metadata['coin:exclude_from_push'] = '0'; } if ($entity->getMetaData()->getLogo() instanceof Logo && $entity->getMetaData()->getLogo()->getUrl() !== '') { - $metadata = array_merge($metadata, $this->generateLogoMetadata($entity)); + $metadata['logo:0:url'] = $entity->getMetaData()->getLogo()->getUrl(); } if ($entity->getMetaData()?->getCoin()->getContractualBase() !== null) { $metadata['coin:contractual_base'] = $entity->getMetaData()->getCoin()->getContractualBase(); @@ -308,41 +308,6 @@ private function generateContactMetadata(string $contactType, int $index, Contac return $metadata; } - /** - * Generate logo metadata fields. - * - * Logo dimensions are required in the SAML spec. They are always present, - * except when the user just created the entity in the interface. We - * determine the dimensions in those situations. - * - * @SuppressWarnings(PHPMD.ErrorControlOperator) - */ - private function generateLogoMetadata(ManageEntity $entity): array - { - $logo = $entity->getMetaData()->getLogo(); - $metadata = []; - if ($logo) { - $metadata = [ - 'logo:0:url' => $logo->getUrl(), - ]; - - $logoData = @getimagesize( - $logo->getUrl() - ); - - if ($logoData !== false) { - [$width, $height] = $logoData; - } else { - $width = 50; - $height = 50; - } - - $metadata['logo:0:width'] = (string)$width; - $metadata['logo:0:height'] = (string)$height; - } - return $metadata; - } - private function generateAclData(ManageEntity $entity): array { if ($entity->getAllowedIdentityProviders()->isAllowAll()) { diff --git a/src/Surfnet/ServiceProviderDashboard/Application/Metadata/OauthClientCredentialsClientJsonGenerator.php b/src/Surfnet/ServiceProviderDashboard/Application/Metadata/OauthClientCredentialsClientJsonGenerator.php index 9c2e0408b..74af15e0f 100644 --- a/src/Surfnet/ServiceProviderDashboard/Application/Metadata/OauthClientCredentialsClientJsonGenerator.php +++ b/src/Surfnet/ServiceProviderDashboard/Application/Metadata/OauthClientCredentialsClientJsonGenerator.php @@ -192,7 +192,7 @@ private function generateMetadataFields(ManageEntity $entity): int|float|array $metadata += $this->generateOidcClient($entity); if ($entity->getMetaData()->getLogo() instanceof Logo && $entity->getMetaData()->getLogo()->getUrl() !== '') { - $metadata += $this->generateLogoMetadata($entity); + $metadata['logo:0:url'] = $entity->getMetaData()->getLogo()->getUrl(); } return $metadata; @@ -288,37 +288,6 @@ private function generateContactMetadata(string $contactType, int $index, Contac return $metadata; } - /** - * Generate logo metadata fields. - * - * Logo dimensions are required in the SAML spec. They are always present, - * except when the user just created the entity in the interface. We - * determine the dimensions in those situations. - * - * @SuppressWarnings(PHPMD.ErrorControlOperator) - */ - private function generateLogoMetadata(ManageEntity $entity): array - { - $logoUrl = $entity->getMetaData()->getLogo()->getUrl(); - $metadata = [ - 'logo:0:url' => $logoUrl, - ]; - - $logoData = @getimagesize($logoUrl); - - if ($logoData !== false) { - [$width, $height] = $logoData; - } else { - $width = 50; - $height = 50; - } - - $metadata['logo:0:width'] = (string) $width; - $metadata['logo:0:height'] = (string) $height; - - return $metadata; - } - private function generateAclData(ManageEntity $entity): array { $acl = $entity->getAllowedIdentityProviders(); diff --git a/src/Surfnet/ServiceProviderDashboard/Application/Metadata/OidcngJsonGenerator.php b/src/Surfnet/ServiceProviderDashboard/Application/Metadata/OidcngJsonGenerator.php index b5302da6a..30018791d 100644 --- a/src/Surfnet/ServiceProviderDashboard/Application/Metadata/OidcngJsonGenerator.php +++ b/src/Surfnet/ServiceProviderDashboard/Application/Metadata/OidcngJsonGenerator.php @@ -196,7 +196,7 @@ private function generateMetadataFields(ManageEntity $entity): int|float|array $metadata += $this->generateOidcClient($entity); if ($entity->getMetaData()->getLogo() instanceof Logo && $entity->getMetaData()->getLogo()->getUrl() !== '') { - $metadata += $this->generateLogoMetadata($entity); + $metadata['logo:0:url'] = $entity->getMetaData()->getLogo()->getUrl(); } if ($entity->getMetaData()?->getCoin()->getContractualBase() !== null) { $metadata['coin:contractual_base'] = $entity->getMetaData()->getCoin()->getContractualBase(); @@ -297,37 +297,6 @@ private function generateContactMetadata(string $contactType, int $index, Contac return $metadata; } - /** - * Generate logo metadata fields. - * - * Logo dimensions are required in the SAML spec. They are always present, - * except when the user just created the entity in the interface. We - * determine the dimensions in those situations. - * - * @SuppressWarnings(PHPMD.ErrorControlOperator) - */ - private function generateLogoMetadata(ManageEntity $entity): array - { - $logoUrl = $entity->getMetaData()->getLogo()->getUrl(); - $metadata = [ - 'logo:0:url' => $logoUrl, - ]; - - $logoData = @getimagesize($logoUrl); - - if ($logoData !== false) { - [$width, $height] = $logoData; - } else { - $width = 50; - $height = 50; - } - - $metadata['logo:0:width'] = (string) $width; - $metadata['logo:0:height'] = (string) $height; - - return $metadata; - } - private function generateAclData(ManageEntity $entity): array { $acl = $entity->getAllowedIdentityProviders(); diff --git a/src/Surfnet/ServiceProviderDashboard/Infrastructure/DashboardBundle/Service/CurlLogoValidationHelper.php b/src/Surfnet/ServiceProviderDashboard/Infrastructure/DashboardBundle/Service/CurlLogoValidationHelper.php index 18e498bba..87dae7e9c 100644 --- a/src/Surfnet/ServiceProviderDashboard/Infrastructure/DashboardBundle/Service/CurlLogoValidationHelper.php +++ b/src/Surfnet/ServiceProviderDashboard/Infrastructure/DashboardBundle/Service/CurlLogoValidationHelper.php @@ -33,12 +33,12 @@ public function __construct(private readonly LoggerInterface $logger) * Using Curl: tests: * - is the curl response code erroneous (>= 400) * - if the content type is correct + * Returns the location where the logo is stored temporarily * - * @param $url * @throws LogoInvalidTypeException * @throws LogoNotFoundException */ - public function validateLogo($url): void + public function validateLogo($url): string { $this->logger->debug(sprintf('Validating logo: "%s" using curl', $url)); @@ -50,7 +50,9 @@ public function validateLogo($url): void curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, 10); curl_setopt($ch, CURLOPT_TIMEOUT, 10); - curl_exec($ch); + $content = curl_exec($ch); + $fileLocation = '/tmp/logo-validation-' . uniqid(); + file_put_contents($fileLocation, $content); $contentType = curl_getinfo($ch, CURLINFO_CONTENT_TYPE); $responseCode = curl_getinfo($ch, CURLINFO_RESPONSE_CODE); @@ -76,5 +78,7 @@ public function validateLogo($url): void $this->logger->info('The logo file type is invalid'); throw new LogoInvalidTypeException('The logo file type is invalid'); } + + return $fileLocation; } } diff --git a/src/Surfnet/ServiceProviderDashboard/Infrastructure/DashboardBundle/Service/LogoValidationHelperInterface.php b/src/Surfnet/ServiceProviderDashboard/Infrastructure/DashboardBundle/Service/LogoValidationHelperInterface.php index 41d29ba2c..2ced8c17b 100644 --- a/src/Surfnet/ServiceProviderDashboard/Infrastructure/DashboardBundle/Service/LogoValidationHelperInterface.php +++ b/src/Surfnet/ServiceProviderDashboard/Infrastructure/DashboardBundle/Service/LogoValidationHelperInterface.php @@ -34,10 +34,10 @@ interface LogoValidationHelperInterface /** * Validates the logo, throws an exception if validation failed. - * + * Returns the local path to the curl'ed image * @param $url * @throws LogoInvalidTypeException * @throws LogoNotFoundException */ - public function validateLogo($url); + public function validateLogo($url): string; } diff --git a/src/Surfnet/ServiceProviderDashboard/Infrastructure/DashboardBundle/Validator/Constraints/ValidLogoValidator.php b/src/Surfnet/ServiceProviderDashboard/Infrastructure/DashboardBundle/Validator/Constraints/ValidLogoValidator.php index 234ae0ca0..aca00cba0 100644 --- a/src/Surfnet/ServiceProviderDashboard/Infrastructure/DashboardBundle/Validator/Constraints/ValidLogoValidator.php +++ b/src/Surfnet/ServiceProviderDashboard/Infrastructure/DashboardBundle/Validator/Constraints/ValidLogoValidator.php @@ -41,15 +41,15 @@ public function __construct(private readonly LogoValidationHelperInterface $logo { } - public function validate($value, Constraint $constraint): void + public function validate(mixed $value, Constraint $constraint): void { if (empty($value)) { return; } try { - $this->logoValidationHelper->validateLogo($value); - $this->getImageSizeValidation($value, $constraint); + $filePath = $this->logoValidationHelper->validateLogo($value); + $this->getImageSizeValidation($filePath, $constraint); } catch (LogoNotFoundException) { $this->context->addViolation(self::STATUS_DOWNLOAD_FAILED); return; @@ -64,7 +64,7 @@ public function validate($value, Constraint $constraint): void * * @param $value */ - private function getImageSizeValidation($value, Constraint $constraint): void + private function getImageSizeValidation(string $value, Constraint $constraint): void { try { $imgData = getimagesize($value); diff --git a/tests/integration/Infrastructure/DashboardBundle/Validator/Constraints/ValidLogoValidatorTest.php b/tests/integration/Infrastructure/DashboardBundle/Validator/Constraints/ValidLogoValidatorTest.php index 478a1d534..b665d7b3e 100644 --- a/tests/integration/Infrastructure/DashboardBundle/Validator/Constraints/ValidLogoValidatorTest.php +++ b/tests/integration/Infrastructure/DashboardBundle/Validator/Constraints/ValidLogoValidatorTest.php @@ -47,7 +47,7 @@ public function test_success_png() $this->validationHelper ->shouldReceive('validateLogo') - ->andReturn(null); + ->andReturn('file://'.__DIR__.'/fixture/logo_validator/small.png'); $this->validator->validate('file://'.__DIR__.'/fixture/logo_validator/small.png', $constraint); @@ -60,7 +60,7 @@ public function test_success_gif() $this->validationHelper ->shouldReceive('validateLogo') - ->andReturn(null); + ->andReturn('file://'.__DIR__.'/fixture/logo_validator/small.gif'); $this->validator->validate('file://'.__DIR__.'/fixture/logo_validator/small.gif', $constraint); @@ -81,7 +81,7 @@ public function test_invalid_image() $this->validationHelper ->shouldReceive('validateLogo') - ->andReturn(null); + ->andReturn('file://'.__DIR__.'/fixture/logo_validator/ufjd'); $this->validator->validate('ufjd', $constraint); From 268cdfea79a0d03b10e709f25d31b2f11c044c11 Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Mon, 16 Sep 2024 15:57:16 +0200 Subject: [PATCH 2/3] Set the correct dev managa api credentials --- .env.dev | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.env.dev b/.env.dev index 735a2c721..53564b97b 100644 --- a/.env.dev +++ b/.env.dev @@ -33,13 +33,13 @@ surfconext_representative_authorization='urn:mace:surfnet.nl:surfnet.nl:sab:role ## Manage test instance manage_test_host='https://manage.dev.openconext.local' -manage_test_username=sp-portal +manage_test_username=sp-dashboard manage_test_password=secret manage_test_publication_status=testaccepted ## Manage production instance manage_prod_host='https://manage.dev.openconext.local' -manage_prod_username=sp-portal +manage_prod_username=sp-dashboard manage_prod_password=secret manage_prod_publication_status=prodaccepted From 5e5c1de2b9ebdc0f2ad3bd08875e50b9b8599a4d Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Mon, 16 Sep 2024 15:57:38 +0200 Subject: [PATCH 3/3] No longer request the headers in the curl response --- .../DashboardBundle/Service/CurlLogoValidationHelper.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Surfnet/ServiceProviderDashboard/Infrastructure/DashboardBundle/Service/CurlLogoValidationHelper.php b/src/Surfnet/ServiceProviderDashboard/Infrastructure/DashboardBundle/Service/CurlLogoValidationHelper.php index 87dae7e9c..0b68ea925 100644 --- a/src/Surfnet/ServiceProviderDashboard/Infrastructure/DashboardBundle/Service/CurlLogoValidationHelper.php +++ b/src/Surfnet/ServiceProviderDashboard/Infrastructure/DashboardBundle/Service/CurlLogoValidationHelper.php @@ -46,7 +46,7 @@ public function validateLogo($url): string curl_setopt($ch, CURLOPT_URL, $url); curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1); curl_setopt($ch, CURLOPT_VERBOSE, 1); - curl_setopt($ch, CURLOPT_HEADER, 1); + curl_setopt($ch, CURLOPT_HEADER, 0); curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, 10); curl_setopt($ch, CURLOPT_TIMEOUT, 10);