openiddict / openiddict-core

Flexible and versatile OAuth 2.0/OpenID Connect stack for .NET
https://openiddict.com/
Apache License 2.0
4.47k stars 527 forks source link

Add Twitch to the supported providers #2142

Closed mbaumanndev closed 4 months ago

mbaumanndev commented 4 months ago

Hello !

I have not played much with it, tested with the Console and ASP.NET Core sandboxes, log-in and refresh token worked fine.

Is there anything else to do/check that is not in the login flow ?

kevinchalet commented 4 months ago

Salut 👋🏻

Thanks for your PR!

Is there anything else to do/check that is not in the login flow ?

Did you test the client credentials grant?

I see you added a quirk to amend the grant types to also include the device grant type. But since the device authorization endpoint is not listed in https://id.twitch.tv/oauth2/.well-known/openid-configuration, I'd assume it's not working properly. Can you please test it?

mbaumanndev commented 4 months ago

Salut 👋🏻

Salut ! 👋🏻

Thanks for your PR!

Your welcome !

Did you test the client credentials grant?

I see you added a quirk to amend the grant types to also include the device grant type. But since the device authorization endpoint is not listed in https://id.twitch.tv/oauth2/.well-known/openid-configuration, I'd assume it's not working properly. Can you please test it?

I don't think I tested it, I added it because Twitch listed this grant on their docs, I'll try to figure how to do that properly, I'll update the PR when I'm done, it may take a couple of days as I don't do .NET or OIDC on a daily basis, I do it for a side project to check what can be done with twitch APIs.

kevinchalet commented 4 months ago

I'll update the PR when I'm done, it may take a couple of days as I don't do .NET or OIDC on a daily basis, I do it for a side project to check what can be done with twitch APIs.

Haha, no worries. Let me know if you need help 😃

mbaumanndev commented 4 months ago

After checking Twitch docs, it seems that we must at some point validate the token for some use cases (https://dev.twitch.tv/docs/authentication/validate-tokens/), is there a file where I should override some behaviour to do it ?

I'm also having some difficulties with the device authorization endpoint, but I think I missied something with the scopes in the flow

kevinchalet commented 4 months ago

After checking Twitch docs, it seems that we must at some point validate the token for some use cases (https://dev.twitch.tv/docs/authentication/validate-tokens/), is there a file where I should override some behaviour to do it ?

That really looks like a stupid design to me (and I strongly doubt most implementations do that, it's an insanely inefficient process).

We could treat that endpoint as an introspection endpoint, but it's not a standard flavor so we'd need some mapping. Not sure it's really worth it. Let's just ignore it? 😄

I'm also having some difficulties with the device authorization endpoint, but I think I missied something with the scopes in the flow

If https://dev.twitch.tv/docs/authentication/getting-tokens-oauth/#device-code-grant-flow correctly reflects how they implemented things, it doesn't seem to diverge much from the standard OAuth 2.0 device authorization grant. What error are you seeing?

mbaumanndev commented 4 months ago

After checking Twitch docs, it seems that we must at some point validate the token for some use cases (https://dev.twitch.tv/docs/authentication/validate-tokens/), is there a file where I should override some behaviour to do it ?

That really looks like a stupid design to me (and I strongly doubt most implementations do that, it's an insanely inefficient process).

We could treat that endpoint as an introspection endpoint, but it's not a standard flavor so we'd need some mapping. Not sure it's really worth it. Let's just ignore it? 😄

For now we may ignore it, I'll try later to see if it's really needed for long during connections, and if that's the case I'll do a new contrib.

I'm also having some difficulties with the device authorization endpoint, but I think I missied something with the scopes in the flow

If https://dev.twitch.tv/docs/authentication/getting-tokens-oauth/#device-code-grant-flow correctly reflects how they implemented things, it doesn't seem to diverge much from the standard OAuth 2.0 device authorization grant. What error are you seeing?

I just updated the PR with the code I have for now, I'm not sure about the scope encoding and if I should force the removal of the openid scope when doing device auth.

Here is the setup I have in the program for testing:

 .AddTwitch(options =>
 {
     // For client credentials : https://github.com/Moerty/Twitch.MediatR?tab=readme-ov-file#prerequisites---twitch-credentials
     options.SetClientId("xxx")
              .SetClientSecret("xxx")
              .SetRedirectUri("callback/login/twitch")
              .AddScopes("user:read:email");
 });

And here is the error I'm getting:

An error occurred while trying to authenticate the user:
OpenIddict.Abstractions.OpenIddictExceptions+ProtocolException: An error occurred while authenticating the user.
  Error: invalid_request
  Error description: A generic 400 error was returned by the remote authorization server.
  Error URI: https://documentation.openiddict.com/errors/ID2161
  at async ValueTask<DeviceChallengeResult> OpenIddict.Client.OpenIddictClientService.ChallengeUsingDeviceAsync(
     DeviceChallengeRequest request) in \openiddict-core\src\OpenIddict.Client\
     OpenIddictClientService.cs:832
  at void System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
  at async ValueTask<DeviceChallengeResult> OpenIddict.Client.OpenIddictClientService.ChallengeUsingDeviceAsync(
     DeviceChallengeRequest request) in \openiddict-core\src\OpenIddict.Client\
     OpenIddictClientService.cs:857
  at void System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
  at void System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
  at TResult System.Threading.Tasks.ValueTask`1.get_Result()
  at async Task OpenIddict.Sandbox.Console.Client.InteractiveService.ExecuteAsync(CancellationToken stoppingToken) in
     \openiddict-core\sandbox\OpenIddict.Sandbox.Console.Client\InteractiveService.cs:
     155

I did not had this error the other day, I had another one because I didn't use the right endpoint... I'll try to take a further look on thursday or friday to check which 400 I get there.

mbaumanndev commented 4 months ago

By the way, I just checked the URL given in the error (https://documentation.openiddict.com/errors/ID2161), and it's a 404 page, I can do a contrib on the docs in the next few weeks to help fix that

kevinchalet commented 4 months ago

I did not had this error the other day, I had another one because I didn't use the right endpoint... I'll try to take a further look on thursday or friday to check which 400 I get there.

Take a look at the logs to see if there is a more useful error 😃

By the way, I just checked the URL given in the error (https://documentation.openiddict.com/errors/ID2161), and it's a 404 page, I can do a contrib on the docs in the next few weeks to help fix that

Yeah, adding a proper page for each error code is tracked by https://github.com/openiddict/openiddict-documentation/issues/30. It's a huge task, tho' 🤣

mbaumanndev commented 4 months ago

I made some progress :D

When I go with step by step debug, the flow give me a link to authenticate to twitch with the code. If I open it and enter the code while the program is stuck in debug, it works.

As soon as I disable the breakpoints, the link is displayed in the console and it fails instantly, no time to click on the link to validate the code, even if the code expires in five minutes.

I'll try to debug more, but if you encountered this issue already, I'm interested for some guidance

kevinchalet commented 4 months ago

I'll try to debug more, but if you encountered this issue already, I'm interested for some guidance

Looks like their authorization_pending error response is not standard, so OpenIddict doesn't even retry to send the token request until the demand is approved or expires:

image

(the parameter must be named error, not message)

If you want to support Twitch's non-standard flow, you'll need to tweak this handler to map the non-standard parameter to its standard equivalent:

https://github.com/openiddict/openiddict-core/blob/164e55afeb3b72e0aa18febdbef342dcf2760651/src/OpenIddict.Client.WebIntegration/OpenIddictClientWebIntegrationHandlers.Exchange.cs#L364-L455

mbaumanndev commented 4 months ago

I'll try to debug more, but if you encountered this issue already, I'm interested for some guidance

Looks like their authorization_pending error response is not standard, so OpenIddict doesn't even retry to send the token request until the demand is approved or expires:

I just found this too before seeing your comment, thanks for your response, I'll check the mappings. I begin to figure out how it's working, but not knowing the standard make it hard, but that's fun 😄

mbaumanndev commented 4 months ago

I can't figure why, even with a breakpoint in MapNonStandardResponseParameters, I don't enter its HandleAsync method, I kept falling in ValidateHttpResponse own HandleAsync first.

I'll stop for today, I'll give a try tomorrow to check why it happens.

kevinchalet commented 4 months ago

I can't figure why, even with a breakpoint in MapNonStandardResponseParameters, I don't enter its HandleAsync method, I kept falling in ValidateHttpResponse own HandleAsync first.

Ah crap, do they return a non-200 response code?! (so the status code would be returned as a proper HTTP status AND as a custom status parameter? 🤣)

If so, we'll need a new MapNonStandardError handler that executes before ValidateHttpResponse (ValidateHttpResponse<ExtractTokenResponseContext>.Descriptor.Order - 500) 😅

mbaumanndev commented 4 months ago

I can't figure why, even with a breakpoint in MapNonStandardResponseParameters, I don't enter its HandleAsync method, I kept falling in ValidateHttpResponse own HandleAsync first.

Ah crap, do they return a non-200 response code?! (so the status code would be returned as a proper HTTP status AND as a custom status parameter? 🤣)

If so, we'll need a new MapNonStandardError handler that executes before ValidateHttpResponse (ValidateHttpResponse<ExtractTokenResponseContext>.Descriptor.Order - 500) 😅

Oh, it make sense, I'll check tomorrow but I'm pretty sure they send a 400, as it's catched as a bad request by the handler

kevinchalet commented 4 months ago

Thanks! 👍🏻

mbaumanndev commented 4 months ago

I began to check for today, I don't know where to put the new handler : should it belong to WebIntergration as it's specific to Twitch ? If so, how do I check that I have a 400 at that level.

Or should I put it in SystemNetHttp to catch the 400 ?, If so, how should I make it specific as I don't have access to the provider types here ?

I'll try to dig more into the problem in the meantime

kevinchalet commented 4 months ago

Thanks for asking 😃

I began to check for today, I don't know where to put the new handler : should it belong to WebIntergration as it's specific to Twitch ?

The rule is that any provider-specific quirk must live in the OpenIddict.Client.WebIntegration package (that itself depends on the OpenIddict.Client.SystemNetHttp package). Only fully standard things are allowed in the OpenIddict.Client or OpenIddict.Client.SystemNetHttp packages 😃

If so, how do I check that I have a 400 at that level.

You can access the HttpResponseMessage using context.Transaction.GetHttpResponseMessage(). Here's an example in the same project:

https://github.com/openiddict/openiddict-core/blob/164e55afeb3b72e0aa18febdbef342dcf2760651/src/OpenIddict.Client.WebIntegration/OpenIddictClientWebIntegrationHandlers.Userinfo.cs#L291-L354

kevinchalet commented 4 months ago

Let me know if you still need help (or we can just pretend Twitch doesn't support the device authorization flow and make our lives easier 😄)

mbaumanndev commented 4 months ago

I did not had the time to check longer this week-end, I may have some time today or tomorrow :)

mbaumanndev commented 4 months ago

@kevinchalet Sorry for the delay, I had some plans that changed a few things in my life recently, I'm not sure I will have some time to code as a hobby or to contribute more in the next few months (if so, I'll be happy to contribute, I really want to do this device code flow ^^)

For now we can do as you suggested : drop the device flow. I tested with the console, everything works fine.

I'll push my wip on my fork if anyone want to take a look at it, but I'll try to open a PR for the device flow before the end of october :)

kevinchalet commented 4 months ago

No problem, @mbaumanndev. Hope you're doing well.

Let's do that 👍🏻

mbaumanndev commented 4 months ago

No problem, @mbaumanndev. Hope you're doing well.

I'm doing well, thanks ! I'll just be a little more busy than expected with work 😅

kevinchalet commented 4 months ago

Merged. Merci beaucoup pour ta contribution ! 😃