Support flow-specific OIDC request and response filters

[michalvavrik]

• closes: Flow specific OIDC request/response filter support #46697

[github-actions]

🙈 The PR is closed and the preview is expired.

[sberyozkin]

Hi @michalvavrik, I've looked at the code, and I'm not sure I follow the reason for a rather significant code update across these few extensions.

First of all, why are OIDC client and client registrations extensions impacted ? The use case is about restricting for the quarkus-oidc , only this extension has to deal with the bearer or code flow.

Second, if quarkus.oidc.application-type is all we need to figure out if the filters should be applied to either the bearer or code flow, then why can't we just apply an additional check to check either the bearer or code flow annotation when OidcProviderClientImpl is initialized, just to iterate over the Map of filters and discard fiters that are meant for another flow ?

[michalvavrik]

Hi @michalvavrik, I've looked at the code, and I'm not sure I follow the reason for a rather significant code update across these few extensions.
First of all, why are OIDC client and client registrations extensions impacted ? The use case is about restricting for the quarkus-oidc , only this extension has to deal with the bearer or code flow.

When filters are collected by OIDC Common utils, they list all filters, not just filters for "OIDC Client" and "Oidc Client Registration". So the existing code queries Arc. inspect CDI beans annotations and decides what filters to keep. And then every single time it queries a hash map for only matching filters.

Long story short:

• this PR moves part of the logic to the build time rather then runtime
• sorts filters per endpoints once (and you don't have to join ALL and DISCOVERY (or other endpoint type) every time you are looking for matching filters (which is on every request)
• keeps sorted filters in enum map which should be better optimized
• previously if e.g. Oidc Client and OIDC where both present, both you keep their own maps (identical in content), now the share same maps.

But most importantly, it makes this concept extendable. I don't think it is a good idea to just patch it on OIDC extension level and next time we want something (let say for OIDC CLient or Registration) we need to make something specific for these extensions.

I thought this PR provides an improvement to original implementation 🤷 but I understand that it is very subjective thing to say.

Second, if quarkus.oidc.application-type is all we need to figure out if the filters should be applied to either the bearer or code flow, then why can't we just apply an additional check to check either the bearer or code flow annotation when OidcProviderClientImpl is initialized, just to iterate over the Map of filters and discard fiters that are meant for another flow ?

For record, it is not just OidcProviderClient, it is also DynamicVerificationKeyResolver. But yes, that is perfectly legit implementation if you want to keep what you have. I don't like to "having remember" that every time I want to call these filters I need to remember that OIDC have some specific filter checks and that I need to apply OIDC utils method in addition to OIDC Common utils, but I realize it is absolutely common way of doing things.

[sberyozkin]

@michalvavrik

But most importantly, it makes this concept extendable. I don't think it is a good idea to just patch it on OIDC extension level and next time we want something (let say for OIDC CLient or Registration) we need to make something specific for these extensions.

OIDC client extension does not have any flow variability. Nor does OIDC client registration have it. What are the use cases for any of these extensions ?

I know, you have put the effort, which is always appreciated. But again, a lot more is going on in this PR than this advanced case actually requires, as you know, I do prefer issue specific PRs.

this PR moves part of the logic to the build time rather then runtime...

This and other optimizations are sounding interesting, but they are not related to the issue this PR resolves. They are complex enough to warrant a dedicated PR

[michalvavrik]

@michalvavrik

But most importantly, it makes this concept extendable. I don't think it is a good idea to just patch it on OIDC extension level and next time we want something (let say for OIDC CLient or Registration) we need to make something specific for these extensions.

OIDC client extension does not have any flow variability. Nor does OIDC client registration have it. What are the use cases for any of these extensions ?

OIDC client extension and OIDC client registry don't need to use any flow variability. If you want, you can use oidcFilterStorage.getOidcRequestFilters(endpointType) and they will only profit from this PR, without a downside I can see. Sure, I can be wrong. Yes, I realize that I added oidcFilterStorage.getOidcRequestFilters(endpointType, context), but they don't need to use it, I just thought using the context is right thing to do so that if you want to extend this concept, you don't need to change it everywhere.

I know, you have put the effort, which is always appreciated.

I am ready to rewrite it to your implementation suggestion (which will have quite little value, it just adds another map and couple of predicates, so it will be something I could had done in few hours rather than few days). That is life, no worry.

But again, a lot more is going on in this PR than this advanced case actually requires, as you know, I do prefer issue specific PRs.

My opinion is that it is the same thing. You can't patch one thing on top of another without thinking. I implemented here one storage because that is what is needed to implemented linked feature effectively and keep current implementation extendable.

If I were honest, I don't think you mentioned any good reason against this PR other than doing it current way is easier for us (for now).

this PR moves part of the logic to the build time rather then runtime...

This and other optimizations are sounding interesting, but they are not related to the issue this PR resolves. They are complex enough to warrant a dedicated PR

Yes, it is complex, but as I mentioned above, it doesn't make sense to me to implement this feature and then do these changes. This PR enables this feature. Anyway, I'll do what you want.

[michalvavrik]

This and other optimizations are sounding interesting

Just general note: I have mentioned it before in context of TenantFeatureFinder, I think this idea that we inspect collect and inspect annotation instances during the runtime. I thought that implementing things in Quarkus, we are supposed to try doing things during the build time unless it makes things complex. I don't think this PR brings complexity, but I think if someone else reads it, they can say it does. I agree that doing things like are done currently with io.quarkus.oidc.common.runtime.OidcCommonUtils#getOidcFilters is easier to read and maintain.

[michalvavrik]

Not sure if it makes sense to comment on current impl. as I have to rewrite it, but OIDC & OIDC CLient & OIDC CLient Registration share certain methods like OidcCommonUtils.discoverMetadata, which is why I used storage there as well (so that I don't need to use "matching filters" from OIDC common utils which this PR doesn't need).

[sberyozkin]

Hi Michal @michalvavrik, sure, please don't take it the wrong way, you write great code and have a ton of ideas, but I'm just an old and conservative person who finds it difficult to focus on the review and feel the situation is under control, when PR contains much more than the original issue requires.

For example, the Discovery vs ALL filter target you mentioned, this is off topic for the issue, and I'm not too worried about doing this check on every request given that there will be 2/3 filters max in the majority of cases. We can definitely try to optimize it, but again, it is not related to restricting filters to either bearer or code flows, the problem relevant for the OIDC extension only.

The other thing, that with my rather primitive approach, I'd only change one source class, OidcProviderClientImpl, its constructor, where the passed map of filters is passed to the static method that gets rid of the filters that do not much the flow requirement which is already available expressed in the configuration - done once at the init time.

And may be something in the recorder to handle the same for the discovery/jwks

[sberyozkin]

By the way, please take your time to refactor it as I'll probably have no time to review before I sign off at lunch time tomorrow sharp, and indeed keep the other changes recorded to discuss them later

[michalvavrik]

@sberyozkin I"ll implement it the way you suggest and have it ready when you are back.

[sberyozkin]

Brilliant work @michalvavrik, thanks

[sberyozkin]

@michalvavrik It is ready to go, but I think we need to do a minor doc update similarly to what is suggested in the comment, thanks

[quarkus-bot]

Status for workflow Quarkus Documentation CI

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

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

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

---

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 extensions/infinispan-cache/deployment

❌ io.quarkus.cache.infinispan.InfinispanCacheTest.testGetAsyncWithParallelCalls - History

• expected: "thread1" but was: "thread2" - org.opentest4j.AssertionFailedError

org.opentest4j.AssertionFailedError: 

expected: "thread1"
 but was: "thread2"
	at io.quarkus.cache.infinispan.InfinispanCacheTest.testGetAsyncWithParallelCalls(InfinispanCacheTest.java:283)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)
	at io.quarkus.test.QuarkusUnitTest.runExtensionMethod(QuarkusUnitTest.java:534)
	at io.quarkus.test.QuarkusUnitTest.interceptTestMethod(QuarkusUnitTest.java:448)