brokenAuthHeaderProviders mechanism insufficient for Salesforce

[ohler]

Turns out that URLs of the form https://*.force.com/ and https://*.*.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.

[rakyll]

You can register lazily, providerAuthHeaderWorks is called during token retrieval. What we can improve is to make RegisterBrokenAuthHeaderProvider safe for concurrent use.

[ohler]

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.

[jbizzle]

(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.

[bradfitz]

Can we just hard-code force.com as broken in the source, rather than require them to be registered?

[jbizzle]

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.

[bradfitz]

Can you give some examples of which should be included and which shouldn't?

Do they always have hostnames ending in .force.com?

[jbizzle]

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.

[ericchiang]

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/

[borisroman]

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