Reject metadata push when IdP discovery entry is missing English name (#1957)

* Reject metadata push when IdP discovery entry is missing English name

A discovery entry without name_en was silently dropped, which could
leave an IdP with no discoveries and cause a WAYF crash. Now throws
InvalidDiscoveryException, which propagates to ConnectionsController
as HTTP 400. Fixes #1865.

* Include entity ID and entry index in InvalidDiscoveryException message

Author: kayjoosten

src/OpenConext/EngineBlock/Metadata/Entity/Assembler/DiscoveryAssembler.php

@@ -18,6 +18,7 @@
 
 namespace OpenConext\EngineBlock\Metadata\Entity\Assembler;
 
+use OpenConext\EngineBlock\Exception\InvalidDiscoveryException;
 use OpenConext\EngineBlock\Metadata\Discovery;
 use OpenConext\EngineBlock\Metadata\Logo;
 use OpenConext\EngineBlockBundle\Localization\LanguageSupportProvider;
@@ -41,15 +42,24 @@ public function assembleDiscoveries(stdClass $connection): array
             return [];
         }
 
+        $entityId = $connection->name ?? 'unknown';
         $discoveries = [];
-        foreach ($connection->metadata->discoveries as $discovery) {
+        foreach ($connection->metadata->discoveries as $index => $discovery) {
             $names = $this->extractLocalizedFields($discovery, 'name');
             $keywords = $this->extractLocalizedFields($discovery, 'keywords');
             $logo = $this->assembleLogo($discovery);
 
-            if (isset($names['en'])) {
-                $discoveries[] = Discovery::create($names, $keywords, $logo);
+            if (!isset($names['en'])) {
+                throw new InvalidDiscoveryException(
+                    sprintf(
+                        'Discovery entry #%d for entity "%s" is missing a required English name (name_en). ' .
+                        'Refusing to process metadata push.',
+                        $index,
+                        $entityId
+                    )
+                );
             }
+            $discoveries[] = Discovery::create($names, $keywords, $logo);
         }
 
         return empty($discoveries) ? [] : ['discoveries' => $discoveries];

tests/functional/OpenConext/EngineBlockBundle/Controller/Api/ConnectionsControllerTest.php

@@ -396,6 +396,39 @@ public function pushing_entity_with_valid_manipulation_code_succeeds()
         $this->assertStatusCode(Response::HTTP_OK, $client);
     }
 
+    #[Group('Api')]
+    #[Group('Connections')]
+    #[Group('MetadataPush')]
+    #[Test]
+    public function pushing_idp_with_discovery_missing_english_name_returns_bad_request(): void
+    {
+        $client = $this->createAuthorizedClient();
+
+        $payload = '{"connections":{"idp1":{
+            "allow_all_entities":true,
+            "allowed_connections":[],
+            "metadata":{
+                "name":{"en":"Test IdP","nl":"Test IdP nl"},
+                "discoveries":[{"name_nl":"Ontbrekend Engels"}],
+                "SingleSignOnService":[{
+                    "Binding":"urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST",
+                    "Location":"https://idp.example.com/sso"
+                }]
+            },
+            "name":"https://idp.example.com",
+            "state":"prodaccepted",
+            "type":"saml20-idp"
+        }}}';
+
+        $client->request('POST', 'https://engine-api.dev.openconext.local/api/connections', [], [], [], $payload);
+
+        $this->assertStatusCode(Response::HTTP_BAD_REQUEST, $client);
+        $this->assertJson($client->getResponse()->getContent());
+        $isContentTypeJson = $client->getResponse()->headers->contains('Content-Type', 'application/json');
+        $this->assertTrue($isContentTypeJson, 'Response should have Content-Type: application/json header');
+        $this->assertStringContainsString('name_en', $client->getResponse()->getContent());
+    }
+
     public static function invalidHttpMethodProvider()
     {
         return [

tests/integration/OpenConext/EngineBlock/Metadata/Entity/Assembler/DiscoveryAssemblerTest.php

@@ -18,6 +18,7 @@
 
 namespace OpenConext\EngineBlock\Tests;
 
+use OpenConext\EngineBlock\Exception\InvalidDiscoveryException;
 use OpenConext\EngineBlock\Metadata\Discovery;
 use OpenConext\EngineBlock\Metadata\Entity\Assembler\DiscoveryAssembler;
 use OpenConext\EngineBlock\Metadata\Logo;
@@ -84,8 +85,11 @@ public function testAssembleDiscoveriesProcessesValidDiscovery(): void
         $this->assertEquals(100, $discovery->getLogo()->height);
     }
 
-    public function testAssembleDiscoveriesSkipsDiscoveryWithoutEnglishName(): void
+    public function testAssembleDiscoveriesThrowsWhenDiscoveryMissingEnglishName(): void
     {
+        $this->expectException(InvalidDiscoveryException::class);
+        $this->expectExceptionMessage('missing a required English name');
+
         $connection = new stdClass();
         $connection->metadata = new stdClass();
         $connection->metadata->discoveries = [
@@ -95,9 +99,7 @@ public function testAssembleDiscoveriesSkipsDiscoveryWithoutEnglishName(): void
             ])
         ];
 
-        $result = $this->discoveryAssembler->assembleDiscoveries($connection);
-
-        $this->assertEquals([], $result);
+        $this->discoveryAssembler->assembleDiscoveries($connection);
     }
 
     public function testAssembleDiscoveriesWithTrimmedValues(): void

tests/integration/OpenConext/EngineBlock/Metadata/Entity/Assembler/PushMetadataAssemblerTest.php

@@ -20,6 +20,7 @@
 
 use Mockery as m;
 use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration;
+use OpenConext\EngineBlock\Exception\InvalidDiscoveryException;
 use OpenConext\EngineBlock\Metadata\AttributeReleasePolicy;
 use OpenConext\EngineBlock\Metadata\Discovery;
 use OpenConext\EngineBlock\Metadata\Entity\Assembler\PushMetadataAssembler;
@@ -569,6 +570,15 @@ public function test_it_assembles_metadata_without_discoveries_details()
         $this->assertSame('https://engine.dev.openconext.local/images/logo.png?v=dev', $idp->logo->url);
     }
 
+    public function test_it_rejects_idp_with_discovery_missing_english_name(): void
+    {
+        $this->expectException(InvalidDiscoveryException::class);
+        $this->expectExceptionMessage('missing a required English name');
+
+        $input = $this->readFixture('metadata_idp_discovery_missing_english.json');
+        $this->assembler->assemble($input);
+    }
+
     private function readFixture(string $file): object
     {
         return json_decode(file_get_contents(__DIR__ . '/fixtures/'. basename($file)), false);

tests/integration/OpenConext/EngineBlock/Metadata/Entity/Assembler/fixtures/metadata_idp_discovery_missing_english.json

@@ -0,0 +1,24 @@
+{
+    "2d96e27a-76cf-4ca2-ac70-ece5d4c49525": {
+        "name": "https:\/\/serviceregistry.dev.openconext.local\/simplesaml\/module.php\/saml\/sp\/metadata.php\/default-idp",
+        "state": "prodaccepted",
+        "type": "saml20-idp",
+        "metadata": {
+            "discoveries": [
+                {
+                    "name_nl": "Ontbrekend Engels"
+                }
+            ],
+            "SingleSignOnService": [
+                {
+                    "Binding": "urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST",
+                    "Location": "https:\/\/mujina-idp.dev.openConext.local\/SingleSignOnService"
+                }
+            ],
+            "name": {
+                "en": "Dummy IdP",
+                "nl": "Dummy IdP nl"
+            }
+        }
+    }
+}