Use the current Roller session during OAuth authorization

app/src/main/java/org/apache/roller/weblogger/business/OAuthManager.java

@@ -88,10 +88,39 @@ OAuthAccessor getAccessor(OAuthMessage requestMessage)
             throws IOException, OAuthProblemException;
 
     /**
-     * Set the access token 
+     * Set the access token
+     *
+     * @deprecated Records approval against the consumer key alone, without
+     *             naming the request token being approved and without
+     *             requiring that it is still pending. Use
+     *             {@link #authorizeRequestToken(String, String, String)},
+     *             which does both in one statement. No longer called from
+     *             Roller; retained for callers outside the project.
      */
+    @Deprecated
     void markAsAuthorized(OAuthAccessor accessor, String userId)
-            throws OAuthException;    
+            throws OAuthException;
+
+    /**
+     * Record a user's approval of one pending request token.
+     *
+     * <p>The whole transition happens in a single conditional statement: the
+     * record is claimed only if it still matches the consumer key and the
+     * exact request token, has not been authorized already, and has not yet
+     * been exchanged for an access token. That makes approval one-shot without
+     * a read-then-write window in which the same token could be approved
+     * twice.
+     *
+     * @param consumerKey  key of the consumer the token was issued to
+     * @param requestToken the pending request token being approved
+     * @param userName     the approving user
+     * @return true if this call performed the approval; false if the record
+     *         did not match, was already authorized, or was already exchanged.
+     *         Callers should not distinguish these cases to the client.
+     * @throws OAuthException on persistence failure
+     */
+    boolean authorizeRequestToken(String consumerKey, String requestToken, String userName)
+            throws OAuthException;
 
     /**
      * Generate a fresh request token and secret for a consumer.

app/src/main/java/org/apache/roller/weblogger/business/jpa/JPAOAuthManagerImpl.java

@@ -22,6 +22,7 @@
 import java.sql.Timestamp;
 import java.util.Date;
 import java.util.UUID;
+import jakarta.persistence.Query;
 import jakarta.persistence.TypedQuery;
 import net.oauth.OAuthAccessor;
 import net.oauth.OAuthConsumer;
@@ -139,6 +140,25 @@ public void markAsAuthorized(OAuthAccessor accessor, String userId)
         }
     }
 
+    @Override
+    public boolean authorizeRequestToken(String consumerKey, String requestToken, String userName)
+            throws OAuthException {
+        if (consumerKey == null || requestToken == null || userName == null) {
+            return false;
+        }
+        try {
+            Query q = strategy.getNamedUpdate("OAuthAccessorRecord.authorizeRequestToken");
+            q.setParameter(1, userName);
+            q.setParameter(2, new Timestamp(new Date().getTime()));
+            q.setParameter(3, consumerKey);
+            q.setParameter(4, requestToken);
+            return q.executeUpdate() == 1;
+
+        } catch (WebloggerException ex) {
+            throw new OAuthException("ERROR: authorizing request token", ex);
+        }
+    }
+
     /**
      * Generate a fresh request token and secret for a consumer.
      * @throws OAuthException

app/src/main/java/org/apache/roller/weblogger/webservices/oauth/AuthorizationServlet.java

@@ -28,11 +28,14 @@
 import net.oauth.OAuth;
 import net.oauth.OAuthAccessor;
 import net.oauth.OAuthMessage;
+import net.oauth.OAuthProblemException;
 import net.oauth.server.OAuthServlet;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.apache.roller.weblogger.business.OAuthManager;
 import org.apache.roller.weblogger.business.WebloggerFactory;
+import org.apache.roller.weblogger.pojos.User;
+import org.apache.roller.weblogger.ui.core.RollerSession;
 
 /**
  * Authorization request handler.
@@ -42,7 +45,14 @@
  */
 public class AuthorizationServlet extends HttpServlet {
     protected static final Log log = LogFactory.getFactory().getInstance(AuthorizationServlet.class);
-
+
+    /**
+     * One response for every refusal, so the endpoint reveals nothing about
+     * tokens the caller does not hold.
+     */
+    private static final String PERMISSION_DENIED = "permission_denied";
+
+
     @Override
     public void doGet(HttpServletRequest request, HttpServletResponse response)
             throws IOException, ServletException {
@@ -71,40 +81,93 @@ public void doPost(HttpServletRequest request, HttpServletResponse response)
 
         try{
             OAuthMessage requestMessage = OAuthServlet.getMessage(request, null);
-            
+
             OAuthManager omgr = WebloggerFactory.getWeblogger().getOAuthManager();
             OAuthAccessor accessor = omgr.getAccessor(requestMessage);
-
-            String userId = request.getParameter("userId");
-            if (userId == null) {
-                userId = request.getParameter("xoauth_requestor_id");
+            if (accessor == null || accessor.consumer == null || accessor.requestToken == null) {
+                denyPermission(response);
+                return;
             }
-
-            if (userId == null) {
-                // no user associted with the key, must be site-wide key,
-                // so get user to login and do the authorization process
+
+            // The approving identity comes from the browser session, consistent
+            // with the rest of the UI. Without a session there is nobody to
+            // approve on behalf of, so send the caller through the login flow.
+            User user = getAuthenticatedUser(request);
+            if (user == null) {
                 sendToAuthorizePage(request, response, accessor);
-
-            } else {
+                return;
+            }
+            if (!Boolean.TRUE.equals(user.getEnabled())) {
+                denyPermission(response);
+                return;
+            }
+            String userId = user.getUserName();
 
-                // if consumer key is for specific user, check username match
-                String consumerUserId = (String)accessor.consumer.getProperty("userId");
-                if (consumerUserId != null && !userId.equals(consumerUserId)) {
-                    throw new ServletException("ERROR: invalid or unspecified userId");
-                }
+            // A consumer key bound to one user may only be approved by that
+            // user. A site-wide key has no bound user and is approved as
+            // whoever is logged in.
+            String consumerUserId = (String)accessor.consumer.getProperty("userId");
+            if (consumerUserId != null && !consumerUserId.equals(userId)) {
+                denyPermission(response);
+                return;
+            }
 
-                // set userId in accessor and mark it as authorized
-                omgr.markAsAuthorized(accessor, userId);
-                WebloggerFactory.getWeblogger().flush();
+            // Older clients still post the identity; accept it only when it
+            // agrees with the session.
+            String submittedUserId = request.getParameter("userId");
+            if (submittedUserId == null) {
+                submittedUserId = request.getParameter("xoauth_requestor_id");
             }
-
+            if (submittedUserId != null && !submittedUserId.equals(userId)) {
+                denyPermission(response);
+                return;
+            }
+
+            // Claim the pending request token in one conditional statement, so
+            // approval is one-shot. A token that is missing, belongs to another
+            // consumer, or has already been approved or exchanged all produce
+            // the same answer here and the same response below.
+            if (!omgr.authorizeRequestToken(
+                    accessor.consumer.consumerKey, accessor.requestToken, userId)) {
+                denyPermission(response);
+                return;
+            }
+            WebloggerFactory.getWeblogger().flush();
+
+            accessor.setProperty("userId", userId);
+            accessor.setProperty("authorized", Boolean.TRUE);
+
             returnToConsumer(request, response, accessor);
-
+
+        } catch (OAuthProblemException e) {
+            denyPermission(response);
         } catch (Exception e){
             handleException(e, request, response, true);
         }
     }
-
+
+    /**
+     * The Roller user behind this request's session, or null if there is none.
+     */
+    private User getAuthenticatedUser(HttpServletRequest request) {
+        RollerSession rollerSession = RollerSession.getRollerSession(request);
+        return rollerSession == null ? null : rollerSession.getAuthenticatedUser();
+    }
+
+    /**
+     * Refuse the approval, in the OAuth problem-reporting form and with the
+     * same body for every reason. Written directly rather than thrown so the
+     * response does not vary with how the library happens to render a given
+     * exception.
+     */
+    private void denyPermission(HttpServletResponse response) throws IOException {
+        response.setStatus(HttpServletResponse.SC_FORBIDDEN);
+        response.setContentType("text/plain");
+        try (PrintWriter out = response.getWriter()) {
+            out.println("oauth_problem=" + PERMISSION_DENIED);
+        }
+    }
+
     private void sendToAuthorizePage(HttpServletRequest request, 
             HttpServletResponse response, OAuthAccessor accessor)
     throws IOException, ServletException{

app/src/main/resources/org/apache/roller/weblogger/pojos/OAuthAccessorRecord.orm.xml

@@ -16,6 +16,9 @@
         <named-query name="OAuthAccessorRecord.getByUserName">
             <query>SELECT p FROM OAuthAccessorRecord p WHERE p.userName = ?1</query>
         </named-query>
+        <named-query name="OAuthAccessorRecord.authorizeRequestToken">
+            <query>UPDATE OAuthAccessorRecord p SET p.userName = ?1, p.authorized = true, p.updated = ?2 WHERE p.consumerKey = ?3 AND p.requestToken = ?4 AND (p.authorized IS NULL OR p.authorized = false) AND p.accessToken IS NULL</query>
+        </named-query>
         <attributes>
             <id name="consumerKey">
                 <column name="consumerkey"/>

app/src/main/webapp/WEB-INF/jsps/core/OAuthAuthorize.jsp

@@ -30,7 +30,7 @@
 </p>
 
 <form name="authZForm" action="authorize" method="POST">
-    <input type="hidden" name="userId" value="<s:property value="userName" />" size="20" /><br>
+    <s:hidden name="salt"/>
     <input type="hidden" name="oauth_token" value='<s:property value="token" />' />
     <input type="hidden" name="oauth_callback" value='<s:property value="callback" />' />
     <input type="submit" name="Authorize" value="Authorize"/>

app/src/main/webapp/WEB-INF/web.xml

@@ -153,6 +153,16 @@
 		<url-pattern>/roller-ui/*</url-pattern>
 	</filter-mapping>
 
+	<!-- The OAuth consent form is served by a forward into /roller-ui/, so it
+	     already receives a token from LoadSaltFilter above, but it posts back
+	     outside /roller-ui/. Validate that one exact URL; the request-token and
+	     access-token endpoints are consumer-to-server calls carrying an OAuth
+	     signature and must not be included. -->
+	<filter-mapping>
+		<filter-name>ValidateSaltFilter</filter-name>
+		<url-pattern>/roller-services/oauth/authorize</url-pattern>
+	</filter-mapping>
+
     <!-- Request mapping, this is what allows the urls to work -->
     <filter-mapping>
         <filter-name>RequestMappingFilter</filter-name>

app/src/test/java/org/apache/roller/weblogger/business/jpa/JPAOAuthManagerTest.java

@@ -90,4 +90,34 @@ public void testCRUD() throws Exception {
         TestUtils.endSession(true);
         assertNull(omgr.getConsumerByKey(consumerKey));
     }
+
+    @Test
+    public void testAuthorizeRequestTokenTransition() throws Exception {
+        JPAOAuthManagerImpl omgr = (JPAOAuthManagerImpl)
+            WebloggerFactory.getWeblogger().getOAuthManager();
+
+        String consumerKey = "authorization-consumer";
+        String requestToken = "pending-request-token";
+        OAuthConsumer consumer = omgr.addConsumer("authorization-owner", consumerKey);
+
+        OAuthAccessor accessor = new OAuthAccessor(consumer);
+        accessor.requestToken = requestToken;
+        accessor.tokenSecret = "pending-token-secret";
+        omgr.addAccessor(accessor);
+        TestUtils.endSession(true);
+
+        assertFalse(omgr.authorizeRequestToken(consumerKey, "another-token", "alice"));
+        assertTrue(omgr.authorizeRequestToken(consumerKey, requestToken, "alice"));
+        assertFalse(omgr.authorizeRequestToken(consumerKey, requestToken, "alice"));
+        TestUtils.endSession(true);
+
+        OAuthAccessor authorized = omgr.getAccessorByToken(requestToken);
+        assertNotNull(authorized);
+        assertEquals("alice", authorized.getProperty("userId"));
+        assertEquals(Boolean.TRUE, authorized.getProperty("authorized"));
+
+        omgr.removeAccessor(authorized);
+        omgr.removeConsumer(consumer);
+        TestUtils.endSession(true);
+    }
 }