mraible commented on code in PR #166:
URL: https://github.com/apache/roller/pull/166#discussion_r3891395611
##########
app/src/main/java/org/apache/roller/weblogger/webservices/atomprotocol/RollerAtomHandler.java:
##########
@@ -118,15 +116,15 @@ public RollerAtomHandler(HttpServletRequest request,
HttpServletResponse respons
roller = WebloggerFactory.getWeblogger();
String userName;
- if
("oauth".equals(WebloggerRuntimeConfig.getProperty("webservices.atomPubAuth")))
{
+ String authenticationMethod =
+ WebloggerRuntimeConfig.getProperty("webservices.atomPubAuth");
+ if ("oauth".equals(authenticationMethod)) {
userName = authenticationOAUTH(request, response);
-
- } else if
("wsse".equals(WebloggerRuntimeConfig.getProperty("webservices.atomPubAuth"))) {
- userName = authenticateWSSE(request);
-
- } else {
- // default to basic
+ } else if ("basic".equals(authenticationMethod)) {
userName = authenticateBASIC(request);
+ } else {
Review Comment:
This is now the only password-based AtomPub path, and it doesn't work:
authenticateBASIC (around line 432) checks the password against the instance
field `user`, which is still null while the constructor is running, instead of
the `inUser` it just looked up. The NPE is caught and logged at debug, valid
stays false, and every correctly authenticated Basic request gets a 401.
Pre-existing on master, but this PR points former WSSE users at it. The fix is
`inUser.getPassword()` in place of `user.getPassword()`; apache/roller commit
dd2b28397 on feature/jakarta-ee-10-migration has exactly that change plus a
smoke test if you want to cherry-pick.
##########
app/src/main/java/org/apache/roller/weblogger/webservices/atomprotocol/RollerAtomHandler.java:
##########
@@ -118,15 +116,15 @@ public RollerAtomHandler(HttpServletRequest request,
HttpServletResponse respons
roller = WebloggerFactory.getWeblogger();
String userName;
- if
("oauth".equals(WebloggerRuntimeConfig.getProperty("webservices.atomPubAuth")))
{
+ String authenticationMethod =
+ WebloggerRuntimeConfig.getProperty("webservices.atomPubAuth");
+ if ("oauth".equals(authenticationMethod)) {
userName = authenticationOAUTH(request, response);
-
- } else if
("wsse".equals(WebloggerRuntimeConfig.getProperty("webservices.atomPubAuth"))) {
- userName = authenticateWSSE(request);
-
- } else {
- // default to basic
+ } else if ("basic".equals(authenticationMethod)) {
Review Comment:
webservices.atomPubAuth is a free-text runtime property (a plain textbox on
the config page), and the old code fell through to Basic for anything that
wasn't oauth or wsse. An exact, case-sensitive match means an admin who typed
Basic or left trailing whitespace is now denied. A trim() and toLowerCase()
before the comparisons keeps the fail-closed behavior for genuinely unknown
values without punishing that.
##########
app/src/main/java/org/apache/roller/weblogger/webservices/atomprotocol/RollerAtomHandler.java:
##########
@@ -118,15 +116,15 @@ public RollerAtomHandler(HttpServletRequest request,
HttpServletResponse respons
roller = WebloggerFactory.getWeblogger();
String userName;
- if
("oauth".equals(WebloggerRuntimeConfig.getProperty("webservices.atomPubAuth")))
{
+ String authenticationMethod =
+ WebloggerRuntimeConfig.getProperty("webservices.atomPubAuth");
+ if ("oauth".equals(authenticationMethod)) {
userName = authenticationOAUTH(request, response);
-
- } else if
("wsse".equals(WebloggerRuntimeConfig.getProperty("webservices.atomPubAuth"))) {
- userName = authenticateWSSE(request);
-
- } else {
- // default to basic
+ } else if ("basic".equals(authenticationMethod)) {
userName = authenticateBASIC(request);
+ } else {
+ log.warn("Unsupported AtomPub authentication method;
authentication denied");
Review Comment:
For an upgraded install that still has wsse persisted in roller_properties,
this fires on every AtomPub request without saying what the value is or what to
change it to, so the lockout is hard to diagnose from the log. Including the
value and the accepted options (basic, oauth) in the message makes it
self-explanatory; logging it once rather than per request would be a bonus.
##########
app/src/test/java/org/apache/roller/weblogger/webservices/atomprotocol/RollerAtomHandlerTest.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0.
+ */
+package org.apache.roller.weblogger.webservices.atomprotocol;
+
+import org.apache.roller.weblogger.business.URLStrategy;
+import org.apache.roller.weblogger.business.UserManager;
+import org.apache.roller.weblogger.business.Weblogger;
+import org.apache.roller.weblogger.business.WebloggerFactory;
+import org.apache.roller.weblogger.config.WebloggerRuntimeConfig;
+import org.apache.roller.weblogger.pojos.User;
+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.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import java.nio.charset.StandardCharsets;
+import java.security.MessageDigest;
+import java.time.Instant;
+import java.util.Base64;
+
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.mockito.Mockito.mockStatic;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+class RollerAtomHandlerTest {
+
+ private static final String USER_NAME = "alice";
+ private static final String PASSWORD = "test-password";
+
+ @Mock
+ private HttpServletRequest request;
+
+ @Mock
+ private HttpServletResponse response;
+
+ @Mock
+ private Weblogger weblogger;
+
+ @Mock
+ private UserManager userManager;
+
+ @Mock
+ private URLStrategy urlStrategy;
+
+ @BeforeEach
+ void setUp() throws Exception {
+ MockitoAnnotations.openMocks(this);
+
+ User user = new User();
+ user.setUserName(USER_NAME);
+ user.setPassword(PASSWORD);
+
+ when(weblogger.getUserManager()).thenReturn(userManager);
+ when(weblogger.getUrlStrategy()).thenReturn(urlStrategy);
+
when(urlStrategy.getAtomProtocolURL(true)).thenReturn("https://example.test/app";);
+ when(userManager.getUserByUserName(USER_NAME)).thenReturn(user);
+ }
+
+ @Test
+ void wsseAuthenticationModeIsRejected() throws Exception {
Review Comment:
This covers the wsse-is-denied case, but nothing asserts that basic still
authenticates through the Authorization header (which would have caught the NPE
above), or that null and unknown values are denied. Those are the cases a later
refactor of this dispatch is most likely to break.
--
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]
