IdentityServer
IdentityServer copied to clipboard
DuendeSoftware
Add logging when DistributedCacheStateDataFormatter returns null
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?
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".
Yes, Microsoft should definitely handle this better. Is it better to send a PR to the OIDC handler?
Over here please: https://github.com/dotnet/aspnetcore
FYI, I don't work on AspNetCore anymore.
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?
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.
Yes, closing for now. If support indicates that people are still confused by this even with the new logging we can reopen.