oidc: add public error type for missing user info endpoint

[raggi]

Some providers are missing this endpoint, it's useful to have a type to match on rather than the error string.

[ericchiang]

see #373 (comment)

would that satisfy this issue without a new API?

[raggi]

Oh, as you note over there the Claims naming is a little odd, but I see.

The addition of the error type burdens the API very little - it changes no existing call sites contractual behaviors (it's still an error type return, with .Error() returning the same string), it just enables the use of errors.Is with a well-known value.

A slightly larger change I'd be tempted to add an .ProviderConfig() API to *Provider, that would effectively round-trip from ProviderConfig{...}.NewProvider(), as that's already a defined type in the API with a fairly known burden.

We could certainly use the claims strategy, though it seems it's more expensive than checking the error string due to needing to re-parse all of the JSON each time. I think given the efficiency consideration the error would be preferable - that is, even if we didn't get a strongly typed error, string matching the error might be preferable for efficiency reasons.

[ericchiang]

Since we've already parsed the entire JSON discovery document, I'm happy to add an API like:

func (p *Provider) UserInfoEndpoint() string

I'd much rather callers figure out if the user_info_endpoint was provided before calling UserInfo. That makes it clear that it's a config check rather than something that might result in a full HTTP request. You can also do it without performing the OAuth2 token dance.

[raggi]

Happy to type that one, curious what you think about round-tripping the ProviderConfig as I mentioned in the middle, which provides mostly the same, but covering potentially a few more use cases?

[ericchiang]

what you think about round-tripping the ProviderConfig

In my head, the way you'd do that would be:

p, err := oidc.NewProvider(ctx, "https://example.com")
if err != nil {
    // ...
}
var claims struct {
    // Things I'm interested in.
}
if err := p.Claims(&claims); err != nil {
    // ...
}

If that gives you the right data, I don't think we'd want two ways to do the same thing.

If that's not efficient enough, we could potentially defer certain parsing operations. Though I'd assume a profile would be dominated by the actual HTTP request.

[raggi]

This is what I had in mind for ProviderConfig: 47cb207

Happy either way, I can send an PR for just the UserInfoEndpoint() too/alternatively.

[ericchiang]

This is what I had in mind for ProviderConfig: 47cb207

Yeah. My issue is that this is what the Claims() method is for. ProviderConfig() also wouldn't support values we don't track in this library, which is one of the main use cases for Claims().

Again, I'm skeptical that the performance of Claims() matters given that calling NewProvider() does a full HTTP request and parses the response body.

Happy either way, I can send an PR for just the UserInfoEndpoint() too/alternatively.

Works for me! Let's start there since it's helpful for anyone using the UserInfo() method, and then if you have other use cases we can follow up later.

[raggi]

Sent #375.

Again, I'm skeptical that the performance of Claims() matters given that calling NewProvider() does a full HTTP request and parses the response body.

Part of the difference in cost is that for most of our requests NewProvider is actually cached with an HTTP cache at the transport, so you're right that it still makes a fair share of garbage, it's most of the time not actually a network round trip, but we are regularly re-parsing that JSON sadly.

Thanks for all the discussion!