Skip to content

Commit

Permalink
Merge pull request #42917 from sberyozkin/oidc_code_flow_error_message
Browse files Browse the repository at this point in the history 
Include AuthenticationCompletionException messages in HTTP response in devmode
  • Loading branch information
@sberyozkin
sberyozkin authored Sep 2, 2024
2 parents 43f73ea + 74ee216 commit 644cc9c
Show file tree
Hide file tree
Showing 8 changed files with 80 additions and 26 deletions.
17 changes: 17 additions & 0 deletions ...c/deployment/src/test/java/io/quarkus/oidc/test/CodeFlowDevModeDefaultTenantTestCase.java
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package io.quarkus.oidc.test;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;

import java.io.IOException;
Expand Down Expand Up @@ -61,6 +62,22 @@ public void testAccessAndRefreshTokenInjectionDevMode() throws IOException, Inte
assertEquals("alice", page.getBody().asNormalizedText());

webClient.getCookieManager().clearCookies();

// Enable the invalid scope
test.modifyResourceFile("application.properties",
s -> s.replace("#quarkus.oidc.authentication.scopes=invalid-scope",
"quarkus.oidc.authentication.scopes=invalid-scope"));

try {
webClient.getPage("http://localhost:8080/protected");
fail("Exception is expected because an invalid scope is provided");
} catch (FailingHttpStatusCodeException ex) {
assertEquals(401, ex.getStatusCode());
assertTrue(ex.getResponse().getContentAsString()
.contains(
"Authorization code flow has failed, error code: invalid_scope, error description: Invalid scopes: openid invalid-scope"),
"The reason behind 401 must be returned in devmode");
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion extensions/oidc/deployment/src/test/java/io/quarkus/oidc/test/CodeFlowDevModeTestCase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ public void testAccessAndRefreshTokenInjectionDevMode() throws IOException, Inte
}
webClient.getCookieManager().clearCookies();

// Now set the correct client-id
// Now set the correct client secret
test.modifyResourceFile("application.properties", s -> s.replace("secret-from-vault-typo", "secret-from-vault"));

page = webClient.getPage("http://localhost:8080/protected");
Expand Down
3 changes: 2 additions & 1 deletion extensions/oidc/deployment/src/test/resources/application-dev-mode-default-tenant.properties
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
quarkus.oidc.client-id=quarkus-web-app
quarkus.oidc.credentials.secret=secret
#quarkus.oidc.application-type=web-app
#quarkus.oidc.authentication.scopes=invalid-scope

quarkus.log.category."org.htmlunit.javascript.host.css.CSSStyleSheet".level=FATAL
quarkus.log.category."org.htmlunit".level=ERROR

2 changes: 1 addition & 1 deletion extensions/oidc/deployment/src/test/resources/application-dev-mode.properties
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ quarkus.oidc.credentials.client-secret.provider.key=secret-from-vault-typo
quarkus.oidc.application-type=web-app
quarkus.oidc.logout.path=/protected/logout
quarkus.oidc.authentication.pkce-required=true
quarkus.log.category."org.htmlunit.javascript.host.css.CSSStyleSheet".level=FATAL
quarkus.log.category."org.htmlunit".level=ERROR
quarkus.log.category."io.quarkus.oidc.runtime.TenantConfigContext".level=DEBUG
quarkus.log.file.enable=true

Expand Down
61 changes: 40 additions & 21 deletions ...sions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/CodeAuthenticationMechanism.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -174,9 +174,10 @@ private Uni<SecurityIdentity> processRedirectFromOidc(RoutingContext context, Oi
String[] parsedStateCookieValue = COOKIE_PATTERN.split(stateCookie.getValue());
OidcUtils.removeCookie(context, oidcTenantConfig, stateCookie.getName());
if (!parsedStateCookieValue[0].equals(stateQueryParam.get(0))) {
LOG.debug("State cookie value does not match the state query parameter value, "
+ "completing the code flow with HTTP status 401");
return Uni.createFrom().failure(new AuthenticationCompletionException());
final String error = "State cookie value does not match the state query parameter value, "
+ "completing the code flow with HTTP status 401";
LOG.error(error);
return Uni.createFrom().failure(new AuthenticationCompletionException(error));
}

// State cookie is available, try to complete the code flow and start a new session
Expand Down Expand Up @@ -239,13 +240,19 @@ public Uni<SecurityIdentity> apply(TenantConfigContext tenantContext) {

});
} else {
LOG.error(
"Authentication has failed but no error handler is found, completing the code flow with HTTP status 401");
return Uni.createFrom().failure(new AuthenticationCompletionException());
final String errorMessage = """
Authorization code flow has failed, error code: %s, error description: %s.
Error handler path is not configured. Have a public JAX-RS resource listening
on a path such as '/error' and point to it with 'quarkus.oidc.authentication.error-path=/error'.
Completing the flow with HTTP status 401.
""".formatted(error, errorDescription == null ? "" : errorDescription);
LOG.error(errorMessage);
return Uni.createFrom().failure(new AuthenticationCompletionException(errorMessage));
}
} else {
LOG.error("State cookie is present but neither 'code' nor 'error' query parameter is returned");
return Uni.createFrom().failure(new AuthenticationCompletionException());
final String error = "State cookie is present but neither 'code' nor 'error' query parameter is returned";
LOG.error(error);
return Uni.createFrom().failure(new AuthenticationCompletionException(error));
}

}
Expand All @@ -272,9 +279,10 @@ private static String filterRedirect(RoutingContext context,
private Uni<SecurityIdentity> stateParamIsMissing(OidcTenantConfig oidcTenantConfig, RoutingContext context,
Map<String, Cookie> cookies, boolean multipleStateQueryParams) {
if (multipleStateQueryParams) {
LOG.warn("State query parameter can not be multi-valued if the state cookie is present");
final String error = "State query parameter can not be multi-valued if the state cookie is present";
LOG.error(error);
removeStateCookies(oidcTenantConfig, context, cookies);
return Uni.createFrom().failure(new AuthenticationCompletionException());
return Uni.createFrom().failure(new AuthenticationCompletionException(error));
}
LOG.debug("State parameter can not be empty if the state cookie is present");
return stateCookieIsNotMatched(oidcTenantConfig, context, cookies);
Expand All @@ -292,7 +300,9 @@ private Uni<SecurityIdentity> stateCookieIsNotMatched(OidcTenantConfig oidcTenan
|| context.request().path().equals(getRedirectPath(oidcTenantConfig, context))) {
if (oidcTenantConfig.authentication.failOnMissingStateParam) {
removeStateCookies(oidcTenantConfig, context, cookies);
return Uni.createFrom().failure(new AuthenticationCompletionException());
final String error = "State query parameter is missing";
LOG.error(error);
return Uni.createFrom().failure(new AuthenticationCompletionException(error));
}
if (!oidcTenantConfig.authentication.allowMultipleCodeFlows) {
removeStateCookies(oidcTenantConfig, context, cookies);
Expand Down Expand Up @@ -365,14 +375,14 @@ public Uni<? extends SecurityIdentity> apply(Throwable t) {
.hasErrorCode(ErrorCodes.EXPIRED);

if (!expired) {
logAuthenticationError(context, t);
String error = logAuthenticationError(context, t);
return removeSessionCookie(context, configContext.oidcConfig)
.replaceWith(Uni.createFrom()
.failure(t
.getCause() instanceof AuthenticationCompletionException
? t.getCause()
: new AuthenticationCompletionException(
t.getCause())));
error, t.getCause())));
}
// Token has expired, try to refresh
if (isRpInitiatedLogout(context, configContext)) {
Expand Down Expand Up @@ -896,23 +906,31 @@ public Throwable apply(Throwable tInner) {
return tInner;
}

logAuthenticationError(context, tInner);
return new AuthenticationCompletionException(tInner);
final String errorMessage = logAuthenticationError(context, tInner);
return new AuthenticationCompletionException(errorMessage, tInner);
}
});
}

});
}

private static void logAuthenticationError(RoutingContext context, Throwable t) {
final String errorMessage = errorMessage(t);
private static String logAuthenticationError(RoutingContext context, Throwable t) {
String errorMessage = null;
final boolean accessTokenFailure = context.get(OidcUtils.CODE_ACCESS_TOKEN_FAILURE) != null;
if (accessTokenFailure) {
LOG.errorf("Access token verification has failed: %s.", errorMessage);
errorMessage = """
Access token verification has failed: %s
""".formatted(errorMessage(t));
LOG.error(errorMessage);
} else {
LOG.errorf("ID token verification has failed: %s", errorMessage);
errorMessage = """
ID token verification has failed: %s
""".formatted(errorMessage(t));
LOG.error(errorMessage);
}

return errorMessage;
}

private static boolean prepareNonceForVerification(RoutingContext context, OidcTenantConfig oidcConfig,
Expand Down Expand Up @@ -1017,8 +1035,9 @@ public Uni<? extends Void> apply(Void t) {
JsonObject idTokenJson = OidcUtils.decodeJwtContent(idToken);

if (!idTokenJson.containsKey("exp") || !idTokenJson.containsKey("iat")) {
LOG.error("ID Token is required to contain 'exp' and 'iat' claims");
throw new AuthenticationCompletionException();
final String error = "ID Token is required to contain 'exp' and 'iat' claims";
LOG.error(error);
throw new AuthenticationCompletionException(error);
}
long maxAge = idTokenJson.getLong("exp") - idTokenJson.getLong("iat");
LOG.debugf("ID token is valid for %d seconds", maxAge);
Expand Down
11 changes: 10 additions & 1 deletion ...s/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/QuarkusErrorHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@
import io.netty.handler.codec.http.HttpHeaderNames;
import io.netty.handler.codec.http.HttpResponseStatus;
import io.quarkus.runtime.ErrorPageAction;
import io.quarkus.runtime.LaunchMode;
import io.quarkus.runtime.TemplateHtmlBuilder;
import io.quarkus.security.AuthenticationCompletionException;
import io.quarkus.security.AuthenticationException;
import io.quarkus.security.ForbiddenException;
import io.quarkus.security.UnauthorizedException;
Expand Down Expand Up @@ -109,7 +111,14 @@ public void accept(Throwable throwable) {
//end here as failing event makes it possible to customize response, however when proactive security is
//disabled, this should be handled elsewhere and if we get to this point bad things have happened,
//so it is better to send a response than to hang
event.response().end();

if (event.failure() instanceof AuthenticationCompletionException
&& event.failure().getMessage() != null
&& LaunchMode.current() == LaunchMode.DEVELOPMENT) {
event.response().end(event.failure().getMessage());
} else {
event.response().end();
}
return;
}

Expand Down
9 changes: 8 additions & 1 deletion ...tp/runtime/src/main/java/io/quarkus/vertx/http/runtime/security/HttpSecurityRecorder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@

import io.quarkus.arc.Arc;
import io.quarkus.arc.InstanceHandle;
import io.quarkus.runtime.LaunchMode;
import io.quarkus.runtime.RuntimeValue;
import io.quarkus.runtime.annotations.Recorder;
import io.quarkus.runtime.configuration.ConfigurationException;
Expand Down Expand Up @@ -275,7 +276,13 @@ protected void proceed(Throwable throwable) {
protected void proceed(Throwable throwable) {
//we can't fail event here as request processing has already begun (e.g. in RESTEasy Reactive)
//and extensions may have their ways to handle failures
event.end();
if (throwable instanceof AuthenticationCompletionException
&& throwable.getMessage() != null
&& LaunchMode.current() == LaunchMode.DEVELOPMENT) {
event.end(throwable.getMessage());
} else {
event.end();
}
}
});
}
Expand Down
1 change: 1 addition & 0 deletions integration-tests/oidc-code-flow/src/test/java/io/quarkus/it/keycloak/CodeFlowTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ public void testCodeFlowScopeError() throws IOException {
// response from Quarkus
webResponse = webClient.loadWebResponse(new WebRequest(endpointLocationUri.toURL()));
assertEquals(401, webResponse.getStatusCode());
assertEquals("", webResponse.getContentAsString(), "The reason behind 401 can only be returned in devmode");
webClient.getCookieManager().clearCookies();
}
}
Expand Down

0 comments on commit 644cc9c

Please sign in to comment.