From 36e21854ab822dd36f2db8d2ecba3dd04d8b2d6e Mon Sep 17 00:00:00 2001 From: Rich Ellis Date: Thu, 12 May 2016 10:58:35 +0100 Subject: [PATCH] Fixed 403 null reason deserialization Changed exception `error` and `reason` assigns to use `getAsString` utility method. Added check for `JsonNull` to `getAsString` utility method. Added new tests for 403 with a missing or null `reason` property: * `handleNonExpiry403NoReason` * `handleNonExpiry403NullReason` Moved all 403 tests together. Used a common method for the 403 tests with different args. --- CHANGES.md | 1 + .../client/org/lightcouch/CouchDbClient.java | 6 +- .../org/lightcouch/internal/CouchDbUtil.java | 11 +- .../java/com/cloudant/tests/HttpTest.java | 149 +++++++++++------- 4 files changed, 107 insertions(+), 60 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 094db6977..b9818c0d4 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -16,6 +16,7 @@ - [FIX] Documentation that suggested calling `database("dbname", false)` would immediately throw a `NoDocumentException` if the database did not exist. The exception is not thrown until the first operation on the `Database` instance. +- [FIX] `ClassCastException` when the server responded `403` with a `null` reason in the JSON. # 2.4.3 (2016-05-05) - [IMPROVED] Reduced the length of the User-Agent header string. diff --git a/cloudant-client/src/main/java/com/cloudant/client/org/lightcouch/CouchDbClient.java b/cloudant-client/src/main/java/com/cloudant/client/org/lightcouch/CouchDbClient.java index 42fcbde82..163a30d6c 100644 --- a/cloudant-client/src/main/java/com/cloudant/client/org/lightcouch/CouchDbClient.java +++ b/cloudant-client/src/main/java/com/cloudant/client/org/lightcouch/CouchDbClient.java @@ -475,10 +475,8 @@ public HttpConnection execute(HttpConnection connection) { try { JsonObject errorResponse = new Gson().fromJson(e.error, JsonObject .class); - exception.error = errorResponse.getAsJsonPrimitive - ("error").getAsString(); - exception.reason = errorResponse.getAsJsonPrimitive - ("reason").getAsString(); + exception.error = getAsString(errorResponse, "error"); + exception.reason = getAsString(errorResponse, "reason"); } catch (JsonParseException jpe) { exception.error = e.error; } diff --git a/cloudant-client/src/main/java/com/cloudant/client/org/lightcouch/internal/CouchDbUtil.java b/cloudant-client/src/main/java/com/cloudant/client/org/lightcouch/internal/CouchDbUtil.java index 6b67b48ec..f2a9c56a4 100644 --- a/cloudant-client/src/main/java/com/cloudant/client/org/lightcouch/internal/CouchDbUtil.java +++ b/cloudant-client/src/main/java/com/cloudant/client/org/lightcouch/internal/CouchDbUtil.java @@ -93,10 +93,17 @@ public static T jsonToObject(Gson gson, JsonElement elem, String key, Class< } /** - * @return A JSON element as a String, or null if not found. + * @return A JSON element as a String, or null if there is no member with that name or the + * value was a JSON null. */ public static String getAsString(JsonObject j, String e) { - return (j.get(e) == null) ? null : j.get(e).getAsString(); + if (j != null && e != null) { + JsonElement element = j.get(e); + if (element != null && !element.isJsonNull()) { + return element.getAsString(); + } + } + return null; } /** diff --git a/cloudant-client/src/test/java/com/cloudant/tests/HttpTest.java b/cloudant-client/src/test/java/com/cloudant/tests/HttpTest.java index c83fd24d9..3a07d5e81 100644 --- a/cloudant-client/src/test/java/com/cloudant/tests/HttpTest.java +++ b/cloudant-client/src/test/java/com/cloudant/tests/HttpTest.java @@ -24,7 +24,10 @@ import com.cloudant.tests.util.MockWebServerResources; import com.cloudant.tests.util.Utils; import com.google.gson.Gson; +import com.google.gson.JsonElement; +import com.google.gson.JsonNull; import com.google.gson.JsonObject; +import com.google.gson.JsonPrimitive; import com.squareup.okhttp.mockwebserver.Dispatcher; import com.squareup.okhttp.mockwebserver.MockResponse; import com.squareup.okhttp.mockwebserver.MockWebServer; @@ -228,43 +231,6 @@ public void cookieInterceptorURLEncoding() throws Exception { } } - /** - * This test checks that the cookie is successfully renewed if a 403 with an error of - * "credentials_expired" is returned. - * - * @throws Exception - */ - @Test - public void cookie403Renewal() throws Exception { - - // Request sequence - // _session request to get Cookie - // GET request -> 403 - // _session for new cookie - // GET replay -> 200 - mockWebServer.enqueue(MockWebServerResources.OK_COOKIE); - mockWebServer.enqueue(new MockResponse().setResponseCode(403).setBody - ("{\"error\":\"credentials_expired\", \"reason\":\"Session expired\"}\r\n")); - mockWebServer.enqueue(MockWebServerResources.OK_COOKIE); - mockWebServer.enqueue(new MockResponse()); - - CloudantClient c = CloudantClientHelper.newMockWebServerClientBuilder(mockWebServer) - .username("a") - .password("b") - .build(); - //the GET request will try to get a session, then perform the GET - //the GET will result in a 403, which should mean another request to _session - //followed by a replay of GET - c.executeRequest(Http.GET(c.getBaseUri())); - - //if we don't handle the 403 correctly an exception will be thrown - - // also assert that there were 4 calls - assertEquals("The server should have received 4 requests", 4, mockWebServer - .getRequestCount()); - - } - /** * This test check that the cookie is renewed if the server presents a Set-Cookie header * after the cookie authentication. @@ -278,7 +244,7 @@ public void cookieRenewal() throws Exception { "AuthSession=\"RenewCookie_a2ltc3RlYmVsOjUxMzRBQTUzOtiY2_IDUIdsTJEVNEjObAbyhrgz\";"; // Request sequence // _session request to get Cookie - // GET request -> 403 + // GET request -> 200 with a Set-Cookie // _session for new cookie // GET replay -> 200 mockWebServer.enqueue(MockWebServerResources.OK_COOKIE); @@ -312,6 +278,19 @@ public void cookieRenewal() throws Exception { "\"", headerValue); } + /** + * This test checks that the cookie is successfully renewed if a 403 with an error of + * "credentials_expired" is returned. + * + * @throws Exception + */ + @Test + public void cookie403Renewal() throws Exception { + + // Test for a 403 with expired credentials, should result in 4 requests + basic403Test("credentials_expired", "Session expired", 4); + } + /** * This test checks that if we get a 403 that is not an error of "credentials_expired" then * the exception is correctly thrown and the error stream is deserialized. This is important @@ -322,28 +301,90 @@ public void cookieRenewal() throws Exception { @Test public void handleNonExpiry403() throws Exception { - // Request sequence - // _session request to get Cookie - // GET request -> 403 (CouchDbException) + // Test for a non-expiry 403, expect 2 requests + basic403Test("403_not_expired_test", "example reason", 2); + } + + /** + * Same as {@link #handleNonExpiry403()} but with no reason property in the JSON. + * + * @throws Exception + */ + @Test + public void handleNonExpiry403NoReason() throws Exception { + + // Test for a non-expiry 403, expect 2 requests + basic403Test("403_not_expired_test", null, 2); + } + + /** + * * Same as {@link #handleNonExpiry403()} but with a {@code null} reason property in the JSON. + * + * @throws Exception + */ + @Test + public void handleNonExpiry403NullReason() throws Exception { + + // Test for a non-expiry 403, expect 2 requests + basic403Test("403_not_expired_test", "null", 2); + } + + /** + * Method that performs a basic test for a 403 response. The sequence of requests is: + *
    + *
  1. _session request to get Cookie
    2. + *
  2. GET request -> a 403 response
    3. + *
  3. _session for new cookie*
    4. + *
  4. GET replay -> a 200 response*
    5. + *
+ * The requests annotated * should only happen in the credentials_expired 403 case + * + * @param error the response JSON error content for the 403 + * @param reason the response JSON reason content for the 403 + */ + private void basic403Test(String error, String reason, int expectedRequests) throws + Exception { mockWebServer.enqueue(MockWebServerResources.OK_COOKIE); - mockWebServer.enqueue(new MockResponse().setResponseCode(403).setBody - ("{\"error\":\"403_not_expired_test\", \"reason\":\"example reason\"}\r\n")); + JsonObject responseBody = new JsonObject(); + responseBody.add("error", new JsonPrimitive(error)); + JsonElement jsonReason; + if (reason != null) { + if ("null".equals(reason)) { + jsonReason = JsonNull.INSTANCE; + reason = null; // For the assertion we need a real null, not a JsonNull + } else { + jsonReason = new JsonPrimitive(reason); + } + responseBody.add("reason", jsonReason); + } + mockWebServer.enqueue(new MockResponse().setResponseCode(403).setBody(responseBody + .toString())); + mockWebServer.enqueue(MockWebServerResources.OK_COOKIE); + mockWebServer.enqueue(new MockResponse()); + + CloudantClient c = CloudantClientHelper.newMockWebServerClientBuilder(mockWebServer) + .username("a") + .password("b") + .build(); + //the GET request will try to get a session, then perform the GET + //the GET will result in a 403, which in a renewal case should mean another request to + // _session followed by a replay of GET try { - CloudantClient c = CloudantClientHelper.newMockWebServerClientBuilder(mockWebServer) - .username("a") - .password("b") - .build(); - //the GET request will try to get a session, then perform the GET - //the GET will result in a 403, which should result in a CouchDbException c.executeRequest(Http.GET(c.getBaseUri())); - fail("A 403 not due to cookie expiry should result in a CouchDbException"); + if (!error.equals("credentials_expired")) { + fail("A 403 not due to cookie expiry should result in a CouchDbException"); + } } catch (CouchDbException e) { - e.printStackTrace(); - assertNotNull("The error should not be null", e.getError()); - assertEquals("The error message should be the expected one", "403_not_expired_test", e - .getError()); + assertEquals("The exception error should be the expected message", error, e.getError()); + assertEquals("The exception reason should be the expected message", reason, e + .getReason()); } + + // also assert that there were the correct number of calls + assertEquals("The server should receive the expected number of requests", + expectedRequests, mockWebServer + .getRequestCount()); } @Test