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

quarkus-oidc adapter has to support KeycloakConfigResolver alternative available in VertxOAuth2 #4448

Closed
sberyozkin opened this issue Oct 8, 2019 · 10 comments
Labels
area/oidc kind/enhancement New feature or request

Comments

@sberyozkin
Copy link
Member

@pedroigor FYI

@sberyozkin sberyozkin added kind/enhancement New feature or request area/oidc labels Oct 8, 2019
@sberyozkin
Copy link
Member Author

A comment from a user:

Description
I've implemented the KeycloakConfigResolver due to the fact that our backend service must deal with multiple tenants. In that situation, why must i specify a keycloak.json file (or properties) inside resources folder? I know that can be useful as a "default fallback", but in some cases we just don't want something like this. Imagine a sidecar service which intercepts calls before forwarding to the underlying microservice. In this case we'll have:

    Several tenants configuration
    Clients in configurations could be different from one another
    files named as the realm name (we intercept the JWT and resolve the config path extracting the realm name from the iss field)

@sberyozkin
Copy link
Member Author

See this Vertx issue.

@sberyozkin
Copy link
Member Author

Hi Pedro @pedroigor I'll be happy to work with you on this one in the next week or so. If you'll have time then I'll be helping alongside, otherwise I can probably fix it.

The question I have is, now that we have a realm value appended to the IDP URL, do we need to parameterize that value ? Or, with the multi tenant deployments, it is still the same realm, only different client properties within that realm ?

@sberyozkin
Copy link
Member Author

@pedroigor I reckon now it does not really matter, whether it is realm specific or not, we'll likely need to use the configuration profiles anyway.

@pedroigor
Copy link
Contributor

@sberyozkin We should also be able to allow configuration to be chosen based on incoming requests (dynamically). For instance, depending on a path you may want to use Tenant A, etc ..

@sberyozkin
Copy link
Member Author

sberyozkin commented Dec 4, 2019

The discussion at the now closed #5899 is relevant. CC @yuhaibohotmail

@sberyozkin
Copy link
Member Author

sberyozkin commented Dec 5, 2019

@pedroigor sure, the question is how to group/scope the configurations which will be selected dynamically. @emmanuelbernard suggested earlier we should follow the way it is done for supporting multiple DB providers, I suppose we should have some convention:

quarkus.oidc.`qualifier1`.auth-base-url`=...
quarkus.oidc.`qualifier2`.auth-base-url`=...

and then the handler would decide on the qualifier and load it. Or may be to simplify things we can start from expecting the users constructing the adapter configuration in the handler directly...

@pedroigor
Copy link
Contributor

@pedroigor sure, the question is how to group/scope the configurations which will be selected dynamically. @emmanuelbernard suggested earlier we should follow the way it is done for supporting multiple DB providers, I suppose we should have some convention:

quarkus.oidc.`qualifier1`.auth-base-url`=...
quarkus.oidc.`qualifier2`.auth-base-url`=...

and then the handler would decide on the qualifier and load it. Or may be to simplify things we can start from expecting the users constructing the adapter configuration in the handler directly...

@sberyozkin I think both solutions should be considered. So users can either have a static definition of their tenants or a more dynamic version they can define/choose during runtime.

FYI, in Keycloak we pretty much do #2, where the adapter configuration is built at runtime based on whatever logic you have for choosing a tenant.

Maybe, for #2, it should be just a matter of allowing applications to produce a TenantConfiguration (or something similar) so that we grab instances at runtime and use them during the processing. Makes sense for you ?

@pedroigor
Copy link
Contributor

@sberyozkin I started something at this regard and from a user perspective, you would implement the following interface in order to resolve tenants:

public interface TenantResolver {   
    String resolve(RoutingContext context);
}

While it works for resolving tenants based on the static configuration in application.properties we should also provide some form of resolver that users can implement to provide the configuration as a object.

What do you think?

pedroigor added a commit to pedroigor/quarkus that referenced this issue Dec 27, 2019
pedroigor added a commit to pedroigor/quarkus that referenced this issue Dec 27, 2019
pedroigor added a commit to pedroigor/quarkus that referenced this issue Dec 27, 2019
pedroigor added a commit to pedroigor/quarkus that referenced this issue Dec 27, 2019
pedroigor added a commit to pedroigor/quarkus that referenced this issue Dec 27, 2019
pedroigor added a commit to pedroigor/quarkus that referenced this issue Dec 27, 2019
@sberyozkin
Copy link
Member Author

sberyozkin commented Dec 30, 2019

@pedroigor Hi Pedro, sorry I've missed this comment. As noted in your PR, the way you've done the resolution from the application properties based on the tenant id idea loos very good IMHO.

Re the configuration as an object, can MultiTenantHandler help ? Or do you have something different in mind ? We can discuss it here or in the PR, as you prefer

pedroigor added a commit to pedroigor/quarkus that referenced this issue Dec 30, 2019
pedroigor added a commit to pedroigor/quarkus that referenced this issue Dec 30, 2019
pedroigor added a commit to pedroigor/quarkus that referenced this issue Dec 30, 2019
pedroigor added a commit to pedroigor/quarkus that referenced this issue Dec 30, 2019
pedroigor added a commit to pedroigor/quarkus that referenced this issue Jan 2, 2020
pedroigor added a commit to pedroigor/quarkus that referenced this issue Jan 2, 2020
pedroigor added a commit to pedroigor/quarkus that referenced this issue Jan 2, 2020
sberyozkin added a commit that referenced this issue Jan 15, 2020
[fixes #4448] - OIDC Multi-tenancy Support
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/oidc kind/enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants