keycloak-api-rails icon indicating copy to clipboard operation
keycloak-api-rails copied to clipboard

[Draft] Initial refactoring for dynamic config based on request

Open rajaravivarma-r opened this issue 1 year ago • 2 comments

Hi Lorent,

I've made some untested changes to make the configuration dynamic and removed some duplicate code. The idea behind this pull request is to see if you would be willing to accept something like this.

We have been using this gem to authenticate our Rails APIs, but we use different Keycloak realms for different tenants. Therefore, we would like to configure the Keycloak endpoints based on the request.

For example, if the URL is https://ourapp.com/hdfc, then we will know that hdfc is the tenant, and we will configure the Keycloak endpoints accordingly.

Now, the configure block would look like this:

Keycloak.configure do |request|
  lambda do |config|
    config.realm_id = request.url.split('/').last
    ...
  end
end

Please let me know your thoughts. I can add tests accordingly.

rajaravivarma-r avatar Aug 01 '23 11:08 rajaravivarma-r

Hi @rajaravivarma-r , thanks for your message.

The idea of making every call configurable is nice.

I acknowledge that the current implementation is not yet finalized, and I appreciate that you are presenting this idea for further consideration. I have carefully reviewed the proposal with this context in mind.

Before diving into the code details, I'd like to establish some guidelines to ensure the proposed changes align with the library's goals:

  • Optional Alternative Behavior: The changes should provide an alternative option to the default behavior, while still allowing the existing behavior to be maintained.
  • No breaking change : We want to avoid breaking existing implementations and ensure backward compatibility. If there are breaking changes, they should be documented.
  • Runtime Configurability: The implementation you suggest might be configurable at runtime. Consider adopting a "Dependency Injection" approach or similar techniques to allow developers to provide their own implementations of specific components. For instance, the ServiceFactory currently tightly couples with Keycloak::PublicKeyCachedResolver, but we should explore making these dependencies configurable.
  • Performance Considerations: If certain services require caching or other optimizations, it would be beneficial to provide cached implementations for better performance. For example, the ServiceFactory implementation might cause significant performance impact if it repeatedly retrieves the public key of the realm for each call. If such behavior is desired, it should be achieved through a specific implementation of a KeyResolver that can be "injected" as needed.
  • Testing: As you said, it needs to be tested

looorent avatar Aug 02 '23 07:08 looorent

@looorent Thanks for the detailed response and all of them make sense. I'll try to tackle them one by one.

rajaravivarma-r avatar Aug 02 '23 11:08 rajaravivarma-r