mraible commented on code in PR #165:
URL: https://github.com/apache/roller/pull/165#discussion_r3891405471
##########
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>
Review Comment:
Good to gate the consent POST, but `ValidateSaltFilter` throws
`ServletException("Security Violation")` when the salt is missing, expired
(`SaltCache` entries live 60 minutes, and `cache.salt.size`=5000 evicts),
already consumed, or issued to a different user. A user who leaves the consent
page open for an hour and then clicks Authorize gets the container's 500 page
rather than the consent form again or an OAuth problem response. Same on a
cluster without a shared `SaltCache`. Catching that case in the servlet
(re-render with a fresh salt) would keep the protection without the cliff.
##########
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.
+ *
+ * <p>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<WebloggerFactory> factory =
mockStatic(WebloggerFactory.class);
+ MockedStatic<RollerSession> 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");
Review Comment:
The mismatched-identity tests assert that nothing was authorized for admin,
but not the 403, and not that alice wasn't authorized either. As written they'd
pass if the mismatch were silently accepted for alice. Asserting the response
status and the absence of any authorization pins the behavior the description
promises.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
