Throw exception that was created but not thrown

[ThomasVitale]

Fixes #5462

In the DigestAuthenticationFilter, a BadCredentialsException was created when the nonce signature is invalid, but never thrown.

This case is already covered by a test (testNonceWithIncorrectSignatureForNumericFieldReturnsForbidden in the DigestAuthenticationFilterTests class), but it was green since an exception was thrown anyway at a later point of the filter, resulting in a 401 status (line 217).

spring-security/web/src/main/java/org/springframework/security/web/authentication/www/DigestAuthenticationFilter.java

Line 217 in ad9dc49

fail(request, response,

)

[jzheaux]

@ThomasVitale good catch! I see your comment about an existing test.

Is there a test that you could add which would correctly fail before your change, but would pass afterward? It'd be nice to add that as part of this PR.

[ThomasVitale]

I thought about that, but I couldn't find a reasonable way to do it. The filter implementation can throw a BadCredentialsException for different reasons, but from the outside we are not aware of that since the filter catches the exception internally and returns a 401 response. And that is what is tested in DigestAuthenticationFilterTests. Do you have any suggestion? I would really much like to improve this PR with a better test.

[jzheaux]

Yeah, this is a bit of a tricky one. And digest auth is pretty complex on its own.

Thinking out loud here a bit, what we'd need is for this check:

String expectedNonceSignature = 
		DigestAuthUtils.md5Hex(this.nonceExpiryTime + ":" + entryPointKey);

if (!expectedNonceSignature.equals(nonceTokens[1])) {

to fail, but not any other check.

So, if the test's nonce were generated with something other than the configured entryPointKey, then I think that check would fail while the others would pass. Your test, for example, could call generateNonce, but not use KEY:

@Test
public void test() throws Exception {
	String badNonce = generateNonce(60, "badkey");
	String responseDigest = DigestAuthUtils.generateDigest(false, USERNAME, REALM,
			PASSWORD, "GET", REQUEST_URI, QOP, badNonce, NC, CNONCE);

	request.addHeader("Authorization", createAuthorizationHeader(USERNAME, REALM,
			badNonce, REQUEST_URI, responseDigest, QOP, NC, CNONCE));

	// ...
}

[jzheaux]

Thanks again, @ThomasVitale, for the contribution! This is now merged into master via d88c2c1. I also added a unit test via 20a7bc4.