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
Support scope deserialization for non-compliant implementations #52
Conversation
This is the error I'm seeing on Windows for the tests: |
I definitely feel like this is a bug in Twitch and not this crate, but I'm sympathetic to users who want this crate to work despite Twitch's RFC non-compliance. Since this format isn't standards compliant, I would prefer not to enable this parsing behavior by default, but I think it would be fine to make it a compile-time feature flag. Would that work? |
Thanks for getting to the bottom of it! I don't have a Windows machine to test a fix on, but I would accept patches to fix the tests on Windows and add a Windows build to Travis CI (if supported). |
Oh, and let's add unit test coverage for the new deserialization format. |
This would be awkward since it would make it hard to use two different oauth2 flows in the same application. Some kind of generics would probably be preferable? |
I'm working on it. I'll add tests here once we've got a good approach and #53 has been merged ;). |
Good point. I guess we can make This approach will require callers who want to use non-standards-compliant APIs to do a bit more work, but it'll avoid putting any non-standards-compliant code directly into this crate, at least. |
@ramosbugs I was experimenting a bit and found adding another type parameter and trait ( |
Thanks for working on this! I think that approach addresses this specific incompatibility, but I worry that there will be future PRs for other incompatibilities and that we'll end up with a series of one-off fixes. Also, I think adding new type parameters is a breaking change, so this option won't be available once 2.0.0 is out. If we replace the entire |
@ramosbugs I'm not entirely convinced that is the best way forward right now. I've tried to implement the change and it requires a significant rewrite of most of the library. It's rather involved to get rid of the extra type parameters because of the way the TokenResponse API is designed right now. I'm probably not suited to do that right now since it's such a big change and there will probably be a number of design decision back and forth. It would be better if you or someone more involved with the project at least set the groundwork for it. If I find the time I might take another stab at it in a while. But right now I don't have it. |
Ok thanks for taking a stab at it! I'll see if I can get that change done this weekend. |
This allows clients to support non-standards-compliant OAuth2 providers. For discussion, see #52.
I pushed 7c371e9, which should satisfy this use case, I think. You'll probably want to implement your own version of |
I encountered this when trying to implement an authentication flow against Twitch.
You can see that their documentation specifies the correct scope format (i.e. space-separated string), but in the example it responds with an array of strings. I've verified that this is also the case with the actual API.
I don't know if I'm doing something wrong, nor how proliferate these kind of implementations are. But from a pragmatic perspective it would be nice to be able to use
oauth2-rs
to get tokens from Twitch.