fix: use context to fetch key set in userinfo

[zepatrik]

Currently context.Background() is used although a context would be available.

I'd also propose to deprecate the (*Provider).Verifier method in favor of (*Provider).VerifierContext, but haven't included that in this PR.

[zepatrik]

Using an RWMutex instead, because we only do reads after the initial write. This should eliminate any lock contention.

[zepatrik]

If this function would be eventually removed, there would also not be any need for the Provider.client anymore.

[ericchiang]

Since #433 the remote key set ignores context cancelation. In hindsight, it was a really bad idea for this package to mirror golang.org/x/oauth2 and use context values for configuration, but that's the API we're stuck with now. For example, VerifierContext is now documented that the provided context cancelation is ignored.

What's the goal here with updating this?

On a general note, I'm a little confused why userinfo is used so widely, particularly with the JWT signatures. Can't that information more easily be fetched from the ID Token and skip a round trip?

[zepatrik]

I was determining how we can best implement caching in a multi-tenant application where we get rate-limited on the /.well-known/openid-configuration endpoint.
It first raised concerns that there is a reference to the initial client that is passed around, which can cause issues in multi-tenant reuse scenarios. While reviewing the usage of that, I figured that this is a bug in the userinfo implementation where the passed-in client (through context) doesn't actually get used to fetch the JWKs. I don't think we'd be running into this, as we're using the token instead. However, I did want to fix this obvious bug now that I found it.

[ericchiang]

So, currently the remote key set is intended as a background process, since it many-to-ones individual requests to verify a token with a single upstream request to refresh keys. The background context lives longer than any one request. Therefore I don't actually think that use of context background is a bug.

In the alternate setup where a request's context is used, one requests attempt to verify a key could impact another, which doesn't seem right.

I appreciate you raising this! But, there's been a lot of discussion about the use of contexts in this package over the years, and I'm inclined to stop changing them around without strong reason. It's not ideal, but seems to work for most users today.

For the caching, that seems like you'd want to cache provider structs rather than recreating them constantly? That seems independent from this issue here.

[zepatrik]

To be clear, this is not about context cancellation propagation. The context also holds the client, and I think it is a bug to use the provider's stored client when another one is available. I have now changed the signature of the internal function to make it clear that we only want to propagate the client.