From 8c652b61a172830e755acdaf93a112975c4d842a Mon Sep 17 00:00:00 2001 Date: Sat, 12 Sep 2026 11:05:37 +0100 Subject: [PATCH] OAuth 2: honour user-supplied parameters for servers JOSM does not know For an API URL without a built-in client id, the advanced OAuth panel lets the user enter the client id (and secret) of an OAuth 2 application they registered on that server - but nothing consumed them. Three defects: 1. AuthoriseNowAction.updateEnabledState() asked createFromApiUrl() for a client id, which only knows stored tokens and the built-in list, so 'Authorize now (Fully automatic)' stayed disabled however the panel was filled in - although the wizard is handed exactly the panel's parameters when pressed. The action now uses the panel's client id whenever 'Use default settings' is off, and is re-evaluated as the field or the checkbox changes. 2. AdvancedOAuthPropertiesPanel.rememberPreferences() saved custom parameters under oauth.access-token.parameters.OAuth20., but no code read that key: on the next start initialize() got the empty default back and re-ticked 'Use default settings', losing the client id unless a token had been obtained. createFromApiUrl() now reads the remembered parameters before falling back to the defaults. 3. The token exchange never sent the client secret, so a confidential application - the default when registering one on openstreetmap-website - was refused with a 401 whose empty body then failed as JSON in OAuth20Token. The secret is sent when the panel has one, and a rejected exchange now reports the HTTP status and the server's message. Tested against an openstreetmap-website instance not in the built-in list (lugus.opengeofiction.net) with user-registered applications, confidential with id and secret, and public with id alone: the button enables, the automatic flow completes and uploads work, and the parameters survive a restart. --- .../josm/data/oauth/OAuth20Authorization.java | 13 +++++ .../josm/data/oauth/OAuthParameters.java | 37 ++++++++++++++ .../oauth/AdvancedOAuthPropertiesPanel.java | 48 +++++++++++++++++++ .../OAuthAuthenticationPreferencesPanel.java | 36 ++++++++++---- 4 files changed, 124 insertions(+), 10 deletions(-) diff --git a/src/org/openstreetmap/josm/data/oauth/OAuth20Authorization.java b/src/org/openstreetmap/josm/data/oauth/OAuth20Authorization.java index f5d039f9ad..ac3c3f27c7 100644 --- a/src/org/openstreetmap/josm/data/oauth/OAuth20Authorization.java +++ b/src/org/openstreetmap/josm/data/oauth/OAuth20Authorization.java @@ -27,6 +27,7 @@ import org.openstreetmap.josm.gui.widgets.HtmlPanel; import org.openstreetmap.josm.io.remotecontrol.handler.AuthorizationHandler; import org.openstreetmap.josm.io.remotecontrol.handler.RequestHandler; import org.openstreetmap.josm.tools.HttpClient; +import org.openstreetmap.josm.tools.Utils; import org.openstreetmap.josm.tools.JosmRuntimeException; import org.openstreetmap.josm.tools.OpenBrowser; @@ -125,7 +126,12 @@ public class OAuth20Authorization implements IOAuthAuthorization { String code = args.get("code"); try { HttpClient tradeCodeForToken = HttpClient.create(new URL(parameters.getAccessTokenUrl()), "POST"); + // A confidential client (the default when registering an application on + // openstreetmap-website) must present its secret at the token endpoint; + // the built-in OSM application is a public client and has none. + String clientSecret = parameters.getClientSecret(); tradeCodeForToken.setRequestBody(("grant_type=authorization_code&client_id=" + parameters.getClientId() + + (!Utils.isEmpty(clientSecret) ? "&client_secret=" + Utils.encodeUrl(clientSecret) : "") + "&redirect_uri=" + parameters.getRedirectUri() + "&code=" + code + (this.codeVerifier != null ? "&code_verifier=" + this.codeVerifier : "") @@ -134,6 +140,13 @@ public class OAuth20Authorization implements IOAuthAuthorization { try { tradeCodeForToken.connect(); HttpClient.Response response = tradeCodeForToken.getResponse(); + if (response.getResponseCode() >= 400) { + // Doorkeeper answers a rejected exchange with a JSON error body, but an + // authentication failure can come back with none - say what the server said + String body = response.fetchContent(); + throw new OAuth20Exception(tr("The server refused the token request: HTTP {0} {1}", + response.getResponseCode(), Utils.isEmpty(body) ? response.getResponseMessage() : body)); + } OAuth20Token oAuth20Token = new OAuth20Token(parameters, response.getContentReader()); consumer.accept(Optional.of(oAuth20Token)); } catch (IOException | OAuth20Exception e) { diff --git a/src/org/openstreetmap/josm/data/oauth/OAuthParameters.java b/src/org/openstreetmap/josm/data/oauth/OAuthParameters.java index c829efbcac..dd7109337f 100644 --- a/src/org/openstreetmap/josm/data/oauth/OAuthParameters.java +++ b/src/org/openstreetmap/josm/data/oauth/OAuthParameters.java @@ -78,6 +78,38 @@ public final class OAuthParameters { } } + /** + * Get the OAuth 2.0 parameters the user entered in the advanced OAuth settings and saved with + * {@link IOAuthParameters#rememberPreferences()}. Until a token exists, this is the only place a + * client id for a server not in {@link #createDefault(String, OAuthVersion)} can come from. + * {@link OAuth20Parameters#rememberPreferences()} keys by the API URL as given; callers may pass + * either the URL or its host, so both are tried. + * @param keys The preference key suffixes to try, in order + * @return The remembered parameters, or {@code null} if none are saved or they cannot be parsed + * @since xxx + */ + private static IOAuthParameters createFromPreferences(String... keys) { + for (String key : keys) { + if (Utils.isEmpty(key)) { + continue; + } + final String json = Config.getPref().get("oauth.access-token.parameters." + OAuthVersion.OAuth20 + "." + key, null); + if (Utils.isEmpty(json)) { + continue; + } + try { + OAuth20Parameters parameters = new OAuth20Parameters(json); + if (!Utils.isEmpty(parameters.getClientId())) { + return parameters; + } + } catch (IllegalArgumentException | NullPointerException | JsonParsingException e) { + // A preference written by hand, or by an older JOSM with different fields + Logging.trace(e); + } + } + return null; + } + private static JsonObject getRFC8414Parameters(String apiUrl) { HttpClient client = null; try { @@ -199,6 +231,7 @@ public final class OAuthParameters { * @since 18650 */ public static IOAuthParameters createFromApiUrl(String apiUrl, OAuthVersion oAuthVersion) { + final String originalApiUrl = apiUrl; // We actually need the host if (apiUrl.startsWith("https://") || apiUrl.startsWith("http://")) { try { @@ -218,6 +251,10 @@ public final class OAuthParameters { } catch (CredentialsAgentException e) { Logging.trace(e); } + IOAuthParameters remembered = createFromPreferences(originalApiUrl, apiUrl); + if (remembered != null) { + return remembered; + } return createDefault(apiUrl, oAuthVersion); default: throw new IllegalArgumentException("Unknown OAuth version: " + oAuthVersion); diff --git a/src/org/openstreetmap/josm/gui/oauth/AdvancedOAuthPropertiesPanel.java b/src/org/openstreetmap/josm/gui/oauth/AdvancedOAuthPropertiesPanel.java index a1c1cf8c0e..c07549bb62 100644 --- a/src/org/openstreetmap/josm/gui/oauth/AdvancedOAuthPropertiesPanel.java +++ b/src/org/openstreetmap/josm/gui/oauth/AdvancedOAuthPropertiesPanel.java @@ -14,6 +14,8 @@ import javax.swing.BorderFactory; import javax.swing.JCheckBox; import javax.swing.JLabel; import javax.swing.JOptionPane; +import javax.swing.event.DocumentEvent; +import javax.swing.event.DocumentListener; import org.openstreetmap.josm.data.oauth.IOAuthParameters; import org.openstreetmap.josm.data.oauth.OAuth20Parameters; @@ -47,6 +49,12 @@ import org.openstreetmap.josm.tools.ImageProvider; * @since 2746 */ public class AdvancedOAuthPropertiesPanel extends VerticallyScrollablePanel { + /** + * Property fired whenever the parameters this panel would return change: the client id was edited, + * or "use default settings" was toggled. Listeners re-read {@link #getAdvancedParameters()}. + * @since xxx + */ + public static final String PARAMETERS_CHANGED_PROP = AdvancedOAuthPropertiesPanel.class.getName() + ".parametersChanged"; private final JCheckBox cbUseDefaults = new JCheckBox(tr("Use default settings")); private final JosmTextField tfConsumerKey = new JosmTextField(); @@ -137,6 +145,46 @@ public class AdvancedOAuthPropertiesPanel extends VerticallyScrollablePanel { ilUseDefault = new UseDefaultItemListener(); cbUseDefaults.addItemListener(ilUseDefault); cbUseDefaults.setSelected(Config.getPref().getBoolean("oauth.settings.use-default", true)); + cbUseDefaults.addItemListener(e -> fireParametersChanged()); + tfConsumerKey.getDocument().addDocumentListener(new DocumentListener() { + @Override + public void insertUpdate(DocumentEvent e) { + fireParametersChanged(); + } + + @Override + public void removeUpdate(DocumentEvent e) { + fireParametersChanged(); + } + + @Override + public void changedUpdate(DocumentEvent e) { + fireParametersChanged(); + } + }); + } + + private void fireParametersChanged() { + firePropertyChange(PARAMETERS_CHANGED_PROP, null, null); + } + + /** + * Determines if the panel is set to use the default parameters for the API URL rather than + * the values entered in its fields. + * @return {@code true} if "use default settings" is selected + * @since xxx + */ + public boolean isUseDefaultSettings() { + return cbUseDefaults.isSelected(); + } + + /** + * Get the client id as entered in the panel, without falling back to the defaults. + * @return the client id field's text + * @since xxx + */ + public String getEnteredClientId() { + return tfConsumerKey.getText(); } protected boolean hasCustomSettings() { diff --git a/src/org/openstreetmap/josm/gui/preferences/server/OAuthAuthenticationPreferencesPanel.java b/src/org/openstreetmap/josm/gui/preferences/server/OAuthAuthenticationPreferencesPanel.java index dde4ca262f..1ceeea0347 100644 --- a/src/org/openstreetmap/josm/gui/preferences/server/OAuthAuthenticationPreferencesPanel.java +++ b/src/org/openstreetmap/josm/gui/preferences/server/OAuthAuthenticationPreferencesPanel.java @@ -109,9 +109,24 @@ public class OAuthAuthenticationPreferencesPanel extends JPanel implements Prope ) ); pnlAdvancedProperties.setVisible(false); + pnlAdvancedProperties.addPropertyChangeListener(this); return pnl; } + /** + * Re-evaluate whether the "Authorize now" buttons can be enabled, after the API URL or the + * advanced OAuth parameters changed. + */ + private void updateAuthoriseNowActions() { + for (JPanel panel : Arrays.asList(this.pnlNotYetAuthorised, (JPanel) this.pnlAlreadyAuthorised.getComponent(6))) { + for (Component component : panel.getComponents()) { + if (component instanceof JButton && ((JButton) component).getAction() instanceof AuthoriseNowAction) { + ((AuthoriseNowAction) ((JButton) component).getAction()).updateEnabledState(); + } + } + } + } + /** * builds the UI */ @@ -148,13 +163,7 @@ public class OAuthAuthenticationPreferencesPanel extends JPanel implements Prope public void setApiUrl(String apiUrl) { this.apiUrl = apiUrl; pnlAdvancedProperties.setApiUrl(apiUrl); - for (JPanel panel : Arrays.asList(this.pnlNotYetAuthorised, (JPanel) this.pnlAlreadyAuthorised.getComponent(6))) { - for (Component component : panel.getComponents()) { - if (component instanceof JButton && ((JButton) component).getAction() instanceof AuthoriseNowAction) { - ((AuthoriseNowAction) ((JButton) component).getAction()).updateEnabledState(); - } - } - } + updateAuthoriseNowActions(); } /** @@ -318,6 +327,11 @@ public class OAuthAuthenticationPreferencesPanel extends JPanel implements Prope void updateEnabledState() { if (procedure == AuthorizationProcedure.MANUALLY) { this.setEnabled(true); + } else if (!pnlAdvancedProperties.isUseDefaultSettings()) { + // The user supplied the parameters - the client id of an OAuth application they + // registered on the server themselves. The wizard is given exactly these parameters + // when the button is pressed, so they decide whether it can be pressed. + this.setEnabled(!Utils.isEmpty(pnlAdvancedProperties.getEnteredClientId())); } else if (Utils.isValidUrl(apiUrl)) { final URI apiURI; try { @@ -420,8 +434,10 @@ public class OAuthAuthenticationPreferencesPanel extends JPanel implements Prope @Override public void propertyChange(PropertyChangeEvent evt) { - if (!evt.getPropertyName().equals(OsmApiUrlInputPanel.API_URL_PROP)) - return; - setApiUrl((String) evt.getNewValue()); + if (AdvancedOAuthPropertiesPanel.PARAMETERS_CHANGED_PROP.equals(evt.getPropertyName())) { + updateAuthoriseNowActions(); + } else if (OsmApiUrlInputPanel.API_URL_PROP.equals(evt.getPropertyName())) { + setApiUrl((String) evt.getNewValue()); + } } } -- 2.53.0