Ticket #24925: oauth-auth-on-upload.patch

src/org/openstreetmap/josm/data/oauth/OAuthParameters.java

@@ -262 +262 @@
                 if (remembered != null) {
                     return remembered;
                 }
-                return createDefault(apiUrl, oAuthVersion);
+                // Use the URL as it was given: apiUrl is the host by now, which is never a valid URL, so
+                // createDefault() would silently fall back to the API currently configured in the preferences.
+                return createDefault(originalApiUrl, oAuthVersion);
             default:
                 throw new IllegalArgumentException("Unknown OAuth version: " + oAuthVersion);
         }

src/org/openstreetmap/josm/io/OsmConnection.java

@@ -33 +33 @@
 import org.openstreetmap.josm.tools.HttpClient;
 import org.openstreetmap.josm.tools.JosmRuntimeException;
 import org.openstreetmap.josm.tools.Logging;
+import org.openstreetmap.josm.tools.Utils;
 
 /**
  * Base class that handles common things like authentication for the reader and writer
@@ -146 +147 @@
      * @throws MissingOAuthAccessTokenException if the process cannot be completed successfully
      */
     private void obtainOAuth20Token() throws MissingOAuthAccessTokenException {
+        if (Utils.isEmpty(this.oAuth20Parameters.getClientId())) {
+            // Without a client id the authorization request is rejected by the server with "Missing required
+            // parameter: client_id", so do not send the user to the browser at all. The message of the exception
+            // is for the log only; what the user is shown already points at the preferences, which is where the
+            // client id of an OAuth application registered on this server has to be entered.
+            throw new MissingOAuthAccessTokenException(
+                    "No OAuth client id for " + OsmApi.getOsmApi().getServerUrl() + ", cannot request a token");
+        }
         if (!Boolean.TRUE.equals(GuiHelper.runInEDTAndWaitAndReturn(() ->
                 ConditionalOptionPaneUtil.showConfirmationDialog("oauth.oauth20.obtain.automatically",
                     MainApplication.getMainFrame(),
@@ -200 +209 @@
      */
     protected void addOAuth20AuthorizationHeader(HttpClient connection) throws OsmTransferException {
         if (this.oAuth20Parameters == null) {
-            this.oAuth20Parameters = OAuthParameters.createFromApiUrl(connection.getURL().getHost(), OAuthVersion.OAuth20);
+            // The parameters are stored under the API URL, so look them up with the API URL and not with the
+            // host of the request: OAuthParameters.createFromApiUrl() reduces it to the host itself where that
+            // is what it needs.
+            this.oAuth20Parameters = OAuthParameters.createFromApiUrl(OsmApi.getOsmApi().getServerUrl(), OAuthVersion.OAuth20);
         }
         OAuthAccessTokenHolder holder = OAuthAccessTokenHolder.getInstance();
         IOAuthToken token = holder.getAccessToken(connection.getURL().toExternalForm(), OAuthVersion.OAuth20);

test/unit/org/openstreetmap/josm/data/oauth/OAuthParametersTest.java

@@ -5 +5 @@
 import static org.junit.jupiter.api.Assertions.assertNotEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 
+import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.Test;
 import org.openstreetmap.josm.TestUtils;
+import org.openstreetmap.josm.io.NetworkManager;
+import org.openstreetmap.josm.io.OnlineResource;
 import org.openstreetmap.josm.spi.preferences.Config;
+import org.openstreetmap.josm.testutils.annotations.BasicPreferences;
 import org.openstreetmap.josm.tools.Logging;
 
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
@@ -16 +20 @@
 /**
  * Unit tests for class {@link OAuthParameters}.
  */
+@BasicPreferences
 class OAuthParametersTest {
+
+    @AfterEach
+    void tearDown() {
+        NetworkManager.setOnline(OnlineResource.ALL);
+        // the API URL is static state shared with the other tests of this class
+        Config.getPref().put("osm-server.url", null);
+    }
+
+    /**
+     * {@link OAuthParameters#createFromApiUrl} answers for the server it is asked about, whichever server is
+     * configured in the preferences, and finds the parameters the user entered in the advanced OAuth settings.
+     * <p>
+     * Non-regression test for #24925: the URL was reduced to the host before the fallback to
+     * {@link OAuthParameters#createDefault}, and a host is never a valid URL, so the defaults of the configured
+     * API were returned instead of those of the server which was asked about.
+     */
+    @Test
+    void testCreateFromApiUrlUsesTheRequestedServer() {
+        // the RFC 8414 lookup must not make a network request
+        NetworkManager.setOffline(OnlineResource.ALL);
+        final String thirdParty = "https://nonprod-mapops.example.org/api";
+        Config.getPref().put("osm-server.url", thirdParty);
+
+        // a server JOSM has a client id for is answered with that client id, not with the configured server's
+        assertEquals("Hl5yIhFS-Egj6aY7A35ouLOuZl0EHjj8JJQQ46IO96E",
+                OAuthParameters.createFromApiUrl("https://api.openhistoricalmap.org/api", OAuthVersion.OAuth20).getClientId());
+        // a server JOSM knows nothing about has no client id until the user enters one
+        assertEquals("", OAuthParameters.createFromApiUrl(thirdParty, OAuthVersion.OAuth20).getClientId());
+
+        // once the parameters are remembered - as the authentication preferences do, under the API URL - they
+        // are found both by the API URL and by the host, which is what a connection has at hand
+        new OAuth20Parameters("entered-client-id", null, thirdParty, thirdParty, "http://127.0.0.1:8111/oauth_authorization")
+                .rememberPreferences();
+        assertEquals("entered-client-id",
+                OAuthParameters.createFromApiUrl(thirdParty, OAuthVersion.OAuth20).getClientId());
+        assertEquals("https://api.openhistoricalmap.org/api",
+                OAuthParameters.createFromApiUrl("https://api.openhistoricalmap.org/api", OAuthVersion.OAuth20).getApiUrl());
+    }
+
     /**
      * Unit test of method {@link OAuthParameters#createDefault}.
      */

new file test/unit/org/openstreetmap/josm/io/OsmConnectionTest.java

@@ +1 @@
+// License: GPL. For details, see LICENSE file.
+package org.openstreetmap.josm.io;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.net.MalformedURLException;
+import java.net.URL;
+
+import javax.swing.JOptionPane;
+
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.openstreetmap.josm.data.oauth.IOAuthParameters;
+import org.openstreetmap.josm.data.oauth.OAuth20Exception;
+import org.openstreetmap.josm.data.oauth.OAuth20Parameters;
+import org.openstreetmap.josm.data.oauth.OAuth20Token;
+import org.openstreetmap.josm.data.oauth.OAuthAccessTokenHolder;
+import org.openstreetmap.josm.data.oauth.OAuthVersion;
+import org.openstreetmap.josm.spi.preferences.Config;
+import org.openstreetmap.josm.testutils.annotations.BasicPreferences;
+import org.openstreetmap.josm.tools.HttpClient;
+
+/**
+ * Unit tests of {@link OsmConnection}.
+ */
+@BasicPreferences
+class OsmConnectionTest {
+
+    private static final String API_URL = "https://nonprod-mapops.example.org/api";
+    private static final String CLIENT_ID = "a-client-id-of-an-application-registered-on-that-server";
+    private static final String REDIRECT_URI = "http://127.0.0.1:8111/oauth_authorization";
+
+    /** Gives the test access to the protected members of {@link OsmConnection} */
+    private static final class TestConnection extends OsmConnection {
+        IOAuthParameters parameters() {
+            return this.oAuth20Parameters;
+        }
+    }
+
+    @BeforeEach
+    void setUp() {
+        // the RFC 8414 lookup of OAuthParameters must not make a network request
+        NetworkManager.setOffline(OnlineResource.ALL);
+        Config.getPref().put("osm-server.url", API_URL);
+    }
+
+    @AfterEach
+    void tearDown() {
+        NetworkManager.setOnline(OnlineResource.ALL);
+        OAuthAccessTokenHolder.getInstance().clear();
+        // preferences and the token holder are static state shared with the other tests of this class
+        Config.getPref().put("oauth.access-token.parameters." + OAuthVersion.OAuth20 + "." + API_URL, null);
+        Config.getPref().put("osm-server.url", null);
+    }
+
+    private static HttpClient apiRequest() throws MalformedURLException {
+        return HttpClient.create(new URL(API_URL + "/0.6/changeset/create"), "PUT");
+    }
+
+    /**
+     * Stores the advanced OAuth parameters the way the authentication preferences do, that is under the API URL.
+     */
+    private static OAuth20Parameters rememberParameters() {
+        OAuth20Parameters parameters = new OAuth20Parameters(CLIENT_ID, null, API_URL, API_URL, REDIRECT_URI);
+        parameters.rememberPreferences();
+        return parameters;
+    }
+
+    /**
+     * The OAuth parameters of a connection are the ones the user entered for the configured API.
+     * <p>
+     * Non-regression test for #24925: they were looked up with the host of the request although they are stored
+     * under the API URL, so the lookup missed and the client id was empty - which made the authorization request
+     * on upload fail with "Missing required parameter: client_id".
+     * @throws Exception if the connection cannot be signed
+     */
+    @Test
+    void testOAuthParametersComeFromTheApiUrl() throws Exception {
+        OAuth20Parameters parameters = rememberParameters();
+        // a token is needed, otherwise the connection would try to obtain one
+        OAuthAccessTokenHolder.getInstance().setAccessToken(API_URL, new OAuth20Token(parameters,
+                "{\"token_type\":\"bearer\", \"access_token\":\"a-token\"}"));
+
+        TestConnection connection = new TestConnection();
+        assertNull(connection.parameters());
+        HttpClient client = apiRequest();
+        connection.addOAuth20AuthorizationHeader(client);
+
+        assertEquals(CLIENT_ID, connection.parameters().getClientId());
+        assertEquals(API_URL, connection.parameters().getApiUrl());
+        assertEquals("Bearer a-token", client.getRequestHeader("Authorization"));
+    }
+
+    /**
+     * Without a client id there is nothing to authorize with, so the user is told to configure one instead of
+     * being sent to a browser which the server then rejects with "Missing required parameter: client_id".
+     * <p>
+     * The confirmation dialog is pre-answered with "No" so that the test still terminates if the check is ever
+     * removed; what tells the two apart is that only the check names the server it gave up on.
+     * @throws MalformedURLException never
+     */
+    @Test
+    void testNoClientIdFailsBeforeAskingToOpenABrowser() throws MalformedURLException {
+        Config.getPref().putBoolean("message.oauth.oauth20.obtain.automatically", false);
+        Config.getPref().putInt("message.oauth.oauth20.obtain.automatically.value", JOptionPane.NO_OPTION);
+
+        // no parameters remembered for this API, and it has no client id built into JOSM
+        HttpClient client = apiRequest();
+        MissingOAuthAccessTokenException thrown = assertThrows(MissingOAuthAccessTokenException.class,
+                () -> new TestConnection().addOAuth20AuthorizationHeader(client));
+        assertNotNull(thrown.getMessage(), "gave up only after asking the user, rather than for the missing client id");
+        assertTrue(thrown.getMessage().contains(API_URL), thrown.getMessage());
+        assertNull(client.getRequestHeader("Authorization"));
+    }
+
+    /**
+     * An existing token is used without the parameters having to be stored, since the token carries its own.
+     * @throws OAuth20Exception if the token cannot be built
+     * @throws Exception if the connection cannot be signed
+     */
+    @Test
+    void testExistingTokenSignsTheConnection() throws Exception {
+        OAuth20Parameters parameters = new OAuth20Parameters(CLIENT_ID, null, API_URL, API_URL, REDIRECT_URI);
+        OAuthAccessTokenHolder.getInstance().setAccessToken(API_URL, new OAuth20Token(parameters,
+                "{\"token_type\":\"bearer\", \"access_token\":\"another-token\"}"));
+
+        HttpClient client = apiRequest();
+        new TestConnection().addOAuth20AuthorizationHeader(client);
+        assertEquals("Bearer another-token", client.getRequestHeader("Authorization"));
+    }
+}