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

Add dynamic configuration support for Keycloak Authz #17750

Conversation

ethan-gallant
Copy link
Contributor

This is an implementation for issue #17664

Most of this code is inspired by the implementation used in the OIDC extension, however, things differ in places due to multiple configurations being depended on and keycloak-authorization being an "addon" to OIDC.

@sberyozkin
Copy link
Member

sberyozkin commented Jun 8, 2021

@ethan-gallant Hi, thanks for this PR. I was expecting it to be significantly simpler, the proposed implementation idea I typed at #17664 was likely not a good idea.
See, by the time quarkus-keycloak-authorization is invoked in a multi-tenant case, tenant-id must've already been set by quarkus-oidc - the expectation is it must be the same tenant-id that should be used when quarkus-keycloak-authorization creates its own dynamic config - we assumed it must be the case with @pedroigor when the initial multi-tenancy support was introduced.

Also I don't think OIDC DefaultTenantConfigResolver needs to be mirrored in this case - in case of quarkus-keycloak-authorization we only need a single call to get the required config and it won't be called again in scope of the current request.

Can you please simplify this PR, please don't rename the existing PolicyEnforcerResolver at the moment - we can do it later, just to make it easier to review.

Lets start with having a new PolicyTenantConfigResolver injected into PolicyEnforcerResolver and I think PolicyEnforcerResolver needs to continue accepting a tenantId parameter only and a new PolicyTenantConfigResolver should only accept tenantId (which has already been prepared by quarkus-oidc), I think my implementation idea to pass RoutingContext was wrong: both quarkus-oidc and quarkus-keycloak-authorization work as a single unit, so the situation where quarkus-oidc has one tenant-id and quarkus-keycloak-authorization another different one would be problematic.

PolicyEnforcerResolver would indeed have a map for keeping the dynamically prepared policy enforcers but that would be the only similarity I guess (and we can rename it if you'd like - but a bit later on).

Please also consider turning this PR into a draft one for the moment

Thanks.

@sberyozkin
Copy link
Member

sberyozkin commented Jun 8, 2021

@ethan-gallant Please also note the conversion of the config to PolicyEnforcer is not blocking - no remote operation is done at this time, so the function should simply return PolicyEnforcer - no need to trace the dynamic map, or run it on the blocking executor, etc.
as far as KeycloakRecorder is concerned, I believe the only change that should happen there is passing this Function instance to PolicyEnforcerResolver bean.

@sberyozkin
Copy link
Member

sberyozkin commented Jun 8, 2021

Re RoutingContext - I forgot it is still needed to retrieve OidcTenantConfig which is one of the inputs into the function creating PolicyEnforcer - so it can be passed alongside the tenant id to PolicyEnforcerResolver but the dynamic keycloak tenant config resolver does not need it. I guess it would also be clearer if a new interface is called KeycloakTenantConfigResolver

@ethan-gallant ethan-gallant marked this pull request as draft June 8, 2021 15:25
@gsmet
Copy link
Member

gsmet commented Feb 2, 2023

@sberyozkin what should we do with this PR?

@gsmet gsmet added triage/needs-feedback We are waiting for feedback. triage/needs-rebase This PR needs to be rebased first because it has merge conflicts labels Feb 2, 2023
@michalvavrik
Copy link
Member

hey everyone, there has been no activity for nearly 3 years. I'm closing this in favor of the #39643 I just opened. @ethan-gallant please if you ever find a time to contribute in the future, ping @sberyozkin . Thanks

@quarkus-bot quarkus-bot bot added the triage/invalid This doesn't seem right label Mar 22, 2024
@michalvavrik michalvavrik removed triage/needs-rebase This PR needs to be rebased first because it has merge conflicts triage/needs-feedback We are waiting for feedback. labels Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage/invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants