fix(admin): allow the SPNEGO pre-authentication password to be cleared - #3242
Merged
Conversation
The General screen refused to store an empty spnego.preauth.password so that submitting the mask rendered by updateForm would not overwrite the stored secret. For this setting an empty value is not "unchanged" but a configuration in its own right: the SPNEGO library only uses a keytab when both the pre-authentication user name and the password are empty. Once any password had been saved, a keytab setup was therefore unreachable from the admin screen. Treat an absent or empty submission as a request to remove the key, and keep ignoring a submission made only of mask characters. Also apply the NTLM prompt rule to the API. The rule rejecting spnego.allow.basic=false together with spnego.prompt.ntlm=true was only checked on the HTML path, so the same combination could still be stored through PUT /api/admin/general and break SSO on the next login. The API merges the request body over the stored settings, so the check has to run on the merged result rather than on the request body alone.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two gaps left by #3181 / #3220 in the General settings, both on the admin write path.
1. The SPNEGO pre-authentication password cannot be cleared
updateConfigstored the password only when the submitted value survived the mask check:That is the same shape as the other secrets on this screen, and for them it is right: the
form renders
**********instead of the stored value, so an unchanged submission must notoverwrite it. Clearing the field simply did nothing.
For SPNEGO an empty password is not "unchanged", it is a configuration:
#3181 changed the coded defaults for
spnego.preauth.username/spnego.preauth.passwordto empty precisely so a keytab-based server login would work. But once any password had been
saved through this screen, the key could never be removed again, so
useKeyTab()stayedfalse and the keytab setup was unreachable without editing
system.propertiesby hand.An absent or empty submission now removes the key. A submission made only of mask characters
is still ignored, so the round trip through
updateFormkeeps behaving as before, and theother secrets are untouched.
2. The NTLM prompt rule was not applied to the API
#3220 rejects
spnego.allow.basic=falsetogether withspnego.prompt.ntlm=true, because theSPNEGO library refuses that combination when it builds its configuration — lazily, on the
first login, so without this check SSO breaks silently long after the save. The check lived
only in
AdminGeneralAction#update, soPUT /api/admin/generalcould still store it.Moving the same rule into the API needs one thing beyond copying it: the API merges the
request body over the stored settings, so a request that carries only one half of the pair
takes the other half from what is already stored. The check therefore runs on the merged
body, after
BeanUtil.copyBeanToBean, while the request body itself is still what getsvalidated against its own bean constraints. The rule is extracted to
AdminGeneralAction#isSpnegoNtlmPromptUnsupportedso both paths share one definition.Tests
AdminGeneralActionTest: 10 tests, no failures (7 + 3).test_updateConfig_spnegoPreauthPassword_canBeCleared— a stored password is removed by anullsubmission (the HTML path, where LastaFlute maps an empty field tonull) and by anempty string (the API path, which does not)
test_updateConfig_spnegoPreauthPassword_maskKeepsStoredValue— anupdateForm/updateConfiground trip leaves the stored password alonetest_isSpnegoNtlmPromptUnsupported— the extracted ruleThe first test fails against the previous code with
expected: <null> but was: <secret>,which is the defect it pins.
mvn javadoc:jarwas regenerated from scratch: no new warnings from the touched files.