OIDC: Encapsulate static/dynamic tenants maps in `TenantConfigBean` #43007

snazy · 2024-09-04T10:08:51Z

Lets TenantConfigBean be the sole "owner" of the static/dynamic tenants maps, adds/changes accessor methods for tenants. Also introduces a functional interface to create tenants.

No functional change, only moving code around.

Also some minor code cleanups (reducing the amount of warnings in IntelliJ).

quarkus-bot · 2024-09-04T10:09:11Z

Thanks for your pull request!

Your pull request does not follow our editorial rules. Could you have a look?

description should not be empty, describe your intent or provide links to the issues this PR is fixing (using Fixes #NNNNN) or changelogs

_{This message is automatically generated by a bot.}

github-actions · 2024-09-04T11:15:18Z

🎊 PR Preview 47db797 has been successfully built and deployed to https://quarkus-pr-main-43007-preview.surge.sh/version/main/guides/

Images of blog posts older than 3 months are not available.
Newsletters older than 3 months are not available.

sberyozkin · 2024-09-04T17:16:14Z

@snazy Thanks, can you fix the formatting please, I'll have a look tomorrow once I finish another PR

snazy · 2024-09-04T19:10:02Z

@snazy Thanks, can you fix the formatting please, I'll have a look tomorrow once I finish another PR

Sure, fixed

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

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

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

sberyozkin · 2024-09-05T08:57:04Z

@snazy Thanks, it all looks good, refactoring makes sense and makes things a bit clearer. Michal, myself, added minor comments, but something did not work out during the initial refactoring, tests are impacted

snazy · 2024-09-05T12:58:57Z

The issue that's fixed now is that TenantConfigContexts for static tenants with a null provider were "leaked", causing the NPEs. Also updated the code to make things a bit clearer/leaner.

snazy · 2024-09-05T14:59:38Z

Um - let me look into those new test failures

snazy · 2024-09-05T15:31:30Z

Hm - the tests in those projects don't fail locally

michalvavrik · 2024-09-05T16:50:55Z

Hm - the tests in those projects don't fail locally

Did you declare you have Linux containers available locally with -Dtest-containers -Dstart-containers appended to your mvn clean ....?

snazy · 2024-09-05T16:53:22Z

Hm - the tests in those projects don't fail locally

Did you declare you have Linux containers available locally with -Dtest-containers -Dstart-containers appended to your mvn clean ....?

Yup - just found those params in the GH workflow

sberyozkin · 2024-09-05T16:58:28Z

@snazy One thing I know is that even a single line update can have an impact there, a lot is going on with the tenant resolution, and this is why, having a bit of clean up and adding extra clarity would not be bad at all. It will help going forward, thanks for working on this PR.
But I wonder, would it make sense to do a series of really small PRs for us to have a better control of these changes ?
For example:

PR1: get static tenants created at the first request time recorded in the static map
PR2: Consolidate the code which is really belonging to the tenant bean in that bean

snazy · 2024-09-05T17:21:46Z

Yea - something is quite broken with this change. Let me start from scratch.

sberyozkin · 2024-09-05T17:41:43Z

Thanks @snazy, yeah, I think the introduction of the functional interface is nice, and making sure static tenants are only recorded in the static map, is worth a dedicated PR of its own, so if you are OK with it, please do it first, and then complete the preparation with the 2nd PR focusing on the rest of your refactoring here

snazy · 2024-09-07T08:56:52Z

Marked as a draft PR for now.

snazy · 2024-09-07T09:41:41Z

Ah! I found out why the PR fails in the CodeFlowTest: I've changed the order of the dynamic/static tenant checks in i.q.oidc.runtime.DefaultTenantConfigResolver#resolveConfig, which broke the tests. Which means, the refactoring was broken in the sense that static tenants still leaked into the dynamic map. Anyway - not a problem, as I'm going to change this PR.

snazy · 2024-09-07T09:48:19Z

Updated the PR entirely, focusing on moving the tenants-maps into the TenantConfigBean and not let these maps escape.

snazy · 2024-09-07T09:50:49Z

Note: OidcClientRegistrationTest.testRegisteredClientDynamicTenant in integration-tests/oidc-client-registration/ fails on main as well.

Opened #43106

sberyozkin · 2024-09-07T11:36:51Z

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

-    public Map<String, TenantConfigContext> getStaticTenantsConfig() {
-        return staticTenantsConfig;
+    public Uni<TenantConfigContext> createTenantContext(OidcTenantConfig oidcConfig, boolean dynamicTenant) {
+        if (oidcConfig.logout.backchannel.path.isPresent()) {


dynamicTenant is not checked yet

Static tenants still escape into the dynamicTenants map in this PR.
I've opened #43110 to let static tenants not escape - tests should pass on 43110 now - the actual change wasn't that big - could actually sneak it back into this PR. WDYT?

sberyozkin · 2024-09-07T11:39:08Z

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

                                if (tenantContext != null) {
                                    // Dynamic map may contain the static contexts initialized on demand,
-                                    if (tenantConfigBean.getStaticTenantsConfig().containsKey(tenantId)) {
+                                    if (tenantConfigBean.getStaticTenant(tenantId) != null) {


Please remove the comment above since it is no longer relevant

#43110 removes it (the comment's still valid for this PR)

sberyozkin · 2024-09-07T11:55:09Z

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

-        this.staticTenantsConfig = staticTenantsConfig;
-        this.dynamicTenantsConfig = dynamicTenantsConfig;
+            TenantContextFactory tenantContextFactory) {
+        this.staticTenantsConfig = new ConcurrentHashMap<>(staticTenantsConfig);


It is not clear to me why it is now a concurrent map, given the dynamicTenant in the function below is not checked

#43110 should make it clear

Lets `TenantConfigBean` be the sole "owner" of the static/dynamic tenants maps, adds/changes accessor methods for tenants. Also introduces a functional interface to create tenants. No functional change, only moving code around. Also some minor code cleanups (reducing the amount of warnings in IntelliJ).

quarkus-bot · 2024-09-08T13:02:29Z

Status for workflow `Quarkus Documentation CI`

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

✅ 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.

sberyozkin · 2024-09-24T15:01:02Z

Hi @snazy Can you please pickup the latest updates and rework this PR next week or so, to have access to the dynamic (and static) map encapsulated inside TenantConfigBean, and replacing an explicit Function reference with an interface;

Now that the static tenant does not leak into the dynamic map anymore after a fix from @michalvavrik, the PR should become much simpler,

Let me now please if you won't have much time, thanks.

snazy · 2024-09-30T09:29:19Z

Closed in favor of #43590

quarkus-bot bot added the area/oidc label Sep 4, 2024

snazy mentioned this pull request Sep 4, 2024

OIDC: Limit number of dynamic tenants #42804

Draft