-
Notifications
You must be signed in to change notification settings - Fork 189
sso-auth: add provider for individual e-mail address authentication #113
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
Conversation
|
Thank you for opening @tahoward! We will review in the next few days and get back to you with any feedback we may have. |
benjsto
left a comment
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.
This is great - I tested it and it worked flawlessly. I just left a couple of minor naming comments. My only other feedback is that might be nice to also update the quickstart/docker-compose.yml to have this value in addition to the EMAIL_DOMAIN value.
internal/auth/options.go
Outdated
| Port int `envconfig:"PORT" default:"4180"` | ||
|
|
||
| EmailDomains []string `envconfig:"SSO_EMAIL_DOMAIN"` | ||
| EmailAddresses []string `envconfig:"SSO_EMAIL_ADDRESS"` |
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.
I get that this is following the existing pattern with SSO_EMAIL_DOMAIN and EMAIL_DOMAIN but is there any case where these values would be different? Maybe it should just be EMAIL_ADDRESS for both?
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.
They both effectively serve the same purpose. We could combine them, however, we'd need a new parameter input for users to specify the intended validator to be used. Leaving them separated for now serves backwards comparability for those already using EMAIL_DOMAIN based validator.
|
Thanks for the review. I'll make some time this weekend to implement the suggestions. |
|
@tahoward Any update? |
|
Any chance of getting this one in soon? |
|
Did anyone push a version of the |
|
Thanks for updating @tahoward ! |
|
thanks for merging this! I tried Shoulnt there be an |
|
@xeor I agree with your analysis yep, I think you are good for opening a new issue now this has been merged, too bad this wasn't detected earlier |
|
Weird that noone saw that :p Oh well... @tahoward do you want to do this one as well? My |
|
@xeor @victornoel : thanks for bringing this up (and creating a separate issue for it). I think it's perfectly valid to propose defining |
|
@benjsto yep, sure, I think this should anyway be tackled in another PR since it's a bit different. Actually I wonder why there isn't also an |
Problem
Addresses: #32
Solution
Add new parameters:
AUTH:
SSO_EMAIL_ADDRESSESPROXY:
EMAIL_ADDRESSESIf parameters provided they override
SSO_EMAIL_DOMAINandEMAIL_DOMAINrespectively.New parameters use a validater to check full e-mail address.
Notes
Example deployment: