Add logging when DistributedCacheStateDataFormatter returns null

[AndersAbel]

If the data is not found in the cache we return null which is perfectly fine. However, the OIDC Handler logs the error as "Unable to unprotect message.state" which indicates it's a data protection issue. We should add a log message when we return null, something like

Key {key} not found in the registered IDistributedCache. Returning null which will cause an "Unable to UnProtect message.state" exception.

With that line in the log right before the exception it would be easier to understand the reason for the error.

Only question I have is the log level. It feels like Debug level of detail. But I also want it to show up in logs right next to the exception which indicates error or warning.

Maybe Error and a config switch on the state data protector that can disable the logs for those who has found out how it works?

[brockallen]

Is there something that the MS code could do better here? They should report a different thing, it seems -- IOW, there's "no data to unprotect" vs. "the unprotecting of the data found has failed".

[AndersAbel]

Yes, Microsoft should definitely handle this better. Is it better to send a PR to the OIDC handler?

[brockallen]

Well, we should start by opening an issue with them. // @Tratcher

[Tratcher]

Over here please: https://github.com/dotnet/aspnetcore

FYI, I don't work on AspNetCore anymore.

[brockallen]

Over here please: https://github.com/dotnet/aspnetcore

Thank you!

FYI, I don't work on AspNetCore anymore.

Ah, what's your focus these days?

[AndersAbel]

Yes, Microsoft should definitely handle this better. Is it better to send a PR to the OIDC handler?

I've investigated possibilities for better handling and unfortunately the current interfaces/function signatures do not allow any improved error handling. The returned value need to be a valid AuthenticationProperties instance or null.

We could add logging every time we return null because the key wasn't found in cache. But it could also be that just a startup message will help enough, #1550.

My suggestions is to put this on hold for the time being, merge #1550 and see if that is enough.

[brockallen]

So should we close this now, given the PR is merged?

[AndersAbel]

Yes, closing for now. If support indicates that people are still confused by this even with the new logging we can reopen.