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

Implement SSO via OIDC #127

Draft
wants to merge 15 commits into
base: next
Choose a base branch
from
Draft

Conversation

evan-goode
Copy link
Member

Remaining work is hinted by some TODO comments. I'll make a checklist here too once I get a little farther. Login still unimplemented (only registration works). I'd like to get this in before releasing 3.0.0 due to the API and config schema changes.

I decided to go with https://github.com/zitadel/oidc instead of https://github.com/coreos/go-oidc since the former can implement an OIDC IDP ("server") as well as an RP ("client"), in case we want/need to become an IDP in the future.

For #39

@evan-goode
Copy link
Member Author

evan-goode commented Jan 11, 2025

  • Login
  • Registration as new player
  • Registration from existing player
  • Actually verify ID tokens in CreateUser
  • Support invites
  • Link existing account
    • By username/password
    • By authenticating with another OIDC provider?
    • Without signing in to the account, on the OIDC complete-registration page, to support forcing password -> OIDC migration
  • Unlink OIDC provider from account
  • API
    • Allow creating OIDC-provided accounts
    • Get OIDC info about a user
  • Gracefully deprecate old config options
    • RegistrationNewPlayer.AllowchoosingUUID
    • RegistrationExistingPlayer.Nickname
    • RegistrationExistingPlayer.AccountURL
    • RegistrationExistingPlayer.SessionURL
    • RegistrationExistingPlayer.SetSkinURL
    • RegistrationExistingPlayer.RequireSkinVerification
  • Users with any OIDC providers should not be able to log in with a password
  • Config option to allow/disallow password login
  • Preserve destination parameter during login using state cookie
  • PKCE support (probably a bool config option for now)
  • Encrypt ID token cookie?
  • UI changes under /web/user
  • "Minecraft passwords"
  • Docs
  • Tests

@lbrooney
Copy link

Apologies if this is out of scope or would be better asked in the original issue, but is there planned support for custom claims and using the retrieved info?

To use Twitch as an example, they have a custom claim preferred_username, which as the name implies provides the username. It would be nice to use that as the default username for a registering user (maybe with option to allow or disallow changing?). There are other claims like email_verified and email which would be helpful with user management/admin though personally that is a lower priority.

I understand if this request broadens this PR too much as it sounds like it might require a plugin type system to provide customized support, but you're likely the better judge for the requirements and how feasible this is. I can move this request to its own issue if that's preferred.

Very excited for what this PR enables!

@evan-goode
Copy link
Member Author

Apologies if this is out of scope or would be better asked in the original issue, but is there planned support for custom claims and using the retrieved info?

Happy to discuss it here! Hadn't thought about custom claims yet.

To use Twitch as an example, they have a custom claim preferred_username, which as the name implies provides the username. It would be nice to use that as the default username for a registering user (maybe with option to allow or disallow changing?). There are other claims like email_verified and email which would be helpful with user management/admin though personally that is a lower priority.

preferred_username is an OIDC standard claim and currently this PR does suggest the user's preferred_username as the player name on the complete-registration page, but the user can choose a different player name (IMO we cannot force the player name to always be the preferred_username since, to quote the OIDC spec, The RP MUST NOT rely upon this value being unique). The user's email is used as their Drasl username.

It might be good to have a drasl_admin claim for automatically making certain users admins. Or deriving their preferred language (#76) from their locale. Are you asking about these sorts of things that would apply to all Drasl instances, or are you thinking more about customizations that administrators could set up for their specific instance?

An example of the latter might be "every user with this OIDC claim should automatically get assigned a solid green cape when they create their account". For that kind of thing, I think that would be best done by a separate service that interacts with both the OIDC identity provider and the Drasl HTTP API. For that particular use case, we'd want to add an API route for getting a user by their username and maybe webhooks for triggering actions when a user creates an account/updates their account/etc. Drasl's HTTP API is my preference for building plugins/extensions. A true "plugin system" with e.g. https://github.com/traefik/yaegi would also be an option but Drasl's codebase isn't the cleanest and it would be a lot of work to maintain a stable plugin API.

@lbrooney
Copy link

I should have been more familiar with the OIDC spec, I didn't realize how many of these claims were actually a part of the standard😅.

In regards to preferred_username, I specifically meant using that value only on first signup, afterwards disregard it and only refer to playernames managed by drasl. While I acknowledge that there is no guarantee that the username will be unique, for the services that do provide some guarantee like Twitch (although they do allow usernames changes every few months and someone else can take the old username), personally I think it would a nice option to toggle (per provider) whether a user is allowed to change their player name at signup given a valid value in preferred_username. In the case, they want a name change they can request it to an admin. An example being a new server using one provider as it would be easy for players to identify another across services.

When I mentioned a plugin type system, I was thinking about authorization and how to handle that. For example if a certain value returned by a claim isn't satisfactory, for example email_verified is false, we block sign up for that user or alternatively sign up is allowed, but the user is blocked from joining the server. They're provided some indication for how they can satisfy the requirements.

Seperately I was thinking about webhooks and how they would be cool for handling events and external interactions. I don't know if webhooks would be appropriate pattern for the usecase I outlined above however.

Adds the ability to link one or more OIDC identity providers on the user
page. I would also like to support the flow from logged out -> sign in
via OIDC -> link existing account or create new? -> link existing
account. That way, we can add a AllowPasswordLogin setting that forces
users to migrate to OIDC login and doesn't allow them to log in via
password, only link their account.
@evan-goode
Copy link
Member Author

I think it would a nice option to toggle (per provider) whether a user is allowed to change their player name at signup given a valid value in preferred_username.

OK I could do that, should be easy enough to implement and maintain. I assume that would be used with AllowChangingPlayerName = false but that setting is sort of broken on the next branch currently. Since #107, a user can simply delete their player and create a new one with a different name, which will have a new UUID so the player technically didn't change names, but still... I wonder what would be an elegant way to patch that hole.

When I mentioned a plugin type system, I was thinking about authorization and how to handle that. For example if a certain value returned by a claim isn't satisfactory, for example email_verified is false, we block sign up for that user or alternatively sign up is allowed, but the user is blocked from joining the server. They're provided some indication for how they can satisfy the requirements.

We could add a Drasl config option [[OIDCProvider]].RequiredClaims that requires users of that IDP to have all the listed claims to register a Drasl account. This is a common pattern in OAuth2 (see https://kanidm.github.io/kanidm/stable/integrations/oauth2/examples.html). But this would fail the "helpful error message" requirement: Your <provider> account is missing one or more of the required claims: "email_verified". Not very friendly.

Seperately I was thinking about webhooks and how they would be cool for handling events and external interactions. I don't know if webhooks would be appropriate pattern for the usecase I outlined above however.

I could see webhooks being appropriate here. For example, a "CreateUser" webhook could respond 200 to tell Drasl to go ahead and create the user, or 400 and an JSON error message to deny the action. Internationalization might get tricky though.

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