Changeset 18786 in josm

Timestamp:

2023-07-27T15:47:48+02:00 (19 months ago)

Author:

taylor.smock

Message:

Fix #23083: SocketTimeoutException in OAuth20Authorization$OAuth20AuthorizationHandler.handleRequest

Location:

trunk

Files:

• src/org/openstreetmap/josm/data/oauth/OAuth20Authorization.java (modified) (1 diff)
• src/org/openstreetmap/josm/gui/preferences/server/OAuthAuthenticationPreferencesPanel.java (modified) (2 diffs)
• test/unit/org/openstreetmap/josm/data/oauth/OAuth20AuthorizationTest.java (modified) (10 diffs)

trunk/src/org/openstreetmap/josm/data/oauth/OAuth20Authorization.java

@@ -102 +102 @@
                 } catch (IOException | OAuth20Exception e) {
                     consumer.accept(Optional.empty());
-                    throw new JosmRuntimeException(e);
+                    throw new RequestHandler.RequestHandlerErrorException(e);
                 } finally {
                     tradeCodeForToken.disconnect();
                 }
             } catch (MalformedURLException e) {
-                throw new JosmRuntimeException(e);
+                consumer.accept(Optional.empty());
+                throw new RequestHandler.RequestHandlerBadRequestException(e);
             }
             return null;

trunk/src/org/openstreetmap/josm/gui/preferences/server/OAuthAuthenticationPreferencesPanel.java

@@ -21 +21 @@
 import javax.swing.JCheckBox;
 import javax.swing.JLabel;
+import javax.swing.JOptionPane;
 import javax.swing.JPanel;
 
@@ -375 +376 @@
                     OAuthAccessTokenHolder.getInstance().save(CredentialsManager.getInstance());
                     GuiHelper.runInEDT(OAuthAuthenticationPreferencesPanel.this::refreshView);
+                    if (!token.isPresent()) {
+                        GuiHelper.runInEDT(() -> JOptionPane.showMessageDialog(MainApplication.getMainPanel(),
+                                tr("Authentication failed, please check browser for details."),
+                                tr("OAuth Authentication Failed"),
+                                JOptionPane.ERROR_MESSAGE));
+                    }
                 }, OsmScopes.read_gpx, OsmScopes.write_gpx,
                         OsmScopes.read_prefs, OsmScopes.write_prefs,

trunk/test/unit/org/openstreetmap/josm/data/oauth/OAuth20AuthorizationTest.java

@@ -2 +2 @@
 package org.openstreetmap.josm.data.oauth;
 
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
@@ -8 +9 @@
 
 import java.io.IOException;
+import java.net.URL;
 import java.util.HashMap;
 import java.util.Map;
@@ -20 +22 @@
 import com.github.tomakehurst.wiremock.extension.Parameters;
 import com.github.tomakehurst.wiremock.extension.ResponseTransformer;
+import com.github.tomakehurst.wiremock.http.FixedDelayDistribution;
 import com.github.tomakehurst.wiremock.http.HttpHeader;
 import com.github.tomakehurst.wiremock.http.HttpHeaders;
@@ -62 +65 @@
     private static final String CODE_CHALLENGE = "code_challenge";
 
+    private enum ConnectionProblems {
+        NONE,
+        SOCKET_TIMEOUT
+    }
+
     private static class OAuthServerWireMock extends ResponseTransformer {
         String stateToReturn;
+        ConnectionProblems connectionProblems = ConnectionProblems.NONE;
         @Override
         public Response transform(Request request, Response response, FileSource files, Parameters parameters) {
@@ -89 +98 @@
                 return Response.Builder.like(response).but().status(500).build();
             }
-            return Response.Builder.like(response).but().body("{\"token_type\": \"bearer\", \"access_token\": \"test_access_token\"}").build();
+            switch (connectionProblems) {
+                case SOCKET_TIMEOUT:
+                    return Response.Builder.like(response).but().configureDelay(null, null,
+                                    10_000, new FixedDelayDistribution(0)).build();
+                case NONE:
+                default:
+                    return Response.Builder.like(response).but()
+                            .body("{\"token_type\": \"bearer\", \"access_token\": \"test_access_token\"}").build();
+            }
         }
 
@@ -136 +153 @@
         RemoteControl.stop(); // Ensure remote control is stopped
         oauthServer.stateToReturn = null;
+        oauthServer.connectionProblems = ConnectionProblems.NONE;
     }
 
@@ -164 +182 @@
     }
 
-    @Test
-    void testAuthorize(WireMockRuntimeInfo wireMockRuntimeInfo) throws IOException {
+    private HttpClient generateClient(WireMockRuntimeInfo wireMockRuntimeInfo, AtomicReference<Optional<IOAuthToken>> consumer) {
         final OAuth20Authorization authorization = new OAuth20Authorization();
-        final AtomicReference<Optional<IOAuthToken>> consumer = new AtomicReference<>();
         OAuth20Parameters parameters = (OAuth20Parameters) OAuthParameters.createDefault(OsmApi.getOsmApi().getBaseUrl(), OAuthVersion.OAuth20);
         RemoteControl.start();
@@ -174 +190 @@
                 parameters.getRedirectUri()), consumer::set, OsmScopes.read_gpx);
         assertEquals(1, OpenBrowserMocker.getCalledURIs().size());
-        HttpClient client = HttpClient.create(OpenBrowserMocker.getCalledURIs().get(0).toURL());
+        final URL url = assertDoesNotThrow(() -> OpenBrowserMocker.getCalledURIs().get(0).toURL());
+        return HttpClient.create(url);
+    }
+
+    @Test
+    void testAuthorize(WireMockRuntimeInfo wireMockRuntimeInfo) throws IOException {
+        final AtomicReference<Optional<IOAuthToken>> consumer = new AtomicReference<>();
+        final HttpClient client = generateClient(wireMockRuntimeInfo, consumer);
         try {
             HttpClient.Response response = client.connect();
@@ -191 +214 @@
     void testAuthorizeBadState(WireMockRuntimeInfo wireMockRuntimeInfo) throws IOException {
         oauthServer.stateToReturn = "Bad_State";
-        final OAuth20Authorization authorization = new OAuth20Authorization();
         final AtomicReference<Optional<IOAuthToken>> consumer = new AtomicReference<>();
-        OAuth20Parameters parameters = (OAuth20Parameters) OAuthParameters.createDefault(OsmApi.getOsmApi().getBaseUrl(), OAuthVersion.OAuth20);
-        RemoteControl.start();
-        authorization.authorize(new OAuth20Parameters(parameters.getClientId(), parameters.getClientSecret(),
-                wireMockRuntimeInfo.getHttpBaseUrl() + "/oauth2", wireMockRuntimeInfo.getHttpBaseUrl() + "/api",
-                parameters.getRedirectUri()), consumer::set, OsmScopes.read_gpx);
-        assertEquals(1, OpenBrowserMocker.getCalledURIs().size());
-        HttpClient client = HttpClient.create(OpenBrowserMocker.getCalledURIs().get(0).toURL());
+        final HttpClient client = generateClient(wireMockRuntimeInfo, consumer);
         try {
             HttpClient.Response response = client.connect();
@@ -210 +226 @@
         assertNull(consumer.get(), "The OAuth consumer should not be called since the state does not match");
     }
+
+    @Test
+    void testSocketTimeout(WireMockRuntimeInfo wireMockRuntimeInfo) throws Exception {
+        // 1s before timeout
+        Config.getPref().putInt("socket.timeout.connect", 1);
+        Config.getPref().putInt("socket.timeout.read", 1);
+        oauthServer.connectionProblems = ConnectionProblems.SOCKET_TIMEOUT;
+
+        final AtomicReference<Optional<IOAuthToken>> consumer = new AtomicReference<>();
+        final HttpClient client = generateClient(wireMockRuntimeInfo, consumer)
+                .setConnectTimeout(15_000).setReadTimeout(30_000);
+        try {
+            HttpClient.Response response = client.connect();
+            assertEquals(500, response.getResponseCode());
+            String content = response.fetchContent();
+            assertTrue(content.contains("java.net.SocketTimeoutException: Read timed out"));
+        } finally {
+            client.disconnect();
+        }
+        assertEquals(Optional.empty(), consumer.get());
+    }
 }