Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add Twitch Extension endpoints in Helix #114

Merged
merged 2 commits into from
Aug 31, 2021
Merged

Add Twitch Extension endpoints in Helix #114

merged 2 commits into from
Aug 31, 2021

Conversation

jackmcguire1
Copy link
Contributor

@jackmcguire1 jackmcguire1 commented Jul 27, 2021 •
edited

Twitch are deprecating the Extension endpoints and migrated them to helix

This work is based off of my go module go-twitch-ext and for simplicity
I would like to migrate to Helix

FEATURES:
JWT:

  • create a EBS claim - in preperation for signed JWTs
  • sign JWT token from EBS CLAIM, to use in endpoint calls
  • verify JWT tokens, this is useful for verifying client JWTS sent from Twitch Extensions

Endpoints:

  • CreateExtensionSecret
  • GetExtensionSecrets
  • GetExtensionConfigurationSegment
  • SetExtensionConfigurationSegment
  • SetExtensionRequiredConfiguration
  • SendExtensionPubSubMessage
  • SendExtensionChatMessage

Things to note:

  • users to set JWT Bearer Token AUTH§ header via client opts getter/setter
  • uses "github.com/dgrijalva/jwt-go" go module, twitch used this in example code so I built functionality with this lib

TODO:

  • unit tests
  • continue to improve docs/extensions_docs.MD

@jackmcguire1 jackmcguire1 force-pushed the twitch-ext branch 2 times, most recently from 43e48e7 to aff0e4c Compare July 27, 2021 16:42
@coveralls
Copy link

coveralls commented Jul 27, 2021 •
edited

Pull Request Test Coverage Report for Build 1185154481

  • 141 of 201 (70.15%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-4.09%) to 92.55%
Changes Missing Coverage Covered Lines Changed/Added Lines %
extension_pubsub.go 33 35 94.29%
extensions.go 14 16 87.5%
extension_configuration.go 37 44 84.09%
helix.go 1 11 9.09%
extension_jwt.go 56 75 74.67%
extension_secrets.go 0 20 0.0%
Totals Coverage Status
Change from base Build 1179954764: -4.09%
Covered Lines: 1205
Relevant Lines: 1302
💛 - Coveralls

@nicklaw5 nicklaw5 changed the title PROPOSAL: Twitch Extension endpoints in Helix Add Twitch Extension endpoints in Helix Jul 27, 2021
nicklaw5
nicklaw5 requested changes Jul 27, 2021
View reviewed changes
extension_jwt.go Outdated
"fmt"
"time"

"github.com/dgrijalva/jwt-go"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the Go standard library have equivalent support for this package? This package hasn't been updated in years and is no longer maintained.

Additionally, I was hoping that we would not have to use any external packages in this project.

Copy link
Contributor Author

@jackmcguire1 jackmcguire1 Jul 27, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of the box it appears it does not... I see that the repo has instead be re-located to https://github.com/golang-jwt/jwt, which is maintained.

I think it'll be mega overkill to re-invent the wheel in all honesty... is this something you'll eventually budge on @nicklaw5?

I think utilizing other go modules is something you should really consider in the future of this
repo, as I separately raised the issue it would be ideal to implement OIDC AUTH with something that already exists.

Dependabot for GitHub is fantastic at pushing PRs to update go modules for you see my post on reddit here

Another noteworthy look to assure your nuances is this https://github.blog/2021-07-22-github-supply-chain-security-features-go-community/

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to budge. Give me a few days to review. It's a busy time for me atm.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @nicklaw5 i understand entirely!

This PR is still a [W]ork [I]n [P]rogress, but I will continue to add on to it over the next week or so

@jackmcguire1 jackmcguire1 force-pushed the twitch-ext branch 2 times, most recently from 098f71a to c6e74d3 Compare July 29, 2021 18:59
@nicklaw5
Copy link
Owner

@jackmcguire1 Is this ready to be reviewed? Or are you still making changes?

@jackmcguire1
Copy link
Contributor Author

@nicklaw5 maybe have a skim through if you have a chance and see what I’m trying to achieve.

it’s a still not complete, sorry I’ve had covid and bad food poisoning recently and not had the energy to pick this up.

@jackmcguire1
PROPOSAL: helix extension endpoints
5459ac2 
- JWT signing
- extension configuration segments endpoints
- extension secrets endpoints
- UPDATE extension docs
Update Docs

- remove role parameter when creating claim
Updates to extension endpoints PR

- update go mod to use https://github.com/golang-jwt/jwt
- gofmt on project
- validation check error on extension options owner id
HOTFIX: json unmarshalling

- adapt secrets structs to properly handle api response
- add missing json tag for many extension segment configurations
HOTFIX: broadcaster ID in extension configuration requests

The Twitch docs are not really clear on some parts of the requests

getExtensionConfiguration:
 - broadcasterID is an optional query parameter

setExtensionConfiguration:
- broadcasterId is an optional body parameter
- validate the segment type if broadcasterID is provided

MISC:
- update 'GetExtensionSecret' -> 'GetExtensionSecrets'
HOTFIX: pubsub requests

- fix pubsub requests to be an array
Extension Configuration

- refactor setExtension params
- refactor Extension Configuration Segment consts
- add validation to get extension configuration if broadcaster Id provided
- add unit test for getExtensionConfigurationSegment
- add unit test for. setExtensionConfigurationSegment
Remove unnecessary options

- remove options from the extension options config, as these can just be provided in parameters
Extension Tests

Add unit tests
- JWT tests
- pubsub
- extension required
- extension send chat message
Verify JWT Params test

- add test to verify params
@jackmcguire1
Copy link
Contributor Author

@nicklaw5 hi nick, i have squashed the commits and pushed some more tests, i believe this is in a state for review now

nicklaw5
nicklaw5 requested changes Aug 29, 2021
View reviewed changes
Copy link
Owner

@nicklaw5 nicklaw5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall. I've added some comments in regards to minor formatting changes I'd prefer.

Can you also update the "Contributions" section in the README.md and remove the following sentences?

All new features should rely solely on the Go standard library. No external dependencies should be included in your solutions.

docs/extensions_docs.md Outdated Show resolved Hide resolved
docs/extensions_docs.md Outdated Show resolved Hide resolved
docs/extensions_docs.md Outdated Show resolved Hide resolved
docs/extensions_docs.md Outdated Show resolved Hide resolved
docs/extensions_docs.md Outdated Show resolved Hide resolved
docs/extensions_docs.md Outdated Show resolved Hide resolved
extension_configuration.go Outdated Show resolved Hide resolved
extension_jwt.go Outdated Show resolved Hide resolved
extension_jwt.go Outdated Show resolved Hide resolved
extension_jwt.go Outdated Show resolved Hide resolved
@jackmcguire1
Copy link
Contributor Author

pushed changes @nicklaw5

@jackmcguire1
Extension improvements
79c49b9 
- address comments by nicklaw5
- create Extension Claimsfunc  to accept struct as parameter
- improve JWT unit tests
- refactor 'SendExtensionPubSubMessage' structs to be prefixed by 'Extension'
@nicklaw5
Copy link
Owner

Appreciate you making those adjustments 👍

@nicklaw5 nicklaw5 merged commit 2874167 into nicklaw5:main Aug 31, 2021
@jackmcguire1 jackmcguire1 mentioned this pull request Nov 6, 2021
Open
@jackmcguire1 jackmcguire1 deleted the twitch-ext branch September 27, 2022 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicklaw5 nicklaw5 nicklaw5 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jackmcguire1 @coveralls @nicklaw5