Remove authentication via query parameters for websocket upgrade requ…

…ests
signalapp · Jan 15, 2025 · 3ceaa8b · 3ceaa8b
1 parent 790b9bb
commit 3ceaa8b
Show file tree

Hide file tree

Showing 2 changed files with 7 additions and 50 deletions.
diff --git a/...c/main/java/org/whispersystems/textsecuregcm/websocket/WebSocketAccountAuthenticator.java b/...c/main/java/org/whispersystems/textsecuregcm/websocket/WebSocketAccountAuthenticator.java
@@ -8,9 +8,6 @@
 import static org.whispersystems.textsecuregcm.util.HeaderUtils.basicCredentialsFromAuthHeader;
 
 import com.google.common.net.HttpHeaders;
-import io.dropwizard.auth.basic.BasicCredentials;
-import java.util.List;
-import java.util.Map;
 import javax.annotation.Nullable;
 import org.eclipse.jetty.websocket.api.UpgradeRequest;
 import org.whispersystems.textsecuregcm.auth.AccountAuthenticator;
@@ -26,7 +23,6 @@ public class WebSocketAccountAuthenticator implements WebSocketAuthenticator<Aut
   private static final ReusableAuth<AuthenticatedDevice> CREDENTIALS_NOT_PRESENTED = ReusableAuth.anonymous();
 
   private static final ReusableAuth<AuthenticatedDevice> INVALID_CREDENTIALS_PRESENTED = ReusableAuth.invalid();
-
   private final AccountAuthenticator accountAuthenticator;
   private final PrincipalSupplier<AuthenticatedDevice> principalSupplier;
 
@@ -40,37 +36,15 @@ public WebSocketAccountAuthenticator(final AccountAuthenticator accountAuthentic
   public ReusableAuth<AuthenticatedDevice> authenticate(final UpgradeRequest request)
       throws AuthenticationException {
     try {
-      // If the `Authorization` header was set for the request it takes priority, and we use the result of the
-      // header-based auth ignoring the result of the query-based auth.
-      final String authHeader = request.getHeader(HttpHeaders.AUTHORIZATION);
-      if (authHeader != null) {
-        return authenticatedAccountFromHeaderAuth(authHeader);
-      }
-      return authenticatedAccountFromQueryParams(request);
+      return authenticatedAccountFromHeaderAuth(request.getHeader(HttpHeaders.AUTHORIZATION));
     } catch (final Exception e) {
       // this will be handled and logged upstream
       // the most likely exception is a transient error connecting to account storage
       throw new AuthenticationException(e);
     }
   }
 
-  private ReusableAuth<AuthenticatedDevice> authenticatedAccountFromQueryParams(final UpgradeRequest request) {
-    final Map<String, List<String>> parameters = request.getParameterMap();
-    final List<String> usernames = parameters.get("login");
-    final List<String> passwords = parameters.get("password");
-    if (usernames == null || usernames.size() == 0 ||
-        passwords == null || passwords.size() == 0) {
-      return CREDENTIALS_NOT_PRESENTED;
-    }
-    final BasicCredentials credentials = new BasicCredentials(usernames.get(0).replace(" ", "+"),
-        passwords.get(0).replace(" ", "+"));
-    return accountAuthenticator.authenticate(credentials)
-        .map(authenticatedAccount -> ReusableAuth.authenticated(authenticatedAccount, this.principalSupplier))
-        .orElse(INVALID_CREDENTIALS_PRESENTED);
-  }
-
-  private ReusableAuth<AuthenticatedDevice> authenticatedAccountFromHeaderAuth(@Nullable final String authHeader)
-      throws AuthenticationException {
+  private ReusableAuth<AuthenticatedDevice> authenticatedAccountFromHeaderAuth(@Nullable final String authHeader) {
     if (authHeader == null) {
       return CREDENTIALS_NOT_PRESENTED;
     }

diff --git a/...st/java/org/whispersystems/textsecuregcm/websocket/WebSocketAccountAuthenticatorTest.java b/...st/java/org/whispersystems/textsecuregcm/websocket/WebSocketAccountAuthenticatorTest.java
@@ -13,8 +13,6 @@
 import com.google.common.net.HttpHeaders;
 import com.google.i18n.phonenumbers.PhoneNumberUtil;
 import io.dropwizard.auth.basic.BasicCredentials;
-import java.util.List;
-import java.util.Map;
 import java.util.Optional;
 import java.util.stream.Stream;
 import javax.annotation.Nullable;
@@ -64,11 +62,9 @@ void setUp() {
   @MethodSource
   void testAuthenticate(
       @Nullable final String authorizationHeaderValue,
-      final Map<String, List<String>> upgradeRequestParameters,
       final boolean expectAccount,
       final boolean expectInvalid) throws Exception {
 
-    when(upgradeRequest.getParameterMap()).thenReturn(upgradeRequestParameters);
     if (authorizationHeaderValue != null) {
       when(upgradeRequest.getHeader(eq(HttpHeaders.AUTHORIZATION))).thenReturn(authorizationHeaderValue);
     }
@@ -84,29 +80,16 @@ void testAuthenticate(
   }
 
   private static Stream<Arguments> testAuthenticate() {
-    final Map<String, List<String>> paramsMapWithValidAuth =
-        Map.of("login", List.of(VALID_USER), "password", List.of(VALID_PASSWORD));
-    final Map<String, List<String>> paramsMapWithInvalidAuth =
-        Map.of("login", List.of(INVALID_USER), "password", List.of(INVALID_PASSWORD));
     final String headerWithValidAuth =
         HeaderUtils.basicAuthHeader(VALID_USER, VALID_PASSWORD);
     final String headerWithInvalidAuth =
         HeaderUtils.basicAuthHeader(INVALID_USER, INVALID_PASSWORD);
     return Stream.of(
-        // if `Authorization` header is present, outcome should not depend on the value of query parameters
-        Arguments.of(headerWithValidAuth, Map.of(), true, false),
-        Arguments.of(headerWithInvalidAuth, Map.of(), false, true),
-        Arguments.of("invalid header value", Map.of(), false, true),
-        Arguments.of(headerWithValidAuth, paramsMapWithValidAuth, true, false),
-        Arguments.of(headerWithInvalidAuth, paramsMapWithValidAuth, false, true),
-        Arguments.of("invalid header value", paramsMapWithValidAuth, false, true),
-        Arguments.of(headerWithValidAuth, paramsMapWithInvalidAuth, true, false),
-        Arguments.of(headerWithInvalidAuth, paramsMapWithInvalidAuth, false, true),
-        Arguments.of("invalid header value", paramsMapWithInvalidAuth, false, true),
-        // if `Authorization` header is not set, outcome should match the query params based auth
-        Arguments.of(null, paramsMapWithValidAuth, true, false),
-        Arguments.of(null, paramsMapWithInvalidAuth, false, true),
-        Arguments.of(null, Map.of(), false, false)
+        Arguments.of(headerWithValidAuth, true, false),
+        Arguments.of(headerWithInvalidAuth, false, true),
+        Arguments.of("invalid header value", false, true),
+        // if `Authorization` header is not set, we expect no account and anonymous credentials
+        Arguments.of(null, false, false)
     );
   }
 }