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

Implements a Discord provider #528

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

Conversation

loganintech
Copy link

@loganintech loganintech commented Apr 5, 2023

As was discussed in #312, Discord doesn't fulfill the proper spec for unique identification. They return a User object with Username, Discriminator, and Id. Discord guarantees a user's Username#Discriminator is unique across the app, so I've used that as the unique ID to be checked. This allows the whiteList array to have a list of usernames in the format that Discord users are used to, as opposed to their snowflake id.

I also had to bump the docker images go build versions because a dependency requires go1.19 but the images were on go1.18.


An update to Discord's username specifications brings this more inline with other providers and removes discriminators. Now the username field is unique for each user provided they've taken the steps to select one.

@loganintech
Copy link
Author

Discord has just announced their intent to remove discriminators, so I'm going to adjust to use the ID until their API is updated to reflect the new constraint of unique usernames. Alternatively, regardless of what they do with usernames, IDs will remain unique.

https://support.discord.com/hc/en-us/articles/12620128861463

@loganintech loganintech force-pushed the loganintech/discord-provider branch from 139b62a to d6c22f9 Compare May 3, 2023 20:13
@inklesspen
Copy link

Users can change their usernames (as frequently as twice a week), so the only reliable identifier is the 'id' field (which is a Twitter-style "snowflake")

@loganintech
Copy link
Author

loganintech commented Mar 11, 2024

@inklesspen That's true, and we could use that, but it's important to note that now that usernames are unique they're an equally reliable source of truth in terms of unique identification. It seems easier to me to write a list of usernames the way you would a list of emails instead of a list of 18 digit numbers. In the FAQ you listed it says

Usernames are unique to each user and no two users can share the same username

Nobody could impersonate you and improperly gain auth unless you change your username, at which point you'd also immediately lose auth.

Also I believe at least three other providers use Username or Email when given and vouch compares the User struct's Username prop for verification it makes more sense to me to use the unique usernames.

@inklesspen
Copy link

Nobody could impersonate you and improperly gain auth unless you change your username, at which point you'd also immediately lose auth.

Yes, and at which point an unauthorized person could gain auth. That's my concern here.

@loganintech loganintech force-pushed the loganintech/discord-provider branch from ce7fb2f to 7dc4825 Compare March 24, 2024 20:37
@loganintech loganintech force-pushed the loganintech/discord-provider branch from 4759ead to 6bacd9e Compare March 24, 2024 20:42
@loganintech
Copy link
Author

I understand your concern. It's the same vulnerability in the other providers that behave the same way. I've updated the provider to be configurable such that if you set discord_use_ids to true the provider will check the IDs instead of the Username.

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