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 mixing of the @TestSecurity annotation with HTTP request credentials inside one test method #41174

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions docs/src/main/asciidoc/security-testing.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -136,9 +136,46 @@
=== Mixing security tests

If it becomes necessary to test security features using both `@TestSecurity` and Basic Auth (which is the fallback auth
mechanism when none is defined), then Basic Auth needs to be enabled explicitly,

Check warning on line 139 in docs/src/main/asciidoc/security-testing.adoc

View workflow job for this annotation

GitHub Actions / Linting with Vale

[vale] reported by reviewdog 🐶 [Quarkus.CaseSensitiveTerms] Use 'Basic HTTP authentication (first instance)' or 'Basic authentication' rather than 'Basic Auth'. Raw Output: {"message": "[Quarkus.CaseSensitiveTerms] Use 'Basic HTTP authentication (first instance)' or 'Basic authentication' rather than 'Basic Auth'.", "location": {"path": "docs/src/main/asciidoc/security-testing.adoc", "range": {"start": {"line": 139, "column": 28}}}, "severity": "INFO"}

Check warning on line 139 in docs/src/main/asciidoc/security-testing.adoc

View workflow job for this annotation

GitHub Actions / Linting with Vale

[vale] reported by reviewdog 🐶 [Quarkus.TermsWarnings] Consider using 'Basic HTTP authentication (first instance)' or 'Basic authentication' rather than 'Basic Auth' unless updating existing content that uses the term. Raw Output: {"message": "[Quarkus.TermsWarnings] Consider using 'Basic HTTP authentication (first instance)' or 'Basic authentication' rather than 'Basic Auth' unless updating existing content that uses the term.", "location": {"path": "docs/src/main/asciidoc/security-testing.adoc", "range": {"start": {"line": 139, "column": 28}}}, "severity": "WARNING"}

Check warning on line 139 in docs/src/main/asciidoc/security-testing.adoc

View workflow job for this annotation

GitHub Actions / Linting with Vale

[vale] reported by reviewdog 🐶 [Quarkus.Fluff] Depending on the context, consider using 'Rewrite the sentence, or use 'must', instead of' rather than 'needs to'. Raw Output: {"message": "[Quarkus.Fluff] Depending on the context, consider using 'Rewrite the sentence, or use 'must', instead of' rather than 'needs to'.", "location": {"path": "docs/src/main/asciidoc/security-testing.adoc", "range": {"start": {"line": 139, "column": 39}}}, "severity": "INFO"}
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,

Check warning on line 142 in docs/src/main/asciidoc/security-testing.adoc

View workflow job for this annotation

GitHub Actions / Linting with Vale

[vale] reported by reviewdog 🐶 [Quarkus.TermsSuggestions] Depending on the context, consider using 'by using' or 'that uses' rather than 'using'. Raw Output: {"message": "[Quarkus.TermsSuggestions] Depending on the context, consider using 'by using' or 'that uses' rather than 'using'.", "location": {"path": "docs/src/main/asciidoc/security-testing.adoc", "range": {"start": {"line": 142, "column": 50}}}, "severity": "INFO"}

Check warning on line 142 in docs/src/main/asciidoc/security-testing.adoc

View workflow job for this annotation

GitHub Actions / Linting with Vale

[vale] reported by reviewdog 🐶 [Quarkus.TermsWarnings] Consider using 'use' rather than 'leverage' unless updating existing content that uses the term. Raw Output: {"message": "[Quarkus.TermsWarnings] Consider using 'use' rather than 'leverage' unless updating existing content that uses the term.", "location": {"path": "docs/src/main/asciidoc/security-testing.adoc", "range": {"start": {"line": 142, "column": 119}}}, "severity": "WARNING"}
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].
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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));
}

Expand Down
Original file line number Diff line number Diff line change
@@ -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() {
Expand Down Expand Up @@ -43,6 +57,7 @@ public void testBasicAuthentication() {
.statusCode(401);
}

@TestHTTPEndpoint(MultipleAuthMechResource.class)
@TestSecurity(user = "testUser", authMechanism = "Bearer")
@Test
public void testBearerBasedAuthentication() {
Expand Down Expand Up @@ -72,6 +87,7 @@ public void testBearerBasedAuthentication() {
.statusCode(200);
}

@TestHTTPEndpoint(MultipleAuthMechResource.class)
@TestSecurity(user = "testUser", authMechanism = "custom")
@Test
public void testCustomAuthentication() {
Expand Down Expand Up @@ -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"));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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() {
Expand Down Expand Up @@ -54,11 +52,6 @@ public Uni<HttpCredentialTransport> 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;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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 {

}
Original file line number Diff line number Diff line change
@@ -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.
* <p>
* 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<SecurityIdentity> 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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}
}
Expand Down