Skip to content

Commit

Permalink
Fix mixing @testsecurity with HTTP request credentials
Browse files Browse the repository at this point in the history
  • Loading branch information
michalvavrik committed Jun 13, 2024
1 parent 00bf910 commit 5164af2
Show file tree
Hide file tree
Showing 9 changed files with 190 additions and 16 deletions.
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 @@ -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,

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

0 comments on commit 5164af2

Please sign in to comment.