Wrapping the exception to provide better feedback to the user; fixes …#4606
Conversation
There was a problem hiding this comment.
Pull request overview
This PR attempts to address issue #4592 by wrapping JWT license validation in exception handling to provide better user feedback. The changes add a try-catch block around the ValidateKey method to catch platform-specific exceptions when RSA cryptography is not supported (e.g., in Blazor WASM scenarios).
Changes:
- Added try-catch block around JWT validation logic in ValidateKey method
- Added specific handling for PlatformNotSupportedException with informative logging
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| catch (PlatformNotSupportedException) | ||
| { | ||
| _logger.LogCritical(validateResult.Exception, "Error validating the Lucky Penny software license key"); | ||
| _logger.LogInformation( | ||
| "RSA cryptography is not supported on this platform. " + | ||
| "For client redistribution scenarios such as Blazor WASM, see: " + | ||
| "https://docs.automapper.io/en/latest/License-configuration.html#client-redistribution-scenarios"); | ||
| return []; | ||
| } |
There was a problem hiding this comment.
The PR description states it fixes issue #4592 which asks for better exception messages for JWT validation errors (specifically ArgumentException and FormatException like "IDX14101: Unable to decode the payload"). However, the code changes only add a try-catch for PlatformNotSupportedException, which is a different scenario (unsupported cryptography on certain platforms like Blazor WASM). The original issue's JWT decoding errors would still not be caught or given better messages by this implementation.
| "For client redistribution scenarios such as Blazor WASM, see: " + | ||
| "https://docs.automapper.io/en/latest/License-configuration.html#client-redistribution-scenarios"); | ||
| return []; | ||
| } |
There was a problem hiding this comment.
The PR description states this fixes issue #4592, which mentions catching exceptions like ArgumentException and FormatException during JWT validation to provide better error messages. However, the current implementation only catches PlatformNotSupportedException. The ArgumentException and FormatException shown in the issue (IDX14101, IDX10400) would not be caught by this try-catch block. Consider adding a catch block for these exceptions (or a more general exception type) that logs a user-friendly message like "The provided license key is invalid or malformed. Please check that you have copied the entire license key correctly."
| } | |
| } | |
| catch (ArgumentException ex) | |
| { | |
| _logger.LogWarning(ex, | |
| "The provided license key is invalid or malformed. Please check that you have copied the entire license key correctly."); | |
| return []; | |
| } | |
| catch (FormatException ex) | |
| { | |
| _logger.LogWarning(ex, | |
| "The provided license key is invalid or malformed. Please check that you have copied the entire license key correctly."); | |
| return []; | |
| } |
| ValidateLifetime = false | ||
| }; | ||
|
|
||
| var validateResult = handler.ValidateTokenAsync(licenseKey, parms).Result; |
There was a problem hiding this comment.
Using .Result on an async method can cause deadlocks in certain synchronization contexts and wraps exceptions in AggregateException. Since the handler.ValidateTokenAsync call might throw exceptions synchronously (before returning a Task), those exceptions may be wrapped in AggregateException when accessing .Result. This could complicate exception handling. Consider using GetAwaiter().GetResult() instead, which unwraps the exception, or making this method async and using await.
| var validateResult = handler.ValidateTokenAsync(licenseKey, parms).Result; | |
| var validateResult = handler.ValidateTokenAsync(licenseKey, parms).GetAwaiter().GetResult(); |
jbogard
deleted the
4592-provide-a-better-exception-message-for-errors-validating-jwt
branch
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. |
…#4592