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 OAuth support #1964

Closed
wants to merge 11 commits into from
Closed

Add OAuth support #1964

wants to merge 11 commits into from

Conversation

jer3m01
Copy link

@jer3m01 jer3m01 commented Apr 22, 2020

OAuth Sign In Support

Allows users to connect using external services through OAuth.

Out of the box this will support Discord, Google, GitHub, Twitter,
GitLab, BitBucket, LinkedIn and Facebook.

This is a follow-up of #1488.

Still in progress.

@FoksVHox

This comment has been minimized.

@jer3m01
Copy link
Author

jer3m01 commented Apr 27, 2020

Preview of the admin settings page.

Admin Settings Page

@matthewpi
Copy link
Member

You might want to check the config/pterodactyl.php, you are using env('APP_OAUTH_DISCORD_REDIRECT_URI') as the redirect for all of the providers, if this is intended (for all of them to redirect to a single place) then it should probably be renamed to APP_OAUTH_REDIRECT_URI.

@FoksVHox
Copy link
Contributor

I feel like the admin page for the OAuths is a bit doll or old looking. Maybe I could look good if every OAuth had its own box?
image
I know that there isn't that many OAuth out of the box, as there is boxes on the picture.

@jer3m01
Copy link
Author

jer3m01 commented Apr 28, 2020

You might want to check the config/pterodactyl.php, you are using env('APP_OAUTH_DISCORD_REDIRECT_URI') as the redirect for all of the providers, if this is intended (for all of them to redirect to a single place) then it should probably be renamed to APP_OAUTH_REDIRECT_URI.

The route has been hardcoded, that was only used as a placeholder. controller route

I feel like the admin page for the OAuths is a bit doll or old looking. Maybe I could look good if every OAuth had its own box?
image
I know that there isn't that many OAuth out of the box, as there is boxes on the picture.

Going to leave it as it is since the admin UI will get recoded in React.
But you would still only view this page one time when you set it up
so I don't really see a need for a fancy interface.

@jer3m01
Copy link
Author

jer3m01 commented Apr 28, 2020

Preview of the login page.

Login page

@ocanty
Copy link

ocanty commented May 3, 2020

Would it be possible to have a 'custom' oauth driver? i.e one that supports generic providers rather than the social media ones outlined above

Letting it interface with stuff like Keycloak comes to mind, would be v. helpful
I would be willing to test this for you if possible

@jer3m01
Copy link
Author

jer3m01 commented May 19, 2020

Would it be possible to have a 'custom' oauth driver? i.e one that supports generic providers rather than the social media ones outlined above

Letting it interface with stuff like Keycloak comes to mind, would be v. helpful
I would be willing to test this for you if possible

Of course, custom drivers are planned.
This relies on Laravel Socialite & Socialite Providers.

More specifically a socialite provider for Keycloak already exists.
In depth configuration will be provided later on in the pterodactyl docs.

@jer3m01
Copy link
Author

jer3m01 commented May 20, 2020

Preview of the account OAuth linking page.

OAuth linking page

@DaneEveritt
Copy link
Member

I think Google & Discord are plenty, perhaps Github/Gitlab as well. Linkedin/Facebook/BitBucket/Twitter are not going to be very frequent. I'm a fan of the oauth support, but not a huge fan of having every provider under the sun be shipped.

Especially because of the inevitable flood of "why isn't this working" that will come with this feature.

@schrej
Copy link
Member

schrej commented May 20, 2020

Imo there should be a "custom" option as well, then people can just add the providers they need, e.g. their own.

@FoksVHox
Copy link
Contributor

Imo there should be a "custom" option as well, then people can just add the providers they need, e.g. their own.

I agree, I’m currently working on a SSO for a hosting service, who uses Pterodactyl as the their game panel, if this comes before I’m done with the SSO it would be very helpful since I don’t need to work as hard as I otherwise should.

@jer3m01
Copy link
Author

jer3m01 commented May 20, 2020

I think Google & Discord are plenty, perhaps Github/Gitlab as well. Linkedin/Facebook/BitBucket/Twitter are not going to be very frequent. I'm a fan of the oauth support, but not a huge fan of having every provider under the sun be shipped.

Especially because of the inevitable flood of "why isn't this working" that will come with this feature.

I get what you mean but except discord those ship with Laravel Socialite listed here.

Imo there should be a "custom" option as well, then people can just add the providers they need, e.g. their own.

I agree, I’m currently working on a SSO for a hosting service, who uses Pterodactyl as the their game panel, if this comes before I’m done with the SSO it would be very helpful since I don’t need to work as hard as I otherwise should.

Sure I'll provide a generic driver with endpoint options in the config.

@FoksVHox
Copy link
Contributor

Any news on this @jer3m01?

@jer3m01
Copy link
Author

jer3m01 commented Jul 14, 2020

Has been a while, I'll get back on this.

@jer3m01
Copy link
Author

jer3m01 commented Jul 14, 2020

Preview of the driver creation
OAuth settings creation modal
OAuth settings

@FoksVHox
Copy link
Contributor

FoksVHox commented Jul 14, 2020

Please capitalize the name of the drivers, it triggering my OCD, but else does it look very sick!

@jer3m01
Copy link
Author

jer3m01 commented Jul 14, 2020

Please capitalize the name of the drivers, it triggering my OCD, but else does it look very sick!

Thanks!
The name of the drivers are case sensitive, can't capitalize them.

@FoksVHox
Copy link
Contributor

Thanks!
The name of the drivers are case sensitive, can't capitalize them.

Just do {{ Str::ucfirst($driver) }}?

@jer3m01
Copy link
Author

jer3m01 commented Jul 14, 2020

Just do {{ Str::ucfirst($driver) }}?

Yes it is possible but I would rather see the name as is rather than have the
hassle of my driver (user added) not working because when added it's name
wasn't correctly capitalized.

@jer3m01
Copy link
Author

jer3m01 commented Jul 14, 2020

Preview of the generic driver.
OAuth settings
Generic Driver options
Located at pterodactyl.auth.oauth.generic

@jer3m01 jer3m01 marked this pull request as ready for review July 14, 2020 22:36
Copy link

@Bencor29 Bencor29 left a comment

Choose a reason for hiding this comment

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

Hi,

I don't understand why you try to get the user from a discord URL for the generic driver.
Is it an error ?

https://github.com/pterodactyl/panel/pull/1964/files#diff-f4e0aea31a5a10cfbbdd5963a0435fa3R65-R77

@jer3m01
Copy link
Author

jer3m01 commented Aug 3, 2020

I don't understand why you try to get the user from a discord URL for the generic driver.
Is it an error ?

Yes that is a mistake, thanks for the catch.

fadedmax
fadedmax previously approved these changes Aug 27, 2020
Copy link

@fadedmax fadedmax left a comment

Choose a reason for hiding this comment

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

not a official reviewer, but the code looks good to me :)

@ocanty
Copy link

ocanty commented Sep 17, 2020

@jer3m01 would it be possible to add ENV variables that allow you to change the generic driver URIs without the need to modify the PHP file?

image

@Wunderharke
Copy link
Contributor

How is it going with this feature? Could this be merged before the release of v1.0.0?

@DaneEveritt
Copy link
Member

It will not be merged before 1.0 is released, we're under a feature freeze until that time.

@Torbikini
Copy link

This looks pretty good, nice work. Only thing I’m curious about is do people have to still create a normal pterodactyl account and then associate a social provider or are you just checking by matching email addresses? (Matching by email addresses is fine if and only if you know that the OAuth provider has verified the email. I know some differentiate between an email and verified email.)

I’m curious to see how this is going to play out and major kudos to you for taking on such a daunting task! I can’t wait to get OAuth support added to my panel down the road!

@nicolas-janzen
Copy link

I would appreciate merging to

felixklauke
felixklauke previously approved these changes Mar 31, 2021
nicolas-janzen
nicolas-janzen previously approved these changes Mar 31, 2021
FlorianGH
FlorianGH previously approved these changes Mar 31, 2021
c-sakel
c-sakel previously approved these changes Mar 31, 2021
@pterodactyl pterodactyl locked as spam and limited conversation to collaborators Mar 31, 2021
@DaneEveritt
Copy link
Member

To everyone constantly spamming comments on here and giving your "approved" reviews: please understand it is incredibly annoying and spams all of us with notifications. Your reviews do not make me go "yeah I'm just going to click merge" on this.

Your spam has made me even less likely to be motivated enough to look at this because it is constantly low-value contributions and discussions. As a result we've locked this discussion.

@schrej
Copy link
Member

schrej commented Nov 30, 2021

Closing this since the fork got removed. See #3774 for an updated PR.

@jer3m01 thanks for opening this PR and sorry that we didn't find the time to review and merge this. Sometimes it's difficult. I hope that the new one will get merged instead, which still contains your work, so it's hopefully not lost.

@schrej schrej closed this Nov 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet