Ensure the NameID is correctly set

Due to the NameId juggling taking place during the Input filtering. There MUST be a point in time when we set the correct outgoing NameId value and format. And that is done in the AddIdentityAttributes filter. During the optimization I moved the 'were done here' return statement to a block that seemed to make more sense to me. But that caused the NameId from not being set even though it was resolved. So I moved that block back to the position where it is supposed to be. It is meant as a guard from injustly overwriting the eduPersonTargettedId. Not for preventing the setting of the correct NameId in the subject.
OpenConext · Sep 4, 2024 · 4828d21 · 4828d21
1 parent 8e9116b
commit 4828d21
Show file tree

Hide file tree

Showing 2 changed files with 75 additions and 5 deletions.
diff --git a/library/EngineBlock/Corto/Filter/Command/AddIdentityAttributes.php b/library/EngineBlock/Corto/Filter/Command/AddIdentityAttributes.php
@@ -16,6 +16,7 @@
  * limitations under the License.
  */
 
+use OpenConext\EngineBlock\Metadata\AttributeReleasePolicy;
 use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider;
 use Psr\Log\LoggerInterface;
 use SAML2\Constants;
@@ -84,11 +85,6 @@ public function execute()
                 $isResolved = true;
                 $this->_response->getAssertion()->setNameId($nameId);
             }
-
-            // If there's an ARP, but it does not contain the EPTI, we're done now.
-            if (!$arp->hasAttribute(Constants::EPTI_URN_MACE)) {
-                return;
-            }
         }
 
         if (!$isResolved || !isset($nameId)){
@@ -105,6 +101,11 @@ public function execute()
             $this->_response->getAssertion()->setNameId($nameId);
         }
 
+        // If there's an ARP, but it does not contain the EPTI, we're done now.
+        if ($arp instanceof AttributeReleasePolicy && !$arp->hasAttribute(Constants::EPTI_URN_MACE)) {
+            return;
+        }
+
         // We arrive here if either:
         // 1) the ARP is NULL, this means no ARP = let everything through including EPTI; or
         // 2) the ARP is not null and does contain the EPTI attribute.

diff --git a/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/NameIdFormat.feature b/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/NameIdFormat.feature
@@ -0,0 +1,69 @@
+Feature:
+  To ensure no confusion about the NameID Format
+  As EngineBlock
+  I want to be sure after ARP my name id format is presented correctly to the SP
+
+  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"
+
+  Scenario: EngineBlock should not update the Unspecified NameIdFormat when no ARP filters are applied
+    Given SP "SSO-SP" uses the Unspecified NameID format
+    When I log in at "SSO-SP"
+    And I pass through EngineBlock
+    And I pass through the IdP
+    When I give my consent
+    And I pass through EngineBlock
+    And the response should match xpath '/samlp:Response/saml:Assertion/saml:Subject/saml:NameID[@Format="urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified"]'
+
+  Scenario: EngineBlock should not update the Unspecified NameIdFormat when the ARP is applied
+    Given SP "SSO-SP" uses the Unspecified NameID format
+    And SP "SSO-SP" allows an attribute named "urn:mace:dir:attribute-def:uid"
+    When I log in at "SSO-SP"
+    And I pass through EngineBlock
+    And I pass through the IdP
+    When I give my consent
+    And I pass through EngineBlock
+    And the response should match xpath '/samlp:Response/saml:Assertion/saml:Subject/saml:NameID[@Format="urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified"]'
+
+  Scenario: EngineBlock should not update the Persistent NameIdFormat when no ARP filters are applied
+    Given SP "SSO-SP" uses the Persistent NameID format
+    When I log in at "SSO-SP"
+    And I pass through EngineBlock
+    And I pass through the IdP
+    When I give my consent
+    And I pass through EngineBlock
+    And the response should match xpath '/samlp:Response/saml:Assertion/saml:Subject/saml:NameID[@Format="urn:oasis:names:tc:SAML:2.0:nameid-format:persistent"]'
+
+  Scenario: EngineBlock should not update the Persistent NameIdFormat when the ARP is applied
+    Given SP "SSO-SP" uses the Persistent NameID format
+    And SP "SSO-SP" allows an attribute named "urn:mace:dir:attribute-def:uid"
+    And SP "SSO-SP" allows an attribute named "urn:mace:terena.org:attribute-def:schacHomeOrganization"
+    When I log in at "SSO-SP"
+    And I pass through EngineBlock
+    And I pass through the IdP
+    When I give my consent
+    And I pass through EngineBlock
+    And the response should match xpath '/samlp:Response/saml:Assertion/saml:Subject/saml:NameID[@Format="urn:oasis:names:tc:SAML:2.0:nameid-format:persistent"]'
+
+  Scenario: EngineBlock should not update the Transient NameIdFormat when no ARP filters are applied
+    Given SP "SSO-SP" uses the Transient NameID format
+    When I log in at "SSO-SP"
+    And I pass through EngineBlock
+    And I pass through the IdP
+    When I give my consent
+    And I pass through EngineBlock
+    And the response should match xpath '/samlp:Response/saml:Assertion/saml:Subject/saml:NameID[@Format="urn:oasis:names:tc:SAML:2.0:nameid-format:transient"]'
+
+  Scenario: EngineBlock should not update the Transient NameIdFormat when the ARP is applied
+    Given SP "SSO-SP" uses the Transient NameID format
+    And SP "SSO-SP" allows an attribute named "urn:mace:terena.org:attribute-def:schacHomeOrganization"
+    When I log in at "SSO-SP"
+    And I pass through EngineBlock
+    And I pass through the IdP
+    When I give my consent
+    And I pass through EngineBlock
+    And the response should match xpath '/samlp:Response/saml:Assertion/saml:Subject/saml:NameID[@Format="urn:oasis:names:tc:SAML:2.0:nameid-format:transient"]'