Please add some log or re-throw in case of exception about identitymodel

[leastprivilege]

This library tries to minimize dependencies, e.g. on a logging infrastructure.

Do the .Error or .Exception properties don't give any further information?

[kiranchandran]

Hi @leastprivilege,

I get this issue only when I install this excel add-in. I have nothing more than the above error message which I got from the event logger.

Instead of suppressing the exception we could have retuned the error in the Protocol response as an additional property or could even throw this right? Now we have no clue what's happening, it could be a dll not found or a dll version mismatch etc.

[leastprivilege]

You always need to check the IsError and Error properties to find out if any protocol errors occurred. This is inline with the design of HttpClient.

[kiranchandran]

Hi @leastprivilege we are not using the IdentityModel directly, we are using IdentityModel.OidcClient which uses this as a dependency. Either of these framework is not throwing the actual exception and suppressing it.

I am trying to find out how we can get the actual exception. I am not sure why we suppress the error and do you think catch without a log or re-throw is a better coding practice?

[leastprivilege]

I am not sure why we suppress the error and do you think catch without a log or re-throw is a better coding practice?

Me neither. And some exceptions should probably be re-thrown to be inline with HttpClient behavior.

But right now it is the way it is. Feel free to do an analysis what excactly would need to change and propose as a draft PR.

[leastprivilege]

@josephdecock Right now we always "swallow" all exception and surface them on the .Exception property. Should some exception bubble up to be more inline with standard HttpClient behavior? Opinions?

[josephdecock]

I don't think that we should change our error handling, since

1. it would be a breaking change that would only surface at run-time
2. you'd have two different error checking code paths:

try
{
   var response = await client.RequestClientCredentialsTokenAsync(...);
   if(response.IsError)
   {
       // Handle errors once
   }
}
catch
{
   // Handle errors twice
}

I think we should just be consistent, and change the two catch { } blocks to set the error properties in the response. (I just searched for all our catches, and there are only 2 that don't set error properties, both in ProtocolResponse).

[josephdecock]

I've looked into this a bit more, and here are my thoughts about the 2 places where IdentityModel has a catch to suppress errors.

• If we get a non successful http response, the body of the http response could be json or not. We always try to parse the body as json, and if parsing succeeds, we use it to populate the ProtocolResponse.Json. I think suppressing an exception here is sensible, because non-json will throw but it is not unusual (and explicitly something we allow) to have errors not return json. IOW, we make a best effort to provide parsed json, if that was returned, but it is not an error if that doesn't work.
• On the other hand, if reading the http response's content fails, I think we should be capturing error details that today we do not.

But regardless of that, the actual exception is coming out of IdentityModel.OidcClient, which does actually have logging dependencies. The code that is throwing is here which you can see has many error checks that all log the specifics before throwing, so I would think that if you start writing the oidc client logs, you'll get more useful details.

[josephdecock]

I also notice that in this block, httpResponse could be null:

        try
        {
            content = await httpResponse.Content.ReadAsStringAsync().ConfigureAwait();
            response.Raw = content;
        }
        catch { }

This is the problem in #532, and I'm working on making the HttpResponse nullable in its type (and handling nulls).