Select OIDC tenant using annotations

[j-be]

Fixes #22974.

This PR contains a small code change, as well as documentation for selecting OIDC providers based on annotations, rather than using TenantResolver.

It is currently implemented as suggested by @sberyozkin.

I'd like to propose exposing and using DefaultTenantConfigResolver.CURRENT_STATIC_TENANT_ID directly. This would reduce the necessary code change, like:

        String tenantId = context.get(CURRENT_STATIC_TENANT_ID);

        if (tenantId == null && tenantResolver.isResolvable() && context.get(CURRENT_STATIC_TENANT_ID_NULL) == null) {
            tenantId = tenantResolver.get().resolve(context);
            if (tenantId != null) {
                context.put(CURRENT_STATIC_TENANT_ID, tenantId);
            } else {
                context.put(CURRENT_STATIC_TENANT_ID_NULL, true);
            }
        }

This would also flip precedence, i.e. in the current implementation TenantResolver takes precedence, while by using DefaultTenantConfigResolver.CURRENT_STATIC_TENANT_ID directly the annotations would, which imho. would be the preferred behavior, as annotations on the code are more specific than a globally registered bean.

EDIT: UnitTests/IntegrationTests are still missing. I'm happy to provide some after the above was decided.

[sberyozkin]

@j-be Thanks for opening this PR.

Re the precedence, while it is probably more precise in principle to check the annotations first, I don't think we should be concerned about a situation where TenantResolver decides to return finance while in fact it should be hr as requested via the annotation. These are really two alternative - one either checks everything in TenantResolver - path or header or cookie (in case of web-app) based checks or prefers to have it typed at the endpoint level with the annotations. This is the value of the feature you have proposed - let users make the tenant selection more visible at the endpoint level.

The same is the case with the dynamic vs static - the dynamic resolver is checked first - but may be we could've flipped it around - but we've never had any issues.

FYI, the reason both CURRENT_STATIC_TENANT_ID and CURRENT_STATIC_TENANT_ID_NULL are used in that TenantResolver must be invoked only once during the current request, so CURRENT_STATIC_TENANT_ID_NULL records a possible null return from TenantResolver.

We can flip it the moment it proves to be a limitation for some real world cases, but right now, even this minor update is sensitive as it took me awhile to tune it around various cases. So I propose to take it 1 step at a time.

Can you please:

• squash the commits and have the same commit message as this PR name (minor typo there)
• add the test - we have many tenant specific tests, I guess we can enhance one of the existing ones, for example, this test checks that the endpoint is protected even if the OIDC provider was not available at startup, the tenant configuration is here, and the tenant id is selected here and the call eventually reaches this endpoint. So if you copy this endpoint to a new one and have a root path such as /recovered-no-discovery-annotation/api/users and have the custom annotation and interceptor resolving it to no-discovery then adding one more call here to /recovered-no-discovery-annotation/api/users will verify the PR

thanks

[j-be]

@sberyozkin Alright, will do.

But the way you proposed on how to test it doesn't work afaict. As there is CustomTenantResolver tenantResolver.isResolvable() is always true, but this PR uses that else branch to set the tenant. I don't see any way how to disable a bean using QuarkusTestProfile, so it seems I can't test within that module.

I could just copy the stuff I need from quarkus-integration-test-oidc-wiremock to a dedicated module. What do you think?

[sberyozkin]

@j-be Not sure it is worth creating a new integration tests module.

As there is CustomTenantResolver tenantResolver.isResolvable() is always true, but this PR uses that else branch to set the tenant.

But that is fine, CustomTenantResolver will return null because it does not has anything to return for /recovered-no-discovery-annotation

[sberyozkin]

@j-be Oh I see, then lets update the PR to check RoutingContext if null is returned from TenantResolver :-)

[sberyozkin]

By the way, while TenantResolver resolves static configurations it is more dynamic compared to the fixed annotations approach (ex, different profiles can apply different static configurations to the same endpoint - varying the paths), so I think we are OK with the current order

[j-be]

Fair enough, I changed the implementation in a dedicated commit - will squash only when I'm done for better observability.

Yet, the test you chose seems to be one of the few which does not actually care which tenant is actually active. Even if I patch CustomTenantResolver to be the following:

@ApplicationScoped
public class CustomTenantResolver implements TenantResolver {
    @Override
    public String resolve(RoutingContext context) {
        return null;
    }
}

testOidcRecoveredWithNoDiscovery still succeeds. Hence, I think this is not a good choice for basing tenant selection on.

I'm not 100% sure what all of this is doing, but it seems that - no matter what - the OIDC provider is always http://localhost:8180/auth/realms/quarkus2/ (i.e. the fake Keycloak provided by WiremockTestResource), no matter which tenant is active.

I'll try and find another place where to put my test.

[sberyozkin]

@j-be The test does care it just that the default and this tenant are identical, which is a test weakness but it is not meant to be. So lets fix it and update it as proposed. In Keycloak test resource you can create another realm and update the tenant config accordingly to make sure the default config is not picked up

[sberyozkin]

@j-be

Yet, the test you chose seems to be one of the few which does not actually care which tenant is actually active

What would help if you'd instead open an issue and provide more details. Just stating this fact and suggesting to test this PR elsewhere is not the right approach IMHO

[j-be]

@sberyozkin I wasn't sure if it is a weakness in the test, or if I'm missing something and that very test doesn't really care. Anyway, will open a dedicated issue for that.

I already thought of creating a second realm in WiremockTestResource. But that would mean creating a new key JSON, like privateKey.jwt, right? I failed on finding any information on how to do that.

[sberyozkin]

@j-be I think you got me confused. In the test I've described what is tested is that no discovery is performed as well. When you return null it is the default config which enables the discovery is loaded - which does not refute the test. The weakness in the test is that there is no guarantee it was this tenant id which was loaded. So lets not worry about it for now, I'll take care of tightening it up later on, unless you prefer to tighten it alongside this PR, I don't mind

[sberyozkin]

@j-be No, the realms are created from the test factory using the API. (Ignore it please - I forgot this test uses Wiremock...)

But I see creating realms won't guarantee in the test itself that again that tenant was loaded.

What I do in a few other tests is that I echo back the tenant id - so perhaps RoutingContext can be also injected in the endpoint used by this tests and the tenant id can be reported back alongside the name... Something like that should do, can you try it please ?

[j-be]

@sberyozkin what I am saying is, that as far as I can tell within the module quarkus-integration-test-oidc-wiremock both the default OIDC tenant and the no-discovery OIDC tenant are indistinguishable. Hence, using the no-discovery OIDC tenant as it is is not a good choice for testing anything related to tenant selection.

I never meant to say it is an issue for the test as it is right now, nor did I mean to imply that the test is not doing what it is supposed to.

I'm still not sure what exactly is going on with the realms in that very test. I can see WiremockTestResource which differently from OidcWiremockTestResource seems to feature as single hardcoded realm? But again, not sure what exactly is going on here.

As for replying the tenant: that should work. This time reading static.tenant.id would actually be the right thing to do, right?

[sberyozkin]

@j-be Sorry, I think you are right that me suggesting to test the PR as I proposed would not really confirm this PR is effective.

I think what we need to test is that a correct tenant-id is picked up, we can simplify, there are tests around testing individual tenant configurations, this PR is not about it. So how about a slightly different plan:

• copy https://github.com/quarkusio/quarkus/blob/main/integration-tests/oidc-wiremock/src/test/java/io/quarkus/it/keycloak/BearerTokenAuthorizationTest.java#L32 and add /hr to /api/users/preferredUserName/bearer, and copy the endpoint which backs it up to have /hr/api/users/preferredUserName/bearer and also echo the tenant from there - and the tenant id should be hr - this will confirm the PR is correct

Does it work for you ?

I have to sign off now, thanks for your time and enjoy the weekend :-)

[j-be]

@sberyozkin Will try that when I find the time - need to leave now, too. Wish you a nice weekend, too.

[j-be]

@sberyozkin I can't seem to be able to reproduce the behavior I saw in Dev mode using a unit test. During the tests the @Interceptor always seems to trigger after DefaultTenantConfigResolver.

I created a new module on my branch (integration-tests/oidc-annotation-based-tenant) to avoid any weird side-effects from whatever is already in oidc-wiremock, but still the interceptor only triggers after DefaultTenantConfigResolver.

I'll need to dig further trying to find out what is going on here. I'll report back as soon as I found out what is going on.

[sberyozkin]

@j-be Sure, if keeping a separate module will help you focus on this problem better then it is fine; but in the end IMHO the test should be added to one of the existing test modules - it just feels the minor update with this PR does not warrant the introduction of a new integration tests module.
Please check if disabling the proactive authentication can make a difference, https://quarkus.io/guides/security-built-in-authentication#proactive-authentication. If it does and it only works with it disabled then I still think your idea would be of interest to Quarkus users (the only difference the proactive authentication adds is that if one sends an invalid token to a public resource this token will still be validated)...

Thanks

[j-be]

@sberyozkin I finally found some time to further analyze this. You are right, this hinges on quarkus.http.auth.proactive, which we had already disabled on our application for other reasons. But setting it to true (or removing the setting - true is default) breaks my approach.

For completeness:

• the order with quarkus.http.auth.proactive=false is:

  Interceptor -> TenantResolver (if any) -> DefaultTenantConfigResolver
  

• in all other cases it is:

  TenantResolver (if any) -> DefaultTenantConfigResolver -> Interceptor
  

I updated the documentation to reflect this. Due to the importance of the fact I chose to put it in a [NOTE] - I hope that's acceptable.

I also completed my integration-tests and moved them to integration-tests/oidc-wiremock, squashed all commits together and used the title of this PR as commit message, and I rebased to current main - because: why not.

If something is missing: let me know 😃

[quarkus-bot]

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 885b3bc

Status	Name	Step	Failures	Logs	Raw logs
✖	Initial JDK 11 Build	Build	Failures	Logs	Raw logs

Failures

⚙️ Initial JDK 11 Build #

- Failing: integration-tests/oidc-wiremock 

📦 integration-tests/oidc-wiremock

✖ Failed to execute goal net.revelc.code:impsort-maven-plugin:1.6.2:check (check-imports) on project quarkus-integration-test-oidc-wiremock: Imports are not sorted in /home/runner/work/quarkus/quarkus/integration-tests/oidc-wiremock/src/test/java/io/quarkus/it/keycloak/AnnotationBasedTenantTest.java

[sberyozkin]

@j-be Nice work, thanks for finding the time to complete this PR, can you please format the oidc-wiremock sources and squash again.
I wonder if we should consider disabling the proactive authentication by default in the future. @FroMage, also FYI, may be related to your @OidcError idea

[j-be]

@sberyozkin My pleasure. Sorry for the import ordering - not sure how that slipped by.

[sberyozkin]

@j-be Thanks, no problems re the build failure, I'm causing too many build failures myself (even though every time I thought I checked everything :-) )

[sberyozkin]

@FroMage By the way, besides @OidcError, you might want to experiment with the annotation based approach for selecting the tenants as well, see this PR's docs

[FroMage]

Sorry I'm late, but I don't understand why we document an API that might be useful, instead of providing that API.

I'd have made it @OidcTenant("google") myself, too, so that it's generic. But I really don't understand why we go an tell users what to write instead of providing that solution ourselves.

[j-be]

@FroMage That was one of my proposals - see #22974.

From what I saw that would have been a major change though, as it seems there is no way to easily evaluate annotation values - at least I wasn't able to find any. Hence, my approach uses one annotation per tenant, which is not the nicest, but it works quite well and with very little code changes.

I agree that this doesn't scale very well with the number of tenants, but for a handful of tenants it is imho. fine. And it avoids having to copy (or refactor) the code handling e.g. @CacheInvalidateAll's cacheName, which seems to do exactly that. But that is quite a beast.

If there is interest I'd be happy to have a go at that.