SSP-2031_OIDC_module_fix_display_of_public_clients

[ioigoume]

Improve display of public clients

[cicnavi]

Thanks @ioigoume! It definitely makes sense to write out the type of client instead of changing color of the label... Since this is well defined in the spec: https://datatracker.ietf.org/doc/html/rfc6749#section-2.1, I would even go for a 'radio' options in edit screen and a separate row for the client type for the show screen, but I'll merge it as is if you don't have time to do it ... What is the meaning of a 'lock' icon in this context?

[ioigoume]

@cicnavi, thank you for the feeback.

Thanks @ioigoume! It definitely makes sense to write out the type of client instead of changing color of the label... Since this is well defined in the spec: https://datatracker.ietf.org/doc/html/rfc6749#section-2.1, I would even go for a 'radio' options in edit screen and a separate row for the client type for the show screen, but I'll merge it as is if you don't have time to do it ...

I will make the radio button change today and ping you.

What is the meaning of a 'lock' icon in this context?

Locked for confidential and unlocked for public. Do you think the use of an icon is inappropriate or perhaps use a different icon?

Ioannis

[cicnavi]

IMHO the lock is a bit confusing... I would simply not use the icon at all.

[ioigoume]

@cicnavi , changing the client type to a list of radio buttons means we must also update the database. Now we use the is_confidential boolean column that is true in case of confidential and false otherwise. If we move to radio buttons we have to rename the column to client_type and then store a different string for each type. Is this what you have in mind?

I pushed some more changes for the show view according to your comments

Ioannis

[pradtke]

I don't think we should change the DB. If the day comes where there are more than two types of clients we can talk about doing such a change.

I think switching to radio buttons does make it more clear that you are choosing between being public and confidential, so I'm in favor of that change. I think you can isolate the clientform changes from the DB model in EditController->__invoke and maybe ClientForm->setDefaults()

[cicnavi]

Right, so no DB changes. Just two radio options with boolean values 0 or 1 which would map to 'is_confidential'. It is already casted to bool when persisting, for example: src/Controller/Client/EditController.php:101 or src/Controller/Client/CreateController.php:94, with no tinkering with it in ClientForm::getValues and ClientForm::setDefaults. But if needed, you know sanitize / cast that input in those methods in ClientForm.

[ioigoume]

@cicnavi , @pradtke acknowledged!

[cicnavi]

Oh, and here are samples on the radios from semantic.css which is used: https://fomantic-ui.com/modules/checkbox.html#radio Maybe inline or invisible... (invisible not available in current version)

[ioigoume]

@cicnavi PR is ready for review