incorrecte foutmelding bij edit test entity

[teunfransen]

Als ik een RP wijzig en publiceer op de testomgeving, zie ik zowel een groene 'geslaagd' als een rode 'niet geslaagd' melding in beeld

In het voorbeeld hieronder heb ik het logo aangepast en de redirect URI aangepast

[teunfransen]

@baszoetekouw gereproduceerd op 18 juni op 12:48: Wijziging aangebracht (name dienst en beschrijving) rp (tweestbart5punt0.nl). Eerst krijg ik een error occurred, daarna een rode balk (unable to publish the entity, try again later) en een succesvolle groene balk ‘changes were saved’. In manage zijn de wijzigingen goed doorgevoerd

[baszoetekouw]

Het probleem hier is dat ergens tijdens het publiceren de hoofdletters in de entityid zijn verdwenen. Engineblock kan hier niet goed tegen, waardoor de push faalt.

Issue wordt dus veroorzaakt door deze changeset in Manage:

Vraag is dit hoe dit gebeurd kan zijn, want die entityid/clientid is in de interface al immutable.

[johanib]

Ik heb het nog niet kunnen reproduceren, maar zie in de code wel een aantal aanwijzingen:

• https://github.com/SURFnet/sp-dashboard/blob/release/6.2/src/Surfnet/ServiceProviderDashboard/Domain/Entity/Entity/OidcngResourceServerClient.php#L50
• https://github.com/SURFnet/sp-dashboard/blob/release/6.2/src/Surfnet/ServiceProviderDashboard/Application/Command/Entity/SaveOidcngResourceServerEntityCommand.php#L226
• https://github.com/SURFnet/sp-dashboard/blob/release/6.2/src/Surfnet/ServiceProviderDashboard/Application/Command/Entity/SaveOidcngEntityCommand.php#L352

Ik weet niet of we die strtolower zomaar kunnen weghalen, want dat kan gevolgen hebben voor evt case-sensitive comparisons.
Hier moet ik nog even goed naar kijken dus, maar lijkt op het eerste gezicht aannemelijk dat spdashboard die casing veranderd.

[johanib]

This seems to stem from #472 & #476

As far as I can see, the enity id should not be converted to lowercase, but the client id should?

@teunfransen or @baszoetekouw Can you give some context as to why the entity id or clientId should be lowercased?
Also, is it correct this issue only applies to oidc entities?

Tentative PR: #1387

[baszoetekouw]

I think in general it is wise to require that everything should be lowercase (so that we avoid problems with case changes in Manage/EB). However, we can't require that for SAML entityids, because the SP itself might generate an entityid with capitals.

So yes, I think the behaviour is correct that this only applies to OIDC.

However, for legacy entries (that currently have uppercase client_id) this will lead to errors when editing and republishing the entry (because EB can't handle the case change).

I think you PR is correct. How hard would it be to add a functionality that would require lower-case client_id for new entityids only?

[johanib]

That should be doable, as they are separate controller actions \Surfnet\ServiceProviderDashboard\Infrastructure\DashboardBundle\Factory\EntityTypeFactory::createCreateForm

Should that also apply to the entityId? Can entityId's contain capitals?