Login & authentication

[ihodes]

Resolves #709

This PR adds basic authentication to Cycledash, both to the API code + to the frontend views. It also includes a simple registration system (anyone can register at this point; there is no account activation). All objects are available to any logged in user (there is no ACL of any kind).

The system brcrypts the password and stores it in the user table (migration code below) along with a username and email address.

Future plans include:

• LDAP auth Integrate with LDAP for auth #772
• ACL + groups for object access Need ACL with User Groups #770
• Email verification + user auth/activation (admins, basically) Email verification + user registration authorization #771

Highlights:

• Add user object + validations
• Add login/registration/logout code
• Add authentication code
• Modify tests to automatically handle basic auth (+ other repetitive code)
• Add tests for authentication

Migration code:

CREATE TABLE users (
       id BIGSERIAL PRIMARY KEY,
       username TEXT UNIQUE NOT NULL,
       password TEXT NOT NULL,
       email TEXT UNIQUE NOT NULL
);

TODO

• Finish login/logout/registration
• Add authentication to API
• Tests
• pdiff tests (need to be logged in for tests now!)
• Test updates for existing tests
• Notion of user groups/ACL (probably push & file)

[hammer]

Have you filed an issue for integration with LDAP?

[ihodes]

Filed #772

[danvk]

Thanks for doing this! Most of my comments are small.

Now that we have accounts, we don't need the "username" field when you're filling out comments. Can you drop that, or file an issue to do so in a follow-up?

---

Reviewed 39 of 65 files at r1.
Review status: 39 of 65 files reviewed at latest revision, 23 unresolved discussions, all commit checks successful.

---

^{config.py, line 7 [r1] (raw file):}
This is kinda hard to grok. Is it the same as

if not value or value.lower() == 'false':

---

^{config.py, line 34 [r1] (raw file):}
I like that this is required to be set!

---

^{cycledash/api/init.py, line 10 [r1] (raw file):}
Does flask.ext.restful not have built-in support for authentication?

---

^{cycledash/api/init.py, line 14 [r1] (raw file):}
self.require_auth

(It's more descriptive and avoids having two names for the same thing.)

---

^{cycledash/api/init.py, line 18 [r1] (raw file):}
It might be cleaner to make this class AuthenticatedResource, ditch the require_auth property and use flask.ext.Resource when you don't care about authorization. This class is a no-op is require_auth=False.

---

^{cycledash/api/projects.py, line 13 [r1] (raw file):}
why the rename? (In general, if we want to call this tasksapi, I think we should name the file tasksapi.py.)

---

^{cycledash/auth.py, line 1 [r1] (raw file):}
docstring

---

^{cycledash/auth.py, line 40 [r1] (raw file):}
"it exists", "the password"

---

^{cycledash/auth.py, line 55 [r1] (raw file):}
Does flask-login force these to be methods? (I'd expect them to be @property)

---

^{cycledash/auth.py, line 106 [r1] (raw file):}
Why not use wrap_user or class User here?

---

^{cycledash/auth.py, line 108 [r1] (raw file):}
Here is_authenticated is True whereas in the User class is_authenticated is a method which returns True. Is the distinction intentional?

---

^{cycledash/templates/layouts/layout.html, line 5 [r1] (raw file):}
A momentous change!

---

^{cycledash/templates/login.html, line 9 [r1] (raw file):}
In the screenshot tests, the "Login" text is wrapped to the next line (website_login.png).

---

^{cycledash/templates/login.html, line 12 [r1] (raw file):}
I think "Login" = noun, "Log in" = verb. When I visit facebook.com, for example, the button to sign in is "Log In", two words.

---

^{cycledash/templates/macros/nav.html, line 22 [r1] (raw file):}
"Log Out" (c.f. Facebook)

---

^{cycledash/templates/register.html, line 27 [r1] (raw file):}
It would be helpful to indicate the password requirements somewhere on the form, e.g. "your password must be eight or more characters"

---

^{cycledash/views.py, line 16 [r1] (raw file):}
I'm not comfortable with having two variables that differ only in capitalization. Using api.api wouldn't bother me.

---

^{cycledash/views.py, line 90 [r1] (raw file):}
Can we use @login_required for Resources as well?

---

^{scripts/travis-run-pdiff-tests.sh, line 11 [r1] (raw file):}
Any reason this needs to be on the same line as python run.py?

---

^{tests/python/helpers.py, line 12 [r1] (raw file):}
This seems really similar to auth.register. Can you just use that?

---

^{tests/python/test_authentication.py, line 26 [r1] (raw file):}
rm debug code.

---

^{tests/python/test_authentication.py, line 65 [r1] (raw file):}
One downside of doing it this way (instead of eq_(401, ...) on every line) is that, if any of these return a different code, you'll have a harder time figuring out which one it was.

---

^{tests/python/test_comments_api.py, line 36 [r1] (raw file):}
Won't ResourceTest do this anyway? (Same comment on the other resource test modules.)

---

Comments from the review on Reviewable.io

[ihodes]

filed #779

---

Review status: 29 of 65 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

^{cycledash/api/init.py, line 10 [r1] (raw file):}
It does not.

---

^{cycledash/api/init.py, line 18 [r1] (raw file):}
I'd rather use the same class for all API resources. This class'll be extended to handle generating docs etc.

---

^{cycledash/auth.py, line 55 [r1] (raw file):}
Yes, the current release forces these to be methods. The upcoming release changes this to properties.

---

^{cycledash/views.py, line 90 [r1] (raw file):}
We could rewrite @login_required and use it for it, but they're doing different things right now.

---

^{scripts/travis-run-pdiff-tests.sh, line 11 [r1] (raw file):}
Nope, I'll move it out of line.

---

^{tests/python/helpers.py, line 12 [r1] (raw file):}
auth.register is specific to the flask request context, so wouldn't work here.

---

^{tests/python/test_authentication.py, line 65 [r1] (raw file):}
The debug print statement above (now removed) makes it quite easy to debug in practice (I had to use it to debug in fact).

---

^{tests/python/test_comments_api.py, line 36 [r1] (raw file):}
It would do this at the end of the quite, but not at the end of each test case (as this does)

---

Comments from the review on Reviewable.io

[danvk]

LGTM

---

Review status: 29 of 65 files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from the review on Reviewable.io

[ihodes]

Thanks for the review!