Ticket #24890: 0001-OAuth-2-honour-user-supplied-parameters-for-servers-.patch

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

@@ -27 +27 @@
 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 +126 @@
             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 +140 @@
                 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) {

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

@@ -78 +78 @@
         }
     }
 
+    /**
+     * 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 +231 @@
      * @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 +251 @@
                 } 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);

src/org/openstreetmap/josm/gui/oauth/AdvancedOAuthPropertiesPanel.java

@@ -14 +14 @@
 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 +49 @@
  * @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 +145 @@
         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() {

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

@@ -109 +109 @@
                 )
         );
         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 +163 @@
     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 +327 @@
         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 +434 @@
 
     @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());
+        }
     }
 }