Closes #820 Improve handling of failed steam redirect validations (#822)

src/main/java/com/faforever/api/error/ErrorCode.java

@@ -117,6 +117,7 @@ public enum ErrorCode {
   LESS_PERMISSIVE_LICENSE(207, "Less permissive license", "New license is less permissive than current license."),
   MALFORMED_URL(208, "Malformed URL", "Provided url ''{0}'' is malformed."),
   NOT_ALLOWED_URL_HOST(209, "URL host not allowed", "Provided URL's host is not allowed. URL: ''{0}'', allowed hosts: ''{1}''."),
+  STEAM_LOGIN_VALIDATION_FAILED(210, "Login via Steam failed", "Invalid OpenID redirect code"),
   ;
 
 

src/main/java/com/faforever/api/user/SteamService.java

@@ -2,9 +2,15 @@
 
 import com.faforever.api.config.FafApiProperties;
 import com.faforever.api.config.FafApiProperties.Steam;
+import com.faforever.api.data.domain.AccountLink;
+import com.faforever.api.data.domain.LinkedServiceType;
+import com.faforever.api.error.ApiException;
+import com.faforever.api.error.ErrorCode;
+
 import lombok.RequiredArgsConstructor;
 import lombok.SneakyThrows;
 import lombok.extern.slf4j.Slf4j;
+import org.apache.commons.lang3.StringUtils;
 import org.json.JSONObject;
 import org.springframework.stereotype.Service;
 import org.springframework.web.client.RestTemplate;
@@ -16,12 +22,14 @@
 import java.net.http.HttpRequest;
 import java.net.http.HttpResponse.BodyHandlers;
 import java.util.Map;
+import java.util.Optional;
 
 @Service
 @Slf4j
 @RequiredArgsConstructor
 public class SteamService {
   private final FafApiProperties properties;
+  private final AccountLinkRepository accountLinkRepository;
 
   String buildLoginUrl(String redirectUrl) {
     log.debug("Building steam login url for redirect url: {}", redirectUrl);
@@ -36,11 +44,13 @@ String buildLoginUrl(String redirectUrl) {
       .toUriString();
   }
 
+  @SneakyThrows
   String parseSteamIdFromLoginRedirect(HttpServletRequest request) {
     log.trace("Parsing steam id from request: {}", request);
-
-    String identityUrl = request.getParameter("openid.identity");
-    return identityUrl.substring(identityUrl.lastIndexOf("/") + 1);
+    return Optional.ofNullable(request.getParameter("openid.identity"))
+        .map(identityUrl -> identityUrl.substring(identityUrl.lastIndexOf("/") + 1))
+        .orElseThrow(() -> {log.warn("Steam redirect could not be validated! The request does not contain 'openid.identity' parameter. Original OpenID response:\n {}", request);
+          return ApiException.of(ErrorCode.STEAM_LOGIN_VALIDATION_FAILED);});
   }
 
   @SneakyThrows
@@ -73,15 +83,33 @@ void validateSteamRedirect(HttpServletRequest request) {
     String recodedUri = builder.toUriString().replace("+", "%2B");
     log.debug("Verification uri: {}", recodedUri);
 
-    // the Spring RestTemplate still struggles with the + character so we use the default Java http client
+    // the Spring RestTemplate still struggles with the + character, so we use the default Java http client
     String result = HttpClient.newHttpClient()
       .send(HttpRequest.newBuilder(new URI(recodedUri)).build(), BodyHandlers.ofString())
       .body();
 
     if (result == null || !result.contains("is_valid:true")) {
-      throw new IllegalArgumentException("Steam redirect could not be validated! Original response:\n" + result);
+      handleInvalidOpenIdRedirect(request, result);
     } else {
       log.debug("Steam response successfully validated.");
     }
   }
+
+  void handleInvalidOpenIdRedirect(final HttpServletRequest request, final String openIdResponseBody) {
+    final String steamId = parseSteamIdFromLoginRedirect(request);
+
+    if (StringUtils.isNotBlank(steamId)) {
+      accountLinkRepository.findOneByServiceIdAndServiceType(steamId, LinkedServiceType.STEAM)
+          .map(AccountLink::getUser)
+          .ifPresentOrElse(u -> log.warn("Steam redirect could not be validated for user with id: ''{}'' and login: ''{}''. Original OpenID response:\n {}",
+              u.getId(), u.getLogin(), openIdResponseBody),
+              () -> log.warn("Steam redirect could not be validated! The steam id ''{}'' does not match any account. Original OpenID response:\n {}",
+            StringUtils.deleteWhitespace(steamId).replace("'", ""), // prevent potential log poisoning attack
+            openIdResponseBody));
+    } else {
+      log.warn("Steam redirect could not be validated! The steamId from the OpenId redirect is blank. Original OpenID response:\n {}", openIdResponseBody);
+    }
+
+    throw ApiException.of(ErrorCode.STEAM_LOGIN_VALIDATION_FAILED);
+  }
 }

src/test/java/com/faforever/api/user/SteamServiceTest.java

@@ -0,0 +1,82 @@
+package com.faforever.api.user;
+
+import static org.junit.jupiter.api.Assertions.*;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.Mockito.when;
+
+import com.faforever.api.data.domain.AccountLink;
+import com.faforever.api.data.domain.LinkedServiceType;
+import com.faforever.api.data.domain.User;
+import com.faforever.api.error.ApiException;
+import com.faforever.api.error.ErrorCode;
+import jakarta.servlet.http.HttpServletRequest;
+import java.util.Optional;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@ExtendWith(MockitoExtension.class)
+class SteamServiceTest {
+
+  private static final String IDENTITY_NAME_PARAM = "openid.identity";
+  private static final String DUMMY_URL = "valid.url.domain/login/123";
+  private static final String DUMMY_RESPONSE = "dummy response";
+  @Mock
+  private AccountLinkRepository accountLinkRepositoryMock;
+  @Mock
+  private HttpServletRequest requestMock;
+  @InjectMocks
+  private SteamService beanUnderTest;
+
+  @Test
+  void testHandleInvalidOpenIdRedirect() {
+    when(requestMock.getParameter(IDENTITY_NAME_PARAM)).thenReturn(DUMMY_URL);
+
+    ApiException thrownException = assertThrows(ApiException.class,
+        () -> beanUnderTest.handleInvalidOpenIdRedirect(requestMock, DUMMY_RESPONSE));
+    assertEquals(ErrorCode.STEAM_LOGIN_VALIDATION_FAILED,
+        thrownException.getErrors()[0].getErrorCode());
+  }
+
+  @Test
+  void testHandleInvalidOpenIdRedirectBlankIdentityParam() {
+    final String blankDummyUrl = "";
+    when(requestMock.getParameter(IDENTITY_NAME_PARAM)).thenReturn(blankDummyUrl);
+
+    ApiException thrownException = assertThrows(ApiException.class,
+        () -> beanUnderTest.handleInvalidOpenIdRedirect(requestMock, DUMMY_RESPONSE));
+    assertEquals(ErrorCode.STEAM_LOGIN_VALIDATION_FAILED,
+        thrownException.getErrors()[0].getErrorCode());
+  }
+
+  @Test
+  void testHandleInvalidOpenIdRedirectNoIdentityInRequest() {
+    ApiException thrownException = assertThrows(ApiException.class,
+        () -> beanUnderTest.handleInvalidOpenIdRedirect(requestMock, DUMMY_RESPONSE));
+    assertEquals(ErrorCode.STEAM_LOGIN_VALIDATION_FAILED,
+        thrownException.getErrors()[0].getErrorCode());
+  }
+
+  @Test
+  void testHandleInvalidOpenIdRedirectLinkedAccountExists() {
+    User userMock = Mockito.mock(User.class);
+    when(userMock.getId()).thenReturn(1);
+    when(userMock.getLogin()).thenReturn("dummyLogin");
+    AccountLink accountLinkMock = Mockito.mock(AccountLink.class);
+    when(accountLinkMock.getUser()).thenReturn(userMock);
+    when(requestMock.getParameter(IDENTITY_NAME_PARAM)).thenReturn(DUMMY_URL);
+    when(accountLinkRepositoryMock.findOneByServiceIdAndServiceType(anyString(),
+        any(LinkedServiceType.class))).thenReturn(
+        Optional.of(accountLinkMock));
+
+    ApiException thrownException = assertThrows(ApiException.class,
+        () -> beanUnderTest.handleInvalidOpenIdRedirect(requestMock, DUMMY_RESPONSE));
+    assertEquals(ErrorCode.STEAM_LOGIN_VALIDATION_FAILED,
+        thrownException.getErrors()[0].getErrorCode());
+  }
+
+}