From d665fc17ec0d59213455c54c77dd43f256e4e745 Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Mon, 13 Dec 2021 16:06:03 -0300 Subject: [PATCH 1/3] Add /status and /setup mailbox API call with tests --- bramble-core/build.gradle | 2 + .../bramble/mailbox/MailboxApi.java | 53 +++++ .../bramble/mailbox/MailboxApiImpl.java | 87 ++++++++ .../bramble/mailbox/MailboxApiTest.java | 193 ++++++++++++++++++ bramble-core/witness.gradle | 10 + briar-headless/build.gradle | 2 +- briar-headless/witness.gradle | 6 +- build.gradle | 1 + 8 files changed, 350 insertions(+), 4 deletions(-) create mode 100644 bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxApi.java create mode 100644 bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxApiImpl.java create mode 100644 bramble-core/src/test/java/org/briarproject/bramble/mailbox/MailboxApiTest.java diff --git a/bramble-core/build.gradle b/bramble-core/build.gradle index 0d4a255b6..0a9644544 100644 --- a/bramble-core/build.gradle +++ b/bramble-core/build.gradle @@ -20,6 +20,7 @@ dependencies { //noinspection GradleDependency implementation "com.squareup.okhttp3:okhttp:$okhttp_version" + implementation "com.fasterxml.jackson.core:jackson-databind:$jackson_version" annotationProcessor "com.google.dagger:dagger-compiler:$dagger_version" @@ -30,6 +31,7 @@ dependencies { testImplementation "org.jmock:jmock:$jmock_version" testImplementation "org.jmock:jmock-junit4:$jmock_version" testImplementation "org.jmock:jmock-imposters:$jmock_version" + testImplementation "com.squareup.okhttp3:mockwebserver:4.9.3" testAnnotationProcessor "com.google.dagger:dagger-compiler:$dagger_version" 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 new file mode 100644 index 000000000..77add349d --- /dev/null +++ b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxApi.java @@ -0,0 +1,53 @@ +package org.briarproject.bramble.mailbox; + +import java.io.IOException; + +import javax.annotation.concurrent.Immutable; + +interface MailboxApi { + + /** + * Sets up the mailbox with the setup token. + * + * @param properties MailboxProperties with the setup token + * @return the owner token + * @throws PermanentFailureException for 401 response. + */ + String setup(MailboxProperties properties) + throws IOException, PermanentFailureException; + + /** + * Checks the status of the mailbox. + * + * @return true if the status is OK, false otherwise. + * @throws PermanentFailureException for 401 response. + */ + 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 { + /** + * If true, the failure is fatal and requires user attention. + * The entire task queue will most likely need to stop. + */ + final boolean fatal; + + PermanentFailureException(boolean fatal) { + this.fatal = fatal; + } + } +} 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 new file mode 100644 index 000000000..30c52212d --- /dev/null +++ b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxApiImpl.java @@ -0,0 +1,87 @@ +package org.briarproject.bramble.mailbox; + +import com.fasterxml.jackson.core.JacksonException; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.json.JsonMapper; + +import org.briarproject.bramble.api.WeakSingletonProvider; +import org.briarproject.bramble.api.nullsafety.NotNullByDefault; + +import java.io.IOException; + +import javax.inject.Inject; + +import okhttp3.OkHttpClient; +import okhttp3.Request; +import okhttp3.Response; +import okhttp3.ResponseBody; + +import static com.fasterxml.jackson.databind.MapperFeature.BLOCK_UNSAFE_POLYMORPHIC_BASE_TYPES; +import static okhttp3.internal.Util.EMPTY_REQUEST; + +@NotNullByDefault +class MailboxApiImpl implements MailboxApi { + + private final WeakSingletonProvider httpClientProvider; + private final JsonMapper mapper = JsonMapper.builder() + .enable(BLOCK_UNSAFE_POLYMORPHIC_BASE_TYPES) + .build(); + + @Inject + MailboxApiImpl(WeakSingletonProvider httpClientProvider) { + this.httpClientProvider = httpClientProvider; + } + + @Override + public String setup(MailboxProperties properties) + throws IOException, PermanentFailureException { + if (!properties.isOwner) throw new IllegalArgumentException(); + Request request = getRequestBuilder(properties.token) + .url(properties.baseUrl + "/setup") + .put(EMPTY_REQUEST) + .build(); + OkHttpClient client = httpClientProvider.get(); + Response response = client.newCall(request).execute(); + if (response.code() == 401) { + throw new PermanentFailureException(true); + } + if (!response.isSuccessful()) throw new IOException(); + ResponseBody body = response.body(); + if (body == null) throw new PermanentFailureException(false); + try { + JsonNode node = mapper.readTree(body.string()); + JsonNode tokenNode = node.get("token"); + if (tokenNode == null) { + throw new PermanentFailureException(false); + } + String ownerToken = tokenNode.textValue(); + if (ownerToken == null) { + throw new PermanentFailureException(false); + } + return ownerToken; + } catch (JacksonException e) { + throw new PermanentFailureException(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") + .build(); + OkHttpClient client = httpClientProvider.get(); + Response response = client.newCall(request).execute(); + if (response.code() == 401) { + throw new PermanentFailureException(true); + } + return response.isSuccessful(); + } + + private Request.Builder getRequestBuilder(String token) { + return new Request.Builder() + .addHeader("Authorization", "Bearer " + token); + } + +} 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 new file mode 100644 index 000000000..374d99eec --- /dev/null +++ b/bramble-core/src/test/java/org/briarproject/bramble/mailbox/MailboxApiTest.java @@ -0,0 +1,193 @@ +package org.briarproject.bramble.mailbox; + +import org.briarproject.bramble.api.WeakSingletonProvider; +import org.briarproject.bramble.mailbox.MailboxApi.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; + +import okhttp3.OkHttpClient; +import okhttp3.mockwebserver.MockResponse; +import okhttp3.mockwebserver.MockWebServer; +import okhttp3.mockwebserver.RecordedRequest; + +import static java.util.concurrent.TimeUnit.MILLISECONDS; +import static org.briarproject.bramble.util.StringUtils.getRandomString; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; + +public class MailboxApiTest extends BrambleTestCase { + + private final OkHttpClient client = new OkHttpClient.Builder() + .socketFactory(SocketFactory.getDefault()) + .connectTimeout(60_000, MILLISECONDS) + .build(); + private final WeakSingletonProvider httpClientProvider = + new WeakSingletonProvider() { + @Override + @Nonnull + public OkHttpClient createInstance() { + return client; + } + }; + private final MailboxApiImpl api = new MailboxApiImpl(httpClientProvider); + + private final String token = getRandomString(64); + private final String token2 = getRandomString(64); + + @Test + public void testSetup() throws Exception { + String validResponse = "{\"token\":\"" + token2 + "\"}"; + String invalidResponse = "{\"foo\":\"bar\"}"; + String invalidTokenResponse = "{\"token\":{\"foo\":\"bar\"}}"; + + MockWebServer server = new MockWebServer(); + server.enqueue(new MockResponse().setBody(validResponse)); + server.enqueue(new MockResponse()); + server.enqueue(new MockResponse().setBody(invalidResponse)); + server.enqueue(new MockResponse().setResponseCode(401)); + server.enqueue(new MockResponse().setResponseCode(500)); + server.enqueue(new MockResponse().setBody(invalidTokenResponse)); + server.start(); + String baseUrl = getBaseUrl(server); + MailboxProperties properties = + new MailboxProperties(baseUrl, token, true); + MailboxProperties properties2 = + new MailboxProperties(baseUrl, token2, true); + + assertEquals(token2, api.setup(properties)); + + PermanentFailureException e2 = + assertThrows(PermanentFailureException.class, + () -> api.setup(properties) + ); + + PermanentFailureException e3 = + assertThrows(PermanentFailureException.class, + () -> api.setup(properties) + ); + + PermanentFailureException e4 = assertThrows( + PermanentFailureException.class, + () -> api.setup(properties2) + ); + + assertThrows(IOException.class, + () -> api.setup(properties) + ); + + PermanentFailureException e6 = assertThrows( + PermanentFailureException.class, + () -> api.setup(properties) + ); + + RecordedRequest request1 = server.takeRequest(); + assertEquals("/setup", request1.getPath()); + assertEquals("PUT", request1.getMethod()); + assertToken(request1, token); + + RecordedRequest request2 = server.takeRequest(); + assertEquals("/setup", request2.getPath()); + assertEquals("PUT", request2.getMethod()); + assertToken(request2, token); + assertFalse(e2.fatal); + + RecordedRequest request3 = server.takeRequest(); + assertEquals("/setup", request3.getPath()); + assertEquals("PUT", request3.getMethod()); + assertToken(request3, token); + assertFalse(e3.fatal); + + RecordedRequest request4 = server.takeRequest(); + assertEquals("/setup", request4.getPath()); + assertEquals("PUT", request4.getMethod()); + assertToken(request4, token2); + assertTrue(e4.fatal); + + RecordedRequest request5 = server.takeRequest(); + assertEquals("/setup", request5.getPath()); + assertEquals("PUT", request5.getMethod()); + assertToken(request5, token); + + RecordedRequest request6 = server.takeRequest(); + assertEquals("/setup", request6.getPath()); + assertEquals("PUT", request6.getMethod()); + assertToken(request6, token); + assertFalse(e6.fatal); + } + + @Test + public void testSetupOnlyForOwner() { + MailboxProperties properties = + new MailboxProperties("", token, false); + assertThrows( + IllegalArgumentException.class, + () -> api.setup(properties) + ); + } + + @Test + public void testStatus() throws Exception { + MockWebServer server = new MockWebServer(); + server.enqueue(new MockResponse()); + server.enqueue(new MockResponse().setResponseCode(401)); + server.enqueue(new MockResponse().setResponseCode(500)); + server.start(); + String baseUrl = getBaseUrl(server); + MailboxProperties properties = + new MailboxProperties(baseUrl, token, true); + MailboxProperties properties2 = + new MailboxProperties(baseUrl, token2, true); + + assertTrue(api.checkStatus(properties)); + + PermanentFailureException e2 = assertThrows( + PermanentFailureException.class, + () -> api.checkStatus(properties2) + ); + + assertFalse(api.checkStatus(properties)); + + RecordedRequest request1 = server.takeRequest(); + assertEquals("/status", request1.getPath()); + assertToken(request1, token); + + RecordedRequest request2 = server.takeRequest(); + assertEquals("/status", request2.getPath()); + assertToken(request2, token2); + assertTrue(e2.fatal); + + RecordedRequest request3 = server.takeRequest(); + assertEquals("/status", request3.getPath()); + assertToken(request3, token); + } + + @Test + public void testStatusOnlyForOwner() { + MailboxProperties properties = + new MailboxProperties("", token, false); + assertThrows( + IllegalArgumentException.class, + () -> api.checkStatus(properties) + ); + } + + private String getBaseUrl(MockWebServer server) { + String baseUrl = server.url("").toString(); + return baseUrl.substring(0, baseUrl.length() - 1); + } + + private void assertToken(RecordedRequest request, String token) { + assertNotNull(request.getHeader("Authorization")); + assertEquals("Bearer " + token, request.getHeader("Authorization")); + } + +} diff --git a/bramble-core/witness.gradle b/bramble-core/witness.gradle index e60375e4b..b38b997b2 100644 --- a/bramble-core/witness.gradle +++ b/bramble-core/witness.gradle @@ -1,6 +1,9 @@ dependencyVerification { verify = [ 'cglib:cglib:3.2.8:cglib-3.2.8.jar:3f64de999ecc5595dc84ca8ff0879d8a34c8623f9ef3c517a53ed59023fcb9db', + 'com.fasterxml.jackson.core:jackson-annotations:2.13.0:jackson-annotations-2.13.0.jar:81f9724d8843e8b08f8f6c0609e7a2b030d00c34861c4ac7e2099a7235047d6f', + 'com.fasterxml.jackson.core:jackson-core:2.13.0:jackson-core-2.13.0.jar:348bc59b348df2e807b356f1d62d2afb41a974073328abc773eb0932b855d2c8', + 'com.fasterxml.jackson.core:jackson-databind:2.13.0:jackson-databind-2.13.0.jar:9c826d27176268777adcf97e1c6e2051c7e33a7aaa2c370c2e8c6077fd9da3f4', 'com.google.code.findbugs:annotations:3.0.1:annotations-3.0.1.jar:6b47ff0a6de0ce17cbedc3abb0828ca5bce3009d53ea47b3723ff023c4742f79', 'com.google.code.findbugs:jsr305:3.0.2:jsr305-3.0.2.jar:766ad2a0783f2687962c8ad74ceecc38a28b9f72a2d085ee438b7813e928d0c7', 'com.google.dagger:dagger-compiler:2.33:dagger-compiler-2.33.jar:aa8a0d8370c578fd6999802d0d90b9829377a46d2c1141e11b8f737970e7155e', @@ -15,8 +18,11 @@ dependencyVerification { 'com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava:listenablefuture-9999.0-empty-to-avoid-conflict-with-guava.jar:b372a037d4230aa57fbeffdef30fd6123f9c0c2db85d0aced00c91b974f33f99', 'com.google.j2objc:j2objc-annotations:1.1:j2objc-annotations-1.1.jar:2994a7eb78f2710bd3d3bfb639b2c94e219cedac0d4d084d516e78c16dddecf6', 'com.h2database:h2:1.4.192:h2-1.4.192.jar:225b22e9857235c46c93861410b60b8c81c10dc8985f4faf188985ba5445126c', + 'com.squareup.okhttp3:mockwebserver:4.9.3:mockwebserver-4.9.3.jar:9c8c581c29f22f877a35d11380462f75bb24bf1886204fe835ee695594a2784e', 'com.squareup.okhttp3:okhttp:3.12.13:okhttp-3.12.13.jar:508234e024ef7e270ab1a6d5b356f5b98e786511239ca986d684fd1e2cf7bc82', + 'com.squareup.okhttp3:okhttp:4.9.3:okhttp-4.9.3.jar:93ecd6cba19d87dccfe566ec848d91aae799e3cf16c00709358ea69bd9227219', 'com.squareup.okio:okio:1.15.0:okio-1.15.0.jar:693fa319a7e8843300602b204023b7674f106ebcb577f2dd5807212b66118bd2', + 'com.squareup.okio:okio:2.8.0:okio-jvm-2.8.0.jar:4496b06e73982fcdd8a5393f46e5df2ce2fa4465df5895454cac68a32f09bbc8', 'com.squareup:javapoet:1.13.0:javapoet-1.13.0.jar:4c7517e848a71b36d069d12bb3bf46a70fd4cda3105d822b0ed2e19c00b69291', 'javax.annotation:jsr250-api:1.0:jsr250-api-1.0.jar:a1a922d0d9b6d183ed3800dfac01d1e1eb159f0e8c6f94736931c1def54a941f', 'javax.inject:javax.inject:1:javax.inject-1.jar:91c77044a50c481636c32d916fd89c9118a72195390452c81065080f957de7ff', @@ -40,7 +46,11 @@ dependencyVerification { 'org.hamcrest:hamcrest-library:2.1:hamcrest-library-2.1.jar:b7e2b6895b3b679f0e47b6380fda391b225e9b78505db9d8bdde8d3cc8d52a21', 'org.hamcrest:hamcrest:2.1:hamcrest-2.1.jar:ba93b2e3a562322ba432f0a1b53addcc55cb188253319a020ed77f824e692050', 'org.hsqldb:hsqldb:2.3.5:hsqldb-2.3.5.jar:6676a6977ac98997a80f827ddbd3fe8ca1e0853dad1492512135fd1a222ccfad', + 'org.jetbrains.kotlin:kotlin-stdlib-common:1.4.10:kotlin-stdlib-common-1.4.10.jar:4681f2d436a68c7523595d84ed5758e1382f9da0f67c91e6a848690d711274fe', 'org.jetbrains.kotlin:kotlin-stdlib-common:1.4.20:kotlin-stdlib-common-1.4.20.jar:a7112c9b3cefee418286c9c9372f7af992bd1e6e030691d52f60cb36dbec8320', + 'org.jetbrains.kotlin:kotlin-stdlib-jdk7:1.4.10:kotlin-stdlib-jdk7-1.4.10.jar:f9566380c08722c780ce33ceee23e98ddf765ca98fabd3e2fabae7975c8d232b', + 'org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.4.10:kotlin-stdlib-jdk8-1.4.10.jar:39b7a9442d7a3865e0f4a732c56c1d5da0e11ffb3bb82a461d32deb0c0ca7673', + 'org.jetbrains.kotlin:kotlin-stdlib:1.4.10:kotlin-stdlib-1.4.10.jar:01ecb09782c042b931c1839acf21a188340b295d05400afd6e3415d4475b8daa', 'org.jetbrains.kotlin:kotlin-stdlib:1.4.20:kotlin-stdlib-1.4.20.jar:b8ab1da5cdc89cb084d41e1f28f20a42bd431538642a5741c52bbfae3fa3e656', 'org.jetbrains.kotlinx:kotlinx-metadata-jvm:0.1.0:kotlinx-metadata-jvm-0.1.0.jar:9753bb39efef35957c5c15df9a3cb769aabf2cdfa74b47afcb7760e5146be3b5', 'org.jetbrains:annotations:13.0:annotations-13.0.jar:ace2a10dc8e2d5fd34925ecac03e4988b2c0f851650c94b8cef49ba1bd111478', diff --git a/briar-headless/build.gradle b/briar-headless/build.gradle index d95750adf..14545dbd3 100644 --- a/briar-headless/build.gradle +++ b/briar-headless/build.gradle @@ -23,7 +23,7 @@ dependencies { implementation 'org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.4.32' implementation 'io.javalin:javalin:3.5.0' implementation 'org.slf4j:slf4j-simple:1.7.30' - implementation 'com.fasterxml.jackson.core:jackson-databind:2.12.2' + implementation "com.fasterxml.jackson.core:jackson-databind:$jackson_version" implementation 'com.github.ajalt:clikt:2.2.0' def daggerVersion = '2.24' diff --git a/briar-headless/witness.gradle b/briar-headless/witness.gradle index 43237e81a..5935eb0da 100644 --- a/briar-headless/witness.gradle +++ b/briar-headless/witness.gradle @@ -1,8 +1,8 @@ dependencyVerification { verify = [ - 'com.fasterxml.jackson.core:jackson-annotations:2.12.2:jackson-annotations-2.12.2.jar:558561786c071af202e849b6ae3d39c87ed417ecc83d45e398c12eb3bffa557b', - 'com.fasterxml.jackson.core:jackson-core:2.12.2:jackson-core-2.12.2.jar:7883331763729b72735fdd8a117f32eb7d22695babfb37cc99df8392c196efc3', - 'com.fasterxml.jackson.core:jackson-databind:2.12.2:jackson-databind-2.12.2.jar:c4002f861d8d33f3202bf8effabb53acc320c5276cc50c1bfaae73c36ce8db32', + 'com.fasterxml.jackson.core:jackson-annotations:2.13.0:jackson-annotations-2.13.0.jar:81f9724d8843e8b08f8f6c0609e7a2b030d00c34861c4ac7e2099a7235047d6f', + 'com.fasterxml.jackson.core:jackson-core:2.13.0:jackson-core-2.13.0.jar:348bc59b348df2e807b356f1d62d2afb41a974073328abc773eb0932b855d2c8', + 'com.fasterxml.jackson.core:jackson-databind:2.13.0:jackson-databind-2.13.0.jar:9c826d27176268777adcf97e1c6e2051c7e33a7aaa2c370c2e8c6077fd9da3f4', 'com.github.ajalt:clikt:2.2.0:clikt-2.2.0.jar:beb3136d06764ec8ce0810a8fd6c8b7b49d04287d1deef3a07c016e43a458d33', 'com.github.gundy:semver4j:0.16.4:semver4j-0.16.4.jar:def9b4225fa37219e18f81d01f0e52d73dca1257a38f5475be9dd58f87736510', 'com.google.code.findbugs:jsr305:3.0.2:jsr305-3.0.2.jar:766ad2a0783f2687962c8ad74ceecc38a28b9f72a2d085ee438b7813e928d0c7', diff --git a/build.gradle b/build.gradle index d5863b881..dfb46b7c5 100644 --- a/build.gradle +++ b/build.gradle @@ -37,6 +37,7 @@ buildscript { dagger_version = "2.33" // okhttp 3.12.x is supported until end of 2021, newer versions need minSdk 21 okhttp_version = "3.12.13" + jackson_version = "2.13.0" tor_version = "0.3.5.17" obfs4proxy_version = "0.0.12-dev-40245c4a" junit_version = "4.13.2" From 9f1757ccaf4e0f0571cc3a957079a0a22e3a25c4 Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Wed, 15 Dec 2021 16:54:04 -0300 Subject: [PATCH 2/3] Remove concept of fatal permanent exceptions All exceptions will just cause the request to be tried again with some backoff. --- .../bramble/mailbox/MailboxApi.java | 9 ----- .../bramble/mailbox/MailboxApiImpl.java | 16 +++----- .../bramble/mailbox/MailboxApiTest.java | 38 +++++-------------- 3 files changed, 16 insertions(+), 47 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 77add349d..97539bd84 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 @@ -40,14 +40,5 @@ interface MailboxApi { @Immutable class PermanentFailureException extends Exception { - /** - * If true, the failure is fatal and requires user attention. - * The entire task queue will most likely need to stop. - */ - final boolean fatal; - - PermanentFailureException(boolean fatal) { - this.fatal = fatal; - } } } 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 30c52212d..df35d5723 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 @@ -42,25 +42,23 @@ class MailboxApiImpl implements MailboxApi { .build(); OkHttpClient client = httpClientProvider.get(); Response response = client.newCall(request).execute(); - if (response.code() == 401) { - throw new PermanentFailureException(true); - } + if (response.code() == 401) throw new PermanentFailureException(); if (!response.isSuccessful()) throw new IOException(); ResponseBody body = response.body(); - if (body == null) throw new PermanentFailureException(false); + if (body == null) throw new PermanentFailureException(); try { JsonNode node = mapper.readTree(body.string()); JsonNode tokenNode = node.get("token"); if (tokenNode == null) { - throw new PermanentFailureException(false); + throw new PermanentFailureException(); } String ownerToken = tokenNode.textValue(); if (ownerToken == null) { - throw new PermanentFailureException(false); + throw new PermanentFailureException(); } return ownerToken; } catch (JacksonException e) { - throw new PermanentFailureException(false); + throw new PermanentFailureException(); } } @@ -73,9 +71,7 @@ class MailboxApiImpl implements MailboxApi { .build(); OkHttpClient client = httpClientProvider.get(); Response response = client.newCall(request).execute(); - if (response.code() == 401) { - throw new PermanentFailureException(true); - } + if (response.code() == 401) throw new PermanentFailureException(); return response.isSuccessful(); } 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 374d99eec..42054f5fc 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 @@ -65,27 +65,15 @@ public class MailboxApiTest extends BrambleTestCase { assertEquals(token2, api.setup(properties)); - PermanentFailureException e2 = - assertThrows(PermanentFailureException.class, - () -> api.setup(properties) - ); - - PermanentFailureException e3 = - assertThrows(PermanentFailureException.class, - () -> api.setup(properties) - ); - - PermanentFailureException e4 = assertThrows( - PermanentFailureException.class, - () -> api.setup(properties2) - ); - + assertThrows(PermanentFailureException.class, + () -> api.setup(properties)); + assertThrows(PermanentFailureException.class, + () -> api.setup(properties)); + assertThrows(PermanentFailureException.class, + () -> api.setup(properties2)); assertThrows(IOException.class, - () -> api.setup(properties) - ); - - PermanentFailureException e6 = assertThrows( - PermanentFailureException.class, + () -> api.setup(properties)); + assertThrows(PermanentFailureException.class, () -> api.setup(properties) ); @@ -98,19 +86,16 @@ public class MailboxApiTest extends BrambleTestCase { assertEquals("/setup", request2.getPath()); assertEquals("PUT", request2.getMethod()); assertToken(request2, token); - assertFalse(e2.fatal); RecordedRequest request3 = server.takeRequest(); assertEquals("/setup", request3.getPath()); assertEquals("PUT", request3.getMethod()); assertToken(request3, token); - assertFalse(e3.fatal); RecordedRequest request4 = server.takeRequest(); assertEquals("/setup", request4.getPath()); assertEquals("PUT", request4.getMethod()); assertToken(request4, token2); - assertTrue(e4.fatal); RecordedRequest request5 = server.takeRequest(); assertEquals("/setup", request5.getPath()); @@ -121,7 +106,6 @@ public class MailboxApiTest extends BrambleTestCase { assertEquals("/setup", request6.getPath()); assertEquals("PUT", request6.getMethod()); assertToken(request6, token); - assertFalse(e6.fatal); } @Test @@ -149,9 +133,8 @@ public class MailboxApiTest extends BrambleTestCase { assertTrue(api.checkStatus(properties)); - PermanentFailureException e2 = assertThrows( - PermanentFailureException.class, - () -> api.checkStatus(properties2) + assertThrows(PermanentFailureException.class, () -> + api.checkStatus(properties2) ); assertFalse(api.checkStatus(properties)); @@ -163,7 +146,6 @@ public class MailboxApiTest extends BrambleTestCase { RecordedRequest request2 = server.takeRequest(); assertEquals("/status", request2.getPath()); assertToken(request2, token2); - assertTrue(e2.fatal); RecordedRequest request3 = server.takeRequest(); assertEquals("/status", request3.getPath()); From 421b00517fd14dec038948043e25b0915f915d6a Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Thu, 6 Jan 2022 14:11:19 -0300 Subject: [PATCH 3/3] 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);