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

With disabled proactive sec., only create sec. identity when required #28061

Conversation

michalvavrik
Copy link
Contributor

fixes: #27316

When proactive security is disabled (quarkus.http.auth.proactive=false), security identity is only created (or more specifically, io.quarkus.vertx.http.runtime.security.HttpAuthenticator#attemptAuthentication is only called) when required. This behavior is expected by user (see linked issue) and was described here #27316 (comment) by @sberyozkin.

Added test io.quarkus.vertx.http.security.DisabledProactiveSecIdentityProviderTest#testAuthenticationIsNotAttempted would fail prior to this PR (tested).

@michalvavrik michalvavrik force-pushed the feature/prevent-calling-ip-with-disabled-proactive branch from daa56db to 1830cb8 Compare September 19, 2022 13:29
@quarkus-bot

This comment has been minimized.

@michalvavrik
Copy link
Contributor Author

Sorry about cancelled workflow run, I'll be more careful.

@sberyozkin sberyozkin self-requested a review September 19, 2022 17:15
@sberyozkin
Copy link
Member

Hi Stuart @stuartwdouglas I think it is the correct solution, but double check please if you get a chance

@quarkus-bot

This comment has been minimized.

@michalvavrik
Copy link
Contributor Author

io.quarkus.it.keycloak.CodeFlowTest.testTokenAutoRefresh - flaky test

@sberyozkin
Copy link
Member

I've just merged the PR which adds a few more log messages. Unfortunately something strange is going on with the refresh token tests. Can you please rebase and have this build run again ? It is most likely unrelated but this change a very sensitive change, so we need to be 100% certain, and as it happens these tests run with the proactive authentication disabled, thanks

@michalvavrik michalvavrik force-pushed the feature/prevent-calling-ip-with-disabled-proactive branch from 1830cb8 to 76f2aa4 Compare September 19, 2022 20:15
@michalvavrik
Copy link
Contributor Author

@sberyozkin sure, done, let see

@quarkus-bot
Copy link

quarkus-bot bot commented Sep 20, 2022

Failing Jobs - Building 76f2aa4

Status Name Step Failures Logs Raw logs
Devtools Tests - JDK 11 Build Failures Logs Raw logs
✔️ Devtools Tests - JDK 11 Windows
Devtools Tests - JDK 17 Build Failures Logs Raw logs
✔️ Gradle Tests - JDK 11
Gradle Tests - JDK 11 Windows Build Failures Logs Raw logs
✔️ JVM Tests - JDK 11
JVM Tests - JDK 11 Windows Build Failures Logs Raw logs
✔️ JVM Tests - JDK 17
✔️ JVM Tests - JDK 18

Full information is available in the Build summary check run.

Failures

⚙️ Devtools Tests - JDK 11 #

- Failing: integration-tests/devtools 

📦 integration-tests/devtools

io.quarkus.devtools.codestarts.quarkus.QuarkusCodestartBuildIT.testGradle(String)[3] line 81 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: 0

io.quarkus.devtools.codestarts.quarkus.QuarkusCodestartBuildIT.testGradleKotlinDSL(String)[1] line 88 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: 0

io.quarkus.devtools.codestarts.quarkus.QuarkusCodestartBuildIT.testGradle(String)[3] line 81 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: 0

io.quarkus.devtools.codestarts.quarkus.QuarkusCodestartBuildIT.testGradleKotlinDSL(String)[1] line 88 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: 0

⚙️ Devtools Tests - JDK 17 #

- Failing: integration-tests/devtools 

📦 integration-tests/devtools

io.quarkus.devtools.codestarts.quarkus.QuarkusCodestartBuildIT.testGradle(String)[3] line 81 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: 0

io.quarkus.devtools.codestarts.quarkus.QuarkusCodestartBuildIT.testGradleKotlinDSL(String)[1] line 88 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: 0

io.quarkus.devtools.codestarts.quarkus.QuarkusCodestartBuildIT.testGradle(String)[3] line 81 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: 0

io.quarkus.devtools.codestarts.quarkus.QuarkusCodestartBuildIT.testGradleKotlinDSL(String)[1] line 88 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: 0

⚙️ Gradle Tests - JDK 11 Windows #

- Failing: integration-tests/gradle 

📦 integration-tests/gradle

io.quarkus.gradle.QuarkusPluginFunctionalTest.canDetectClasspathChangeWhenBuilding line 98 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

Expecting value to be true but was false

io.quarkus.gradle.QuarkusPluginFunctionalTest.canDetectUpToDateBuild line 58 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

Expecting value to be true but was false

⚙️ JVM Tests - JDK 11 Windows #

- Failing: integration-tests/opentelemetry-reactive 

📦 integration-tests/opentelemetry-reactive

io.quarkus.it.opentelemetry.reactive.OpenTelemetryReactiveClientTest.post line 94 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: expected: <6cfc9008dc47a77e> but was: <005afaae68a80aa4>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)

@sberyozkin
Copy link
Member

@michalvavrik So that was that random test failure not related to this PR and the test failures here are not related so I think we can merge, @stuartwdouglas - ping us please if you'll have some concerns

@sberyozkin sberyozkin merged commit cfc4b66 into quarkusio:main Sep 20, 2022
@quarkus-bot quarkus-bot bot added this to the 2.14 - main milestone Sep 20, 2022
@michalvavrik michalvavrik deleted the feature/prevent-calling-ip-with-disabled-proactive branch September 20, 2022 09:29
@gsmet gsmet modified the milestones: 2.14 - main, 2.13.0.Final Sep 20, 2022
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.

quarkus.http.auth.proactive = false and @PermitAll still trigger custom auth
3 participants