-
Notifications
You must be signed in to change notification settings - Fork 984
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
brokenAuthHeaderProviders - time to try something else? #111
Comments
The exported APIs shouldn't reflect that we have workarounds for buggy implementations. If there are any alternative solutions we can consider without touching the APIs, I would like to hear. A wildcard match might more be useful, why don't we convert the brokenAuthHeaderProviders into a regex list? |
Unfortunately the world is buggy... I'd think the API should reflect whatever is useful to operate in the world as we find it. I don't think a regex list would help particularly - you'd still need a code change to deal with a new site. I can't see why you're happier to make code changes for every site with this bug than to make a single small code change to cope with all of them. |
We're fine with maintaining the list for now. The world may be buggy, but the users don't have to know about it. This package is a higher-level implementation of OAuth 2.0. Most of our users don't even truly know the spec and how OAuth 2.0 works. Exporting an API to turn on/off a workaround is unlikely to be user-friendly. |
I in no way agree with you, but its your project and I'm very grateful for it. Please add [shop].myshopify.com to the list. The host name varies by shop, but no other change is required to get it to work. |
cc/ @bradfitz Brad, do you think it might be worthy to export the busted provider's list, so the users can modify it? Additional to the current bug report, our current solution doesn't help for those who can't upstream their domain names to the list for various reasons such as confidentiality. |
I like the List Of Shame and a simpler API. The latency to add to the List Of Shame should put pressure (directly or indirectly) on servers to comply with specs. |
This is a very common problem sadly.. please add I like the clean API, but being able to append a host to the list at runtime or toggle the authheader manually would make this library usable. |
I think that's a Shame. And one of the most charmingly passive-aggressive attempts to get something fixed I've seen in a long time. Are you lobbying other OAUTH implementations to add a similar hidden list? Or perhaps your scheme is further along than I imagine and every language has an OAUTH implementation with a secret internal list of naughty hosts? That will show them. Unfortunately, for the shorter, unqualified entries, once a service is on the list they can't then fix their implementation without adding a new domain name. Perhaps you should at least ask for prefixes with some amount of path. |
Maybe the community should do something like publicsuffix.org with a shared list of sites which have quirks. I don't know. The Go community values simplicity. This is an experiment to see if we can have both simplicity and still work. It's an ongoing experiment. |
Moving this configuration to a flag in the Endpoint doesn't feel to me less simple than a hard-coded list. I tried it in my own fork (https://github.com/philpearl/oauth2) and it feels quite nice. If the Endpoints are defined in the repo like the github, etc, ones are then the end-user interface is no more complex. |
@bradfitz @rakyll unfortunately, I think it puts more pressure on the people consuming non-compliant servers than on the servers themselves. This is penalising the package users, rather than the servers. With that being said, I understand your point for not adding workarounds for non-compliant APIs, but at least this should be documented and warned in the package. Current usability means developers try to use the package, it fails, and then they dig into the package's source and find the internal whitelist. If we are talking about better developer experience, in my opinion it would be better to clearly document the issue. I would also suggest for dropping the internal whitelist at all since the same non-compliant implementation might work or not depending on an internal whitelist most developers are not aware of. |
Has anybody explored auto-detecting this condition? |
@bradfitz IMHO, auto-detecting the condition would enable people building non-compliant providers. I think it should be very clear when a provider is broken rather than working around those silently. I agree it should be very clear when a provider is broken so we can put some pressure on the server. However, in my opinion OAuth consumers should not be forced to fork the package in order to consume APIs from non-compliant providers. I would suggest:
This way, anybody working around a non-compliant provider can clearly see it's broken and we are not making it easy for them to do such integration. That would make it very clear the provider is broken and might help putting some pressure on the providers. Ideally, the current internal whitelist would be removed and there would be no exception. Unfortunately, that would probably break applications depending on this package. |
@ernesto-jimenez it makes more sense to have a flag on the Endpoint as this behaviour varies with the Endpoint, not a particular configuration using/connecting to the Endpoint |
@philpearl I didn't pay much attention to naming and where in the In my opinion, it might still be better to make integrating non-compliant providers a bit awkward rather than just setting up a flag. Integrating a provider that follows the spec would be seamless, but integrating one that doesn't would feel a bit more wrong and the person integrating it would need to learn what the difference is. |
I can't agree with making something more difficult for no reason other than On Sat, 11 Apr 2015 00:29 Ernesto Jiménez notifications@github.com wrote:
|
@bradfitz, server implementations differ. There is no standard response for the basic-auth-not-supported case. We can't fallback to the query parameters every time we receive a non-200 response from Basic Authorization. |
According to RFC6749 section 2.3.1 page 16
The text then proceeds to give an example of sending the client id and secret in a refresh request. I don't see where using Basic Authentication is required. |
@jfcote87, the first paragraph of that section says:
However, that section is about exchanging a client password for a token. |
The appropriate section describing client authentication for the token endpoint, section 3.2.1, does not require basic authentication (except in the aforementioned scenario):
Section 2.3 says:
Section 2.3.2 says:
This seems to indicate that the current behavior is broken except for |
@tt, how is the current behaviour broken? The package currently uses Basic auth to authenticate with the server. if !bustedAuth {
req.SetBasicAuth(c.ClientID, c.ClientSecret)
} The issue is with the servers that rely on sending the From the Section 2.3.1 you quoted (emphasis mine):
Which menas that servers that require receiving the Section 2.3.2 about "Other Authentication Methods" is there to make the spec extensible and allow other means of authentication such as certificates. However, password authentication is clearly specified. @tt, also, regarding:
How is |
@ernesto-jimenez, you're right. The language confused me. I interpreted the "password" in section 2.3.1 to relate to cases where you exchange a password for a token. |
Yeah, maybe we can let callers do:
So then we can occasionally grep all of Github and find the public ones to add to our central list in batches. That might be a good middle ground. |
I'm happy with that. On 21 May 2015 at 07:20, Brad Fitzpatrick notifications@github.com wrote:
|
Once there's an option to enable the non-compliant providers. Why maintain a central list? In my opinion, the biggest issue with the current implementation is inconsistency. As a developer, I might use this package to integrate with Google or Dropbox, then have to integrate another provider with the same API, but the code would fail because of some internal whitelist I'm not aware of. Same API, same library, same code, different results. |
@ernesto-jimenez, if you're going to be using providers A, B, and C, with the "same" API but different interpretations/buggy implementations of OAuth2, the workaround config has to live somewhere. What I don't want is for that workaround config to be part of the OAuth config in user code. I'd like to flag buggy providers centrally so things work for most people by default, but new buggy sites discovered can be quickly fixed by users while waiting for us to update the central list. Why I opposed adding this to Endpoint earlier was because it brings this too close to the user as a concept they need to be aware of. In general they should not be aware of this. We should do our best to maintain the bad list automatically. |
But we are not talking abut different implementations, are we? After all in many cases the request is to simply add a new provider to the list. Imagine the following situation:
If I integrate provider B, and then provider C fails, I would be very confused. Just my opinion :) Re: Keeping the workaround away from the user's OAuth config. |
You would be confused in that case regardless of whether this package had any knowledge of this workaround, and regardless of whether it exposed an API. It sucks, because people suck at writing and implementing specs. Fact of life. This whole bug is purely about how we handle the confusion and where we shove it. |
On 21 May 2015 at 15:41, Nikolay Turpitko notifications@github.com wrote:
Everything that follows this is way more complicated than it needs to be,
What, why? |
On 22 May 2015 at 03:25, Nikolay Turpitko notifications@github.com wrote:
That sounds like a reasonable move to me. |
Confirmed Github Enterprise 2.1 does not work with Auth Header, and it is not possible to regex github enterprise URL or even It might be better to give user a chance to register |
The auth on Netatmo api need ClientSecret in post request. Like descripted in github issue at #111 Change-Id: Ia85120d231e8a5c0ec851ddc3557bad26ecad41d Reviewed-on: https://go-review.googlesource.com/11833 Reviewed-by: Andrew Gerrand <adg@golang.org>
+1 on this.. or add slack.com .. just like |
Sorry, going back to #129 .. I wasn't set up to do Gerrit reviews.. so I dropped it there.. but I'd prefer a Register call nonetheless. |
I'd prefer we export the list. There is less harm in exporting BrokenAuthHeaderHosts than a Register function. Registration makes broken implementations more legitimate. An exported list with an awful name that helps the programs reading who is not implementing the spec fully is a safer option. |
NEWS from the github enterprise side: https://enterprise.github.com/releases/2.3.4/notes Perhaps thanks to some pushing as a Github:E user. |
@giganteous, excellent. |
I'm still okay with a |
I appreciate you guys want this to work for the most common providers by default. I also understand that you don't want to be burdened by the mistakes of the providers. But I also feel like you are unnecessarily punishing developers that want to implement some other provider that isn't following the specs. It's not the developer that just picked a random provider to implement that didn't follow the specs. Most providers won't comply and you'll have to add it to your blacklist which is too much work IMHO. When I want to add the current provider to the list, I'd have to add 6 prefixes to the list, with possibly more to come (they use a different domain for every country). It would be great if we get a workaround. My suggestion is adding a HasBrokenAuthHeader field on the oauth2.Endpoint type. We can still keep our bad provider list to make sure the most common providers keep working. |
+1 to the Register function, will send a CL soon. |
I'm going to reopen this. I've started to work on this in https://go-review.googlesource.com/c/oauth2/+/157820 I want it to just be automatic with no configuration or required maintenance of registries of quirks. |
Perhaps add a BrokenAuthHeader to EndPoint?
The server bug behind this seems pretty common (it's also in Shopify, with domain names [shop].myshopify.com), and it seems unfortunate to need to update this library every time someone finds one. I'd bet 90% of the forks are to add another broken provider.
Let me know if you might accept a change like this and I'll work on it.
The text was updated successfully, but these errors were encountered: