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

Provide a way to observe security events #37472

Merged

Conversation

michalvavrik
Copy link
Member

closes: #26549

@quarkus-bot

This comment has been minimized.

Copy link

github-actions bot commented Dec 3, 2023

🙈 The PR is closed and the preview is expired.

@michalvavrik michalvavrik force-pushed the feature/add-security-event-observer branch 3 times, most recently from 36fab11 to 6d6ad1e Compare December 4, 2023 08:15
@sberyozkin
Copy link
Member

@michalvavrik Very nice, nearly ready to be merged

@quarkus-bot

This comment has been minimized.

@michalvavrik
Copy link
Member Author

@Ladicek @mkouba @manovotn You are right, I'll rewrite this PR so that only CDI events are used and there will be no API. I had following reasons to define API:

  • one place to disable security events at runtime
  • options for users to provide their own implementation of the SecurityEventProducer
  • I tried to optimize as you saw too well :-)

@mkouba suggested on Zulip way to do all that I want to do and as you explained, handling of CDI events is already optimized and user can just observe events and then fire events with Kafka or whatever he wants.

I'll put this to draft, rework the PR and ask all of you for review when I'm done.

BTW @sberyozkin I'm sure you have opinions as well, but let me rework this first so that you can see what I have in mind this time. Thanks

@michalvavrik michalvavrik marked this pull request as draft December 4, 2023 16:14
@sberyozkin
Copy link
Member

@michalvavrik I think we have agreed in all the pending/open comments, including keeping an interface and configuration property (even if we look at when to use it a bit differently), and IMHO this PR is nearly ready to be merged, thanks for your effort

@michalvavrik michalvavrik force-pushed the feature/add-security-event-observer branch from 14b6c87 to f1bfef5 Compare December 12, 2023 17:22
@quarkus-bot

This comment has been minimized.

@michalvavrik michalvavrik force-pushed the feature/add-security-event-observer branch 2 times, most recently from 619e069 to 008451f Compare December 13, 2023 09:17
@sberyozkin sberyozkin self-requested a review December 13, 2023 10:56
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

@quarkus-bot

This comment has been minimized.

@michalvavrik michalvavrik force-pushed the feature/add-security-event-observer branch from 008451f to f684ade Compare December 13, 2023 16:05
Copy link
Contributor

@manovotn manovotn left a comment

Choose a reason for hiding this comment

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

My comments are just minor nitpicks, otherwise the CDI bits and the event-firing optimization look good to me.

@michalvavrik michalvavrik force-pushed the feature/add-security-event-observer branch from f684ade to c06cf1e Compare December 13, 2023 18:59
@quarkus-bot
Copy link

quarkus-bot bot commented Dec 13, 2023

Failing Jobs - Building c06cf1e

Status Name Step Failures Logs Raw logs Build scan
✔️ JVM Tests - JDK 17 🚧
JVM Tests - JDK 21 Build Failures Logs Raw logs 🚧
Maven Tests - JDK 17 Build Failures Logs Raw logs 🚧
✔️ Maven Tests - JDK 17 Windows 🚧
Native Tests - Data1 Build Failures Logs Raw logs 🚧

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 21 #

- Failing: extensions/vertx-http/deployment 
! Skipped: devtools/cli extensions/agroal/deployment extensions/amazon-lambda-http/deployment and 364 more

📦 extensions/vertx-http/deployment

io.quarkus.vertx.http.testrunner.includes.IncludePatternTestCase.checkTestsAreRun line 65 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Failed to wait for test run 3 State{lastRun=2, running=true, inProgress=false, run=2, passed=2, failed=0, skipped=0, isBrokenOnly=false, isTestOutput=false, isInstrumentationBasedReload=false, isLiveReload=true}
	at io.quarkus.test.ContinuousTestingTestUtils.waitForNextCompletion(ContinuousTestingTestUtils.java:44)
	at io.quarkus.vertx.http.testrunner.includes.IncludePatternTestCase.checkTestsAreRun(IncludePatternTestCase.java:65)

⚙️ Maven Tests - JDK 17 #

- Failing: integration-tests/maven 

📦 integration-tests/maven

io.quarkus.maven.it.DevMojoIT.testThatNewResourcesAreServed line 967 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: io.quarkus.maven.it.DevMojoIT expected "077fb8af-e91a-4074-bd9a-975119608bd4" 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)

io.quarkus.maven.it.DevMojoIT.testThatNewResourcesAreServed line 967 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: io.quarkus.maven.it.DevMojoIT expected "077fb8af-e91a-4074-bd9a-975119608bd4" 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)

⚙️ Native Tests - Data1 #

- Failing: integration-tests/hibernate-orm-tenancy/connection-resolver 

📦 integration-tests/hibernate-orm-tenancy/connection-resolver

io.quarkus.it.hibernate.multitenancy.fruit.HibernateTenancyFunctionalityInGraalITCase.testAddDeleteDefaultTenant - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Expected status code is <201> but was <500>.

io.quarkus.it.hibernate.multitenancy.fruit.HibernateTenancyFunctionalityInGraalITCase.testGetFruitsDefaultTenant - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Expected status code is <200> but was <500>.

io.quarkus.it.hibernate.multitenancy.fruit.HibernateTenancyFunctionalityInGraalITCase.testGetFruitsTenantMycompany - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Expected status code is <200> but was <500>.

io.quarkus.it.hibernate.multitenancy.fruit.HibernateTenancyFunctionalityInGraalITCase.testPostFruitDefaultTenant - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Expected status code is <201> but was <500>.

io.quarkus.it.hibernate.multitenancy.fruit.HibernateTenancyFunctionalityInGraalITCase.testUpdateFruitDefaultTenant - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Expected status code is <201> but was <500>.

@michalvavrik
Copy link
Member Author

Checked CI failures:

integration-tests/hibernate-orm-tenancy/connection-resolver

contains no security and it didn't fail before latest changes

IncludePatternTestCase.checkTestsAreRun

same as above plus works on JDK 17 and I can't see what is wrong from logs, it looks flaky

@sberyozkin
Copy link
Member

sberyozkin commented Dec 14, 2023

I believe we can merge, thanks everyone

@sberyozkin sberyozkin merged commit 5290dc7 into quarkusio:main Dec 14, 2023
50 of 53 checks passed
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Dec 14, 2023
@quarkus-bot quarkus-bot bot added this to the 3.7 - main milestone Dec 14, 2023
@michalvavrik michalvavrik deleted the feature/add-security-event-observer branch December 14, 2023 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

SPI for performed authorization checks
5 participants