This is an automated email from the ASF dual-hosted git repository. rombert pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/sling-whiteboard.git
commit e377fff2365975a9772288a7e9531fd9a6fed82d Author: Robert Munteanu <[email protected]> AuthorDate: Thu Oct 24 23:40:15 2024 +0200 feat(oidc-rp): switch to exceptions for handling errors in user-facing flows --- org.apache.sling.servlets.oidc-rp/README.md | 18 ++++- .../oauth_client/impl/OAuthCallbackException.java | 29 ++++++++ .../oauth_client/impl/OAuthCallbackServlet.java | 85 +++++++++++----------- .../impl/OAuthEntryPointException.java | 29 ++++++++ .../oauth_client/impl/OAuthEntryPointServlet.java | 36 ++++----- .../oauth_client/impl/OAuthFlowException.java | 34 +++++++++ 6 files changed, 168 insertions(+), 63 deletions(-) diff --git a/org.apache.sling.servlets.oidc-rp/README.md b/org.apache.sling.servlets.oidc-rp/README.md index b1f300ef..1377f947 100644 --- a/org.apache.sling.servlets.oidc-rp/README.md +++ b/org.apache.sling.servlets.oidc-rp/README.md @@ -52,6 +52,22 @@ public class MySlingServlet extends OAuthEnabledSlingServlet { TODO +### Error handling + +The top-level servlets used for the OAuth flow will throw specific subclasses of ServletException. +These exceptions will return generic messages that can be displayed directly to the user and store +the actual cause in nested exception so that it is logged. + +These exceptions are: + +- `org.apache.sling.extensions.oauth_client.impl.OAuthCallbackException` +- `org.apache.sling.extensions.oauth_client.impl.OAuthEntryPointException` +- `org.apache.sling.extensions.oauth_client.impl.OAuthFlowException` (superclass) + +It is recommended that applications install specific error handlers for these exceptions. See the +[Apache Sling error handling documentation](https://sling.apache.org/documentation/the-sling-engine/errorhandling.html) +for more details. + ### Client registration Client registration is specific to each provider. When registering, note the following: @@ -231,4 +247,4 @@ The following items should be addressed for the bundle to be able to completely - handle tokens that are invalid for reasons other than expiry - revoked tokens - tokens that expired implicitly when the expiry_time is not included in the responses -- mark all APIs with `@ProviderType` during pre-1.0 releases to allow evolving the APIs easier \ No newline at end of file +- mark all APIs with `@ProviderType` during pre-1.0 releases to allow evolving the APIs easier diff --git a/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/extensions/oauth_client/impl/OAuthCallbackException.java b/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/extensions/oauth_client/impl/OAuthCallbackException.java new file mode 100644 index 00000000..de3fe982 --- /dev/null +++ b/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/extensions/oauth_client/impl/OAuthCallbackException.java @@ -0,0 +1,29 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.sling.extensions.oauth_client.impl; + +/** + * Signals that an exceptional condition has occurred while processing an OAuth callback + */ +public class OAuthCallbackException extends OAuthFlowException { + + private static final long serialVersionUID = 1L; + + public OAuthCallbackException(String userFacingMessage, Throwable cause) { + super(userFacingMessage, cause); + } +} diff --git a/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/extensions/oauth_client/impl/OAuthCallbackServlet.java b/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/extensions/oauth_client/impl/OAuthCallbackServlet.java index 904dbd82..3fd6c3c6 100644 --- a/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/extensions/oauth_client/impl/OAuthCallbackServlet.java +++ b/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/extensions/oauth_client/impl/OAuthCallbackServlet.java @@ -16,11 +16,11 @@ */ package org.apache.sling.extensions.oauth_client.impl; +import static java.lang.String.format; import static org.osgi.service.component.annotations.ReferencePolicyOption.GREEDY; import java.io.IOException; import java.net.URI; -import java.net.URISyntaxException; import java.net.URLDecoder; import java.nio.charset.StandardCharsets; import java.util.List; @@ -46,14 +46,13 @@ import org.apache.sling.servlets.annotations.SlingServletPaths; import org.osgi.service.component.annotations.Activate; import org.osgi.service.component.annotations.Component; import org.osgi.service.component.annotations.Reference; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; import com.nimbusds.oauth2.sdk.AuthorizationCode; import com.nimbusds.oauth2.sdk.AuthorizationCodeGrant; +import com.nimbusds.oauth2.sdk.AuthorizationErrorResponse; import com.nimbusds.oauth2.sdk.AuthorizationResponse; +import com.nimbusds.oauth2.sdk.ErrorResponse; import com.nimbusds.oauth2.sdk.ParseException; -import com.nimbusds.oauth2.sdk.TokenErrorResponse; import com.nimbusds.oauth2.sdk.TokenRequest; import com.nimbusds.oauth2.sdk.TokenResponse; import com.nimbusds.oauth2.sdk.auth.ClientSecretBasic; @@ -72,7 +71,6 @@ public class OAuthCallbackServlet extends SlingAllMethodsServlet { private static final long serialVersionUID = 1L; - private final Logger logger = LoggerFactory.getLogger(getClass()); private final Map<String, ClientConnection> connections; private final OAuthTokenStore tokenStore; private final OAuthStateManager stateManager; @@ -86,6 +84,20 @@ public class OAuthCallbackServlet extends SlingAllMethodsServlet { return request.getScheme() + "://" + request.getServerName() + portFragment + PATH; } + + private static String toErrorMessage(String context, ErrorResponse error) { + + String description = error.getErrorObject().getDescription(); + + StringBuilder message = new StringBuilder(); + message.append(context) + .append(": ") + .append(error.getErrorObject().getCode()); + if ( description != null ) + message.append(description); + + return message.toString(); + } @Activate public OAuthCallbackServlet(@Reference(policyOption = GREEDY) List<ClientConnection> connections, @@ -101,6 +113,7 @@ public class OAuthCallbackServlet extends SlingAllMethodsServlet { protected void doGet(SlingHttpServletRequest request, SlingHttpServletResponse response) throws ServletException, IOException { try { + StringBuffer requestURL = request.getRequestURL(); if ( request.getQueryString() != null ) requestURL.append('?').append(request.getQueryString()); @@ -108,30 +121,21 @@ public class OAuthCallbackServlet extends SlingAllMethodsServlet { AuthorizationResponse authResponse = AuthorizationResponse.parse(new URI(requestURL.toString())); Optional<OAuthState> clientState = stateManager.toOAuthState(authResponse.getState()); - if ( !clientState.isPresent() ) { - logger.debug("Failed state check: no state found in authorization response"); - response.sendError(HttpServletResponse.SC_BAD_REQUEST, "State check failed"); - return; - } + if ( !clientState.isPresent() ) + throw new IllegalStateException("Failed state check: no state found in authorization response"); Cookie stateCookie = request.getCookie(OAuthStateManager.COOKIE_NAME_REQUEST_KEY); - if ( stateCookie == null ) { - logger.debug("Failed state check: No request cookie named {} found", OAuthStateManager.COOKIE_NAME_REQUEST_KEY); - response.sendError(HttpServletResponse.SC_BAD_REQUEST, "State check failed"); - return; - } + if ( stateCookie == null ) + throw new IllegalStateException(format("Failed state check: No request cookie named '%s' found", OAuthStateManager.COOKIE_NAME_REQUEST_KEY)); String stateFromAuthServer = clientState.get().perRequestKey(); String stateFromClient = stateCookie.getValue(); - if ( ! stateFromAuthServer.equals(stateFromClient) ) { - logger.debug("Failed state check: request keys from client and server are not the same"); - response.sendError(HttpServletResponse.SC_BAD_REQUEST, "State check failed"); - return; - } + if ( ! stateFromAuthServer.equals(stateFromClient) ) + throw new IllegalStateException("Failed state check: request keys from client and server are not the same"); if ( !authResponse.indicatesSuccess() ) { - response.sendError(HttpServletResponse.SC_BAD_REQUEST, authResponse.toErrorResponse().getErrorObject().toString()); - return; + AuthorizationErrorResponse errorResponse = authResponse.toErrorResponse(); + throw new OAuthCallbackException("Authentication failed", new RuntimeException(toErrorMessage("Error in authentication response", errorResponse))); } Optional<String> redirect = Optional.ofNullable(clientState.get().redirect()); @@ -139,17 +143,12 @@ public class OAuthCallbackServlet extends SlingAllMethodsServlet { String authCode = authResponse.toSuccessResponse().getAuthorizationCode().getValue(); String desiredConnectionName = clientState.get().connectionName(); - if ( desiredConnectionName == null || desiredConnectionName.isEmpty() ) { - logger.warn("No connection found in clientState"); - response.sendError(HttpServletResponse.SC_BAD_REQUEST); - return; - } + if ( desiredConnectionName == null || desiredConnectionName.isEmpty() ) + throw new IllegalArgumentException("No connection found in clientState"); + ClientConnection connection = connections.get(desiredConnectionName); - if ( connection == null ) { - logger.debug("Requested unknown connection {}", desiredConnectionName); - response.sendError(HttpServletResponse.SC_BAD_REQUEST); - return; - } + if ( connection == null ) + throw new IllegalArgumentException(String.format("Requested unknown connection '%s'", desiredConnectionName)); ResolvedOAuthConnection conn = ResolvedOAuthConnection.resolve(connection); @@ -176,13 +175,8 @@ public class OAuthCallbackServlet extends SlingAllMethodsServlet { TokenResponse tokenResponse = TokenResponse.parse(httpResponse); - if ( !tokenResponse.indicatesSuccess() ) { - TokenErrorResponse errorResponse = tokenResponse.toErrorResponse(); - logger.warn("Error returned when trying to obtain access token : {}, {}", - errorResponse.getErrorObject().getCode(), errorResponse.getErrorObject().getDescription()); - response.sendError(HttpServletResponse.SC_BAD_REQUEST); - return; - } + if ( !tokenResponse.indicatesSuccess() ) + throw new IllegalArgumentException(toErrorMessage("Error in token response", tokenResponse.toErrorResponse())); OAuthTokens tokens = Converter.toSlingOAuthTokens(tokenResponse.toSuccessResponse().getTokens()); @@ -193,8 +187,17 @@ public class OAuthCallbackServlet extends SlingAllMethodsServlet { } else { response.sendRedirect(URLDecoder.decode(redirect.get(), StandardCharsets.UTF_8)); } - } catch (ParseException | URISyntaxException | IOException e) { - throw new ServletException(e); + + } catch (IllegalStateException e) { + throw new OAuthCallbackException("State check failed", e); + } catch (IllegalArgumentException e) { + throw new OAuthCallbackException("Internal error", e); + } catch (ParseException e) { + throw new OAuthCallbackException("Invalid invocation", e); + } catch ( OAuthCallbackException e) { + throw e; + } catch (Exception e) { + throw new OAuthCallbackException("Unknown error", e); } } } diff --git a/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/extensions/oauth_client/impl/OAuthEntryPointException.java b/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/extensions/oauth_client/impl/OAuthEntryPointException.java new file mode 100644 index 00000000..393499e8 --- /dev/null +++ b/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/extensions/oauth_client/impl/OAuthEntryPointException.java @@ -0,0 +1,29 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.sling.extensions.oauth_client.impl; + +/** + * Signals that an exceptional condition has occurred while starting the OAuth flow + */ +public class OAuthEntryPointException extends OAuthFlowException { + + private static final long serialVersionUID = 1L; + + public OAuthEntryPointException(String userFacingMessage, Throwable cause) { + super(userFacingMessage, cause); + } +} diff --git a/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/extensions/oauth_client/impl/OAuthEntryPointServlet.java b/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/extensions/oauth_client/impl/OAuthEntryPointServlet.java index 31a24258..1295f4c9 100644 --- a/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/extensions/oauth_client/impl/OAuthEntryPointServlet.java +++ b/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/extensions/oauth_client/impl/OAuthEntryPointServlet.java @@ -28,7 +28,6 @@ import java.util.stream.Collectors; import javax.servlet.Servlet; import javax.servlet.ServletException; import javax.servlet.http.Cookie; -import javax.servlet.http.HttpServletResponse; import org.apache.sling.api.SlingHttpServletRequest; import org.apache.sling.api.SlingHttpServletResponse; @@ -39,8 +38,6 @@ import org.apache.sling.servlets.annotations.SlingServletPaths; import org.osgi.service.component.annotations.Activate; import org.osgi.service.component.annotations.Component; import org.osgi.service.component.annotations.Reference; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; import com.nimbusds.oauth2.sdk.AuthorizationRequest; import com.nimbusds.oauth2.sdk.ResponseType; @@ -63,12 +60,9 @@ public class OAuthEntryPointServlet extends SlingAllMethodsServlet { private static final int COOKIE_MAX_AGE_SECONDS = 300; public static final String PATH = "/system/sling/oauth/entry-point"; // NOSONAR - private final Logger logger = LoggerFactory.getLogger(getClass()); - private final Map<String, ClientConnection> connections; private final OAuthStateManager stateManager; - @Activate public OAuthEntryPointServlet(@Reference(policyOption = GREEDY) List<ClientConnection> connections, @Reference OAuthStateManager stateManager) { @@ -81,23 +75,23 @@ public class OAuthEntryPointServlet extends SlingAllMethodsServlet { protected void doGet(SlingHttpServletRequest request, SlingHttpServletResponse response) throws ServletException, IOException { - String desiredConnectionName = request.getParameter("c"); - if ( desiredConnectionName == null ) { - logger.debug("Missing mandatory request parameter 'c'"); - response.sendError(HttpServletResponse.SC_BAD_REQUEST); - return; - } + try { + String desiredConnectionName = request.getParameter("c"); + if ( desiredConnectionName == null ) + throw new IllegalArgumentException("Missing mandatory request parameter 'c'"); - ClientConnection connection = connections.get(desiredConnectionName); - if ( connection == null ) { - logger.debug("Client requested unknown connection {}; known: {}", desiredConnectionName, connections.keySet()); - response.sendError(HttpServletResponse.SC_BAD_REQUEST); - return; + ClientConnection connection = connections.get(desiredConnectionName); + if ( connection == null ) + throw new IllegalArgumentException(String.format("Client requested unknown connection '%s'; known: '%s'", desiredConnectionName, connections.keySet())); + + var redirect = getAuthenticationRequestUri(connection, request, URI.create(OAuthCallbackServlet.getCallbackUri(request))); + response.addCookie(redirect.cookie()); + response.sendRedirect(redirect.uri().toString()); + } catch (IllegalArgumentException e) { + throw new OAuthEntryPointException("Invalid invocation", e); + } catch (Exception e) { + throw new OAuthEntryPointException("Internal error", e); } - - var redirect = getAuthenticationRequestUri(connection, request, URI.create(OAuthCallbackServlet.getCallbackUri(request))); - response.addCookie(redirect.cookie()); - response.sendRedirect(redirect.uri().toString()); } private RedirectTarget getAuthenticationRequestUri(ClientConnection connection, SlingHttpServletRequest request, URI redirectUri) { diff --git a/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/extensions/oauth_client/impl/OAuthFlowException.java b/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/extensions/oauth_client/impl/OAuthFlowException.java new file mode 100644 index 00000000..03c52ecf --- /dev/null +++ b/org.apache.sling.servlets.oidc-rp/src/main/java/org/apache/sling/extensions/oauth_client/impl/OAuthFlowException.java @@ -0,0 +1,34 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.sling.extensions.oauth_client.impl; + +import javax.servlet.ServletException; + +/** + * Superclass for all OAuth exception classes that propagate as servlet exceptions + * + * <p>Its intent it is simplify exception handling for implementors by allowing them to install a + * single error handler</p> + */ +public class OAuthFlowException extends ServletException { + + private static final long serialVersionUID = 1L; + + public OAuthFlowException(String userFacingMessage, Throwable rootCause) { + super(userFacingMessage, rootCause); + } +} \ No newline at end of file
