@jonkoops Thanks so much for your review. I've done the requested changes and sorry for not running the linter before the initial PR.

Sorry for the late review, had a tonne of backlog I needed to plow through. @sschu I know you had some ideas about this implementation, so would you mind giving this a review? @ssilvert I would also like your take on this.

@ullgren I think we can call this implementation final right? I will take the PR out of draft.

2 flaky tests on run #9661 ↗︎

0

527

48

0

2

Details:

  clients_test.spec.ts • 1 flaky test • chrome

View Output Video

Test		Artifacts
Clients test > Keys tab test > Generate new keys		Test Replay Output Screenshots

  realm_settings_general_tab_test.spec.ts • 1 flaky test • chrome

View Output Video

Test		Artifacts
Realm settings general tab tests > Test all general tab switches		Test Replay Output Screenshots

Review all test suite changes for PR #24266 ↗︎

@jonkoops My frontend knowledge is pretty close to zero so I fear I cannot really help with the code review.
However, I am still not convinced that binding the decision whether clientid and secret are shown to the fact if the provider is a SocialIdentityProvider is a good solution. It should really just depend on the protocol being used.

However, I am still not convinced that binding the decision whether clientid and secret are shown to the fact if the provider is a SocialIdentityProvider is a good solution. It should really just depend on the protocol being used.

So what would this implementation preferably look like? If there is a better way to solve this issue I'd like to make an assessment of the effort that would take. I think I will loop in the core team to discuss this. @keycloak/core.

I think from the UI perspective it would be sufficient if an identity provider exposes metadata on the fields it would like to have rendered in the UI. Either specific to the problem at hand (showClientIDAndSecret) or maybe even all the fields it would like to have. But maybe that is over the top. Could be that the only real variation here is if client id and secret are there or not and the rest is identical.

@sschu I agree with you about using metadata would be a better solution. There are more fields that would be good to be able to control using the metadata, the "Display Name" and "Alias" fields are two examples that come to mind. Those fields are currently shown if the provider has a id that includes the sub-strings "oidc" and/or "saml".
IMHO it would be much better if the UI is completely configured based on the metadata. For instance I don't see the reason for having the divide between Social and non-social providers which is in the admin UI today.
But I also see this as a bigger redesign since it will change the provider API.

I see this PR as a intermediate fix, to be able to ship a 22.x version of our community providers. Something that we have been unable to do since the old admin theme was retired. While not making things worse since there are already decisions based on what interface the provider implements or parts of the provider id string.

@ullgren I get your reasoning. I am just a bit concerned something breaks, i.e. there already are non-social IPDs that require clientid/secret. How can we ensure this is not the case?

I'm on board with @sschu. The fact that social providers require client id and the secret is because they are most of the time OAuth2/OIDC ASs/OPs. We also have brokers that are not social and might be set with client id and secret (E.g.: generic OAuth2/OIDC brokers).

The decision to show those fields should be based on the metadata exposed by the provider implementation. It would be the ideal solution.

However, I see that reviewing all implementations (and UIs) to expose their metadata is not a trivial task.

@jonkoops I would vote to create an issue/epic to review both UI and broker implementations to use metadata and try to workaround the problem somehow in the UI.

@sschu All out of the box providers work with this patch. The once that need client id/secret either inherits the SocialIdentityProvider or there providerid includes "oidc" which makes the code load the <OIDCGeneralSettings /> component instead (as it did before the patch).

I've also looked through the providers listed on the extension page (https://www.keycloak.org/extensions.html) and while I have not tested them, all of them also inherits the SocialIdentityProvider interface and should work as expected with this patch.

Thanks for the effort! However, people might still have their own providers, so you cannot really guarantee nothing breaks.

Possibly related to #24710

I would vote to create an issue/epic to review both UI and broker implementations to use metadata and try to workaround the problem somehow in the UI.

+1. I unfortunately don't have time for this now, can someone set this up? What do we want to do with this PR?

I afraid that if we accept this patch, there will be probably someone else who will report bug like "My custom identity provider does not work anymore. It does not show clientId/clientSecret" . And unfortunately that one can be considered as a "regression" and hence we would need to treat it with the priority once it happens ;-) And people with existing providers cannot easily rename them to something different containing "oidc" (DB migration might not be trivial due the fact that we seem to use "providerAlias" in FederatedIdentityModel for some reason and hence renaming alias will likely break all the linked users).

So my vote is to not accept this, but rather have some metadata available on IDP itself whether clientId/clientSecret should be used or not.

I've been looking into the option to provide meta data in the api and as I said I agree that this is the better option.

However this would also break any existing implementation in a similar manner since there is no guarantee which interfaces and classes existing implementations are using. So before I go on with such a implementation I would like to know if it is OK that such a solution, where metadata is provided from the IDP, does introduce potential breaking changes.

Of course we can make it as non intrusive as possible, such as making sure that inheriting SocialIdentityProvider will generate the correct metadata to show the client id/secret fields. But since we will then move away from the current implementation where substring matching of the class name of the IDP implementation is used it will be hard to make it 100% backwards compatible.