From 26732c90a0ca24affc3d118fc008d248d6b495f3 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 12 Jul 2022 12:40:45 +1000 Subject: [PATCH] Issue #8216 - Use HttpServletRequest.logout() for openid end_session_endpoint redirect Signed-off-by: Lachlan Roberts --- .../security/openid/OpenIdAuthenticator.java | 164 +++++++----------- .../security/openid/OpenIdConfiguration.java | 27 +-- 2 files changed, 69 insertions(+), 122 deletions(-) diff --git a/jetty-openid/src/main/java/org/eclipse/jetty/security/openid/OpenIdAuthenticator.java b/jetty-openid/src/main/java/org/eclipse/jetty/security/openid/OpenIdAuthenticator.java index cd4804eecdd..9c9e2ad78f5 100644 --- a/jetty-openid/src/main/java/org/eclipse/jetty/security/openid/OpenIdAuthenticator.java +++ b/jetty-openid/src/main/java/org/eclipse/jetty/security/openid/OpenIdAuthenticator.java @@ -16,7 +16,6 @@ package org.eclipse.jetty.security.openid; import java.io.IOException; import java.io.Serializable; import java.math.BigInteger; -import java.net.URLEncoder; import java.nio.charset.StandardCharsets; import java.security.SecureRandom; import java.util.LinkedHashMap; @@ -69,6 +68,7 @@ public class OpenIdAuthenticator extends LoginAuthenticator public static final String RESPONSE = "org.eclipse.jetty.security.openid.response"; public static final String ISSUER = "org.eclipse.jetty.security.openid.issuer"; public static final String REDIRECT_PATH = "org.eclipse.jetty.security.openid.redirect_path"; + public static final String LOGOUT_REDIRECT_PATH = "org.eclipse.jetty.security.openid.logout_redirect_path"; public static final String ERROR_PAGE = "org.eclipse.jetty.security.openid.error_page"; public static final String J_URI = "org.eclipse.jetty.security.openid.URI"; public static final String J_POST = "org.eclipse.jetty.security.openid.POST"; @@ -76,8 +76,6 @@ public class OpenIdAuthenticator extends LoginAuthenticator public static final String J_SECURITY_CHECK = "/j_security_check"; public static final String ERROR_PARAMETER = "error_description_jetty"; private static final String CSRF_MAP = "org.eclipse.jetty.security.openid.csrf_map"; - public static final String LOGOUT_PAGE = "org.eclipse.jetty.security.openid.logout_page"; - public static final String REDIRECT_PATH_AFTER_LOGOUT = "org.eclipse.jetty.security.openid.redirect_after_logout"; @Deprecated public static final String CSRF_TOKEN = "org.eclipse.jetty.security.openid.csrf_token"; @@ -85,16 +83,15 @@ public class OpenIdAuthenticator extends LoginAuthenticator private final SecureRandom _secureRandom = new SecureRandom(); private OpenIdConfiguration _openIdConfiguration; private String _redirectPath; + private String _logoutRedirectPath = "/"; private String _errorPage; private String _errorPath; private String _errorQuery; private boolean _alwaysSaveUri; - private String _logoutPage; - private String _redirectPathAfterLogout; public OpenIdAuthenticator() { - this(null, J_SECURITY_CHECK, (String) null); + this(null, J_SECURITY_CHECK, null); } public OpenIdAuthenticator(OpenIdConfiguration configuration) @@ -106,32 +103,20 @@ public class OpenIdAuthenticator extends LoginAuthenticator { this(configuration, J_SECURITY_CHECK, errorPage); } - - public OpenIdAuthenticator(String errorPage, String logoutPage, OpenIdConfiguration configuration) - { - this(configuration, J_SECURITY_CHECK, errorPage, logoutPage, null); - } - - public OpenIdAuthenticator(String errorPage, String logoutPage, String redirectAfterLogout, OpenIdConfiguration configuration) - { - this(configuration, J_SECURITY_CHECK, errorPage, logoutPage, redirectAfterLogout); - } public OpenIdAuthenticator(OpenIdConfiguration configuration, String redirectPath, String errorPage) { - this(configuration, redirectPath, errorPage, null, null); + this(configuration, redirectPath, errorPage, null); } - public OpenIdAuthenticator(OpenIdConfiguration configuration, String redirectPath, String errorPage, - String logoutPage, String redirectPathAfterLogout) + public OpenIdAuthenticator(OpenIdConfiguration configuration, String redirectPath, String errorPage, String logoutRedirectPath) { _openIdConfiguration = configuration; setRedirectPath(redirectPath); if (errorPage != null) setErrorPage(errorPage); - - _logoutPage = logoutPage; - _redirectPathAfterLogout = redirectPathAfterLogout; + if (logoutRedirectPath != null) + setLogoutRedirectPath(logoutRedirectPath); } @Override @@ -147,19 +132,15 @@ public class OpenIdAuthenticator extends LoginAuthenticator String redirectPath = authConfig.getInitParameter(REDIRECT_PATH); if (redirectPath != null) - _redirectPath = redirectPath; + setRedirectPath(redirectPath); String error = authConfig.getInitParameter(ERROR_PAGE); if (error != null) setErrorPage(error); - String logout = authConfig.getInitParameter(LOGOUT_PAGE); + String logout = authConfig.getInitParameter(LOGOUT_REDIRECT_PATH); if (logout != null) - _logoutPage = logout; - - String redirectAfterLogoutPath = authConfig.getInitParameter(REDIRECT_PATH_AFTER_LOGOUT); - if (redirectAfterLogoutPath != null) - _redirectPathAfterLogout = redirectAfterLogoutPath; + setLogoutRedirectPath(logout); super.setConfiguration(new OpenIdAuthConfiguration(_openIdConfiguration, authConfig)); } @@ -198,6 +179,22 @@ public class OpenIdAuthenticator extends LoginAuthenticator _redirectPath = redirectPath; } + public void setLogoutRedirectPath(String logoutRedirectPath) + { + if (logoutRedirectPath == null) + { + LOG.warn("redirect path must not be null, defaulting to /"); + logoutRedirectPath = "/"; + } + else if (!logoutRedirectPath.startsWith("/")) + { + LOG.warn("redirect path must start with /"); + logoutRedirectPath = "/" + logoutRedirectPath; + } + + _logoutRedirectPath = logoutRedirectPath; + } + public void setErrorPage(String path) { if (path == null || path.trim().length() == 0) @@ -250,6 +247,7 @@ public class OpenIdAuthenticator extends LoginAuthenticator @Override public void logout(ServletRequest request) { + attemptLogoutRedirect(request); super.logout(request); HttpServletRequest httpRequest = (HttpServletRequest)request; HttpSession session = httpRequest.getSession(false); @@ -265,6 +263,43 @@ public class OpenIdAuthenticator extends LoginAuthenticator } } + private void attemptLogoutRedirect(ServletRequest request) + { + try + { + Request baseRequest = Objects.requireNonNull(Request.getBaseRequest(request)); + Response baseResponse = baseRequest.getResponse(); + String endSessionEndpoint = _openIdConfiguration.getEndSessionEndpoint(); + if (endSessionEndpoint == null) + return; + + StringBuilder redirectUri = new StringBuilder(128); + URIUtil.appendSchemeHostPort(redirectUri, request.getScheme(), request.getServerName(), request.getServerPort()); + redirectUri.append(baseRequest.getContextPath()); + redirectUri.append(_logoutRedirectPath); + + HttpSession session = baseRequest.getSession(false); + if (session == null) + return; + + Object openIdResponse = session.getAttribute(OpenIdAuthenticator.RESPONSE); + if (openIdResponse instanceof Map) + { + @SuppressWarnings("rawtypes") + String idToken = (String)((Map)openIdResponse).get("id_token"); + + baseResponse.sendRedirect(endSessionEndpoint + + "?id_token_hint=" + UrlEncoded.encodeString(idToken, StandardCharsets.UTF_8) + + "&post_logout_redirect_uri=" + UrlEncoded.encodeString(redirectUri.toString(), StandardCharsets.UTF_8), + true); + } + } + catch (Throwable t) + { + LOG.warn("failed to redirect to end_session_endpoint", t); + } + } + @Override public void prepareRequest(ServletRequest request) { @@ -446,10 +481,6 @@ public class OpenIdAuthenticator extends LoginAuthenticator } if (LOG.isDebugEnabled()) LOG.debug("auth {}", authentication); - - if (isLogoutPage(request.getRequestURI()) && isAuthenticatedUser(authentication)) - return this.logout(request, response); - return authentication; } @@ -574,73 +605,6 @@ public class OpenIdAuthenticator extends LoginAuthenticator { return req.isSecure(); } - - public String getEndSessionEndpoint() - { - return _openIdConfiguration.getEndSessionEndpoint(); - } - - public boolean isLogoutPage(String contextPath) - { - return _logoutPage != null && (_logoutPage.equals(contextPath)); - } - - protected boolean isAuthenticatedUser(Authentication authentication) { - return authentication != null && authentication instanceof Authentication.User - && ((Authentication.User) authentication).getUserIdentity() != null; - } - - protected Authentication logout(HttpServletRequest request, HttpServletResponse response) throws ServerAuthException { - try - { - if (_redirectPathAfterLogout == null || (_redirectPathAfterLogout != null && _redirectPathAfterLogout.isEmpty())) - { - final Request baseRequest = Objects.requireNonNull(Request.getBaseRequest(request)); - final Response baseResponse = baseRequest.getResponse(); - // simple redirect without id_token_hint - baseResponse.sendRedirect(getEndSessionEndpoint(), true); - } else - { - // redirect with id_token_hint - OpenIdAuthenticator.logout(request, response, getEndSessionEndpoint(), _redirectPathAfterLogout); - } - - return Authentication.SEND_CONTINUE; - } catch (IOException e) - { - throw new ServerAuthException(e); - } - } - - public static void logout(HttpServletRequest request, HttpServletResponse response, String endSessionEndpoint, String redirectPath) throws IOException { - HttpSession session = request != null? request.getSession(false): null; - - if (request != null && response != null && endSessionEndpoint != null) - { - final Request baseRequest = Objects.requireNonNull(Request.getBaseRequest(request)); - final Response baseResponse = baseRequest.getResponse(); - - StringBuffer redirectUri = new StringBuffer(); - if (redirectPath != null && !redirectPath.isEmpty()) - { - URIUtil.appendSchemeHostPort(redirectUri, request.getScheme(), request.getServerName(), request.getServerPort()); - redirectUri.append(redirectPath); - } - - if (session != null) - { - Object openIdResponse = session.getAttribute(OpenIdAuthenticator.RESPONSE); - if (openIdResponse != null && openIdResponse instanceof Map) - { - Map responseMap = (Map) openIdResponse; - String idToken = (String) responseMap.get("id_token"); - - baseResponse.sendRedirect(endSessionEndpoint + "?id_token_hint=" + URLEncoder.encode(idToken, StandardCharsets.UTF_8.toString()) - + (redirectPath != null? ("&post_logout_redirect_uri=" + URLEncoder.encode(redirectUri.toString(), StandardCharsets.UTF_8.toString())): "" ), true); - } - } - } - } private UriRedirectInfo removeAndClearCsrfMap(HttpSession session, String csrf) { diff --git a/jetty-openid/src/main/java/org/eclipse/jetty/security/openid/OpenIdConfiguration.java b/jetty-openid/src/main/java/org/eclipse/jetty/security/openid/OpenIdConfiguration.java index d6d11666233..141c872d8fb 100644 --- a/jetty-openid/src/main/java/org/eclipse/jetty/security/openid/OpenIdConfiguration.java +++ b/jetty-openid/src/main/java/org/eclipse/jetty/security/openid/OpenIdConfiguration.java @@ -78,26 +78,9 @@ public class OpenIdConfiguration extends ContainerLifeCycle public OpenIdConfiguration(String issuer, String authorizationEndpoint, String tokenEndpoint, String clientId, String clientSecret, HttpClient httpClient) { - this(issuer, authorizationEndpoint, tokenEndpoint, null, clientId, clientSecret, "client_secret_post", httpClient); + this(issuer, authorizationEndpoint, tokenEndpoint, clientId, clientSecret, "client_secret_post", httpClient); } - - /** - * Create an OpenID configuration for a specific OIDC provider. - * @param issuer The URL of the OpenID provider. - * @param authorizationEndpoint the URL of the OpenID provider's authorization endpoint if configured. - * @param tokenEndpoint the URL of the OpenID provider's token endpoint if configured. - * @param endSessionEndpoint the URL of the OpdnID provider's end session endpoint if configured. - * @param httpClient The {@link HttpClient} instance to use. - * @param clientId OAuth 2.0 Client Identifier valid at the Authorization Server. - * @param clientSecret The client secret known only by the Client and the Authorization Server. - */ - public OpenIdConfiguration(String issuer, String authorizationEndpoint, String tokenEndpoint, String endSesseionEndpoint, - HttpClient httpClient, String clientId, String clientSecret) - { - this(issuer, authorizationEndpoint, tokenEndpoint, endSesseionEndpoint, clientId, clientSecret, "client_secret_post", - httpClient); - } - + /** * Create an OpenID configuration for a specific OIDC provider. * @param issuer The URL of the OpenID provider. @@ -116,7 +99,7 @@ public class OpenIdConfiguration extends ContainerLifeCycle @Name("authMethod") String authMethod, @Name("httpClient") HttpClient httpClient) { - this(issuer, authorizationEndpoint, tokenEndpoint, null, clientId, clientSecret, authMethod, httpClient); + this(issuer, authorizationEndpoint, tokenEndpoint, null, clientId, clientSecret, authorizationEndpoint, httpClient); } /** @@ -146,7 +129,7 @@ public class OpenIdConfiguration extends ContainerLifeCycle this.endSessionEndpoint = endSessionEndpoint; this.tokenEndpoint = tokenEndpoint; this.httpClient = httpClient != null ? httpClient : newHttpClient(); - this.authMethod = authMethod; + this.authMethod = authMethod == null ? "client_secret_post" : authMethod; if (this.issuer == null) throw new IllegalArgumentException("Issuer was not configured"); @@ -185,7 +168,7 @@ public class OpenIdConfiguration extends ContainerLifeCycle endSessionEndpoint = (String)discoveryDocument.get("end_session_endpoint"); if (endSessionEndpoint == null) throw new IllegalArgumentException("end_session_endpoint"); - + // We are lenient and not throw here as some major OIDC providers do not conform to this. if (!Objects.equals(discoveryDocument.get(ISSUER), issuer)) LOG.warn("The issuer in the metadata is not correct.");