Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hide nameIdFormat when unspecified #668

Merged
merged 1 commit into from
Aug 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ public static function getValidNameIdFormats(): array
return [
Constants::NAME_ID_FORMAT_TRANSIENT,
Constants::NAME_ID_FORMAT_PERSISTENT,
Constants::NAME_ID_FORMAT_UNSPECIFIED,
];
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@
use Symfony\Component\Form\Extension\Core\Type\TextareaType;
use Symfony\Component\Form\Extension\Core\Type\TextType;
use Symfony\Component\Form\FormBuilderInterface;
use Symfony\Component\Form\FormEvent;
use Symfony\Component\Form\FormEvents;
use Symfony\Component\OptionsResolver\OptionsResolver;

/**
Expand Down Expand Up @@ -373,6 +375,18 @@ public function buildForm(FormBuilderInterface $builder, array $options): void

->add('publishButton', SubmitType::class, ['label'=> $options['publish_button_label'], 'attr' => ['class' => 'button']])
->add('cancel', SubmitType::class, ['attr' => ['class' => 'button']]);

// When the Oidc entity is set to have an UNSPECIFIED subject type (in manage) do not show the field on the form
$builder->addEventListener(FormEvents::PRE_SET_DATA, function (FormEvent $event): void {
/** @var SaveOidcngEntityCommand $data */
$data = $event->getData();
if ($data->getSubjectType() === Constants::NAME_ID_FORMAT_UNSPECIFIED) {
$form = $event->getForm();
if ($form->has('metadata') && $form->get('metadata')->has('subjectType')) {
$form->get('metadata')->remove('subjectType');
}
}
});
}

private function buildAttributeTypes(FormBuilderInterface $container): FormBuilderInterface
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
use Symfony\Component\Form\Extension\Core\Type\TextareaType;
use Symfony\Component\Form\Extension\Core\Type\TextType;
use Symfony\Component\Form\FormBuilderInterface;
use Symfony\Component\Form\FormEvent;
use Symfony\Component\Form\FormEvents;
use Symfony\Component\OptionsResolver\OptionsResolver;

/**
Expand Down Expand Up @@ -323,6 +325,18 @@ public function buildForm(FormBuilderInterface $builder, array $options): void

->add('publishButton', SubmitType::class, ['label'=> $options['publish_button_label'], 'attr' => ['class' => 'button']])
->add('cancel', SubmitType::class, ['attr' => ['class' => 'button']]);

// When the SAML2.0 entity is set to have an UNSPECIFIED name id format (in manage) do not show the field on the form
$builder->addEventListener(FormEvents::PRE_SET_DATA, function (FormEvent $event): void {
/** @var SaveSamlEntityCommand $data */
$data = $event->getData();
if ($data->getNameIdFormat() === Constants::NAME_ID_FORMAT_UNSPECIFIED) {
$form = $event->getForm();
if ($form->has('metadata') && $form->get('metadata')->has('nameIdFormat')) {
$form->get('metadata')->remove('nameIdFormat');
}
}
});
}
Comment on lines +328 to 340
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see some repetition

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not realy a dry thing in my opinion; the one block deals with the nameIdFormat and the other with the clientId. They kind-of are the same thing. Leaving it as is for now


public function configureOptions(OptionsResolver $resolver): void
Expand Down
28 changes: 28 additions & 0 deletions tests/webtests/EntityEditTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
namespace Surfnet\ServiceProviderDashboard\Webtests;

use Surfnet\ServiceProviderDashboard\Application\Service\AttributeService;
use Surfnet\ServiceProviderDashboard\Domain\Entity\Constants;
use Surfnet\ServiceProviderDashboard\Infrastructure\DashboardBundle\DataFixtures\ORM\WebTestFixtures;
use Surfnet\ServiceProviderDashboard\Infrastructure\DashboardBundle\Form\Entity\AttributeType;

Expand Down Expand Up @@ -81,6 +82,33 @@ public function test_it_renders_the_form()
);
}

public function test_it_hides_name_id_format_unspecified()
{
// 1. When NameIdFormat is not Unspecified, the form field does appear on the form
$crawler = self::$pantherClient->request('GET', "/entity/edit/test/{$this->manageId}/1");
$this->assertOnPage(
'Name id format',
$crawler
);
// 2. But when the nameIdFormat is unspecified, the form field is not displayed on the form
$this->registerManageEntity(
'test',
'saml20_sp',
'88888888-1111-1111-1111-888888888888',
'SP1',
'https://spx-entityid.example.com',
'https://spx-entityid.example.com/metadata',
WebTestFixtures::TEAMNAME_SURF,
'12',
Constants::NAME_ID_FORMAT_UNSPECIFIED
);
$crawler = self::$pantherClient->request('GET', "/entity/edit/test/88888888-1111-1111-1111-888888888888/1");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not have to be with a full blown panther instance I guess, could also be done with lightweigth kernelbrowser

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, only thing is we only have a panther based test setup. This could be a nice optimization we could look into. Although the performance gain is not that big IIRC from when we chose to move to Panther.

$this->assertNotOnPage(
'Name id format',
$crawler
);
}

public function test_it_rejects_unauthorized_visitors()
{
$ibuildings = $this->getServiceRepository()->findByName('Ibuildings B.V.');
Expand Down
8 changes: 7 additions & 1 deletion tests/webtests/Manage/Client/ClientResult.php
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ class ClientResult implements ClientResultInterface
private $teamName;

private string $institutionId;
private string $nameIdFormat;

public function __construct(
string $protocol,
Expand All @@ -50,6 +51,7 @@ public function __construct(
string $name,
?string $teamName,
?string $institutionId,
?string $nameIdFormat,
) {
$this->id = $id;
$this->protocol = $protocol;
Expand All @@ -64,6 +66,7 @@ public function __construct(
if ($teamName === null) {
$this->teamName = WebTestFixtures::TEAMNAME_SURF;
}
$this->nameIdFormat = $nameIdFormat ?? 'nameidformat';
}

public function getEntityResult(): array
Expand Down Expand Up @@ -93,7 +96,8 @@ public function getEntityResult(): array
$this->name,
str_replace('_', '-', $this->protocol),
$this->teamName,
$this->institutionId
$this->institutionId,
$this->nameIdFormat,
);
return json_decode($data, true);
}
Expand Down Expand Up @@ -124,6 +128,7 @@ public static function decode($data): self
$data['name'],
$data['teamName'],
$data['institutionId'],
$data['nameIdFormat'],
);
}

Expand All @@ -137,6 +142,7 @@ public function encode(): array
'name' => $this->name,
'teamName' => $this->teamName,
'institutionId' => $this->institutionId,
'nameIdFormat' => $this->nameIdFormat,
];
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class FakeIdentityProviderClient implements IdentityProviderRepository

public function registerEntity(string $protocol, string $id, string $entityId, string $name, string $institutionId = '')
{
$this->entities[$id] = new ClientResult($protocol, $id, $entityId, null, $name, null, $institutionId);
$this->entities[$id] = new ClientResult($protocol, $id, $entityId, null, $name, null, $institutionId, null);
$this->storeEntities();
}

Expand Down
3 changes: 2 additions & 1 deletion tests/webtests/Manage/Client/FakeQueryClient.php
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,9 @@ public function registerEntity(
string $name,
?string $teamName = null,
?string $institutionId = '',
?string $nameIdFormat = '',
) {
$this->entities[$id] = new ClientResult($protocol, $id, $entityId, $metadataUrl, $name, $teamName, $institutionId);
$this->entities[$id] = new ClientResult($protocol, $id, $entityId, $metadataUrl, $name, $teamName, $institutionId, $nameIdFormat);
$this->storeEntities();
}

Expand Down
2 changes: 1 addition & 1 deletion tests/webtests/Manage/Client/template/ccc.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
"grants": [
"client_credentials"
],
"NameIDFormat": "nameidformat",
"NameIDFormat": "%9$s",
"isResourceServer": false,
"contacts:0:emailAddress": "[email protected]",
"contacts:0:contactType": "support",
Expand Down
2 changes: 1 addition & 1 deletion tests/webtests/Manage/Client/template/oidc10.json
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@
"authorization_code",
"refresh_token"
],
"NameIDFormat": "nameidformat",
"NameIDFormat": "%9$s",
"isResourceServer": false,
"contacts:0:emailAddress": "[email protected]",
"contacts:0:contactType": "support",
Expand Down
2 changes: 1 addition & 1 deletion tests/webtests/Manage/Client/template/saml20_sp.json
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
"metaDataFields": {
"AssertionConsumerService:0:Binding": "urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST",
"AssertionConsumerService:0:Location": "%3$s\/acs",
"NameIDFormat": "nameidformat",
"NameIDFormat": "%9$s",
"description:en": "%5$s Description English",
"description:nl": "%5$s Description Dutch",
"name:en": "%5$s Name English",
Expand Down
10 changes: 8 additions & 2 deletions tests/webtests/WebTestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
use PHPUnit\Framework\ExpectationFailedException;
use RuntimeException;
use Surfnet\ServiceProviderDashboard\Application\Exception\InvalidArgumentException;
use Surfnet\ServiceProviderDashboard\Domain\Entity\Constants;
use Surfnet\ServiceProviderDashboard\Domain\Entity\Service;
use Surfnet\ServiceProviderDashboard\Domain\Repository\DeleteManageEntityRepository;
use Surfnet\ServiceProviderDashboard\Domain\Repository\IdentityProviderRepository;
Expand Down Expand Up @@ -182,6 +183,7 @@ protected function registerManageEntity(
?string $metadataUrl = null,
?string $teamName = null,
?string $institutionId = '',
?string $nameIdFormat = Constants::NAME_ID_FORMAT_PERSISTENT
) {
switch ($protocol) {
case 'saml20_sp':
Expand All @@ -196,6 +198,7 @@ protected function registerManageEntity(
$metadataUrl,
$teamName,
$institutionId,
$nameIdFormat,
);
break;
case 'saml20_idp':
Expand Down Expand Up @@ -242,6 +245,7 @@ private function registerSp(
?string $metadataUrl = null,
?string $teamName = null,
?string $institutionId = '',
?string $nameIdFormat = '',
) {
switch ($env) {
case 'production':
Expand All @@ -252,7 +256,8 @@ private function registerSp(
$metadataUrl,
$name,
$teamName,
$institutionId
$institutionId,
$nameIdFormat
);
break;
case 'test':
Expand All @@ -263,7 +268,8 @@ private function registerSp(
$metadataUrl,
$name,
$teamName,
$institutionId
$institutionId,
$nameIdFormat
);
break;
default:
Expand Down
Loading