Always set OIDC tenant id and document how to access it in Hibernate and Mongo resolvers

[michalvavrik]

• closes: Unable to read RoutingContext from CustomTenantResolver #44168

[michalvavrik]

\cc @yrodiere in case you want to review, thanks

[github-actions]

🙈 The PR is closed and the preview is expired.

[sberyozkin]

Michal, thanks, I'm pretty sure we have a note about it in one of the Quarkus Hibernate docs.

As far as setting tenant id is concerned, OIDC sets it itself when the session cookie exists, and @TenantID resolver sets it too.
We should probably have it set in DefaultTenantConfigResolver if it is not already set.
IMHO it will be better

[michalvavrik]

Michal, thanks, I'm pretty sure we have a note about it in one of the Quarkus Hibernate docs.

I can see notes in Hibernate docs and Mongo docs, however CDI request context activation in auth should concern authentication. I am just completing here incomplete existing documentation. Are you sure you want to duplicate this inside Hibernate (and Mongo) about proactive authentication?

As far as setting tenant id is concerned, OIDC sets it itself when the session cookie exists, and @TenantID resolver sets it too. We should probably have it set in DefaultTenantConfigResolver if it is not already set. IMHO it will be better

I don't mind, but then maybe drop the existing example? Please look a few lines above the text I am adding.

Anyway that is really not a point of change here. Let me try to summarize what I try to document here, maybe I put it wrong into the words? I try to say:

• you want to use RoutingContext, so if you use @ActivateRequestContext and the CDI request context had not been active already, you will get CDI request context without RoutingContext (because how exactly woudl it get there without us setting it)
  • the easiest way to work around that is to use lazy auth and standard security annotations or JAX-RS HTTP perms
• if you call db from scheduler, you cannot inject RoutingContext, please inject CurrentVertxRequest instead

In another words, whether you set it the tenant id in the DefaultTenantConfigResolver or not, this will still be true and users need to understand it if they want to get this tenant id from the RoutingContext.

[michalvavrik]

@sberyozkin if you meant that I am putting this information into a wrong place or that I should duplicate it into Hibernate and Mongo guides, np, I am just not sure if I understood you, so I'll wait for a response.

[sberyozkin]

Sorry for the confusion @michalvavrik, indeed, what I thought was available in the Hibernate docs is indeed sitting just above the Hibernate TenantResolver example you added in this PR :-), where I show the custom OIDC resolver propagating tenant id via a routing context attribute.

But what your PR highlights is that code example I added earlier is outdated, the OIDC multi-tenancy docs should not ask users to propagate OIDC tenant id like I did in that example, instead, Quarkus OIDC should do it itself by setting a tenant-id attribute (when it is not already set), and then the example you add would show that a tenant-id attribute should be retrieved, not tenantId as shown in my outdated example...

[michalvavrik]

@sberyozkin what I try to show is important for users if they want to get the OIDC tenant id inside of the Hibernate tenant resolver. Should I rewrite that and move this information to https://quarkus.io/guides/hibernate-orm#writing-the-application ? And maybe drop that original example with link to the Hibernate one?

[sberyozkin]

@michalvavrik I support this PR, it just depends on the outdated OIDC resolution example, which sets a custom tenantId attribute while Quarkus OIDC already manages it with the tenant-id attribute.
So what I propose is to make sure tenant-id is always set, remove the outdated oidc resolver example, and update your PR to show that tenant-id is read

[michalvavrik]

np, I'll deal with it tomorrow.

[michalvavrik]

@sberyozkin regarding the scheduler scenario (when there is no active CDI request context), I think it can be easily handled by changing RoutingContext injection point inside of the OidcSessionImpl to the CurrentVertxRequest. I'll follow up in a separate PR when I have more time to write a test for that. Cheers.

[sberyozkin]

@michalvavrik

regarding the scheduler scenario (when there is no active CDI request context), I think it can be easily handled by changing RoutingContext injection point inside of the OidcSessionImpl to the CurrentVertxRequest

I'm sorry, I'm not sure why the scheduler scenario is introduced, I certainly did not raise it. The example with OidcSession does not work with bearer access tokens and will confuse users. IMHO your original proposed Doc update was better, and there I only asked to align it with the current status quo, where Quarkus OIDC already sets tenant-id, and Hibernate resolvers checking it, as opposed to us asking users to set some other custom tenantId attribute, duplicating what Quarkus OIDC already does...

When I said we need to make sure tenant id is always set, I did not mean scheduled scenarios. I'm only not 100% sure tenant-id is set by OIDC DefaultTenantResolverBean in all the cases, which is what I meant to be double checked.

Sorry for the confusion, IMHO we need to avoid referring to OidcSession

[sberyozkin]

It does not work with bearer access tokens

[michalvavrik]

I am probably missing something, I 'll wait for answer below before I will address this comment. Thanks

[michalvavrik]

I'm sorry, I'm not sure why the scheduler scenario is introduced, I certainly did not raise it.

User tried to access RoutingContext for DB queries from scheduler: #44168. I say "we should avoid exception, let's return null for tenant id in such case".

The example with OidcSession does not work with bearer access tokens and will confuse users. IMHO your original proposed Doc update was better, and there I only asked to align it with the current status quo, where Quarkus OIDC already sets tenant-id

Do you say that tenant-id is set by OIDC and user should get it? Why should users work manually with some attribute on RoutingContext when OIDC retrieves it completely same?

quarkus/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcSessionImpl.java

Line 33 in 1e7e874

return routingContext.get(OidcUtils.TENANT_ID_ATTRIBUTE);

When I said we need to make sure tenant id is always set, I did not mean scheduled scenarios. I'm only not 100% sure tenant-id is set by OIDC DefaultTenantResolverBean in all the cases, which is what I meant to be double checked.

It isn't, and I did double check. However it didn't make sense to me, there is OIdcSession where user can get tenant id and it doesn't work? I thought I was missing something.

[michalvavrik]

Sorry for the confusion, IMHO we need to avoid referring to OidcSession

Ok.

[sberyozkin]

Hi Michal, I was just thinking, so https://github.com/quarkusio/quarkus/blob/main/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcSessionImpl.java#L33 works, but it is a wrong concept on the bearer access token path so IMHO we should indeed avoid recommending it as a general tenant id check mechanism... I'll probably need to tighten its implementation to throw illegal access exception when some of its methods are called if no ID token is available...

In any case sorry if I was not clear, if you'd like, I can look at your original PR update and tune it a bit as proposed to save you some time...

[michalvavrik]

I misunderstood OidcSession, I thought it was nice CDI bean that allows to avoid dealing with RoutingContext manually, but it is only meant for things related to this OIDC session https://openid.net/specs/openid-connect-session-1_0.html#CreatingUpdatingSessions. Sorry, I'll deal with fixes later today.

[michalvavrik]

So I have done whatever I understood from @sberyozkin comment. Sorry, but this may take few iterations. Also I don't have time to run all related tests locally as I have to go (run just few modules), so I'll just trigger CI and look at potential failures.

[michalvavrik]

@yrodiere I have requested review from you because I don't have permissions to just dismiss your review. I don't think it makes sense for you to review ATM because this might take a while. Thanks that you looked before.

[yrodiere]

Commenting to finish the dismissing of my review :)
Agreed, it's more about OIDC than Hibernate ORM in the end, so I'm not much help here.

Not relevant anymore.

[quarkus-bot]

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit df602e0.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit df602e0.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

[sberyozkin]

Thanks @yrodiere

[sberyozkin]

Thanks @michalvavrik