keycloak-api-rails
keycloak-api-rails copied to clipboard
[Draft] Initial refactoring for dynamic config based on request
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.
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 withKeycloak::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 aKeyResolver
that can be "injected" as needed. - Testing: As you said, it needs to be tested
@looorent Thanks for the detailed response and all of them make sense. I'll try to tackle them one by one.