From e6917d8427e272046e70c723c02e60b5c3f63a78 Mon Sep 17 00:00:00 2001 From: Sergey Skrobotov Date: Wed, 31 May 2023 23:39:02 -0700 Subject: [PATCH] minor cleanup, docs, and integration tests for username API --- .../org/signal/integration/Operations.java | 9 ++ .../org/signal/integration/AccountTest.java | 121 +++++++++++++++ .../signal/integration/IntegrationTest.java | 140 ------------------ .../org/signal/integration/MessagingTest.java | 60 ++++++++ .../signal/integration/RegistrationTest.java | 54 +++++++ .../controllers/AccountController.java | 103 +++++++------ .../controllers/AccountControllerTest.java | 2 +- 7 files changed, 306 insertions(+), 183 deletions(-) create mode 100644 integration-tests/src/test/java/org/signal/integration/AccountTest.java delete mode 100644 integration-tests/src/test/java/org/signal/integration/IntegrationTest.java create mode 100644 integration-tests/src/test/java/org/signal/integration/MessagingTest.java create mode 100644 integration-tests/src/test/java/org/signal/integration/RegistrationTest.java diff --git a/integration-tests/src/main/java/org/signal/integration/Operations.java b/integration-tests/src/main/java/org/signal/integration/Operations.java index e57eb8605..e59f64d64 100644 --- a/integration-tests/src/main/java/org/signal/integration/Operations.java +++ b/integration-tests/src/main/java/org/signal/integration/Operations.java @@ -269,6 +269,15 @@ public final class Operations { return requireNonNull(execute.getRight()); } + public void executeExpectStatusCode(final int expectedStatusCode) { + final Pair execute = execute(Void.class); + Validate.isTrue( + execute.getLeft() == expectedStatusCode, + "Unexpected response code: %d", + execute.getLeft() + ); + } + public Pair execute(final Class expectedType) { builder.uri(serverUri(endpoint, queryParams)) .header(HttpHeaders.USER_AGENT, USER_AGENT); diff --git a/integration-tests/src/test/java/org/signal/integration/AccountTest.java b/integration-tests/src/test/java/org/signal/integration/AccountTest.java new file mode 100644 index 000000000..4768ab87c --- /dev/null +++ b/integration-tests/src/test/java/org/signal/integration/AccountTest.java @@ -0,0 +1,121 @@ +/* + * Copyright 2023 Signal Messenger, LLC + * SPDX-License-Identifier: AGPL-3.0-only + */ + +package org.signal.integration; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import java.util.Arrays; +import java.util.Base64; +import java.util.List; +import org.apache.commons.lang3.tuple.Pair; +import org.apache.http.HttpStatus; +import org.junit.jupiter.api.Test; +import org.signal.libsignal.usernames.BaseUsernameException; +import org.signal.libsignal.usernames.Username; +import org.whispersystems.textsecuregcm.entities.AccountIdentifierResponse; +import org.whispersystems.textsecuregcm.entities.AccountIdentityResponse; +import org.whispersystems.textsecuregcm.entities.ConfirmUsernameHashRequest; +import org.whispersystems.textsecuregcm.entities.ReserveUsernameHashRequest; +import org.whispersystems.textsecuregcm.entities.ReserveUsernameHashResponse; +import org.whispersystems.textsecuregcm.entities.UsernameHashResponse; + +public class AccountTest { + + @Test + public void testCreateAccount() throws Exception { + final TestUser user = Operations.newRegisteredUser("+19995550101"); + try { + final Pair execute = Operations.apiGet("/v1/accounts/whoami") + .authorized(user) + .execute(AccountIdentityResponse.class); + assertEquals(HttpStatus.SC_OK, execute.getLeft()); + } finally { + Operations.deleteUser(user); + } + } + + @Test + public void testCreateAccountAtomic() throws Exception { + final TestUser user = Operations.newRegisteredUserAtomic("+19995550201"); + try { + final Pair execute = Operations.apiGet("/v1/accounts/whoami") + .authorized(user) + .execute(AccountIdentityResponse.class); + assertEquals(HttpStatus.SC_OK, execute.getLeft()); + } finally { + Operations.deleteUser(user); + } + } + + @Test + public void testUsernameOperations() throws Exception { + final TestUser user = Operations.newRegisteredUser("+19995550102"); + try { + verifyFullUsernameLifecycle(user); + // no do it again to check changing usernames + verifyFullUsernameLifecycle(user); + } finally { + Operations.deleteUser(user); + } + } + + private static void verifyFullUsernameLifecycle(final TestUser user) throws BaseUsernameException { + final String preferred = "test"; + final List candidates = Username.candidatesFrom(preferred, preferred.length(), preferred.length() + 1); + + // reserve a username + final ReserveUsernameHashRequest reserveUsernameHashRequest = new ReserveUsernameHashRequest( + candidates.stream().map(Username::getHash).toList()); + // try unauthorized + Operations + .apiPut("/v1/accounts/username_hash/reserve", reserveUsernameHashRequest) + .executeExpectStatusCode(HttpStatus.SC_UNAUTHORIZED); + + final ReserveUsernameHashResponse reserveUsernameHashResponse = Operations + .apiPut("/v1/accounts/username_hash/reserve", reserveUsernameHashRequest) + .authorized(user) + .executeExpectSuccess(ReserveUsernameHashResponse.class); + + // find which one is the reserved username + final byte[] reservedHash = reserveUsernameHashResponse.usernameHash(); + final Username reservedUsername = candidates.stream() + .filter(u -> Arrays.equals(u.getHash(), reservedHash)) + .findAny() + .orElseThrow(); + + // confirm a username + final ConfirmUsernameHashRequest confirmUsernameHashRequest = new ConfirmUsernameHashRequest( + reservedUsername.getHash(), + reservedUsername.generateProof() + ); + // try unauthorized + Operations + .apiPut("/v1/accounts/username_hash/confirm", confirmUsernameHashRequest) + .executeExpectStatusCode(HttpStatus.SC_UNAUTHORIZED); + Operations + .apiPut("/v1/accounts/username_hash/confirm", confirmUsernameHashRequest) + .authorized(user) + .executeExpectSuccess(UsernameHashResponse.class); + + + // lookup username + final AccountIdentifierResponse accountIdentifierResponse = Operations + .apiGet("/v1/accounts/username_hash/" + Base64.getUrlEncoder().encodeToString(reservedHash)) + .executeExpectSuccess(AccountIdentifierResponse.class); + assertEquals(user.aciUuid(), accountIdentifierResponse.uuid()); + // try authorized + Operations + .apiGet("/v1/accounts/username_hash/" + Base64.getUrlEncoder().encodeToString(reservedHash)) + .authorized(user) + .executeExpectStatusCode(HttpStatus.SC_BAD_REQUEST); + + // delete username + Operations + .apiDelete("/v1/accounts/username_hash") + .authorized(user) + .executeExpectSuccess(); + } +} diff --git a/integration-tests/src/test/java/org/signal/integration/IntegrationTest.java b/integration-tests/src/test/java/org/signal/integration/IntegrationTest.java deleted file mode 100644 index a6df3db81..000000000 --- a/integration-tests/src/test/java/org/signal/integration/IntegrationTest.java +++ /dev/null @@ -1,140 +0,0 @@ -/* - * Copyright 2023 Signal Messenger, LLC - * SPDX-License-Identifier: AGPL-3.0-only - */ - -package org.signal.integration; - -import static org.junit.jupiter.api.Assertions.assertArrayEquals; -import static org.junit.jupiter.api.Assertions.assertEquals; - -import java.nio.charset.StandardCharsets; -import java.util.Base64; -import java.util.List; -import org.apache.commons.lang3.tuple.Pair; -import org.junit.jupiter.api.Assertions; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.ValueSource; -import org.whispersystems.textsecuregcm.entities.AccountIdentityResponse; -import org.whispersystems.textsecuregcm.entities.CreateVerificationSessionRequest; -import org.whispersystems.textsecuregcm.entities.IncomingMessage; -import org.whispersystems.textsecuregcm.entities.IncomingMessageList; -import org.whispersystems.textsecuregcm.entities.OutgoingMessageEntityList; -import org.whispersystems.textsecuregcm.entities.SendMessageResponse; -import org.whispersystems.textsecuregcm.entities.SubmitVerificationCodeRequest; -import org.whispersystems.textsecuregcm.entities.UpdateVerificationSessionRequest; -import org.whispersystems.textsecuregcm.entities.VerificationCodeRequest; -import org.whispersystems.textsecuregcm.entities.VerificationSessionResponse; -import org.whispersystems.textsecuregcm.storage.Device; - -public class IntegrationTest { - - @Test - public void testCreateAccount() throws Exception { - final TestUser user = Operations.newRegisteredUser("+19995550101"); - try { - final Pair execute = Operations.apiGet("/v1/accounts/whoami") - .authorized(user) - .execute(AccountIdentityResponse.class); - assertEquals(200, execute.getLeft()); - } finally { - Operations.deleteUser(user); - } - } - - @Test - public void testCreateAccountAtomic() throws Exception { - final TestUser user = Operations.newRegisteredUserAtomic("+19995550201"); - try { - final Pair execute = Operations.apiGet("/v1/accounts/whoami") - .authorized(user) - .execute(AccountIdentityResponse.class); - assertEquals(200, execute.getLeft()); - } finally { - Operations.deleteUser(user); - } - } - - @Test - public void testRegistration() throws Exception { - final UpdateVerificationSessionRequest originalRequest = new UpdateVerificationSessionRequest( - "test", UpdateVerificationSessionRequest.PushTokenType.FCM, null, null, null, null); - final CreateVerificationSessionRequest input = new CreateVerificationSessionRequest("+19995550102", originalRequest); - - final VerificationSessionResponse verificationSessionResponse = Operations - .apiPost("/v1/verification/session", input) - .executeExpectSuccess(VerificationSessionResponse.class); - System.out.println("session created: " + verificationSessionResponse); - - final String sessionId = verificationSessionResponse.id(); - final String pushChallenge = Operations.peekVerificationSessionPushChallenge(sessionId); - - // supply push challenge - final UpdateVerificationSessionRequest updatedRequest = new UpdateVerificationSessionRequest( - "test", UpdateVerificationSessionRequest.PushTokenType.FCM, pushChallenge, null, null, null); - final VerificationSessionResponse pushChallengeSupplied = Operations - .apiPatch("/v1/verification/session/%s".formatted(sessionId), updatedRequest) - .executeExpectSuccess(VerificationSessionResponse.class); - System.out.println("push challenge supplied: " + pushChallengeSupplied); - - Assertions.assertTrue(pushChallengeSupplied.allowedToRequestCode()); - - // request code - final VerificationCodeRequest verificationCodeRequest = new VerificationCodeRequest( - VerificationCodeRequest.Transport.SMS, "android-ng"); - - final VerificationSessionResponse codeRequested = Operations - .apiPost("/v1/verification/session/%s/code".formatted(sessionId), verificationCodeRequest) - .executeExpectSuccess(VerificationSessionResponse.class); - System.out.println("code requested: " + codeRequested); - - // verify code - final SubmitVerificationCodeRequest submitVerificationCodeRequest = new SubmitVerificationCodeRequest("265402"); - final VerificationSessionResponse codeVerified = Operations - .apiPut("/v1/verification/session/%s/code".formatted(sessionId), submitVerificationCodeRequest) - .executeExpectSuccess(VerificationSessionResponse.class); - System.out.println("sms code supplied: " + codeVerified); - } - - @ParameterizedTest - @ValueSource(booleans = {true, false}) - public void testSendMessageUnsealed(final boolean atomicAccountCreation) throws Exception { - final TestUser userA; - final TestUser userB; - - if (atomicAccountCreation) { - userA = Operations.newRegisteredUserAtomic("+19995550102"); - userB = Operations.newRegisteredUserAtomic("+19995550103"); - } else { - userA = Operations.newRegisteredUser("+19995550102"); - userB = Operations.newRegisteredUser("+19995550103"); - } - - try { - final byte[] expectedContent = "Hello, World!".getBytes(StandardCharsets.UTF_8); - final String contentBase64 = Base64.getEncoder().encodeToString(expectedContent); - final IncomingMessage message = new IncomingMessage(1, Device.MASTER_ID, userB.registrationId(), contentBase64); - final IncomingMessageList messages = new IncomingMessageList(List.of(message), false, true, System.currentTimeMillis()); - - System.out.println("Sending message"); - final Pair sendMessage = Operations - .apiPut("/v1/messages/%s".formatted(userB.aciUuid().toString()), messages) - .authorized(userA) - .execute(SendMessageResponse.class); - System.out.println("Message sent: " + sendMessage); - - System.out.println("Receive message"); - final Pair receiveMessages = Operations.apiGet("/v1/messages") - .authorized(userB) - .execute(OutgoingMessageEntityList.class); - System.out.println("Message received: " + receiveMessages); - - final byte[] actualContent = receiveMessages.getRight().messages().get(0).content(); - assertArrayEquals(expectedContent, actualContent); - } finally { - Operations.deleteUser(userA); - Operations.deleteUser(userB); - } - } -} diff --git a/integration-tests/src/test/java/org/signal/integration/MessagingTest.java b/integration-tests/src/test/java/org/signal/integration/MessagingTest.java new file mode 100644 index 000000000..634e55066 --- /dev/null +++ b/integration-tests/src/test/java/org/signal/integration/MessagingTest.java @@ -0,0 +1,60 @@ +/* + * Copyright 2023 Signal Messenger, LLC + * SPDX-License-Identifier: AGPL-3.0-only + */ + +package org.signal.integration; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; + +import java.nio.charset.StandardCharsets; +import java.util.Base64; +import java.util.List; +import org.apache.commons.lang3.tuple.Pair; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import org.whispersystems.textsecuregcm.entities.IncomingMessage; +import org.whispersystems.textsecuregcm.entities.IncomingMessageList; +import org.whispersystems.textsecuregcm.entities.OutgoingMessageEntityList; +import org.whispersystems.textsecuregcm.entities.SendMessageResponse; +import org.whispersystems.textsecuregcm.storage.Device; + +public class MessagingTest { + + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void testSendMessageUnsealed(final boolean atomicAccountCreation) throws Exception { + final TestUser userA; + final TestUser userB; + + if (atomicAccountCreation) { + userA = Operations.newRegisteredUserAtomic("+19995550102"); + userB = Operations.newRegisteredUserAtomic("+19995550103"); + } else { + userA = Operations.newRegisteredUser("+19995550104"); + userB = Operations.newRegisteredUser("+19995550105"); + } + + try { + final byte[] expectedContent = "Hello, World!".getBytes(StandardCharsets.UTF_8); + final String contentBase64 = Base64.getEncoder().encodeToString(expectedContent); + final IncomingMessage message = new IncomingMessage(1, Device.MASTER_ID, userB.registrationId(), contentBase64); + final IncomingMessageList messages = new IncomingMessageList(List.of(message), false, true, System.currentTimeMillis()); + + final Pair sendMessage = Operations + .apiPut("/v1/messages/%s".formatted(userB.aciUuid().toString()), messages) + .authorized(userA) + .execute(SendMessageResponse.class); + + final Pair receiveMessages = Operations.apiGet("/v1/messages") + .authorized(userB) + .execute(OutgoingMessageEntityList.class); + + final byte[] actualContent = receiveMessages.getRight().messages().get(0).content(); + assertArrayEquals(expectedContent, actualContent); + } finally { + Operations.deleteUser(userA); + Operations.deleteUser(userB); + } + } +} diff --git a/integration-tests/src/test/java/org/signal/integration/RegistrationTest.java b/integration-tests/src/test/java/org/signal/integration/RegistrationTest.java new file mode 100644 index 000000000..b27faf800 --- /dev/null +++ b/integration-tests/src/test/java/org/signal/integration/RegistrationTest.java @@ -0,0 +1,54 @@ +/* + * Copyright 2023 Signal Messenger, LLC + * SPDX-License-Identifier: AGPL-3.0-only + */ + +package org.signal.integration; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; +import org.whispersystems.textsecuregcm.entities.CreateVerificationSessionRequest; +import org.whispersystems.textsecuregcm.entities.SubmitVerificationCodeRequest; +import org.whispersystems.textsecuregcm.entities.UpdateVerificationSessionRequest; +import org.whispersystems.textsecuregcm.entities.VerificationCodeRequest; +import org.whispersystems.textsecuregcm.entities.VerificationSessionResponse; + +public class RegistrationTest { + + @Test + public void testRegistration() throws Exception { + final UpdateVerificationSessionRequest originalRequest = new UpdateVerificationSessionRequest( + "test", UpdateVerificationSessionRequest.PushTokenType.FCM, null, null, null, null); + final CreateVerificationSessionRequest input = new CreateVerificationSessionRequest("+19995550102", originalRequest); + + final VerificationSessionResponse verificationSessionResponse = Operations + .apiPost("/v1/verification/session", input) + .executeExpectSuccess(VerificationSessionResponse.class); + + final String sessionId = verificationSessionResponse.id(); + final String pushChallenge = Operations.peekVerificationSessionPushChallenge(sessionId); + + // supply push challenge + final UpdateVerificationSessionRequest updatedRequest = new UpdateVerificationSessionRequest( + "test", UpdateVerificationSessionRequest.PushTokenType.FCM, pushChallenge, null, null, null); + final VerificationSessionResponse pushChallengeSupplied = Operations + .apiPatch("/v1/verification/session/%s".formatted(sessionId), updatedRequest) + .executeExpectSuccess(VerificationSessionResponse.class); + + Assertions.assertTrue(pushChallengeSupplied.allowedToRequestCode()); + + // request code + final VerificationCodeRequest verificationCodeRequest = new VerificationCodeRequest( + VerificationCodeRequest.Transport.SMS, "android-ng"); + + final VerificationSessionResponse codeRequested = Operations + .apiPost("/v1/verification/session/%s/code".formatted(sessionId), verificationCodeRequest) + .executeExpectSuccess(VerificationSessionResponse.class); + + // verify code + final SubmitVerificationCodeRequest submitVerificationCodeRequest = new SubmitVerificationCodeRequest("265402"); + final VerificationSessionResponse codeVerified = Operations + .apiPut("/v1/verification/session/%s/code".formatted(sessionId), submitVerificationCodeRequest) + .executeExpectSuccess(VerificationSessionResponse.class); + } +} diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java index 9d29566d8..9b575f544 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java @@ -35,7 +35,6 @@ import java.util.Optional; import java.util.UUID; import java.util.concurrent.CompletionException; import javax.annotation.Nullable; -import javax.servlet.http.HttpServletRequest; import javax.validation.Valid; import javax.validation.constraints.NotNull; import javax.ws.rs.BadRequestException; @@ -52,7 +51,6 @@ import javax.ws.rs.PathParam; import javax.ws.rs.Produces; import javax.ws.rs.QueryParam; import javax.ws.rs.WebApplicationException; -import javax.ws.rs.core.Context; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; import javax.ws.rs.core.Response.Status; @@ -91,7 +89,6 @@ import org.whispersystems.textsecuregcm.entities.StaleDevices; import org.whispersystems.textsecuregcm.entities.UsernameHashResponse; import org.whispersystems.textsecuregcm.entities.UsernameLinkHandle; import org.whispersystems.textsecuregcm.limits.RateLimitedByIp; -import org.whispersystems.textsecuregcm.limits.RateLimiter; import org.whispersystems.textsecuregcm.limits.RateLimiters; import org.whispersystems.textsecuregcm.metrics.UserAgentTagUtil; import org.whispersystems.textsecuregcm.push.PushNotification; @@ -704,7 +701,15 @@ public class AccountController { @DELETE @Path("/username_hash") @Produces(MediaType.APPLICATION_JSON) - public void deleteUsernameHash(final @Auth AuthenticatedAccount auth) { + @Operation( + summary = "Delete username hash", + description = """ + Authenticated endpoint. Deletes previously stored username for the account. + """ + ) + @ApiResponse(responseCode = "204", description = "Username successfully deleted.", useReturnTypeSchema = true) + @ApiResponse(responseCode = "401", description = "Account authentication check failed.") + public void deleteUsernameHash(@Auth final AuthenticatedAccount auth) { clearUsernameLink(auth.getAccount()); accounts.clearUsernameHash(auth.getAccount()); } @@ -714,13 +719,25 @@ public class AccountController { @Path("/username_hash/reserve") @Produces(MediaType.APPLICATION_JSON) @Consumes(MediaType.APPLICATION_JSON) - public ReserveUsernameHashResponse reserveUsernameHash(@Auth AuthenticatedAccount auth, - @HeaderParam(HeaderUtils.X_SIGNAL_AGENT) String userAgent, - @NotNull @Valid ReserveUsernameHashRequest usernameRequest) throws RateLimitExceededException { + @Operation( + summary = "Reserve username hash", + description = """ + Authenticated endpoint. Takes in a list of hashes of potential username hashes, finds one that is not taken, + and reserves it for the current account. + """ + ) + @ApiResponse(responseCode = "200", description = "Username hash reserved successfully.", useReturnTypeSchema = true) + @ApiResponse(responseCode = "401", description = "Account authentication check failed.") + @ApiResponse(responseCode = "409", description = "All username hashes from the list are taken.") + @ApiResponse(responseCode = "422", description = "Invalid request format.") + @ApiResponse(responseCode = "429", description = "Ratelimited.") + public ReserveUsernameHashResponse reserveUsernameHash( + @Auth final AuthenticatedAccount auth, + @NotNull @Valid final ReserveUsernameHashRequest usernameRequest) throws RateLimitExceededException { rateLimiters.getUsernameReserveLimiter().validate(auth.getAccount().getUuid()); - for (byte[] hash : usernameRequest.usernameHashes()) { + for (final byte[] hash : usernameRequest.usernameHashes()) { if (hash.length != USERNAME_HASH_LENGTH) { throw new WebApplicationException(Response.status(422).build()); } @@ -742,9 +759,21 @@ public class AccountController { @Path("/username_hash/confirm") @Produces(MediaType.APPLICATION_JSON) @Consumes(MediaType.APPLICATION_JSON) + @Operation( + summary = "Confirm username hash", + description = """ + Authenticated endpoint. For a previously reserved username hash, confirm that this username hash is now taken + by this account. + """ + ) + @ApiResponse(responseCode = "200", description = "Username hash confirmed successfully.", useReturnTypeSchema = true) + @ApiResponse(responseCode = "401", description = "Account authentication check failed.") + @ApiResponse(responseCode = "409", description = "Given username hash doesn't match the reserved one or no reservation found.") + @ApiResponse(responseCode = "410", description = "Username hash not available (username can't be used).") + @ApiResponse(responseCode = "422", description = "Invalid request format.") + @ApiResponse(responseCode = "429", description = "Ratelimited.") public UsernameHashResponse confirmUsernameHash( @Auth final AuthenticatedAccount auth, - @HeaderParam(HeaderUtils.X_SIGNAL_AGENT) final String userAgent, @NotNull @Valid final ConfirmUsernameHashRequest confirmRequest) throws RateLimitExceededException { rateLimiters.getUsernameSetLimiter().validate(auth.getAccount().getUuid()); @@ -777,19 +806,20 @@ public class AccountController { @Path("/username_hash/{usernameHash}") @Produces(MediaType.APPLICATION_JSON) @RateLimitedByIp(RateLimiters.For.USERNAME_LOOKUP) + @Operation( + summary = "Lookup username hash", + description = """ + Forced unauthenticated endpoint. For the given username hash, look up a user ID. + """ + ) + @ApiResponse(responseCode = "200", description = "Account found for the given username.", useReturnTypeSchema = true) + @ApiResponse(responseCode = "400", description = "Request must not be authenticated.") + @ApiResponse(responseCode = "404", description = "Account not fount for the given username.") public AccountIdentifierResponse lookupUsernameHash( - @HeaderParam(HeaderUtils.X_SIGNAL_AGENT) final String userAgent, - @HeaderParam(HttpHeaders.X_FORWARDED_FOR) final String forwardedFor, - @PathParam("usernameHash") final String usernameHash, - @Context final HttpServletRequest request) throws RateLimitExceededException { - - // Disallow clients from making authenticated requests to this endpoint - if (StringUtils.isNotBlank(request.getHeader("Authorization"))) { - throw new BadRequestException(); - } - - rateLimitByClientIp(rateLimiters.getUsernameLookupLimiter(), forwardedFor); + @Auth final Optional maybeAuthenticatedAccount, + @PathParam("usernameHash") final String usernameHash) throws RateLimitExceededException { + requireNotAuthenticated(maybeAuthenticatedAccount); final byte[] hash; try { hash = Base64.getUrlDecoder().decode(usernameHash); @@ -879,13 +909,11 @@ public class AccountController { @ApiResponse(responseCode = "422", description = "Invalid request format.") @ApiResponse(responseCode = "429", description = "Ratelimited.") public EncryptedUsername lookupUsernameLink( - @Auth Optional authenticatedAccount, + @Auth final Optional maybeAuthenticatedAccount, @PathParam("uuid") final UUID usernameLinkHandle) { final Optional maybeEncryptedUsername = accounts.getByUsernameLinkHandle(usernameLinkHandle) .flatMap(Account::getEncryptedUsername); - if (authenticatedAccount.isPresent()) { - throw new ForbiddenException("must not use authenticated connection for connection graph revealing operations"); - } + requireNotAuthenticated(maybeAuthenticatedAccount); if (maybeEncryptedUsername.isEmpty()) { throw new WebApplicationException(Status.NOT_FOUND); } @@ -896,13 +924,11 @@ public class AccountController { @Path("/account/{uuid}") @RateLimitedByIp(RateLimiters.For.CHECK_ACCOUNT_EXISTENCE) public Response accountExists( - @PathParam("uuid") final UUID uuid, - @Context HttpServletRequest request) throws RateLimitExceededException { + @Auth final Optional authenticatedAccount, + @PathParam("uuid") final UUID uuid) throws RateLimitExceededException { // Disallow clients from making authenticated requests to this endpoint - if (StringUtils.isNotBlank(request.getHeader("Authorization"))) { - throw new BadRequestException(); - } + requireNotAuthenticated(authenticatedAccount); final Status status = accounts.getByAccountIdentifier(uuid) .or(() -> accounts.getByPhoneNumberIdentifier(uuid)) @@ -911,19 +937,6 @@ public class AccountController { return Response.status(status).build(); } - private void rateLimitByClientIp(final RateLimiter rateLimiter, final String forwardedFor) throws RateLimitExceededException { - final String mostRecentProxy = HeaderUtils.getMostRecentProxy(forwardedFor) - .orElseThrow(() -> { - // Missing/malformed Forwarded-For, so we cannot check for a rate-limit. - // This shouldn't happen, so conservatively assume we're over the rate-limit - // and indicate that the client should retry - logger.error("Missing/bad Forwarded-For: {}", forwardedFor); - return new RateLimitExceededException(Duration.ofHours(1), true); - }); - - rateLimiter.validate(mostRecentProxy); - } - @VisibleForTesting static boolean pushChallengeMatches( final String number, @@ -1038,4 +1051,10 @@ public class AccountController { throw rateLimitExceededException; } } + + private void requireNotAuthenticated(final Optional authenticatedAccount) { + if (authenticatedAccount.isPresent()) { + throw new BadRequestException("Operation requires unauthenticated access"); + } + } } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerTest.java index 15f354b31..c948e34aa 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerTest.java @@ -1988,7 +1988,7 @@ class AccountControllerTest { static Stream testLookupUsernameLink() { return Stream.of( - Arguments.of(false, true, true, true, 403), + Arguments.of(false, true, true, true, 400), Arguments.of(true, false, true, true, 429), Arguments.of(true, true, false, true, 404), Arguments.of(true, true, true, false, 404),