Consider logging missing code_verifier when code_challenge is included in authorization request

[sjohnr]

spring-authorization-server/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/authentication/CodeVerifierAuthenticator.java

Lines 133 to 134 in 27a893f

	if (!StringUtils.hasText(codeVerifier)) {
	return false;

We should consider adding a log entry at DEBUG level in CodeVerifierAuthenticator for this case. This would allow the logging level to be tuned specifically for this logging.

[dciarniello]

I'm wondering why DEBUG and not INFO or WARN, something more appropriate for Production, in other words.

[sjohnr]

@dciarniello we want to be very conservative about what shows up at INFO or WARN because these logs would show up for most deployments by default, which potentially could be very noisy. I'd rather be asked to turn up logging then turn it down. Instead, these logs are in their own classes at DEBUG so you can opt-in to whichever you want by enabling DEBUG for that particular class (logger) only. Does that make sense?

[dciarniello]

@sjohnr , Yes, the reasoning does make sense as there's issue of having enough information in the logs to troubleshoot issue versus having logs that are so noisy that it actually impedes troubleshooting. In this particular case, however, I would assume that the errors would be infrequent enough that they would not generate a lot of noise so I would lean towards logging them at a higher level.

DEBUG is not something that we would generally turn on in production exactly because of the potential for increased noise. Having said that, I have not examined the code to see if turning on DEBUG for the classes in question would be excessively noisy.

[sjohnr]

Hi @dciarniello!

In this particular case, however, I would assume that the errors would be infrequent enough that they would not generate a lot of noise so I would lean towards logging them at a higher level.

I hear you, and I agree in general. However, consider a provider built using SAS that has 100,000 registered clients and allows free developer trials, which frequently have invalid configurations pointed at it. This would not be ideal for such a provider. While I can't say how likely that is, the goal here is to provide the option (tuning DEBUG for the classes in question) for those that need it, but not impacting those that don't.

If the out-of-the-box logging story does not suit you, there are options available to add your own, and I think having certain errors at INFO level would qualify as a customization need. If you find it is impossible to customize as you need, feel free to open another issue with details and we can take a deeper look.

DEBUG is not something that we would generally turn on in production exactly because of the potential for increased noise. Having said that, I have not examined the code to see if turning on DEBUG for the classes in question would be excessively noisy.

This is exactly why we placed the DEBUG logs in the classes where the relevant error occurs instead of in the Filter implementation. I believe it should suit you, but please let me know if you have issues I didn't foresee.

[dciarniello]

Hi, @sjohnr
100,000 clients isn't something we deal with so I hadn't considered that :-)

I've now looked at the code and I believe that the change will, indeed, work for us. I'll let you know if it doesn't after we've had a chance to try it out.

Thanks!