From 421b00517fd14dec038948043e25b0915f915d6a Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Thu, 6 Jan 2022 14:11:19 -0300 Subject: [PATCH] Address review comments for MailboxApi --- .../bramble/mailbox/MailboxApi.java | 15 +---- .../bramble/mailbox/MailboxApiImpl.java | 30 ++++++--- .../bramble/mailbox/MailboxApiTest.java | 63 +++++++++++-------- 3 files changed, 62 insertions(+), 46 deletions(-) diff --git a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxApi.java b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxApi.java index 97539bd84..6c5999990 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxApi.java +++ b/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 { } diff --git a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxApiImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxApiImpl.java index df35d5723..3c0906fb5 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxApiImpl.java +++ b/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(); - Request request = getRequestBuilder(properties.token) - .url(properties.baseUrl + "/setup") + if (!properties.isOwner()) throw new IllegalArgumentException(); + Request request = getRequestBuilder(properties.getAuthToken()) + .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(); - Request request = getRequestBuilder(properties.token) - .url(properties.baseUrl + "/status") + if (!properties.isOwner()) throw new IllegalArgumentException(); + Request request = getRequestBuilder(properties.getAuthToken()) + .url(properties.getOnionAddress() + "/status") .build(); OkHttpClient client = httpClientProvider.get(); Response response = client.newCall(request).execute(); diff --git a/bramble-core/src/test/java/org/briarproject/bramble/mailbox/MailboxApiTest.java b/bramble-core/src/test/java/org/briarproject/bramble/mailbox/MailboxApiTest.java index 42054f5fc..c74570a62 100644 --- a/bramble-core/src/test/java/org/briarproject/bramble/mailbox/MailboxApiTest.java +++ b/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 token2 = getRandomString(64); + private final String token = toHexString(getRandomBytes(32)); + 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);