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

Fix logic around what happens to client when client type is deleted #29111

Closed

Conversation

vickeybrown
Copy link
Contributor

If you try to retrieve a client that has a client type that's been deleted, the client type gets set to null and the client is successfully retrieved.

closes #29061

…leted, the client type gets set to null and the client is successfully retrieved.

closes keycloak#29061

Signed-off-by: vibrown <vibrown@redhat.com>
@vickeybrown vickeybrown requested a review from a team as a code owner April 26, 2024 14:49
@vickeybrown vickeybrown requested review from patrickjennings and jsorah and removed request for patrickjennings and jsorah April 26, 2024 14:50
This was referenced Apr 26, 2024
@vickeybrown vickeybrown marked this pull request as draft April 26, 2024 19:08
@mposolda
Copy link
Contributor

mposolda commented Apr 29, 2024

@vickeybrown I am not 100% sure about this change.

When client-type is not available and there are some clients of this type, there should be either error thrown or at least the client should not be available at all in Keycloak. With the change, I can imagine various sorts of issues like for instance:

  • Client type foo restricts to not allow any protocol mappers
  • Now the Keycloak is accidentally triggered in some environment with not deployed client type foo , but against same DB where clients with client type foo are defined
  • Assume Keycloak will allow to still list such clients and use them, the client admin can now add any protocol mapper to the clients with this client type foo, which is not requested behaviour

My vote is either to keep current behaviour or ignore all clients with the undeployed client-type and log the warning into log about such client being ignored, as unavailable client-type probably means that something bad is going on.

Any particular use-case for this change?

@mposolda mposolda self-assigned this Apr 29, 2024
@vickeybrown
Copy link
Contributor Author

@vickeybrown I am not 100% sure about this change.

When client-type is not available and there are some clients of this type, there should be either error thrown or at least the client should not be available at all in Keycloak. With the change, I can imagine various sorts of issues like for instance:

  • Client type foo restricts to not allow any protocol mappers
  • Now the Keycloak is accidentally triggered in some environment with not deployed client type foo , but against same DB where clients with client type foo are defined
  • Assume Keycloak will allow to still list such clients and use them, the client admin can now add any protocol mapper to the clients with this client type foo, which is not requested behaviour

My vote is either to keep current behaviour or ignore all clients with the undeployed client-type and log the warning into log about such client being ignored, as unavailable client-type probably means that something bad is going on.

Any particular use-case for this change?

My thought for a use case was mainly if we made a custom client type. If we made a type of client called "foo" that was for a specific application, and later on that application gets shut down, we probably wouldn't need that client type anymore but we might still want those clients to be around. Thinking about it though, maybe if the client type wasn't necessary anymore, the clients wouldn't be either? @jsorah @patrickjennings got any thoughts?

@vickeybrown
Copy link
Contributor Author

Talking with the team, I suppose it does make sense to just let the client error out if the client type is removed. If you don't need the client type anymore, do you really need its corresponding clients? Maybe if we encounter a case where we need a client but not the client type, we could just change the client's client type to something else so it's still usable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix logic around what happens to client when client type is deleted
2 participants