Provide CSV file with names and passwords #2
Conversation
Also, adjust imports so that they actually work. The password-generation script is renamed to disambiguate it from the module. Additionally, __all__ should be a list of strings, not objects.
| import requests | ||
| from traitlets.config import PyFileConfigLoader | ||
|
|
||
| from tornado.ioloop import IOLoop |
There was a problem hiding this comment.
I hope this is listed in the dependencies somewhere?
There was a problem hiding this comment.
Not explicitly, for two reasons:
- Tornado is a dependency of JupyterHub, so if you've got JH installed, you'll be okay here.
- I feel like this service is a little bonus, but the meat of this project is the authenticator itself. If you just want to use the authenticator, you shouldn't have to install the things to make the service work. I don't know how to represent this in the setup.py file, so I haven't put anything additional there.
| get_password = generate_password_function() | ||
|
|
||
|
|
||
| class HashAuthHandler(HubOAuthenticated, RequestHandler): |
There was a problem hiding this comment.
Is it worth using flask instead? I'm thinking about keeping our development compatible with our main website. Don't know if it's worth rewriting this though ...
There was a problem hiding this comment.
If you look back in the git history, you'll see I started with a Flask version. I think tornado is right for this particular project, though. Since we've released this as an open-source project, we should make it as easy to install as possible. If they have JupyterHub, they have Tornado, so we might as well just use that. JupyterHub provides some authentication helpers for tornado, so this code is actually more compact than the Flask version.
Also, I don't see this expanding past a single endpoint. The grader, on the other hand, I've done in Flask and I see no reason to move it to tornado.
scripts/hashauthpw
Outdated
|
|
||
| if __name__ == '__main__': | ||
| if len(sys.argv) not in [3,4]: | ||
| print('Usage: hashauthenticator secret_key user [len]') | ||
| if len(sys.argv) not in [2,3,4]: |
There was a problem hiding this comment.
Use argparse which makes this much more clear.
Signature changed slightly: now the length is indicated with a flag.
README.md
Outdated
| @@ -30,7 +30,7 @@ This package comes with a command called `hashauthpw`. Example usage: | |||
|
|
|||
| ```bash | |||
| $ hashauthpw | |||
| Usage: hashauthenticator user [secret_key [len]] | |||
| usage: hashauthpw [-h] [--length LENGTH] username [secret_key] | |||
There was a problem hiding this comment.
You can also just say hashauthpw -h for more as this future proofs against future changes.
@tianhuil, I don't know if you want to do code reviews here or not. If you don't feel like it, I'll just merge it in now and have Zach and Chris look over this code carefully when I start to bring them up to speed on JH.
The main thrust of this code is to provide a JupyterHub service that reports (to admin users only) a CSV file full of names and passwords. This will make getting set up for day 1 easier -- the teacher doesn't have to know the secret_key or even have downloaded this package. They can just go to /services/hashauth/ and get everything they need.
Around that, there's a fair amount of cleanup I ended up doing, mainly related to getting the install working and improving the command line script. I don't think those things particularly need review.