From e081da00daac2fd6d1c04f0721dcf7b851145e6d Mon Sep 17 00:00:00 2001 From: Dusan Jakub Date: Tue, 19 Sep 2023 21:23:38 +0200 Subject: [PATCH] error handling split among: 1. user visible exception (directly shown to users as html) 2. redirect exception (pass back to redirect uri) 3. api exception (json) --- .../java/com/ysoft/geecon/OAuthResource.java | 37 ++++++++++++------- .../java/com/ysoft/geecon/dto/AuthParams.java | 8 ++++ .../com/ysoft/geecon/error/ErrorResponse.java | 3 +- .../ysoft/geecon/error/ExceptionMappers.java | 20 +--------- .../ysoft/geecon/error/OAuthApiException.java | 20 ++++++++++ .../ysoft/geecon/error/OAuthException.java | 19 ++++------ .../geecon/error/OAuthRedirectException.java | 27 ++++++++++++++ .../error/OAuthUserVisibleException.java | 15 ++++++++ .../com/ysoft/geecon/AuthCodeGrantTest.java | 10 +++++ .../geecon/helpers/AuthorizationCodeFlow.java | 28 ++++++++++++++ 10 files changed, 141 insertions(+), 46 deletions(-) create mode 100644 src/main/java/com/ysoft/geecon/error/OAuthApiException.java create mode 100644 src/main/java/com/ysoft/geecon/error/OAuthRedirectException.java create mode 100644 src/main/java/com/ysoft/geecon/error/OAuthUserVisibleException.java diff --git a/src/main/java/com/ysoft/geecon/OAuthResource.java b/src/main/java/com/ysoft/geecon/OAuthResource.java index 7dbf74f..dfa61c0 100644 --- a/src/main/java/com/ysoft/geecon/OAuthResource.java +++ b/src/main/java/com/ysoft/geecon/OAuthResource.java @@ -2,7 +2,9 @@ package com.ysoft.geecon; import com.ysoft.geecon.dto.*; import com.ysoft.geecon.error.ErrorResponse; -import com.ysoft.geecon.error.OAuthException; +import com.ysoft.geecon.error.OAuthApiException; +import com.ysoft.geecon.error.OAuthRedirectException; +import com.ysoft.geecon.error.OAuthUserVisibleException; import com.ysoft.geecon.repo.ClientsRepo; import com.ysoft.geecon.repo.SessionsRepo; import com.ysoft.geecon.repo.UsersRepo; @@ -30,7 +32,8 @@ public class OAuthResource { @FormParam("password") String password) { - sessionsRepo.getSession(sessionId).orElseThrow(() -> new OAuthException(ErrorResponse.Error.access_denied, "Invalid session")); + sessionsRepo.getSession(sessionId).orElseThrow( + () -> new OAuthUserVisibleException(ErrorResponse.Error.access_denied, "Invalid session")); var user = validateUser(username, password); if (user == null) { return Templates.login(username, sessionId, "invalid_credentials"); @@ -65,7 +68,7 @@ public class OAuthResource { @FormParam("sessionId") String sessionId, @FormParam("scope") List scopes) { - sessionsRepo.getSession(sessionId).orElseThrow(() -> new OAuthException(ErrorResponse.Error.access_denied, "Invalid session")); + sessionsRepo.getSession(sessionId).orElseThrow(() -> new OAuthUserVisibleException(ErrorResponse.Error.access_denied, "Invalid session")); var session = sessionsRepo.authorizeSession(sessionId, scopes); String redirectUri = session.params().getRedirectUri(); @@ -120,7 +123,7 @@ public class OAuthResource { return switch (params.getGrantType()) { case "authorization_code" -> redeemAuthorizationCode(params); case "urn:ietf:params:oauth:grant-type:device_code" -> redeemDeviceCode(params); - default -> throw new OAuthException(ErrorResponse.Error.invalid_request, "Unsupported grant type"); + default -> throw new OAuthApiException(ErrorResponse.Error.invalid_request, "Unsupported grant type"); }; } @@ -148,23 +151,23 @@ public class OAuthResource { private AccessTokenResponse redeemAuthorizationCode(TokenParams params) { var session = sessionsRepo.redeemAuthorizationCode(params.getCode()) - .orElseThrow(() -> new OAuthException(ErrorResponse.Error.access_denied, "Invalid code")); + .orElseThrow(() -> new OAuthApiException(ErrorResponse.Error.access_denied, "Invalid code")); validateClient(params, session); if (!session.validateCodeChallenge(params.getCodeVerifier())) { - throw new OAuthException(ErrorResponse.Error.access_denied, "Invalid code verifier"); + throw new OAuthApiException(ErrorResponse.Error.access_denied, "Invalid code verifier"); } return session.tokens(); } private AccessTokenResponse redeemDeviceCode(TokenParams params) { var session = sessionsRepo.getByAuthorizationCode(params.getDeviceCode()) - .orElseThrow(() -> new OAuthException(ErrorResponse.Error.access_denied, "Invalid device code")); + .orElseThrow(() -> new OAuthApiException(ErrorResponse.Error.access_denied, "Invalid device code")); validateClient(params, session); if (session.tokens() != null) { sessionsRepo.redeemAuthorizationCode(params.getDeviceCode()); return session.tokens(); } else { - throw new OAuthException(ErrorResponse.Error.authorization_pending, "Authorization pending"); + throw new OAuthApiException(ErrorResponse.Error.authorization_pending, "Authorization pending"); } } @@ -176,24 +179,30 @@ public class OAuthResource { private OAuthClient validateClient(AuthParams params) { var client = clientsRepo.getClient(params.getClientId()) - .orElseThrow(() -> new OAuthException(ErrorResponse.Error.invalid_request, "Not a valid client")); + // must NOT redirect to not validated client + .orElseThrow(() -> new OAuthUserVisibleException(ErrorResponse.Error.invalid_request, "Not a valid client")); if (!client.validateRedirectUri(params.getRedirectUri())) { - throw new OAuthException(ErrorResponse.Error.invalid_request, "Invalid redirect URI"); + // must NOT redirect to invalid redirect URI + throw new OAuthUserVisibleException(ErrorResponse.Error.invalid_request, "Invalid redirect URI"); } if (StringUtil.isNullOrEmpty(params.getState())) { - throw new OAuthException(ErrorResponse.Error.invalid_request, "Invalid state"); + throw new OAuthRedirectException(params, ErrorResponse.Error.invalid_request, "Missing state"); + } + if (!params.validateResponseType()) { + throw new OAuthRedirectException(params, ErrorResponse.Error.unsupported_response_type, + "Unsupported response type"); } return client; } private OAuthClient validateClient(TokenParams params, AuthorizationSession session) { var client = clientsRepo.getClient(params.getClientId()) - .orElseThrow(() -> new OAuthException(ErrorResponse.Error.invalid_request, "Not a valid client")); + .orElseThrow(() -> new OAuthApiException(ErrorResponse.Error.invalid_request, "Not a valid client")); if (!session.validateRedirectUri(params.getRedirectUri())) { - throw new OAuthException(ErrorResponse.Error.invalid_request, "Invalid redirect URI"); + throw new OAuthApiException(ErrorResponse.Error.invalid_request, "Invalid redirect URI"); } if (!client.validateSecret(params.getClientSecret())) { - throw new OAuthException(ErrorResponse.Error.unauthorized_client, "Invalid secret"); + throw new OAuthApiException(ErrorResponse.Error.unauthorized_client, "Invalid secret"); } return client; } diff --git a/src/main/java/com/ysoft/geecon/dto/AuthParams.java b/src/main/java/com/ysoft/geecon/dto/AuthParams.java index 927fd4a..8030907 100644 --- a/src/main/java/com/ysoft/geecon/dto/AuthParams.java +++ b/src/main/java/com/ysoft/geecon/dto/AuthParams.java @@ -12,6 +12,14 @@ public class AuthParams { .toList(); } + public boolean validateResponseType() { + try { + return !getResponseTypes().isEmpty(); + } catch (IllegalArgumentException exception) { + return false; + } + } + @RestQuery("login_hint") String loginHint; @RestQuery("response_type") diff --git a/src/main/java/com/ysoft/geecon/error/ErrorResponse.java b/src/main/java/com/ysoft/geecon/error/ErrorResponse.java index 145a7b3..2fb9acb 100644 --- a/src/main/java/com/ysoft/geecon/error/ErrorResponse.java +++ b/src/main/java/com/ysoft/geecon/error/ErrorResponse.java @@ -5,6 +5,7 @@ import com.fasterxml.jackson.annotation.JsonProperty; public record ErrorResponse(@JsonProperty("error") Error error, @JsonProperty("error_description") String description) { public enum Error { - invalid_request, unauthorized_client, access_denied, invalid_scope, server_error, temporarily_unavailable, authorization_pending + invalid_request, unauthorized_client, unsupported_response_type, + access_denied, invalid_scope, server_error, temporarily_unavailable, authorization_pending } } diff --git a/src/main/java/com/ysoft/geecon/error/ExceptionMappers.java b/src/main/java/com/ysoft/geecon/error/ExceptionMappers.java index 7eecaea..ce4f495 100644 --- a/src/main/java/com/ysoft/geecon/error/ExceptionMappers.java +++ b/src/main/java/com/ysoft/geecon/error/ExceptionMappers.java @@ -3,33 +3,15 @@ package com.ysoft.geecon.error; import io.quarkus.qute.CheckedTemplate; import io.quarkus.qute.TemplateInstance; import jakarta.enterprise.context.ApplicationScoped; -import jakarta.inject.Inject; -import jakarta.ws.rs.Produces; -import jakarta.ws.rs.container.ResourceInfo; -import jakarta.ws.rs.core.MediaType; import jakarta.ws.rs.core.Response; import org.jboss.resteasy.reactive.server.ServerExceptionMapper; -import java.util.Arrays; - @ApplicationScoped class ExceptionMappers { - @Inject - ResourceInfo resourceInfo; @ServerExceptionMapper public Response exception(OAuthException exception) { - Object entity = producesJson() ? exception.getResponse() : Templates.error(exception.getResponse()); - return Response.status(Response.Status.BAD_REQUEST).entity(entity).build(); - } - - private boolean producesJson() { - Produces annotation = resourceInfo.getResourceMethod().getAnnotation(Produces.class); - if (annotation == null) { - return false; - } - String[] produces = annotation.value(); - return Arrays.asList(produces).contains(MediaType.APPLICATION_JSON); + return exception.getResponse(); } diff --git a/src/main/java/com/ysoft/geecon/error/OAuthApiException.java b/src/main/java/com/ysoft/geecon/error/OAuthApiException.java new file mode 100644 index 0000000..45d71f7 --- /dev/null +++ b/src/main/java/com/ysoft/geecon/error/OAuthApiException.java @@ -0,0 +1,20 @@ +package com.ysoft.geecon.error; + +import jakarta.ws.rs.core.Response; + +public class OAuthApiException extends OAuthException { + + public OAuthApiException(ErrorResponse response) { + super("OAuth error: " + response.error() + " " + response.description(), response); + } + + public OAuthApiException(ErrorResponse.Error error, String description) { + this(new ErrorResponse(error, description)); + } + + @Override + public Response getResponse() { + return Response.status(Response.Status.BAD_REQUEST).entity(response).build(); + } + +} diff --git a/src/main/java/com/ysoft/geecon/error/OAuthException.java b/src/main/java/com/ysoft/geecon/error/OAuthException.java index bfc762a..1a11038 100644 --- a/src/main/java/com/ysoft/geecon/error/OAuthException.java +++ b/src/main/java/com/ysoft/geecon/error/OAuthException.java @@ -1,19 +1,14 @@ package com.ysoft.geecon.error; -public class OAuthException extends RuntimeException { - private final ErrorResponse response; +import jakarta.ws.rs.core.Response; - public OAuthException(ErrorResponse response) { - super("OAuth error: " + response.error() + " " + response.description()); +public abstract class OAuthException extends RuntimeException { + protected final ErrorResponse response; + + public OAuthException(String message, ErrorResponse response) { + super(message); this.response = response; } - public OAuthException(ErrorResponse.Error error, String description) { - this(new ErrorResponse(error, description)); - } - - public ErrorResponse getResponse() { - return response; - } - + public abstract Response getResponse(); } diff --git a/src/main/java/com/ysoft/geecon/error/OAuthRedirectException.java b/src/main/java/com/ysoft/geecon/error/OAuthRedirectException.java new file mode 100644 index 0000000..a6e437a --- /dev/null +++ b/src/main/java/com/ysoft/geecon/error/OAuthRedirectException.java @@ -0,0 +1,27 @@ +package com.ysoft.geecon.error; + +import com.ysoft.geecon.dto.AuthParams; +import jakarta.ws.rs.core.Response; +import jakarta.ws.rs.core.UriBuilder; + +public class OAuthRedirectException extends OAuthApiException { + private final AuthParams authParams; + + public OAuthRedirectException(AuthParams authParams, ErrorResponse.Error error, String description) { + super(error, description); + this.authParams = authParams; + } + + public AuthParams getAuthParams() { + return authParams; + } + + public Response getResponse() { + UriBuilder uri = UriBuilder.fromUri(authParams.getRedirectUri()) + .fragment("") + .queryParam("state", authParams.getState()) + .queryParam("error", response.error()) + .queryParam("error_description", response.description()); + return Response.seeOther(uri.build()).build(); + } +} diff --git a/src/main/java/com/ysoft/geecon/error/OAuthUserVisibleException.java b/src/main/java/com/ysoft/geecon/error/OAuthUserVisibleException.java new file mode 100644 index 0000000..8fda690 --- /dev/null +++ b/src/main/java/com/ysoft/geecon/error/OAuthUserVisibleException.java @@ -0,0 +1,15 @@ +package com.ysoft.geecon.error; + +import jakarta.ws.rs.core.Response; + +public class OAuthUserVisibleException extends OAuthApiException { + + public OAuthUserVisibleException(ErrorResponse.Error error, String description) { + super(error, description); + } + + public Response getResponse() { + Object entity = ExceptionMappers.Templates.error(response); + return Response.status(Response.Status.BAD_REQUEST).entity(entity).build(); + } +} diff --git a/src/test/java/com/ysoft/geecon/AuthCodeGrantTest.java b/src/test/java/com/ysoft/geecon/AuthCodeGrantTest.java index b9a65a2..da68dfe 100644 --- a/src/test/java/com/ysoft/geecon/AuthCodeGrantTest.java +++ b/src/test/java/com/ysoft/geecon/AuthCodeGrantTest.java @@ -2,6 +2,7 @@ package com.ysoft.geecon; import com.ysoft.geecon.dto.OAuthClient; import com.ysoft.geecon.dto.User; +import com.ysoft.geecon.error.ErrorResponse; import com.ysoft.geecon.helpers.AuthorizationCodeFlow; import com.ysoft.geecon.helpers.ConsentScreen; import com.ysoft.geecon.helpers.LoginScreen; @@ -10,6 +11,7 @@ import com.ysoft.geecon.repo.UsersRepo; import io.quarkus.test.common.http.TestHTTPResource; import io.quarkus.test.junit.QuarkusTest; import jakarta.inject.Inject; +import org.jsoup.Connection; import org.jsoup.nodes.Document; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -56,6 +58,14 @@ public class AuthCodeGrantTest { assertThat(flow.getAccessToken(), is(notNullValue())); } + @Test + public void authCodeGrant_invalidResponseType() throws IOException { + AuthorizationCodeFlow flow = new AuthorizationCodeFlow(authUrl, CLIENT); + Connection.Response response = flow.startExpectError(Map.of("response_type", "")); + Map query = flow.parseAndValidateRedirectError(response); + assertThat(query.get("error"), is(ErrorResponse.Error.unsupported_response_type.name())); + } + @Test public void implicitGrant() throws IOException { AuthorizationCodeFlow flow = new AuthorizationCodeFlow(authUrl, CLIENT); diff --git a/src/test/java/com/ysoft/geecon/helpers/AuthorizationCodeFlow.java b/src/test/java/com/ysoft/geecon/helpers/AuthorizationCodeFlow.java index e87e5ab..a043564 100644 --- a/src/test/java/com/ysoft/geecon/helpers/AuthorizationCodeFlow.java +++ b/src/test/java/com/ysoft/geecon/helpers/AuthorizationCodeFlow.java @@ -49,6 +49,20 @@ public class AuthorizationCodeFlow { return new LoginScreen(login); } + public Connection.Response startExpectError(Map additionalData) throws IOException { + var data = defaultQuery(); + if (additionalData != null) { + data.putAll(additionalData); + } + + return Jsoup.connect(authUrl) + .followRedirects(false) + .data(data) + .get() + .connection() + .response(); + } + private Map defaultQuery() { var map = new HashMap(); map.put("client_id", client.clientId()); @@ -76,6 +90,20 @@ public class AuthorizationCodeFlow { idToken = query.get("id_token"); } + public Map parseAndValidateRedirectError(Connection.Response response) { + assertThat(response.statusCode(), is(303)); + assertThat(response.header("location"), startsWith(client.redirectUri())); + + URI location = URI.create(Objects.requireNonNull(response.header("location"))); + Map query = URLEncodedUtils.parse(location.getQuery(), Charset.defaultCharset()) + .stream().collect(Collectors.toMap(NameValuePair::getName, NameValuePair::getValue)); + + assertThat(query.get("state"), is(state)); + assertThat(query.get("error"), is(notNullValue())); + assertThat(query.get("error_description"), is(notNullValue())); + return query; + } + public AccessTokenResponse exchangeCode() { Map tokenForm = new HashMap<>(); tokenForm.put("grant_type", "authorization_code");