Skip to content
This repository has been archived by the owner on Nov 19, 2024. It is now read-only.

Add a specific exception when the clientId is empty #73

Merged
merged 5 commits into from
May 6, 2024

Conversation

kallayj
Copy link
Contributor

@kallayj kallayj commented Mar 14, 2024

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)

@insylogo
Copy link

It wasn't obvious that my config wasn't being initialized and this would definitely give me a heads up that something is wrong

@brockallen brockallen requested a review from josephdecock March 14, 2024 02:26
@josephdecock
Copy link
Member

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?

@josephdecock josephdecock requested a review from brockallen May 6, 2024 02:51
@josephdecock josephdecock merged commit 5c41c64 into DuendeSoftware:main May 6, 2024
5 checks passed
@josephdecock josephdecock changed the title Adds a specific exception when the clientId is empty. Add a specific exception when the clientId is empty. May 6, 2024
@josephdecock josephdecock changed the title Add a specific exception when the clientId is empty. Add a specific exception when the clientId is empty May 6, 2024
@kallayj
Copy link
Contributor Author

kallayj commented May 9, 2024

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.
Both the old and new behavior is to conflate missing configuration values with no client having been configured at all. It is good that the assumption that multiple missing values indicate nothing was configured is explicitly stated in the code, but the assumption is incorrect :-)
Catching misconfiguration is what PostConfigure is for, and so I still think this was the right way to implement the fix.

@josephdecock
Copy link
Member

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):

"clientToDoLater": {
}
"clientWithLotsOfTypso": {
  "clienttID": "foo",
  "tokenEdntpoint": "bar"
}
"clientWithSnakeCaseProperties": {
   "client_id": "foo",
   "token_endpoint": "bar"
}

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants