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

sso_auth: remove email validators #252

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Jusshersmith
Copy link
Contributor

@Jusshersmith Jusshersmith commented Sep 9, 2019

Problem

As a follow up to #247, this removes some redundant logic from the sso authenticator, particularly surrounding the AUTHORIZE_EMAIL_DOMAINS and AUTHORIZE_EMAIL_ADDRESSES configuration variables.

Solution

AUTHORIZE_EMAIL_ADDRESSES was only used for email validation, which is already done in the proxy so this has been removed.

AUTHORIZE_EMAIL_DOMAINS was used in two places: email validation (which has also been removed) and population of the sign in page.

Rather than needing to pass these domains in as configuration variables, (or reducing the usefulness of the sign in page) sso proxy adds the allowed domains to the SignInPage URL as a query parameter, which is then parsed by sso authenticator for use within the sign in page

@jphines
Copy link
Contributor

jphines commented Sep 9, 2019

Overall, I agree with the changeset. However, there are some open questions if you could investigate and respond to appropriately:

  • How does this (if at all) weaken our security posture?
  • If a user mistakenly authenticates with the wrong domain, what is the user flow re-authenticate with the correctly email domain?
  • Does this mean we will require the proxy to always pass in allowed email domains, even if it's a wildcard *?

@Jusshersmith
Copy link
Contributor Author

Jusshersmith commented Oct 1, 2019

If a user mistakenly authenticates with the wrong domain, what is the user flow re-authenticate with the correctly email domain?

I think we could definitely improve the UX of this situation (and others) -- so far I've found this to trigger only static error pages which would then require a user to manually sign out using the /sign_out endpoint. I can add in some extra logic in to make this a little bit easier for users.

Does this mean we will require the proxy to always pass in allowed email domains, even if it's a wildcard *?

I think this depends if we want to maintain the usefulness of the current error page -- at the moment it tells users which domain and provider they can log in with, which I think is fairly useful information. Alternatively, we could try to move some logic surrounding this to the Proxy. (I've had an initial look at possibilities here, but have a few other possible avenues to look into)

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