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

Fix priority of interceptors preventing repeated security checks so that interceptor activating CDI request context runs first #40840

Conversation

michalvavrik
Copy link
Member

Makes sure that ActivateRequestContextInterceptor runs before interceptors that prevent repeated checks which already happened eagerly.

Motivation for this change: quarkiverse/quarkus-langchain4j#539 (comment)

When HTTP request has completed, the interceptors preventing repeated checks must applied without internal error. See linked issue for more information.

New order:

  • ActivateRequestContextInterceptor priority 100 (activate request context)
  • StandardSecurityCheckInterceptors priority 900 (prevent repeated checks if the security check had been performed eagerly)
  • standard security interceptors 1000 (secure CDI beans and play fallback security role for the resources)

It is fairly edge case because it's hard to avoid active CDI request context at that point and if you activate it, you don't have the identity unless you do custom security yourself. Still, it's right thing to fix it.

@michalvavrik michalvavrik changed the title Fix priority of interceptors preventing repeated checks so that interceptor activating CDI request context runs first Fix priority of interceptors preventing repeated security checks so that interceptor activating CDI request context runs first May 25, 2024
Copy link

quarkus-bot bot commented May 25, 2024

Status for workflow Quarkus CI

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

Failing Jobs

Status Name Step Failures Logs Raw logs Build scan
MicroProfile TCKs Tests Verify Failures Logs Raw logs 🔍

Full information is available in the Build summary check run.
You can consult the Develocity build scans.

Failures

⚙️ MicroProfile TCKs Tests #

- Failing: tcks/microprofile-opentelemetry 

📦 tcks/microprofile-opentelemetry

org.eclipse.microprofile.telemetry.tracing.tck.async.MpRestClientAsyncTest.testIntegrationWithMpRestClient - History - More details - Source on GitHub

java.util.concurrent.RejectedExecutionException: event executor terminated
	at io.netty.util.concurrent.SingleThreadEventExecutor.reject(SingleThreadEventExecutor.java:934)
	at io.netty.util.concurrent.SingleThreadEventExecutor.offerTask(SingleThreadEventExecutor.java:351)
	at io.netty.util.concurrent.SingleThreadEventExecutor.addTask(SingleThreadEventExecutor.java:344)
	at io.netty.util.concurrent.SingleThreadEventExecutor.execute(SingleThreadEventExecutor.java:836)
	at io.netty.util.concurrent.SingleThreadEventExecutor.execute0(SingleThreadEventExecutor.java:827)
	at io.netty.util.concurrent.SingleThreadEventExecutor.execute(SingleThreadEventExecutor.java:817)
	at io.vertx.core.impl.EventLoopExecutor.execute(EventLoopExecutor.java:35)

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 integration-tests/reactive-messaging-kafka

io.quarkus.it.kafka.KafkaConnectorTest.testFruits - History

  • Assertion condition defined as a Lambda expression in io.quarkus.it.kafka.KafkaConnectorTest expected: <6> but was: <5> within 10 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Assertion condition defined as a Lambda expression in io.quarkus.it.kafka.KafkaConnectorTest expected: <6> but was: <5> within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.untilAsserted(ConditionFactory.java:790)
	at io.quarkus.it.kafka.KafkaConnectorTest.testFruits(KafkaConnectorTest.java:63)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)

@sberyozkin
Copy link
Member

Thanks @michalvavrik, I also opened the discussion at #40798. Like you say this change won't make the identity available due ActiveRequestContext methods if the context was already terminated, but at least we can now avoid a confusing error from the standard security check interceptor

@sberyozkin sberyozkin merged commit edf4d5f into quarkusio:main May 25, 2024
51 of 52 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.12 - main milestone May 25, 2024
@michalvavrik michalvavrik deleted the feature/fix-context-activation-for-sec-interceptors branch May 25, 2024 20:21
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.

2 participants