Fail fast when UserDetailsService returns null in onLoginSuccess

[skdas20]

When the successful Authentication carries no credentials, onLoginSuccess falls back to looking the password up through the configured UserDetailsService, but dereferences the result without checking it:

UserDetails user = getUserDetailsService().loadUserByUsername(username);
password = user.getPassword();

A UserDetailsService that returns null rather than throwing UsernameNotFoundException therefore shows up as a bare NullPointerException from inside the remember-me filter, with nothing in the stack trace pointing at the service that is actually misconfigured.

processAutoLoginCookie in this same class already guards the identical call, and is explicit about what a null return means:

UserDetails userDetails = getUserDetailsService().loadUserByUsername(cookieTokens[0]);
Assert.notNull(userDetails, () -> "UserDetailsService " + getUserDetailsService()
        + " returned null for username " + cookieTokens[0] + ". "
        + "This is an interface contract violation");

autoLoginClearsCookieIfUserServiceMisconfigured pins that behaviour in the tests. This change applies the same guard to onLoginSuccess so both lookups report the misconfiguration the same way, and adds loginSuccessFailsIfUserServiceMisconfigured alongside the existing loginSuccess tests. Without the production change that test fails with the reported NullPointerException.

One note on the approach: gh-19535 suggests skipping cookie generation instead, with

password = (user == null) ? password : user.getPassword();

I went with the existing in-class precedent rather than that, because silently continuing would quietly swallow a misconfiguration that the class elsewhere treats as a contract violation — and the user would get no remember-me cookie with no indication why. Happy to switch to the lenient form if you'd rather keep onLoginSuccess tolerant; it's a small change either way.

Closes gh-19535

[skylarsutton]

I guess my objection to this strategy is that, while it does conform to the guards found in processAutoLoginCookie it does not conform to the behavior within the rest of the method (onLoginSuccess).

Two lines above your proposed change, if the username cannot be obtained it does not fail with an assertion error, it simply returns without generating a cookie:

if (!StringUtils.hasLength(username)) {
    this.logger.debug("Unable to retrieve username");
    return;
}

Two lines below your proposed change, if the user can be obtained but has no password, it does not fail with an assertion error, it ALSO simply returns without generating a cookie:

password = user.getPassword();
if (!StringUtils.hasLength(password)) {
    this.logger.debug("Unable to obtain password for user: " + username);
    return;
}

IMHO - the implication is that onLoginSuccess intentionally allows nulls, and nulls mean "do not generate a cookie".

[skdas20]

@skylarsutton that's a fair point and I've switched to it — thanks for pushing back.

You're right that the local pattern is the more relevant one. I anchored on processAutoLoginCookie because it guards the same call, but that's a different method with a different job: it's validating an untrusted cookie, where a null user genuinely is a broken configuration worth surfacing. onLoginSuccess is on the happy path after a successful authentication, and both of its neighbouring guards treat "couldn't get what I need" as a reason to skip the cookie rather than fail. Throwing there would also mean a misconfigured UserDetailsService breaks login itself, not just remember-me, which is a worse outcome than silently not issuing a cookie.

Now reads:

UserDetails user = getUserDetailsService().loadUserByUsername(username);
if (user == null) {
    this.logger.debug("Unable to obtain user details for user: " + username);
    return;
}
password = user.getPassword();

The test asserts no remember-me cookie is written rather than expecting an exception, and the commit message has been updated to match. Full class is green (26/26).