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

Support permission checkers for WebSockets Next #45704

Conversation

michalvavrik
Copy link
Member

Copy link

github-actions bot commented Jan 20, 2025

🙈 The PR is closed and the preview is expired.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@michalvavrik michalvavrik force-pushed the feature/permission-checkers-for-websockets-next branch from 2b28108 to 4ce0937 Compare January 20, 2025 16:00
@michalvavrik
Copy link
Member Author

I pushed it a little sooner than I meant, I'll make couple more changes in a gist.

This comment has been minimized.

@michalvavrik michalvavrik force-pushed the feature/permission-checkers-for-websockets-next branch from 4ce0937 to 967cc07 Compare January 20, 2025 17:09
@michalvavrik
Copy link
Member Author

michalvavrik commented Jan 20, 2025

hey @mkouba @sberyozkin , thanks for the review, I addressed your comments, but I didn't just apply your suggestions so there are more changes (improvements hopefully) to docs. I also improved test coverage. Please have a look and let's start the review again.

@michalvavrik michalvavrik force-pushed the feature/permission-checkers-for-websockets-next branch from 967cc07 to 9585e23 Compare January 20, 2025 17:18

This comment has been minimized.

@michalvavrik
Copy link
Member Author

I have also realized there are no authorization failure/success events fired for HTTP upgrade checks, so I'll follow-up with other PR to fix it. It's a small thing, but I think users wouldn't understand why the events are not fired for security annotations on endpoints but are fired for other annotation instances.

This comment has been minimized.

@sberyozkin
Copy link
Member

A few Elytron security tests are failing

@michalvavrik
Copy link
Member Author

A few Elytron security tests are failing

I think their assertions are not written well, I'll fix the tests.

@michalvavrik michalvavrik force-pushed the feature/permission-checkers-for-websockets-next branch from 9585e23 to 20c8cc1 Compare January 20, 2025 22:34
@michalvavrik michalvavrik force-pushed the feature/permission-checkers-for-websockets-next branch from 20c8cc1 to 8c0ddee Compare January 21, 2025 16:16
@michalvavrik
Copy link
Member Author

Thanks @sberyozkin , things look always better after your review.

This comment has been minimized.

@sberyozkin
Copy link
Member

Thanks @michalvavrik, not sure about that :-), but thanks anyway :-)

@sberyozkin sberyozkin self-requested a review January 21, 2025 19:25
Copy link
Member

@sberyozkin sberyozkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @michalvavrik, let's wait for @mkouba to approve as well

This comment has been minimized.

@michalvavrik
Copy link
Member Author

That TestSecurityTestCase failed again after the latest changes, I'll look into it.

@michalvavrik michalvavrik force-pushed the feature/permission-checkers-for-websockets-next branch from 8c0ddee to 0b54d67 Compare January 21, 2025 21:21
@michalvavrik
Copy link
Member Author

Sorry, stupid mistake, fixed now.

This comment has been minimized.

This comment has been minimized.

@michalvavrik michalvavrik force-pushed the feature/permission-checkers-for-websockets-next branch from 0b54d67 to e7cd9c7 Compare January 22, 2025 10:25
Copy link
Contributor

@mkouba mkouba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good in general and I believe that @sberyozkin carefully reviewed the security bits ;-).

@mkouba mkouba added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jan 22, 2025
Copy link

quarkus-bot bot commented Jan 22, 2025

Status for workflow Quarkus Documentation CI

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

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

Copy link

quarkus-bot bot commented Jan 22, 2025

Status for workflow Quarkus CI

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

Failing Jobs

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

You can consult the Develocity build scans.

Failures

⚙️ MicroProfile TCKs Tests #

- Failing: tcks/microprofile-opentelemetry 

📦 tcks/microprofile-opentelemetry

Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.5.2:test (tracing) on project quarkus-tck-microprofile-opentelemetry: There are test failures.

See /home/runner/work/quarkus/quarkus/tcks/microprofile-opentelemetry/target/surefire-reports-tracing for the individual test results.
See dump files (if any exist) [date].dump, [date]-jvmRun[N].dump and [date].dumpstream.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 extensions/smallrye-reactive-messaging-kafka/deployment

io.quarkus.smallrye.reactivemessaging.kafka.deployment.testing.KafkaDevServicesContinuousTestingWorkingAppPropsTestCase.testContinuousTestingScenario3 - History

  • io.quarkus.builder.BuildException: Build failure: Build failed due to errors [error]: Build step io.quarkus.apicurio.registry.devservice.DevServicesApicurioRegistryProcessor\#startApicurioRegistryDevService threw an exception: java.lang.RuntimeException: org.testcontainers.containers.ContainerLaunchException: Container startup failed for image quay.io/apicurio/apicurio-registry-mem:2.4.2.Final at io.quarkus.apicurio.registry.devservice.DevServicesApicurioRegistryProcessor.startApicurioRegistryDevService(DevServicesApicurioRegistryProcessor.java:90) at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:732) at io.quarkus.deployment.ExtensionLoader$3.execute(ExtensionLoader.java:856) - java.lang.RuntimeException
java.lang.RuntimeException: 
io.quarkus.builder.BuildException: Build failure: Build failed due to errors
	[error]: Build step io.quarkus.apicurio.registry.devservice.DevServicesApicurioRegistryProcessor#startApicurioRegistryDevService threw an exception: java.lang.RuntimeException: org.testcontainers.containers.ContainerLaunchException: Container startup failed for image quay.io/apicurio/apicurio-registry-mem:2.4.2.Final
	at io.quarkus.apicurio.registry.devservice.DevServicesApicurioRegistryProcessor.startApicurioRegistryDevService(DevServicesApicurioRegistryProcessor.java:90)
	at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:732)
	at io.quarkus.deployment.ExtensionLoader$3.execute(ExtensionLoader.java:856)
	at io.quarkus.builder.BuildContext.run(BuildContext.java:256)
	at org.jboss.threads.ContextHandler$1.runWith(ContextHandler.java:18)

📦 integration-tests/observability-lgtm

io.quarkus.observability.test.LgtmLifecycleTest.testTracing - History

  • java.lang.RuntimeException: java.lang.reflect.InvocationTargetException - java.lang.RuntimeException
java.lang.RuntimeException: java.lang.RuntimeException: java.lang.reflect.InvocationTargetException
	at io.quarkus.test.junit.QuarkusTestExtension.throwBootFailureException(QuarkusTestExtension.java:611)
	at io.quarkus.test.junit.QuarkusTestExtension.interceptTestClassConstructor(QuarkusTestExtension.java:695)
	at java.base/java.util.Optional.orElseGet(Optional.java:364)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
Caused by: java.lang.RuntimeException: java.lang.reflect.InvocationTargetException
	at io.quarkus.test.junit.TestResourceUtil$TestResourceManagerReflections.startReflectively(TestResourceUtil.java:214)

⚙️ JVM Tests - JDK 21

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

📦 extensions/smallrye-reactive-messaging-kafka/deployment

io.quarkus.smallrye.reactivemessaging.kafka.deployment.testing.KafkaDevServicesContinuousTestingTestCase.testContinuousTestingScenario2 - History

  • io.quarkus.builder.BuildException: Build failure: Build failed due to errors [error]: Build step io.quarkus.redis.deployment.client.DevServicesRedisProcessor\#startRedisContainers threw an exception: java.lang.RuntimeException: org.testcontainers.containers.ContainerLaunchException: Container startup failed for image docker.io/redis:7 at io.quarkus.redis.deployment.client.DevServicesRedisProcessor.startRedisContainers(DevServicesRedisProcessor.java:124) at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:733) at io.quarkus.deployment.ExtensionLoader$3.execute(ExtensionLoader.java:856) - java.lang.RuntimeException
java.lang.RuntimeException: 
io.quarkus.builder.BuildException: Build failure: Build failed due to errors
	[error]: Build step io.quarkus.redis.deployment.client.DevServicesRedisProcessor#startRedisContainers threw an exception: java.lang.RuntimeException: org.testcontainers.containers.ContainerLaunchException: Container startup failed for image docker.io/redis:7
	at io.quarkus.redis.deployment.client.DevServicesRedisProcessor.startRedisContainers(DevServicesRedisProcessor.java:124)
	at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:733)
	at io.quarkus.deployment.ExtensionLoader$3.execute(ExtensionLoader.java:856)
	at io.quarkus.builder.BuildContext.run(BuildContext.java:256)
	at org.jboss.threads.ContextHandler$1.runWith(ContextHandler.java:18)

📦 integration-tests/grpc-hibernate

com.example.grpc.hibernate.BlockingRawTest.shouldAdd - History

  • Condition with Lambda expression in com.example.grpc.hibernate.BlockingRawTestBase was not fulfilled within 30 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in com.example.grpc.hibernate.BlockingRawTestBase was not fulfilled within 30 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:975)
	at com.example.grpc.hibernate.BlockingRawTestBase.shouldAdd(BlockingRawTestBase.java:59)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)

⚙️ Maven Tests - JDK 17 Windows

📦 integration-tests/devmode

io.quarkus.test.devui.DevUIGrpcSmokeTest.testTestService - History

  • Too many recursions, message not returned for id [1451529146] - java.lang.RuntimeException
java.lang.RuntimeException: Too many recursions, message not returned for id [1451529146]
	at io.quarkus.devui.tests.DevUIJsonRPCTest.objectResultFromJsonRPC(DevUIJsonRPCTest.java:164)
	at io.quarkus.devui.tests.DevUIJsonRPCTest.objectResultFromJsonRPC(DevUIJsonRPCTest.java:167)
	at io.quarkus.devui.tests.DevUIJsonRPCTest.objectResultFromJsonRPC(DevUIJsonRPCTest.java:167)
	at io.quarkus.devui.tests.DevUIJsonRPCTest.objectResultFromJsonRPC(DevUIJsonRPCTest.java:167)
	at io.quarkus.devui.tests.DevUIJsonRPCTest.objectResultFromJsonRPC(DevUIJsonRPCTest.java:167)
	at io.quarkus.devui.tests.DevUIJsonRPCTest.objectResultFromJsonRPC(DevUIJsonRPCTest.java:167)
	at io.quarkus.devui.tests.DevUIJsonRPCTest.objectResultFromJsonRPC(DevUIJsonRPCTest.java:167)

@michalvavrik
Copy link
Member Author

MicroProfile OpenTelemetry TCK failure seems unrelated. IMHO this is ready for merge.

@sberyozkin sberyozkin merged commit b95f874 into quarkusio:main Jan 22, 2025
54 of 55 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.19 - main milestone Jan 22, 2025
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jan 22, 2025
@michalvavrik michalvavrik deleted the feature/permission-checkers-for-websockets-next branch January 22, 2025 18:08
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.

4 participants