Skip to content

plugins.twitch: better access token error handling #5011

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

Conversation

bastimeyer
Copy link
Member

@bastimeyer bastimeyer commented Dec 3, 2022

  • Refactor TwitchAPI class:
    • Set CLIENT_ID as a class attribute (for test references)
    • Allow call() to pass custom headers (currently unused)
    • Remove unneeded json.dumps()
  • Handle PlaybackAccessToken error response and log error messages
  • Add Options.clear() for resetting plugin options state after tests
  • Add access token API tests

Resolves #5009

This adds error logging to the access token processing and also adds tests for the access token responses and how the --twitch-api-header and --twitch-access-token-param influence the access token request.


Small side-note in regards to the added Options.clear() method:

I've already mentioned somewhere in the Plugin.bind() removal in 5.0.0 (#4744 / #4768) that Plugin.options will also need to get fixed at some point. Similar to the previously bound class attributes, Plugin.options is a class attribute that gets shared accross all plugin classes unless it gets overridden by streamlink_cli.main.setup_plugin_args(). This means that when streamlink_cli is not involved in plugin construction, e.g. in tests or when using the Streamlink session API directly, setting any kind of plugin option always requires clearing the options, which is bad. I'm pretty sure that there are a couple of tests which are polluting the shared plugin options dict. I'll check once I get the time.

The reason why I didn't touch Plugin.options at the time is that removing Plugin.bind() was already a lot of work. In hindsight, this should've been done in one go, because changing it will probably be a breaking API change, because of how the plugins will need to get initialized. Currently, the options are set by the session instance before plugins are initialized, and it expects the options dict to be available on the plugin class. Options should however only be set on plugin instances, so there's no shared options dict, neither between plugin classes, nor between different instances of the same plugin.

I will need to have a closer look at this and figure out a better solution before making any further comments. This PR should be fine though.

@bastimeyer bastimeyer added the plugin enhancement A new feature for a working Plugin label Dec 3, 2022
Copy link
Member

@gravyboat gravyboat 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 to me.

@@ -421,18 +423,30 @@ def access_token(self, is_live, channel_or_vod):
validate.union_get("signature", "value"),
)

return self.call(query, schema=validate.Schema(
{"data": validate.any(
return self.call(query, acceptable_status=(200, 401), schema=validate.Schema(
Copy link
Member

Choose a reason for hiding this comment

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

Are these the only status codes that are ever returned from a "successful" request?

Copy link
Member Author

Choose a reason for hiding this comment

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

400 and 403 could maybe be added, but apart from that, I don't think anything else will be useful when trying to parse the {"error": str, ...} response schema. The remaining 4xx errors or any 5xx errors will be handled by the HTTPSession where it'll raise a PluginError instead.

Copy link
Member

@gravyboat gravyboat Dec 4, 2022

Choose a reason for hiding this comment

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

Sounds good to me. Figured it was worth double checking just in case so there's no issue when this goes live that will require a second release. 👍

- Refactor `TwitchAPI` class:
  - Set `CLIENT_ID` as a class attribute (for test references)
  - Allow `call()` to pass custom headers (currently unused)
  - Remove unneeded `json.dumps()`
- Handle `PlaybackAccessToken` error response and log error messages
- Add `Options.clear()` for resetting plugin options state after tests
- Add access token API tests
@bastimeyer bastimeyer force-pushed the plugins/twitch/access-token-error-handling branch from d41d557 to 8e21e43 Compare December 4, 2022 00:20
@back-to back-to merged commit 3151175 into streamlink:master Dec 4, 2022
@bastimeyer bastimeyer deleted the plugins/twitch/access-token-error-handling branch December 4, 2022 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin enhancement A new feature for a working Plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

plugins.twitch: Show error message when the provided OAuth token is not valid
3 participants