-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
DigitalOcean Auth Provider #351
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code wise LGTM! Very cleanly done, thank you!
I think it would be good to get an ack from another maintainer before we merge however 🙂
One thing I would ask though, would you mind adding yourself to the owners file so that future PRs for modification to the provider can notify you for sanity checking and testing?
Good call, I added myself. Thanks, although I can't take full credit for the code haha :) Thanks for reviewing! |
Is there a reason to hard code so many providers that in the end should all be just oidc providers? We're bloating a security service that should be a simple proxy with a bunch of code users won't use (not speaking of this provider specifically) |
A unification of the providers was started in #299, I would like to pick this up at some point and try, eventually, to have a single minimal provider with some configuration that is predefined. You're right, these providers should all pretty much be able to be the OIDC provider with some additional config. The problem is trying to find a way to make the configuration expressive enough to be able to represent all of the options that an individual provider may or may not want to set. Ideally then, each provider that is currently built in, would just become a number of configuration options, maybe we include those in presets, maybe we just document them. (Initially likely presets for backwards compatibility) When I have some time, I plan to write up a proposal for this unification, but my time is pretty tight at the moment as I've just started a new role. |
Maybe just determining which drivers could work as is with the existing oidc provider and deprecate those. and maybe a policy for not letting new providers in unless they can't work with the existing oidc provider would help. Also maybe just accepting new patches to the oidc provider to make it more featureful as new drivers come in rather then accepting all new drivers. That would be less work then rewriting all the existing ones. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you and congrats on your first contribution! 🎉
Description
This PR adds a DigitalOcean auth provider. Users can authenticate using the email addresses associated with their accounts. Heavily "inspired" by the LinkedIn provider (i.e. a 1:1 copy), so a huge thank you to whoever wrote it! I hope that's ok. 😅
How Has This Been Tested?
Tests are included.
Checklist:
I will update the CHANGELOG once I create the PR so I can copy the #.