Do not require IdentityProviders for HTTP Authentication mechanisms

[michalvavrik]

closes: #38508

[sberyozkin]

Hey @michalvavrik Thanks for the quick fix. I'm sorry but I think we should simplify it, specifically, I don't think the builtin check is necessary. The whole HttpAuthenticationMechanism and IdentityProvider split is a best approach design decision recommendation which we should recommend, especially when more than one IdentityProvider can meaningfully process the current authentication credentials, but it should not be enforced. The decision whether to delegate to IdentityProviders or not should be entirely up to a given HttpAuthenticationMechanism, for example, delegating it to the identity provider gives an option to use a blocking context etc. But if all one wants to do is to check some custom headers against some local key or similar, it should work, without users having to register an identity provider, etc

IMHO there should only be 2 small code changes: 1. Remove the check which blocks HttpAuthenticationMechanism call if no IdentityProvider is registered, and 2. Throw a server error if IdentityProviderManager is called if no IdentityProvider is registered. May be also add a debug message if no IdentityProvider is registered, advising that in this case HttpAuthenticationMechanism must be able to resolve SecurityIdentity itself.

Also if you don't mind we should add a small section to the top of the Custom HttpAuthenticationMechanism docs that we recommend to use IdentityProvider because it allows for more options with verifying the credentials, as well as running blocking operations, but simple mechanisms requiring no blocking operations are not required to do it

[michalvavrik]

Thank you for detailed comment. I was afraid that what you suggest has potential for breaking change as till now, if someone added an extension with HttpAuthenticationMechanism and didn't provide his own IdentityProvider, nothing would happen. But when I read your comment I realized that if such auth mechanism is enabled and added as bean, there is no point having it without IdentityProvider. I inspected all our builtin implementations and I can't see how it could break anything. Your suggestion is better.

2. Throw a server error if IdentityProviderManager is called if no IdentityProvider is registered.

That is already happening.

May be also add a debug message if no IdentityProvider is registered, advising that in this case HttpAuthenticationMechanism must be able to resolve SecurityIdentity itself.

That can't be done inside IdentityProviderManager as it is not aware of the HttpAuthenticationMechanisms. To do that inside Vert.x HTTP it would require to do what we do right now just to write debug message. Honestly I think it is clear from thrown error message No IdentityProviders were registered to handle AuthenticationRequest xyz. So I'd rather not do it.

I'll try to add docs comment as well and I'm sure we will iterate on that :-).

[quarkus-bot[bot]]

---

:waning_crescent_moon: This workflow status is outdated as a new workflow run has been triggered.

---

Status for workflow Quarkus Documentation CI

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

:white_check_mark: 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 check runs running, make sure you don't need to wait for their status before merging.

[github-actions[bot]]

🙈 The PR is closed and the preview is expired.

[quarkus-bot[bot]]

---

:waning_crescent_moon: This workflow status is outdated as a new workflow run has been triggered.

---

Status for workflow Quarkus CI

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

Failing Jobs

Status	Name	Step	Failures	Logs	Raw logs	Build scan
✖	JVM Tests - JDK 17	Build	Failures	Logs	Raw logs	:construction:
✖	JVM Tests - JDK 17 Windows	Build	Failures	Logs	Raw logs	:construction:
✖	JVM Tests - JDK 21	Build	Failures	Logs	Raw logs	:construction:

Full information is available in the Build summary check run.

Failures

:gear: JVM Tests - JDK 17 #

- Failing: extensions/elytron-security-properties-file/deployment 
! Skipped: devtools/cli extensions/agroal/deployment extensions/avro/deployment and 278 more

:package: extensions/elytron-security-properties-file/deployment

✖ io.quarkus.security.test.NoConfiguredRealmsTestCase.testSecureAccessFailure line 30 - History - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Expected status code <403> but was <401>.

	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499)

---

:gear: JVM Tests - JDK 17 Windows #

- Failing: extensions/elytron-security-properties-file/deployment 
! Skipped: devtools/cli extensions/agroal/deployment extensions/avro/deployment and 278 more

:package: extensions/elytron-security-properties-file/deployment

✖ io.quarkus.security.test.NoConfiguredRealmsTestCase.testSecureAccessFailure line 30 - History - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Expected status code <403> but was <401>.

	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499)

---

:gear: JVM Tests - JDK 21 #

- Failing: extensions/elytron-security-properties-file/deployment 
! Skipped: devtools/cli extensions/agroal/deployment extensions/avro/deployment and 278 more

:package: extensions/elytron-security-properties-file/deployment

✖ io.quarkus.security.test.NoConfiguredRealmsTestCase.testSecureAccessFailure line 30 - History - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Expected status code <403> but was <401>.

	at java.base/jdk.internal.reflect.DirectConstructorHandleAccessor.newInstance(DirectConstructorHandleAccessor.java:62)
	at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:502)
	at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:486)
	at org.codehaus.groovy.reflection.CachedConstructor.invoke(CachedConstructor.java:73)

[michalvavrik]

NoConfiguredRealmsTestCase.testSecureAccessFailure failed because the BasicAuthenticationMechanism is added when not explicitly disabled and neither form or mTLS is enabled etc. Previously it was just ignored if there was no identity provider supporting it's credentials. I find this somehow confusing considering that documentation says it is enabled by default if no auth mechanisms are configured:

https://github.com/quarkusio/quarkus/blob/1fd700609396389759d03de7d22d4ee1da32e14c/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/AuthConfig.java#L17

1. It wasn't really true because we enabled it depending on mTLS and form, but there is plenty of other mechanisms ignored.
2. It wasn't added if there wasn't necessary identity provider.

Furthermore, for such a use case as "default" mechanism it really makes sense to only introduce it when there is required IdentityProvider, otherwise authentication can't ever succeed. It is a special case, therefore I added special handling to keep original behavior. I still believe it's good idea to make this change.

[quarkus-bot[bot]]

---

:waning_crescent_moon: This workflow status is outdated as a new workflow run has been triggered.

---

Status for workflow Quarkus Documentation CI

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

:white_check_mark: 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 check runs running, make sure you don't need to wait for their status before merging.

[quarkus-bot[bot]]

---

:waning_crescent_moon: This workflow status is outdated as a new workflow run has been triggered.

---

Status for workflow Quarkus CI

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

Failing Jobs

Status	Name	Step	Failures	Logs	Raw logs	Build scan
:heavy_check_mark:	JVM Tests - JDK 17			Logs	Raw logs	:construction:
✖	JVM Tests - JDK 21	Build	Failures	Logs	Raw logs	:construction:

Failures

:gear: JVM Tests - JDK 21 #

- Failing: integration-tests/hibernate-search-orm-opensearch 

:package: integration-tests/hibernate-search-orm-opensearch

✖ Failed to execute goal io.fabric8:docker-maven-plugin:0.43.4:start (docker-start) on project quarkus-integration-test-hibernate-search-orm-opensearch: I/O Error

---

Flaky tests - Develocity

:gear: JVM Tests - JDK 17

:package: extensions/smallrye-health/deployment

✖ io.quarkus.smallrye.health.test.HealthCheckContextPropagationTest.testContextPropagatedToHealthChecks - History

• `3 expectations failed. JSON path status doesn't match. Expected: is "UP" Actual: DOWN

JSON path checks.status doesn't match. Expected: iterable containing ["UP"] Actual: <[DOWN]>

JSON path checks.name doesn't match. Expected: iterable containing ["io.quarkus.smallrye.health.test.HealthCheckContextPropagationTest$ContextualHC"] Actual: <[io.quarkus.smallrye.health.test.HealthCheckContextPropagationTest_ContextualHC_ClientProxy]> ` - java.lang.AssertionError

java.lang.AssertionError: 
3 expectations failed.
JSON path status doesn't match.
Expected: is "UP"
  Actual: DOWN

JSON path checks.status doesn't match.
Expected: iterable containing ["UP"]

---

:gear: Maven Tests - JDK 17

:package: integration-tests/maven

✖ io.quarkus.maven.it.DevMojoIT.testThatNewResourcesAreServed - History

• io.quarkus.maven.it.DevMojoIT expected "d90d4ee0-2f59-4388-85ad-9952996c53eb" but was "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua." within 2 minutes. - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: io.quarkus.maven.it.DevMojoIT expected "d90d4ee0-2f59-4388-85ad-9952996c53eb" but was "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua." within 2 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AbstractHamcrestCondition.await(AbstractHamcrestCondition.java:86)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:985)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:691)
	at io.quarkus.maven.it.DevMojoIT.testThatNewResourcesAreServed(DevMojoIT.java:967)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

✖ io.quarkus.maven.it.DevMojoIT.testThatNewResourcesAreServed - History

• io.quarkus.maven.it.DevMojoIT expected "d90d4ee0-2f59-4388-85ad-9952996c53eb" but was "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua." within 2 minutes. - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: io.quarkus.maven.it.DevMojoIT expected "d90d4ee0-2f59-4388-85ad-9952996c53eb" but was "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua." within 2 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AbstractHamcrestCondition.await(AbstractHamcrestCondition.java:86)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:985)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:691)
	at io.quarkus.maven.it.DevMojoIT.testThatNewResourcesAreServed(DevMojoIT.java:967)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

[michalvavrik]

Hmm, I'll need to change much more tests to make this work. @sberyozkin I really think there needs to be an exception for basic auth mechanism. There is countless of tests failing just in the Vert.x HTTP module.

[michalvavrik]

Thanks @michalvavrik, about to propose a couple of tiny changes only

Appreciated, but I'll also push changes related to basic auth as there is too many tests failing. You will need to review again, sorry.

[sberyozkin]

I really think there needs to be an exception for basic auth mechanism.

Well, lets not fail then if the match does not exist if it is so tricky to untangle it for the basic mechanism, have a look though if it might be a test specific issue

[michalvavrik]

Well, lets not fail then if the match does not exist if it is so tricky to untangle it for the basic mechanism, have a look though if it might be a test specific issue

I think almost all test failed in Vert.x HTTP module so I'm pretty sure it is not test specific. But TBH I stopped testing after dozen of them.

The root cause is that basic authentication mechanism is added almost always unless you use @Alternative or you use form or mTLS. But I think that should be handled and discussed separately as it is breaking change. Changes in the PR now should be fine and not break things.

[quarkus-bot[bot]]

---

:waning_crescent_moon: This workflow status is outdated as a new workflow run has been triggered.

---

Status for workflow Quarkus Documentation CI

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

:white_check_mark: 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[bot]]

Status for workflow Quarkus Documentation CI

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

:white_check_mark: 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[bot]]

Status for workflow Quarkus CI

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

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