Retire the WSSE AtomPub authentication mode - #166
Conversation
WSSE has no other production caller; AtomPub retains Basic and OAuth authentication. Claude-Session: https://claude.ai/code/session_01A1fhY1E2PCFU6UAPXu2WtV
mraible
left a comment
mraible
left a comment
There was a problem hiding this comment.
Reviewed with the multi-agent find-and-verify pass. The removal itself is clean and failing closed on unknown values is the right call (we did the same for the removed oauth value in #154). One thing blocks approval, and it's a one-word fix: the Basic path this PR now routes every password-based client to is dead on master. Details inline, with the fix. Two smaller notes on the dispatch and one on test coverage. Happy to approve as soon as the Basic fix is in.
Heads up for whichever of this and #154 merges second: both touch this dispatch block, so there will be a small conflict to resolve.
| // default to basic | ||
| } else if ("basic".equals(authenticationMethod)) { | ||
| userName = authenticateBASIC(request); | ||
| } else { |
There was a problem hiding this 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 dd2b283 on feature/jakarta-ee-10-migration has exactly that change plus a smoke test if you want to cherry-pick.
|
|
||
| } else { | ||
| // default to basic | ||
| } else if ("basic".equals(authenticationMethod)) { |
There was a problem hiding this 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.
| } else if ("basic".equals(authenticationMethod)) { | ||
| userName = authenticateBASIC(request); | ||
| } else { | ||
| log.warn("Unsupported AtomPub authentication method; authentication denied"); |
There was a problem hiding this 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.
| } | ||
|
|
||
| @Test | ||
| void wsseAuthenticationModeIsRejected() throws Exception { |
There was a problem hiding this 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.
2 participants
Roller's AtomPub endpoint supports a WSSE digest authentication mode that is no
longer used and is not maintained. This change removes it rather than carrying
it forward.
What changed
wssefrom the AtomPub authentication dispatch and delete the WSSEparser/generator utility, which has no other production caller.
webservices.atomPubAuth=wsse(or any other unsupported value); never silentlyreinterpret that value as Basic authentication.
coordination is required.
Tests
webservices.atomPubAuth=wsse, a request stays unauthenticated and doesnot fall back to Basic authentication.
pass.