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.

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

Add twitch login support #153

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Add twitch login support #153

wants to merge 13 commits into from

Conversation

Stigstille
Copy link

I feel like this is really jank, but it works. Lmk if there is something I should change to make this fit better into the codebase

@RedFlames
Copy link
Collaborator

Since the TwitchOAuth function is virtually identical to the DiscordOAuth endpoint, could things be refactored into one generic function that then allows configuring arbitrary OAuth2 providers via server config files?

Not 100% sure how it'd work yet, I'm guessing there'd be only one RCEP left, something like /api/oauth and then it gets a parameter to select a provider?...

Somehow in the FrontendSettings there'd need to be a list of structs or dicts something, I don't quite recall how complex things can be that get serialized to YAML.
And then each provider would have an identifer, its client id+secret, auth URL, redirect URL, token URL, user info API URL, avatar API URL... the latter two might need to be a sort of template string like we already have for constructing greeting/kick/ban messages and in the chat module (I think 🤔)
(For example discord avatar URLs are kind of annoying since they need two specific bits from the user data... and for Twitch there's the issue that it gives an array with one element)

I don't think the obsolete "Discrim" field in our user data should be used to tell these apart, because there's the somewhat theoretical but possible chance that a Twitch User ID and a Discord UID would cause a collision... and they're used as database keys. So they'd maybe need to all be prefixed/suffixed with the provider name... not sure if that's all that elegant though or what could fail from that.

We also really need to start using the state parameter of the OAuth flow, and for my suggestion of having a shared /api/oauth endpoint we could then perhaps use state to remember which provider that user had triggered. (Currently idk if there are any other problems with this idea)

Then the only remaining question will be which providers besides Discord we'll be enabling on the official server. We could enable Twitch, maybe Steam, Reddit, ... but probably we don't actually want that many of 'em. Unsure.


As for your current implementation, reviewing it may be moot if it should be implemented as a refactoring of the existing discord auth... but I did notice some issues just in case.

  • you have some LogLevel.CRI logging at the start of both OAuth endpoints now that should be LogLevel.VVV or LogLevel.DBG
  • response_type=code appears twice in TwitchOAuthURL
  • when a redirect comes back with an error parameter like access_denied I suppose it would be good to attempt to log error_description and some info? Discord seemingly doesn't provide description, but Twitch would
  • maybe if there's no display_name, fall back to login, just as for Discord it tries global_name and then username... seems like these JSON keys would also need to be configurable.

@Stigstille
Copy link
Author

@RedFlames

Not 100% sure how it'd work yet, I'm guessing there'd be only one RCEP left, something like /api/oauth and then it gets a parameter to select a provider?...

How would you tell the backend what OAuth to use?

Twitch User ID and a Discord UID would cause a collision...

(Both of these are mine) Twitch user ID is short (e.g. 735413483) and Discord ID is longer (e.g. 1073248324024537208), so I doubt that will be a problem

Then the only remaining question will be which providers besides Discord we'll be enabling on the official server. We could enable Twitch, maybe Steam, Reddit, ... but probably we don't actually want that many of 'em. Unsure.

This is up to the CelesteNet contributors and admins

maybe if there's no display_name, fall back to login, just as for Discord it tries global_name and then username... seems like these JSON keys would also need to be configurable.

I checked on an empty, brand new account, and display_name was still there


you have some LogLevel.CRI logging at the start of both OAuth endpoints now that should be LogLevel.VVV or LogLevel.DBG
response_type=code appears twice in TwitchOAuthURL

when a redirect comes back with an error parameter like access_denied I suppose it would be good to attempt to log error_description and some info? Discord seemingly doesn't provide description, but Twitch would

Give me a little bit to fix these. Should be a quick fix if I dont get distracted

@Stigstille
Copy link
Author

@RedFlames Fixed all of your original concerns afaik (apart from what other oauths to use. that is still up to CelesteNet admins). Please re-check this PR

@RedFlames
Copy link
Collaborator

I'm sorry to say this, but I don't see this rewrite as an improvement...

Like, how would one add another oauth2 provider to this? Implement 5 new hard-coded settings properties and extend all the if-statements and ternary operators in... at least 10 places throughout the RCEndpoint?
That's completely unworkable.

You did motivate me to mock up a draft of how I imagined more generic OAuth2 support to look.

#162

:laugheline: / :snip_embarrassed:

@Stigstille
Copy link
Author

Wasn't exactly sure how to set it up, so fair. I might try and do this later once the new OAuth system is added in? If you have any tips for any future PR (either here or elsewhere) please let me know as I am trying to improve :>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants