-
Notifications
You must be signed in to change notification settings - Fork 69
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
Allow some users (but not all) to not need admin approval #145
Conversation
…e in a different way)
(which is same thing in ChangeAuthorizationHandler but they are different here, because of crypto URL)
Thanks for submitting your first pull request! You are awesome! 🤗 |
…to make them pass
…ared, e.g. at DST change
…additional dependencies
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:
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) |
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. |
…imezone string parsing, which is already included in the %z
Alright, this should be it. Thanks! |
Ok so I ran all my tests and everything appears to be working fine. I pushed just:
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. |
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. Would it be possible for you to implement that as well? If that's an unreasonable request, I can do it myself after merging. |
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. |
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. |
I'm working on issue #127, not on this one. So you can continue if you like. |
…cator into domain-restrictions
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 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 |
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. |
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.
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: