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.

Sign up for GitHub

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

Optimize OIDC code flow to do one less redirect when the original path has to be restored #12857

Merged
merged 1 commit into from
Nov 9, 2020
Merged

Optimize OIDC code flow to do one less redirect when the original path has to be restored #12857

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
83 changes: 30 additions & 53 deletions ...sions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/CodeAuthenticationMechanism.java
View file
Open in desktop
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");
sberyozkin marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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