search

buzzfeed / sso

sso, aka S.S.Octopus, aka octoboi, is a single sign-on solution for securing internal services
MIT License
3.07k stars 187 forks source link

sso_auth: remove email validators #252

Open Jusshersmith opened 4 years ago

Jusshersmith commented 4 years ago

Problem

As a follow up to https://github.com/buzzfeed/sso/pull/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 commented 4 years ago

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

Jusshersmith commented 4 years ago

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)