Support specifying multiple tenants in @TenantFeature

[Eng-Fouad]

• Resolves: Support specifying multiple tenants in @TenantFeature #40358

[sberyozkin]

@Eng-Fouad Thanks, I thought the plan was only to have an array to let users list more than one tenant name, I'm not sure about the Repeatable approach. We already have an array for example here: https://github.com/quarkusio/quarkus/blob/main/extensions/oidc-common/runtime/src/main/java/io/quarkus/oidc/common/OidcEndpoint.java#L51

[Eng-Fouad]

@Eng-Fouad Thanks, I thought the plan was only to have an array to let users list more than one tenant name, I'm not sure about the Repeatable approach. We already have an array for example here: https://github.com/quarkusio/quarkus/blob/main/extensions/oidc-common/runtime/src/main/java/io/quarkus/oidc/common/OidcEndpoint.java#L51

AFAIK, if we use array member (array of tenants) within the qualifier @TenantFeature, we wouldn't be able to look up CDI beans using a single tenant. In other words,

String tenantId = "tenant1";
return Arc.container()
          .instance(tenantFeatureClass, TenantFeature.TenantFeatureLiteral.of(tenantId))
          .get()

will not find/match the following feature class:

@TenantFeature({"tenant1", "tenant2"})
@ApplicationScoped
public class CustomValidator implements Validator {
    @Override
    public String validate(JwtContext jwtContext) throws MalformedClaimException {
        return null; // TODO
    }
}

it must match the full array:

String[] tenantIds = {"tenant1", "tenant2"};
return Arc.container()
          .instance(tenantFeatureClass, TenantFeature.TenantFeatureLiteral.of(tenantIds))
          .get()

that's why I used an alternative approach which is a repeating qualifier, and fortunately it works without any further modifications (I believe) in TenantFeatureFinder:

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

Lines 38 to 40 in 6ce0c8a

	return container
	.instance(TokenCustomizer.class, TenantFeature.TenantFeatureLiteral.of(oidcConfig.tenantId.get()))
	.get();

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

Lines 54 to 55 in 6ce0c8a

	for (var instance : Arc.container().listAll(tenantFeatureClass,
	TenantFeatureLiteral.of(oidcTenantConfig.tenantId.get()))) {

Btw, @OidcEndpoint is not a CDI qualifier, so it is not the same case.

---

Update:

After reviewing how @OidcEndpoint is queried:

quarkus/extensions/oidc-common/runtime/src/main/java/io/quarkus/oidc/common/runtime/OidcCommonUtils.java

Lines 512 to 530 in 6ce0c8a

	public static Map<OidcEndpoint.Type, List<OidcRequestFilter>> getOidcRequestFilters() {
	ArcContainer container = Arc.container();
	if (container != null) {
	Map<OidcEndpoint.Type, List<OidcRequestFilter>> map = new HashMap<>();
	for (OidcRequestFilter filter : container.listAll(OidcRequestFilter.class).stream().map(handle -> handle.get())
	.collect(Collectors.toList())) {
	OidcEndpoint endpoint = ClientProxy.unwrap(filter).getClass().getAnnotation(OidcEndpoint.class);
	if (endpoint != null) {
	for (OidcEndpoint.Type type : endpoint.value()) {
	map.computeIfAbsent(type, k -> new ArrayList<OidcRequestFilter>()).add(filter);
	}
	} else {
	map.computeIfAbsent(OidcEndpoint.Type.ALL, k -> new ArrayList<OidcRequestFilter>()).add(filter);
	}
	}
	return map;
	}
	return Map.of();
	}

Indeed, we can do the same thing for @TenantFeature, however we need to:

• Remove @Qualifier annotation from @TenantFeature, as we won't need it anymore.
• Change TenantFeatureFinder implementation by replacing query CDI beans by tenantFeatureClass and qualifier with query all CDI beans by tenantFeatureClass then filter them by which beans has @TenantFeature that has the specified tenant in its array member.

WDYT?

[sberyozkin]

@Eng-Fouad
Thanks for the analysis. As far as I recall the way you propose to fix it to support the array format, is how it used to work awhile back (though indeed, back then, a single value was checked).

I'm OK with doing it how you proposed it, the main reason, is that I'd like to avoid a mix of 2 different styles. For example, we had one user asking to provide a tenant id to OidcRequestFilter, and indeed it is available as one of the context map parameters passed to the OidcRequestFilter filter method. And I've been thinking, would be it be great to have @TenantFeature("my tenant") attached to OidcRequestFilter if needed, but note, OidcRequestFilter can have the endpoint annotation which uses an array to list several endpoints. So if we have more than one tenant to which this filter must apply with several endpoints, we'll have a mix of 2 styles.

@michalvavrik Do you have a strong reason not do it ? Asking since you preferred the Qualifier approach

[michalvavrik]

I know about this PR and issue and I intentionally didn't comment yet as I said in past we should refactor @TenantFeature implementation because IMO optimization would help. I proposed it in past in one of my PRs I think, but there was suggestion to postpone the refactoring back then. Build-time approach wouldn't mind changing string to array as it's just Jandex job.

@michalvavrik Do you have a strong reason not do it ? Asking since you preferred the Qualifier approach

I thought it was quicker for that previous use case, so I refactored your code back then. Now that you need to add more values, you can't use the qualifier. Just iterate over the list in TenantFeatureFinder.java, it's not perfect anyway.

[michalvavrik]

So let's drop the annotation literal @Eng-Fouad .

[sberyozkin]

OK, thanks @michalvavrik, and indeed, when you get a chance, more refactoring can be done to improve things

[gsmet]

FWIW, you can also use the option of having an AnnotationsTransformer to rewrite the public facing annotation to something more suitable to CDI.

Not saying it's the way to go but I wanted to mention the option.

[sberyozkin]

Thanks @gsmet, this is a nice feature indeed.

@michalvavrik Is is something you'd like to consider at the later stage, use the transformer to support an array property indirectly at the CDI level, with the Qualifier, etc, and for now, the plan proposed by @Eng-Fouad stands ? I'm not sure how much do we want to optimize in this case, as it is a rather case where we may have several OIDC providers and a feature applies only to 2 or more, but not all of them :-)

[michalvavrik]

@sberyozkin I agree it is nice, but I suppose we should leave it to @Eng-Fouad what he comes with. FWIW the easiest is to do what OidcEndpoint as it's almost same thing.

Personally, I think we know which OIDC tenant has which feature at the build time. And when it has no feature so that there is no point looking during (which is main case), so I think we should just collect it during the build time. I had it implemented in one of my commits that got eventually dropped. I think gain is very small, but it would allow us to reuse same approach for the endpoint as well. I'll propose such changes in a PR one day.

[sberyozkin]

Sounds good @michalvavrik, sure, let @Eng-Fouad proceed with the current plan for now and then you can review if it will be worth optimizing further, thanks

[Eng-Fouad]

@sberyozkin @michalvavrik @gsmet Updated the PR to use the same approach as what it is applied to @OidcEndpoint.

[Eng-Fouad]

I thought I had to change TenantFeatureFinder only. I didn't notice this field injection with @TenantFeature as a qualifier:

quarkus/integration-tests/oidc-wiremock/src/main/java/io/quarkus/it/keycloak/OrderService.java

Lines 24 to 26 in 49daec1

	@TenantFeature("bearer")
	@Inject
	TenantIdentityProvider identityProvider;

Also, is the following a CDI injection? I don't see @Inject:

quarkus/integration-tests/oidc-wiremock/src/main/java/io/quarkus/it/keycloak/StartupService.java

Lines 30 to 40 in 49daec1

	@TenantFeature("bearer")
	TenantIdentityProvider identityProviderBearer;
	@TenantFeature("bearer-role-claim-path")
	TenantIdentityProvider identityProviderBearerRoleClaimPath;
	private final Map<String, Map<String, Set<String>>> tenantToIdentityWithRole = new ConcurrentHashMap<>();
	void onStartup(@Observes StartupEvent event,
	@TenantFeature(DEFAULT_TENANT_ID) TenantIdentityProvider defaultTenantProvider,
	TenantIdentityProvider defaultTenantProviderDefaultQualifier) {

I am not sure how to change the following with @TenantFeature being not a qualifier:

quarkus/extensions/oidc/deployment/src/main/java/io/quarkus/oidc/deployment/OidcBuildStep.java

Lines 150 to 160 in 49daec1

	@BuildStep
	QualifierRegistrarBuildItem addQualifiers() {
	// this seems to be necessary; I think it's because sometimes we only access beans
	// annotated with @TenantFeature programmatically and no injection point is annotated with it
	return new QualifierRegistrarBuildItem(new QualifierRegistrar() {
	@Override
	public Map<DotName, Set<String>> getAdditionalQualifiers() {
	return Map.of(TENANT_FEATURE_NAME, Set.of());
	}
	});
	}

quarkus/extensions/oidc/deployment/src/main/java/io/quarkus/oidc/deployment/OidcBuildStep.java

Lines 173 to 212 in 49daec1

	@Record(ExecutionTime.STATIC_INIT)
	@BuildStep
	void produceTenantIdentityProviders(BuildProducer<SyntheticBeanBuildItem> syntheticBeanProducer,
	OidcRecorder recorder, BeanDiscoveryFinishedBuildItem beans, CombinedIndexBuildItem combinedIndex) {
	// create TenantIdentityProviders for tenants selected with @TenantFeature like: @TenantFeature("my-tenant")
	if (!combinedIndex.getIndex().getAnnotations(TENANT_FEATURE_NAME).isEmpty()) {
	// create TenantIdentityProviders for tenants selected with @TenantFeature like: @TenantFeature("my-tenant")
	beans
	.getInjectionPoints()
	.stream()
	.filter(ip -> ip.getRequiredQualifier(TENANT_FEATURE_NAME) != null)
	.filter(OidcBuildStep::isTenantIdentityProviderType)
	.map(ip -> ip.getRequiredQualifier(TENANT_FEATURE_NAME).value().asString())
	.distinct()
	.forEach(tenantName -> syntheticBeanProducer.produce(
	SyntheticBeanBuildItem
	.configure(TenantIdentityProvider.class)
	.addQualifier().annotation(TENANT_FEATURE_NAME).addValue("value", tenantName).done()
	.scope(APPLICATION.getInfo())
	.supplier(recorder.createTenantIdentityProvider(tenantName))
	.unremovable()
	.done()));
	}
	// create TenantIdentityProvider for default tenant when tenant is not explicitly selected via @TenantFeature
	boolean createTenantIdentityProviderForDefaultTenant = beans
	.getInjectionPoints()
	.stream()
	.filter(InjectionPointInfo::hasDefaultedQualifier)
	.anyMatch(OidcBuildStep::isTenantIdentityProviderType);
	if (createTenantIdentityProviderForDefaultTenant) {
	syntheticBeanProducer.produce(
	SyntheticBeanBuildItem
	.configure(TenantIdentityProvider.class)
	.addQualifier(DEFAULT)
	.scope(APPLICATION.getInfo())
	.supplier(recorder.createTenantIdentityProvider(DEFAULT_TENANT_ID))
	.unremovable()
	.done());
	}
	}

[Eng-Fouad]

Based on suggestion by @gsmet, is this a correct implementation?

// Replace
//
// @TenantFeature({"a", "b"})
//
// with
//
// @TenantFeature("a")
// @TenantFeature("b")
@BuildStep
void tenantFeatureAnnotationTransformer(BuildProducer<AnnotationsTransformerBuildItem> annotationsTransformer) {
    annotationsTransformer.produce(new AnnotationsTransformerBuildItem(context -> {
        AnnotationInstance annotationInstance = context.getTarget().declaredAnnotation(TENANT_FEATURE_NAME);
        if (annotationInstance != null) {
            String[] tenants = annotationInstance.value().asStringArray();
            if (tenants != null && tenants.length > 1) {
                Transformation transformation = context.transform();
                for (String tenant : tenants) {
                    transformation.removeAll();
                    transformation.add(AnnotationInstance.create(TENANT_FEATURE_NAME, context.getTarget(),
                            new AnnotationValue[] { AnnotationValue.createArrayValue("value",
                                    new AnnotationValue[] { AnnotationValue.createStringValue("", tenant) }) }));
                }
                transformation.done();
            }
        }
    }));
}

Supposing @TenantFeature is as a repeating qualifier, and its value is an array.

[sberyozkin]

Thanks @Eng-Fouad, I'd keep the same approach as for the OidcRequestFilter.

As far as

 @TenantFeature("bearer-role-claim-path") 
 TenantIdentityProvider identityProviderBearerRoleClaimPath; 

is concerned, I think we can look at it as either a test or implementation bug. It really must be

 @Tenant("bearer-role-claim-path") 
 TenantIdentityProvider identityProviderBearerRoleClaimPath; 

The idea here is to let TenantIdentityProvider know which tenant configuration to use to for resolving the identity out of band. This is what @Tenant is about.
@TenantFeature is about deciding which tenant is impacted by a given feature, so using it with TenantIdentityProvider is a mistake.
If we already recommend it in the docs, then we can fix it anyway, I'd not even worry marking it as a breaking change, as it is a bug, though I guess it can't be avoided. Michal, my proposal is to fix it first to use @Tenant and then let @Eng-Fouad pick up that fix, I can handle it if you agree, let me know please, thanks

[michalvavrik]

Also, is the following a CDI injection? I don't see @Inject
I am not sure how to change the following with @TenantFeature being not a qualifier:

I made TenantFeature a qualifier for reasons that are explained in your link above, in the buildstep comments (see your link). We document this injection with @Inject inside the documentation https://quarkus.io/guides/security-oidc-bearer-token-authentication#authentication-after-an-http-request-has-completed. Whether the @Inject is required goes down to the CDI and not to the OIDC.

The idea here is to let TenantIdentityProvider know which tenant configuration to use to for resolving the identity out of band. This is what @Tenant is about.

I'll provide context, I do not imply it is correctly implemented, just trying to explain the situation:

The PR description says that it uses @TenantFeature and it is in a code and documentation and tests from the start: #36631.

Javadoc of the TenantFeature https://github.com/quarkusio/quarkus/blob/49daec12ad75fa214e5aea2727caf5087a567e37/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/TenantFeature.java says Qualifier used to specify which named tenant is associated with one or more OIDC feature.. In case of the TenantIdentityProvider, we use the @TenantFeature annotation to decide tenant configuration. Javadoc of this class says it here https://github.com/quarkusio/quarkus/blob/main/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/TenantIdentityProvider.java#L7.

If we already recommend it in the docs, then we can fix it anyway, I'd not even worry marking it as a breaking change, as it is a bug

I'll try to explain why I used @TenantFeature without implying it is right or not:

My understanding of the TenantIdentityProvider is that the @TenantFeature is used to determine which tenant config context is used. You are right it selects tenant. The @TenantFeature instance placed on TokenCustomizer is also selecting the tenant. Difference is whether something is a feature. Here my thinking was:

• Associating specific tenant configuration with a provider
• Associating specific tenant with a customizer.

I respect your arguments. I'll leave this for you to decide.

Michal, my proposal is to fix it first and then let @Eng-Fouad pick up that fix, I can handle it if you agree, let me know please, thanks

I am very sorry. I have limited personal time for Quarkus. I only submit very small PRs for fixes or things I have in progress for a long time like certificate role mapping. Please handle both this review and whatever fix you deem necessary. Thank you.

[sberyozkin]

@michalvavrik Michal, thanks for the clarifications, and by the way, I reviewed your PR, so if anyone is to bear some kind of a blame then it can only be me, no doubt about it. You made the important thing happen, TenantIdentityProvider, I should've spotted what I believe is the issue during the review.

IMHO we need to have a very clear border between @Tenant and @TenantFeature and avoid any ambiguities at all costs. The reason using @TenantFeature with TenantIdentityProvider was a mistake is actually highlighted by this current PR, because @TenantFeature("tenant-a", "tenant-b") TenantIdentityProvider does not work. Which is why we also can't follow a transformation idea because that would mean we'd have to imagine how even @TenantFeature("tenant-a", "tenant-b") TenantIdentityProvider works in that case which is impossible: there could be a single tenant at a time which can secure an endpoint or verify a token for a current request.

Also, you don't have to say sorry, you have invested a lot of your personal time into Quarkus, which is very appreciated. Thank you

[sberyozkin]

@Eng-Fouad Can you please try to complete this PR ? It should become possible now with thanks to Michal going ahead with #40843

[Eng-Fouad]

@Eng-Fouad Can you please try to complete this PR ? It should become possible now with thanks to Michal going ahead with #40843

Merged main branch into the current feature branch. Could you please approve the workflow to start? Thanks.

[sberyozkin]

Sorry @Eng-Fouad I've missed your update, let me do it, I'll follow up with a test update, but indeed, I'd like to see how it builds on main CI while it is quite quiet

[sberyozkin]

@Eng-Fouad Can you please drop https://github.com/quarkusio/quarkus/blob/main/extensions/oidc/deployment/src/main/java/io/quarkus/oidc/deployment/OidcBuildStep.java#L160 as well, I think this is what Michal meant

[Eng-Fouad]

@Eng-Fouad Can you please drop https://github.com/quarkusio/quarkus/blob/main/extensions/oidc/deployment/src/main/java/io/quarkus/oidc/deployment/OidcBuildStep.java#L160 as well, I think this is what Michal meant

Done.

[sberyozkin]

@Eng-Fouad I'll look into adding a test, thanks

[sberyozkin]

I had problems adding a test, eventually, I found that without that Qualifier build item, ArcContainer can be null, I thought AcrContainer would always be non-null. So I had to restore that build item for now. And then I also could not make the test passing for a while as a 2nd Arc.container call would return null in a tenant feature finder.

Michal, if you have some ideas why keeping the Qualifier build item is required, even though TenantFeature itself is no longer a qualifier, let us know please. I suspect it indirectly ensures Arc is initialized eagerly, which is fine if it is what what is required...

[michalvavrik]

Michal, if you have some ideas why keeping the Qualifier build item is required, even though TenantFeature itself is no longer a qualifier, let us know please. I suspect it indirectly ensures Arc is initialized eagerly, which is fine if it is what what is required...

I'll try it tomorrow evening and let you know if I can see why it is happening. I don't think it's a sensible behavior.

[sberyozkin]

@michalvavrik Hi, please don't spend time on it for now, it may have been something in my setup, I'll retry a bit later and ping you if that build item is still needed. Thanks

[michalvavrik]

@michalvavrik Hi, please don't spend time on it for now, it may have been something in my setup, I'll retry a bit later and ping you if that build item is still needed. Thanks

deal

[sberyozkin]

@Eng-Fouad I've tweaked it to support a list for TokenCustomizer, as is done for all other features. Should be ready to go

[sberyozkin]

@Eng-Fouad Can you please squash ? I tried earlier and it did not go well after an additional rebase from upstream, not sure why.

@michalvavrik, this is totally not urgent, but that build item qualifying TenantFeature as a Qualifier still makes a difference, removing it causes oidc-wiremock test fail. I don't see why though since TenantFeature is no longer requires it.

Please have a quick look in the next few weeks, cheers

[Eng-Fouad]

@Eng-Fouad Can you please squash ? I tried earlier and it did not go well after an additional rebase from upstream, not sure why.

Done.

[quarkus-bot]

Status for workflow Quarkus CI

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

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

[michalvavrik]

Hello @Eng-Fouad @sberyozkin ,

I had a look why you cannot remove a build item that makes @TenantFeature qualifier. The reason is that if bean has no other qualifiers, it is going to be @jakarta.enterprise.inject.Default() and @jakarta.enterprise.inject.Any(). Therefore when you do container.listAll(tenantFeatureClass, Default.Literal.INSTANCE) you find it even though it is annotated with @TenantFeature.

More specifically, the oidc-wiremock will fails as the https://github.com/quarkusio/quarkus/blob/main/integration-tests/oidc-wiremock/src/main/java/io/quarkus/it/keycloak/BearerTenantTokenChainValidator.java is registered to other tenants as well.

It's not a bug, it makes quite a sense. Personally, I think you should list all the beans of that type (no qualifiers specified) and then add these beans without annotation to global ones and add beans with annotation only if the value matches the tenant name.

Cheers

[sberyozkin]

Hi Michal, @michalvavrik thanks,

I did not quite get what you explained above,

More specifically, the oidc-wiremock will fails as the https://github.com/quarkusio/quarkus/blob/main/integration-tests/oidc-wiremock/src/main/java/io/quarkus/it/keycloak/BearerTenantTokenChainValidator.java is registered to other tenants as well.

How do you mean, it is only supposed to work with the bearer-chain-custom-validator bean.

It's not a bug, it makes quite a sense. Personally, I think you should list all the beans of that type (no qualifiers specified) and then add these beans without annotation to global ones and add beans with annotation only if the value matches the tenant name.

Can you clarify it please ?

[michalvavrik]

How do you mean, it is only supposed to work with the bearer-chain-custom-validator bean.

@sberyozkin when you drop @TenantFeature as a qualifier you are applying https://github.com/quarkusio/quarkus/blob/main/integration-tests/oidc-wiremock/src/main/java/io/quarkus/it/keycloak/BearerTenantTokenChainValidator.java to the tenant that are not bearer-chain-custom-validator because container.listAll(tenantFeatureClass, Default.Literal.INSTANCE) returns BearerTenantTokenChainValidator.

Can you clarify it please ?

https://jakarta.ee/specifications/cdi/4.0/jakarta-cdi-spec-4.0.html#builtin_qualifiers says:

• Every bean has the built-in qualifier @Any, even if it does not explicitly declare this qualifier.
• If a bean does not explicitly declare a qualifier other than @Named or @Any, the bean has exactly one additional qualifier, of type @Default

So yeah, this is expected behavior, isn't it?