Closed ohler closed 7 years ago
You can register lazily, providerAuthHeaderWorks is called during token retrieval. What we can improve is to make RegisterBrokenAuthHeaderProvider safe for concurrent use.
If that's the way you want to go, I would recommend making the Register function idempotent (maybe backed by a map instead of a slice of strings), since otherwise the caller needs extra logic (and its own locking) to avoid adding duplicates to the list and growing it without bound.
A flag somewhere in oauth2.Config/Endpoint would be so much simpler, though... I understand the desire to avoid exposing server quirks to users, and the hard-coded list of known-bad servers helps with that; but is there really a reason why RegisterBrokenAuthHeaderProvider – called at runtime with dynamically-computed arguments – is OK and a flag is not? For use cases like this one, mutating some global state instead of passing a flag down the call stack is just unnecessarily contorted. I agree both can work; but one is straightforward, the other is not.
(Including @bradfitz in case he has some historical context to share)
As another option, could we make a change to add something like RegisterBrokenAuthHeaderProviderRegexp() that would allow us to specify a single pattern that would match all of the hosts we need to handle? This would be totally backwards compatible and is still in the spirit of the current API.
Can we just hard-code force.com
as broken in the source, rather than require them to be registered?
Do you mean [brokenAuthHeaderProviders in token.go](https://github.com/golang/oauth2/blob/master/internal/token.go#L92|the list in token.go)? If so I'm not sure if that addresses what we need, since we're trying to match a specific category of wildcard subdomains within force.com, but not force.com itself.
Can you give some examples of which should be included and which shouldn't?
Do they always have hostnames ending in .force.com
?
They do always end in ".force.com". The URLs that we need to deal with look like "ourapp-version.xy.force.com". The "ourapp" is constant, but the "version" and "xy" values change independently, often, and in ways we can't know or control ahead of time.
FYI Okta has the same issue. @curtisallen pointed out in #224 that https://*.okta.com
and https://*.oktapreview.com
should be detected too.
edit: sent https://go-review.googlesource.com/c/38376/
Besides https://*.okta.com
and https://*.oktapreview.com
Okta also hands out OAuth endpoints on https://*.okta-emea.com
.
edit: sent https://go-review.googlesource.com/c/oauth2/+/58510
Turns out that URLs of the form
https://*.force.com/
andhttps://*.*.force.com/
can also host Salesforce OAuth provider endpoints, and they require the brokenAuthHeaderProviders workaround. See http://resources.docs.salesforce.com/198/13/en-us/sfdc/pdf/salesforce_communities_implementation.pdf at the top of page 108.It would be convenient if I could call
RegisterBrokenAuthHeaderProvider
with additional URLs as I encounter them at runtime, but it expects to be called from init() (does no locking).Allowing me to set a flag on oauth2.Config that tells oauth2 to use the workaround would be a much more straightforward solution, though.