feat: improve diagnostics, allow custom HTTP options, account for IdP support for scopes #189
Conversation
| try { | ||
| parsed = new URL(url); | ||
| } catch { | ||
| return '<Invalid URL>'; |
There was a problem hiding this comment.
NIT: Would be nice to log.debug? somehow the wrong URL or at least return the invalid url in case there is an issue.
| return '<Invalid URL>'; | |
| return `<Invalid URL: '${url}'>`; |
There was a problem hiding this comment.
@kmruiz Well ... I've gone back and forth on this, the point of this function is to redact URLs so that sensitive parameters like the authorization code don't end up being logged (and consequently stored to disk), so I wanted to err on the side of caution. You're also obviously right that that makes for bad diagnostics, not having the URL available at all :) I'd lean a bit towards leaving it like this, this condition Should Never Happen™ anyway because we should always be working with valid URLs, but let me know if you feel differently
There was a problem hiding this comment.
I merged this for now, but happy to adjust in a follow-up if you have more thoughts on this :)
The commits here are fairly independent (and can be reviewed indepedently), but they would otherwise be fairly small changes or conflict with each other, so this is just one big PR to handle all of them together. If it's easier, I'm happy to split them out and PR them sequentially as well 🙂
chore: add token type to auth-succeeded log event
fix: improve error message when
Issuer.discover()fails COMPASS-7605chore: disable last remaining eslint warning in tests
feat: do not request scopes if the IdP announces lack of support COMPASS-7437
feat: track HTTP calls, allow custom HTTP options MONGOSH-1712
(This would be used to implement
useSystemCAsupport in devtools-connect)