diff --git a/.gitignore b/.gitignore index e616e526a4..3a30032dc1 100644 --- a/.gitignore +++ b/.gitignore @@ -20,6 +20,7 @@ bin/ignore_me.php .idea local-php-security-checker /tests/e2e/node_modules +/tests/.phpunit.result.cache /languages/overrides.*.php /theme/node_modules /theme/.sass-cache diff --git a/CHANGELOG.md b/CHANGELOG.md index 4bdc6f8be9..b4d3c51310 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,9 @@ We will continue to post relevant release notes on the GitHub release page. More More information about our release strategy can be found in the [Development Guidelines](https://github.com/OpenConext/OpenConext-engineblock/wiki/Development-Guidelines#release-notes) on the EngineBlock wiki. +## 6.14.0 +* A new feature was added to allow overwriting the internal StepUp auth EntityId + ## 6.13.0 * Move most HTML from translatable strings into Twig templates, where it diff --git a/UPGRADING.md b/UPGRADING.md index add15b5fd9..cc61cebbbe 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -1,5 +1,24 @@ # UPGRADE NOTES +## 6.13 -> 6.14 +Previously the SAML EntityID of the EngineBlock SP that was used to do Stepup (SFO) authentications to the Stepup-Gateway +always was https:///authentication/stepup/metadata. For these authentication the default +EngineBlock key is always used for signing. + +If you'd like to key-rollover the StepUp entity (baked into EngineBlock). +The key used to sign the SAML AuthnRequests from this SP is the engineblock default key. + +To facilitate a rolling configuration update I want the SP entityID that is used for Stepup to be configurable so that at the same time that the engineblock default key is updated, this entityID can be changed. This then allows two entities, with two different keys, to be configured in the Stepup-Gateway. + +There are two new parameters that configure this behavior. + +1. `feature_stepup_sfo_override_engine_entityid` [bool] enables/disables the feature. Default: disabled +2. `stepup.sfo.override_engine_entityid` [string] should be set with the Entity ID you'd like to use for the stepup EntityId. Default: '' + +The feature flag was added mainly to aid our test suite to easily test this feature. + +By default this feature is disabled and the default Entity Id is used for the StepUp entity. + ## 6.12 -> 6.13 Some translatable strings have been changed and "raw" use of HTML in diff --git a/app/config/config.yml b/app/config/config.yml index 00dc0aefb4..5079db3c20 100644 --- a/app/config/config.yml +++ b/app/config/config.yml @@ -27,6 +27,8 @@ open_conext_engine_block: eb.enable_sso_notification: "%feature_enable_sso_notification%" eb.feature_enable_consent: "%feature_enable_consent%" eb.enable_sso_session_cookie: "%feature_enable_sso_session_cookie%" + eb.stepup.sfo.override_engine_entityid: "%feature_stepup_sfo_override_engine_entityid%" + swiftmailer: transport: "%mailer_transport%" diff --git a/app/config/config_test.yml b/app/config/config_test.yml index 74e1989631..8fff176bbf 100644 --- a/app/config/config_test.yml +++ b/app/config/config_test.yml @@ -48,6 +48,7 @@ monolog: activation_strategy: engineblock.logger.manual_or_error_activation_strategy passthru_level: "%logger.fingers_crossed.passthru_level%" handler: file + channels: ['!event'] file: type: stream path: "%kernel.logs_dir%/%kernel.environment%.log" diff --git a/app/config/functional_testing.yml.dist b/app/config/functional_testing.yml.dist index 5a303c6cab..c365ff0294 100644 --- a/app/config/functional_testing.yml.dist +++ b/app/config/functional_testing.yml.dist @@ -3,5 +3,6 @@ parameters: router.request_context.scheme: "https" # Where must we store the writable state of the Mock IdP and Mock SP? - idp_fixture_file: "%kernel.root_dir%/../tmp/fixtures/db/idp.states.php.serialized" - sp_fixture_file: "%kernel.root_dir%/../tmp/fixtures/db/sp.states.php.serialized" + idp_fixture_file: '/tmp/eb-fixtures/db/idp.states.php.serialized' + sp_fixture_file: '/tmp/eb-fixtures/db/sp.states.php.serialized' + stepup.sfo.override_engine_entityid: 'https://engine.vm.openconext.com/new/stepup/metadata' diff --git a/app/config/parameters.yml.dist b/app/config/parameters.yml.dist index 42b9c0bfb4..ad4fe2b23a 100644 --- a/app/config/parameters.yml.dist +++ b/app/config/parameters.yml.dist @@ -223,6 +223,7 @@ parameters: feature_run_all_manipulations_prior_to_consent: false feature_block_user_on_violation: false feature_enable_consent: true + feature_stepup_sfo_override_engine_entityid: false ########################################################################################## ## PROFILE SETTINGS @@ -261,6 +262,7 @@ parameters: stepup.gateway.sfo.sso_location: 'https://gateway.stepup.vm.openconext.org/second-factor-only/single-sign-on' ## The public key from the Stepup Gateway IdP stepup.gateway.sfo.key_file: /etc/openconext/engineblock.crt + stepup.sfo.override_engine_entityid: 'https://engine.vm.openconext.com/new/stepup/metadata' ########################################################################################## ## THEME SETTINGS diff --git a/docker/php-fpm/Dockerfile-php72 b/docker/php-fpm/Dockerfile-php72 index 4bbfb36628..57bc5dbd7e 100644 --- a/docker/php-fpm/Dockerfile-php72 +++ b/docker/php-fpm/Dockerfile-php72 @@ -3,6 +3,7 @@ FROM ghcr.io/openconext/openconext-basecontainers/php72-apache2-node16-composer2 RUN a2enmod ssl # Copy phpfpm config COPY docker/php-fpm/app.ini /usr/local/etc/php/conf.d/ +RUN rm -rf /etc/apache2/sites-enabled/* COPY docker/php-fpm/apache2.conf /etc/apache2/sites-enabled/ RUN chown -R www-data: /var/www/ WORKDIR /opt/openconext/OpenConext-engineblock diff --git a/library/EngineBlock/Application/DiContainer.php b/library/EngineBlock/Application/DiContainer.php index bf08ac5797..c9efe77b4d 100644 --- a/library/EngineBlock/Application/DiContainer.php +++ b/library/EngineBlock/Application/DiContainer.php @@ -17,7 +17,7 @@ */ use Doctrine\ORM\EntityManager; -use OpenConext\EngineBlock\Metadata\Factory\Factory\ServiceProviderFactor; +use OpenConext\EngineBlock\Metadata\Factory\Factory\ServiceProviderFactory; use OpenConext\EngineBlock\Metadata\LoaRepository; use OpenConext\EngineBlock\Metadata\MetadataRepository\MetadataRepositoryInterface; use OpenConext\EngineBlock\Service\MfaHelperInterface; @@ -521,6 +521,12 @@ protected function getStepupEndpoint() return $this->container->get('engineblock.configuration.stepup.endpoint'); } + /** @return string */ + public function getStepupEntityIdOverrideValue() + { + return $this->container->getParameter('stepup.sfo.override_engine_entityid'); + } + public function getCookieDomain() { return $this->container->getParameter('cookie.locale.domain'); diff --git a/library/EngineBlock/Corto/Module/Service/StepupAssertionConsumer.php b/library/EngineBlock/Corto/Module/Service/StepupAssertionConsumer.php index 0f417cde01..561ab0631f 100644 --- a/library/EngineBlock/Corto/Module/Service/StepupAssertionConsumer.php +++ b/library/EngineBlock/Corto/Module/Service/StepupAssertionConsumer.php @@ -73,12 +73,15 @@ public function __construct( */ public function serve($serviceName, Request $httpRequest) { - $serviceEntityId = $this->_server->getUrl('stepupMetadataService'); - $expectedDestination = $this->_server->getUrl('stepupAssertionConsumerService'); - $application = EngineBlock_ApplicationSingleton::getInstance(); $log = $application->getLogInstance(); + // EngineBlock will verify the incoming assertion's Audience which will + // be set to the entityID it used on the outgoing AuthNRequest, so this + // place will also need to handle the override if present. + $serviceEntityId = $this->determineRemoteSpEntityId(); + $expectedDestination = $this->_server->getUrl('stepupAssertionConsumerService'); + $checkResponseSignature = true; try { $receivedResponse = $this->_server->getBindingsModule()->receiveResponse($serviceEntityId, $expectedDestination); @@ -138,6 +141,30 @@ public function serve($serviceName, Request $httpRequest) return; } + /** + * Returns the `stepupMetadataService` if no override is defined. + * To define an override (for StepUp key rollover) configure: + * `eb.stepup.sfo.override_engine_entityid`. See UPGRADING.md (6.13 -> 6.14) + * for details. + * + * @return string + */ + private function determineRemoteSpEntityId() + { + $application = EngineBlock_ApplicationSingleton::getInstance(); + $container = $application->getDiContainer(); + $entityIdOverrideValue = $container->getStepupEntityIdOverrideValue(); + $features = $container->getFeatureConfiguration(); + $isConfigured = $features->hasFeature('eb.stepup.sfo.override_engine_entityid'); + $isEnabled = $features->isEnabled('eb.stepup.sfo.override_engine_entityid'); + + $serviceEntityId = $this->_server->getUrl('stepupMetadataService'); + if ($isEnabled && $isConfigured) { + return $entityIdOverrideValue; + } + return $serviceEntityId; + } + /** * @return AuthenticationState */ diff --git a/library/EngineBlock/Corto/ProxyServer.php b/library/EngineBlock/Corto/ProxyServer.php index 3b1e47c846..1741a253e6 100644 --- a/library/EngineBlock/Corto/ProxyServer.php +++ b/library/EngineBlock/Corto/ProxyServer.php @@ -16,6 +16,7 @@ * limitations under the License. */ +use OpenConext\EngineBlock\Exception\MissingParameterException; use OpenConext\EngineBlock\Metadata\Entity\AbstractRole; use OpenConext\EngineBlock\Metadata\Entity\IdentityProvider; use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; @@ -487,6 +488,32 @@ public function sendStepupAuthenticationRequest( $nameIdOverwrite->setFormat(Constants::NAMEID_UNSPECIFIED); $sspMessage->setNameId($nameIdOverwrite); + // See: UPGRADING.md -> ## 6.13 -> 6.14 + $container = EngineBlock_ApplicationSingleton::getInstance()->getDiContainer(); + $entityIdOverrideValue = $container->getStepupEntityIdOverrideValue(); + $features = $container->getFeatureConfiguration(); + $isConfigured = $features->hasFeature('eb.stepup.sfo.override_engine_entityid'); + $isEnabled = $features->isEnabled('eb.stepup.sfo.override_engine_entityid'); + + if ($isEnabled && $isConfigured) { + if (empty($entityIdOverrideValue)) { + throw new MissingParameterException( + 'When feature "feature_stepup_sfo_override_engine_entityid" is enabled, + you must provide the "stepup.sfo.override_engine_entityid" parameter.' + ); + } + $this->_logger->notice( + sprintf( + 'Feature eb.stepup.sfo.override_engine_entityid is enabled, overriding the Issuer of the AR to the ' . + 'StepUp Gateway. Updated the Issuer to "%s"', + $entityIdOverrideValue + ) + ); + $issuer = new Issuer(); + $issuer->setValue($entityIdOverrideValue); + $sspMessage->setIssuer($issuer); + } + // Link with the original Request $authnRequestRepository = new EngineBlock_Saml2_AuthnRequestSessionRepository($this->_logger); $authnRequestRepository->store($spRequest); diff --git a/src/OpenConext/EngineBlock/Metadata/Factory/Factory/ServiceProviderFactory.php b/src/OpenConext/EngineBlock/Metadata/Factory/Factory/ServiceProviderFactory.php index 4402efbd0e..d49b21759a 100644 --- a/src/OpenConext/EngineBlock/Metadata/Factory/Factory/ServiceProviderFactory.php +++ b/src/OpenConext/EngineBlock/Metadata/Factory/Factory/ServiceProviderFactory.php @@ -19,6 +19,7 @@ namespace OpenConext\EngineBlock\Metadata\Factory\Factory; use EngineBlock_Attributes_Metadata as AttributesMetadata; +use OpenConext\EngineBlock\Exception\MissingParameterException; use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; use OpenConext\EngineBlock\Metadata\Factory\Adapter\ServiceProviderEntity; use OpenConext\EngineBlock\Metadata\Factory\Decorator\EngineBlockServiceProvider; @@ -28,11 +29,13 @@ use OpenConext\EngineBlock\Metadata\Factory\ValueObject\EngineBlockConfiguration; use OpenConext\EngineBlock\Metadata\Mdui; use OpenConext\EngineBlock\Metadata\X509\KeyPairFactory; +use OpenConext\EngineBlockBundle\Configuration\FeatureConfigurationInterface; use OpenConext\EngineBlockBundle\Url\UrlProvider; /** * This factory is used for instantiating an entity with the required adapters and/or decorators set. * It also makes sure that static, internally used, entities can be generated without the use of the database. + * @SuppressWarnings(PHPMD.CouplingBetweenObjects) */ class ServiceProviderFactory { @@ -55,16 +58,30 @@ class ServiceProviderFactory */ private $urlProvider; + /** + * @var string + */ + private $entityIdOverrideValue; + + /** + * @var FeatureConfigurationInterface + */ + private $featureConfiguration; + public function __construct( AttributesMetadata $attributes, KeyPairFactory $keyPairFactory, EngineBlockConfiguration $engineBlockConfiguration, - UrlProvider $urlProvider + UrlProvider $urlProvider, + FeatureConfigurationInterface $featureConfiguration, + string $entityIdOverrideValue ) { $this->attributes = $attributes; $this->keyPairFactory = $keyPairFactory; $this->engineBlockConfiguration = $engineBlockConfiguration; $this->urlProvider = $urlProvider; + $this->featureConfiguration = $featureConfiguration; + $this->entityIdOverrideValue = $entityIdOverrideValue; } public function createEngineBlockEntityFrom(string $keyId): ServiceProviderEntityInterface @@ -86,8 +103,20 @@ public function createEngineBlockEntityFrom(string $keyId): ServiceProviderEntit public function createStepupEntityFrom(string $keyId): ServiceProviderEntityInterface { + $isConfigured = $this->featureConfiguration->hasFeature('eb.stepup.sfo.override_engine_entityid'); + $isEnabled = $this->featureConfiguration->isEnabled('eb.stepup.sfo.override_engine_entityid'); $entityId = $this->urlProvider->getUrl('metadata_stepup', false, null, null); + if ($isEnabled && $isConfigured) { + if (empty($this->entityIdOverrideValue)) { + throw new MissingParameterException( + 'When feature "feature_stepup_sfo_override_engine_entityid" is enabled, you must provide the '. + '"stepup.sfo.override_engine_entityid" parameter.' + ); + } + $entityId = $this->entityIdOverrideValue; + } + $entity = $this->buildServiceProviderOrmEntity($entityId); return new ServiceProviderStepup( // Add stepup data diff --git a/src/OpenConext/EngineBlock/Stepup/StepupEntityFactory.php b/src/OpenConext/EngineBlock/Stepup/StepupEntityFactory.php index 31493ddcd3..0b592c6682 100644 --- a/src/OpenConext/EngineBlock/Stepup/StepupEntityFactory.php +++ b/src/OpenConext/EngineBlock/Stepup/StepupEntityFactory.php @@ -18,7 +18,6 @@ namespace OpenConext\EngineBlock\Stepup; use EngineBlock_X509_CertificateFactory; -use OpenConext\EngineBlock\Metadata\EmptyMduiElement; use OpenConext\EngineBlock\Metadata\Entity\IdentityProvider; use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; use OpenConext\EngineBlock\Metadata\IndexedService; @@ -29,7 +28,6 @@ class StepupEntityFactory { - /** * @throws \EngineBlock_Exception */ diff --git a/src/OpenConext/EngineBlockBundle/Configuration/TestFeatureConfiguration.php b/src/OpenConext/EngineBlockBundle/Configuration/TestFeatureConfiguration.php index 4c1dace502..7d4167991c 100644 --- a/src/OpenConext/EngineBlockBundle/Configuration/TestFeatureConfiguration.php +++ b/src/OpenConext/EngineBlockBundle/Configuration/TestFeatureConfiguration.php @@ -45,6 +45,7 @@ public function __construct() $this->setFeature(new Feature('eb.enable_sso_notification', false)); $this->setFeature(new Feature('eb.feature_enable_consent', true)); $this->setFeature(new Feature('eb.enable_sso_session_cookie', true)); + $this->setFeature(new Feature('eb.stepup.sfo.override_engine_entityid', false)); } public function setFeature(Feature $feature): void diff --git a/src/OpenConext/EngineBlockBundle/Resources/config/services.yml b/src/OpenConext/EngineBlockBundle/Resources/config/services.yml index 9c75b6704e..c7bf6582d6 100644 --- a/src/OpenConext/EngineBlockBundle/Resources/config/services.yml +++ b/src/OpenConext/EngineBlockBundle/Resources/config/services.yml @@ -129,6 +129,8 @@ services: - '@OpenConext\EngineBlock\Metadata\X509\KeyPairFactory' - '@OpenConext\EngineBlock\Metadata\Factory\ValueObject\EngineBlockConfiguration' - '@engineblock.url_provider' + - '@engineblock.features' + - '%stepup.sfo.override_engine_entityid%' OpenConext\EngineBlock\Metadata\Factory\ValueObject\EngineBlockConfiguration: public: false diff --git a/src/OpenConext/EngineBlockFunctionalTestingBundle/Controllers/StepupMockController.php b/src/OpenConext/EngineBlockFunctionalTestingBundle/Controllers/StepupMockController.php index 664bd0e468..87b8a55dd4 100644 --- a/src/OpenConext/EngineBlockFunctionalTestingBundle/Controllers/StepupMockController.php +++ b/src/OpenConext/EngineBlockFunctionalTestingBundle/Controllers/StepupMockController.php @@ -96,6 +96,10 @@ private function getAvailableResponses(Request $request) $samlResponse = $this->mockStepupGateway->handleSsoSuccess($request, $this->getFullRequestUri($request)); $results['success'] = $this->getResponseData($request, $samlResponse); + // Parse successfull loa3 with changed audience + $samlResponse = $this->mockStepupGateway->handleSsoSuccess($request, $this->getFullRequestUri($request), true); + $results['success-audience'] = $this->getResponseData($request, $samlResponse); + // Parse successfull loa2 $samlResponse = $this->mockStepupGateway->handleSsoSuccessLoa2($request, $this->getFullRequestUri($request)); $results['loa2'] = $this->getResponseData($request, $samlResponse); diff --git a/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/Context/EngineBlockContext.php b/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/Context/EngineBlockContext.php index 7e388c5edf..ada66f7efc 100644 --- a/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/Context/EngineBlockContext.php +++ b/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/Context/EngineBlockContext.php @@ -18,14 +18,11 @@ namespace OpenConext\EngineBlockFunctionalTestingBundle\Features\Context; -use Behat\Behat\Tester\Exception\PendingException; use Behat\Gherkin\Node\TableNode; use Behat\Mink\Exception\ExpectationException; use DOMDocument; use DOMElement; use DOMXPath; -use Ingenerator\BehatTableAssert\AssertTable; -use Ingenerator\BehatTableAssert\TableParser\HTMLTable; use OpenConext\EngineBlockFunctionalTestingBundle\Fixtures\FunctionalTestingAttributeAggregationClient; use OpenConext\EngineBlockFunctionalTestingBundle\Fixtures\FunctionalTestingAuthenticationLoopGuard; use OpenConext\EngineBlockFunctionalTestingBundle\Fixtures\FunctionalTestingFeatureConfiguration; @@ -39,10 +36,6 @@ use RuntimeException; use SAML2\Constants; use SAML2\DOMDocumentFactory; -use function assertStringNotMatchesFormat; -use function assertStringStartsWith; -use function preg_match; -use function sprintf; /** * @SuppressWarnings(PHPMD.TooManyPublicMethods) Both set up and tasks can be a lot... diff --git a/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/Context/StepupContext.php b/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/Context/StepupContext.php index 0d1facf65d..560b24e0d8 100644 --- a/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/Context/StepupContext.php +++ b/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/Context/StepupContext.php @@ -71,6 +71,16 @@ public function stepupWillsSuccessfullyVerifyAUser() $mink->pressButton('Submit-success'); } + /** + * @Given /^Stepup will successfully verify a user with override entityID$/ + */ + public function stepupWillsSuccessfullyVerifyAUserAndUpdateAudience() + { + $mink = $this->getMinkContext(); + + $mink->pressButton('Submit-success-audience'); + } + /** * @Given /^Stepup will successfully verify a user with a LoA 2 token$/ */ diff --git a/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/StepupKeyRollover.feature b/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/StepupKeyRollover.feature new file mode 100644 index 0000000000..aeac7057ad --- /dev/null +++ b/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/StepupKeyRollover.feature @@ -0,0 +1,43 @@ +Feature: + In order to facilitate a rolling configuration update + As EngineBlock + I want the SP entityID that is used for Stepup auth to be configurable so that at the same time + that the EngineBlock default key is updated, this entityID can be changed. + This then allows two entities, with two different keys, to be configured in the Stepup-Gateway + + Background: + Given an EngineBlock instance on "vm.openconext.org" + And no registered SPs + And no registered Idps + And an Identity Provider named "SSO-IdP" + And a Service Provider named "SSO-SP" + And an Identity Provider named "Dummy-IdP" + And a Service Provider named "Dummy-SP" + And a Service Provider named "Proxy-SP" + + Scenario: When stepup.sfo.override_engine_entityid is not configured, stepup/metadata should show default EntityId + Given feature "eb.stepup.sfo.override_engine_entityid" is disabled + When I go to Engineblock URL "/authentication/stepup/metadata" + Then the response should match xpath '//md:EntityDescriptor[@entityID="https://engine.vm.openconext.org/authentication/stepup/metadata"]' + + Scenario: When stepup.sfo.override_engine_entityid is configured with a valid EntityId, stepup/metadata should show that EntityId + Given feature "eb.stepup.sfo.override_engine_entityid" is enabled + When I go to Engineblock URL "/authentication/stepup/metadata" + Then the response should match xpath '//md:EntityDescriptor[@entityID="https://engine.vm.openconext.com/new/stepup/metadata"]' + + # Note that we can not ascertain programatically if the Issuer is updated as this is an internal + # redirect response where we can not easily intervene with the browser (we would need to disable + # auto-following of redirects). This test does hit the code, and proves that the authentication + # is not broken by it. + Scenario: When stepup.sfo.override_engine_entityid is configured, the the Issuer is updated + Given feature "eb.stepup.sfo.override_engine_entityid" is enabled + And the SP "SSO-SP" requires Stepup LoA "http://vm.openconext.org/assurance/loa2" + When I log in at "SSO-SP" + And I select "SSO-IdP" on the WAYF + And I pass through EngineBlock + # This is where the Issuer is overridden. See: \EngineBlock_Corto_ProxyServer::sendStepupAuthenticationRequest + And I pass through the IdP + And Stepup will successfully verify a user with override entityID + And I give my consent + And I pass through EngineBlock + Then the url should match "/functional-testing/SSO-SP/acs" diff --git a/src/OpenConext/EngineBlockFunctionalTestingBundle/Fixtures/FunctionalTestingFeatureConfiguration.php b/src/OpenConext/EngineBlockFunctionalTestingBundle/Fixtures/FunctionalTestingFeatureConfiguration.php index abd796e584..819558f565 100644 --- a/src/OpenConext/EngineBlockFunctionalTestingBundle/Fixtures/FunctionalTestingFeatureConfiguration.php +++ b/src/OpenConext/EngineBlockFunctionalTestingBundle/Fixtures/FunctionalTestingFeatureConfiguration.php @@ -42,7 +42,7 @@ final class FunctionalTestingFeatureConfiguration implements FeatureConfiguratio */ private $dataStore; - public function __construct(TestFeatureConfiguration $featureConfiguration, AbstractDataStore $dataStore) + public function __construct(FeatureConfigurationInterface $featureConfiguration, AbstractDataStore $dataStore) { $this->featureConfiguration = $featureConfiguration; $this->dataStore = $dataStore; diff --git a/src/OpenConext/EngineBlockFunctionalTestingBundle/Mock/MockStepupGateway.php b/src/OpenConext/EngineBlockFunctionalTestingBundle/Mock/MockStepupGateway.php index 20e792bf2d..21ea574f6b 100644 --- a/src/OpenConext/EngineBlockFunctionalTestingBundle/Mock/MockStepupGateway.php +++ b/src/OpenConext/EngineBlockFunctionalTestingBundle/Mock/MockStepupGateway.php @@ -57,23 +57,31 @@ class MockStepupGateway */ private $gatewayConfiguration; + /** + * @var string + */ + private $sfoRolloverEntityId; + /** * @param FunctionalTestingStepupGatewayMockConfiguration $gatewayConfiguration * @throws \Exception */ public function __construct( - FunctionalTestingStepupGatewayMockConfiguration $gatewayConfiguration + FunctionalTestingStepupGatewayMockConfiguration $gatewayConfiguration, + $sfoRolloverEntityId ) { $this->gatewayConfiguration = $gatewayConfiguration; $this->currentTime = new DateTime(); + $this->sfoRolloverEntityId = $sfoRolloverEntityId; } /** * @param Request $request * @param string $fullRequestUri + * @param bool $updateAudience [default] false * @return Response */ - public function handleSsoSuccess(Request $request, $fullRequestUri) + public function handleSsoSuccess(Request $request, $fullRequestUri, $updateAudience = false) { // parse the authnRequest $authnRequest = $this->parseRequest($request, $fullRequestUri); @@ -89,7 +97,8 @@ public function handleSsoSuccess(Request $request, $fullRequestUri) $nameId, $destination, $authnContextClassRef, - $requestId + $requestId, + $updateAudience ); } @@ -144,16 +153,18 @@ public function handleSsoFailure(Request $request, $fullRequestUri, $status, $su * @param string $destination The ACS location * @param string|null $authnContextClassRef The loa level * @param string $requestId The requestId + * @param bool $updateAudience [default] false * @return Response */ - private function createSecondFactorOnlyResponse($nameId, $destination, $authnContextClassRef, $requestId) + private function createSecondFactorOnlyResponse($nameId, $destination, $authnContextClassRef, $requestId, $updateAudience = false) { return $this->createNewAuthnResponse( $this->createNewAssertion( $nameId, $authnContextClassRef, $destination, - $requestId + $requestId, + $updateAudience ), $destination, $requestId @@ -320,9 +331,10 @@ private function createNewAuthnResponse(Assertion $newAssertion, $destination, $ * @param string $authnContextClassRef * @param string $destination The ACS location * @param string $requestId The requestId + * @param bool $updateAudience [default] false * @return Assertion */ - private function createNewAssertion($nameId, $authnContextClassRef, $destination, $requestId) + private function createNewAssertion($nameId, $authnContextClassRef, $destination, $requestId, $updateAudience = false) { $newAssertion = new Assertion(); $newAssertion->setNotBefore($this->currentTime->getTimestamp()); @@ -337,7 +349,12 @@ private function createNewAssertion($nameId, $authnContextClassRef, $destination $newNameId->setValue($nameId); $newNameId->setFormat(Constants::NAMEID_UNSPECIFIED); $newAssertion->setNameId($newNameId); - $newAssertion->setValidAudiences([$this->gatewayConfiguration->getServiceProviderEntityId()]); + $audiences = [$this->gatewayConfiguration->getServiceProviderEntityId()]; + // If the entity id being updated, then set that new EntityId as the audience for this assertion + if ($updateAudience) { + $audiences = [$this->sfoRolloverEntityId]; + } + $newAssertion->setValidAudiences($audiences); $this->addAuthenticationStatementTo($newAssertion, $authnContextClassRef); return $newAssertion; diff --git a/src/OpenConext/EngineBlockFunctionalTestingBundle/Resources/config/mocks.yml b/src/OpenConext/EngineBlockFunctionalTestingBundle/Resources/config/mocks.yml index c91d74b426..288229bdd5 100644 --- a/src/OpenConext/EngineBlockFunctionalTestingBundle/Resources/config/mocks.yml +++ b/src/OpenConext/EngineBlockFunctionalTestingBundle/Resources/config/mocks.yml @@ -30,3 +30,4 @@ services: class: OpenConext\EngineBlockFunctionalTestingBundle\Mock\MockStepupGateway arguments: - "@engineblock.functional_testing.fixture.stepup_gateway_mock" + - '%stepup.sfo.override_engine_entityid%' diff --git a/src/OpenConext/EngineBlockFunctionalTestingBundle/Resources/config/services.yml b/src/OpenConext/EngineBlockFunctionalTestingBundle/Resources/config/services.yml index d8fadd3cce..b4031c05fe 100644 --- a/src/OpenConext/EngineBlockFunctionalTestingBundle/Resources/config/services.yml +++ b/src/OpenConext/EngineBlockFunctionalTestingBundle/Resources/config/services.yml @@ -1,12 +1,12 @@ parameters: - engineblock.functional_testing.service_registry_data_store.dir: "%kernel.root_dir%/../tmp/eb-fixtures/metadata-push/" - engineblock.functional_testing.service_registry_data_store.file: "%kernel.root_dir%/../tmp/eb-fixtures/metadata-push/entities" - engineblock.functional_testing.features_data_store.file: "%kernel.root_dir%/../tmp/eb-fixtures/features.json" - engineblock.functional_testing.authentication_loop_guard_data_store.file: "%kernel.root_dir%/../tmp/eb-fixtures/authentication-loop-guard.json" - engineblock.functional_testing.pdp_data_store.file: "%kernel.root_dir%/../tmp/eb-fixtures/policy_decision.json" - engineblock.functional_testing.attribute_aggregation_data_store.file: "%kernel.root_dir%/../tmp/eb-fixtures/attribute_aggregation.json" - engineblock.functional_testing.stepup_gateway_mock_data_store.file: "%kernel.root_dir%/../tmp/eb-fixtures/stepup_gateway_mock.json" - engineblock.functional_testing.translator_mock_data_store.file: "%kernel.root_dir%/../tmp/eb-fixtures/translator_mock.json" + engineblock.functional_testing.service_registry_data_store.dir: "/tmp/eb-fixtures/metadata-push/" + engineblock.functional_testing.service_registry_data_store.file: "/tmp/eb-fixtures/metadata-push/entities" + engineblock.functional_testing.features_data_store.file: "/tmp/eb-fixtures/features.json" + engineblock.functional_testing.authentication_loop_guard_data_store.file: "/tmp/eb-fixtures/authentication-loop-guard.json" + engineblock.functional_testing.pdp_data_store.file: "/tmp/eb-fixtures/policy_decision.json" + engineblock.functional_testing.attribute_aggregation_data_store.file: "/tmp/eb-fixtures/attribute_aggregation.json" + engineblock.functional_testing.stepup_gateway_mock_data_store.file: "/tmp/eb-fixtures/stepup_gateway_mock.json" + engineblock.functional_testing.translator_mock_data_store.file: "/tmp/eb-fixtures/translator_mock.json" services: engineblock.functional_testing.service.engine_block: @@ -109,3 +109,13 @@ services: class: OpenConext\EngineBlockBundle\Configuration\ErrorFeedbackConfiguration arguments: - "@engineblock.functional_testing.mock.translator" + + engineblock.factory.service_provider_factory: + class: OpenConext\EngineBlock\Metadata\Factory\Factory\ServiceProviderFactory + arguments: + - '@engineblock.compat.metadata.definitions' + - '@OpenConext\EngineBlock\Metadata\X509\KeyPairFactory' + - '@OpenConext\EngineBlock\Metadata\Factory\ValueObject\EngineBlockConfiguration' + - '@engineblock.url_provider' + - '@engineblock.functional_testing.fixture.features' + - '%stepup.sfo.override_engine_entityid%' diff --git a/tests/behat.yml b/tests/behat.yml index 273730cb68..0791a01f37 100644 --- a/tests/behat.yml +++ b/tests/behat.yml @@ -87,4 +87,3 @@ default: - "--no-sandbox" - "--disable-dev-shm-usage" Behat\Symfony2Extension: ~ - diff --git a/tests/unit/OpenConext/EngineBlock/Metadata/Factory/Factory/ServiceProviderFactoryTest.php b/tests/unit/OpenConext/EngineBlock/Metadata/Factory/Factory/ServiceProviderFactoryTest.php index 4ed5dd357e..b320f9dfc7 100644 --- a/tests/unit/OpenConext/EngineBlock/Metadata/Factory/Factory/ServiceProviderFactoryTest.php +++ b/tests/unit/OpenConext/EngineBlock/Metadata/Factory/Factory/ServiceProviderFactoryTest.php @@ -18,6 +18,8 @@ namespace OpenConext\EngineBlock\Metadata\Factory\Factory; use EngineBlock_Attributes_Metadata as AttributesMetadata; +use Mockery\Mock; +use OpenConext\EngineBlock\Exception\MissingParameterException; use OpenConext\EngineBlock\Metadata\Coins; use OpenConext\EngineBlock\Metadata\ContactPerson; use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; @@ -32,7 +34,9 @@ use OpenConext\EngineBlock\Metadata\X509\KeyPairFactory; use OpenConext\EngineBlock\Metadata\X509\X509Certificate; use OpenConext\EngineBlock\Metadata\X509\X509KeyPair; +use OpenConext\EngineBlockBundle\Configuration\TestFeatureConfiguration; use OpenConext\EngineBlockBundle\Url\UrlProvider; +use PHPUnit\Framework\MockObject\MockObject; use RobRichards\XMLSecLibs\XMLSecurityKey; use SAML2\Constants; use Symfony\Component\Translation\TranslatorInterface; @@ -44,44 +48,56 @@ class ServiceProviderFactoryTest extends AbstractEntityTest */ private $factory; /** - * @var \PHPUnit\Framework\MockObject\MockObject + * @var MockObject */ private $attributes; /** - * @var \PHPUnit\Framework\MockObject\MockObject + * @var MockObject */ private $keyPairFactory; /** - * @var \PHPUnit\Framework\MockObject\MockObject + * @var MockObject */ private $configuration; /** - * @var \PHPUnit\Framework\MockObject\MockObject + * @var MockObject */ private $urlProvider; /** - * @var \PHPUnit\Framework\MockObject\MockObject + * @var MockObject */ private $translator; + /** @var TestFeatureConfiguration&MockObject */ + private $featureConfiguration; + /** @var string */ + private $entityIdOverride; + public function setUp(): void { $this->attributes = $this->createMock(AttributesMetadata::class); $this->keyPairFactory = $this->createMock(KeyPairFactory::class); $this->configuration = $this->createMock(EngineBlockConfiguration::class); $this->urlProvider = $this->createMock(UrlProvider::class); - - $this->factory = new ServiceProviderFactory($this->attributes, $this->keyPairFactory, $this->configuration, $this->urlProvider); + $this->featureConfiguration = $this->createMock(TestFeatureConfiguration::class); + $this->entityIdOverride = 'https://foobar.openconext.org'; + + $this->factory = new ServiceProviderFactory( + $this->attributes, + $this->keyPairFactory, + $this->configuration, + $this->urlProvider, + $this->featureConfiguration, + $this->entityIdOverride + ); $this->translator = $this->createMock(TranslatorInterface::class); } public function test_create_engineblock_entity_from() { - $entity = new ServiceProvider('entityId'); $entity = $this->factory->createEngineBlockEntityFrom( - 'entityID', - 'default' + 'entityID' ); $this->assertInstanceOf(ServiceProviderEntityInterface::class, $entity); @@ -89,10 +105,8 @@ public function test_create_engineblock_entity_from() public function test_create_stepup_entity_from() { - $entity = new ServiceProvider('entityId'); $entity = $this->factory->createStepupEntityFrom( - 'entityID', - 'default' + 'entityID' ); $this->assertInstanceOf(ServiceProviderEntityInterface::class, $entity); @@ -149,19 +163,25 @@ public function test_eb_properties() $this->urlProvider->expects($this->exactly(2)) ->method('getUrl') ->withConsecutive( - // EntityId: EngineBlockServiceProvider::getEntityId + // EntityId: EngineBlockServiceProvider::getEntityId ['metadata_sp', false, null, null], // ACS: EngineBlockServiceProvider::getAssertionConsumerService ['authentication_sp_consume_assertion', false, null, null] - ) ->willReturnOnConsecutiveCalls( - // EntityId + )->willReturnOnConsecutiveCalls( + // EntityId 'EbEntityId', // ACS 'proxiedAcsLocation' ); - $this->factory = new ServiceProviderFactory($this->attributes, $this->keyPairFactory, $this->configuration, $this->urlProvider); - + $this->factory = new ServiceProviderFactory( + $this->attributes, + $this->keyPairFactory, + $this->configuration, + $this->urlProvider, + $this->featureConfiguration, + $this->entityIdOverride + ); $adapter = $this->createServiceProviderAdapter(); $decorator = $this->factory->createEngineBlockEntityFrom('initial-key-id'); @@ -315,18 +335,25 @@ public function test_stepup_properties() $this->urlProvider->expects($this->exactly(2)) ->method('getUrl') ->withConsecutive( - // EntityId + // EntityId ['metadata_stepup', false, null, null], // ACS: ServiceProvider::getAssertionConsumerService ['authentication_stepup_consume_assertion', false, null, null] - ) ->willReturnOnConsecutiveCalls( - // EntityId + )->willReturnOnConsecutiveCalls( + // EntityId 'StepupEntityId', // ACS 'proxiedAcsLocation' ); - $this->factory = new ServiceProviderFactory($this->attributes, $this->keyPairFactory, $this->configuration, $this->urlProvider); + $this->factory = new ServiceProviderFactory( + $this->attributes, + $this->keyPairFactory, + $this->configuration, + $this->urlProvider, + $this->featureConfiguration, + $this->entityIdOverride + ); $adapter = $this->createServiceProviderAdapter(); $decorator = $this->factory->createStepupEntityFrom('initial-key-id'); @@ -401,4 +428,80 @@ public function test_stepup_properties() $this->assertInstanceOf(ServiceProviderEntityInterface::class, $decorator); } + + public function test_stepup_entity_id_can_be_overridden() + { + $this->featureConfiguration + ->expects($this->once()) + ->method('hasFeature') + ->with('eb.stepup.sfo.override_engine_entityid') + ->willReturn(true); + + $this->featureConfiguration + ->expects($this->once()) + ->method('isEnabled') + ->with('eb.stepup.sfo.override_engine_entityid') + ->willReturn(true); + + $entity = $this->factory->createStepupEntityFrom( + 'entityID' + ); + + $this->assertEquals($this->entityIdOverride, $entity->getEntityId()); + } + + public function test_stepup_entity_id_override_feature_flag_must_be_enabled() + { + $this->featureConfiguration + ->expects($this->once()) + ->method('hasFeature') + ->with('eb.stepup.sfo.override_engine_entityid') + ->willReturn(true); + + $this->featureConfiguration + ->expects($this->once()) + ->method('isEnabled') + ->with('eb.stepup.sfo.override_engine_entityid') + ->willReturn(false); + + $this->urlProvider + ->expects($this->once()) + ->method('getUrl') + ->willReturn('https://normal-entity-id.example.org'); + + $entity = $this->factory->createStepupEntityFrom( + 'entityID' + ); + + $this->assertEquals('https://normal-entity-id.example.org', $entity->getEntityId()); + } + + public function test_stepup_entity_id_when_flag_enabled_override_must_be_set() + { + $this->factory = new ServiceProviderFactory( + $this->attributes, + $this->keyPairFactory, + $this->configuration, + $this->urlProvider, + $this->featureConfiguration, + '' + ); + $this->featureConfiguration + ->expects($this->once()) + ->method('hasFeature') + ->with('eb.stepup.sfo.override_engine_entityid') + ->willReturn(true); + + $this->featureConfiguration + ->expects($this->once()) + ->method('isEnabled') + ->with('eb.stepup.sfo.override_engine_entityid') + ->willReturn(true); + + $this->expectException(MissingParameterException::class); + $this->expectExceptionMessage('When feature "feature_stepup_sfo_override_engine_entityid" is enabled, you must provide the "stepup.sfo.override_engine_entityid" parameter.'); + $this->factory->createStepupEntityFrom( + 'entityID' + ); + } }