panva / openid-client

OAuth 2 / OpenID Connect Client API for JavaScript Runtimes
MIT License
1.83k stars 392 forks source link

Log runtime values when authorization fails #165

Closed mgwidmann closed 5 years ago

mgwidmann commented 5 years ago

Is your feature request related to a problem? Please describe. When authorization fails, it is difficult to diagnose what the problem is.

Describe the solution you'd like When authorization fails, I would like to see the runtime values which failed to match.

Describe alternatives you've considered In order to debug my issue, I had to modify the code of this library.

Additional context Given an improperly setup identity provider or an improper client configuration, it is not easy to debug which side of the line the problem is on.

Example

Within the validateIdToken function

Checks like the following:

if (payload.iss !== undefined) {
  assert.equal(payload.iss, this.issuer.issuer, 'unexpected iss value');
}

Logging 'unexpected iss value' is not very useful when you don't know what the values are. Instead this library should do something like the following:

if (payload.iss !== undefined) {
  assert.equal(payload.iss, this.issuer.issuer, 'unexpected iss value: ' + payload.iss + ' (payload) vs. ' + this.issuer.issuer + ' (expected issuer)');
}

So instead users see unexpected iss value: https://accounts.google.com (payload) vs. https://accounts.google.com/misconfiguration (expected issuer). In my case, it was actually the reverse, the identity provider is providing one value for the payload and a separate value for the issuer (via discovery) making it impossible to authenticate. However, without this log it was impossible to tell if it was the client or the provider.

This should happen for all checks within the validateIdToken method. This way when something in production happens, all information is delivered at the time of failure and a repeat request with modifications like this is not necessary.

panva commented 5 years ago

I believe properly inspecting the assertion error you get the promise rejected with is also an option, wouldn't you say so?

AssertionError [ERR_ASSERTION]: nonce mismatch
    at Client.validateIdToken (/Users/panva/repo/client/lib/client.js:602:14) {
  code: 'ERR_ASSERTION',
  actual: 'nonce!!!',
  expected: 'foo'

AssertionError [ERR_ASSERTION]: unexpected iss value
    at Client.validateIdToken (/Users/panva/repo/client/lib/client.js:578:16) {
  code: 'ERR_ASSERTION',
  actual: 'https://login.microsoftonline.com/foo/v2.0',
  expected: 'https://login.microsoftonline.com/{tenantid}/v2.0'
}
mgwidmann commented 5 years ago

Hmm, ok well thats not how it was unpacked for me when it was logged out in lambda. Perhaps it happened that way because I was using https://github.com/auth0/express-openid-connect ?

panva commented 5 years ago

I'll see what i can do for v3.x until then the assertion errors contain the details necessary to understand the problem. If a consuming library doesn't forward the details in full, that's a thing to pick up with said library. That being said, v3.x will contain an entirely unique classes for OP errors and RP assertion errors.