-
Notifications
You must be signed in to change notification settings - Fork 8
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
Find out why DoHealthCheck() Fails #24
Comments
@JoelDKraft We got the same request from an internal Duo team that uses this client, which led to 1d94501 which is not in a release yet. Does that change look like it would suit your use case? If so, I can work on getting a release out soon. |
That takes care of half of what I described, which is being able to determine if there was an HTTP exception thrown by the API call. If there is no exception, but the API returns FAIL, that is information is still not available. Now that you've added that parameter, maybe it makes sense to throw a DuoException when the health check comes back negative, as well?
|
@JoelDKraft thanks for the clarification. I'll look into it. |
I'm going to throw one more comment in here: the same type of thing happens in DoPost. The following line throws an exception if the call does not return a 200 code... So in my case, I got the following exception: The problem here is that the API actually returned a response with an error message, and I really needed to know that response to figure out what was going wrong. But this blanket exception again removed any chance of me seeing that underlying error. Replacing it with something like this would let you actually see the API response. I was able to get this server response, and now I know what is wrong: I would probably not just dump in the raw JSON, but you get the idea... |
Detailed Description
I have been working with this new API for a few days. It is pretty straightforward, but DoHealthCheck() kept returning false for an integration that we are attempting to migrate. I knew that Duo was "available" as a service, but we could not figure out why it was failing, because there is no mechanism in the Client class to retrieve the result.
ExchangeAuthorizationCodeFor2faResult() will throw all sorts of DuoExceptions, but DoHealthCheck() only looks at the "OK" status, and ignores any error message. Even an HttpRequestException exception (which could mean the API URL is incorrect) is hidden by this function.
Use Case
In my case, we were able to find out that the error was "invalid_client: Integration type does not support frameless access". The solution is easier once we had the error.
I don't know what the best solution is here. The way these two functions are designed is completely different, so I assume it was intentional that DoHealthCheck() not throw an exception. It could be that there are some serious problems like this that should still throw an exception, or the exception can be saved by the Client and we could check it if DoHealthCheck() returns false.
Maybe a different version of DoHealthCheck() that returns the HealthCheckResult object would be the easiest to implement without breaking anything. Those of us that want details can use the new function.
Workarounds
For our testing, we built the DLL from source and included an exception if the HealthCheckResult was not OK.
The text was updated successfully, but these errors were encountered: