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

Hide nameIdFormat when unspecified #668

merged 1 commit into from
Aug 15, 2024

Conversation

MKodde
Copy link
Contributor

@MKodde MKodde commented Aug 8, 2024

When the 'subjectType' or 'nameIdFormat' is unspecified. The matching form field is not displayed on the form. That way we do not allow the user to overwrite this value in Manage (which is explicitly set for that SP by a product owner)

To test this, the mock SP storage needed to track the nameIdFormat value as a manage entity. That requires quite some test code to be edited..

See: https://www.pivotaltracker.com/story/show/185952423

When the 'subjectType' or 'nameIdFormat' is unspecified. The matching form field
is not displayed on the form. That way we do not allow the user to
overwrite this value in Manage (which is explicitly set for that SP by a
product owner)

To test this, the mock SP storage needed to track the nameIdFormat value
as a manage entity. That requires quite some test code to be edited..

See: https://www.pivotaltracker.com/story/show/185952423
@MKodde MKodde requested a review from parijke August 8, 2024 06:24
Copy link
Contributor

@parijke parijke left a comment

Choose a reason for hiding this comment

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

Excellent job again!, just a few questions/ suggestions

Comment on lines +328 to 340

// 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');
}
}
});
}
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

'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.

@MKodde MKodde merged commit c6c15a3 into main Aug 15, 2024
2 of 3 checks passed
@MKodde MKodde deleted the feature/stay-unspecified branch August 15, 2024 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants