Logging and misc improvements #133
Conversation
|
Thanks for the contribution! I'll look a little more closely when I have a moment and provide some feedback. |
| } | ||
|
|
||
| if ( | ||
| pluginStrategy == PLUGIN_STRATEGY_OIDC && |
There was a problem hiding this comment.
This check needs to remain.
There was a problem hiding this comment.
Can you elaborate on this? We use OAuth2 and need it to validate nbf and iss.
| } | ||
|
|
||
| if ( | ||
| pluginStrategy == PLUGIN_STRATEGY_OIDC && |
There was a problem hiding this comment.
This check needs to remain.
| } | ||
|
|
||
| if ( | ||
| pluginStrategy == PLUGIN_STRATEGY_OIDC && |
There was a problem hiding this comment.
This check needs to remain.
There was a problem hiding this comment.
I don't understand this, is id_token assertions not available in OAuth2?
| } | ||
|
|
||
| if ( | ||
| pluginStrategy == PLUGIN_STRATEGY_OIDC && |
There was a problem hiding this comment.
This check needs to remain.
| plugin.config.assertions.access_token | ||
| ) { | ||
| let accessToken; | ||
| accessToken = jwt.decode(tokenSet.access_token); |
There was a problem hiding this comment.
Was this the line that was blowing up previously? Or was it something else?
There was a problem hiding this comment.
Yes, I believe the decode function doesn't handle undefined inputs
| @@ -188,7 +188,7 @@ function get_parent_request_uri(req) { | |||
| originalRequestURI = originalRequestURI.replace(/^(.+?)\/*?$/, "$1"); // remove all trailing slashes | |||
| originalRequestURI += req.headers["x-forwarded-uri"]; | |||
| } else { | |||
| originalRequestURI += req.headers["x-forwarded-uri"]; | |||
| originalRequestURI += req.headers["x-forwarded-uri"] || ''; | |||
There was a problem hiding this comment.
I'll think about this change. It should be handled differently but not sure how exactly :( it probably needs to throw an error if missing as the data is critical to function properly.
There was a problem hiding this comment.
Ok, throwing a hard error is much better than it being undefined as you'll be able to identify the problem quickly.
Hi, we've been playing around with this for a couple of days now and made some improvements you may want. They're mainly concerning logging to allow us to figure out what was happening on our end.
We found that if scope was only "openid offline_access" then of course no access_token is issued. While not a big problem
access_token = nullcausedaccess_token_assertionsto throw an error. I added a check and a log entry in order to handle it gracefully, then did the same for id_token in case someone don't have openid in the scope.Lastly, like we discussed in #131 we got a domain
mydomain.comundefinedbecausex-forwarded-uriwas assumed to have a value. So we added a fallback value as it's better to have no path than mess up the domain.