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

Rename OIDC Tenant annotation to TenantFeature #34862

Merged
merged 1 commit into from
Jul 20, 2023

Conversation

sberyozkin
Copy link
Member

quarkus.oidc.Tenant annotation was introduced in 3.2.0 after we discussed with @geoand how we can let users associate some OIDC features with individual OIDC tenants without having to introduce new properties and rely on named beans.
At the moment @Tenant can only be recognized if it is added to a custom TokenCustomizer interface implementation. As it happens, the only real case we've had so far is related to using TokenCustomizer to support preprocessing some Azure tokens - but we ship AzureTokenCustomizer which is a named bean which can be wired in with specific tenants using a configuration property.
It is very unlikely that, since 3.2.0 was released, that there was any other, non Azure related case requiring the token customization in a multi-tenant setup. It can become more realistic with more features supported by custom beans will be introduced.

The reason I've been clarifying all of the above is that we have an enhancement request to be able to bind tenant configurations to JAX-RS classes or methods, and @michalvavrik opened a nice PR, #34833, where using @Tenant annotation makes perfect sense.

However, now we have a dual application of @Tenant:

  • Effectively enhance a tenant configuration by applying @Tenant to some feature interface implementing bean, like CustomTokenCustomizer
  • Use a selected tenant configuration when securing a given JAX-RS class or method - with the latter requirement also extending the @Tenant application to Method - which does not make sense for the original case where it can only be a class level annotation.

We've discussed it with @michalvavrik in #34833.

It does seem that this is the right time to rename originally misnamed @Tenant to @TenantFeature - with nearly 0% of breaking any user code at this point of time (see some reasoning above), and have @TenantFeature used for doing the feature binding to specific tenant configurations which is what @Tenant was originally meant for, while continuing using @Tenant but for new, better scoped purposes - apply specific tenant configuration to JAX-RS methods.

I'll add a migration note - but as I said, I'm fairly confident this technically breaking change won't affect 3.2.x users.

@geoand Can you please comment/review since we worked together on the original @Tenant ?

@quarkus-bot
Copy link

quarkus-bot bot commented Jul 19, 2023

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

Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

Would it also make sense to add a note to the Javadoc of the 3.2 branch warning that the annotation is experimental?
That way any people that start using it from now on (likely very few) will at least have an expectation that things can change?

@sberyozkin
Copy link
Member Author

sberyozkin commented Jul 20, 2023

@geoand Do you think this PR can go ahead or we should wait till #34877 being actually merged (not sure Guillaume will pick it up for 3.2.1, I'll ask) ? I suppose #34877 is our best effort to inform the 3.2.x users only. I'm hoping we can have #34833 as a new feature for 3.3.0.CR1

@geoand
Copy link
Contributor

geoand commented Jul 20, 2023

Let's merge this one.

@sberyozkin
Copy link
Member Author

@geoand Sounds good, can you approve please :-)

Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

💪🏼

@sberyozkin
Copy link
Member Author

@michalvavrik Please pick this change up in your PR :-)

@sberyozkin sberyozkin merged commit ac078d9 into quarkusio:main Jul 20, 2023
20 checks passed
@sberyozkin sberyozkin deleted the oidc_tenant_feature branch July 20, 2023 11:50
@quarkus-bot quarkus-bot bot added this to the 3.3 - main milestone Jul 20, 2023
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.

None yet

2 participants