Fix 'response head already sent' when the login request to form-based authentication when proactive authentication is disabled

[michalvavrik]

• closes: IllegalStateException: Response head already sent during login #44458

[quarkus-bot]

Thanks for your pull request!

Your pull request does not follow our editorial rules. Could you have a look?

• title should preferably start with an uppercase character (if it makes sense!)
• title should not start with chore/docs/feat/fix/refactor but be a proper sentence

_{This message is automatically generated by a bot.}

[geoand]

Is event.response().end(); actually intended here? Why not event.next()?

[michalvavrik]

Is event.response().end(); actually intended here? Why not event.next()?

It is, I don't know why Stuart implemented this whole endpoint this way, I think one they someone will report bug regarding other authentication mechanisms interfering, but when you call https://quarkus.io/guides/all-config#quarkus-vertx-http_quarkus-http-auth-form-post-location request path, you are supposed to be logged in and that is it. It should not continue let say to Quarkus REST (which is what was happening) or somewhere else. There is following property:

• quarkus.http.auth.form.landing-page that is where Quarkus redirects you if this request was successful to log in
• is it is not set, e.g. like quarkus.http.auth.form.landing-page=, then we return 200 (for SPA where JS determines what next)

So I never actually expect this to be not ended. I was strongly considering to return 500 and log some error message, but there are users that inherit from this mechanism and customize it, I am not 100 % sure what they do, hence just ending it.

[michalvavrik]

of course we can continue using event.next(), but I don't see a point. if you have an idea, I can change it, it is not the root cause

[michalvavrik]

also I am not 100 % there is always some handler that ends the response, I never checked if Vert.x or Quarkus has something like "last resort handler" that ends it if noone else did?

[michalvavrik]

I suppose it is implemented this way to align with proactive auth.

[michalvavrik]

@sberyozkin I am just fixing bug (IMHO correctly), it is not my design/code, that is how it worked from the start (I assume).

No, I don't think it can be constrained to the FormAuthenticationMechanism because we know some users inherits this mechanism, some can use delegate pattern and call that mechanism completely differently. We would break their applications. This code is basically same thing as:

• enforce authentication for path /j_security_check

So it is completely safe, I just don't think it is always reliable, like if there are other authentication mechanisms like the code flow, they can interfere). So maybe some users will need to use path-based authentication if they combine multiple authentication mechanisms.

[michalvavrik]

One more thing, from @geoand and @sberyozkin questions I am not sure it is clear, maybe I should had written it to the PR description. The issue here was that FormAuthenticationMechanism does what is supposed to do, it sends the response, but this code still did next(). The fact that I added end(), well, it just felt right. :-)

[geoand]

Yeah, what you did feels right to me too, but I am not privy to this code at all

[sberyozkin]

@michalvavrik Oh, this code is specific to the form processing, done inside the form specific handler, it was not easy to see it in the PR, so LGTM

[michalvavrik]

fun fact, I think this issue #30637 is example of such unreliability I mentioned; wdyt @sberyozkin ? we can move discussion into the issue

[quarkus-bot]

Status for workflow Quarkus CI

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

✅ 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:513)
	at io.quarkus.test.QuarkusUnitTest.interceptTestMethod(QuarkusUnitTest.java:427)

---

⚙️ JVM Tests - JDK 21

📦 extensions/hibernate-orm/deployment

✖ io.quarkus.hibernate.orm.applicationfieldaccess.PublicFieldAccessAssociationsTest.testFieldAccess - History

• expected: io.quarkus.hibernate.orm.applicationfieldaccess.PublicFieldAccessAssociationsTest$ContainedEntity@5adafe99 but was: null - org.opentest4j.AssertionFailedError

org.opentest4j.AssertionFailedError: 

expected: io.quarkus.hibernate.orm.applicationfieldaccess.PublicFieldAccessAssociationsTest$ContainedEntity@5adafe99
 but was: null
	at io.quarkus.hibernate.orm.applicationfieldaccess.PublicFieldAccessAssociationsTest$FieldAccessEnhancedDelegate$5.assertValueAndLaziness(PublicFieldAccessAssociationsTest.java:240)
	at io.quarkus.hibernate.orm.applicationfieldaccess.PublicFieldAccessAssociationsTest.doTestFieldAccess(PublicFieldAccessAssociationsTest.java:106)
	at io.quarkus.hibernate.orm.applicationfieldaccess.PublicFieldAccessAssociationsTest.testFieldAccess(PublicFieldAccessAssociationsTest.java:60)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)