From 7ecf5674c99d8c7c9953972205a39934855592e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20K=C3=B6rber?= Date: Wed, 16 Dec 2015 00:38:03 +0100 Subject: [PATCH] Improve HTTP status code and JSON problem handling --- .../acme4j/connector/Connection.java | 8 +++ .../exception/AcmeConflictException.java | 46 ++++++++++++++++ .../acme4j/impl/AbstractAcmeClient.java | 54 +++++++++++++------ .../acme4j/impl/DefaultConnection.java | 28 +++++----- .../acme4j/impl/DefaultConnectionTest.java | 36 ++----------- 5 files changed, 110 insertions(+), 62 deletions(-) create mode 100644 acme4j-client/src/main/java/org/shredzone/acme4j/exception/AcmeConflictException.java diff --git a/acme4j-client/src/main/java/org/shredzone/acme4j/connector/Connection.java b/acme4j-client/src/main/java/org/shredzone/acme4j/connector/Connection.java index de387cff..c30034ab 100644 --- a/acme4j-client/src/main/java/org/shredzone/acme4j/connector/Connection.java +++ b/acme4j-client/src/main/java/org/shredzone/acme4j/connector/Connection.java @@ -19,6 +19,7 @@ import java.util.Map; import org.shredzone.acme4j.Account; import org.shredzone.acme4j.exception.AcmeException; +import org.shredzone.acme4j.exception.AcmeServerException; import org.shredzone.acme4j.util.ClaimBuilder; /** @@ -100,6 +101,13 @@ public interface Connection extends AutoCloseable { */ URI getLink(String relation) throws AcmeException; + /** + * Handles a problem by throwing an exception. If a JSON problem was returned, an + * {@link AcmeServerException} will be thrown. Otherwise a generic + * {@link AcmeException} is thrown. + */ + void throwAcmeException() throws AcmeException; + /** * Closes the {@link Connection}, releasing all resources. */ diff --git a/acme4j-client/src/main/java/org/shredzone/acme4j/exception/AcmeConflictException.java b/acme4j-client/src/main/java/org/shredzone/acme4j/exception/AcmeConflictException.java new file mode 100644 index 00000000..d199ec8c --- /dev/null +++ b/acme4j-client/src/main/java/org/shredzone/acme4j/exception/AcmeConflictException.java @@ -0,0 +1,46 @@ +/* + * acme4j - Java ACME client + * + * Copyright (C) 2015 Richard "Shred" Körber + * http://acme4j.shredzone.org + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + */ +package org.shredzone.acme4j.exception; + +import java.net.URI; + +import org.shredzone.acme4j.Account; +import org.shredzone.acme4j.AcmeClient; +import org.shredzone.acme4j.Registration; + +/** + * An exception that is thrown when there is a conflict with the request. For example, + * this exception is thrown when {@link AcmeClient#newRegistration(Account, Registration)} + * is invoked, but the registration already exists. + * + * @author Richard "Shred" Körber + */ +public class AcmeConflictException extends AcmeException { + private static final long serialVersionUID = 7454201988845449591L; + + private final URI location; + + public AcmeConflictException(String msg, URI location) { + super(msg); + this.location = location; + } + + /** + * Location of the conflicting resource. + */ + public URI getLocation() { + return location; + } + +} diff --git a/acme4j-client/src/main/java/org/shredzone/acme4j/impl/AbstractAcmeClient.java b/acme4j-client/src/main/java/org/shredzone/acme4j/impl/AbstractAcmeClient.java index 2256f4f1..cf20317b 100644 --- a/acme4j-client/src/main/java/org/shredzone/acme4j/impl/AbstractAcmeClient.java +++ b/acme4j-client/src/main/java/org/shredzone/acme4j/impl/AbstractAcmeClient.java @@ -13,6 +13,7 @@ */ package org.shredzone.acme4j.impl; +import java.net.HttpURLConnection; import java.net.URI; import java.security.cert.X509Certificate; import java.util.ArrayList; @@ -28,8 +29,8 @@ import org.shredzone.acme4j.challenge.Challenge; import org.shredzone.acme4j.connector.Connection; import org.shredzone.acme4j.connector.Resource; import org.shredzone.acme4j.connector.Session; +import org.shredzone.acme4j.exception.AcmeConflictException; import org.shredzone.acme4j.exception.AcmeException; -import org.shredzone.acme4j.exception.AcmeServerException; import org.shredzone.acme4j.util.ClaimBuilder; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -82,16 +83,19 @@ public abstract class AbstractAcmeClient implements AcmeClient { } if (registration.getAgreementUrl() != null) { claims.put("agreement", registration.getAgreementUrl()); + + int rc = conn.sendSignedRequest(resourceUri(Resource.NEW_REG), claims, session, account); + if (rc != HttpURLConnection.HTTP_CREATED && rc != HttpURLConnection.HTTP_CONFLICT) { + conn.throwAcmeException(); } - try { - conn.sendSignedRequest(resourceUri(Resource.NEW_REG), claims, session, account); - } catch (AcmeServerException ex) { - URI location = conn.getLocation(); - if (location != null) { - registration.setLocation(location); - } - throw ex; + URI location = conn.getLocation(); + if (location != null) { + registration.setLocation(location); + } + + if (rc == HttpURLConnection.HTTP_CONFLICT) { + throw new AcmeConflictException("Account is already registered", location); } } } @@ -113,7 +117,10 @@ public abstract class AbstractAcmeClient implements AcmeClient { claims.put("agreement", registration.getAgreementUrl()); } - conn.sendSignedRequest(registration.getLocation(), claims, session, account); + int rc = conn.sendSignedRequest(registration.getLocation(), claims, session, account); + if (rc != HttpURLConnection.HTTP_ACCEPTED) { + conn.throwAcmeException(); + } registration.setLocation(conn.getLocation()); } @@ -129,7 +136,10 @@ public abstract class AbstractAcmeClient implements AcmeClient { .put("type", "dns") .put("value", auth.getDomain()); - conn.sendSignedRequest(resourceUri(Resource.NEW_AUTHZ), claims, session, account); + int rc = conn.sendSignedRequest(resourceUri(Resource.NEW_AUTHZ), claims, session, account); + if (rc != HttpURLConnection.HTTP_CREATED) { + conn.throwAcmeException(); + } Map result = conn.readJsonResponse(); @@ -175,7 +185,10 @@ public abstract class AbstractAcmeClient implements AcmeClient { claims.putResource("challenge"); challenge.marshall(claims); - conn.sendSignedRequest(challenge.getUri(), claims, session, account); + int rc = conn.sendSignedRequest(challenge.getUri(), claims, session, account); + if (rc != HttpURLConnection.HTTP_ACCEPTED) { + conn.throwAcmeException(); + } challenge.unmarshall(conn.readJsonResponse()); } @@ -185,7 +198,11 @@ public abstract class AbstractAcmeClient implements AcmeClient { public void updateChallenge(Account account, Challenge challenge) throws AcmeException { LOG.debug("updateChallenge"); try (Connection conn = createConnection()) { - conn.sendRequest(challenge.getUri()); + int rc = conn.sendRequest(challenge.getUri()); + if (rc != HttpURLConnection.HTTP_ACCEPTED) { + conn.throwAcmeException(); + } + challenge.unmarshall(conn.readJsonResponse()); } } @@ -198,7 +215,10 @@ public abstract class AbstractAcmeClient implements AcmeClient { claims.putResource(Resource.NEW_CERT); claims.putBase64("csr", csr); - conn.sendSignedRequest(resourceUri(Resource.NEW_CERT), claims, session, account); + int rc = conn.sendSignedRequest(resourceUri(Resource.NEW_CERT), claims, session, account); + if (rc != HttpURLConnection.HTTP_CREATED) { + conn.throwAcmeException(); + } // Optionally returns the certificate. Currently it is just ignored. // X509Certificate cert = conn.readCertificate(); @@ -211,7 +231,11 @@ public abstract class AbstractAcmeClient implements AcmeClient { public X509Certificate downloadCertificate(URI certUri) throws AcmeException { LOG.debug("downloadCertificate"); try (Connection conn = createConnection()) { - conn.sendRequest(certUri); + int rc = conn.sendRequest(certUri); + if (rc != HttpURLConnection.HTTP_OK) { + conn.throwAcmeException(); + } + return conn.readCertificate(); } } diff --git a/acme4j-client/src/main/java/org/shredzone/acme4j/impl/DefaultConnection.java b/acme4j-client/src/main/java/org/shredzone/acme4j/impl/DefaultConnection.java index 6a15c348..1297178c 100644 --- a/acme4j-client/src/main/java/org/shredzone/acme4j/impl/DefaultConnection.java +++ b/acme4j-client/src/main/java/org/shredzone/acme4j/impl/DefaultConnection.java @@ -91,8 +91,6 @@ public class DefaultConnection implements Connection { conn.connect(); - throwException(); - return conn.getResponseCode(); } catch (IOException ex) { throw new AcmeException("API access failed", ex); @@ -139,8 +137,6 @@ public class DefaultConnection implements Connection { session.setNonce(getNonceFromHeader(conn)); - throwException(); - return conn.getResponseCode(); } catch (JoseException | IOException ex) { throw new AcmeException("Failed to send request to " + uri, ex); @@ -272,25 +268,27 @@ public class DefaultConnection implements Connection { } @Override - public void close() { - conn = null; - } - - /** - * Checks if the server returned an error, and if so, throws a {@link AcmeException}. - * - * @throws AcmeException - * if the server returned a JSON problem - */ - protected void throwException() throws AcmeException { + public void throwAcmeException() throws AcmeException { if ("application/problem+json".equals(conn.getHeaderField("Content-Type"))) { Map map = readJsonResponse(); String type = (String) map.get("type"); String detail = (String) map.get("detail"); throw new AcmeServerException(type, detail); + } else { + try { + throw new AcmeException("HTTP " + conn.getResponseCode() + ": " + + conn.getResponseMessage()); + } catch (IOException ex) { + throw new AcmeException("Network error"); + } } } + @Override + public void close() { + conn = null; + } + /** * Extracts a nonce from the header. * diff --git a/acme4j-client/src/test/java/org/shredzone/acme4j/impl/DefaultConnectionTest.java b/acme4j-client/src/test/java/org/shredzone/acme4j/impl/DefaultConnectionTest.java index 208da9ce..739f7f62 100644 --- a/acme4j-client/src/test/java/org/shredzone/acme4j/impl/DefaultConnectionTest.java +++ b/acme4j-client/src/test/java/org/shredzone/acme4j/impl/DefaultConnectionTest.java @@ -186,22 +186,6 @@ public class DefaultConnectionTest { verifyNoMoreInteractions(mockUrlConnection); } - /** - * Test that no exception is thrown if there is no problem. - */ - @Test - public void testNoThrowException() throws AcmeException { - when(mockUrlConnection.getHeaderField("Content-Type")).thenReturn("application/json"); - - try (DefaultConnection conn = new DefaultConnection(mockHttpConnection)) { - conn.conn = mockUrlConnection; - conn.throwException(); - } - - verify(mockUrlConnection).getHeaderField("Content-Type"); - verifyNoMoreInteractions(mockUrlConnection); - } - /** * Test if an {@link AcmeServerException} is thrown on an acme problem. */ @@ -215,7 +199,7 @@ public class DefaultConnectionTest { try (DefaultConnection conn = new DefaultConnection(mockHttpConnection)) { conn.conn = mockUrlConnection; - conn.throwException(); + conn.throwAcmeException(); fail("Expected to fail"); } catch (AcmeServerException ex) { assertThat(ex.getType(), is("urn:acme:error:unauthorized")); @@ -249,7 +233,7 @@ public class DefaultConnectionTest { }; }) { conn.conn = mockUrlConnection; - conn.throwException(); + conn.throwAcmeException(); fail("Expected to fail"); } catch (AcmeServerException ex) { assertThat(ex.getType(), is("urn:zombie:error:apocalypse")); @@ -290,14 +274,7 @@ public class DefaultConnectionTest { */ @Test public void testSendRequest() throws Exception { - final Set invoked = new HashSet<>(); - - try (DefaultConnection conn = new DefaultConnection(mockHttpConnection) { - @Override - protected void throwException() throws AcmeException { - invoked.add("throwException"); - }; - }) { + try (DefaultConnection conn = new DefaultConnection(mockHttpConnection)) { conn.sendRequest(requestUri); } @@ -307,7 +284,6 @@ public class DefaultConnectionTest { verify(mockUrlConnection).connect(); verify(mockUrlConnection).getResponseCode(); verifyNoMoreInteractions(mockUrlConnection); - assertThat(invoked, hasItem("throwException")); } /** @@ -325,10 +301,6 @@ public class DefaultConnectionTest { when(mockUrlConnection.getHeaderField("Replay-Nonce")).thenReturn(Base64Url.encode(nonce2)); try (DefaultConnection conn = new DefaultConnection(mockHttpConnection) { - @Override - protected void throwException() throws AcmeException { - invoked.add("throwException"); - }; @Override public void startSession(URI uri, Session session) throws AcmeException { assertThat(uri, is(requestUri)); @@ -359,7 +331,7 @@ public class DefaultConnectionTest { verify(mockUrlConnection).getOutputStream(); verify(mockUrlConnection).getResponseCode(); verifyNoMoreInteractions(mockUrlConnection); - assertThat(invoked, hasItems("throwException", "startSession")); + assertThat(invoked, hasItem("startSession")); String[] written = CompactSerializer.deserialize(new String(outputStream.toByteArray(), "utf-8")); String header = Base64Url.decodeToUtf8String(written[0]);