-
Notifications
You must be signed in to change notification settings - Fork 68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a specific exception when the clientId is empty #73
Add a specific exception when the clientId is empty #73
Conversation
It wasn't obvious that my config wasn't being initialized and this would definitely give me a heads up that something is wrong |
src/Duende.AccessTokenManagement/ClientCredentialsTokenManagementBuilder.cs
Outdated
Show resolved
Hide resolved
src/Duende.AccessTokenManagement/ClientCredentialsTokenManagementBuilder.cs
Outdated
Show resolved
Hide resolved
I think we can simplify this code by not putting the check into the post configure action, so I went ahead made that change here. @brockallen do you mind taking a look? |
I regret not double-checking the changes @josephdecock made before the PR was merged, because they have the effect of negating the fix for the scenario that led to the PR being submitted. |
Okay - well the good news is that there is still time to revisit this before the 3.0.0 release. With the post configure, we were getting a possible null reference compiler warning in the service. The post configure should prevent that from being null, but I wanted to at least resolve the warning. I thought it was perhaps easier to reason about the code with all the checks in one place, but that is a pretty small consideration here. I did also change the error message so that it would include the client name, hoping that would make things a bit easier to debug as well. But I suppose there are a bunch of cases where the error message would be confusing (some of these seem more likely than others to me, but still):
|
What issue does this PR address?
The exception caused by an empty or null clientid is confusingly labeled "unknown client", which provides the impression that the named client hasn't been configured rather than that it was misconfigured. It is common for the client options to come from another dependency-injected object like an Option object, and this being misconfigured needs to be clearly distinguished from the client itself not being configured at all.
Note that the added test passes without the changed behavior, because Shouldly doesn't seem to actually compare the exception message with the expected value. Would appreciate a look at this from someone more familiar with Shouldly.
Important: Any code or remarks in your Pull Request are under the following terms:
If You provide us with any comments, bug reports, feedback, enhancements, or modifications proposed or suggested by You for the Software, such Feedback is provided on a non-confidential basis (notwithstanding any notice to the contrary You may include in any accompanying communication), and Licensor shall have the right to use such Feedback at its discretion, including, but not limited to the incorporation of such suggested changes into the Software. You hereby grant Licensor a perpetual, irrevocable, transferable, sublicensable, nonexclusive license under all rights necessary to incorporate and use your Feedback for any purpose, including to make and sell any products and services.
(see our license, section 7)