diff --git a/app/src/main/java/org/apache/roller/weblogger/business/OAuthManager.java b/app/src/main/java/org/apache/roller/weblogger/business/OAuthManager.java index 329b225b00..ef8549354e 100644 --- a/app/src/main/java/org/apache/roller/weblogger/business/OAuthManager.java +++ b/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. + * + *

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. diff --git a/app/src/main/java/org/apache/roller/weblogger/business/jpa/JPAOAuthManagerImpl.java b/app/src/main/java/org/apache/roller/weblogger/business/jpa/JPAOAuthManagerImpl.java index baebb320d1..373711c94a 100644 --- a/app/src/main/java/org/apache/roller/weblogger/business/jpa/JPAOAuthManagerImpl.java +++ b/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 diff --git a/app/src/main/java/org/apache/roller/weblogger/webservices/oauth/AuthorizationServlet.java b/app/src/main/java/org/apache/roller/weblogger/webservices/oauth/AuthorizationServlet.java index 7f4787642b..93e366f21d 100644 --- a/app/src/main/java/org/apache/roller/weblogger/webservices/oauth/AuthorizationServlet.java +++ b/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{ diff --git a/app/src/main/resources/org/apache/roller/weblogger/pojos/OAuthAccessorRecord.orm.xml b/app/src/main/resources/org/apache/roller/weblogger/pojos/OAuthAccessorRecord.orm.xml index 3c710aff7e..3cac65a7fa 100644 --- a/app/src/main/resources/org/apache/roller/weblogger/pojos/OAuthAccessorRecord.orm.xml +++ b/app/src/main/resources/org/apache/roller/weblogger/pojos/OAuthAccessorRecord.orm.xml @@ -16,6 +16,9 @@ SELECT p FROM OAuthAccessorRecord p WHERE p.userName = ?1 + + 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 + diff --git a/app/src/main/webapp/WEB-INF/jsps/core/OAuthAuthorize.jsp b/app/src/main/webapp/WEB-INF/jsps/core/OAuthAuthorize.jsp index bc63ca2a42..befdfe26fd 100644 --- a/app/src/main/webapp/WEB-INF/jsps/core/OAuthAuthorize.jsp +++ b/app/src/main/webapp/WEB-INF/jsps/core/OAuthAuthorize.jsp @@ -30,7 +30,7 @@

- " size="20" />
+ diff --git a/app/src/main/webapp/WEB-INF/web.xml b/app/src/main/webapp/WEB-INF/web.xml index 0418832da1..1a3f147eae 100644 --- a/app/src/main/webapp/WEB-INF/web.xml +++ b/app/src/main/webapp/WEB-INF/web.xml @@ -153,6 +153,16 @@ /roller-ui/* + + + ValidateSaltFilter + /roller-services/oauth/authorize + + RequestMappingFilter diff --git a/app/src/test/java/org/apache/roller/weblogger/business/jpa/JPAOAuthManagerTest.java b/app/src/test/java/org/apache/roller/weblogger/business/jpa/JPAOAuthManagerTest.java index 0b2ab39bfc..26a5be182c 100644 --- a/app/src/test/java/org/apache/roller/weblogger/business/jpa/JPAOAuthManagerTest.java +++ b/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); + } } diff --git a/app/src/test/java/org/apache/roller/weblogger/webservices/oauth/AuthorizationServletTest.java b/app/src/test/java/org/apache/roller/weblogger/webservices/oauth/AuthorizationServletTest.java new file mode 100644 index 0000000000..a4c7f08029 --- /dev/null +++ b/app/src/test/java/org/apache/roller/weblogger/webservices/oauth/AuthorizationServletTest.java @@ -0,0 +1,325 @@ +/* +* Licensed to the Apache Software Foundation (ASF) under one or more +* contributor license agreements. The ASF licenses this file to You +* under the Apache License, Version 2.0 (the "License"); you may not +* use this file except in compliance with the License. +* You may obtain a copy of the License at +* +* http://www.apache.org/licenses/LICENSE-2.0 +* +* Unless required by applicable law or agreed to in writing, software +* distributed under the License is distributed on an "AS IS" BASIS, +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +* See the License for the specific language governing permissions and +* limitations under the License. For additional information regarding +* copyright in this work, please see the NOTICE file in the top level +* directory of this distribution. +*/ + +package org.apache.roller.weblogger.webservices.oauth; + +import net.oauth.OAuthAccessor; +import net.oauth.OAuthConsumer; +import net.oauth.OAuthProblemException; +import org.apache.roller.weblogger.business.OAuthManager; +import org.apache.roller.weblogger.business.Weblogger; +import org.apache.roller.weblogger.business.WebloggerFactory; +import org.apache.roller.weblogger.pojos.User; +import org.apache.roller.weblogger.ui.core.RollerSession; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.Mock; +import org.mockito.MockedStatic; +import org.mockito.MockitoAnnotations; + +import javax.servlet.RequestDispatcher; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import java.io.PrintWriter; +import java.io.StringWriter; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.*; + +/** + * Verifies which identity {@link AuthorizationServlet} authorizes a request + * token for. + * + *

The servlet runs the browser consent step: a logged-in user approves a + * consumer's pending request token. The identity being approved is a property + * of the session, so these tests pin it there and check that values arriving + * in the request body cannot redirect the approval onto a different account. + */ +public class AuthorizationServletTest { + + private static final String CONSUMER_KEY = "test-consumer-key"; + private static final String REQUEST_TOKEN = "test-request-token"; + + private AuthorizationServlet servlet; + + @Mock + private HttpServletRequest request; + + @Mock + private HttpServletResponse response; + + @Mock + private RequestDispatcher dispatcher; + + @Mock + private RollerSession rollerSession; + + @Mock + private Weblogger weblogger; + + @Mock + private OAuthManager oauthManager; + + private OAuthAccessor accessor; + private StringWriter responseBody; + + @BeforeEach + public void setUp() throws Exception { + MockitoAnnotations.openMocks(this); + servlet = new AuthorizationServlet(); + + // A site-wide consumer: no "userId" property bound to the key. This is + // the configuration in which the servlet has no consumer-side identity + // to compare against and must fall back on the session. + OAuthConsumer consumer = + new OAuthConsumer("http://example.com/callback", CONSUMER_KEY, "secret", null); + accessor = new OAuthAccessor(consumer); + accessor.requestToken = REQUEST_TOKEN; + + when(request.getMethod()).thenReturn("POST"); + when(request.getRequestURL()) + .thenReturn(new StringBuffer("https://example.com/roller-services/oauth/authorize")); + when(request.getLocalName()).thenReturn("example.com"); + when(request.getRequestDispatcher(anyString())).thenReturn(dispatcher); + responseBody = new StringWriter(); + when(response.getWriter()).thenReturn(new PrintWriter(responseBody)); + + when(weblogger.getOAuthManager()).thenReturn(oauthManager); + when(oauthManager.getAccessor(any())).thenReturn(accessor); + } + + private User user(String userName) { + User u = new User(); + u.setUserName(userName); + u.setEnabled(Boolean.TRUE); + return u; + } + + /** + * Assert that no approval was recorded for {@code userName}, through either + * of the manager's authorizing entry points. Checking both matters: a test + * that named only one of them would pass whenever the servlet happened to + * use the other. + */ + private void verifyNothingAuthorizedFor(String userName) throws Exception { + verify(oauthManager, never()).markAsAuthorized(any(), eq(userName)); + verify(oauthManager, never()).authorizeRequestToken(anyString(), anyString(), eq(userName)); + } + + /** + * Assert that no approval was recorded at all. + */ + private void verifyNothingAuthorized() throws Exception { + verify(oauthManager, never()).markAsAuthorized(any(), anyString()); + verify(oauthManager, never()).authorizeRequestToken(anyString(), anyString(), anyString()); + } + + /** + * The session belongs to "alice" but the posted form names "admin". The + * approval must not be recorded for "admin". + */ + @Test + public void postedUserIdDoesNotChooseTheIdentity() throws Exception { + try (MockedStatic factory = mockStatic(WebloggerFactory.class); + MockedStatic session = mockStatic(RollerSession.class)) { + + factory.when(WebloggerFactory::getWeblogger).thenReturn(weblogger); + session.when(() -> RollerSession.getRollerSession(request)).thenReturn(rollerSession); + when(rollerSession.getAuthenticatedUser()).thenReturn(user("alice")); + + when(request.getParameter("userId")).thenReturn("admin"); + + servlet.doPost(request, response); + + verifyNothingAuthorizedFor("admin"); + } + } + + /** + * Same shape, using the alternate parameter name the servlet also reads. + */ + @Test + public void postedRequestorIdDoesNotChooseTheIdentity() throws Exception { + try (MockedStatic factory = mockStatic(WebloggerFactory.class); + MockedStatic session = mockStatic(RollerSession.class)) { + + factory.when(WebloggerFactory::getWeblogger).thenReturn(weblogger); + session.when(() -> RollerSession.getRollerSession(request)).thenReturn(rollerSession); + when(rollerSession.getAuthenticatedUser()).thenReturn(user("alice")); + + when(request.getParameter("xoauth_requestor_id")).thenReturn("admin"); + + servlet.doPost(request, response); + + verifyNothingAuthorizedFor("admin"); + } + } + + /** + * With nobody logged in there is no identity to approve, so nothing may be + * recorded no matter what the request body says. + */ + @Test + public void noSessionAuthorizesNobody() throws Exception { + try (MockedStatic factory = mockStatic(WebloggerFactory.class); + MockedStatic session = mockStatic(RollerSession.class)) { + + factory.when(WebloggerFactory::getWeblogger).thenReturn(weblogger); + session.when(() -> RollerSession.getRollerSession(request)).thenReturn(rollerSession); + when(rollerSession.getAuthenticatedUser()).thenReturn(null); + + when(request.getParameter("userId")).thenReturn("admin"); + + servlet.doPost(request, response); + + verifyNothingAuthorized(); + } + } + + /** + * A disabled account cannot approve anything, even with a live session. + */ + @Test + public void disabledUserAuthorizesNobody() throws Exception { + try (MockedStatic factory = mockStatic(WebloggerFactory.class); + MockedStatic session = mockStatic(RollerSession.class)) { + + factory.when(WebloggerFactory::getWeblogger).thenReturn(weblogger); + session.when(() -> RollerSession.getRollerSession(request)).thenReturn(rollerSession); + User disabled = user("alice"); + disabled.setEnabled(Boolean.FALSE); + when(rollerSession.getAuthenticatedUser()).thenReturn(disabled); + when(request.getParameter("userId")).thenReturn("alice"); + + servlet.doPost(request, response); + + verifyNothingAuthorized(); + } + } + + /** + * The ordinary path: the logged-in user approves, and the approval is + * recorded against the session identity and the pending token. + */ + @Test + public void sessionUserIsTheAuthorizedIdentity() throws Exception { + try (MockedStatic factory = mockStatic(WebloggerFactory.class); + MockedStatic session = mockStatic(RollerSession.class)) { + + factory.when(WebloggerFactory::getWeblogger).thenReturn(weblogger); + session.when(() -> RollerSession.getRollerSession(request)).thenReturn(rollerSession); + when(rollerSession.getAuthenticatedUser()).thenReturn(user("alice")); + when(oauthManager.authorizeRequestToken(CONSUMER_KEY, REQUEST_TOKEN, "alice")) + .thenReturn(Boolean.TRUE); + + servlet.doPost(request, response); + + verify(oauthManager).authorizeRequestToken(CONSUMER_KEY, REQUEST_TOKEN, "alice"); + } + } + + /** + * A legacy client may still post the parameter; when it agrees with the + * session it is simply redundant and the flow proceeds. + */ + @Test + public void matchingPostedUserIdIsTolerated() throws Exception { + try (MockedStatic factory = mockStatic(WebloggerFactory.class); + MockedStatic session = mockStatic(RollerSession.class)) { + + factory.when(WebloggerFactory::getWeblogger).thenReturn(weblogger); + session.when(() -> RollerSession.getRollerSession(request)).thenReturn(rollerSession); + when(rollerSession.getAuthenticatedUser()).thenReturn(user("alice")); + when(request.getParameter("userId")).thenReturn("alice"); + when(oauthManager.authorizeRequestToken(CONSUMER_KEY, REQUEST_TOKEN, "alice")) + .thenReturn(Boolean.TRUE); + + servlet.doPost(request, response); + + verify(oauthManager).authorizeRequestToken(CONSUMER_KEY, REQUEST_TOKEN, "alice"); + } + } + + /** + * A consumer key bound to one user may only be approved by that user, even + * though the identity now comes from the session rather than the request. + */ + @Test + public void consumerBoundToAnotherUserIsRefused() throws Exception { + try (MockedStatic factory = mockStatic(WebloggerFactory.class); + MockedStatic session = mockStatic(RollerSession.class)) { + + accessor.consumer.setProperty("userId", "bob"); + + factory.when(WebloggerFactory::getWeblogger).thenReturn(weblogger); + session.when(() -> RollerSession.getRollerSession(request)).thenReturn(rollerSession); + when(rollerSession.getAuthenticatedUser()).thenReturn(user("alice")); + when(request.getParameter("userId")).thenReturn("bob"); + + servlet.doPost(request, response); + + verifyNothingAuthorized(); + } + } + + /** + * Failure of the one-shot persistence transition must not reveal why the + * pending token was not claimed. + */ + @Test + public void unmatchedPendingTokenIsRefusedGenerically() throws Exception { + try (MockedStatic factory = mockStatic(WebloggerFactory.class); + MockedStatic session = mockStatic(RollerSession.class)) { + + factory.when(WebloggerFactory::getWeblogger).thenReturn(weblogger); + session.when(() -> RollerSession.getRollerSession(request)).thenReturn(rollerSession); + when(rollerSession.getAuthenticatedUser()).thenReturn(user("alice")); + when(oauthManager.authorizeRequestToken(CONSUMER_KEY, REQUEST_TOKEN, "alice")) + .thenReturn(Boolean.FALSE); + + servlet.doPost(request, response); + + verify(response).setStatus(HttpServletResponse.SC_FORBIDDEN); + assertEquals("oauth_problem=permission_denied\n", responseBody.toString()); + verify(weblogger, never()).flush(); + } + } + + /** + * Accessor lookup failures use the same response as a failed conditional + * transition, so callers cannot distinguish token states. + */ + @Test + public void unknownTokenIsRefusedGenerically() throws Exception { + try (MockedStatic factory = mockStatic(WebloggerFactory.class)) { + factory.when(WebloggerFactory::getWeblogger).thenReturn(weblogger); + when(oauthManager.getAccessor(any())) + .thenThrow(new OAuthProblemException("token_expired")); + + servlet.doPost(request, response); + + verify(response).setStatus(HttpServletResponse.SC_FORBIDDEN); + assertEquals("oauth_problem=permission_denied\n", responseBody.toString()); + verifyNothingAuthorized(); + } + } +}