From 5164af2d01afa2f7b365afcb8f0eb92e46233971 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Vav=C5=99=C3=ADk?= Date: Thu, 13 Jun 2024 19:49:25 +0200 Subject: [PATCH] Fix mixing @TestSecurity with HTTP request credentials --- docs/src/main/asciidoc/security-testing.adoc | 37 +++++++++++ .../quarkus/it/keycloak/TenantResource.java | 9 +++ .../src/main/resources/application.properties | 2 +- .../BearerTokenAuthorizationTest.java | 4 +- .../TestSecurityCombiningAuthMechTest.java | 61 ++++++++++++++++++- ...tractTestHttpAuthenticationMechanism.java} | 11 +--- ...llbackTestHttpAuthenticationMechanism.java | 13 ++++ ...hBasedTestHttpAuthenticationMechanism.java | 60 ++++++++++++++++++ .../QuarkusSecurityTestExtension.java | 9 ++- 9 files changed, 190 insertions(+), 16 deletions(-) rename test-framework/security/src/main/java/io/quarkus/test/security/{TestHttpAuthenticationMechanism.java => AbstractTestHttpAuthenticationMechanism.java} (87%) create mode 100644 test-framework/security/src/main/java/io/quarkus/test/security/FallbackTestHttpAuthenticationMechanism.java create mode 100644 test-framework/security/src/main/java/io/quarkus/test/security/PathBasedTestHttpAuthenticationMechanism.java diff --git a/docs/src/main/asciidoc/security-testing.adoc b/docs/src/main/asciidoc/security-testing.adoc index bc79dc86e6571..fa84c80d55444 100644 --- a/docs/src/main/asciidoc/security-testing.adoc +++ b/docs/src/main/asciidoc/security-testing.adoc @@ -139,6 +139,43 @@ If it becomes necessary to test security features using both `@TestSecurity` and mechanism when none is defined), then Basic Auth needs to be enabled explicitly, for example by setting `quarkus.http.auth.basic=true` or `%test.quarkus.http.auth.basic=true`. +Similarly, if it becomes necessary to test security features using both `@TestSecurity` and Bearer token authentication, +you can leverage both like in the example below: + +[source, java] +---- +@Test +@TestSecurity(user = "Bob") +public void testSecurityMetaAnnotation { + RestAssured.given() + .auth().oauth2(getTokenForUser("Alice")) <1> + .get("hello") + .then() + .statusCode(200) + .body(Matchers.is("Hello Alice")); + RestAssured.given() + .get("hello") + .then() + .statusCode(200) + .body(Matchers.is("Hello Bob")); <2> +} + +@Path("hello") +public static class HelloResource { + + @Inject + SecurityIdentity identity; + + @Authenticated + @GET + public String sayHello() { + return "Hello " + identity.getPrincipal().getName(); + } +} +---- +<1> Bearer token authentication mechanism is used, because a Bearer access token is sent with the HTTP request. +<2> No authorization header is present, therefore the Test Security Extension creates user `Bob`. + === Path-based authentication `@TestSecurity` can also be used when xref:security-authentication-mechanisms.adoc#combining-authentication-mechanisms[authentication mechanisms must be combined]. diff --git a/integration-tests/oidc-tenancy/src/main/java/io/quarkus/it/keycloak/TenantResource.java b/integration-tests/oidc-tenancy/src/main/java/io/quarkus/it/keycloak/TenantResource.java index 9d2565e7cd201..3121f0e8ed051 100644 --- a/integration-tests/oidc-tenancy/src/main/java/io/quarkus/it/keycloak/TenantResource.java +++ b/integration-tests/oidc-tenancy/src/main/java/io/quarkus/it/keycloak/TenantResource.java @@ -197,6 +197,15 @@ public String scopePermissionsWebApp2(@PathParam("tenant") String tenant) { .collect(Collectors.joining(" ")); } + @AuthorizationCodeFlow + @GET + @Path("webapp-local-logout") + @RolesAllowed("user") + public String localLogout() { + oidcSession.logout().await().indefinitely(); + return securityIdentity.getPrincipal().getName(); + } + private String getNameWebAppType(String name, String idTokenNameClaim, String idTokenNameClaimNotExpected) { diff --git a/integration-tests/oidc-tenancy/src/main/resources/application.properties b/integration-tests/oidc-tenancy/src/main/resources/application.properties index 85b6a546f18f3..11b4b1d989edd 100644 --- a/integration-tests/oidc-tenancy/src/main/resources/application.properties +++ b/integration-tests/oidc-tenancy/src/main/resources/application.properties @@ -54,7 +54,7 @@ quarkus.oidc.tenant-web-app.application-type=web-app quarkus.oidc.tenant-web-app.roles.source=userinfo quarkus.oidc.tenant-web-app.allow-user-info-cache=false # Adding this property should not affect the flow if no expected request header -# "HX-Request" identifiying it as a JavaScript request is found +# "HX-Request" identifying it as a JavaScript request is found quarkus.oidc.tenant-web-app.authentication.java-script-auto-redirect=false # Tenant Web App Java Script diff --git a/integration-tests/oidc-tenancy/src/test/java/io/quarkus/it/keycloak/BearerTokenAuthorizationTest.java b/integration-tests/oidc-tenancy/src/test/java/io/quarkus/it/keycloak/BearerTokenAuthorizationTest.java index 72d591502e151..a1e7e351e9e29 100644 --- a/integration-tests/oidc-tenancy/src/test/java/io/quarkus/it/keycloak/BearerTokenAuthorizationTest.java +++ b/integration-tests/oidc-tenancy/src/test/java/io/quarkus/it/keycloak/BearerTokenAuthorizationTest.java @@ -900,7 +900,7 @@ private String getOpaqueAccessTokenFromSimpleOidc() { return object.getString("access_token"); } - private WebClient createWebClient() { + static WebClient createWebClient() { WebClient webClient = new WebClient(); webClient.setCssErrorHandler(new SilentCssErrorHandler()); return webClient; @@ -916,7 +916,7 @@ private Cookie getStateCookie(WebClient webClient, String tenantId) { return null; } - private Cookie getSessionCookie(WebClient webClient, String tenantId) { + static Cookie getSessionCookie(WebClient webClient, String tenantId) { return webClient.getCookieManager().getCookie("q_session" + (tenantId == null ? "" : "_" + tenantId)); } diff --git a/integration-tests/oidc-tenancy/src/test/java/io/quarkus/it/keycloak/TestSecurityCombiningAuthMechTest.java b/integration-tests/oidc-tenancy/src/test/java/io/quarkus/it/keycloak/TestSecurityCombiningAuthMechTest.java index 8c1f6bf5a6579..21d5636f81f95 100644 --- a/integration-tests/oidc-tenancy/src/test/java/io/quarkus/it/keycloak/TestSecurityCombiningAuthMechTest.java +++ b/integration-tests/oidc-tenancy/src/test/java/io/quarkus/it/keycloak/TestSecurityCombiningAuthMechTest.java @@ -1,17 +1,31 @@ package io.quarkus.it.keycloak; +import static io.quarkus.it.keycloak.AnnotationBasedTenantTest.getTokenWithRole; +import static io.quarkus.it.keycloak.BearerTokenAuthorizationTest.createWebClient; +import static io.quarkus.it.keycloak.BearerTokenAuthorizationTest.getSessionCookie; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; + +import java.io.IOException; + +import org.hamcrest.Matchers; +import org.htmlunit.WebClient; +import org.htmlunit.html.HtmlForm; +import org.htmlunit.html.HtmlPage; import org.junit.jupiter.api.Test; +import io.quarkus.test.common.QuarkusTestResource; import io.quarkus.test.common.http.TestHTTPEndpoint; import io.quarkus.test.junit.QuarkusTest; import io.quarkus.test.security.TestSecurity; import io.restassured.RestAssured; import io.restassured.http.ContentType; -@TestHTTPEndpoint(MultipleAuthMechResource.class) @QuarkusTest +@QuarkusTestResource(KeycloakRealmResourceManager.class) public class TestSecurityCombiningAuthMechTest { + @TestHTTPEndpoint(MultipleAuthMechResource.class) @TestSecurity(user = "testUser", authMechanism = "basic") @Test public void testBasicAuthentication() { @@ -43,6 +57,7 @@ public void testBasicAuthentication() { .statusCode(401); } + @TestHTTPEndpoint(MultipleAuthMechResource.class) @TestSecurity(user = "testUser", authMechanism = "Bearer") @Test public void testBearerBasedAuthentication() { @@ -72,6 +87,7 @@ public void testBearerBasedAuthentication() { .statusCode(200); } + @TestHTTPEndpoint(MultipleAuthMechResource.class) @TestSecurity(user = "testUser", authMechanism = "custom") @Test public void testCustomAuthentication() { @@ -102,4 +118,47 @@ public void testCustomAuthentication() { .then() .statusCode(401); } + + @TestHTTPEndpoint(TenantEchoResource.class) + @TestSecurity(user = "testUser", authMechanism = "Bearer", roles = "role1") + @Test + public void testHttpCredentialsHasPriorityOverTestSecurity() { + // token has priority over @TestSecurity + RestAssured.given().auth().oauth2(getTokenWithRole("role1")) + .when().get("jax-rs-perm-check") + .then().statusCode(200) + .body(Matchers.equalTo(("tenant-id=tenant-public-key, static.tenant.id=tenant-public-key, name=alice"))); + // no token -> use @TestSecurity + RestAssured.given() + .when().get("jax-rs-perm-check") + .then().statusCode(200) + .body(Matchers.equalTo(("tenant-id=tenant-public-key, static.tenant.id=null, name=testUser"))); + } + + @TestSecurity(user = "testUser", authMechanism = "Bearer", roles = "role1") + @Test + public void testSessionCookieHasPriorityOverTestSecurity() throws IOException { + // @TestSecurity still use Bearer authentication as we didn't specify credentials + RestAssured.given() + .redirects().follow(false) + .when().get("/tenant/tenant-web-app/api/user/webapp-local-logout") + .then().statusCode(302); + RestAssured.given() + .when().get("/api/tenant-echo/jax-rs-perm-check") + .then().statusCode(200) + .body(Matchers.equalTo(("tenant-id=tenant-public-key, static.tenant.id=null, name=testUser"))); + + // path specific authentication is still possible, the @TestSecurity is not used as it uses Bearer, not code + try (final WebClient webClient = createWebClient()) { + HtmlPage page = webClient.getPage("http://localhost:8081/tenant/tenant-web-app/api/user/webapp-local-logout"); + assertNull(getSessionCookie(webClient, "tenant-web-app")); + assertEquals("Sign in to quarkus-webapp", page.getTitleText()); + HtmlForm loginForm = page.getForms().get(0); + loginForm.getInputByName("username").setValueAttribute("alice"); + loginForm.getInputByName("password").setValueAttribute("alice"); + page = loginForm.getInputByName("login").click(); + assertEquals("alice", page.getBody().asNormalizedText()); + assertNull(getSessionCookie(webClient, "tenant-web-app")); + } + } } diff --git a/test-framework/security/src/main/java/io/quarkus/test/security/TestHttpAuthenticationMechanism.java b/test-framework/security/src/main/java/io/quarkus/test/security/AbstractTestHttpAuthenticationMechanism.java similarity index 87% rename from test-framework/security/src/main/java/io/quarkus/test/security/TestHttpAuthenticationMechanism.java rename to test-framework/security/src/main/java/io/quarkus/test/security/AbstractTestHttpAuthenticationMechanism.java index 1bf6a853d50d6..1359b83e041f7 100644 --- a/test-framework/security/src/main/java/io/quarkus/test/security/TestHttpAuthenticationMechanism.java +++ b/test-framework/security/src/main/java/io/quarkus/test/security/AbstractTestHttpAuthenticationMechanism.java @@ -4,7 +4,6 @@ import java.util.Set; import jakarta.annotation.PostConstruct; -import jakarta.enterprise.context.ApplicationScoped; import jakarta.inject.Inject; import io.quarkus.runtime.LaunchMode; @@ -17,13 +16,12 @@ import io.smallrye.mutiny.Uni; import io.vertx.ext.web.RoutingContext; -@ApplicationScoped -public class TestHttpAuthenticationMechanism implements HttpAuthenticationMechanism { +abstract class AbstractTestHttpAuthenticationMechanism implements HttpAuthenticationMechanism { @Inject TestIdentityAssociation testIdentityAssociation; - volatile String authMechanism = null; + protected volatile String authMechanism = null; @PostConstruct public void check() { @@ -54,11 +52,6 @@ public Uni getCredentialTransport(RoutingContext contex : Uni.createFrom().item(new HttpCredentialTransport(HttpCredentialTransport.Type.TEST_SECURITY, authMechanism)); } - @Override - public int getPriority() { - return 3000; - } - void setAuthMechanism(String authMechanism) { this.authMechanism = authMechanism; } diff --git a/test-framework/security/src/main/java/io/quarkus/test/security/FallbackTestHttpAuthenticationMechanism.java b/test-framework/security/src/main/java/io/quarkus/test/security/FallbackTestHttpAuthenticationMechanism.java new file mode 100644 index 0000000000000..69c79a5d758da --- /dev/null +++ b/test-framework/security/src/main/java/io/quarkus/test/security/FallbackTestHttpAuthenticationMechanism.java @@ -0,0 +1,13 @@ +package io.quarkus.test.security; + +import jakarta.enterprise.context.ApplicationScoped; + +/** + * This test mechanism is fallback when no other mechanism manages to authenticate. + * When the test method is annotated with the {@link TestSecurity} annotation, + * users can still send credentials inside HTTP request and the credentials will have priority. + */ +@ApplicationScoped +public class FallbackTestHttpAuthenticationMechanism extends AbstractTestHttpAuthenticationMechanism { + +} diff --git a/test-framework/security/src/main/java/io/quarkus/test/security/PathBasedTestHttpAuthenticationMechanism.java b/test-framework/security/src/main/java/io/quarkus/test/security/PathBasedTestHttpAuthenticationMechanism.java new file mode 100644 index 0000000000000..fac1f7d467181 --- /dev/null +++ b/test-framework/security/src/main/java/io/quarkus/test/security/PathBasedTestHttpAuthenticationMechanism.java @@ -0,0 +1,60 @@ +package io.quarkus.test.security; + +import static io.netty.handler.codec.http.HttpHeaderNames.AUTHORIZATION; + +import jakarta.enterprise.context.ApplicationScoped; + +import io.quarkus.security.identity.IdentityProviderManager; +import io.quarkus.security.identity.SecurityIdentity; +import io.smallrye.mutiny.Uni; +import io.vertx.core.http.Cookie; +import io.vertx.ext.web.RoutingContext; + +/** + * When authentication mechanism is selected with the {@link TestSecurity#authMechanism()} annotation attribute, + * we must be sure that the test mechanism is primary identity provider for that authentication type. + *

+ * For example when a test method is annotated with `@TestSecurity(authMechanism = "basic")`, + * we want to be the ones providing basic authentication when no authorization headers are present, + * and not the {@link io.quarkus.vertx.http.runtime.security.BasicAuthenticationMechanism} mechanism. + * This test mechanism must exist because when a path-specific authentication mechanism is selected, + * for example via {@link io.quarkus.vertx.http.runtime.security.annotation.BasicAuthentication}, + * it is also required and therefore exactly one mechanism is enforced. + */ +@ApplicationScoped +public class PathBasedTestHttpAuthenticationMechanism extends AbstractTestHttpAuthenticationMechanism { + + @Override + public Uni authenticate(RoutingContext context, IdentityProviderManager identityProviderManager) { + if (authMechanism != null && requestNotAuthenticated(context)) { + // return the SecurityIdentity defined via @TestSecurity + return super.authenticate(context, identityProviderManager); + } + // do not authenticate - give a change to other mechanisms + return Uni.createFrom().nullItem(); + } + + @Override + public int getPriority() { + return 3000; + } + + private static boolean requestNotAuthenticated(RoutingContext context) { + // on a best-effort basis try to guess whether incoming request is authorized + return context.request().getHeader(AUTHORIZATION) == null + && !hasOidcSessionCookieCandidate(context); + } + + private static boolean hasOidcSessionCookieCandidate(RoutingContext context) { + if (context.request().cookies() == null) { + return false; + } + for (Cookie cookie : context.request().cookies()) { + if (cookie.getName() != null && cookie.getName().startsWith("q_session")) { + // there is a possibility this is an OIDC session cookie + return true; + } + } + return false; + } +} diff --git a/test-framework/security/src/main/java/io/quarkus/test/security/QuarkusSecurityTestExtension.java b/test-framework/security/src/main/java/io/quarkus/test/security/QuarkusSecurityTestExtension.java index 44a0bc2750278..b574d1286b43e 100644 --- a/test-framework/security/src/main/java/io/quarkus/test/security/QuarkusSecurityTestExtension.java +++ b/test-framework/security/src/main/java/io/quarkus/test/security/QuarkusSecurityTestExtension.java @@ -26,7 +26,9 @@ public void afterEach(QuarkusTestMethodContext context) { try { if (getAnnotationContainer(context).isPresent()) { CDI.current().select(TestAuthController.class).get().setEnabled(true); - CDI.current().select(TestHttpAuthenticationMechanism.class).get().setAuthMechanism(null); + for (var testMechanism : CDI.current().select(AbstractTestHttpAuthenticationMechanism.class)) { + testMechanism.setAuthMechanism(null); + } var testIdentity = CDI.current().select(TestIdentityAssociation.class).get(); testIdentity.setTestIdentity(null); testIdentity.setPathBasedIdentity(false); @@ -66,8 +68,9 @@ public void beforeEach(QuarkusTestMethodContext context) { SecurityIdentity userIdentity = augment(user.build(), allAnnotations); CDI.current().select(TestIdentityAssociation.class).get().setTestIdentity(userIdentity); if (!testSecurity.authMechanism().isEmpty()) { - CDI.current().select(TestHttpAuthenticationMechanism.class).get() - .setAuthMechanism(testSecurity.authMechanism()); + for (var testMechanism : CDI.current().select(AbstractTestHttpAuthenticationMechanism.class)) { + testMechanism.setAuthMechanism(testSecurity.authMechanism()); + } CDI.current().select(TestIdentityAssociation.class).get().setPathBasedIdentity(true); } }