Skip to content

Commit

Permalink
Merge pull request #12857 from sberyozkin/oidc_code_flow_custom_code_…
Browse files Browse the repository at this point in the history 
…query

Optimize OIDC code flow to do one less redirect when the original path has to be restored
  • Loading branch information
@gsmet
gsmet authored Nov 9, 2020
2 parents b4c1995 + 290520e commit 67fd251
Show file tree
Hide file tree
Showing 5 changed files with 72 additions and 63 deletions.
83 changes: 30 additions & 53 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 @@ -213,16 +213,17 @@ public Uni<ChallengeData> getChallenge(RoutingContext context) {

private Uni<SecurityIdentity> performCodeFlow(IdentityProviderManager identityProviderManager,
RoutingContext context, DefaultTenantConfigResolver resolver) {
JsonObject params = new JsonObject();

String code = context.request().getParam("code");
if (code == null) {
return Uni.createFrom().optional(Optional.empty());
}

TenantConfigContext configContext = resolver.resolve(context, true);

Cookie stateCookie = context.getCookie(getStateCookieName(configContext));

String userPath = null;
String userQuery = null;
if (stateCookie != null) {
List<String> values = context.queryParam("state");
Expand All @@ -233,58 +234,31 @@ private Uni<SecurityIdentity> performCodeFlow(IdentityProviderManager identityPr
} else if (!stateCookie.getValue().startsWith(values.get(0))) {
LOG.debug("State cookie value does not match the state query parameter value");
return Uni.createFrom().failure(new AuthenticationCompletionException());
} else if (context.queryParam("pathChecked").isEmpty()) {
// This is an original redirect from IDP, check if the request path needs to be updated
String[] pair = COOKIE_PATTERN.split(stateCookie.getValue());
if (pair.length == 2) {
// The extra path that needs to be added to the current request path
String extraPath = pair[1];

// The original user query if any will be added to the final redirect URI
// after the authentication has been complete

int userQueryIndex = extraPath.indexOf("?");
if (userQueryIndex != 0) {
if (userQueryIndex > 0) {
extraPath = extraPath.substring(0, userQueryIndex);
}
// Adding a query marker that the state cookie has already been used to restore the path
// as deleting it now would increase the risk of CSRF
String extraQuery = "?pathChecked=true";

// The query parameters returned from IDP need to be included
if (context.request().query() != null) {
extraQuery += ("&" + context.request().query());
}

String localRedirectUri = buildUri(context, isForceHttps(configContext), extraPath + extraQuery);
LOG.debugf("Local redirect URI: %s", localRedirectUri);
return Uni.createFrom().failure(new AuthenticationRedirectException(localRedirectUri));
} else if (userQueryIndex + 1 < extraPath.length()) {
// only the user query needs to be restored, no need to redirect
userQuery = extraPath.substring(userQueryIndex + 1);
}
}
// The original request path does not have to be restored, the state cookie is no longer needed
removeCookie(context, configContext, getStateCookieName(configContext));
} else {
// This is an original redirect from IDP, check if the original request path and query need to be restored
String[] pair = COOKIE_PATTERN.split(stateCookie.getValue());
if (pair.length == 2) {
int userQueryIndex = pair[1].indexOf("?");
if (userQueryIndex >= 0 && userQueryIndex + 1 < pair[1].length()) {
userQuery = pair[1].substring(userQueryIndex + 1);
if (userQueryIndex >= 0) {
userPath = pair[1].substring(0, userQueryIndex);
if (userQueryIndex + 1 < pair[1].length()) {
userQuery = pair[1].substring(userQueryIndex + 1);
}
} else {
userPath = pair[1];
}
}
// Local redirect restoring the original request path, the state cookie is no longer needed
removeCookie(context, configContext, getStateCookieName(configContext));
}
} else {
// State cookie must be available to minimize the risk of CSRF
LOG.debug("The state cookie is missing after a redirect from IDP");
LOG.debug("The state cookie is missing after a redirect from IDP, authentication has failed");
return Uni.createFrom().failure(new AuthenticationCompletionException());
}

// Code grant request
JsonObject params = new JsonObject();

// 'code': the code grant value returned from IDP
params.put("code", code);

Expand All @@ -303,6 +277,7 @@ private Uni<SecurityIdentity> performCodeFlow(IdentityProviderManager identityPr
params.put("client_assertion", signJwtWithClientSecret(configContext.oidcConfig));
}

final String finalUserPath = userPath;
final String finalUserQuery = userQuery;
return Uni.createFrom().emitter(new Consumer<UniEmitter<? super SecurityIdentity>>() {
@Override
Expand All @@ -326,13 +301,24 @@ public void accept(SecurityIdentity identity) {
processSuccessfulAuthentication(context, configContext, authResult.idToken(),
opaqueIdToken, opaqueAccessToken, opaqueRefreshToken, identity);

if (configContext.oidcConfig.authentication.isRemoveRedirectParameters()
&& context.request().query() != null) {
String finalRedirectUri = buildUriWithoutQueryParams(context,
isForceHttps(configContext));
boolean removeRedirectParams = configContext.oidcConfig.authentication
.isRemoveRedirectParameters();
if (removeRedirectParams || finalUserPath != null || finalUserQuery != null) {

final String scheme = isForceHttps(configContext) ? "https"
: context.request().scheme();
URI absoluteUri = URI.create(context.request().absoluteURI());
StringBuilder sb = new StringBuilder(scheme).append("://")
.append(absoluteUri.getAuthority())
.append(finalUserPath != null ? finalUserPath : absoluteUri.getRawPath());
if (!removeRedirectParams) {
sb.append('?').append(absoluteUri.getRawQuery());
}
if (finalUserQuery != null) {
finalRedirectUri += ("?" + finalUserQuery);
sb.append(!removeRedirectParams ? "" : "?");
sb.append(finalUserQuery);
}
String finalRedirectUri = sb.toString();
LOG.debugf("Final redirect URI: %s", finalRedirectUri);
uniEmitter.fail(new AuthenticationRedirectException(finalRedirectUri));
} else {
Expand Down Expand Up @@ -465,15 +451,6 @@ private String buildUri(RoutingContext context, boolean forceHttps, String path)
.toString();
}

private String buildUriWithoutQueryParams(RoutingContext context, boolean forceHttps) {
final String scheme = forceHttps ? "https" : context.request().scheme();
URI absoluteUri = URI.create(context.request().absoluteURI());
return new StringBuilder(scheme).append("://")
.append(absoluteUri.getAuthority())
.append(absoluteUri.getRawPath())
.toString();
}

private void removeCookie(RoutingContext context, TenantConfigContext configContext, String cookieName) {
ServerCookie cookie = (ServerCookie) context.cookieMap().get(cookieName);
if (cookie != null) {
Expand Down
32 changes: 26 additions & 6 deletions ...ation-tests/oidc-code-flow/src/main/java/io/quarkus/it/keycloak/CustomTenantResolver.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ public String resolve(RoutingContext context) {
return "tenant-logout";
}

if (path.contains("tenant-query")) {
return "tenant-query";
}

if (path.contains("tenant-listener")) {
return "tenant-listener";
}
Expand Down Expand Up @@ -51,11 +55,27 @@ public String resolve(RoutingContext context) {
return "tenant-javascript";
}

return path.contains("callback-after-redirect") || path.contains("callback-before-redirect") ? "tenant-1"
: path.contains("callback-jwt-after-redirect") || path.contains("callback-jwt-before-redirect") ? "tenant-jwt"
: path.contains("callback-jwt-not-used-after-redirect")
|| path.contains("callback-jwt-not-used-before-redirect")
? "tenant-jwt-not-used"
: path.contains("/web-app2") ? "tenant-2" : null;
if (path.contains("callback-before-wrong-redirect")) {
return context.getCookie("q_auth_tenant-before-wrong-redirect") == null ? "tenant-before-wrong-redirect"
: "tenant-1";
}

if (path.contains("callback-after-redirect") || path.contains("callback-before-redirect")) {
return "tenant-1";
}

if (path.contains("callback-jwt-after-redirect") || path.contains("callback-jwt-before-redirect")) {
return "tenant-jwt";
}

if (path.contains("callback-jwt-not-used-after-redirect") || path.contains("callback-jwt-not-used-before-redirect")) {
return "tenant-jwt-not-used";
}

if (path.contains("/web-app2")) {
return "tenant-2";
}

return null;
}
}
6 changes: 6 additions & 0 deletions integration-tests/oidc-code-flow/src/main/java/io/quarkus/it/keycloak/ProtectedResource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,12 @@ public String getNameSplitTokens() {
return "tenant-split-tokens:" + getName();
}

@GET
@Path("callback-before-wrong-redirect")
public String getNameCallbackBeforeWrongRedirect() {
throw new InternalServerErrorException("This method must not be invoked");
}

@GET
@Path("callback-before-redirect")
public String getNameCallbackBeforeRedirect() {
Expand Down
6 changes: 6 additions & 0 deletions integration-tests/oidc-code-flow/src/main/resources/application.properties
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,12 @@ quarkus.oidc.tenant-listener.authentication.remove-redirect-parameters=false
quarkus.oidc.tenant-listener.application-type=web-app


quarkus.oidc.tenant-before-wrong-redirect.auth-server-url=${keycloak.url}/realms/quarkus
quarkus.oidc.tenant-before-wrong-redirect.client-id=quarkus-app
quarkus.oidc.tenant-before-wrong-redirect.credentials.client-secret.value=secret
quarkus.oidc.tenant-before-wrong-redirect.token.issuer=${keycloak.url}/realms/quarkus
quarkus.oidc.tenant-before-wrong-redirect.application-type=web-app

# Tenant which does not need to restore a request path after redirect, client_secret_post method
quarkus.oidc.tenant-1.auth-server-url=${keycloak.url}/realms/quarkus
quarkus.oidc.tenant-1.client-id=quarkus-app
Expand Down
8 changes: 4 additions & 4 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 @@ -440,9 +440,9 @@ public void testIdTokenInjectionWithoutRestoredPathDifferentRoot() throws IOExce

@Test
public void testAuthenticationCompletionFailedNoStateCookie() throws IOException, InterruptedException {
// tenant-3 configuration uses a '/some/other/path' redirect parameter which does not have the same root
// tenant-3 configuration uses a '/web-app3' redirect parameter which does not have the same root
// as the original request which is 'web-app', as a result, when the user is returned back to Quarkus
// to '/some/other/path' no state cookie is detected.
// to '/web-app3' no state cookie is detected.
try (final WebClient webClient = createWebClient()) {
HtmlPage page = webClient.getPage("http://localhost:8081/web-app/callback-before-redirect?tenantId=tenant-3");
assertEquals("Log in to quarkus", page.getTitleText());
Expand All @@ -468,7 +468,7 @@ public void testAuthenticationCompletionFailedWrongRedirectUri() throws IOExcept
// When the user is redirected back, CustomTenantResolver will resolve a 'tenant-1' configuration with
// a redirect_uri '/web-app/callback-after-redirect' which will cause a code to token exchange failure
try (final WebClient webClient = createWebClient()) {
HtmlPage page = webClient.getPage("http://localhost:8081/web-app/callback-before-redirect?tenantId");
HtmlPage page = webClient.getPage("http://localhost:8081/web-app/callback-before-wrong-redirect");
assertEquals("Log in to quarkus", page.getTitleText());

HtmlForm loginForm = page.getForms().get(0);
Expand Down Expand Up @@ -644,7 +644,7 @@ public void testAccessAndRefreshTokenInjectionWithoutIndexHtmlAndListener() thro
}

@Test
public void testAccessAndRefreshTokenInjectionWithoutIndexHtmlWithQuery() throws Exception {
public void testAccessAndRefreshTokenInjectionWithQuery() throws Exception {
try (final WebClient webClient = createWebClient()) {
HtmlPage page = webClient.getPage("http://localhost:8081/web-app/refresh-query?a=aValue");
assertEquals("/web-app/refresh-query?a=aValue", getStateCookieSavedPath(webClient, null));
Expand Down

0 comments on commit 67fd251

Please sign in to comment.