From b897d6994d4baf3127e0abdc70d20c0e16e180e8 Mon Sep 17 00:00:00 2001 From: obydog002 <> Date: Thu, 31 Oct 2024 17:59:57 +0800 Subject: [PATCH 1/6] extract error information and fail early if it exists --- .../client/login/OAuthValuesReceiver.java | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/main/java/com/faforever/client/login/OAuthValuesReceiver.java b/src/main/java/com/faforever/client/login/OAuthValuesReceiver.java index 97655c7b20..0b228c0a87 100644 --- a/src/main/java/com/faforever/client/login/OAuthValuesReceiver.java +++ b/src/main/java/com/faforever/client/login/OAuthValuesReceiver.java @@ -33,6 +33,8 @@ public class OAuthValuesReceiver { private static final Pattern CODE_PATTERN = Pattern.compile("code=([^ &]+)"); private static final Pattern STATE_PATTERN = Pattern.compile("state=([^ &]+)"); + private static final Pattern ERROR_PATTERN = Pattern.compile("error=([^ &]+)"); + private static final Pattern ERROR_DESCRIPTION_PATTERN = Pattern.compile("error_description=([^ &]+)"); private final PlatformService platformService; private final LoginService loginService; @@ -89,6 +91,7 @@ private Values readValues(String state, String codeVerifier) { // Do not try with resources as the socket needs to stay open. try { + checkForError(request); Values values = readValues(request, redirectUri); success = true; return values; @@ -147,5 +150,17 @@ private String extractValue(String request, Pattern pattern) { return matcher.group(1); } + private void checkForError(String request) { + Matcher matcher = ERROR_PATTERN.matcher(request); + if (matcher.find()) { + Matcher errorDescriptionMatcher = ERROR_DESCRIPTION_PATTERN.matcher(request); + if (errorDescriptionMatcher.find()) { + throw new IllegalStateException("Login failed with error '" + matcher.group(1) + "' and description: " + errorDescriptionMatcher.group(1) + ". The full request is: " + request); + } else { + throw new IllegalStateException("Login failed with error '" + matcher.group(1) + "'. The full request is: " + request); + } + } + } + public record Values(String code, String state, URI redirectUri) {} } From 8a24a89a43f5e345ec732ffd6a1776721577a857 Mon Sep 17 00:00:00 2001 From: obydog002 <> Date: Thu, 14 Nov 2024 22:11:25 +0800 Subject: [PATCH 2/6] parse specific login errors for more informative messages --- .../faforever/client/login/LoginController.java | 17 ++++++++++++++++- src/main/resources/i18n/messages.properties | 2 ++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/faforever/client/login/LoginController.java b/src/main/java/com/faforever/client/login/LoginController.java index 704ec509cc..a71deb73fe 100644 --- a/src/main/java/com/faforever/client/login/LoginController.java +++ b/src/main/java/com/faforever/client/login/LoginController.java @@ -49,6 +49,7 @@ import java.net.URI; import java.util.List; import java.util.concurrent.CompletableFuture; +import java.util.regex.Pattern; @Component @Scope(ConfigurableBeanFactory.SCOPE_PROTOTYPE) @@ -56,6 +57,9 @@ @RequiredArgsConstructor public class LoginController extends NodeController { + private static final Pattern ERROR_SCOPE_DENIED = Pattern.compile("scope_denied"); + private static final Pattern ERROR_NO_CSRF = Pattern.compile("No\\+CSRF\\+value"); + private final OperatingSystem operatingSystem; private final GameRunner gameRunner; private final LoginService loginService; @@ -279,6 +283,17 @@ private Mono loginWithCode(String code, URI redirectUri, String codeVerifi return loginService.login(code, codeVerifier, redirectUri); } + private String getLoginErrorSpecificCause(Throwable throwable) { + String cause = throwable.getMessage(); + if (ERROR_SCOPE_DENIED.matcher(cause).find()) { + return "login.scopeDenied"; + } + if (ERROR_NO_CSRF.matcher(cause).find()) { + return "login.noCSRF"; + } + return "login.failed"; + } + private Void onLoginFailed(Throwable throwable) { if (loginService.getOwnUser() != null && loginService.getOwnPlayer() != null) { log.info("Previous login request failed but user is already logged in", throwable); @@ -294,7 +309,7 @@ private Void onLoginFailed(Throwable throwable) { List.of(new DismissAction(i18n)))); } else { log.error("Could not log in", throwable); - notificationService.addImmediateErrorNotification(throwable, "login.failed"); + notificationService.addImmediateErrorNotification(throwable, getLoginErrorSpecificCause(throwable)); } showLoginForm(); diff --git a/src/main/resources/i18n/messages.properties b/src/main/resources/i18n/messages.properties index c3d1e31c07..a3a1bf824a 100644 --- a/src/main/resources/i18n/messages.properties +++ b/src/main/resources/i18n/messages.properties @@ -1088,6 +1088,8 @@ map.all = All Versions map.current = Current Version login.remember = Remember Me login.failed = Error occurred during login +login.scopeDenied = Login failed. You did not accept the scopes on the login page. +login.noCSRF = Login failed. Likely your login timed out, please try again. login.badState = State returned by user service does not match initial state if this continues to occur please reach out to technical help on the forum or discord channel login.oauthBaseUrl = OAuth base URL session.expired.title = Session Expired From 304e9a08a10dd83801f30eee83f162a5197f26ec Mon Sep 17 00:00:00 2001 From: obydog002 <> Date: Thu, 14 Nov 2024 22:56:39 +0800 Subject: [PATCH 3/6] fix and add tests --- .../client/login/LoginController.java | 3 +++ .../client/login/LoginControllerTest.java | 26 +++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/src/main/java/com/faforever/client/login/LoginController.java b/src/main/java/com/faforever/client/login/LoginController.java index a71deb73fe..7fee67ee9c 100644 --- a/src/main/java/com/faforever/client/login/LoginController.java +++ b/src/main/java/com/faforever/client/login/LoginController.java @@ -285,6 +285,9 @@ private Mono loginWithCode(String code, URI redirectUri, String codeVerifi private String getLoginErrorSpecificCause(Throwable throwable) { String cause = throwable.getMessage(); + if (cause == null) { + cause = ""; + } if (ERROR_SCOPE_DENIED.matcher(cause).find()) { return "login.scopeDenied"; } diff --git a/src/test/java/com/faforever/client/login/LoginControllerTest.java b/src/test/java/com/faforever/client/login/LoginControllerTest.java index 029cb6d7f0..b7bbe86922 100644 --- a/src/test/java/com/faforever/client/login/LoginControllerTest.java +++ b/src/test/java/com/faforever/client/login/LoginControllerTest.java @@ -170,6 +170,32 @@ public void testLoginFailsNoPorts() throws Exception { assertTrue(instance.loginFormPane.isVisible()); } + @Test + public void testLoginFailsScopeDenied() throws Exception { + when(oAuthValuesReceiver.receiveValues(anyString(), anyString())) + .thenReturn(CompletableFuture.failedFuture(new IllegalStateException("scope_denied"))); + + instance.onLoginButtonClicked(); + WaitForAsyncUtils.waitForFxEvents(); + + verify(notificationService).addImmediateErrorNotification(any(), eq("login.scopeDenied")); + assertFalse(instance.loginProgressPane.isVisible()); + assertTrue(instance.loginFormPane.isVisible()); + } + + @Test + public void testLoginFailsNoCSRF() throws Exception { + when(oAuthValuesReceiver.receiveValues(anyString(), anyString())) + .thenReturn(CompletableFuture.failedFuture(new IllegalStateException("No+CSRF+value"))); + + instance.onLoginButtonClicked(); + WaitForAsyncUtils.waitForFxEvents(); + + verify(notificationService).addImmediateErrorNotification(any(), eq("login.noCSRF")); + assertFalse(instance.loginProgressPane.isVisible()); + assertTrue(instance.loginFormPane.isVisible()); + } + @Test public void testLoginFailsTimeout() throws Exception { when(oAuthValuesReceiver.receiveValues(anyString(), anyString())) From ab007c82e0c0cf490ba4698bd9d433c476802f99 Mon Sep 17 00:00:00 2001 From: obydog002 <> Date: Thu, 14 Nov 2024 23:17:19 +0800 Subject: [PATCH 4/6] decode request string --- .../java/com/faforever/client/login/LoginController.java | 2 +- .../com/faforever/client/login/OAuthValuesReceiver.java | 7 +++++-- .../com/faforever/client/login/LoginControllerTest.java | 2 +- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/faforever/client/login/LoginController.java b/src/main/java/com/faforever/client/login/LoginController.java index 7fee67ee9c..dfd29721dd 100644 --- a/src/main/java/com/faforever/client/login/LoginController.java +++ b/src/main/java/com/faforever/client/login/LoginController.java @@ -58,7 +58,7 @@ public class LoginController extends NodeController { private static final Pattern ERROR_SCOPE_DENIED = Pattern.compile("scope_denied"); - private static final Pattern ERROR_NO_CSRF = Pattern.compile("No\\+CSRF\\+value"); + private static final Pattern ERROR_NO_CSRF = Pattern.compile("No CSRF value"); private final OperatingSystem operatingSystem; private final GameRunner gameRunner; diff --git a/src/main/java/com/faforever/client/login/OAuthValuesReceiver.java b/src/main/java/com/faforever/client/login/OAuthValuesReceiver.java index 0b228c0a87..12a7e2bc30 100644 --- a/src/main/java/com/faforever/client/login/OAuthValuesReceiver.java +++ b/src/main/java/com/faforever/client/login/OAuthValuesReceiver.java @@ -19,6 +19,7 @@ import java.net.ServerSocket; import java.net.Socket; import java.net.URI; +import java.net.URLDecoder; import java.nio.charset.StandardCharsets; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CountDownLatch; @@ -151,13 +152,15 @@ private String extractValue(String request, Pattern pattern) { } private void checkForError(String request) { + String decodedRequest = URLDecoder.decode(request, StandardCharsets.UTF_8); Matcher matcher = ERROR_PATTERN.matcher(request); if (matcher.find()) { Matcher errorDescriptionMatcher = ERROR_DESCRIPTION_PATTERN.matcher(request); if (errorDescriptionMatcher.find()) { - throw new IllegalStateException("Login failed with error '" + matcher.group(1) + "' and description: " + errorDescriptionMatcher.group(1) + ". The full request is: " + request); + String decodedDescription = URLDecoder.decode(errorDescriptionMatcher.group(1), StandardCharsets.UTF_8); + throw new IllegalStateException("Login failed with error '" + matcher.group(1) + "' and description: " + decodedDescription + ". The full request is: " + decodedRequest); } else { - throw new IllegalStateException("Login failed with error '" + matcher.group(1) + "'. The full request is: " + request); + throw new IllegalStateException("Login failed with error '" + matcher.group(1) + "'. The full request is: " + decodedRequest); } } } diff --git a/src/test/java/com/faforever/client/login/LoginControllerTest.java b/src/test/java/com/faforever/client/login/LoginControllerTest.java index b7bbe86922..170a0571b1 100644 --- a/src/test/java/com/faforever/client/login/LoginControllerTest.java +++ b/src/test/java/com/faforever/client/login/LoginControllerTest.java @@ -186,7 +186,7 @@ public void testLoginFailsScopeDenied() throws Exception { @Test public void testLoginFailsNoCSRF() throws Exception { when(oAuthValuesReceiver.receiveValues(anyString(), anyString())) - .thenReturn(CompletableFuture.failedFuture(new IllegalStateException("No+CSRF+value"))); + .thenReturn(CompletableFuture.failedFuture(new IllegalStateException("No CSRF value"))); instance.onLoginButtonClicked(); WaitForAsyncUtils.waitForFxEvents(); From cac1d4f77bbdea07e5321d9ec53b152406a5aa0d Mon Sep 17 00:00:00 2001 From: obydog002 <> Date: Sun, 24 Nov 2024 12:46:37 +0800 Subject: [PATCH 5/6] clean up formatted request and tests --- .../faforever/client/login/LoginController.java | 3 --- .../client/login/OAuthValuesReceiver.java | 16 ++++++---------- .../client/login/LoginControllerTest.java | 4 ++-- 3 files changed, 8 insertions(+), 15 deletions(-) diff --git a/src/main/java/com/faforever/client/login/LoginController.java b/src/main/java/com/faforever/client/login/LoginController.java index dfd29721dd..e5d2d79c48 100644 --- a/src/main/java/com/faforever/client/login/LoginController.java +++ b/src/main/java/com/faforever/client/login/LoginController.java @@ -285,9 +285,6 @@ private Mono loginWithCode(String code, URI redirectUri, String codeVerifi private String getLoginErrorSpecificCause(Throwable throwable) { String cause = throwable.getMessage(); - if (cause == null) { - cause = ""; - } if (ERROR_SCOPE_DENIED.matcher(cause).find()) { return "login.scopeDenied"; } diff --git a/src/main/java/com/faforever/client/login/OAuthValuesReceiver.java b/src/main/java/com/faforever/client/login/OAuthValuesReceiver.java index 12a7e2bc30..359aafc0b4 100644 --- a/src/main/java/com/faforever/client/login/OAuthValuesReceiver.java +++ b/src/main/java/com/faforever/client/login/OAuthValuesReceiver.java @@ -35,7 +35,6 @@ public class OAuthValuesReceiver { private static final Pattern CODE_PATTERN = Pattern.compile("code=([^ &]+)"); private static final Pattern STATE_PATTERN = Pattern.compile("state=([^ &]+)"); private static final Pattern ERROR_PATTERN = Pattern.compile("error=([^ &]+)"); - private static final Pattern ERROR_DESCRIPTION_PATTERN = Pattern.compile("error_description=([^ &]+)"); private final PlatformService platformService; private final LoginService loginService; @@ -143,25 +142,22 @@ private Values readValues(String request, URI redirectUri) { return new Values(code, state, redirectUri); } + private String formatRequest(String request) { + return URLDecoder.decode(request, StandardCharsets.UTF_8); + } + private String extractValue(String request, Pattern pattern) { Matcher matcher = pattern.matcher(request); if (!matcher.find()) { - throw new IllegalStateException("Could not extract value with pattern '" + pattern + "' from: " + request); + throw new IllegalStateException("Could not extract value with pattern '" + pattern + "' from: " + formatRequest(request)); } return matcher.group(1); } private void checkForError(String request) { - String decodedRequest = URLDecoder.decode(request, StandardCharsets.UTF_8); Matcher matcher = ERROR_PATTERN.matcher(request); if (matcher.find()) { - Matcher errorDescriptionMatcher = ERROR_DESCRIPTION_PATTERN.matcher(request); - if (errorDescriptionMatcher.find()) { - String decodedDescription = URLDecoder.decode(errorDescriptionMatcher.group(1), StandardCharsets.UTF_8); - throw new IllegalStateException("Login failed with error '" + matcher.group(1) + "' and description: " + decodedDescription + ". The full request is: " + decodedRequest); - } else { - throw new IllegalStateException("Login failed with error '" + matcher.group(1) + "'. The full request is: " + decodedRequest); - } + throw new IllegalStateException("Login failed with error '" + matcher.group(1) + "'. The full request is: " + formatRequest(request)); } } diff --git a/src/test/java/com/faforever/client/login/LoginControllerTest.java b/src/test/java/com/faforever/client/login/LoginControllerTest.java index 170a0571b1..1b75c6becf 100644 --- a/src/test/java/com/faforever/client/login/LoginControllerTest.java +++ b/src/test/java/com/faforever/client/login/LoginControllerTest.java @@ -160,7 +160,7 @@ public void testLoginFails() throws Exception { @Test public void testLoginFailsNoPorts() throws Exception { when(oAuthValuesReceiver.receiveValues(anyString(), anyString())) - .thenReturn(CompletableFuture.failedFuture(new IllegalStateException())); + .thenReturn(CompletableFuture.failedFuture(new IllegalStateException(""))); instance.onLoginButtonClicked(); WaitForAsyncUtils.waitForFxEvents(); @@ -262,7 +262,7 @@ public void testLoginRefreshFails() { .thenReturn(CompletableFuture.completedFuture(ClientConfigurationBuilder.create().defaultValues().get())); loginPrefs.setRememberMe(true); loginPrefs.setRefreshToken("abc"); - when(loginService.loginWithRefreshToken()).thenReturn(Mono.error(new Exception())); + when(loginService.loginWithRefreshToken()).thenReturn(Mono.error(new Exception(""))); runOnFxThreadAndWait(() -> reinitialize(instance)); verify(loginService).loginWithRefreshToken(); assertFalse(instance.loginProgressPane.isVisible()); From d7b24b7426d92d46e12841228a2a427876ec09d3 Mon Sep 17 00:00:00 2001 From: obydog002 <> Date: Tue, 3 Dec 2024 17:09:48 +0800 Subject: [PATCH 6/6] throw login specific exceptions --- .../login/KnownLoginErrorException.java | 13 ++++++++++++ .../client/login/LoginController.java | 20 +++---------------- .../client/login/OAuthValuesReceiver.java | 12 ++++++++++- .../client/login/LoginControllerTest.java | 19 +++--------------- 4 files changed, 30 insertions(+), 34 deletions(-) create mode 100644 src/main/java/com/faforever/client/login/KnownLoginErrorException.java diff --git a/src/main/java/com/faforever/client/login/KnownLoginErrorException.java b/src/main/java/com/faforever/client/login/KnownLoginErrorException.java new file mode 100644 index 0000000000..b27944b678 --- /dev/null +++ b/src/main/java/com/faforever/client/login/KnownLoginErrorException.java @@ -0,0 +1,13 @@ +package com.faforever.client.login; + +import lombok.Getter; + +@Getter +public class KnownLoginErrorException extends RuntimeException { + private final String i18nKey; + + public KnownLoginErrorException(String message, String i18nKey) { + super(message); + this.i18nKey = i18nKey; + } +} diff --git a/src/main/java/com/faforever/client/login/LoginController.java b/src/main/java/com/faforever/client/login/LoginController.java index e5d2d79c48..6c916226e4 100644 --- a/src/main/java/com/faforever/client/login/LoginController.java +++ b/src/main/java/com/faforever/client/login/LoginController.java @@ -49,17 +49,12 @@ import java.net.URI; import java.util.List; import java.util.concurrent.CompletableFuture; -import java.util.regex.Pattern; @Component @Scope(ConfigurableBeanFactory.SCOPE_PROTOTYPE) @Slf4j @RequiredArgsConstructor public class LoginController extends NodeController { - - private static final Pattern ERROR_SCOPE_DENIED = Pattern.compile("scope_denied"); - private static final Pattern ERROR_NO_CSRF = Pattern.compile("No CSRF value"); - private final OperatingSystem operatingSystem; private final GameRunner gameRunner; private final LoginService loginService; @@ -283,17 +278,6 @@ private Mono loginWithCode(String code, URI redirectUri, String codeVerifi return loginService.login(code, codeVerifier, redirectUri); } - private String getLoginErrorSpecificCause(Throwable throwable) { - String cause = throwable.getMessage(); - if (ERROR_SCOPE_DENIED.matcher(cause).find()) { - return "login.scopeDenied"; - } - if (ERROR_NO_CSRF.matcher(cause).find()) { - return "login.noCSRF"; - } - return "login.failed"; - } - private Void onLoginFailed(Throwable throwable) { if (loginService.getOwnUser() != null && loginService.getOwnPlayer() != null) { log.info("Previous login request failed but user is already logged in", throwable); @@ -307,9 +291,11 @@ private Void onLoginFailed(Throwable throwable) { notificationService.addNotification( new ServerNotification(i18n.get("login.failed"), loginException.getMessage(), Severity.ERROR, List.of(new DismissAction(i18n)))); + } else if (throwable instanceof KnownLoginErrorException loginException) { + notificationService.addImmediateErrorNotification(throwable, loginException.getI18nKey()); } else { log.error("Could not log in", throwable); - notificationService.addImmediateErrorNotification(throwable, getLoginErrorSpecificCause(throwable)); + notificationService.addImmediateErrorNotification(throwable, "login.failed"); } showLoginForm(); diff --git a/src/main/java/com/faforever/client/login/OAuthValuesReceiver.java b/src/main/java/com/faforever/client/login/OAuthValuesReceiver.java index 359aafc0b4..599bc5e7f0 100644 --- a/src/main/java/com/faforever/client/login/OAuthValuesReceiver.java +++ b/src/main/java/com/faforever/client/login/OAuthValuesReceiver.java @@ -35,6 +35,8 @@ public class OAuthValuesReceiver { private static final Pattern CODE_PATTERN = Pattern.compile("code=([^ &]+)"); private static final Pattern STATE_PATTERN = Pattern.compile("state=([^ &]+)"); private static final Pattern ERROR_PATTERN = Pattern.compile("error=([^ &]+)"); + private static final Pattern ERROR_SCOPE_DENIED = Pattern.compile("scope_denied"); + private static final Pattern ERROR_NO_CSRF = Pattern.compile("No\\+CSRF\\+value"); private final PlatformService platformService; private final LoginService loginService; @@ -157,7 +159,15 @@ private String extractValue(String request, Pattern pattern) { private void checkForError(String request) { Matcher matcher = ERROR_PATTERN.matcher(request); if (matcher.find()) { - throw new IllegalStateException("Login failed with error '" + matcher.group(1) + "'. The full request is: " + formatRequest(request)); + String errorMessage = "Login failed with error '" + matcher.group(1) + "'. The full request is: " + formatRequest(request); + if (ERROR_SCOPE_DENIED.matcher(request).find()) { + throw new KnownLoginErrorException(errorMessage, "login.scopeDenied"); + } + + if (ERROR_NO_CSRF.matcher(request).find()) { + throw new KnownLoginErrorException(errorMessage, "login.noCSRF"); + } + throw new IllegalStateException(errorMessage); } } diff --git a/src/test/java/com/faforever/client/login/LoginControllerTest.java b/src/test/java/com/faforever/client/login/LoginControllerTest.java index 1b75c6becf..ba18fabf19 100644 --- a/src/test/java/com/faforever/client/login/LoginControllerTest.java +++ b/src/test/java/com/faforever/client/login/LoginControllerTest.java @@ -171,27 +171,14 @@ public void testLoginFailsNoPorts() throws Exception { } @Test - public void testLoginFailsScopeDenied() throws Exception { + public void testLoginFailsKnownError() throws Exception { when(oAuthValuesReceiver.receiveValues(anyString(), anyString())) - .thenReturn(CompletableFuture.failedFuture(new IllegalStateException("scope_denied"))); + .thenReturn(CompletableFuture.failedFuture(new KnownLoginErrorException("", "login.known"))); instance.onLoginButtonClicked(); WaitForAsyncUtils.waitForFxEvents(); - verify(notificationService).addImmediateErrorNotification(any(), eq("login.scopeDenied")); - assertFalse(instance.loginProgressPane.isVisible()); - assertTrue(instance.loginFormPane.isVisible()); - } - - @Test - public void testLoginFailsNoCSRF() throws Exception { - when(oAuthValuesReceiver.receiveValues(anyString(), anyString())) - .thenReturn(CompletableFuture.failedFuture(new IllegalStateException("No CSRF value"))); - - instance.onLoginButtonClicked(); - WaitForAsyncUtils.waitForFxEvents(); - - verify(notificationService).addImmediateErrorNotification(any(), eq("login.noCSRF")); + verify(notificationService).addImmediateErrorNotification(any(), eq("login.known")); assertFalse(instance.loginProgressPane.isVisible()); assertTrue(instance.loginFormPane.isVisible()); }