Merge lp://staging/~frankban/charms/precise/juju-gui/server-auth into lp://staging/~juju-gui/charms/precise/juju-gui/trunk

Reviewers: mp+176738_code.launchpad.net,

Message:
Please take a look.

Description:
GUI server authentication.

Added authentication management to the Tornado server.
The WebSocket handler includes a way to know if the
connected client is logged in to the API server.
Both Go and Python API implementations are supported.

Also implemented WebSocket messages validation.

I am sorry: this diff is quite long.
In my defence, the code is full of tests and
documentation... Well, sorry again.

Tests: `make unittest`

https://code.launchpad.net/~frankban/charms/precise/juju-gui/server-auth/+merge/176738

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/11725044/

Affected files:
   A [revision details]
   M revision
   M server/guiserver/apps.py
   A server/guiserver/auth.py
   M server/guiserver/handlers.py
   M server/guiserver/manage.py
   M server/guiserver/tests/helpers.py
   A server/guiserver/tests/test_auth.py
   M server/guiserver/tests/test_handlers.py
   M server/guiserver/tests/test_manage.py
   M server/guiserver/tests/test_utils.py
   M server/guiserver/utils.py
   M server/runserver.py

My prelim look over the code was nice. make unittests fails with various
modules apparently missing. What is missing seems to vary from run to
run. I've missed shelltoolbox (from charmtools helpers import) but on
other runs apt, yaml. sometimes they load in.

Have you seen this? I didn't spend too long investigating but when I
hear back from you I can try it again.

https://codereview.appspot.com/11725044/diff/1/server/guiserver/tests/test_handlers.py
File server/guiserver/tests/test_handlers.py (left):

https://codereview.appspot.com/11725044/diff/1/server/guiserver/tests/test_handlers.py#oldcode82
server/guiserver/tests/test_handlers.py:82:
Abstract.

https://codereview.appspot.com/11725044/

On 2013/07/24 21:33:11, benjamin.saller wrote:
> My prelim look over the code was nice. make unittests fails with
various modules
> apparently missing. What is missing seems to vary from run to run.
I've missed
> shelltoolbox (from charmtools helpers import) but on other runs apt,
yaml.
> sometimes they load in.

> Have you seen this? I didn't spend too long investigating but when I
hear back
> from you I can try it again.

No I don't. It seems the virtualenv for tests is not being created
correctly.
Could you please try the following, from the branch root?
make clean # delete the venv
make # network errors?
make unittest

If the problem is still there, could you please paste the output of
`./tests/.venv/bin/pip freeze`? Thanks.

https://codereview.appspot.com/11725044/

Please take a look.

https://codereview.appspot.com/11725044/diff/1/server/guiserver/tests/test_manage.py
File server/guiserver/tests/test_manage.py (right):

https://codereview.appspot.com/11725044/diff/1/server/guiserver/tests/test_manage.py#newcode116
server/guiserver/tests/test_manage.py:116: with
mock.patch('guiserver.manage.options', {'arg1': None}):
On 2013/07/25 13:20:59, benji wrote:
> I think this is the only test method that you didn't include a comment
for. :)

Done.

https://codereview.appspot.com/11725044/diff/1/server/guiserver/tests/test_utils.py
File server/guiserver/tests/test_utils.py (right):

https://codereview.appspot.com/11725044/diff/1/server/guiserver/tests/test_utils.py#newcode60
server/guiserver/tests/test_utils.py:60: # if the resulting object is
not a dict-like object, a warning is
On 2013/07/25 13:20:59, benji wrote:
> There is an extra space at the beginning of the comment and "if"
should be
> capitalized.

Done.

https://codereview.appspot.com/11725044/

LGTM, impressive work indeed. One trivial below. Also, consider only
saying "log in" and "logged in" when used as a verb, but "login" when a
noun, i.e. "login request".

https://codereview.appspot.com/11725044/diff/9001/server/guiserver/manage.py
File server/guiserver/manage.py (right):

https://codereview.appspot.com/11725044/diff/9001/server/guiserver/manage.py#newcode86
server/guiserver/manage.py:86: 'of the bootstrap/state node as returned
by juju status.')
Double quotes around "juju status", maybe?

https://codereview.appspot.com/11725044/

Thank you both for the reviews!

> LGTM, impressive work indeed. One trivial below. Also, consider only
saying "log
> in" and "logged in" when used as a verb, but "login" when a noun, i.e.
"login
> request".

Done.

https://codereview.appspot.com/11725044/diff/9001/server/guiserver/manage.py#newcode86
> server/guiserver/manage.py:86: 'of the bootstrap/state node as
returned by juju
> status.')
> Double quotes around "juju status", maybe?

Done.

https://codereview.appspot.com/11725044/

Please take a look.

https://codereview.appspot.com/11725044/

*** Submitted:

GUI server authentication.

Added authentication management to the Tornado server.
The WebSocket handler includes a way to know if the
connected client is logged in to the API server.
Both Go and Python API implementations are supported.

Also implemented WebSocket messages validation.

I am sorry: this diff is quite long.
In my defence, the code is full of tests and
documentation... Well, sorry again.

Tests: `make unittest`

R=benjamin.saller, benji, teknico
CC=
https://codereview.appspot.com/11725044

https://codereview.appspot.com/11725044/