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

Support specifying multiple tenants in @TenantFeature #40525

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Eng-Fouad
Copy link
Contributor

@Eng-Fouad Eng-Fouad commented May 8, 2024

@quarkus-bot quarkus-bot bot added the area/oidc label May 8, 2024
@sberyozkin
Copy link
Member

@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
Copy link
Contributor Author

Eng-Fouad commented May 9, 2024

@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:

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

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:

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
Copy link
Member

@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
Copy link
Contributor

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
Copy link
Contributor

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

@sberyozkin
Copy link
Member

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

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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
Copy link
Contributor

michalvavrik commented May 10, 2024

@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
Copy link
Member

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 Eng-Fouad changed the title Make @TenantFeature a repeating qualifier Support specifying multiple tenants in @TenantFeature May 10, 2024
@Eng-Fouad
Copy link
Contributor Author

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

@Eng-Fouad
Copy link
Contributor Author

Eng-Fouad commented May 10, 2024

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

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

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

@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:

@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());
}
});
}

@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
Copy link
Contributor Author

Eng-Fouad commented May 11, 2024

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
Copy link
Member

sberyozkin commented May 11, 2024

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
Copy link
Contributor

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
Copy link
Member

sberyozkin commented May 11, 2024

@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
Copy link
Member

@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 force-pushed the #40358 branch 2 times, most recently from 0475089 to 5d9c8fc Compare May 26, 2024 17:28
@Eng-Fouad
Copy link
Contributor Author

@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
Copy link
Member

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

@quarkus-bot

This comment has been minimized.

@sberyozkin
Copy link
Member

@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
Copy link
Contributor Author

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

@Eng-Fouad Eng-Fouad force-pushed the #40358 branch 2 times, most recently from dbf5afd to 2769628 Compare May 30, 2024 03:36
@sberyozkin
Copy link
Member

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

@sberyozkin
Copy link
Member

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

.get();
String tenantId = oidcConfig.tenantId.get();
List<TokenCustomizer> list = findTenantFeaturesByTenantId(TokenCustomizer.class, tenantId, container);
if (!list.isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Eng-Fouad I've only noticed it now, I think it is fine to have multiple features of the same type applied to the same tenant, so propose to drop this check

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sberyozkin You mean droping if (list.size() >= 2)? Then what should we return? Return the first element (list.get(0)) and ignore the others?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Eng-Fouad Oh sorry, does it not return a List... I'd fix it to return a List. Let me look a bit later into it

@quarkus-bot
Copy link

quarkus-bot bot commented May 30, 2024

Status for workflow Quarkus CI

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

✅ 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
Copy link
Contributor

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
Copy link
Member

@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
Copy link
Contributor

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support specifying multiple tenants in @TenantFeature
4 participants