Administrative accounts

[stefano-maggiolo]

This was longer than expected... but I believe that the code is better organized now.

The end structure is comprised of:

• a cookie-based authentication middleware that can be passed to WebService; this translate authentication cookies into authentication headers for the underlying WSGI apps, and viceversa (the app can say when it wants a cookie to be set, deleted, or refreshed);
• the tornado app is similar to CWS, just it doesn't do any cookie-based authentication; the templates change based on the capabilities of the current user;
• the rpc middleware has a new parameter, a function that does authorization based on the cookie.

The capability _jobs that Giovanni suggested is missing, but it is simple enough to add it now (just add the field, change the reevaluation template to enable the buttons in that case, and add the rpc methods to the whitelist).

As for the commits, they are as self-contained as possible. Feel free to review them one at a time, but I think you'd like to see the complete result as well.

One thing that is missing is the possibility of revoking a cookie-based authentication (that in CWS happens when you change the password), because I wanted to avoid DB access in the authentication middleware. The admins can be disabled, or deleted and recreated, so I think that's not a big problem.

[wil93]

Why these leading spaces?

[stefano-maggiolo]

Fixed and rebased immediately, thanks. If you want to review, I'll appreciate.

[landscape-bot]

Repository health increased by 0.01% when pulling 8475009 on stefano-maggiolo:awslogin into 63d9b42 on cms-dev:master.

• 10 new problems were found (including 0 errors and 9 code smells).
• 3 problems were fixed (including 0 errors and 2 code smells).

[wil93]

I think this needs to be rebased because of #449.

---

Reviewed 2 of 2 files at r1, 1 of 1 files at r2, 6 of 6 files at r3, 3 of 3 files at r4, 6 of 6 files at r5, 19 of 19 files at r6, 2 of 2 files at r7, 4 of 4 files at r8, 15 of 15 files at r9, 4 of 4 files at r10.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

^{cms/server/admin/handlers/admin.py, line 132 [r7] (raw file):}
This TODO can be deleted now?

---

^{cms/server/admin/handlers/main.py, line 61 [r6] (raw file):}
"Nonexistent" maybe? Unexisting is less common. (not very important)

---

^{cmscontrib/AddAdmin.py, line 81 [r2] (raw file):}
You probably want to add "cmsAddAdmin=cmscontrib.AddAdmin:main" to the entry_points list in setup.py.

By the way, if it will eventually be assumed that CMS is installed, then all these ifs can be deleted (because setuptools creates runnable scripts that import the module and call the right method, so the condition is always false).

---

Comments from the review on Reviewable.io

[giomasce]

Reviewed 2 of 2 files at r1, 1 of 1 files at r2, 6 of 6 files at r3, 3 of 3 files at r4.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

cms/db/admin.py, line 38 [r1] (raw file):
Small typo.

---

cms/server/admin/templates/admins.html, line 48 [r3] (raw file):
Probably we should hide this snippet until we actually have that feature!

---

requirements.txt, line 14 [r4] (raw file):
This must be also written in the documentation

---

Comments from the review on Reviewable.io

[giomasce]

Reviewed 5 of 6 files at r5.
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

cms/io/web_service.py, line 73 [r5] (raw file):
Can we erase all headers matching X-Cms-*? I find it more future-proof with respect to security.

---

cms/server/admin/server.py, line 130 [r5] (raw file):
This constant is already set in web_service.py. Can we deduplicate it?

---

Comments from the review on Reviewable.io

[giomasce]

Reviewed 1 of 6 files at r5.
Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

cms/server/admin/server.py, line 111 [r5] (raw file):
It would be nice to have specific tests for security-related features, but I do not have time to write them.

---

cms/server/admin/server.py, line 181 [r5] (raw file):
What does httponly do? I could not find any reference.

---

Comments from the review on Reviewable.io

[giomasce]

Reviewed 11 of 19 files at r6.
Review status: all files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.

---

cms/server/admin/handlers/base.py, line 143 [r6] (raw file):
Is there a reason for not reusing the same constants when the decorator is invoked? Using a constant instead of an in-line string seems more robust to me.

---

cms/server/admin/handlers/base.py, line 220 [r6] (raw file):
admin_id should already be an int (it is converted in AWSAuthmiddleware._authenticate().

---

cms/server/admin/handlers/base.py, line 220 [r6] (raw file):
Shouldn't we filter out disabled users here? Otherwise, where does that happen?

---

Comments from the review on Reviewable.io

[giomasce]

Review status: all files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.

---

cms/server/admin/handlers/base.py, line 220 [r6] (raw file):
Ok, I found where it happens (in the login handler). Still, I would expect that disabling a user has immediate effect, so the same check should happen also here.

---

Comments from the review on Reviewable.io

[giomasce]

Reviewed 3 of 19 files at r6.
Review status: all files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.

---

cms/server/admin/handlers/submission.py, line 91 [r6] (raw file):
Shouldn't be "messaging" here?

---

Comments from the review on Reviewable.io

[stefano-maggiolo]

I'm going to push once solving all the comments, then rebase and push again. Will comment here when finished.

---

Review status: 31 of 51 files reviewed at latest revision, 13 unresolved discussions, some commit checks failed.

---

cms/db/admin.py, line 38 [r1] (raw file):
Done.

---

cms/io/web_service.py, line 73 [r5] (raw file):
It doesn't look like with the fixers used here: http://werkzeug.pocoo.org/docs/0.11/contrib/fixers/#werkzeug.contrib.fixers.HeaderRewriterFix

I'll add a TODO.

---

cms/server/admin/handlers/admin.py, line 132 [r7] (raw file):
Done.

---

cms/server/admin/handlers/base.py, line 143 [r6] (raw file):
Done.

---

cms/server/admin/handlers/base.py, line 220 [r6] (raw file):
Done.

---

cms/server/admin/handlers/base.py, line 220 [r6] (raw file):
Good catch, thanks.

---

cms/server/admin/handlers/main.py, line 61 [r6] (raw file):
Done.

---

cms/server/admin/handlers/submission.py, line 91 [r6] (raw file):
Comment is not meant to be contestant-visible

---

cms/server/admin/server.py, line 130 [r5] (raw file):
It was actually a leftover...

---

cms/server/admin/server.py, line 181 [r5] (raw file):
That the browser should enforce that JS cannot access the cookie.

---

cms/server/admin/templates/admins.html, line 48 [r3] (raw file):
Right.

---

cmscontrib/AddAdmin.py, line 81 [r2] (raw file):
Not going to be assumed, but thanks

---

requirements.txt, line 14 [r4] (raw file):
Done.

---

Comments from the review on Reviewable.io

[stefano-maggiolo]

Done. Reviewable merged both pushes to the same revision, but luckily the diff is nice enough (r10 -> r11). As expected, the rebase introduced changes almost only in the tests. Feel free to check this last revision.

---

Review status: 12 of 52 files reviewed at latest revision, 13 unresolved discussions.

---

Comments from the review on Reviewable.io

[giomasce]

Would it be possibile to have an option (i.e., in cms.conf) to disable AWS login altogether? In many use cases it was simply not needed and having to manage another set of username and password is annoying.

---

Reviewed 5 of 19 files at r6, 2 of 2 files at r7, 4 of 4 files at r8.
Review status: 12 of 52 files reviewed at latest revision, 12 unresolved discussions.

---

cms/server/admin/templates/base.html, line 69 [r6] (raw file):
When can this actually happen? Is it in order to support the possibility to use AWS without login or does it do anything else?

---

Comments from the review on Reviewable.io

[giomasce]

Reviewed 15 of 15 files at r9, 4 of 4 files at r10.
Review status: 12 of 52 files reviewed at latest revision, 11 unresolved discussions.

---

Comments from the review on Reviewable.io

[giomasce]

Reviewed 21 of 42 files at r11.
Review status: 33 of 52 files reviewed at latest revision, 6 unresolved discussions.

---

Comments from the review on Reviewable.io

[giomasce]

Reviewed 21 of 42 files at r11.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

Comments from the review on Reviewable.io

[stefano-maggiolo]

I agree (maybe as a command line switch for aws, rather than as a configuration option, I'd like to hear your preference too). But in a different PR :)

---

Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

cms/server/admin/templates/base.html, line 69 [r6] (raw file):
I think nothing uses it, but maybe it's still useful to prevent errors if we forget to redirect to the login page

---

Comments from the review on Reviewable.io

[stefano-maggiolo]

This one I just pushed is probably the last update, just a couple of cosmetic changes.

---

Review status: 49 of 52 files reviewed at latest revision, 4 unresolved discussions.

---

Comments from the review on Reviewable.io

[giomasce]

Ok. I think everything's good, you can push.

I personally would prefer a configuration option, because usually either you want AWS admins or you don't. It is not something you may want to change often between two runs of the same installation.

Another thing that has to be fixed is the user documentation, particularly with respect to the part where we say that AWS has to be protected because it has no login.

---

Reviewed 3 of 3 files at r12.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

Comments from the review on Reviewable.io

[giomasce]

Also documentation can go in another PR. Please open issues to remind us of missing docs and login disabling option.

---

Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

Comments from the review on Reviewable.io

[stefano-maggiolo]

Merged