Support usage of public clients when doing DCF auth flow - #101
Support usage of public clients when doing DCF auth flow#101soulchainer wants to merge 3 commits into
Conversation
Release v1.4.0: Promote test to main
- Add an `AuthClientType` and constants to differentiate the two types of OAuth clients allowed: confidential and public. - Add `ClientType` field in `AuthConfig` to determinate the type of client in use. - Update `RefreshToken` and `PollDeviceToken` to allow to refresh an access token received when finishing a "Device Code Flow" (DCF) for authentication using a public client. - Change and add new tests to cover these changes. - Update `auth.md` documentation to cover these changes. - Update CHANGELOG to reflect these changes.
| type AuthConfig struct { | ||
| ClientID string | ||
| ClientSecret string | ||
| ClientType AuthClientType |
There was a problem hiding this comment.
As ClientType defaults to ConfidentialClient, with falsy value 0, this means this change really doesn't really affect any existing code using kappopher, so people using it with confidential clients (looks like everyone till now) won't need to change it's code at all.
| config := AuthConfig{ | ||
| ClientID: "test-client-id", | ||
| ClientSecret: "test-secret", | ||
| ClientType: ConfidentialClient, |
There was a problem hiding this comment.
Conceptually this line is non necessary (because ConfidentialClient is the falsy/default value, so it would be set anyways to its correct value), I only added it to effectively check that nothing «breaks» because of it (even when, obviosly, shouldn't happen at all).
| } | ||
| } | ||
|
|
||
| func TestAuthClient_PollDeviceToken_PublicClient(t *testing.T) { |
There was a problem hiding this comment.
With tests I was really a bit lost, about what to test and what not.
TestAuthClient_PollDeviceToken already test most of it and, conceptually, the client it's almost the same. So here I only tested the part that should differ, which is where it tests something related with the client secret. I could have dupped all the other code of TestAuthClient_PollDeviceToken here, but I considered it would be just that: duplicity, not needed.
The only thing I could think of test here is that the public client in fact doesn't have a client secret set and that polling the device token doesn't break.
| } | ||
|
|
||
| if c.config.ClientSecret != "" { | ||
| if c.config.ClientType == ConfidentialClient && c.config.ClientSecret != "" { |
There was a problem hiding this comment.
We only need to set the ClientSecret if we are using a confidential client, it doesn't makes sense to take this value into account if we're using a public client, as public clients doesn't have a client secret.
| "refresh_token": {refreshToken}, | ||
| } | ||
|
|
||
| if c.config.ClientType == PublicClient { |
There was a problem hiding this comment.
Here I could have instead edited the url.Values, removing directly "client_secret" in line 417 and then only adding it if c.config.ClientType == ConfidentialClient, but, considering that probably (it seems so, at least, due to the full implementation of this) confidential clients are a bit more common, I thought the best would be to treat that as the default case and simply remove the value later if we're in fact dealing with a PublicClient.
| } | ||
| } | ||
|
|
||
| func TestAuthClient_RefreshToken_PublicClient(t *testing.T) { |
There was a problem hiding this comment.
Same than the previous comment for this one.
So, given the way TestAuthClient_RefreshToken is using public official Twitch API endpoints directly, the only thing I thought I could check here is that, in fact, this code won't error saying the client secret is missing (because it shouldn't be taken into account for a public client). The way TestAuthClient_RefreshToken deals with this, for a public client this would error with a invalid clienterror (due to refreshToken and clientID being just fake strings, non valid real values, so the API would reject it, but I can't handle such error if it doesn't comes directly from this library or another one exporting a error value that can be checked/compared. So, instead of importing some error from Twitch itself (not possible), just checking this doesn't error with the error we don't expect it to error.
1 participant
Description
Support usage of public clients when doing "Device Code Flow" (DCF) auth flow.
Current implementation doesn't allow refreshing tokens coming from a DCF with a public client. Those clients doesn't have a
ClientSecret, neither need one (check official documentation here and here too).Related Issues
Fixes #100
Type of Change
Changes Made
AuthClientTypeand constants to differentiate the two types of OAuth clients allowed: confidential and public.ClientTypefield inAuthConfigto determinate the type of client in use.RefreshTokenandPollDeviceTokento allow to refresh an access token received when finishing a DCF for authentication using a public client.Changelog
API Changes (if applicable)
Endpoint:
GET/POST/etc helix/...New Structs:
// Include any new or modified structsTesting
Test commands run:
go test ./... go build ./...Documentation
docs/folderChecklist
go fmton my codego vetand addressed any issuesScreenshots (if applicable)
Additional Notes
This change is needed because doesn't having this is basically leaving out a whole kind of apps. Several desktop apps, cli apps, any device app which need to rely on the device code flow can't really depend on kappopher how it's right now, because refreshing a token it's not possible without this being fixed. So you would need to make the whole device code flow every single time you open your app or, depending of the kind of app and user, even before than that (access tokens for device code flow expire in 4 hours).
Please, be easy on me: I'm still learning Go and this is my first contribution to a Go project.
Note: I know the fix works, I'm already using it, but about the tests I «wrote»... There was a looooooot of code there, and basically mimicked related tests and changed them a bit, this is also my first approach to tests in go, and it's kinda... different. I really didn't know what to «test» exactly, more with tests that, in the original source, rely even directly in the Twitch official API endpoint (instead of mocks), due to the nature of such specific tests. Most (not all) of the tests I wrote here really felt redundant/not necessary.