From f8b590c12ff69b53eef510587d09be24a70ada07 Mon Sep 17 00:00:00 2001 From: Aaron McIntyre Date: Wed, 18 Sep 2019 15:22:50 -0700 Subject: [PATCH] Fix error handling for http errors everywhere This commit adds check for response codes >= 400 and creates a new type of response for HTTP errors. Add tests for each area changed to handle a bad request http error. --- .../appauth/AuthorizationException.java | 23 +++++ .../openid/appauth/AuthorizationService.java | 21 +++- .../AuthorizationServiceConfiguration.java | 13 ++- ...AuthorizationServiceConfigurationTest.java | 19 ++++ .../appauth/AuthorizationServiceTest.java | 99 ++++++++++--------- 5 files changed, 123 insertions(+), 52 deletions(-) diff --git a/library/java/net/openid/appauth/AuthorizationException.java b/library/java/net/openid/appauth/AuthorizationException.java index 77738e33..65ee33b0 100644 --- a/library/java/net/openid/appauth/AuthorizationException.java +++ b/library/java/net/openid/appauth/AuthorizationException.java @@ -125,6 +125,11 @@ public final class AuthorizationException extends Exception { */ public static final int TYPE_OAUTH_REGISTRATION_ERROR = 4; + /** + * Error type when returning http error code >= 400 + */ + public static final int TYPE_HTTP_ERROR = 5; + @VisibleForTesting static final String KEY_TYPE = "type"; @@ -499,6 +504,24 @@ public static AuthorizationException fromTemplate( rootCause); } + /** + * Creates an exception based on HTTP error codes returned from HttpUrlConnection + * Exception includes actual error code from HttpURLConnection.getResponseStream() + * Errors are considered anything >= 400 + */ + public static AuthorizationException fromHttpError( + int responseCode, + @NonNull String responseMessage, + @NonNull String errorString) { + return new AuthorizationException( + TYPE_HTTP_ERROR, + responseCode, + responseMessage, + errorString, + null, + null); + } + /** * Creates an exception based on one of the existing values defined in * {@link AuthorizationRequestErrors} or {@link TokenRequestErrors}, adding information diff --git a/library/java/net/openid/appauth/AuthorizationService.java b/library/java/net/openid/appauth/AuthorizationService.java index 3fd5c054..c4d7c2b3 100644 --- a/library/java/net/openid/appauth/AuthorizationService.java +++ b/library/java/net/openid/appauth/AuthorizationService.java @@ -450,11 +450,16 @@ protected JSONObject doInBackground(Void... voids) { wr.write(queryData); wr.flush(); - if (conn.getResponseCode() >= HttpURLConnection.HTTP_OK - && conn.getResponseCode() < HttpURLConnection.HTTP_MULT_CHOICE) { + if (conn.getResponseCode() < HttpURLConnection.HTTP_BAD_REQUEST) { is = conn.getInputStream(); } else { is = conn.getErrorStream(); + String errorString = Utils.readInputStream(is); + mException = AuthorizationException.fromHttpError( + conn.getResponseCode(), + conn.getResponseMessage(), + errorString); + return null; } String response = Utils.readInputStream(is); return new JSONObject(response); @@ -599,7 +604,17 @@ protected JSONObject doInBackground(Void... voids) { wr.write(postData); wr.flush(); - is = conn.getInputStream(); + if (conn.getResponseCode() < HttpURLConnection.HTTP_BAD_REQUEST) { + is = conn.getInputStream(); + } else { + is = conn.getErrorStream(); + String errorString = Utils.readInputStream(is); + mException = AuthorizationException.fromHttpError( + conn.getResponseCode(), + conn.getResponseMessage(), + errorString); + return null; + } String response = Utils.readInputStream(is); return new JSONObject(response); } catch (IOException ex) { diff --git a/library/java/net/openid/appauth/AuthorizationServiceConfiguration.java b/library/java/net/openid/appauth/AuthorizationServiceConfiguration.java index 5d2b0e9a..90d95eca 100644 --- a/library/java/net/openid/appauth/AuthorizationServiceConfiguration.java +++ b/library/java/net/openid/appauth/AuthorizationServiceConfiguration.java @@ -329,7 +329,18 @@ protected AuthorizationServiceConfiguration doInBackground(Void... voids) { conn.setDoInput(true); conn.connect(); - is = conn.getInputStream(); + if (conn.getResponseCode() < HttpURLConnection.HTTP_BAD_REQUEST) { + is = conn.getInputStream(); + } else { + is = conn.getErrorStream(); + String errorString = Utils.readInputStream(is); + mException = AuthorizationException.fromHttpError( + conn.getResponseCode(), + conn.getResponseMessage(), + errorString); + return null; + } + JSONObject json = new JSONObject(Utils.readInputStream(is)); AuthorizationServiceDiscovery discovery = diff --git a/library/javatests/net/openid/appauth/AuthorizationServiceConfigurationTest.java b/library/javatests/net/openid/appauth/AuthorizationServiceConfigurationTest.java index e3eb10de..f3a4a643 100644 --- a/library/javatests/net/openid/appauth/AuthorizationServiceConfigurationTest.java +++ b/library/javatests/net/openid/appauth/AuthorizationServiceConfigurationTest.java @@ -217,6 +217,25 @@ public void testFetchFromUrlWithoutName() throws Exception { verify(mHttpConnection).connect(); } + @Test + public void testServiceConfigurationRequest_withBadRequest() throws Exception { + InputStream is = new ByteArrayInputStream(AuthorizationServiceTest.BAD_REQUEST_RESPONSE.getBytes()); + when(mHttpConnection.getErrorStream()).thenReturn(is); + when(mHttpConnection.getResponseCode()).thenReturn(HttpURLConnection.HTTP_BAD_REQUEST); + when(mHttpConnection.getResponseMessage()).thenReturn(AuthorizationServiceTest.BAD_REQUEST_ERROR_MESSAGE); + doFetch(); + mCallback.waitForCallback(); + assertBadRequest(mCallback.error); + } + + private void assertBadRequest(AuthorizationException error) { + assertNotNull(error); + assertEquals(AuthorizationException.TYPE_HTTP_ERROR, error.type); + assertEquals(HttpURLConnection.HTTP_BAD_REQUEST, error.code); + assertEquals(AuthorizationServiceTest.BAD_REQUEST_ERROR_MESSAGE, error.error); + assertEquals(AuthorizationServiceTest.BAD_REQUEST_RESPONSE, error.errorDescription); + } + @Test public void testFetchFromUrl_missingArgument() throws Exception { InputStream is = new ByteArrayInputStream(TEST_JSON_MISSING_ARGUMENT.getBytes()); diff --git a/library/javatests/net/openid/appauth/AuthorizationServiceTest.java b/library/javatests/net/openid/appauth/AuthorizationServiceTest.java index 078c5046..241ee117 100644 --- a/library/javatests/net/openid/appauth/AuthorizationServiceTest.java +++ b/library/javatests/net/openid/appauth/AuthorizationServiceTest.java @@ -100,14 +100,36 @@ public class AuthorizationServiceTest { + " \"application_type\": " + RegistrationRequest.APPLICATION_TYPE_NATIVE + "\n" + "}"; - private static final String INVALID_GRANT_RESPONSE_JSON = "{\n" - + " \"error\": \"invalid_grant\",\n" - + " \"error_description\": \"invalid_grant description\"\n" - + "}"; - - private static final String INVALID_GRANT_NO_DESC_RESPONSE_JSON = "{\n" - + " \"error\": \"invalid_grant\"\n" - + "}"; + public static final String BAD_REQUEST_RESPONSE = "\n" + + "\n" + + "\n" + + " \n" + + " Exception caught\n" + + " \n" + + "\n" + + "\n" + + "\n" + + "\n" + + "\n" + + "
\n" + + "

Network Error

\n" + + "
\n" + + "
\n" + + "

Bad Request

\n" + + "\n" + + "
\n" + + "\n" + + "\n" + + ""; + + public static final String BAD_REQUEST_ERROR_MESSAGE = "Bad Request"; private static final int TEST_INVALID_GRANT_CODE = 2002; @@ -342,39 +364,16 @@ public void testTokenRequest_withPostAuth() throws Exception { } @Test - public void testTokenRequest_withInvalidGrant() throws Exception { + public void testTokenRequest_withBadRequest() throws Exception { ClientSecretPost csp = new ClientSecretPost(TEST_CLIENT_SECRET); - InputStream is = new ByteArrayInputStream(INVALID_GRANT_RESPONSE_JSON.getBytes()); + InputStream is = new ByteArrayInputStream(BAD_REQUEST_RESPONSE.getBytes()); when(mHttpConnection.getErrorStream()).thenReturn(is); when(mHttpConnection.getResponseCode()).thenReturn(HttpURLConnection.HTTP_BAD_REQUEST); + when(mHttpConnection.getResponseMessage()).thenReturn(BAD_REQUEST_ERROR_MESSAGE); TokenRequest request = getTestAuthCodeExchangeRequest(); mService.performTokenRequest(request, csp, mAuthCallback); mAuthCallback.waitForCallback(); - assertInvalidGrant(mAuthCallback.error); - } - - @Test - public void testTokenRequest_withInvalidGrant2() throws Exception { - ClientSecretPost csp = new ClientSecretPost(TEST_CLIENT_SECRET); - InputStream is = new ByteArrayInputStream(INVALID_GRANT_RESPONSE_JSON.getBytes()); - when(mHttpConnection.getErrorStream()).thenReturn(is); - when(mHttpConnection.getResponseCode()).thenReturn(199); - TokenRequest request = getTestAuthCodeExchangeRequest(); - mService.performTokenRequest(request, csp, mAuthCallback); - mAuthCallback.waitForCallback(); - assertInvalidGrant(mAuthCallback.error); - } - - @Test - public void testTokenRequest_withInvalidGrantWithNoDesc() throws Exception { - ClientSecretPost csp = new ClientSecretPost(TEST_CLIENT_SECRET); - InputStream is = new ByteArrayInputStream(INVALID_GRANT_NO_DESC_RESPONSE_JSON.getBytes()); - when(mHttpConnection.getErrorStream()).thenReturn(is); - when(mHttpConnection.getResponseCode()).thenReturn(HttpURLConnection.HTTP_BAD_REQUEST); - TokenRequest request = getTestAuthCodeExchangeRequest(); - mService.performTokenRequest(request, csp, mAuthCallback); - mAuthCallback.waitForCallback(); - assertInvalidGrantWithNoDescription(mAuthCallback.error); + assertBadRequest(mAuthCallback.error); } @Test @@ -400,6 +399,18 @@ public void testRegistrationRequest() throws Exception { assertThat(postBody).isEqualTo(request.toJsonString()); } + @Test + public void testRegistrationRequest_withBadRequest() throws Exception { + InputStream is = new ByteArrayInputStream(BAD_REQUEST_RESPONSE.getBytes()); + when(mHttpConnection.getErrorStream()).thenReturn(is); + when(mHttpConnection.getResponseCode()).thenReturn(HttpURLConnection.HTTP_BAD_REQUEST); + when(mHttpConnection.getResponseMessage()).thenReturn(AuthorizationServiceTest.BAD_REQUEST_ERROR_MESSAGE); + RegistrationRequest request = getTestRegistrationRequest(); + mService.performRegistrationRequest(request, mRegistrationCallback); + mRegistrationCallback.waitForCallback(); + assertBadRequest(mRegistrationCallback.error); + } + @Test public void testRegistrationRequest_IoException() throws Exception { Exception ex = new IOException(); @@ -453,20 +464,12 @@ private void assertTokenResponse( assertEquals(idToken, response.idToken); } - private void assertInvalidGrant(AuthorizationException error) { - assertNotNull(error); - assertEquals(AuthorizationException.TYPE_OAUTH_TOKEN_ERROR, error.type); - assertEquals(TEST_INVALID_GRANT_CODE, error.code); - assertEquals("invalid_grant", error.error); - assertEquals("invalid_grant description", error.errorDescription); - } - - private void assertInvalidGrantWithNoDescription(AuthorizationException error) { + private void assertBadRequest(AuthorizationException error) { assertNotNull(error); - assertEquals(AuthorizationException.TYPE_OAUTH_TOKEN_ERROR, error.type); - assertEquals(TEST_INVALID_GRANT_CODE, error.code); - assertEquals("invalid_grant", error.error); - assertNull(error.errorDescription); + assertEquals(AuthorizationException.TYPE_HTTP_ERROR, error.type); + assertEquals(HttpURLConnection.HTTP_BAD_REQUEST, error.code); + assertEquals(BAD_REQUEST_ERROR_MESSAGE, error.error); + assertEquals(BAD_REQUEST_RESPONSE, error.errorDescription); } private void assertRegistrationResponse(RegistrationResponse response,