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]
