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

Allow some users (but not all) to not need admin approval #145

Merged
merged 72 commits into from
Sep 29, 2021

Conversation

davidedelvento
Copy link

This PR is for a use case very important for https://www.hourofci.org/ and to a lesser degree for http://www.gisandbox.org/ too:

Allow users who have access to an email address matching an admin-provided regular expression to get their account without waiting for admin approval (and overwhelming that person with hundreds of requests). Typical use cases: all students in a class, university or large organization.

This is achieved in three step.

  1. at setup time, by forcing email on signup (and doing various other initialization)
  2. at signup time, by matching the user-provided email with a specified regular expression and sending a cryptographically-signed URL to the that email address
  3. at authorization time, only if the user have access to that email, they can access that URL, click on it and authorize themselves, removing the need for admin intervention for each user. The URL has a 15-minutes expiration time.

If the users registers themselves with a non-matching email, the same behavior of nativeauthenticator without this PR is maintained even when this setting is in use.

If an attacker, without access to an email matching the allowed regex, tries to forge the URL to authorize themselves, the cryptographic signature will prevent the authorization from happening: https://github.com/davidedelvento/nativeauthenticator/blob/domain-restrictions/nativeauthenticator/handlers.py#L133

Caveats:

  • at the moment there is no way for a user to extend an expired URL or to request another one, other than registering a different account with the same email (which is allowed as it was before)
  • this does not solve issue NativeAuthenticator doesn't work with LocalProcessSpawner #105 (see comment https://github.com/davidedelvento/nativeauthenticator/blob/domain-restrictions/nativeauthenticator/handlers.py#L146 ) however I will probably write a separate PR for that
  • there are potential attack surface, particularly scripting repeated sign-ups, overwhelming the SMTP server (however I will implement a separate PR for reCaptcha)
  • of course attackers with deep access to the network could register themselves with a fake email matching the allowed ones, snoop the network traffic, intercept the email (which is in clear) and authorize themselves. FWIW, all major providers do not seem to worry about this issue with their "lost password? we'll send you an email" services, so probably this is not a large attack surface, but please be aware of it.

@welcome
Copy link

welcome bot commented Apr 26, 2021

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@davidedelvento
Copy link
Author

Ok, sorry for pushing to the branch before it was ready (I needed it to get some github functionalities to help me). It's almost there though:

  • Rebased on current upstream's main
  • Django dependency removed
  • Support at least as old as Python v3.6
  • Only a small issue with parsing date in ISO format remains (timezone for newer version of python, non existence of the parsing in python v3.6) -- I will fix this soon

Regarding the documentation, I probably did not explain clearly that the thing I was asking is a setting specific to gmail only, i.e. of users of Google. I thought of adding it because that is what the vast majority of users are doing and it is not widely documented and I wanted to help them and avoid bug reports (since it will appear as an error for nativeauthenticator)

@lambdaTotoro
Copy link
Collaborator

a setting specific to gmail only, i.e. of users of Google. I thought of adding it because that is what the vast majority of users are doing and it is not widely documented and I wanted to help them and avoid bug reports

I see. Yeah, in general, that's fine, as long as the feature also allows (and documentation exists for) other email providers. We do not want the feature to be gmail only (I, for example, would use my university's service). That was necessary for the reCAPTCHA branch, because Google is the dominating provider, but for email, there should be alternatives as well.

@davidedelvento
Copy link
Author

Alright, this should be it.
I wanted to run many more (manual) tests in preliminary installations similar to the production one, but I ran out of time for today.
I pushed it anyway so you can start dissecting it too (and hopefully not find any flaws like you did with my other PR ;-) )
I will try to do my additional tests by the end of the week and keep you posted.

Thanks!

@davidedelvento
Copy link
Author

Ok so I ran all my tests and everything appears to be working fine. I pushed just:

  • better documentation (could improved further, but it's decent, IMHO)
  • a minor change to have more sensible text (to the tone of "check your email") when somebody creates an account with email registration from an authorized domain, rather than from a "random" domain, for which the text remains the current "info sent to the admin"

Also a note about the SMTP server: if it refuses connection, the attempted account creation fails with error 500. I guess I could have made the account creation proceed as if the user specified an unauthorized domain, but to me that's a moot point since admin intervention is needed in either case, and therefore I did not bother.

Last, once the email address is validated, a "professional" solution should include other features such as password reset and various other "self-serve" things, but so far we have found that account creation eats most of the time, and therefore we spent the time to create this solution, and leave the other things for another day -- users are contacting us by email when needing other things.

@lambdaTotoro
Copy link
Collaborator

Thanks for your work and your input. I'm currently on the road so I won't have a chance to test it before Sunday night or Monday.

On the topic of account creation throwing a 500 if the server refuses connection, I'm not super happy with that. Yes, admin intervention is needed in either case, but I don't think it's a moot point.
A clear error message (maybe something like "Self-authorization email could not be sent. Please contact your admin about this.") can inform the user of what went wrong and guide their actions instead of just crashing, which is likely to confuse/frustrate.

Would it be possible for you to implement that as well? If that's an unreasonable request, I can do it myself after merging.

@davidedelvento
Copy link
Author

@lambdaTotoro

By "moot point" I meant trying to workaround the SMTP server error and instead proceed the registration "as if" the user provided an email not matching the regular expression (domain restriction, in the way I am thinking about it). Arguably that is a moot point because it does not produce an immediately usable account, but it would provide a clean, if incomplete, experience to the user and that may be something that some people might prefer.

If I understand what you are asking, it is still a sort of "500 error", just with a UI that makes it not look like a server crash and which provides more information to the user. I think that is better than the plain crash, and I like your short text about it. I would have struggled to keep it short and making it clear that the problem is on the sending email server, i.e. JupyterHub's one and not on the receiving one, i.e. the user's one. Perhaps that could be even further made clearer by saying "Please contact Jupyterhub's admin about this" rather than "your" admin.

So I think this request is not unreasonable, but I have to educate myself on how to report this sort of things in JupyterHub, which I have not done before. Not sure how long that'd take, but I'll take a stab at it as soon as I get some time. If you already know everything that needs to be done, feel free to do it yourself and I'll use your changes to learn what to do the next time.

Safe travels.

@davidedelvento
Copy link
Author

I implemented a crude try-except-reraise to make the condition more meaningful. I see from the conflicts that you must be working on this too, so I'll refrain from further actions until I hear otherwise from you.
Thanks!

@lambdaTotoro
Copy link
Collaborator

I'm working on issue #127, not on this one. So you can continue if you like.

@davidedelvento
Copy link
Author

Thanks for letting me know. Since you were not working on it, I solved the conflicts, as usual not really conflicts just unawareness by git of separate python things going on on the same line which could have been simply concatenated. I also changed the syntax of the defaults with the new tag. IMHO (and flake8 and pytest agree) this is now good to go.

Just to save you time and making sure we are on the same page, in case of email server not being available (or not accepting the connection), that is still an error, but a 503 rather than a 500, and it provides a message to the user. I think it's still a bit crude (probably a customization of tornado's HTTPError class would have been better), but I think doing that work just for this PR only is overkill, especially since all of JupyterHub uses "naked" HTTPErrors instead

@lambdaTotoro
Copy link
Collaborator

I gave this a test today and it worked like a charm. The documentation is sufficiently clear also and I think the error behaviour on missed SMTP connections is also sensible. Happy to merge the pull request.
Thank you for your hard work, @davidedelvento, you put a lot into this. I'll give you a special shoutout in the 1.0 release notes.

@lambdaTotoro lambdaTotoro merged commit 46215f0 into jupyterhub:master Sep 29, 2021
@lambdaTotoro lambdaTotoro added this to the Version 1.0 milestone Sep 30, 2021
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