Client type OIDC base

[patrickjennings]

Creating OIDC client type with applicable fields for OIDC based clients. In the future, this could be a basis for other OIDC types of clients or may be chosen as a default when selecting OIDC protocol.

No auth flow is enabled for this type by default, but one or more applicable flows may be specified during client creation.

In order to have default values for client type fields that are not read-only, we have to augment the ClientRepresentation.

#29422

[keycloak-github-bot]

Unreported flaky test detected, please review

[keycloak-github-bot]

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.forms.BrowserFlowTest#testLoginWithWithNoOTPCredential

Keycloak CI - Forms IT (firefox)

java.lang.AssertionError
	at org.junit.Assert.fail(Assert.java:87)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.junit.Assert.assertTrue(Assert.java:53)
	at org.keycloak.testsuite.forms.BrowserFlowTest.testLoginWithWithNoOTPCredential(BrowserFlowTest.java:929)
...

Report flaky test

[patrickjennings]

@mposolda @vickeybrown @jsorah . Here is the base client type for OIDC.

In order to have fields with default values but are not read-only, we have to have a ClientRepresentation that is aware of the client type configuration. If the user creates a client with type but does not specify a particular value, it will default to what is on the client type config.

Let me know what you think.

[patrickjennings]

What do you think of having these auth flows defaulted to false but not read-only? I could see a case where one would want to pick a generic OIDC client type and specify things themselves, but if this to be a base for other client types, it might also make sense to make these read-only and have the children client types override the fields.

[mposolda]

+1 to not have those as read-only for basic oidc type

[mposolda]

@patrickjennings Added few comments inline.

Also I am curious for the feedback from others assigned to this PR before merging this.

[mposolda]

I am not sure if we need another class like TypeDefaultedClientRepresentation . I was hoping that when representation is needed to be converted to the clientModel, we would be able to do something like:

ClientModel client = RepresentationToModel.toModel(realm, representation);
client = augment(client);
return ModelToRepresentation.toRepresentation(client);

but maybe I am oversimplifying?

I am fine with including your code for now if you don't think the stuff above to be easily doable.

[patrickjennings]

Yeah that would be ideal.

What I am struggling to solve is that some of the getters on ClientModel are returning primitives, for instance, isPublicClient which returns boolean. We cannot know whether the user had specified some value or not within TypeAwareClientModelDelegate. That means we cannot determine whether to return the client type default value (specifically for non-read only fields) or what is currently set on the delegate.

• https://github.com/keycloak/keycloak/blob/main/services/src/main/java/org/keycloak/services/clienttype/client/TypedClientSimpleAttribute.java#L112

Since the TypeAwareClientModelDelegate class is the only thing that is currently client type aware, it seemed like we would have to make something else type aware to satisfy the ability to support client type fields with override-able (non-read only) default values. That was the purpose of the TypeDefaultedClientRepresentation which is currently only applied during client creation. It's purpose is to return the field supplied by the user or if not supplied, return the default value on the client type.

There was some other things we could consider:

• Make these methods return non-primitive alternatives.
  • Con: These are public interfaces, would be a large breaking change.
  • These fields are non-null in the db so they would have to be set before the entity is persisted. So this solution might not be possible at all.
• Make client type field values read-only.
  • Con: Cannot override values. Admin has to create a new client type definition.

Open to suggestions on what you think.
cc @vickeybrown @jsorah

[mposolda]

+1 to not have those as read-only for basic oidc type

[vickeybrown]

LGTM!