add basic auth url decode #346
Conversation
|
As discussed in #336 we'd like to have this try both decodings for a little bit (at least for one release) for backward compatibility issues. Can you update your patch to try the correct encoding, and if that doesn't work, try the old one? |
gerson24
force-pushed
the
336-token-endpoint-wrong-decoding
branch
2 times, most recently
from
6cf503a to
9fa38fb
Compare
|
Check it now, if you want I can move this: https://github.com/coreos/dex/blob/master/server/server.go#L413-L422 to http.go and fix the testing in both files |
| writeTokenError(w, oauth2.NewError(oauth2.ErrorServerError), state) | ||
| return | ||
| } | ||
| if !ok { |
There was a problem hiding this comment.
Please add an comment here about why we have a fallback.
In addition, maybe also something like:
// TODO: Remove support for deprecated encoding later
|
I think the decoding logic should be left in the http handler. This looks good BTW. We'd want a unit test for the handleTokenFunc, but we can add that later if you'd like. |
Now it follows oauth2 spec(http://tools.ietf.org/html/rfc6749#section-2.3.1). Fixes: dexidp#336
gerson24
force-pushed
the
336-token-endpoint-wrong-decoding
branch
from
9fa38fb to
2b818cb
Compare
|
Ok! I'm working on test file |
|
@gerson24 we're hoping to push a 0.3.0 release today and would very much like to see this in there. Would it be okay to merge as is then add tests in another pr? |
|
yep, we can do that! thanks @ericchiang |
|
For last yesterday's release we had a change of heart and pushed the original fix (see #357). Apologies for pushing that ourselves, we were on a tight schedule. Thank you for reporting the issue originally and your work in this PR. Closing this one since the fix is now in master. |
Now it follows oauth2 spec(http://tools.ietf.org/html/rfc6749#section-2.3.1).
Fixes: #336