[#1170] WindowsDesktopSSO: complete /json/authenticate from a Negotiate header in one request

[maximthomas]

Fix #1170

WindowsDesktopSSO presents an HttpCallback followed by a script TextOutputCallback. A request to /json/authenticate with no body but with Authorization: Negotiate <token> filled the HttpCallback. The script callback, however, can never be filled from a request, so RestAuthCallbackHandlerManager converted every callback to JSON, and RestAuthHttpCallbackHandler.convertToJson answered 400. The XUI hits the same 400 when the browser resends its first POST with the header after the 401 challenge, so no browser path completed Kerberos SSO.

An HttpCallback has no JSON representation. When one is present, output-only callbacks (TextOutputCallback, which covers the script callback, and NameValueOutputCallback) now count as filled. The legacy login page (LoginViewBean.processHttpCallback) submits them the same way.

• No header: the handler's 401 challenge with WWW-Authenticate: Negotiate, wherever the HttpCallback sits, as long as the callbacks before it are filled or output-only.
• Header present: the other callbacks are filled from the request where possible (for example a NameCallback from X-OpenAM-Username), and the callbacks are submitted to the module.
• An input callback that cannot be filled from the request, for example [HttpCallback, PasswordCallback] without X-OpenAM-Password, still ends in the 400, as on master.

Callback sets without an HttpCallback take the previous path unchanged. The six existing RestAuthCallbackHandlerManagerTest cases, which pin the order of factory calls, pass as they are.

Tests

• RestAuthCallbackHandlerManagerTest: five cases on the real callback handler factory:

  • the header is present and the script callback cannot be filled: submitted;
  • the header is present next to a NameValueOutputCallback: submitted;
  • the HttpCallback follows the script callback and there is no header: 401, not 400;
  • a NameCallback next to the HttpCallback is filled from X-OpenAM-Username;
  • a PasswordCallback next to the HttpCallback with only Authorization: 400, and the password stays unset.

  The first four fail with master's loop. The last one fails when every callback next to an HttpCallback is submitted. Removing either type from the output-only check turns the matching case red as well. mvn -o -pl openam-core-rest test: 446 run, 0 failures.

• e2e/kerberos/kerberos.spec.mjs: the curl and XUI cases no longer pin the 400. They assert 200 with a tokenId for the Kerberos user, and that the XUI leaves #login with that user's session. The XUI case reads the response body only on failure, because the page navigates away after a successful login. The spec runs in build.yml, step "UI Smoke Tests (Playwright) - HttpOnly disabled".

Not changed

• WindowsDesktopSSO as a later module of a chain through the XUI: the browser resends the POST with the previous module's callbacks, and handleJsonCallbacks rejects the type mismatch with 400. This predates WindowsDesktopSSO: /json/authenticate answers 400 to a Negotiate header, so Kerberos SSO through the XUI fails #1170 and needs a separate issue.
• A non-Negotiate Authorization header (for example Basic added by a proxy) on a bodyless WindowsDesktopSSO login now reaches the module and fails there, instead of failing with 400. The same was already reachable in two requests.

[vharseko]

Thanks, the root cause and the legacy-page parallel (LoginViewBean.processHttpCallback) are clear. One behavioural concern, one possible e2e flake, and a few cleanups inline.

The main point is the first inline comment. With an HttpCallback and its header present, every other callback is now submitted whether it could be filled or not. Before, that case failed closed with 400. None of the shipped modules are affected: WindowsDesktopSSO pairs HttpCallback with an output-only script callback, and HTTPBasic and Ntlm present HttpCallback alone. A custom module that pairs HttpCallback with an input callback would now receive it unfilled.

I did not raise the non-Negotiate Authorization header or the curl case's reliance on tokenId in the body. The description and the spec header already cover both as deliberate.

[vharseko]

The return value of updateCallbackFromRequest is ignored here, so input callbacks that cannot be filled from the request (PasswordCallback, ChoiceCallback, ConfirmationCallback, TextInputCallback, HiddenValueCallback) are submitted to the module unfilled. Before this change, such a set failed closed with 400.

Example: a custom module presents [HttpCallback, PasswordCallback] (a header token plus a PIN/OTP). A request with Authorization but without X-OpenAM-Password used to get 400. Now the module runs with a null password, which gives an NPE/500 or an accidental pass, depending on how the module treats a missing value. ChoiceCallback and ConfirmationCallback likewise arrive with no selection, or with the default one.

A narrower fix for #1170 would treat only output-only callbacks (TextOutputCallback / ScriptTextOutputCallback, NameValueOutputCallback) as satisfied next to a filled HttpCallback, and keep returning false for unfilled input callbacks.

[maximthomas]

Changed to the narrower rule in e6cc8ad. Next to an HttpCallback, only TextOutputCallback (which covers ScriptTextOutputCallback) and NameValueOutputCallback count as filled. An input callback that cannot be filled from the request still returns false, so [HttpCallback, PasswordCallback] with only Authorization gets the 400 from convertToJson, as on master. PollingWaitCallback and RedirectCallback stay outside the rule: both expect a round trip with the client.

[vharseko]

Nit: this branch is unreachable. RestAuthHttpCallbackHandler.updateCallbackFromRequest either throws the 401 RestAuthResponseException or returns true, never false. If it ever did return false, the caller would go down the convertToJson path, which always answers 400 for an HttpCallback, and so would rebuild #1170. A plain call without the return check would state the contract.

[maximthomas]

Removed in e6cc8ad together with handleHttpCallbacksInternally. The HttpCallback now goes through the same loop as every other callback, so no HttpCallback-specific branch is left.

[vharseko]

Nit: the detection loop plus the two loops in handleHttpCallbacksInternally make three passes over the array, and the per-callback dispatch now lives in two methods that have to stay in sync. One pass would do the same:

boolean hasHttpCallback = Arrays.stream(callbacks).anyMatch(HttpCallback.class::isInstance);
for (Callback callback : callbacks) {
    RestAuthCallbackHandler handler =
            restAuthCallbackHandlerFactory.getRestAuthCallbackHandler(callback.getClass());
    if (!handler.updateCallbackFromRequest(request, response, callback) && !hasHttpCallback) {
        return false;
    }
}

The 401 still fires wherever the HttpCallback sits, because its handler throws. If you take the narrower rule from the comment above, the condition becomes "&& !(hasHttpCallback && isOutputOnly(callback))".

[maximthomas]

Done in e6cc8ad with the narrower condition, !(hasHttpCallback && isOutputOnly(callback)).

One consequence: the 401 now fires wherever the HttpCallback sits only when the callbacks before it are filled or output-only. An unfilled input callback before it returns false first and ends in the 400, which is what master does for that set. Sets without an HttpCallback take the same early return as before; the six existing cases, including the never() in shouldHandleCallbacksIntoJson, pass unchanged.

[vharseko]

Nit: @return Whether or not the Callbacks were successfully updated doesn't hold: the method returns true even when callbacks other than the HttpCallback were left unfilled. A reader taking true to mean "every callback was filled" would be misled.

[maximthomas]

The method is gone in e6cc8ad. The Javadoc of handleCallbacksInternally now states that output-only callbacks next to an HttpCallback count as updated and that input callbacks still have to be filled from the request.

[vharseko]

No case covers an input callback that cannot be filled next to an HttpCallback, e.g. [HttpCallback, PasswordCallback] with only Authorization set. Whichever behaviour is intended (400 / JSON, or submit with a null password), a test pinning it would keep a later refactor from flipping it silently.

[maximthomas]

Added shouldRejectHttpCallbackNextToInputCallbackThatCannotBeFilled in e6cc8ad: [HttpCallback, PasswordCallback] with only Authorization gets a RestAuthException with 400, and the password stays unset. It fails on the previous head 34e3959.

I also added shouldSubmitNameValueOutputCallbackWhenHttpCallbackHeaderIsPresent, so each type in the output-only check is pinned. Removing the exemption, removing either type, or exempting every callback (the previous head) each turns a case red. mvn -o -pl openam-core-rest test: 446 run, 0 failures.

[vharseko]

await resp.text() is evaluated on success as well, after the XUI may already have navigated to the success URL. Chromium can drop the previous document's response bodies on navigation, and then resp.text() throws Network.getResponseBody: No resource with given identifier found. The test fails even though Kerberos SSO worked. The old 400 assertion never navigated, so it could not hit this. Reading the body only on failure avoids it:

if (resp.status() !== 200) {
  throw new Error(`/authenticate answered ${resp.status()}: ${await resp.text()}`);
}

[maximthomas]

Done in 81cbe13: the body is read only when the status is not 200.

[vharseko]

Thanks, all six points from the first round are addressed in e6cc8ad and 81cbe13.

• Unfilled input callbacks next to an HttpCallback: fixed with the narrower rule. Only TextOutputCallback (and so ScriptTextOutputCallback) and NameValueOutputCallback count as filled. [HttpCallback, PasswordCallback] with only Authorization ends in the 400 again, as on master. A GET still returns false from the abstract handler before the exemption applies, so it also keeps master's behaviour. Leaving out PollingWaitCallback and RedirectCallback makes sense: both need a round trip with the client.
• Unreachable false branch, three passes, @return wording: handleHttpCallbacksInternally is gone. The HttpCallback goes through the common loop, and the Javadoc states the rule.
• Test for an unfilled input callback: shouldRejectHttpCallbackNextToInputCallbackThatCannotBeFilled is added, plus a NameValueOutputCallback case.
• e2e resp.text() after navigation: the body is now read only on failure.

I checked the test claims locally with RestAuthCallbackHandlerManagerTest (11 run):

Manager variant	Red cases
PR head	none
master's loop (no exemption)	the four new submit/challenge/fill cases
TextOutputCallback removed from isOutputOnly	3 (script, challenge, fill)
NameValueOutputCallback removed	1 (shouldSubmitNameValueOutputCallback…)
every callback exempted / previous head 34e3959	1 (shouldRejectHttpCallbackNextToInputCallback…)

Each part of the condition is pinned by at least one case. Nothing further from me. LGTM once CI is green.