Introduce inclusive authentication mode, improve documentation of inclusive authentication and make its config runtime

[michalvavrik]

• documents how to handle user scenario described in Endpoints with different authentication schemes #45619 until Support several authentication mechanisms for a specific path #46167 is implemented
• introduces inclusive mode; by default the inclusive auth is strictly inclusive, all the mechanisms must create identity, while in lax all the mechanisms must perform authentication, but at least one must create identity
• moves "inclusive" flag to runtime. There is no win in keeping it build-time,we don't use it there. I made initially the build-time property because I was afraid about the moment when HttpAuthenticator is initialized, but:
  • I have inspected all injection points and can't see why would it make a difference even if we kept everything as is
  • I have reduced injection points to one (authentication handler), everyone else will take the authenticator from RoutingContext
  • I plan to work on programmatic security creation very soon and having this property at runtime will allow users to configure it programmatically
• introduces PreRouterFinalizationBuildItem because I realized that FilterBuildItem is sometimes consumed before runtime config are ready (e.g. in UndertowBuildStep) and it would cause a circular reference

[github-actions]

🙈 The PR is closed and the preview is expired.

[sberyozkin]

Hi Michal, @michalvavrik, thanks for working on eliminating the current ambiguity related to the inclusive authentication, it looks good, I've tried to suggest some possible doc simplifications.
Can you also please clarify, what do you mean by
hardening PR that by default all the mechanisms need to create identity if at least one mechanism created it ?

My understanding that if it is lax, then if we have say 5 mechanisms, then it can be anything between 1 and 5 mechanisms that produced the identity, if it is strict, all 5 of them must produce it

[michalvavrik]

I'll check the failures (seems related) and have a look at comments in 2-3 hours.

hardening PR that by default all the mechanisms need to create identity if at least one mechanism created it ?
My understanding that if it is lax, then if we have say 5 mechanisms, then it can be anything between 1 and 5 mechanisms that produced the identity, if it is strict, all 5 of them must produce it

Yes, it is exactly how you say it, let me drop the hardening, I had to go to sleep early last night so I typed it in a hurry. (I didn't type the code in the hurry, just the PR description)

[michalvavrik]

I see, the way Elytron produces SecurityDomain bean is racy (and not good), basically to say that bean is ready even if it is not is wrong, that is what synthetic beans are for. I think I have seen it in past already when HttpAuthenticator was initialized before that build step got executed. I'll make it a synthetic bean instead.

[michalvavrik]

Thank you @sberyozkin for the review, I think I have handled everything, but I also a wrote a new text that you requested, so better have a look again. Thanks

[sberyozkin]

@michalvavrik Hey

I had to go to sleep early last night

You did the right thing :-), we should always do it :-). Let me have a quick look

[sberyozkin]

Thanks Michal @michalvavrik, a few more suggestions are proposed to try to make it clearer how these modes differ and when lax mode can really help, but overall looks good, thanks

[michalvavrik]

@sberyozkin I think your docs suggestions are great, but we are running out of time and I think it will be hard to convince Guillaume to backport this. Unless you can see something major, can we get this in and do a docs PR that would address your further suggestions? This PR needs to get in today. Thanks.

[quarkus-bot]

Status for workflow Quarkus Documentation CI

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

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

I think, given the example you give about relying on the lax mode inclusive authentication to decide which mechanisms did the authentication makes this line a bit confusing... Futhermore, once we at some point just let users configure more than one mechanism per path, the inclusive strict mode can impact if it is and or or combination... IMHO it is worth dropping this line and for us keep tuning things going forward.

[michalvavrik]

This line is also present in the main branch already.

Futhermore, once we at some point just let users configure more than one mechanism per path, the inclusive strict mode can impact if it is and or or combination

I am not sure if I understand you, but inclusive authentication does not apply when for certain path user specifically selected one mechanism. I don't think it even makes sense. If I annotate endpoint with @BasicAuthentication I expect to use this mechanism and not all the registered mechanisms. I suspect I misunderstood your comment?

IMHO it is worth dropping this line and for us keep tuning things going forward.

I think it is extremely important information. You need to know that if you select specific path-matching authentication mechanism, inclusive authentication does not happen.

[sberyozkin]

@michalvavrik

but inclusive authentication does not apply when for certain path user specifically selected one mechanism

It is just that you added a doc section called Using inclusive authentication to enable path-based authentication but this line says the inclusive authentication is ignored for the path based authentication. So the messaging is conflicting,

How about saying something like This property is ignored for the enabled path based authentication which can currently support only a single authentication mechanism.

[sberyozkin]

@michalvavrik Never mind, that doc section clarifies that it is used in the lax mode, the messaging around inclusive authentication can be reworked later when more than one mechanism is supported for a specific path

[michalvavrik]

How about saying something like This property is ignored for the enabled path based authentication which can currently support only a single authentication mechanism.

I see, yes that is a good point.

@michalvavrik Never mind, that doc section clarifies that it is used in the lax mode, the messaging around inclusive authentication can be reworked later when more than one mechanism is supported for a specific path

I put #46167 on my list, I don't think it is realistic someone else will implement it anytime soon, so I'll try to go back to it in few months and we can revise this docs.

[quarkus-bot]

Status for workflow Quarkus CI

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

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

📦 extensions/micrometer-opentelemetry/deployment

✖ io.quarkus.micrometer.opentelemetry.deployment.compatibility.MicrometerTimedInterceptorTest.testTimeMethod - History

• Stream has no elements - java.lang.IllegalArgumentException

java.lang.IllegalArgumentException: Stream has no elements
	at io.quarkus.micrometer.opentelemetry.deployment.common.MetricDataFilter.lambda$lastReading$2(MetricDataFilter.java:213)
	at java.base/java.util.Optional.orElseThrow(Optional.java:403)
	at io.quarkus.micrometer.opentelemetry.deployment.common.MetricDataFilter.lastReading(MetricDataFilter.java:213)
	at io.quarkus.micrometer.opentelemetry.deployment.common.MetricDataFilter.lastReadingDataPoint(MetricDataFilter.java:231)
	at io.quarkus.micrometer.opentelemetry.deployment.compatibility.MicrometerTimedInterceptorTest.testTimeMethod(MicrometerTimedInterceptorTest.java:77)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)
	at io.quarkus.test.QuarkusUnitTest.runExtensionMethod(QuarkusUnitTest.java:513)

[michalvavrik]

@sberyozkin this can't be backported without dedicated changes because Vert.x HTTP config has been migrated to Configmapping in 3.19. I'll drop the backport label, if you still feel this needs to be backported, I can prepare dedicated backport commit.