From 3e67fa0604fc7b1131fb1bd2ee4b98f020e5392d Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Wed, 20 May 2020 14:07:09 +0200 Subject: [PATCH 1/4] Interpret missing exclude from push correctly When resetting a client secret on production and the exclude from push attribute is missing, this indicates we are dealing with a published attribute. As the service desk not unchecks the coin checkbox, but simply removes it. This additional logic covers that (understandable) 'misbehaviour' of that coin. https://www.pivotaltracker.com/story/show/171313433/comments/214527925 --- .../Application/Dto/MetadataConversionDto.php | 5 +++++ .../Application/Metadata/OidcngJsonGenerator.php | 13 ++++++++++--- .../Metadata/OidcngResourceServerJsonGenerator.php | 13 ++++++++++--- .../Infrastructure/Manage/Dto/ManageEntity.php | 10 +++++++++- 4 files changed, 34 insertions(+), 7 deletions(-) diff --git a/src/Surfnet/ServiceProviderDashboard/Application/Dto/MetadataConversionDto.php b/src/Surfnet/ServiceProviderDashboard/Application/Dto/MetadataConversionDto.php index 462e47adc..61f9defec 100644 --- a/src/Surfnet/ServiceProviderDashboard/Application/Dto/MetadataConversionDto.php +++ b/src/Surfnet/ServiceProviderDashboard/Application/Dto/MetadataConversionDto.php @@ -562,6 +562,11 @@ public function isExcludedFromPush() return $this->manageEntity->isExcludedFromPush(); } + public function isExcludedFromPushSet() + { + return $this->manageEntity->isExcludedFromPushSet(); + } + /** * @return array */ diff --git a/src/Surfnet/ServiceProviderDashboard/Application/Metadata/OidcngJsonGenerator.php b/src/Surfnet/ServiceProviderDashboard/Application/Metadata/OidcngJsonGenerator.php index 05582ae46..187db2fbb 100644 --- a/src/Surfnet/ServiceProviderDashboard/Application/Metadata/OidcngJsonGenerator.php +++ b/src/Surfnet/ServiceProviderDashboard/Application/Metadata/OidcngJsonGenerator.php @@ -225,18 +225,25 @@ private function generateMetadataFields(MetadataConversionDto $entity) $metadata['scopes'] = ['openid']; } - // When publishing to production, the coin:exclude_from_push must be present and set to '1'. This prevents the - // entity from being pushed to engineblock. + // Scenario 1: When publishing to production, the coin:exclude_from_push must be present and set to '1'. + // This prevents the entity from being pushed to EngineBlock. if ($entity->isProduction()) { $metadata['coin:exclude_from_push'] = '1'; } - // When dealing with a client secret reset, keep the current exclude from push state. + // Scenario 2: When dealing with a client secret reset, keep the current exclude from push state. $secret = $entity->getClientSecret(); if ($secret && $entity->isManageEntity() && !$entity->isExcludedFromPush()) { $metadata['coin:exclude_from_push'] = '0'; } + // Scenario 3: We are resetting the client secret, the service desk removed the exclude from push coin + // attribute. This also indicates the entity is published. But now we do not want to reset the coin to '0', we + // simply unset it. + if ($secret && $entity->isManageEntity() && !$entity->isExcludedFromPushSet()) { + unset($metadata['coin:exclude_from_push']); + } + $metadata += $this->generateOidcClient($entity); if (!empty($entity->getLogoUrl())) { diff --git a/src/Surfnet/ServiceProviderDashboard/Application/Metadata/OidcngResourceServerJsonGenerator.php b/src/Surfnet/ServiceProviderDashboard/Application/Metadata/OidcngResourceServerJsonGenerator.php index fd52120d8..0e4318e15 100644 --- a/src/Surfnet/ServiceProviderDashboard/Application/Metadata/OidcngResourceServerJsonGenerator.php +++ b/src/Surfnet/ServiceProviderDashboard/Application/Metadata/OidcngResourceServerJsonGenerator.php @@ -188,18 +188,25 @@ private function generateMetadataFields(MetadataConversionDto $entity) // Will become configurable some time in the future. $metadata['scopes'] = ['openid']; - // When publishing to production, the coin:exclude_from_push must be present and set to '1'. This prevents the - // entity from being pushed to engineblock. + // Scenario 1: When publishing to production, the coin:exclude_from_push must be present and set to '1'. + // This prevents the entity from being pushed to EngineBlock. if ($entity->isProduction()) { $metadata['coin:exclude_from_push'] = '1'; } - // When dealing with a client secret reset, keep the current exclude from push state. + // Scenario 2: When dealing with a client secret reset, keep the current exclude from push state. $secret = $entity->getClientSecret(); if ($secret && $entity->isManageEntity() && !$entity->isExcludedFromPush()) { $metadata['coin:exclude_from_push'] = '0'; } + // Scenario 3: We are resetting the client secret, the service desk removed the exclude from push coin + // attribute. This also indicates the entity is published. But now we do not want to reset the coin to '0', we + // simply unset it. + if ($secret && $entity->isManageEntity() && !$entity->isExcludedFromPushSet()) { + unset($metadata['coin:exclude_from_push']); + } + $metadata += $this->generateOidcClient($entity); return $metadata; diff --git a/src/Surfnet/ServiceProviderDashboard/Infrastructure/Manage/Dto/ManageEntity.php b/src/Surfnet/ServiceProviderDashboard/Infrastructure/Manage/Dto/ManageEntity.php index 20417af4d..2128ab6f4 100644 --- a/src/Surfnet/ServiceProviderDashboard/Infrastructure/Manage/Dto/ManageEntity.php +++ b/src/Surfnet/ServiceProviderDashboard/Infrastructure/Manage/Dto/ManageEntity.php @@ -167,10 +167,18 @@ public function isOidcngResourceServer() return false; } + public function isExcludedFromPushSet() + { + if (is_null($this->getMetaData()->getCoin()->getExcludeFromPush())) { + return false; + } + return true; + } + public function isExcludedFromPush() { if (is_null($this->getMetaData()->getCoin()->getExcludeFromPush())) { - return true; + return false; } return $this->getMetaData()->getCoin()->getExcludeFromPush() == 1 ? true : false; } From 6cdae00dd51ed0904796020f5838fecbba100b78 Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Wed, 20 May 2020 14:21:06 +0200 Subject: [PATCH 2/4] Allow builds on feature, bugfix & release branches --- .travis.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index bc21567d5..c62b4425a 100644 --- a/.travis.yml +++ b/.travis.yml @@ -36,4 +36,6 @@ branches: only: - develop - master - - feature/php72-support \ No newline at end of file + - /^feature\/(.*)$/ + - /^bugfix\/(.*)$/ + - /^release\/(.*)$/ \ No newline at end of file From e5c8159ac60d907fe5ad16887e53fc26713489f1 Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Wed, 20 May 2020 15:10:21 +0200 Subject: [PATCH 3/4] Decrease cyclomatic complexity by moving logic This is a temporary solution. The logic should be moved to a dedicated class. This should include reviewing the other generators. Maybe they should merely generate JSON and not actually convert so much. https://www.pivotaltracker.com/story/show/172936842 --- .../Metadata/OidcngJsonGenerator.php | 41 +++++++++++-------- .../OidcngResourceServerJsonGenerator.php | 41 +++++++++++-------- 2 files changed, 46 insertions(+), 36 deletions(-) diff --git a/src/Surfnet/ServiceProviderDashboard/Application/Metadata/OidcngJsonGenerator.php b/src/Surfnet/ServiceProviderDashboard/Application/Metadata/OidcngJsonGenerator.php index 187db2fbb..f971599c0 100644 --- a/src/Surfnet/ServiceProviderDashboard/Application/Metadata/OidcngJsonGenerator.php +++ b/src/Surfnet/ServiceProviderDashboard/Application/Metadata/OidcngJsonGenerator.php @@ -225,24 +225,7 @@ private function generateMetadataFields(MetadataConversionDto $entity) $metadata['scopes'] = ['openid']; } - // Scenario 1: When publishing to production, the coin:exclude_from_push must be present and set to '1'. - // This prevents the entity from being pushed to EngineBlock. - if ($entity->isProduction()) { - $metadata['coin:exclude_from_push'] = '1'; - } - - // Scenario 2: When dealing with a client secret reset, keep the current exclude from push state. - $secret = $entity->getClientSecret(); - if ($secret && $entity->isManageEntity() && !$entity->isExcludedFromPush()) { - $metadata['coin:exclude_from_push'] = '0'; - } - - // Scenario 3: We are resetting the client secret, the service desk removed the exclude from push coin - // attribute. This also indicates the entity is published. But now we do not want to reset the coin to '0', we - // simply unset it. - if ($secret && $entity->isManageEntity() && !$entity->isExcludedFromPushSet()) { - unset($metadata['coin:exclude_from_push']); - } + $this->setExcludeFromPush($metadata, $entity); $metadata += $this->generateOidcClient($entity); @@ -433,4 +416,26 @@ private function generateAllowedResourceServers(MetadataConversionDto $entity) 'allowedResourceServers' => $allowedResourceServers, ]; } + + private function setExcludeFromPush(&$metadata, MetadataConversionDto $entity) + { + // Scenario 1: When publishing to production, the coin:exclude_from_push must be present and set to '1'. + // This prevents the entity from being pushed to EngineBlock. + if ($entity->isProduction()) { + $metadata['coin:exclude_from_push'] = '1'; + } + + // Scenario 2: When dealing with a client secret reset, keep the current exclude from push state. + $secret = $entity->getClientSecret(); + if ($secret && $entity->isManageEntity() && !$entity->isExcludedFromPush()) { + $metadata['coin:exclude_from_push'] = '0'; + } + + // Scenario 3: We are resetting the client secret, the service desk removed the exclude from push coin + // attribute. This also indicates the entity is published. But now we do not want to reset the coin to '0', we + // simply unset it. + if ($secret && $entity->isManageEntity() && !$entity->isExcludedFromPushSet()) { + unset($metadata['coin:exclude_from_push']); + } + } } diff --git a/src/Surfnet/ServiceProviderDashboard/Application/Metadata/OidcngResourceServerJsonGenerator.php b/src/Surfnet/ServiceProviderDashboard/Application/Metadata/OidcngResourceServerJsonGenerator.php index 0e4318e15..654b4c925 100644 --- a/src/Surfnet/ServiceProviderDashboard/Application/Metadata/OidcngResourceServerJsonGenerator.php +++ b/src/Surfnet/ServiceProviderDashboard/Application/Metadata/OidcngResourceServerJsonGenerator.php @@ -188,24 +188,7 @@ private function generateMetadataFields(MetadataConversionDto $entity) // Will become configurable some time in the future. $metadata['scopes'] = ['openid']; - // Scenario 1: When publishing to production, the coin:exclude_from_push must be present and set to '1'. - // This prevents the entity from being pushed to EngineBlock. - if ($entity->isProduction()) { - $metadata['coin:exclude_from_push'] = '1'; - } - - // Scenario 2: When dealing with a client secret reset, keep the current exclude from push state. - $secret = $entity->getClientSecret(); - if ($secret && $entity->isManageEntity() && !$entity->isExcludedFromPush()) { - $metadata['coin:exclude_from_push'] = '0'; - } - - // Scenario 3: We are resetting the client secret, the service desk removed the exclude from push coin - // attribute. This also indicates the entity is published. But now we do not want to reset the coin to '0', we - // simply unset it. - if ($secret && $entity->isManageEntity() && !$entity->isExcludedFromPushSet()) { - unset($metadata['coin:exclude_from_push']); - } + $this->setExcludeFromPush($metadata, $entity); $metadata += $this->generateOidcClient($entity); @@ -332,4 +315,26 @@ private function generateAclData(MetadataConversionDto $entity) 'allowedall' => false, ]; } + + private function setExcludeFromPush(&$metadata, MetadataConversionDto $entity) + { + // Scenario 1: When publishing to production, the coin:exclude_from_push must be present and set to '1'. + // This prevents the entity from being pushed to EngineBlock. + if ($entity->isProduction()) { + $metadata['coin:exclude_from_push'] = '1'; + } + + // Scenario 2: When dealing with a client secret reset, keep the current exclude from push state. + $secret = $entity->getClientSecret(); + if ($secret && $entity->isManageEntity() && !$entity->isExcludedFromPush()) { + $metadata['coin:exclude_from_push'] = '0'; + } + + // Scenario 3: We are resetting the client secret, the service desk removed the exclude from push coin + // attribute. This also indicates the entity is published. But now we do not want to reset the coin to '0', we + // simply unset it. + if ($secret && $entity->isManageEntity() && !$entity->isExcludedFromPushSet()) { + unset($metadata['coin:exclude_from_push']); + } + } } From bd2a92a4d7b7e9ec8ab3a8e39377592d57734392 Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Wed, 20 May 2020 15:32:51 +0200 Subject: [PATCH 4/4] Update CHANGELOG.md --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index aa1e60dd0..6051cac4f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +## 2.5.2 +**Bugfix** +* Interpret missing exclude from push correctly #348 + ## 2.5.1 **Bugfix** * Only preserve the exclude-from-push flag on client secret reset #342