Address review comments for MailboxApi

bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxApi.java

@@ -1,5 +1,7 @@
 package org.briarproject.bramble.mailbox;
 
+import org.briarproject.bramble.api.mailbox.MailboxProperties;
+
 import java.io.IOException;
 
 import javax.annotation.concurrent.Immutable;
@@ -25,19 +27,6 @@ interface MailboxApi {
 	boolean checkStatus(MailboxProperties properties)
 			throws IOException, PermanentFailureException;
 
-	@Immutable
-	class MailboxProperties {
-		final String baseUrl;
-		final String token;
-		final boolean isOwner;
-
-		MailboxProperties(String baseUrl, String token, boolean isOwner) {
-			this.baseUrl = baseUrl;
-			this.token = token;
-			this.isOwner = isOwner;
-		}
-	}
-
 	@Immutable
 	class PermanentFailureException extends Exception {
 	}

bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxApiImpl.java

@@ -5,6 +5,7 @@ import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.json.JsonMapper;
 
 import org.briarproject.bramble.api.WeakSingletonProvider;
+import org.briarproject.bramble.api.mailbox.MailboxProperties;
 import org.briarproject.bramble.api.nullsafety.NotNullByDefault;
 
 import java.io.IOException;
@@ -18,6 +19,7 @@ import okhttp3.ResponseBody;
 
 import static com.fasterxml.jackson.databind.MapperFeature.BLOCK_UNSAFE_POLYMORPHIC_BASE_TYPES;
 import static okhttp3.internal.Util.EMPTY_REQUEST;
+import static org.briarproject.bramble.util.StringUtils.fromHexString;
 
 @NotNullByDefault
 class MailboxApiImpl implements MailboxApi {
@@ -35,15 +37,16 @@ class MailboxApiImpl implements MailboxApi {
 	@Override
 	public String setup(MailboxProperties properties)
 			throws IOException, PermanentFailureException {
-		if (!properties.isOwner) throw new IllegalArgumentException();
+		if (!properties.isOwner()) throw new IllegalArgumentException();
-		Request request = getRequestBuilder(properties.token)
+		Request request = getRequestBuilder(properties.getAuthToken())
-				.url(properties.baseUrl + "/setup")
+				.url(properties.getOnionAddress() + "/setup")
 				.put(EMPTY_REQUEST)
 				.build();
 		OkHttpClient client = httpClientProvider.get();
 		Response response = client.newCall(request).execute();
+		// TODO consider throwing a special exception for the 401 case
 		if (response.code() == 401) throw new PermanentFailureException();
-		if (!response.isSuccessful()) throw new IOException();
+		if (!response.isSuccessful()) throw new PermanentFailureException();
 		ResponseBody body = response.body();
 		if (body == null) throw new PermanentFailureException();
 		try {
@@ -53,7 +56,7 @@ class MailboxApiImpl implements MailboxApi {
 				throw new PermanentFailureException();
 			}
 			String ownerToken = tokenNode.textValue();
-			if (ownerToken == null) {
+			if (ownerToken == null || !isValidToken(ownerToken)) {
 				throw new PermanentFailureException();
 			}
 			return ownerToken;
@@ -62,12 +65,23 @@ class MailboxApiImpl implements MailboxApi {
 		}
 	}
 
+	private boolean isValidToken(String token) {
+		if (token.length() != 64) return false;
+		try {
+			// try to convert to bytes
+			fromHexString(token);
+			return true;
+		} catch (IllegalArgumentException e) {
+			return false;
+		}
+	}
+
 	@Override
 	public boolean checkStatus(MailboxProperties properties)
 			throws IOException, PermanentFailureException {
-		if (!properties.isOwner) throw new IllegalArgumentException();
+		if (!properties.isOwner()) throw new IllegalArgumentException();
-		Request request = getRequestBuilder(properties.token)
+		Request request = getRequestBuilder(properties.getAuthToken())
-				.url(properties.baseUrl + "/status")
+				.url(properties.getOnionAddress() + "/status")
 				.build();
 		OkHttpClient client = httpClientProvider.get();
 		Response response = client.newCall(request).execute();

bramble-core/src/test/java/org/briarproject/bramble/mailbox/MailboxApiTest.java

@@ -1,13 +1,11 @@
 package org.briarproject.bramble.mailbox;
 
 import org.briarproject.bramble.api.WeakSingletonProvider;
-import org.briarproject.bramble.mailbox.MailboxApi.MailboxProperties;
+import org.briarproject.bramble.api.mailbox.MailboxProperties;
 import org.briarproject.bramble.mailbox.MailboxApi.PermanentFailureException;
 import org.briarproject.bramble.test.BrambleTestCase;
 import org.junit.Test;
 
-import java.io.IOException;
-
 import javax.annotation.Nonnull;
 import javax.net.SocketFactory;
 
@@ -17,7 +15,9 @@ import okhttp3.mockwebserver.MockWebServer;
 import okhttp3.mockwebserver.RecordedRequest;
 
 import static java.util.concurrent.TimeUnit.MILLISECONDS;
+import static org.briarproject.bramble.test.TestUtils.getRandomBytes;
 import static org.briarproject.bramble.util.StringUtils.getRandomString;
+import static org.briarproject.bramble.util.StringUtils.toHexString;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
@@ -40,14 +40,16 @@ public class MailboxApiTest extends BrambleTestCase {
 			};
 	private final MailboxApiImpl api = new MailboxApiImpl(httpClientProvider);
 
-	private final String token = getRandomString(64);
+	private final String token = toHexString(getRandomBytes(32));
-	private final String token2 = getRandomString(64);
+	private final String token2 = toHexString(getRandomBytes(32));
 
 	@Test
 	public void testSetup() throws Exception {
 		String validResponse = "{\"token\":\"" + token2 + "\"}";
 		String invalidResponse = "{\"foo\":\"bar\"}";
 		String invalidTokenResponse = "{\"token\":{\"foo\":\"bar\"}}";
+		String invalidTokenResponse2 =
+				"{\"token\":\"" + getRandomString(64) + "\"}";
 
 		MockWebServer server = new MockWebServer();
 		server.enqueue(new MockResponse().setBody(validResponse));
@@ -56,6 +58,7 @@ public class MailboxApiTest extends BrambleTestCase {
 		server.enqueue(new MockResponse().setResponseCode(401));
 		server.enqueue(new MockResponse().setResponseCode(500));
 		server.enqueue(new MockResponse().setBody(invalidTokenResponse));
+		server.enqueue(new MockResponse().setBody(invalidTokenResponse2));
 		server.start();
 		String baseUrl = getBaseUrl(server);
 		MailboxProperties properties =
@@ -63,49 +66,62 @@ public class MailboxApiTest extends BrambleTestCase {
 		MailboxProperties properties2 =
 				new MailboxProperties(baseUrl, token2, true);
 
+		// valid response with valid token
 		assertEquals(token2, api.setup(properties));
-
-		assertThrows(PermanentFailureException.class,
-				() -> api.setup(properties));
-		assertThrows(PermanentFailureException.class,
-				() -> api.setup(properties));
-		assertThrows(PermanentFailureException.class,
-				() -> api.setup(properties2));
-		assertThrows(IOException.class,
-				() -> api.setup(properties));
-		assertThrows(PermanentFailureException.class,
-				() -> api.setup(properties)
-		);
-
 		RecordedRequest request1 = server.takeRequest();
 		assertEquals("/setup", request1.getPath());
 		assertEquals("PUT", request1.getMethod());
 		assertToken(request1, token);
 
+		// empty body
+		assertThrows(PermanentFailureException.class,
+				() -> api.setup(properties));
 		RecordedRequest request2 = server.takeRequest();
 		assertEquals("/setup", request2.getPath());
 		assertEquals("PUT", request2.getMethod());
 		assertToken(request2, token);
 
+		// invalid response
+		assertThrows(PermanentFailureException.class,
+				() -> api.setup(properties));
 		RecordedRequest request3 = server.takeRequest();
 		assertEquals("/setup", request3.getPath());
 		assertEquals("PUT", request3.getMethod());
 		assertToken(request3, token);
 
+		// 401 response
+		assertThrows(PermanentFailureException.class,
+				() -> api.setup(properties2));
 		RecordedRequest request4 = server.takeRequest();
 		assertEquals("/setup", request4.getPath());
 		assertEquals("PUT", request4.getMethod());
 		assertToken(request4, token2);
 
+		// 500 response
+		assertThrows(PermanentFailureException.class,
+				() -> api.setup(properties));
 		RecordedRequest request5 = server.takeRequest();
 		assertEquals("/setup", request5.getPath());
 		assertEquals("PUT", request5.getMethod());
 		assertToken(request5, token);
 
+		// invalid json dict token response
+		assertThrows(PermanentFailureException.class,
+				() -> api.setup(properties)
+		);
 		RecordedRequest request6 = server.takeRequest();
 		assertEquals("/setup", request6.getPath());
 		assertEquals("PUT", request6.getMethod());
 		assertToken(request6, token);
+
+		// invalid non-hex string token response
+		assertThrows(PermanentFailureException.class,
+				() -> api.setup(properties)
+		);
+		RecordedRequest request7 = server.takeRequest();
+		assertEquals("/setup", request7.getPath());
+		assertEquals("PUT", request7.getMethod());
+		assertToken(request7, token);
 	}
 
 	@Test
@@ -132,21 +148,18 @@ public class MailboxApiTest extends BrambleTestCase {
 				new MailboxProperties(baseUrl, token2, true);
 
 		assertTrue(api.checkStatus(properties));
-
-		assertThrows(PermanentFailureException.class, () ->
-				api.checkStatus(properties2)
-		);
-
-		assertFalse(api.checkStatus(properties));
-
 		RecordedRequest request1 = server.takeRequest();
 		assertEquals("/status", request1.getPath());
 		assertToken(request1, token);
 
+		assertThrows(PermanentFailureException.class, () ->
+				api.checkStatus(properties2)
+		);
 		RecordedRequest request2 = server.takeRequest();
 		assertEquals("/status", request2.getPath());
 		assertToken(request2, token2);
 
+		assertFalse(api.checkStatus(properties));
 		RecordedRequest request3 = server.takeRequest();
 		assertEquals("/status", request3.getPath());
 		assertToken(request3, token);