diff --git src/org/openstreetmap/josm/data/oauth/OAuthParameters.java src/org/openstreetmap/josm/data/oauth/OAuthParameters.java index 73ed6ce10c..f4905c04c1 100644 --- src/org/openstreetmap/josm/data/oauth/OAuthParameters.java +++ src/org/openstreetmap/josm/data/oauth/OAuthParameters.java @@ -147,7 +147,7 @@ public final class OAuthParameters { final String clientSecret; final String redirectUri = "http://127.0.0.1:8111/oauth_authorization"; final String baseUrl; - apiUrl = apiUrl == null ? OsmApi.getOsmApi().getServerUrl() : apiUrl; + apiUrl = OsmApi.normalizeApiUrl(apiUrl == null ? OsmApi.getOsmApi().getServerUrl() : apiUrl); switch (apiUrl) { case OSM_API_DEV: case OSM_API_MASTER: @@ -238,6 +238,7 @@ public final class OAuthParameters { * @since 18650 */ public static IOAuthParameters createFromApiUrl(String apiUrl, OAuthVersion oAuthVersion) { + apiUrl = OsmApi.normalizeApiUrl(apiUrl); final String originalApiUrl = apiUrl; // We actually need the host if (apiUrl.startsWith("https://") || apiUrl.startsWith("http://")) { diff --git src/org/openstreetmap/josm/gui/oauth/TestAccessTokenTask.java src/org/openstreetmap/josm/gui/oauth/TestAccessTokenTask.java index 86fe3d9a31..ba6938d5c4 100644 --- src/org/openstreetmap/josm/gui/oauth/TestAccessTokenTask.java +++ src/org/openstreetmap/josm/gui/oauth/TestAccessTokenTask.java @@ -18,6 +18,7 @@ import org.openstreetmap.josm.data.osm.UserInfo; import org.openstreetmap.josm.gui.HelpAwareOptionPane; import org.openstreetmap.josm.gui.PleaseWaitRunnable; import org.openstreetmap.josm.gui.help.HelpUtil; +import org.openstreetmap.josm.io.OsmApi; import org.openstreetmap.josm.io.OsmApiException; import org.openstreetmap.josm.io.OsmServerUserInfoReader; import org.openstreetmap.josm.io.OsmTransferException; @@ -59,7 +60,7 @@ public class TestAccessTokenTask extends PleaseWaitRunnable { CheckParameterUtil.ensureParameterNotNull(accessToken, "accessToken"); this.tokenOAuth2 = accessToken; this.parent = parent; - this.apiUrl = apiUrl; + this.apiUrl = OsmApi.normalizeApiUrl(apiUrl); } @Override @@ -81,21 +82,10 @@ public class TestAccessTokenTask extends PleaseWaitRunnable { this.tokenOAuth2.sign(con); } - protected String normalizeApiUrl(String url) { - // remove leading and trailing white space - url = url.trim(); - - // remove trailing slashes - while (url.endsWith("/")) { - url = url.substring(0, url.lastIndexOf('/')); - } - return url; - } - protected UserInfo getUserDetails() throws OsmOAuthAuthorizationException, XmlParsingException, OsmTransferException { boolean authenticatorEnabled = true; try { - URL url = new URL(normalizeApiUrl(apiUrl) + "/0.6/user/details"); + URL url = new URL(apiUrl + "/0.6/user/details"); authenticatorEnabled = DefaultAuthenticator.getInstance().isEnabled(); DefaultAuthenticator.getInstance().setEnabled(false); diff --git src/org/openstreetmap/josm/gui/preferences/server/ApiUrlTestTask.java src/org/openstreetmap/josm/gui/preferences/server/ApiUrlTestTask.java index ad84e7e2f8..9681092738 100644 --- src/org/openstreetmap/josm/gui/preferences/server/ApiUrlTestTask.java +++ src/org/openstreetmap/josm/gui/preferences/server/ApiUrlTestTask.java @@ -16,6 +16,7 @@ import org.openstreetmap.josm.gui.HelpAwareOptionPane; import org.openstreetmap.josm.gui.PleaseWaitRunnable; import org.openstreetmap.josm.gui.help.HelpUtil; import org.openstreetmap.josm.io.Capabilities; +import org.openstreetmap.josm.io.OsmApi; import org.openstreetmap.josm.io.OsmTransferException; import org.openstreetmap.josm.tools.CheckParameterUtil; import org.openstreetmap.josm.tools.HttpClient; @@ -48,7 +49,7 @@ public class ApiUrlTestTask extends PleaseWaitRunnable { super(parent, tr("Testing OSM API URL ''{0}''", url), false /* don't ignore exceptions */); CheckParameterUtil.ensureParameterNotNull(url, "url"); this.parent = parent; - this.url = url; + this.url = OsmApi.normalizeApiUrl(url); } protected void alertInvalidUrl(String url) { @@ -66,15 +67,15 @@ public class ApiUrlTestTask extends PleaseWaitRunnable { ); } - protected void alertInvalidCapabilitiesUrl(String url) { + protected void alertInvalidCapabilitiesUrl(String capabilitiesUrl) { HelpAwareOptionPane.showMessageDialogInEDT( parent, tr("" + "Failed to build URL ''{0}'' for validating the OSM API server.
" + "Please check the spelling of ''{1}'' and validate again." +"", - url, - getNormalizedApiUrl() + capabilitiesUrl, + url ), tr("Invalid API URL"), JOptionPane.ERROR_MESSAGE, @@ -90,7 +91,7 @@ public class ApiUrlTestTask extends PleaseWaitRunnable { + "Please check the spelling of ''{1}'' and your Internet connection and validate again." +"", url, - getNormalizedApiUrl() + url ), tr("Connection to API failed"), JOptionPane.ERROR_MESSAGE, @@ -107,7 +108,7 @@ public class ApiUrlTestTask extends PleaseWaitRunnable { + "Please check the spelling of ''{1}'' and validate again." + "", retCode, - getNormalizedApiUrl() + url ), tr("Connection to API failed"), JOptionPane.ERROR_MESSAGE, @@ -123,7 +124,7 @@ public class ApiUrlTestTask extends PleaseWaitRunnable { + "It is likely that ''{0}'' is not an OSM API server.
" + "Please check the spelling of ''{0}'' and validate again." + "", - getNormalizedApiUrl() + url ), tr("Connection to API failed"), JOptionPane.ERROR_MESSAGE, @@ -146,30 +147,17 @@ public class ApiUrlTestTask extends PleaseWaitRunnable { // Do nothing } - /** - * Removes leading and trailing whitespace from the API URL and removes trailing '/'. - * - * @return the normalized API URL - */ - protected String getNormalizedApiUrl() { - String apiUrl = url.trim(); - while (apiUrl.endsWith("/")) { - apiUrl = apiUrl.substring(0, apiUrl.lastIndexOf('/')); - } - return apiUrl; - } - @Override protected void realRun() throws SAXException, IOException, OsmTransferException { try { try { - new URL(getNormalizedApiUrl()); + new URL(url); } catch (MalformedURLException e) { - alertInvalidUrl(getNormalizedApiUrl()); + alertInvalidUrl(url); return; } URL capabilitiesUrl; - String getCapabilitiesUrl = getNormalizedApiUrl() + "/0.6/capabilities"; + String getCapabilitiesUrl = url + "/0.6/capabilities"; try { capabilitiesUrl = new URL(getCapabilitiesUrl); } catch (MalformedURLException e) { diff --git src/org/openstreetmap/josm/gui/preferences/server/OsmApiUrlInputPanel.java src/org/openstreetmap/josm/gui/preferences/server/OsmApiUrlInputPanel.java index ca7f38d466..16a6a66184 100644 --- src/org/openstreetmap/josm/gui/preferences/server/OsmApiUrlInputPanel.java +++ src/org/openstreetmap/josm/gui/preferences/server/OsmApiUrlInputPanel.java @@ -125,7 +125,7 @@ public class OsmApiUrlInputPanel extends JPanel { */ public void saveToPreferences() { String oldUrl = OsmApi.getOsmApi().getServerUrl(); - String hmiUrl = getStrippedApiUrl(); + String hmiUrl = getApiUrl(); if (cbUseDefaultServerUrl.isSelected() || Config.getUrls().getDefaultOsmApiUrl().equals(hmiUrl)) { Config.getPref().put("osm-server.url", null); } else { @@ -146,14 +146,15 @@ public class OsmApiUrlInputPanel extends JPanel { } /** - * Returns the entered API URL, stripped of leading and trailing white characters. - * @return the entered API URL, stripped of leading and trailing white characters. May be an empty string - * if nothing has been entered. In this case, it means the user wants to use {@link IUrls#getDefaultOsmApiUrl}. - * @see Utils#strip(String) - * @since 6602 + * Returns the entered API URL, normalized with {@link OsmApi#normalizeApiUrl(String)}, that is stripped of + * leading and trailing white characters and of trailing '/'. + * @return the entered API URL. May be an empty string if nothing has been entered. In this case, it means + * the user wants to use {@link IUrls#getDefaultOsmApiUrl}. + * @see OsmApi#normalizeApiUrl(String) + * @since xxx (renamed from {@code getStrippedApiUrl}, which existed since 6602) */ - public final String getStrippedApiUrl() { - return Utils.strip(tfOsmServerUrl.getText()); + public final String getApiUrl() { + return OsmApi.normalizeApiUrl(tfOsmServerUrl.getText()); } class ValidateApiUrlAction extends AbstractAction implements DocumentListener { @@ -167,7 +168,7 @@ public class OsmApiUrlInputPanel extends JPanel { @Override public void actionPerformed(ActionEvent arg0) { - final String url = getStrippedApiUrl(); + final String url = getApiUrl(); final ApiUrlTestTask task = new ApiUrlTestTask(OsmApiUrlInputPanel.this, url); MainApplication.worker.submit(task); Runnable r = () -> { @@ -190,7 +191,7 @@ public class OsmApiUrlInputPanel extends JPanel { } protected final void updateEnabledState() { - String url = getStrippedApiUrl(); + String url = getApiUrl(); boolean enabled = !url.isEmpty() && !url.equals(lastTestedUrl); if (enabled) { lblValid.setIcon(null); @@ -275,7 +276,7 @@ public class OsmApiUrlInputPanel extends JPanel { class ApiUrlPropagator extends FocusAdapter implements ActionListener { protected void propagate() { - propagate(getStrippedApiUrl()); + propagate(getApiUrl()); } protected void propagate(String url) { diff --git src/org/openstreetmap/josm/io/OsmApi.java src/org/openstreetmap/josm/io/OsmApi.java index 8895b26e7b..69643ebb18 100644 --- src/org/openstreetmap/josm/io/OsmApi.java +++ src/org/openstreetmap/josm/io/OsmApi.java @@ -133,14 +133,36 @@ public class OsmApi extends OsmConnection { * */ public static OsmApi getOsmApi(String serverUrl) { - OsmApi api = instances.get(serverUrl); + String url = normalizeApiUrl(serverUrl); + OsmApi api = instances.get(url); if (api == null) { - api = new OsmApi(serverUrl); + api = new OsmApi(url); cacheInstance(api); } return api; } + /** + * Normalizes an OSM API URL: removes leading and trailing white space and any trailing {@code '/'}. + *

+ * The URL may be entered with a trailing slash, but code which compares it to a known API URL or derives + * another URL from it does not expect one: OAuth would not find the client id of the server, and the links + * to the web site of the server (browse, history, user) would be built from the API path. + * @param serverUrl the server URL, may be null + * @return the normalized URL, or null if {@code serverUrl} is null + * @since xxx + */ + public static String normalizeApiUrl(String serverUrl) { + if (serverUrl == null) { + return null; + } + String url = Utils.strip(serverUrl); + while (url.endsWith("/") && !url.endsWith("://")) { + url = url.substring(0, url.length() - 1); + } + return url; + } + protected static void cacheInstance(OsmApi api) { instances.put(api.getServerUrl(), api); } @@ -181,7 +203,7 @@ public class OsmApi extends OsmConnection { */ protected OsmApi(String serverUrl) { CheckParameterUtil.ensureParameterNotNull(serverUrl, "serverUrl"); - this.serverUrl = serverUrl; + this.serverUrl = normalizeApiUrl(serverUrl); } /** diff --git test/unit/org/openstreetmap/josm/data/preferences/JosmUrlsTest.java test/unit/org/openstreetmap/josm/data/preferences/JosmUrlsTest.java index cea11fd377..fce4aaae73 100644 --- test/unit/org/openstreetmap/josm/data/preferences/JosmUrlsTest.java +++ test/unit/org/openstreetmap/josm/data/preferences/JosmUrlsTest.java @@ -19,4 +19,26 @@ class JosmUrlsTest { void testGetBaseUserUrl() { assertEquals("https://api06.dev.openstreetmap.org/user", Config.getUrls().getBaseUserUrl()); } + + /** + * An API URL entered with a trailing slash must give the same web site URLs as one without, so that + * "History (web)" and the other links to the server are built from the web site and not from the API path. + */ + @Test + void testApiUrlWithTrailingSlash() { + Config.getPref().put("osm-server.url", "https://api06.dev.openstreetmap.org/api/"); + assertEquals("https://api06.dev.openstreetmap.org", Config.getUrls().getOSMWebsiteDependingOnSelectedApi()); + assertEquals("https://api06.dev.openstreetmap.org", Config.getUrls().getBaseBrowseUrl()); + assertEquals("https://api06.dev.openstreetmap.org/user", Config.getUrls().getBaseUserUrl()); + } + + /** + * The default API URL is recognized even when it is entered with a trailing slash, so the links point to + * the OSM web site rather than to the API host. + */ + @Test + void testDefaultApiUrlWithTrailingSlash() { + Config.getPref().put("osm-server.url", Config.getUrls().getDefaultOsmApiUrl() + '/'); + assertEquals(Config.getUrls().getOSMWebsite(), Config.getUrls().getOSMWebsiteDependingOnSelectedApi()); + } } diff --git test/unit/org/openstreetmap/josm/io/OsmApiTest.java test/unit/org/openstreetmap/josm/io/OsmApiTest.java index f5d9013f9a..79020c89da 100644 --- test/unit/org/openstreetmap/josm/io/OsmApiTest.java +++ test/unit/org/openstreetmap/josm/io/OsmApiTest.java @@ -2,6 +2,8 @@ package org.openstreetmap.josm.io; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertSame; import java.io.ByteArrayInputStream; import java.nio.charset.StandardCharsets; @@ -10,10 +12,14 @@ import org.junit.jupiter.api.Test; import org.openstreetmap.josm.data.osm.Changeset; import org.openstreetmap.josm.data.osm.User; import org.openstreetmap.josm.gui.progress.NullProgressMonitor; +import org.openstreetmap.josm.spi.preferences.Config; +import org.openstreetmap.josm.testutils.annotations.BasicPreferences; /** * Unit tests of {@link OsmApi} class. */ +@BasicPreferences +@org.openstreetmap.josm.testutils.annotations.OsmApi class OsmApiTest { /** * Non-regression test for Bug #12675. @@ -36,4 +42,42 @@ class OsmApiTest { NullProgressMonitor.INSTANCE).iterator().next(); assertEquals(User.getAnonymous(), cs2.getUser()); } + + /** + * Unit test of {@link OsmApi#normalizeApiUrl}: white space and trailing slashes are removed. + */ + @Test + void testNormalizeApiUrl() { + assertEquals("https://example.com/api", OsmApi.normalizeApiUrl("https://example.com/api")); + assertEquals("https://example.com/api", OsmApi.normalizeApiUrl("https://example.com/api/")); + assertEquals("https://example.com/api", OsmApi.normalizeApiUrl("https://example.com/api///")); + assertEquals("https://example.com/api", OsmApi.normalizeApiUrl(" https://example.com/api/\t")); + assertEquals("https://example.com", OsmApi.normalizeApiUrl("https://example.com/")); + assertEquals("", OsmApi.normalizeApiUrl(" ")); + assertNull(OsmApi.normalizeApiUrl(null)); + // the slashes of the scheme must not be eaten + assertEquals("https://", OsmApi.normalizeApiUrl("https://")); + } + + /** + * A URL which only differs by a trailing slash is the same API, and is stored without the slash. + */ + @Test + void testGetOsmApiIgnoresTrailingSlash() { + OsmApi api = OsmApi.getOsmApi("https://example.com/api/"); + assertEquals("https://example.com/api", api.getServerUrl()); + assertSame(api, OsmApi.getOsmApi("https://example.com/api")); + assertSame(api, OsmApi.getOsmApi(" https://example.com/api// ")); + } + + /** + * An API URL stored with a trailing slash is used without it, so that code comparing it to a known URL or + * deriving another URL from it works. Non-regression test for the OAuth and "History (web)" breakage. + */ + @Test + void testTrailingSlashInPreference() { + Config.getPref().put("osm-server.url", "https://example.com/api/"); + assertEquals("https://example.com/api", OsmApi.getOsmApi().getServerUrl()); + assertEquals("https://example.com/api/", OsmApi.getOsmApi().getBaseUrl()); + } }