Support scope deserialization for non-compliant implementations

[udoprog]

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.

• Topic raised on their dev forum (but unfortunately ignored)
• Another one

[udoprog]

This is the error I'm seeing on Windows for the tests:
lipanski/mockito#41

[ramosbugs]

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?

[ramosbugs]

This is the error I'm seeing on Windows for the tests:
lipanski/mockito#41

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

[ramosbugs]

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?

Oh, and let's add unit test coverage for the new deserialization format.

[udoprog]

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?

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?

[udoprog]

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?

Oh, and let's add unit test coverage for the new deserialization format.

I'm working on it. I'll add tests here once we've got a good approach and #53 has been merged ;).

[ramosbugs]

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?

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?

Good point. I guess we can make TokenResponse a trait (without ExtraTokenFields) and make it one of Client's type parameters. That'll let callers do whatever they want to customize it. And then maybe the current struct can be renamed to StandardTokenResponse<EF: ExtraTokenFields, TT: TokenType>.

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.

[udoprog]

@ramosbugs I was experimenting a bit and found adding another type parameter and trait (ScopeField) to work pretty OK. I've pushed that over the change right now. What do you think?

[ramosbugs]

@ramosbugs I was experimenting a bit and found adding another type parameter and trait (ScopeField) to work pretty OK. I've pushed that over the change right now. What do you think?

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 TokenResponse struct with a trait, we'll have a future-proof solution that will avoid the need for future breaking changes.

[udoprog]

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

[ramosbugs]

Ok thanks for taking a stab at it! I'll see if I can get that change done this weekend.

[ramosbugs]

I pushed 7c371e9, which should satisfy this use case, I think. You'll probably want to implement your own version of StandardTokenResponse with the customizations needed to support Twitch. Hope that helps!